fix: surface clear error and non-zero exit on non-JSON and 2xx-error API responses

ektasawant · ektasawant · commit 54b833e71a41 · 2026-07-09T08:40:49.000+02:00
diff --git a/main.go b/main.go
@@ -1228,14 +1228,15 @@ func registerAuthCommands(configDir string) {
 			if os.Getenv("DCI_API_KEY") != "" {
 				return fmt.Errorf("login is not needed when DCI_API_KEY is set")
 			}
-			// Trigger the OAuth flow by calling a lightweight endpoint.
-			// Suppress the validate response body — login only needs the OAuth
-			// side effect (token cached), not the API output.
+			// Trigger the OAuth flow via validate. This call is internal (login
+			// only needs the cached token), so isolate all its observable effects
+			// — stdout, guard stderr, guard exit-code flag — from the login result.
 			os.Args = []string{os.Args[0], "dci", "validate"}
-			oldOut := cli.Stdout
-			cli.Stdout = io.Discard
+			oldOut, oldErr := cli.Stdout, cli.Stderr
+			cli.Stdout, cli.Stderr = io.Discard, io.Discard
 			err := cli.Run()
-			cli.Stdout = oldOut
+			cli.Stdout, cli.Stderr = oldOut, oldErr
+			nonJSONErrorResponse = false
 
 			// Auto-configure DoiT employees who have no customer context set.
 			// The validate endpoint requires customerContext for @doit.com accounts,
@@ -1443,12 +1444,12 @@ func (g dciResponseGuard) Format(resp cli.Response) error {
 		return nil
 	}
 	if msg, ok := jsonApplicationError(resp); ok {
-		// Print the structured error body as usual, then flag a non-zero exit.
 		nonJSONErrorResponse = true
 		if err := g.next.Format(resp); err != nil {
 			return err
 		}
-		fmt.Fprintf(os.Stderr, "Error: the DoiT API returned an application error: %s\n", msg)
+		// cli.Stderr (not os.Stderr) so callers like login can suppress it; don't revert.
+		fmt.Fprintf(cli.Stderr, "Error: the DoiT API returned an application error: %s\n", msg)
 		return nil
 	}
 	return g.next.Format(resp)
@@ -1480,17 +1481,30 @@ func isHTMLErrorPage(resp cli.Response) bool {
 }
 
 func bodyLooksLikeHTML(s string) bool {
-	s = strings.ToLower(strings.TrimSpace(s))
+	s = strings.TrimSpace(strings.TrimPrefix(s, "\ufeff")) // strip UTF-8 BOM some proxies prepend
+	if len(s) > 16 {
+		s = s[:16] // only the prefix matters; avoid lowercasing the whole body
+	}
+	s = strings.ToLower(s)
 	return strings.HasPrefix(s, "<!doctype html") ||
 		strings.HasPrefix(s, "<html") ||
 		strings.HasPrefix(s, "<head") ||
-		strings.HasPrefix(s, "<body")
+		strings.HasPrefix(s, "<body") ||
+		strings.HasPrefix(s, "<?xml") ||
+		strings.HasPrefix(s, "<!--")
 }
 
 // jsonApplicationError reports whether a 2xx response carries a non-empty
 // top-level JSON `error` field (e.g. an AVA askSync failure under a locked 2xx
 // status), returning a human-readable message. Such a body is not a valid DCI
 // success shape, so it is treated as a failure.
+//
+// The object-only, top-level-only scoping is load-bearing, verified against the
+// OpenAPI spec: the only other 2xx body carrying a top-level `error` is
+// POST /insights/v1/results, whose 200 is a top-level *array* of per-row error
+// items. The map assertion below lets that array pass through untouched. Do not
+// recurse into arrays or nested objects here, or it will false-positive on
+// legitimate partial-result responses.
 func jsonApplicationError(resp cli.Response) (string, bool) {
 	if resp.Status < 200 || resp.Status >= 300 {
 		return "", false
@@ -1506,6 +1520,7 @@ func jsonApplicationError(resp cli.Response) (string, bool) {
 		}
 		return v, true
 	case map[string]interface{}:
+		// Defensive only: no current DCI endpoint returns an object-typed error (AVA returns a string).
 		if len(v) == 0 {
 			return "", false
 		}
@@ -1528,7 +1543,8 @@ func printNonJSONError(resp cli.Response) {
 		fmt.Fprintf(&b, "Trace: %s\n", trace)
 	}
 	b.WriteString("Please retry in a moment. If it persists, contact DoiT support with the trace above.\n")
-	fmt.Fprint(os.Stderr, b.String())
+	// cli.Stderr (not os.Stderr) so internal callers like login can redirect it.
+	fmt.Fprint(cli.Stderr, b.String())
 }
 
 // traceID returns the first available upstream trace identifier so users can
diff --git a/main_test.go b/main_test.go
@@ -2038,6 +2038,21 @@ func TestIsHTMLErrorPage(t *testing.T) {
 			resp: cli.Response{Body: []byte("<head>...</head>")},
 			want: true,
 		},
+		{
+			name: "xml declaration edge page without content type",
+			resp: cli.Response{Body: "<?xml version=\"1.0\"?><error>upstream</error>"},
+			want: true,
+		},
+		{
+			name: "html comment edge page without content type",
+			resp: cli.Response{Body: "<!-- error --><html></html>"},
+			want: true,
+		},
+		{
+			name: "utf-8 bom before doctype",
+			resp: cli.Response{Body: "\ufeff<!DOCTYPE html><html></html>"},
+			want: true,
+		},
 		{
 			name: "json object body is not html",
 			resp: cli.Response{Headers: map[string]string{"Content-Type": "application/json"}, Body: map[string]interface{}{"answer": "hi"}},
@@ -2112,8 +2127,8 @@ func TestResponseGuardFormat(t *testing.T) {
 		guard := dciResponseGuard{next: next}
 
 		r, w, _ := os.Pipe()
-		oldStderr := os.Stderr
-		os.Stderr = w
+		oldStderr := cli.Stderr
+		cli.Stderr = w
 
 		err := guard.Format(cli.Response{
 			Status:  524,
@@ -2122,7 +2137,7 @@ func TestResponseGuardFormat(t *testing.T) {
 		})
 
 		w.Close()
-		os.Stderr = oldStderr
+		cli.Stderr = oldStderr
 		buf := make([]byte, 4096)
 		n, _ := r.Read(buf)
 		output := string(buf[:n])
@@ -2179,8 +2194,8 @@ func TestResponseGuardFormat(t *testing.T) {
 		guard := dciResponseGuard{next: next}
 
 		r, w, _ := os.Pipe()
-		oldStderr := os.Stderr
-		os.Stderr = w
+		oldStderr := cli.Stderr
+		cli.Stderr = w
 
 		err := guard.Format(cli.Response{
 			Status:  200,
@@ -2189,7 +2204,7 @@ func TestResponseGuardFormat(t *testing.T) {
 		})
 
 		w.Close()
-		os.Stderr = oldStderr
+		cli.Stderr = oldStderr
 		buf := make([]byte, 4096)
 		n, _ := r.Read(buf)
 		output := string(buf[:n])
