From 323020c87d26c1a28a9e0beae042cf09cf7fc6b1 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 6 Oct 2026 11:03:16 -0700 Subject: [PATCH 1/6] Read the agent connection ceremony as data with --json auth agent connect refused every machine output mode, because the link, the code and the wait line all went to stdout. Whatever drives the ceremony for a person (a one-click Connect, a sandbox install, an agent relaying it) had to scrape prose to learn the link and the code. Under --json (and --agent) stdout now carries two JSON values: a verification line, written as soon as Basecamp answers the intake (link, user code, expiry), then the result envelope (profile, account, scope, client id). The person's half moves to stderr and the browser opens as it would. Neither stream carries the client secret or the device code. --jq, --quiet, --ids-only and --count are still refused: none of them can carry the verification line. --- internal/auth/agent_connect.go | 26 ++++ internal/commands/auth_agent.go | 104 ++++++++++++- internal/commands/auth_agent_connect_test.go | 147 ++++++++++++++++++- 3 files changed, 265 insertions(+), 12 deletions(-) diff --git a/internal/auth/agent_connect.go b/internal/auth/agent_connect.go index fafca6a99..1e17ac8c3 100644 --- a/internal/auth/agent_connect.go +++ b/internal/auth/agent_connect.go @@ -111,6 +111,16 @@ type AgentConnectResult struct { ClientID string } +// AgentConnectIntake is what a person needs to approve a connection: the +// link to open, the code to check against the page, and when the code +// dies. The link is the validated URL the browser would be sent to; the +// code is stripped of control sequences, as the terminal copy is. +type AgentConnectIntake struct { + VerificationURI string + UserCode string + ExpiresAt time.Time +} + // AgentConnectOptions configures the connection ceremony. type AgentConnectOptions struct { // DeviceName is where this connector runs, self-asserted and shown to @@ -146,6 +156,14 @@ type AgentConnectOptions struct { // the poll runs, exactly as it does for a device login. Progress io.Writer + // OnIntake, when set, is told what the operator needs to approve the + // connection as soon as the intake has answered, before the browser is + // opened and the wait begins: a caller reading the ceremony as data + // learns the link and the code without parsing the operator's half. + // It is never given the device code, which is the poll's bearer and + // stays inside this flow. + OnIntake func(AgentConnectIntake) + // BeforeStore, when set, runs after the poll has handed over the // credential and the mint has proved it, and before anything is // written. A non-nil error aborts the connection and stores nothing. @@ -256,6 +274,14 @@ func (m *Manager) ConnectAgent(ctx context.Context, opts AgentConnectOptions) (* return nil, err } + if opts.OnIntake != nil { + opts.OnIntake(AgentConnectIntake{ + VerificationURI: intake.verificationURI, + UserCode: intake.userCode, + ExpiresAt: opts.now().Add(intake.lifetime), + }) + } + wait := announceAgentConnection(opts.presentation(), intake, opts.now()) conn, err := m.awaitAgentConnection(ctx, &opts, client, intake) wait.Stop() diff --git a/internal/commands/auth_agent.go b/internal/commands/auth_agent.go index a70239f35..495274c71 100644 --- a/internal/commands/auth_agent.go +++ b/internal/commands/auth_agent.go @@ -1,8 +1,11 @@ package commands import ( + "encoding/json" "fmt" + "io" "os" + "time" "github.com/spf13/cobra" @@ -65,6 +68,20 @@ Over SSH, in CI, or on a host with no display the browser is skipped and the link is yours to open on any device; --local forces a launch anyway, --no-browser skips it. Ctrl-C cancels the wait and stores nothing. +With --json, the ceremony is data for whatever runs it, and stdout carries +two JSON values. The first is one line, written as soon as Basecamp +answers the request: + + {"type":"verification","verification_uri":"…","user_code":"WDJB-MJHT","expires_at":"…","expires_in":600} + +The second is the result envelope every command prints: the profile, its +account, the access approved and the agent's client id — or the error, if +the operator declined, the code expired, or nothing could be stored. The words meant +for a person go to stderr, and the browser opens as it would without +--json. Neither line ever carries the client secret or the device code. +The link and the code are for the person approving at this computer: +show them there, never anywhere else. + Disconnecting is done in Basecamp: disconnecting the agent there kills the client secret this holds. Drop the local copy with ` + "`basecamp auth logout -P `" + `.`, Annotations: map[string]string{AnnotationProfileMayCreate: "true"}, @@ -75,7 +92,7 @@ client secret this holds. Drop the local copy with ` + "`basecamp auth logout -P return fmt.Errorf("app not initialized") } - if err := refuseMachineOutputLogin(app, "the agent connection"); err != nil { + if err := refuseAgentConnectOutputMode(app); err != nil { return err } if err := refuseNonInteractiveAgentConnect(); err != nil { @@ -90,6 +107,7 @@ client secret this holds. Drop the local copy with ` + "`basecamp auth logout -P return connectAgentProfile(cmd, app, agentConnectFlags{ deviceName: deviceName, softwareName: softwareName, scope: scope, noBrowser: noBrowser, local: local, + machine: app.Flags.JSON || app.Flags.Agent, }) }, } @@ -114,6 +132,56 @@ type agentConnectFlags struct { // quiet leaves out what was stored, for the guided setup, which says // in one line of its own which agent this computer is now connected as. quiet bool + + // machine reads the ceremony out as data: the verification line and + // the result envelope on stdout, the operator's words on stderr. + machine bool +} + +// refuseAgentConnectOutputMode lets the ceremony run under --json (and +// --agent, its data-only form), and refuses the output modes that cannot +// carry it. A --jq filter runs over one envelope, and this writes two +// lines; --quiet, --ids-only and --count would each throw the verification +// line away, and with it the only thing the operator needs. +// +// --agent is let through as --json is: it prints the result's data alone, +// after the same verification line. +func refuseAgentConnectOutputMode(app *appctx.App) error { + if app.Flags.JQFilter != "" { + return output.ErrJQNotSupported("the agent connection") + } + if app.Flags.Quiet || app.Flags.IDsOnly || app.Flags.Count { + return output.ErrUsageHint("The agent connection cannot run under --quiet, --ids-only or --count", + "Run it plainly for a person at a terminal, or with --json to read the link, the code and the result as JSON lines.") + } + return nil +} + +// agentConnectVerification is the ceremony's first JSON line: what the +// operator needs to approve the connection, and nothing else. +type agentConnectVerification struct { + Type string `json:"type"` + VerificationURI string `json:"verification_uri"` + UserCode string `json:"user_code"` + ExpiresAt string `json:"expires_at"` + ExpiresIn int `json:"expires_in"` +} + +// writeAgentConnectVerification writes the verification line, one JSON +// object terminated by a newline, so a reader can act on it while the +// ceremony is still waiting. +func writeAgentConnectVerification(w io.Writer, intake auth.AgentConnectIntake) { + line := agentConnectVerification{ + Type: "verification", + VerificationURI: intake.VerificationURI, + UserCode: intake.UserCode, + ExpiresAt: intake.ExpiresAt.UTC().Format(time.RFC3339), + ExpiresIn: max(0, int(time.Until(intake.ExpiresAt).Round(time.Second).Seconds())), + } + // Encode cannot fail on a struct of strings and ints, and a write + // that fails is a stdout that is gone, which the result line will + // meet too. + _ = json.NewEncoder(w).Encode(line) } // connectAgentProfile runs the agent-connection handshake for the active @@ -126,7 +194,15 @@ func connectAgentProfile(cmd *cobra.Command, app *appctx.App, f agentConnectFlag } w := cmd.OutOrStdout() - r := output.NewRendererWithTheme(w, false, tui.ResolveTheme(tui.DetectDark())) + // Under --json stdout is the data, so the operator's half of the + // ceremony — the link, the code, the wait line — goes to stderr. + human := w + var onIntake func(auth.AgentConnectIntake) + if f.machine { + human = cmd.ErrOrStderr() + onIntake = func(intake auth.AgentConnectIntake) { writeAgentConnectVerification(w, intake) } + } + r := output.NewRendererWithTheme(human, false, tui.ResolveTheme(tui.DetectDark())) var isDefault bool ctx, stop := loginContext(cmd) @@ -136,20 +212,38 @@ func connectAgentProfile(cmd *cobra.Command, app *appctx.App, f agentConnectFlag Scope: f.scope, NoBrowser: f.noBrowser, Local: f.local, - Logger: func(msg string) { fmt.Fprintln(w, msg) }, - Progress: w, + Logger: func(msg string) { fmt.Fprintln(human, msg) }, + Progress: human, + OnIntake: onIntake, BeforeStore: func(conn *auth.AgentConnection) error { registered, commitErr := target.commit(app, conn) isDefault = registered return commitErr }, }) - err = loginOutcome(ctx, err, w, r) + err = loginOutcome(ctx, err, human, r) stop() if err != nil || f.quiet { return err } + if f.machine { + data := map[string]any{ + "profile": target.name, + "account_id": result.AccountID, + "base_url": app.Config.BaseURL, + "source": "agent_connection", + "oauth_type": result.OAuthType, + "scope": result.Scope, + "client_id": result.ClientID, + "profile_created": target.existing == nil, + } + if isDefault { + data["default"] = true + } + return app.OK(data, output.WithSummary(fmt.Sprintf("Connected profile %q to a Basecamp agent", target.name))) + } + fmt.Fprintln(w) fmt.Fprintln(w, r.Success.Render(fmt.Sprintf("Connected profile %q to a Basecamp agent", target.name))) fmt.Fprintln(w, r.Muted.Render(fmt.Sprintf("Profile: %s · Account: %s · Access: %s · Token: minted on demand, no refresh token", diff --git a/internal/commands/auth_agent_connect_test.go b/internal/commands/auth_agent_connect_test.go index 7c46da3de..e3b5b7033 100644 --- a/internal/commands/auth_agent_connect_test.go +++ b/internal/commands/auth_agent_connect_test.go @@ -3,10 +3,12 @@ package commands import ( "bytes" "context" + "encoding/json" "fmt" "net/http" "net/http/httptest" "net/url" + "strings" "sync" "testing" @@ -37,6 +39,10 @@ type connectAS struct { // connection renders the successful poll; the default approves a // full-scope agent in account 999. connection func() string + + // pollStatus, when non-zero, is the poll's status in place of 200: a + // refusal, rendered from connection. + pollStatus int } func startConnectAS(t *testing.T) *connectAS { @@ -58,6 +64,9 @@ func startConnectAS(t *testing.T) *connectAS { as.pollForms = append(as.pollForms, r.PostForm) as.mu.Unlock() w.Header().Set("Content-Type", "application/json") + if as.pollStatus != 0 { + w.WriteHeader(as.pollStatus) + } fmt.Fprint(w, as.connection()) }) mux.HandleFunc("/oauth/tokens", func(w http.ResponseWriter, r *http.Request) { @@ -199,17 +208,141 @@ func TestAuthAgentConnectNeedsAProfile(t *testing.T) { assert.Empty(t, as.mints()) } -// TestAuthAgentConnectRefusesMachineOutput: the link, the code and the wait -// line go to stdout, which an envelope also owns. -func TestAuthAgentConnectRefusesMachineOutput(t *testing.T) { +// runAgentConnectJSON runs the ceremony under --json with stdout and stderr +// kept apart, as a program driving it would read them. stdout is a +// lockedBuffer so the mock server's handlers can read it mid-ceremony. +func runAgentConnectJSON(t *testing.T, app *appctx.App, stdout *lockedBuffer, args ...string) (string, error) { + t.Helper() + app.Flags.JSON = true + app.Output = output.New(output.Options{Format: output.FormatJSON, Writer: stdout}) + cmd := NewAuthCmd() + cmd.SetArgs(append([]string{"agent", "connect", "--no-browser"}, args...)) + cmd.SetContext(appctx.WithApp(context.Background(), app)) + var stderr bytes.Buffer + cmd.SetOut(stdout) + cmd.SetErr(&stderr) + cmd.SilenceErrors = true + cmd.SilenceUsage = true + err := cmd.Execute() + return stderr.String(), err +} + +// jsonValues reads stdout as a stream of JSON values, the way a program +// driving the ceremony would: the verification line, then the envelope, +// however that is laid out. +func jsonValues(t *testing.T, stdout string) []map[string]any { + t.Helper() + dec := json.NewDecoder(strings.NewReader(stdout)) + var values []map[string]any + for dec.More() { + var v map[string]any + require.NoError(t, dec.Decode(&v), "stdout is JSON values and nothing else: %q", stdout) + values = append(values, v) + } + return values +} + +// TestAuthAgentConnectJSONWritesTheVerificationThenTheResult: under --json +// the ceremony is two lines of data on stdout. The first carries what the +// operator needs and is written while the ceremony is still waiting on +// them; the last is the result. The operator's words go to stderr. Neither +// stream carries the client secret or the device code. +func TestAuthAgentConnectJSONWritesTheVerificationThenTheResult(t *testing.T) { as := startConnectAS(t) app := connectApp(t, as, &config.Config{ActiveProfile: "agent"}) - app.Flags.JSON = true + stdout := &lockedBuffer{} - _, err := runAgentConnect(t, app) + approve := as.connection + var atPoll string + as.connection = func() string { + atPoll = stdout.String() + return approve() + } + + stderr, err := runAgentConnectJSON(t, app, stdout, "--device-name", "build-box") + require.NoError(t, err, stderr) + + require.NotEmpty(t, atPoll, "the verification line is written before the operator approves, not after") + require.True(t, strings.HasSuffix(atPoll, "}\n") && strings.Count(atPoll, "\n") == 1, + "the verification is one line, newline-terminated, so a reader can act on it at once: %q", atPoll) + verification := jsonValues(t, atPoll) + require.Len(t, verification, 1) + assert.Equal(t, "verification", verification[0]["type"]) + assert.Equal(t, as.srv.URL+"/connect?user_code=WDJB-MJHT", verification[0]["verification_uri"]) + assert.Equal(t, "WDJB-MJHT", verification[0]["user_code"]) + assert.NotEmpty(t, verification[0]["expires_at"]) + expiresIn, ok := verification[0]["expires_in"].(float64) + require.True(t, ok) + assert.InDelta(t, 600, expiresIn, 5) + + lines := jsonValues(t, stdout.String()) + require.Len(t, lines, 2, "the verification line, then the result") + data, ok := lines[1]["data"].(map[string]any) + require.True(t, ok, stdout.String()) + assert.Equal(t, "agent", data["profile"]) + assert.Equal(t, "999", data["account_id"]) + assert.Equal(t, "agent", data["oauth_type"]) + assert.Equal(t, "agent_connection", data["source"]) + assert.Equal(t, "full", data["scope"]) + assert.Equal(t, "agent-client", data["client_id"]) + assert.Equal(t, true, data["profile_created"]) + assert.Equal(t, true, data["default"]) + assert.Equal(t, `Connected profile "agent" to a Basecamp agent`, lines[1]["summary"]) + + assert.Contains(t, stderr, "WDJB-MJHT", "a person at the terminal still sees the code") + assert.NotContains(t, stderr, "Connected profile", "the result is data, not a second copy in prose") + for name, stream := range map[string]string{"stdout": stdout.String(), "stderr": stderr} { + assert.NotContains(t, stream, fakeConnectSecret, "%s carries the client secret", name) + assert.NotContains(t, stream, "dev-code-1", "%s carries the device code, the poll's bearer", name) + } + + creds, err := app.Auth.GetStore().Load("profile:agent") + require.NoError(t, err) + assert.Equal(t, fakeConnectSecret, creds.ClientSecret, "the credential is stored exactly as without --json") +} + +// TestAuthAgentConnectJSONOnADeclineWritesOnlyTheVerification: a declined +// connection ends with an error, which the CLI renders as the last line, +// and nothing is stored. The verification line is already out by then. +func TestAuthAgentConnectJSONOnADeclineWritesOnlyTheVerification(t *testing.T) { + as := startConnectAS(t) + as.pollStatus = 400 + as.connection = func() string { return `{"error":"access_denied"}` } + app := connectApp(t, as, &config.Config{ActiveProfile: "agent"}) + stdout := &lockedBuffer{} + + _, err := runAgentConnectJSON(t, app, stdout, "--device-name", "build-box") require.Error(t, err) - assert.Contains(t, err.Error(), "machine output mode") - assert.Empty(t, as.mints()) + + lines := jsonValues(t, stdout.String()) + require.Len(t, lines, 1, "no result envelope from the command itself; the error is the CLI's to render") + assert.Equal(t, "verification", lines[0]["type"]) + assert.Empty(t, as.mints(), "a declined connection mints nothing") + assertNothingStored(t, app, "agent") +} + +// TestAuthAgentConnectRefusesOutputModesThatCannotCarryIt: --jq filters one +// envelope and this writes two lines; --quiet, --ids-only and --count would +// throw the verification line away. Each is refused before the server is +// asked for anything. +func TestAuthAgentConnectRefusesOutputModesThatCannotCarryIt(t *testing.T) { + for name, set := range map[string]func(*appctx.App){ + "jq": func(a *appctx.App) { a.Flags.JQFilter = ".data" }, + "quiet": func(a *appctx.App) { a.Flags.Quiet = true }, + "ids-only": func(a *appctx.App) { a.Flags.IDsOnly = true }, + "count": func(a *appctx.App) { a.Flags.Count = true }, + } { + t.Run(name, func(t *testing.T) { + as := startConnectAS(t) + app := connectApp(t, as, &config.Config{ActiveProfile: "agent"}) + set(app) + + _, err := runAgentConnect(t, app) + require.Error(t, err) + assert.Equal(t, output.CodeUsage, output.AsError(err).Code) + assert.Empty(t, as.mints()) + }) + } } // TestAuthAgentConnectRefusesNonInteractive: approving a connection needs a From 1eb0a25de407fba5a0ce31ded92c87f4cf91cba8 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 6 Oct 2026 21:08:03 -0700 Subject: [PATCH 2/6] Give the verification line the link the terminal shows, and read stdout to its end The line's link was the raw URL the server named, which url.Parse lets carry Unicode C1 controls, and the JSON encoder writes those through as the bytes a terminal acts on. It is now the stripped copy the terminal shows, as the code already was; the browser is still sent to the URL itself. The test read stdout while More(), which at the top level stops quietly at a stray closing delimiter. It now decodes to the end. --- internal/auth/agent_connect.go | 8 ++-- internal/commands/auth_agent_connect_test.go | 45 ++++++++++++++++++-- 2 files changed, 46 insertions(+), 7 deletions(-) diff --git a/internal/auth/agent_connect.go b/internal/auth/agent_connect.go index 1e17ac8c3..9d2c3ff31 100644 --- a/internal/auth/agent_connect.go +++ b/internal/auth/agent_connect.go @@ -113,8 +113,10 @@ type AgentConnectResult struct { // AgentConnectIntake is what a person needs to approve a connection: the // link to open, the code to check against the page, and when the code -// dies. The link is the validated URL the browser would be sent to; the -// code is stripped of control sequences, as the terminal copy is. +// dies. Both are the copies the terminal shows, stripped of control +// sequences: whoever reads them shows them to a person, and a JSON encoder +// passes UTF-8-encoded C1 controls through raw. The browser is still sent +// to the validated URL itself. type AgentConnectIntake struct { VerificationURI string UserCode string @@ -276,7 +278,7 @@ func (m *Manager) ConnectAgent(ctx context.Context, opts AgentConnectOptions) (* if opts.OnIntake != nil { opts.OnIntake(AgentConnectIntake{ - VerificationURI: intake.verificationURI, + VerificationURI: intake.shownURI, UserCode: intake.userCode, ExpiresAt: opts.now().Add(intake.lifetime), }) diff --git a/internal/commands/auth_agent_connect_test.go b/internal/commands/auth_agent_connect_test.go index e3b5b7033..8ac861bf8 100644 --- a/internal/commands/auth_agent_connect_test.go +++ b/internal/commands/auth_agent_connect_test.go @@ -4,7 +4,9 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" + "io" "net/http" "net/http/httptest" "net/url" @@ -43,6 +45,10 @@ type connectAS struct { // pollStatus, when non-zero, is the poll's status in place of 200: a // refusal, rendered from connection. pollStatus int + + // verificationPath, when set, is the path and query of the link the + // intake hands out, in place of the plain approval page. + verificationPath string } func startConnectAS(t *testing.T) *connectAS { @@ -54,9 +60,13 @@ func startConnectAS(t *testing.T) *connectAS { mux := http.NewServeMux() mux.HandleFunc("/oauth/agent_connections", func(w http.ResponseWriter, _ *http.Request) { + path := "/connect?user_code=WDJB-MJHT" + if as.verificationPath != "" { + path = as.verificationPath + } w.Header().Set("Content-Type", "application/json") fmt.Fprintf(w, `{"device_code":"dev-code-1","user_code":"WDJB-MJHT","verification_uri_complete":%q,"token_uri":%q,"expires_in":600,"interval":1}`, - as.srv.URL+"/connect?user_code=WDJB-MJHT", as.srv.URL+"/oauth/agent_connection_tokens") + as.srv.URL+path, as.srv.URL+"/oauth/agent_connection_tokens") }) mux.HandleFunc("/oauth/agent_connection_tokens", func(w http.ResponseWriter, r *http.Request) { require.NoError(t, r.ParseForm()) @@ -232,14 +242,20 @@ func runAgentConnectJSON(t *testing.T, app *appctx.App, stdout *lockedBuffer, ar // however that is laid out. func jsonValues(t *testing.T, stdout string) []map[string]any { t.Helper() + // Decode to io.EOF rather than while More(): More is for the elements + // of an array or object, and at the top level it stops quietly at a + // stray closing delimiter that a reader of the stream would choke on. dec := json.NewDecoder(strings.NewReader(stdout)) var values []map[string]any - for dec.More() { + for { var v map[string]any - require.NoError(t, dec.Decode(&v), "stdout is JSON values and nothing else: %q", stdout) + err := dec.Decode(&v) + if errors.Is(err, io.EOF) { + return values + } + require.NoError(t, err, "stdout is JSON values and nothing else: %q", stdout) values = append(values, v) } - return values } // TestAuthAgentConnectJSONWritesTheVerificationThenTheResult: under --json @@ -301,6 +317,27 @@ func TestAuthAgentConnectJSONWritesTheVerificationThenTheResult(t *testing.T) { assert.Equal(t, fakeConnectSecret, creds.ClientSecret, "the credential is stored exactly as without --json") } +// TestAuthAgentConnectJSONVerificationCarriesNoControls: the link is what +// the reader shows the person, and the JSON encoder escapes C0 controls but +// passes UTF-8-encoded C1 controls through raw. A link the server laced +// with one (url.Parse accepts it) reaches the line stripped, as the +// terminal copy is, never as the bytes a terminal would act on. +func TestAuthAgentConnectJSONVerificationCarriesNoControls(t *testing.T) { + as := startConnectAS(t) + as.verificationPath = "/connect\u009b31m?user_code=WDJB-MJHT" + app := connectApp(t, as, &config.Config{ActiveProfile: "agent"}) + stdout := &lockedBuffer{} + + stderr, err := runAgentConnectJSON(t, app, stdout, "--device-name", "build-box") + require.NoError(t, err, stderr) + + assert.NotContains(t, stdout.String(), "\u009b", "a C1 control must not reach stdout raw") + lines := jsonValues(t, stdout.String()) + require.Len(t, lines, 2) + assert.Equal(t, as.srv.URL+"/connect31m?user_code=WDJB-MJHT", lines[0]["verification_uri"], + "the link is the copy the terminal shows") +} + // TestAuthAgentConnectJSONOnADeclineWritesOnlyTheVerification: a declined // connection ends with an error, which the CLI renders as the last line, // and nothing is stored. The verification line is already out by then. From 0f01878a93dd2025f01ef0cd208a245ea163e299 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 6 Oct 2026 22:19:21 -0700 Subject: [PATCH 3/6] Stop the ceremony when the verification line can't be written Under --json the line is how the link and the code reach whoever shows them to the person. A stdout that refused it was ignored, and the ceremony went on to open the browser, poll and store a credential that nobody reading the data had been asked to approve. The intake callback can now fail, and a failed write ends the ceremony before anything waits. The test that reads stdout at the poll now hands what it saw across the server's goroutine under a lock. --- internal/auth/agent_connect.go | 12 ++++-- internal/commands/auth_agent.go | 18 +++++---- internal/commands/auth_agent_connect_test.go | 39 +++++++++++++++++++- 3 files changed, 55 insertions(+), 14 deletions(-) diff --git a/internal/auth/agent_connect.go b/internal/auth/agent_connect.go index 9d2c3ff31..cd997181c 100644 --- a/internal/auth/agent_connect.go +++ b/internal/auth/agent_connect.go @@ -163,8 +163,10 @@ type AgentConnectOptions struct { // opened and the wait begins: a caller reading the ceremony as data // learns the link and the code without parsing the operator's half. // It is never given the device code, which is the poll's bearer and - // stays inside this flow. - OnIntake func(AgentConnectIntake) + // stays inside this flow. A non-nil error ends the ceremony before + // the browser is opened or anything waits: the link has not reached + // whoever was to show it, and nothing is stored. + OnIntake func(AgentConnectIntake) error // BeforeStore, when set, runs after the poll has handed over the // credential and the mint has proved it, and before anything is @@ -277,11 +279,13 @@ func (m *Manager) ConnectAgent(ctx context.Context, opts AgentConnectOptions) (* } if opts.OnIntake != nil { - opts.OnIntake(AgentConnectIntake{ + if err := opts.OnIntake(AgentConnectIntake{ VerificationURI: intake.shownURI, UserCode: intake.userCode, ExpiresAt: opts.now().Add(intake.lifetime), - }) + }); err != nil { + return nil, err + } } wait := announceAgentConnection(opts.presentation(), intake, opts.now()) diff --git a/internal/commands/auth_agent.go b/internal/commands/auth_agent.go index 495274c71..a539df324 100644 --- a/internal/commands/auth_agent.go +++ b/internal/commands/auth_agent.go @@ -169,8 +169,10 @@ type agentConnectVerification struct { // writeAgentConnectVerification writes the verification line, one JSON // object terminated by a newline, so a reader can act on it while the -// ceremony is still waiting. -func writeAgentConnectVerification(w io.Writer, intake auth.AgentConnectIntake) { +// ceremony is still waiting. A write that fails ends the ceremony: under +// --json this line is how the link and the code reach whoever shows them +// to the person, so nothing should wait on an approval they never saw. +func writeAgentConnectVerification(w io.Writer, intake auth.AgentConnectIntake) error { line := agentConnectVerification{ Type: "verification", VerificationURI: intake.VerificationURI, @@ -178,10 +180,10 @@ func writeAgentConnectVerification(w io.Writer, intake auth.AgentConnectIntake) ExpiresAt: intake.ExpiresAt.UTC().Format(time.RFC3339), ExpiresIn: max(0, int(time.Until(intake.ExpiresAt).Round(time.Second).Seconds())), } - // Encode cannot fail on a struct of strings and ints, and a write - // that fails is a stdout that is gone, which the result line will - // meet too. - _ = json.NewEncoder(w).Encode(line) + if err := json.NewEncoder(w).Encode(line); err != nil { + return fmt.Errorf("could not write the verification line, so the connection was stopped: %w", err) + } + return nil } // connectAgentProfile runs the agent-connection handshake for the active @@ -197,10 +199,10 @@ func connectAgentProfile(cmd *cobra.Command, app *appctx.App, f agentConnectFlag // Under --json stdout is the data, so the operator's half of the // ceremony — the link, the code, the wait line — goes to stderr. human := w - var onIntake func(auth.AgentConnectIntake) + var onIntake func(auth.AgentConnectIntake) error if f.machine { human = cmd.ErrOrStderr() - onIntake = func(intake auth.AgentConnectIntake) { writeAgentConnectVerification(w, intake) } + onIntake = func(intake auth.AgentConnectIntake) error { return writeAgentConnectVerification(w, intake) } } r := output.NewRendererWithTheme(human, false, tui.ResolveTheme(tui.DetectDark())) diff --git a/internal/commands/auth_agent_connect_test.go b/internal/commands/auth_agent_connect_test.go index 8ac861bf8..690caca66 100644 --- a/internal/commands/auth_agent_connect_test.go +++ b/internal/commands/auth_agent_connect_test.go @@ -221,7 +221,7 @@ func TestAuthAgentConnectNeedsAProfile(t *testing.T) { // runAgentConnectJSON runs the ceremony under --json with stdout and stderr // kept apart, as a program driving it would read them. stdout is a // lockedBuffer so the mock server's handlers can read it mid-ceremony. -func runAgentConnectJSON(t *testing.T, app *appctx.App, stdout *lockedBuffer, args ...string) (string, error) { +func runAgentConnectJSON(t *testing.T, app *appctx.App, stdout io.Writer, args ...string) (string, error) { t.Helper() app.Flags.JSON = true app.Output = output.New(output.Options{Format: output.FormatJSON, Writer: stdout}) @@ -268,16 +268,25 @@ func TestAuthAgentConnectJSONWritesTheVerificationThenTheResult(t *testing.T) { app := connectApp(t, as, &config.Config{ActiveProfile: "agent"}) stdout := &lockedBuffer{} + // The poll handler runs on the server's goroutine, so what it saw is + // handed over under a lock. approve := as.connection - var atPoll string + var ( + atPollMu sync.Mutex + atPoll string + ) as.connection = func() string { + atPollMu.Lock() atPoll = stdout.String() + atPollMu.Unlock() return approve() } stderr, err := runAgentConnectJSON(t, app, stdout, "--device-name", "build-box") require.NoError(t, err, stderr) + atPollMu.Lock() + defer atPollMu.Unlock() require.NotEmpty(t, atPoll, "the verification line is written before the operator approves, not after") require.True(t, strings.HasSuffix(atPoll, "}\n") && strings.Count(atPoll, "\n") == 1, "the verification is one line, newline-terminated, so a reader can act on it at once: %q", atPoll) @@ -338,6 +347,32 @@ func TestAuthAgentConnectJSONVerificationCarriesNoControls(t *testing.T) { "the link is the copy the terminal shows") } +// failingWriter is a stdout that refuses every write: a full disk behind a +// redirect, or an embedding whose sink has failed. +type failingWriter struct{} + +func (failingWriter) Write([]byte) (int, error) { return 0, errors.New("no space left on device") } + +// TestAuthAgentConnectJSONStopsWhenTheVerificationCannotBeWritten: under +// --json the verification line is how the link and the code reach whoever +// shows them to the person, so a stdout that refuses it ends the ceremony +// there: no wait, no poll, nothing stored. +func TestAuthAgentConnectJSONStopsWhenTheVerificationCannotBeWritten(t *testing.T) { + as := startConnectAS(t) + app := connectApp(t, as, &config.Config{ActiveProfile: "agent"}) + + _, err := runAgentConnectJSON(t, app, failingWriter{}, "--device-name", "build-box") + require.Error(t, err) + assert.Contains(t, err.Error(), "no space left on device") + + as.mu.Lock() + polls := len(as.pollForms) + as.mu.Unlock() + assert.Zero(t, polls, "nothing waits on an approval whose link never reached anyone") + assert.Empty(t, as.mints()) + assertNothingStored(t, app, "agent") +} + // TestAuthAgentConnectJSONOnADeclineWritesOnlyTheVerification: a declined // connection ends with an error, which the CLI renders as the last line, // and nothing is stored. The verification line is already out by then. From 10701184bd6a71e366f8fc2c4b06e937497f23eb Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 6 Oct 2026 22:39:14 -0700 Subject: [PATCH 4/6] Keep the deadline the ceremony published The expiry handed to OnIntake, the one the operator is told and the one the poll stops at were each taken from the clock separately, so a slow callback left the poll running past what the verification line said, or opening the browser on a code already dead. ConnectAgent now fixes one deadline as the intake answers and everything counts down to it; a callback that outlived the code ends the ceremony as expired. --- internal/auth/agent_connect.go | 22 +++++++++++++++----- internal/auth/agent_connect_test.go | 31 +++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 5 deletions(-) diff --git a/internal/auth/agent_connect.go b/internal/auth/agent_connect.go index cd997181c..930f47131 100644 --- a/internal/auth/agent_connect.go +++ b/internal/auth/agent_connect.go @@ -278,14 +278,22 @@ func (m *Manager) ConnectAgent(ctx context.Context, opts AgentConnectOptions) (* return nil, err } + // One deadline for the whole ceremony, fixed when the intake answers: + // what OnIntake publishes, what the operator is told and where the + // poll stops are the same instant, however long the caller takes. + intake.expiresAt = opts.now().Add(intake.lifetime) if opts.OnIntake != nil { if err := opts.OnIntake(AgentConnectIntake{ VerificationURI: intake.shownURI, UserCode: intake.userCode, - ExpiresAt: opts.now().Add(intake.lifetime), + ExpiresAt: intake.expiresAt, }); err != nil { return nil, err } + if !opts.now().Before(intake.expiresAt) { + // Nothing left to announce, open or poll. + return nil, agentConnectExpired() + } } wait := announceAgentConnection(opts.presentation(), intake, opts.now()) @@ -412,6 +420,10 @@ type agentIntake struct { tokenURI string lifetime time.Duration interval time.Duration + + // expiresAt is when the code dies, fixed once by ConnectAgent as the + // intake answers; everything after it counts down to this instant. + expiresAt time.Time } // openAgentConnection makes the anonymous intake request and validates @@ -495,7 +507,7 @@ func announceAgentConnection(view *LoginOptions, intake *agentIntake, now time.T view.log("\nConnect this computer to a Basecamp agent\n") view.log(" 1. Open this link on any device") view.log(" " + intake.shownURI) - view.log(" 2. Check that the code shown there matches (expires in " + expiresIn(intake.lifetime) + ")") + view.log(" 2. Check that the code shown there matches (expires in " + expiresIn(intake.expiresAt.Sub(now)) + ")") view.log(" " + intake.userCode) view.log(" 3. Pick the agent this computer acts as, and approve it") view.log("") @@ -504,10 +516,10 @@ func announceAgentConnection(view *LoginOptions, intake *agentIntake, now time.T view.log("") view.announceBrowser(intake.verificationURI) - if wait := startApprovalWait(view.Progress, now.Add(intake.lifetime)); wait != nil { + if wait := startApprovalWait(view.Progress, intake.expiresAt); wait != nil { return wait } - view.log("Waiting for approval… (the code expires in " + expiresIn(intake.lifetime) + ")") + view.log("Waiting for approval… (the code expires in " + expiresIn(intake.expiresAt.Sub(now)) + ")") return nil } @@ -519,7 +531,7 @@ func announceAgentConnection(view *LoginOptions, intake *agentIntake, now time.T // the backstop for a server that answers authorization_pending forever — // the code it issued is dead by then whatever it says. func (m *Manager) awaitAgentConnection(ctx context.Context, opts *AgentConnectOptions, client *http.Client, intake *agentIntake) (*AgentConnection, error) { - deadline := opts.now().Add(intake.lifetime) + deadline := intake.expiresAt interval := intake.interval for { // Never wait past the code's own life: an interval longer than diff --git a/internal/auth/agent_connect_test.go b/internal/auth/agent_connect_test.go index e4da47f27..6f324b6ee 100644 --- a/internal/auth/agent_connect_test.go +++ b/internal/auth/agent_connect_test.go @@ -591,6 +591,37 @@ func TestConnectAgentStopsWhenTheCodeLifetimeRunsOut(t *testing.T) { assertNoAgentCredential(t, m) } +// TestConnectAgentKeepsTheDeadlineItPublished: the code dies when the +// intake said it would, however long the caller took over OnIntake. A +// callback that outlived the code leaves nothing to announce, open or +// poll: the deadline it was given is the one the ceremony keeps. +func TestConnectAgentKeepsTheDeadlineItPublished(t *testing.T) { + as := startConnectAS(t) + m := connectManager(t, as) + clock := newTestClock() + + opts := connectOptions(&collectLogger{}, clock) + opts.NoBrowser = false + opts.Local = true + opts.BrowserLauncher = func(string) error { + t.Fatal("opened the browser on a code that had already expired") + return nil + } + var published time.Time + opts.OnIntake = func(intake AgentConnectIntake) error { + published = intake.ExpiresAt + // The caller takes the code's whole life and a second more. + return clock.sleep(context.Background(), intake.ExpiresAt.Sub(clock.Now())+time.Second) + } + + _, err := m.ConnectAgent(context.Background(), opts) + require.Error(t, err) + assert.Contains(t, err.Error(), "expired") + assert.False(t, published.IsZero()) + assert.Empty(t, as.calls(&as.pollForms), "nothing polls a code past the deadline it was published with") + assertNoAgentCredential(t, m) +} + // TestConnectAgentReportsAnAgentThatIsNoLongerActive: an account canceled // or frozen between approval and the poll answers invalid_grant, and the // remedy is a new connection, not a retry. From 3a4a7959f96f247243d7664d84565e1c6857437f Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 6 Oct 2026 22:39:14 -0700 Subject: [PATCH 5/6] Say what --agent prints second, and prove the refusals ask the server nothing The help called the second value the result envelope in every mode, but --agent prints the data alone. The output-mode refusals claimed to stop before the server was asked anything while only mints were counted; the intake is counted now too. --- internal/commands/auth_agent.go | 5 +++-- internal/commands/auth_agent_connect_test.go | 8 ++++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/internal/commands/auth_agent.go b/internal/commands/auth_agent.go index a539df324..5924f0793 100644 --- a/internal/commands/auth_agent.go +++ b/internal/commands/auth_agent.go @@ -74,8 +74,9 @@ answers the request: {"type":"verification","verification_uri":"…","user_code":"WDJB-MJHT","expires_at":"…","expires_in":600} -The second is the result envelope every command prints: the profile, its -account, the access approved and the agent's client id — or the error, if +The second is the result: the envelope every command prints under --json, +or its data alone under --agent — the profile, its account, the access +approved and the agent's client id — or the error, if the operator declined, the code expired, or nothing could be stored. The words meant for a person go to stderr, and the browser opens as it would without --json. Neither line ever carries the client secret or the device code. diff --git a/internal/commands/auth_agent_connect_test.go b/internal/commands/auth_agent_connect_test.go index 690caca66..5dda615a7 100644 --- a/internal/commands/auth_agent_connect_test.go +++ b/internal/commands/auth_agent_connect_test.go @@ -35,6 +35,7 @@ type connectAS struct { srv *httptest.Server mu sync.Mutex + intakes int pollForms []url.Values tokenForms []url.Values @@ -60,6 +61,9 @@ func startConnectAS(t *testing.T) *connectAS { mux := http.NewServeMux() mux.HandleFunc("/oauth/agent_connections", func(w http.ResponseWriter, _ *http.Request) { + as.mu.Lock() + as.intakes++ + as.mu.Unlock() path := "/connect?user_code=WDJB-MJHT" if as.verificationPath != "" { path = as.verificationPath @@ -412,6 +416,10 @@ func TestAuthAgentConnectRefusesOutputModesThatCannotCarryIt(t *testing.T) { _, err := runAgentConnect(t, app) require.Error(t, err) assert.Equal(t, output.CodeUsage, output.AsError(err).Code) + as.mu.Lock() + intakes := as.intakes + as.mu.Unlock() + assert.Zero(t, intakes, "refused before the intake is asked for a code") assert.Empty(t, as.mints()) }) } From d6a6dfe62b68ccbd7cc53fc68a30ae9253b34b6c Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 6 Oct 2026 23:09:18 -0700 Subject: [PATCH 6/6] Teach the connect skill to read the connection as data The skill ran auth agent connect plainly and scraped the terminal for the link and the code, and its first-time-setup eval forbade --json on the grounds that the connection refuses it, which this branch makes untrue. The skill now runs it with --json and relays the verification line's link, code and expiry, which are already stripped of control characters; auth login and profile create still refuse --json and are run plainly. The eval requires --json, mocks the two JSON values the command writes, and rejects only the output modes the connection still refuses. --agent is no longer rejected: the CLI takes it, and those rules reject only what it refuses. --- .../basecamp-connect/first-time-setup.yml | 20 ++++++++++---- skills/basecamp-connect/SKILL.md | 27 ++++++++++++++----- 2 files changed, 35 insertions(+), 12 deletions(-) diff --git a/skill-evals/cases/basecamp-connect/first-time-setup.yml b/skill-evals/cases/basecamp-connect/first-time-setup.yml index 5fbe18f4e..b24be287a 100644 --- a/skill-evals/cases/basecamp-connect/first-time-setup.yml +++ b/skill-evals/cases/basecamp-connect/first-time-setup.yml @@ -5,8 +5,8 @@ context: >- Everything you need is here, so don't stop to ask: the agent's profile should be called helper, and the agent is called Helper: if the credential turns out to be Helper, I confirm that is the agent. My own profile is jorge. - Only I should be able to drive the agent. When a connection link and code - are printed I open the link and approve the connection. + Only I should be able to drive the agent. When you show me a connection link + and code I open the link and approve the connection. tags: [connect] mocks: - match: 'auth status.*jorge' @@ -19,8 +19,13 @@ mocks: output: '{"ok":false,"code":"api_error","error":"unknown profile \"helper\" (available: jorge)"}' - match: 'profile list' output: '{"ok":true,"data":[{"name":"jorge","account_id":"999","authenticated":true,"base_url":"https://3.basecampapi.com","default":true}],"summary":"1 profile"}' + # Under --json, stdout is two JSON values: the verification line, then the + # result envelope. The person's prose goes to stderr, which the skill does + # not read. - match: 'auth agent connect' - output: "Open this link and enter the code ABCD-EFGH to approve the connection.\nWaiting for approval...\n\nConnected profile \"helper\" to a Basecamp agent\nProfile: helper · Account: 999 · Access: full · Token: minted on demand, no refresh token\nCreated profile \"helper\" for account 999\nCheck it any time: basecamp auth status -P helper" + output: |- + {"type":"verification","verification_uri":"https://3.basecamp.com/connect?user_code=ABCD-EFGH","user_code":"ABCD-EFGH","expires_at":"2026-10-07T12:10:00Z","expires_in":600} + {"ok":true,"data":{"profile":"helper","account_id":"999","base_url":"https://3.basecampapi.com","source":"agent_connection","oauth_type":"agent","scope":"full","client_id":"agent-client","profile_created":true},"summary":"Connected profile \"helper\" to a Basecamp agent"} - match: '^me\b' output: '{"ok":true,"data":{"identity":{"id":7001,"first_name":"Helper","last_name":"","email_address":""},"accounts":[{"id":999,"name":"Acme","current":true}],"person":{"id":4001,"name":"Helper"}}}' - match: 'projects list' @@ -34,6 +39,9 @@ expect_sequence: - match: '^me .*(-P|--profile)[ =]''?helper\b' - match: 'connect setup' accept: + # The connection is read as data: the skill relays the verification line's + # fields rather than scraping the terminal copy. + - 'auth agent connect\b.*\s(--json|-j)(\s|$)' - 'connect setup .*(-P|--profile)[ =]''?helper\b' - 'connect setup .*--operator-profile[ =]''?jorge\b' - 'connect setup .*--serve[ =](''222''|"222"|222)(\s|$)' @@ -88,8 +96,10 @@ reject: - 'connect setup .*--unserve\b' - '--with-token' - '--with-client-credentials' - # Every machine-output mode the interactive connection refuses. - - 'auth agent connect.*(--json|--agent|--quiet|--ids-only|--count|--jq|-j\b|-q\b)' + # The output modes the agent connection refuses: none can carry its + # verification line. --json is required above; --agent is not listed, + # because the CLI takes it, and these rules reject only what it refuses. + - 'auth agent connect.*(--quiet|--ids-only|--count|--jq|-q\b)' - 'connect setup .*--operator[ =]' - 'connect setup .*--(trust[ =]''?(allowlist|project)|allow[ =])' - '--expect-identity' diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index 620b62537..3eae11f0f 100644 --- a/skills/basecamp-connect/SKILL.md +++ b/skills/basecamp-connect/SKILL.md @@ -149,10 +149,22 @@ anything you post. first (`basecamp projects list -P '' --json`), and pass the id. The project called `Launch $(date)` is served as `--serve 222`, never by name. -**Interactive logins.** `basecamp auth agent connect`, `basecamp auth login` and -`basecamp profile create` print a link and wait for a person. Run them without -`--json` or `--quiet`, in the background, and read their output as it arrives, -so you can show the link and code while they wait. +**Interactive logins.** `basecamp auth agent connect` waits for a person to +approve it. Run it with `--json`, in the background, and read its stdout as it +arrives. The first line comes as soon as Basecamp answers, while it waits: + + {"type":"verification","verification_uri":"…","user_code":"WDJB-MJHT","expires_at":"…","expires_in":600} + +Relay `verification_uri`, `user_code` and when it expires to the person right +away. They are already stripped of control characters, so pass them on as they +are. Don't scrape the terminal text. It goes to stderr and is the CLI's copy +for a person at that terminal. The second value is the result envelope: `ok`, +with `data.profile`, `data.account_id` and `data.scope`, or the error and its +hint. + +`basecamp auth login` and `basecamp profile create` refuse `--json`. Run them +plainly, in the background, and read their output as it arrives, so you can +show the link and code while they wait. ## First-time setup @@ -163,9 +175,10 @@ Ask only what you can't find out. '' --json`. - `unknown profile`: it doesn't exist. Connect the agent with - `basecamp auth agent connect -P ''` (add `--no-browser` if the person - is on another device). Show the link and code. They open the link, check the - code, choose the agent (or create one) and approve. + `basecamp auth agent connect -P '' --json` (add `--no-browser` if + the person is on another device). Show them the link and code from its + verification line. They open the link, check the code, choose the agent (or + create one) and approve. - `oauth_type` `agent`: already connected. Don't connect again: that rotates the agent's secret and disconnects any other computer using it. - Anything else: a person's login is stored. Ask; never connect an agent over