amadan
cli: console redirects explained and offered, never followed (#160)
CheckRedirect refuses uniformly (Go's default followed cross-host GETs with Authorization stripped and surfaced raw 3xx on posts). A 3xx whose Location is a console-shaped origin becomes consoleMovedError; a TTY gets the device-flow offer plus a config repoint that keeps the account key. One shared chooser scanner — per-prompt scanners swallow the next answer.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4 files changed,
+222
−7
account_choose.go+23 −4apimode.go+77 −0apimode_test.go+100 −0auth.go+22 −3
diff --git a/account_choose.go b/account_choose.go| index fee2f21d8..185083a39 100644 |
| --- a/account_choose.go |
| +++ b/account_choose.go |
| @@ -49,12 +49,25 @@ var interactiveTerminal = func() bool { |
| // tests for the same reason interactiveTerminal is a var. |
| var chooserIn io.Reader = os.Stdin |
| -// promptLine prints one prompt and reads one trimmed line from chooserIn; |
| -// EOF answers "". The prompts here are one line each on a line-buffered |
| -// terminal, so a fresh Scanner per question does not eat the next answer. |
| +// chooserSc is the ONE scanner over chooserIn, rebound when a test swaps |
| +// the reader. One scanner, not one per prompt: a Scanner buffers ahead, so |
| +// a second Scanner on the same reader would find the bytes the first one |
| +// already swallowed — the second answer in "y\ny\n" simply vanishes. |
| +var chooserSc *bufio.Scanner |
| +var chooserScFor io.Reader |
| + |
| +func chooserScanner() *bufio.Scanner { |
| + if chooserSc == nil || chooserScFor != chooserIn { |
| + chooserSc = bufio.NewScanner(chooserIn) |
| + chooserScFor = chooserIn |
| + } |
| + return chooserSc |
| +} |
| + |
| +// promptLine prints one prompt and reads one trimmed line; EOF answers "". |
| func promptLine(out io.Writer, prompt string) string { |
| fmt.Fprint(out, prompt) |
| - sc := bufio.NewScanner(chooserIn) |
| + sc := chooserScanner() |
| if !sc.Scan() { |
| return "" |
| } |
| @@ -82,6 +95,12 @@ func chooseAccount(in io.Reader, out io.Writer, console string, accts []apiAccou |
| fmt.Fprintf(out, " %d. %s (%s)\n", i+1, a.Name, a.Sqid) |
| } |
| sc := bufio.NewScanner(in) |
| + if in == chooserIn { |
| + // The process-wide input shares the process-wide scanner — a private |
| + // one here would buffer past the chooser's answers and starve any |
| + // later promptLine (see chooserScanner). |
| + sc = chooserScanner() |
| + } |
| var picked *apiAccountEntry |
| for attempt := 0; attempt < 2 && picked == nil; attempt++ { |
| fmt.Fprintf(out, "account [1-%d]: ", len(accts)) |
diff --git a/apimode.go b/apimode.go| index 14127d18b..1d66e0a67 100644 |
| --- a/apimode.go |
| +++ b/apimode.go |
| @@ -119,6 +119,71 @@ func resolveMode(consoleFlag string) (*apiSession, error) { |
| }, nil |
| } |
| +// consoleMovedError is a permanent-redirect answer from a console (#160): |
| +// the host this CLI was pointed at now lives somewhere else, and the |
| +// message carries the whole story — a script that hits it needs no oracle. |
| +type consoleMovedError struct{ From, To string } |
| + |
| +func (e *consoleMovedError) Error() string { |
| + return fmt.Sprintf("this console has moved: %s -> %s — run: carlos auth login -console %s", |
| + e.From, e.To, e.To) |
| +} |
| + |
| +// consoleMoved inspects one response for a console-shaped redirect. Only a |
| +// 3xx whose Location names an ABSOLUTE origin normalizeConsole would accept |
| +// (https, or http strictly on-box) and that differs from the one dialed |
| +// counts — a same-origin or path-level redirect stays a plain unexpected |
| +// status, and a Location the CLI would refuse to log in to is never |
| +// surfaced as a destination. The Location is trusted for DISPLAY AND |
| +// PROMPTING only: nothing follows it, and no credential exists until the |
| +// human confirms the device flow against it. |
| +func consoleMoved(from string, status int, location string) *consoleMovedError { |
| + switch status { |
| + case http.StatusMovedPermanently, http.StatusFound, |
| + http.StatusTemporaryRedirect, http.StatusPermanentRedirect: |
| + default: |
| + return nil |
| + } |
| + u, err := url.Parse(location) |
| + if err != nil || !u.IsAbs() || u.Host == "" { |
| + return nil |
| + } |
| + origin, err := normalizeConsole(u.Scheme + "://" + u.Host) |
| + if err != nil || origin == from { |
| + return nil |
| + } |
| + return &consoleMovedError{From: from, To: origin} |
| +} |
| + |
| +// offerConsoleMoved is the terminal's version of consoleMovedError: explain |
| +// the move, offer the standard device flow against the new host, and — when |
| +// a .carlos/config names the old console — offer to repoint it (keeping |
| +// every other key, account included: the pair moves together). The verb |
| +// still ends nonzero: its request died against the old host mid-flight, and |
| +// "logged in — re-run your command" is the honest state. |
| +func offerConsoleMoved(e *consoleMovedError) error { |
| + if !interactiveTerminal() { |
| + return e |
| + } |
| + fmt.Fprintf(stdout, "this console has moved: %s -> %s\n", e.From, e.To) |
| + if !promptYes(stdout, fmt.Sprintf("log in to %s now? [y/N] ", e.To)) { |
| + return e |
| + } |
| + if err := authLogin(stdout, []string{"-console", e.To}); err != nil { |
| + return err |
| + } |
| + if cfgConsole, src, err := configDefault(); err == nil && cfgConsole == e.From && src != "" { |
| + if promptYes(stdout, fmt.Sprintf("update %s to point there too? [y/N] ", src)) { |
| + if err := saveConfigFile(src, e.To, 0o755); err != nil { |
| + warnf(stdout, "not updated (%s): %v", src, err) |
| + } else { |
| + mutedf(stdout, "updated: %s now names %s", src, e.To) |
| + } |
| + } |
| + } |
| + return fmt.Errorf("logged in to %s — re-run your command against it", e.To) |
| +} |
| + |
| // errNoDeploymentTarget is resolveMode's "nothing configured at all" |
| // refusal — a var so offerLogin can recognize it by identity rather than |
| // by matching its wording. |
| @@ -482,6 +547,9 @@ func (s *apiSession) getJSON(ctx context.Context, path string) (int, []byte, err |
| return 0, nil, err |
| } |
| defer res.Body.Close() |
| + if merr := consoleMoved(s.c.base, res.StatusCode, res.Header.Get("Location")); merr != nil { |
| + return res.StatusCode, nil, offerConsoleMoved(merr) |
| + } |
| body, err := io.ReadAll(io.LimitReader(res.Body, 1<<20)) |
| if err != nil { |
| return res.StatusCode, nil, err |
| @@ -512,6 +580,9 @@ func (s *apiSession) postJSON(ctx context.Context, path string, in any) (int, [] |
| return 0, nil, err |
| } |
| defer res.Body.Close() |
| + if merr := consoleMoved(s.c.base, res.StatusCode, res.Header.Get("Location")); merr != nil { |
| + return res.StatusCode, nil, offerConsoleMoved(merr) |
| + } |
| body, err := io.ReadAll(io.LimitReader(res.Body, 1<<20)) |
| if err != nil { |
| return res.StatusCode, nil, err |
| @@ -1240,6 +1311,9 @@ func (s *apiSession) putJSON(ctx context.Context, path string, in any) (int, []b |
| return 0, nil, err |
| } |
| defer res.Body.Close() |
| + if merr := consoleMoved(s.c.base, res.StatusCode, res.Header.Get("Location")); merr != nil { |
| + return res.StatusCode, nil, offerConsoleMoved(merr) |
| + } |
| body, err := io.ReadAll(io.LimitReader(res.Body, 1<<20)) |
| if err != nil { |
| return res.StatusCode, nil, err |
| @@ -1401,6 +1475,9 @@ func (s *apiSession) deleteJSON(ctx context.Context, path string) (int, []byte, |
| return 0, nil, err |
| } |
| defer res.Body.Close() |
| + if merr := consoleMoved(s.c.base, res.StatusCode, res.Header.Get("Location")); merr != nil { |
| + return res.StatusCode, nil, offerConsoleMoved(merr) |
| + } |
| body, err := io.ReadAll(io.LimitReader(res.Body, 1<<20)) |
| if err != nil { |
| return res.StatusCode, nil, err |
diff --git a/apimode_test.go b/apimode_test.go| index f7702387b..af7b3f4ba 100644 |
| --- a/apimode_test.go |
| +++ b/apimode_test.go |
| @@ -3008,6 +3008,106 @@ func TestDefaultAccountConfigDefault(t *testing.T) { |
| } |
| } |
| +func TestConsoleMovedDetection(t *testing.T) { |
| + from := "https://dashboard.example" |
| + // The real case: a permanent redirect to a new https origin. |
| + if e := consoleMoved(from, 308, "https://console.example/api/cli/apps"); e == nil || |
| + e.To != "https://console.example" { |
| + t.Fatalf("308 = %+v, want console.example", e) |
| + } |
| + want := "this console has moved: https://dashboard.example -> https://console.example — run: carlos auth login -console https://console.example" |
| + if e := consoleMoved(from, 301, "https://console.example/"); e == nil || e.Error() != want { |
| + t.Fatalf("moved error = %v\nwant %q", e, want) |
| + } |
| + // Everything that must NOT read as "moved". |
| + for name, tc := range map[string]struct { |
| + status int |
| + location string |
| + }{ |
| + "non-redirect status": {404, "https://console.example"}, |
| + "no location": {308, ""}, |
| + "relative location": {308, "/somewhere"}, |
| + "same origin": {308, from + "/new-path"}, |
| + "http off-box target": {308, "http://console.example"}, |
| + "garbage location": {308, "::not a url::"}, |
| + "schemeless authority": {308, "console.example/x"}, |
| + } { |
| + if e := consoleMoved(from, tc.status, tc.location); e != nil { |
| + t.Errorf("%s = %+v, want nil", name, e) |
| + } |
| + } |
| +} |
| + |
| +func TestRedirectsAreNeverFollowed(t *testing.T) { |
| + useTempCredentials(t) |
| + captureStdout(t) |
| + forceTerminal(t, false) |
| + // A "new" console that would answer if the client followed — its |
| + // handler must never run. |
| + followed := false |
| + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| + followed = true |
| + w.Write([]byte(`{}`)) |
| + })) |
| + t.Cleanup(target.Close) |
| + old := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| + w.Header().Set("Location", target.URL+r.URL.Path) |
| + w.WriteHeader(http.StatusPermanentRedirect) |
| + })) |
| + t.Cleanup(old.Close) |
| + |
| + s := newTestSession(old.URL) |
| + // GET (the case Go's default policy would have silently followed with |
| + // Authorization stripped) and POST both surface the typed error. |
| + _, _, err := s.getJSON(context.Background(), "/api/cli/apps") |
| + if err == nil || !strings.Contains(err.Error(), "this console has moved: "+old.URL+" -> "+target.URL) { |
| + t.Fatalf("GET through a redirect = %v", err) |
| + } |
| + _, _, err = s.postJSON(context.Background(), "/api/cli/apps/a/b/promote", map[string]string{}) |
| + if err == nil || !strings.Contains(err.Error(), "this console has moved") { |
| + t.Fatalf("POST through a redirect = %v", err) |
| + } |
| + if followed { |
| + t.Fatal("the client followed a redirect — Authorization would have crossed hosts") |
| + } |
| +} |
| + |
| +func TestConsoleMovedOfferLogsInAndRepointsConfig(t *testing.T) { |
| + useTempCredentials(t) |
| + clearDeploymentEnv(t) |
| + out := captureStdout(t) |
| + forceTerminal(t, true) |
| + fc := newFakeConsole(t) // the console's NEW home — real device flow |
| + old := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| + w.Header().Set("Location", fc.ts.URL+r.URL.Path) |
| + w.WriteHeader(http.StatusPermanentRedirect) |
| + })) |
| + t.Cleanup(old.Close) |
| + cfgPath := writeConfig(t, ".", `{"console": "`+old.URL+`", "account": "acme"}`) |
| + oldIn := chooserIn |
| + chooserIn = strings.NewReader("y\ny\n") // log in? yes. repoint config? yes. |
| + t.Cleanup(func() { chooserIn = oldIn }) |
| + |
| + s := newTestSession(old.URL) |
| + _, _, err := s.getJSON(context.Background(), "/api/cli/apps") |
| + if err == nil || !strings.Contains(err.Error(), "re-run your command") { |
| + t.Fatalf("offer accepted = %v, want the re-run message", err) |
| + } |
| + // The device flow really ran against the new host… |
| + f, ferr := loadCredentials() |
| + if ferr != nil || f.find(fc.ts.URL) == nil { |
| + t.Fatalf("no credential for the new console after the offer: %v", ferr) |
| + } |
| + // …and the config was repointed, keeping the account key. |
| + cfg, cerr := loadConfig(cfgPath) |
| + if cerr != nil || cfg.Console != fc.ts.URL || cfg.Account != "acme" { |
| + t.Fatalf("config after repoint = %+v, %v; want new console + acme kept", cfg, cerr) |
| + } |
| + if !strings.Contains(out.String(), "this console has moved") { |
| + t.Fatalf("the move must be explained:\n%s", out.String()) |
| + } |
| +} |
| + |
| // resolveMode on a terminal offers to run the login it would otherwise |
| // only name — and then the ORIGINAL command continues (#162). |
| func TestResolveModeOffersLoginAndContinues(t *testing.T) { |
diff --git a/auth.go b/auth.go| index ec2c7bd16..c0b5b5c51 100644 |
| --- a/auth.go |
| +++ b/auth.go |
| @@ -393,7 +393,15 @@ type consoleClient struct { |
| } |
| func newConsoleClient(base, token string) *consoleClient { |
| - return &consoleClient{base: base, token: token, httpc: &http.Client{Timeout: 30 * time.Second}} |
| + // Redirects are NEVER followed (#160): Go's default policy follows a |
| + // cross-host GET with Authorization stripped (a mystery 401 from the |
| + // new host) and surfaces unreplayable POSTs as raw 3xx — both wrong. |
| + // Refusing uniformly lets every method see the 3xx + Location, which |
| + // consoleMoved (apimode.go) turns into an explanation and an offer. |
| + return &consoleClient{base: base, token: token, httpc: &http.Client{ |
| + Timeout: 30 * time.Second, |
| + CheckRedirect: neverFollow, |
| + }} |
| } |
| // do runs one JSON round-trip. A non-200 comes back carrying the console's |
| @@ -423,6 +431,9 @@ func (c *consoleClient) do(ctx context.Context, method, path string, in, out any |
| return 0, err |
| } |
| defer res.Body.Close() |
| + if merr := consoleMoved(c.base, res.StatusCode, res.Header.Get("Location")); merr != nil { |
| + return res.StatusCode, offerConsoleMoved(merr) |
| + } |
| raw, err := io.ReadAll(io.LimitReader(res.Body, 1<<20)) |
| if err != nil { |
| return res.StatusCode, err |
| @@ -496,13 +507,16 @@ func (c *consoleClient) doMultipart(ctx context.Context, path string, write func |
| } |
| // A client of its own — see shipHTTPClientTimeout above — never c.httpc, |
| // whose 30s absolute Timeout is sized for the JSON verbs, not a large |
| - // upload. |
| - client := &http.Client{Timeout: shipHTTPClientTimeout} |
| + // upload. Same no-redirect rule as c.httpc, same reason. |
| + client := &http.Client{Timeout: shipHTTPClientTimeout, CheckRedirect: neverFollow} |
| res, err := client.Do(req) |
| if err != nil { |
| return 0, nil, err |
| } |
| defer res.Body.Close() |
| + if merr := consoleMoved(c.base, res.StatusCode, res.Header.Get("Location")); merr != nil { |
| + return res.StatusCode, nil, offerConsoleMoved(merr) |
| + } |
| body, err := io.ReadAll(io.LimitReader(res.Body, 1<<20)) |
| if err != nil { |
| return res.StatusCode, nil, err |
| @@ -510,6 +524,11 @@ func (c *consoleClient) doMultipart(ctx context.Context, path string, write func |
| return res.StatusCode, body, nil |
| } |
| +// neverFollow is the one redirect policy every console client uses. |
| +func neverFollow(*http.Request, []*http.Request) error { |
| + return http.ErrUseLastResponse |
| +} |
| + |
| type startResponse struct { |
| UserCode string `json:"user_code"` |
| DeviceCode string `json:"device_code"` |