From b1ddc87d9089e2d6afb2d3239a319d352ef5e167 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 2 Sep 2026 12:57:28 -0700 Subject: [PATCH 1/8] Import personal access tokens with auth login --with-token MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A bot's Basecamp identity should never come from a browser sign-in on a shared machine: whoever's session the browser holds is who the profile quietly becomes, and nothing checks. Basecamp now mints personal access tokens, so the CLI can take one from a secret store and refuse to keep it unless it authenticates as the expected identity. `basecamp auth login --with-token -P --account ` reads the token from stdin (a terminal is refused, with the `op read … |` shape shown), stores it under the profile as a non-expiring bc5 credential — ExpiresAt 0 is the path AccessToken already never refreshes — and creates the profile when --account names its account. The profile entry is written only after /my/profile.json and the authorization endpoint have answered for the token; a rejected token leaves the profile exactly as it was found, previous credential included. Profile registration is factored out of `profile create` so both paths write the same entry, and the root pre-run lets a may-create command name a profile that does not exist yet. `--expect-identity ` makes every login assertive — browser and device flows included — discarding the new credential on a mismatch. `--json` returns the envelope (profile, account, identity, person, oauth_type, scope, expires_at null), and the interactive flows now refuse machine output modes up front instead of printing prose and blocking (the machine-mode half of #669). `BASECAMP_OAUTH_ISSUER` pins the BC5 authorization server and skips discovery, so a device login reaches a server that is piloting the client while its metadata is still dark. It is a temporary escape hatch and is documented as one. `--login-hint ` is wired through LoginOptions; the pinned SDK cannot put it on the wire yet (basecamp/basecamp-sdk#841 adds the option), so this build tells the user the hint instead of sending it, and Launchpad ignores it. --- .surface | 6 + README.md | 28 ++ e2e/auth.bats | 29 ++ internal/auth/auth.go | 76 +++++ internal/auth/auth_test.go | 28 ++ internal/auth/device_test.go | 133 +++++++- internal/cli/root.go | 42 ++- internal/cli/root_test.go | 36 ++ internal/commands/auth.go | 373 +++++++++++++++++++- internal/commands/auth_login_test.go | 494 +++++++++++++++++++++++++++ internal/commands/profile.go | 124 +++---- internal/stdinarg/stdinarg.go | 10 + internal/stdinarg/stdinarg_test.go | 22 ++ skills/basecamp/SKILL.md | 2 + 14 files changed, 1313 insertions(+), 90 deletions(-) diff --git a/.surface b/.surface index cacb7f11..74489d61 100644 --- a/.surface +++ b/.surface @@ -1929,6 +1929,7 @@ FLAG basecamp auth login --agent type=bool FLAG basecamp auth login --cache-dir type=string FLAG basecamp auth login --count type=bool FLAG basecamp auth login --device-code type=bool +FLAG basecamp auth login --expect-identity type=string FLAG basecamp auth login --help type=bool FLAG basecamp auth login --hints type=bool FLAG basecamp auth login --ids-only type=bool @@ -1936,6 +1937,7 @@ FLAG basecamp auth login --in type=string FLAG basecamp auth login --jq type=string FLAG basecamp auth login --json type=bool FLAG basecamp auth login --local type=bool +FLAG basecamp auth login --login-hint type=string FLAG basecamp auth login --markdown type=bool FLAG basecamp auth login --md type=bool FLAG basecamp auth login --no-browser type=bool @@ -1950,6 +1952,7 @@ FLAG basecamp auth login --stats type=bool FLAG basecamp auth login --styled type=bool FLAG basecamp auth login --todolist type=string FLAG basecamp auth login --verbose type=count +FLAG basecamp auth login --with-token type=bool FLAG basecamp auth logout --account type=string FLAG basecamp auth logout --agent type=bool FLAG basecamp auth logout --cache-dir type=string @@ -10660,6 +10663,7 @@ FLAG basecamp login --agent type=bool FLAG basecamp login --cache-dir type=string FLAG basecamp login --count type=bool FLAG basecamp login --device-code type=bool +FLAG basecamp login --expect-identity type=string FLAG basecamp login --help type=bool FLAG basecamp login --hints type=bool FLAG basecamp login --ids-only type=bool @@ -10667,6 +10671,7 @@ FLAG basecamp login --in type=string FLAG basecamp login --jq type=string FLAG basecamp login --json type=bool FLAG basecamp login --local type=bool +FLAG basecamp login --login-hint type=string FLAG basecamp login --markdown type=bool FLAG basecamp login --md type=bool FLAG basecamp login --no-browser type=bool @@ -10681,6 +10686,7 @@ FLAG basecamp login --stats type=bool FLAG basecamp login --styled type=bool FLAG basecamp login --todolist type=string FLAG basecamp login --verbose type=count +FLAG basecamp login --with-token type=bool FLAG basecamp logout --account type=string FLAG basecamp logout --agent type=bool FLAG basecamp logout --cache-dir type=string diff --git a/README.md b/README.md index 29cb2643..eb710ae4 100644 --- a/README.md +++ b/README.md @@ -194,6 +194,28 @@ basecamp auth login --scope full # Full read+write access (default; ignored by L basecamp auth token # Print token for scripts ``` +`--expect-identity ` makes any login assert who it authenticated as: on a +mismatch the new credential is discarded (a profile's previous credential is +kept) and the command exits non-zero. `--login-hint ` suggests which +account to sign in as on the device-flow approval page. + +### Personal access tokens + +A [personal access token](https://3.basecamp.com/my/access_tokens) can be +imported instead of running OAuth — the shape for bots, CI, and any machine +that should never sign in interactively. The token is read from stdin (never +an argument), stored as a non-expiring credential under a named profile, and +verified against the server before anything is kept: + +```bash +op read "op://Vault/Item/credential" | basecamp auth login --with-token -P bot --account 999 +op read "op://Vault/Item/credential" | basecamp auth login --with-token -P bot --account 999 --expect-identity 12345 --json +``` + +`--account` is required when the profile does not exist yet. `--json` returns +an envelope with the profile, account, identity and person, `oauth_type`, +`scope`, and `expires_at: null`. + ### Multiple Identities Use named profiles when the same machine or agent gateway needs more than one Basecamp identity. Each profile has its own stored OAuth credentials and can be selected per command: @@ -218,6 +240,12 @@ To use your own OAuth app (e.g., a custom Launchpad integration): Both `BASECAMP_OAUTH_CLIENT_ID` and `BASECAMP_OAUTH_CLIENT_SECRET` must be set together. +`BASECAMP_OAUTH_ISSUER=https://3.basecamp.com` pins the OAuth authorization +server and skips discovery, so `basecamp auth login` reaches a server that is +serving piloted clients but not yet advertising itself (discovery still 404s). +It is a temporary escape hatch for that dark pilot, not a configuration +surface, and will be removed once the server advertises its metadata. + ## AI Agent Integration `basecamp` works with any AI agent that can run shell commands. diff --git a/e2e/auth.bats b/e2e/auth.bats index 8c879154..8c25aa44 100644 --- a/e2e/auth.bats +++ b/e2e/auth.bats @@ -90,6 +90,35 @@ load test_helper assert_output_contains "default full" } +@test "basecamp auth login --help shows --with-token, --expect-identity, and --login-hint" { + run basecamp auth login --help + assert_success + assert_output_contains "--with-token" + assert_output_contains "--expect-identity" + assert_output_contains "--login-hint" + assert_output_contains "op read" +} + +@test "basecamp auth login --with-token requires a profile" { + run basecamp auth login --with-token //credential" | basecamp auth login --with-token -P bot --account 999 + ... | basecamp auth login --with-token -P bot --account 999 --expect-identity 12345 --json + +--with-token stores the token under the named profile, creating the profile +when --account is given, then verifies who it authenticates as.`, + Annotations: map[string]string{AnnotationProfileMayCreate: "true"}, RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) if app == nil { return fmt.Errorf("app not initialized") } + if withToken { + return runLoginWithToken(cmd, app, scope, expectIdentity) + } + if app.Flags.JQFilter != "" { return output.ErrJQNotSupported("the login command") } + if machineOutputFlagSet(app) { + return output.ErrUsageHint("Interactive login cannot run under a machine output mode", + "Browser and device logins print instructions and wait for approval, which no envelope can carry. "+ + "Check credentials with `basecamp auth status`, or import a token headlessly: "+ + "`... | basecamp auth login --with-token -P --account --json`.") + } + if err := requireIdentityCheckable(expectIdentity); err != nil { + return err + } + if name := app.Config.ActiveProfile; name != "" { + if _, ok := app.Config.Profiles[name]; !ok { + return output.ErrUsageHint(fmt.Sprintf("Profile %q does not exist", name), + fmt.Sprintf("Create it with `basecamp profile create %s`, or import a token: `... | basecamp auth login --with-token -P %s --account `.", name, name)) + } + } if deviceCode { remote = true @@ -259,11 +310,14 @@ func buildLoginCmd(use string) *cobra.Command { fmt.Fprintln(w, r.Summary.Render("Starting Basecamp authentication...")) } + restore := credentialRestorer(app) + result, err := app.Auth.Login(cmd.Context(), auth.LoginOptions{ Scope: scope, NoBrowser: noBrowser, Remote: remote, Local: local, + LoginHint: loginHint, Logger: func(msg string) { fmt.Fprintln(w, msg) }, }) if err != nil { @@ -277,18 +331,16 @@ func buildLoginCmd(use string) *cobra.Command { fmt.Fprintln(w, r.Muted.Render(fmt.Sprintf("Access: %s", result.Scope))) } - resp, profileErr := app.SDK.Get(cmd.Context(), "/my/profile.json") - if profileErr == nil { - var profile struct { - ID int `json:"id"` - Name string `json:"name"` - Email string `json:"email_address"` - } - if err := resp.UnmarshalData(&profile); err == nil { - if err := app.Auth.SetUserIdentity(fmt.Sprintf("%d", profile.ID), profile.Email); err == nil { - fmt.Fprintln(w, r.Data.Render(fmt.Sprintf("Logged in as: %s", profile.Name))) - } - } + // Without an expectation the identity line is informational, as + // it always was; with one, a credential that cannot be verified + // is not kept. + who, err := verifyLoginIdentity(cmd.Context(), app, expectIdentity, expectIdentity != "") + if err != nil { + restore(cmd.ErrOrStderr()) + return err + } + if who != nil { + fmt.Fprintln(w, r.Data.Render("Logged in as: "+who.label())) } printAgentNudge(w, r) @@ -302,12 +354,307 @@ func buildLoginCmd(use string) *cobra.Command { cmd.Flags().BoolVar(&remote, "remote", false, "Force remote/headless mode (paste callback URL instead of local listener)") cmd.Flags().BoolVar(&local, "local", false, "Force local mode (override SSH auto-detection)") cmd.Flags().BoolVar(&deviceCode, "device-code", false, "Headless authentication with manual browser instructions") + cmd.Flags().BoolVar(&withToken, "with-token", false, "Read a personal access token from stdin instead of running OAuth (requires --profile)") + cmd.Flags().StringVar(&expectIdentity, "expect-identity", "", "Identity ID the login must authenticate as; otherwise discard the credentials") + cmd.Flags().StringVar(&loginHint, "login-hint", "", "Email address to sign in as (device flow only; ignored by Launchpad)") cmd.MarkFlagsMutuallyExclusive("remote", "local") cmd.MarkFlagsMutuallyExclusive("device-code", "local") + for _, flag := range []string{"device-code", "remote", "local", "no-browser", "login-hint"} { + cmd.MarkFlagsMutuallyExclusive("with-token", flag) + } return cmd } +// runLoginWithToken imports a personal access token from stdin as the +// active profile's credential. The profile is registered only after the +// token has proven who it authenticates as, and a token that fails that +// check leaves whatever credential the profile had before. +func runLoginWithToken(cmd *cobra.Command, app *appctx.App, scope, expectIdentity string) error { + if err := requireIdentityCheckable(expectIdentity); err != nil { + return err + } + if os.Getenv("BASECAMP_TOKEN") != "" { + return output.ErrUsageHint("BASECAMP_TOKEN is set", + "Every request, including the identity check, would use it instead of the imported token. Unset it and retry.") + } + + name := app.Config.ActiveProfile + if name == "" { + return output.ErrUsageHint("--with-token stores the token under a named profile", + "Pass -P/--profile ; add --account when the profile does not exist yet.") + } + if !isValidProfileName(name) { + return output.ErrUsage(fmt.Sprintf("Invalid profile name %q: use only letters, numbers, hyphens, and underscores", name)) + } + if scope == "" { + scope = "full" + } + + existing := app.Config.Profiles[name] + var created *config.ProfileConfig + switch { + case existing == nil && app.Flags.Account == "": + return output.ErrUsageHint(fmt.Sprintf("Profile %q does not exist", name), + "Pass --account to create it alongside the imported token.") + case existing == nil: + created = &config.ProfileConfig{BaseURL: app.Config.BaseURL, AccountID: app.Flags.Account} + case app.Flags.Account != "" && existing.AccountID != "" && existing.AccountID != app.Flags.Account: + return output.ErrUsageHint(fmt.Sprintf("Profile %q is bound to account %s, not %s", name, existing.AccountID, app.Flags.Account), + "Import into a different profile, or pass the account the profile is bound to.") + } + + token, err := readTokenFromStdin(cmd) + if err != nil { + return err + } + + restore := credentialRestorer(app) + if err := app.Auth.ImportToken(token, scope); err != nil { + return err + } + + who, err := verifyLoginIdentity(cmd.Context(), app, expectIdentity, true) + if err != nil { + restore(cmd.ErrOrStderr()) + return err + } + // The server's word on the token's scope beats the caller's declaration. + if who.Scope != "" && who.Scope != scope { + scope = who.Scope + if err := setStoredScope(app, scope); err != nil { + restore(cmd.ErrOrStderr()) + return err + } + } + + isDefault := false + if created != nil { + created.Scope = scope + if isDefault, err = registerProfile(name, created); err != nil { + restore(cmd.ErrOrStderr()) + return err + } + if app.Config.Profiles == nil { + app.Config.Profiles = make(map[string]*config.ProfileConfig) + } + app.Config.Profiles[name] = created + } + + accountID := app.Flags.Account + if existing != nil && existing.AccountID != "" { + accountID = existing.AccountID + } + + data := map[string]any{ + "profile": name, + "base_url": app.Config.BaseURL, + "source": "token", + "oauth_type": "bc5", + "scope": scope, + "expires_at": nil, + "person": map[string]any{"id": who.PersonID, "name": who.Name, "email": who.Email}, + "profile_created": created != nil, + } + if accountID != "" { + data["account_id"] = accountID + } + if who.IdentityID != 0 { + data["identity"] = map[string]any{"id": who.IdentityID, "email": who.Email} + } + if isDefault { + data["default"] = true + } + + if app.IsMachineOutput() { + return app.OK(data, output.WithSummary("Logged in as "+who.label())) + } + + w := cmd.OutOrStdout() + r := output.NewRendererWithTheme(w, false, tui.ResolveTheme(tui.DetectDark())) + fmt.Fprintln(w, r.Success.Render("Logged in as "+who.label())) + fmt.Fprintln(w, r.Muted.Render(fmt.Sprintf("Profile: %s · Access: %s · Token: personal access token (does not expire)", name, scope))) + if created != nil { + line := fmt.Sprintf("Created profile %q for account %s", name, created.AccountID) + if isDefault { + line += " (default)" + } + fmt.Fprintln(w, r.Muted.Render(line)) + } + return nil +} + +// readTokenFromStdin reads one access token from the command's stdin. A +// terminal is refused outright — a secret typed at a prompt lands in shell +// and terminal history, and the command exists to be piped from a secret +// store. The token is never echoed, logged, or included in an error. +func readTokenFromStdin(cmd *cobra.Command) (string, error) { + in := cmd.InOrStdin() + if stdinarg.IsTerminal(in) { + return "", output.ErrUsageHint("--with-token reads the token from stdin, and stdin is a terminal", + "Pipe it in from a secret store: `op read \"op:////credential\" | basecamp auth login --with-token -P --account `.") + } + + data, err := io.ReadAll(io.LimitReader(in, maxTokenBytes+1)) + if err != nil { + return "", fmt.Errorf("reading token from stdin: %w", err) + } + if len(data) > maxTokenBytes { + return "", output.ErrUsage(fmt.Sprintf("Token on stdin is longer than %d bytes; expected a single access token", maxTokenBytes)) + } + + token := strings.TrimSpace(string(data)) + if token == "" { + return "", output.ErrUsageHint("No token on stdin", + "Pipe it in from a secret store: `op read \"op:////credential\" | basecamp auth login --with-token -P --account `.") + } + if strings.IndexFunc(token, func(c rune) bool { return unicode.IsSpace(c) || unicode.IsControl(c) }) >= 0 { + return "", output.ErrUsage("Token on stdin must be a single line with no whitespace or control characters") + } + return token, nil +} + +// setStoredScope rewrites the scope on the active credential, keeping +// everything else the login recorded on it. +func setStoredScope(app *appctx.App, scope string) error { + store := app.Auth.GetStore() + key := app.Auth.CredentialKey() + creds, err := store.Load(key) + if err != nil { + return err + } + creds.Scope = scope + return store.Save(key, creds) +} + +// machineOutputFlagSet reports whether an explicit output flag asked for a +// machine format. The config-driven formats are deliberately excluded: a +// configured format=json must not lock a person out of an interactive login. +func machineOutputFlagSet(app *appctx.App) bool { + return app.Flags.Agent || app.Flags.JSON || app.Flags.Quiet || app.Flags.IDsOnly || app.Flags.Count +} + +// requireIdentityCheckable rejects an --expect-identity that could not be +// honored: a malformed ID, or a BASECAMP_TOKEN that every request — the +// identity check included — would use instead of the credential being +// verified. +func requireIdentityCheckable(expectIdentity string) error { + if expectIdentity == "" { + return nil + } + if _, err := strconv.ParseInt(expectIdentity, 10, 64); err != nil { + return output.ErrUsage(fmt.Sprintf("Invalid --expect-identity %q: expected a numeric identity ID", expectIdentity)) + } + if os.Getenv("BASECAMP_TOKEN") != "" { + return output.ErrUsageHint("--expect-identity cannot be checked while BASECAMP_TOKEN is set", + "The identity check would run as the environment token, not the new credential. Unset it and retry.") + } + return nil +} + +// credentialRestorer snapshots the active credential and returns a function +// that puts it back — or removes what a login stored when there was none — +// so a login that fails verification leaves the profile as it found it. +func credentialRestorer(app *appctx.App) func(warn io.Writer) { + store := app.Auth.GetStore() + key := app.Auth.CredentialKey() + prev, err := store.Load(key) + if err != nil { + prev = nil + } + return func(warn io.Writer) { + var restoreErr error + if prev != nil { + restoreErr = store.Save(key, prev) + } else { + restoreErr = store.Delete(key) + } + if restoreErr != nil { + fmt.Fprintf(warn, "Warning: could not restore credentials for %s: %v\n", key, restoreErr) + } + } +} + +// loginIdentity is who a freshly stored credential authenticates as: the +// account-scoped person (from /my/profile.json) and the account-independent +// identity (from the authorization endpoint), which is what --expect-identity +// compares against. +type loginIdentity struct { + PersonID int64 + Name string + Email string + IdentityID int64 + Scope string +} + +func (l *loginIdentity) label() string { + label := l.Name + if l.Email != "" { + label += " <" + l.Email + ">" + } + parts := []string{} + if l.IdentityID != 0 { + parts = append(parts, fmt.Sprintf("identity %d", l.IdentityID)) + } + if l.PersonID != 0 { + parts = append(parts, fmt.Sprintf("person %d", l.PersonID)) + } + if len(parts) > 0 { + label += " (" + strings.Join(parts, ", ") + ")" + } + return label +} + +// verifyLoginIdentity resolves who the active credential authenticates as +// and records it on the credential. With strict set, a lookup failure is the +// caller's error to act on; otherwise it is reported as no identity (nil, +// nil), as the post-login line has always been best-effort. A non-empty +// expectIdentity must match the identity ID, and is checked strictly. +func verifyLoginIdentity(ctx context.Context, app *appctx.App, expectIdentity string, strict bool) (*loginIdentity, error) { + resp, err := app.SDK.Get(ctx, "/my/profile.json") + if err != nil { + if !strict { + return nil, nil + } + return nil, convertSDKError(err) + } + var person struct { + ID int64 `json:"id"` + Name string `json:"name"` + Email string `json:"email_address"` + } + if err := resp.UnmarshalData(&person); err != nil { + if !strict { + return nil, nil + } + return nil, output.ErrAPI(0, fmt.Sprintf("unexpected /my/profile.json response: %v", err)) + } + who := &loginIdentity{PersonID: person.ID, Name: person.Name, Email: person.Email} + + endpoint, err := app.Auth.AuthorizationEndpoint(ctx) + if err == nil { + var info *basecamp.AuthorizationInfo + info, err = app.SDK.Authorization().GetInfo(ctx, &basecamp.GetInfoOptions{Endpoint: endpoint, FilterProduct: "bc3"}) + if err == nil { + who.IdentityID = info.Identity.ID + who.Scope = info.Scope + if who.Email == "" { + who.Email = info.Identity.EmailAddress + } + } + } + if err != nil && expectIdentity != "" { + return nil, output.ErrAuth(fmt.Sprintf("Could not verify the identity of the new credential: %v", err)) + } + + if expectIdentity != "" && strconv.FormatInt(who.IdentityID, 10) != expectIdentity { + return nil, output.ErrAuth(fmt.Sprintf("Authenticated as %s, not identity %s; the credential was not kept", who.label(), expectIdentity)) + } + + _ = app.Auth.SetUserIdentity(strconv.FormatInt(person.ID, 10), who.Email) + return who, nil +} + // buildLogoutCmd constructs a logout command with the given Use name. // Shared by newAuthLogoutCmd ("logout" under auth) and NewLogoutCmd (top-level). func buildLogoutCmd(use string) *cobra.Command { diff --git a/internal/commands/auth_login_test.go b/internal/commands/auth_login_test.go index 1c6fee13..a41f5675 100644 --- a/internal/commands/auth_login_test.go +++ b/internal/commands/auth_login_test.go @@ -3,18 +3,28 @@ package commands import ( "bytes" "context" + "encoding/json" + "fmt" + "io" "net/http" "net/http/httptest" "os" + "path/filepath" + "runtime" + "strings" + "sync" "testing" "time" + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/auth" "github.com/basecamp/basecamp-cli/internal/config" + "github.com/basecamp/basecamp-cli/internal/output" + "github.com/basecamp/basecamp-cli/internal/stdinarg" ) // TestAuthLoginDeviceCodeForcesRemoteMode is the regression test for the @@ -78,3 +88,487 @@ func TestAuthLoginDeviceCodeForcesRemoteMode(t *testing.T) { assert.NotContains(t, output, "Opening browser", "remote mode must not attempt a browser launch") } + +// loginIdentityServer is a mock resource server for the post-login identity +// check: /my/profile.json and /authorization.json answer for exactly one +// bearer token and record what they were sent. +type loginIdentityServer struct { + srv *httptest.Server + + mu sync.Mutex + bearers []string + // identityID is what /authorization.json reports; scope is optional. + identityID int64 + scope string + // authorizationStatus overrides the /authorization.json status when non-zero. + authorizationStatus int +} + +func startLoginIdentityServer(t *testing.T, wantToken string) *loginIdentityServer { + t.Helper() + s := &loginIdentityServer{identityID: 28142355} + mux := http.NewServeMux() + record := func(r *http.Request) bool { + bearer := strings.TrimPrefix(r.Header.Get("Authorization"), "Bearer ") + s.mu.Lock() + s.bearers = append(s.bearers, bearer) + s.mu.Unlock() + return bearer == wantToken + } + mux.HandleFunc("/my/profile.json", func(w http.ResponseWriter, r *http.Request) { + if !record(r) { + w.WriteHeader(http.StatusUnauthorized) + return + } + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"id":51177542,"name":"Clawdito","email_address":"clawdito@example.com"}`) + }) + mux.HandleFunc("/authorization.json", func(w http.ResponseWriter, r *http.Request) { + if !record(r) { + w.WriteHeader(http.StatusUnauthorized) + return + } + if s.authorizationStatus != 0 { + w.WriteHeader(s.authorizationStatus) + return + } + w.Header().Set("Content-Type", "application/json") + scope := "" + if s.scope != "" { + scope = fmt.Sprintf(`,"scope":%q`, s.scope) + } + fmt.Fprintf(w, `{"identity":{"id":%d,"email_address":"clawdito@example.com"},"accounts":[{"id":999,"name":"Acme","href":"%s/999","product":"bc3"}]%s,"expires_at":"2036-01-01T00:00:00Z"}`, s.identityID, s.srv.URL, scope) + }) + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { http.NotFound(w, r) }) + s.srv = httptest.NewServer(mux) + t.Cleanup(s.srv.Close) + return s +} + +func (s *loginIdentityServer) seenBearers() []string { + s.mu.Lock() + defer s.mu.Unlock() + return append([]string(nil), s.bearers...) +} + +// managerTokenProvider feeds the SDK client from the auth manager, as the +// production authAdapter does, so the identity check runs as whatever the +// login just stored. +type managerTokenProvider struct{ mgr *auth.Manager } + +func (p *managerTokenProvider) AccessToken(ctx context.Context) (string, error) { + return p.mgr.AccessToken(ctx) +} + +// loginTestApp is an App wired for --with-token tests: file credential +// store under a temp XDG_CONFIG_HOME, SDK client pointed at the identity +// server, JSON output captured in buf. +func loginTestApp(t *testing.T, srv *loginIdentityServer, cfg *config.Config) (*appctx.App, *bytes.Buffer) { + t.Helper() + t.Setenv("BASECAMP_NO_KEYRING", "1") + t.Setenv("BASECAMP_TOKEN", "") + t.Setenv("BASECAMP_OAUTH_ISSUER", "") + tmpDir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", tmpDir) + + cfg.BaseURL = srv.srv.URL + if cfg.Sources == nil { + cfg.Sources = map[string]string{} + } + authMgr := auth.NewManager(cfg, srv.srv.Client()) + authMgr.SetStore(auth.NewStore(config.GlobalConfigDir())) + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: srv.srv.URL}, &managerTokenProvider{mgr: authMgr}, + basecamp.WithTransport(srv.srv.Client().Transport), + basecamp.WithMaxRetries(1), + ) + buf := &bytes.Buffer{} + app := &appctx.App{ + Config: cfg, + Auth: authMgr, + SDK: sdkClient, + Output: output.New(output.Options{Format: output.FormatJSON, Writer: buf}), + } + return app, buf +} + +// runLogin executes `auth login` with stdin set to in and returns the error +// and everything the command wrote to stdout/stderr. +func runLogin(t *testing.T, app *appctx.App, in io.Reader, args ...string) (string, error) { + t.Helper() + cmd := NewAuthCmd() + cmd.SetArgs(append([]string{"login"}, args...)) + cmd.SetContext(appctx.WithApp(context.Background(), app)) + cmd.SetIn(in) + var out bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&out) + cmd.SilenceErrors = true + cmd.SilenceUsage = true + err := cmd.Execute() + return out.String(), err +} + +func readGlobalConfig(t *testing.T) map[string]any { + t.Helper() + data, err := os.ReadFile(filepath.Join(config.GlobalConfigDir(), "config.json")) + if os.IsNotExist(err) { + return map[string]any{} + } + require.NoError(t, err) + var cfg map[string]any + require.NoError(t, json.Unmarshal(data, &cfg)) + return cfg +} + +func TestAuthLoginWithTokenCreatesProfileAndVerifiesIdentity(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + app.Flags.Account = "999" + + // A trailing newline is what `op read` and `echo` deliver; it must not + // reach the Authorization header. + out, err := runLogin(t, app, strings.NewReader("bc_at_secret\n"), "--with-token") + require.NoError(t, err, out) + + assert.Contains(t, out, "Logged in as Clawdito (identity 28142355, person 51177542)") + assert.Contains(t, out, `Created profile "bot" for account 999 (default)`) + assert.NotContains(t, out, "bc_at_secret", "the token must never be echoed") + for _, bearer := range srv.seenBearers() { + assert.Equal(t, "bc_at_secret", bearer) + } + + creds, err := app.Auth.GetStore().Load("profile:bot") + require.NoError(t, err) + assert.Equal(t, "bc_at_secret", creds.AccessToken) + assert.Zero(t, creds.ExpiresAt, "a personal access token is stored as non-expiring") + assert.Empty(t, creds.RefreshToken) + assert.Equal(t, "bc5", creds.OAuthType) + assert.Equal(t, "full", creds.Scope) + assert.Equal(t, "51177542", creds.UserID) + assert.Equal(t, "clawdito@example.com", creds.UserEmail) + + // Non-expiring: AccessToken serves it without attempting a refresh + // (there is no refresh token or token endpoint to attempt one with). + tok, err := app.Auth.AccessToken(context.Background()) + require.NoError(t, err) + assert.Equal(t, "bc_at_secret", tok) + + cfgFile := readGlobalConfig(t) + profiles := cfgFile["profiles"].(map[string]any) + bot := profiles["bot"].(map[string]any) + assert.Equal(t, "999", bot["account_id"]) + assert.Equal(t, srv.srv.URL, bot["base_url"]) + assert.Equal(t, "full", bot["scope"]) + assert.Equal(t, "bot", cfgFile["default_profile"], "the first profile becomes the default") + assert.Equal(t, "999", app.Config.Profiles["bot"].AccountID, "the in-memory config learns the profile too") +} + +func TestAuthLoginWithTokenJSONEnvelope(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, buf := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + app.Flags.Account = "999" + app.Flags.JSON = true + + out, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token", "--expect-identity", "28142355") + require.NoError(t, err, out) + assert.Empty(t, out, "machine mode writes the envelope only") + + var envelope struct { + OK bool `json:"ok"` + Data map[string]any `json:"data"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope), buf.String()) + assert.True(t, envelope.OK) + data := envelope.Data + assert.Equal(t, "bot", data["profile"]) + assert.Equal(t, "999", data["account_id"]) + assert.Equal(t, "token", data["source"]) + assert.Equal(t, "bc5", data["oauth_type"]) + assert.Equal(t, "full", data["scope"]) + assert.Contains(t, data, "expires_at") + assert.Nil(t, data["expires_at"]) + assert.Equal(t, true, data["profile_created"]) + assert.Equal(t, true, data["default"]) + assert.Equal(t, float64(28142355), data["identity"].(map[string]any)["id"]) + assert.Equal(t, "clawdito@example.com", data["identity"].(map[string]any)["email"]) + assert.Equal(t, float64(51177542), data["person"].(map[string]any)["id"]) + assert.Equal(t, "Clawdito", data["person"].(map[string]any)["name"]) + assert.NotContains(t, buf.String(), "bc_at_secret") +} + +func TestAuthLoginWithTokenExpectIdentityMismatchLeavesNothingBehind(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + app.Flags.Account = "999" + + out, err := runLogin(t, app, strings.NewReader("bc_at_secret\n"), "--with-token", "--expect-identity", "1") + require.Error(t, err) + assert.Contains(t, err.Error(), "Authenticated as Clawdito (identity 28142355, person 51177542), not identity 1") + assert.NotContains(t, err.Error()+out, "bc_at_secret") + + var outErr *output.Error + require.ErrorAs(t, err, &outErr) + assert.Equal(t, output.CodeAuth, outErr.Code) + + _, loadErr := app.Auth.GetStore().Load("profile:bot") + assert.Error(t, loadErr, "the rejected credential must be deleted") + assert.NotContains(t, readGlobalConfig(t), "profiles", "no profile entry may be registered for a rejected credential") + assert.NotContains(t, app.Config.Profiles, "bot") +} + +func TestAuthLoginWithTokenMismatchRestoresPreviousCredentials(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_new") + cfg := &config.Config{ + ActiveProfile: "bot", + Profiles: map[string]*config.ProfileConfig{"bot": {AccountID: "999"}}, + } + app, _ := loginTestApp(t, srv, cfg) + prev := &auth.Credentials{AccessToken: "bc_at_old", RefreshToken: "old-refresh", OAuthType: "bc5", Scope: "full", ExpiresAt: 4102444800} + require.NoError(t, app.Auth.GetStore().Save("profile:bot", prev)) + + _, err := runLogin(t, app, strings.NewReader("bc_at_new"), "--with-token", "--expect-identity", "1") + require.Error(t, err) + + creds, loadErr := app.Auth.GetStore().Load("profile:bot") + require.NoError(t, loadErr, "the profile's previous credential must survive a rejected import") + assert.Equal(t, prev, creds) +} + +func TestAuthLoginWithTokenFailsClosedWhenIdentityUnverifiable(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + srv.authorizationStatus = http.StatusUnauthorized + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + app.Flags.Account = "999" + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token", "--expect-identity", "28142355") + require.Error(t, err) + assert.Contains(t, err.Error(), "Could not verify the identity") + _, loadErr := app.Auth.GetStore().Load("profile:bot") + assert.Error(t, loadErr) + + // Without an expectation the identity endpoint is informational: the + // person from /my/profile.json is enough to keep the credential. + app, _ = loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + app.Flags.Account = "999" + out, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.NoError(t, err, out) + assert.Contains(t, out, "Logged in as Clawdito (person 51177542)") +} + +func TestAuthLoginWithTokenRejectedTokenIsNotKept(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + app.Flags.Account = "999" + + _, err := runLogin(t, app, strings.NewReader("bc_at_wrong"), "--with-token") + require.Error(t, err) + _, loadErr := app.Auth.GetStore().Load("profile:bot") + assert.Error(t, loadErr, "a token the server rejects must not be stored") + assert.NotContains(t, readGlobalConfig(t), "profiles") +} + +func TestAuthLoginWithTokenScopeReportedByServerWins(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + srv.scope = "read" + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + app.Flags.Account = "999" + + out, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.NoError(t, err, out) + creds, err := app.Auth.GetStore().Load("profile:bot") + require.NoError(t, err) + assert.Equal(t, "read", creds.Scope) + assert.Equal(t, "51177542", creds.UserID, "identity survives the scope correction") + assert.Equal(t, "read", app.Config.Profiles["bot"].Scope) +} + +func TestAuthLoginWithTokenRequiresAccountToCreateProfile(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), `Profile "bot" does not exist`) + assert.Contains(t, err.Error(), "--account") + assert.Empty(t, srv.seenBearers(), "nothing may be sent before the profile question is settled") + _, loadErr := app.Auth.GetStore().Load("profile:bot") + assert.Error(t, loadErr) +} + +func TestAuthLoginWithTokenRequiresProfile(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{}) + app.Flags.Account = "999" + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), "named profile") + assert.Contains(t, err.Error(), "--profile") + assert.Empty(t, srv.seenBearers()) +} + +func TestAuthLoginWithTokenRejectsAccountMismatchOnExistingProfile(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + cfg := &config.Config{ + ActiveProfile: "bot", + Profiles: map[string]*config.ProfileConfig{"bot": {AccountID: "111"}}, + } + app, _ := loginTestApp(t, srv, cfg) + app.Flags.Account = "222" + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), `Profile "bot" is bound to account 111, not 222`) + assert.Empty(t, srv.seenBearers()) +} + +func TestAuthLoginWithTokenExistingProfileKeepsItsEntry(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + cfg := &config.Config{ + ActiveProfile: "bot", + Profiles: map[string]*config.ProfileConfig{"bot": {AccountID: "999", ProjectID: "42"}}, + } + app, _ := loginTestApp(t, srv, cfg) + require.NoError(t, os.MkdirAll(config.GlobalConfigDir(), 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(config.GlobalConfigDir(), "config.json"), + []byte(`{"profiles":{"bot":{"base_url":"https://3.basecampapi.com","account_id":"999","project_id":"42"}},"default_profile":"human"}`), 0o600)) + + out, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.NoError(t, err, out) + assert.NotContains(t, out, "Created profile") + + cfgFile := readGlobalConfig(t) + bot := cfgFile["profiles"].(map[string]any)["bot"].(map[string]any) + assert.Equal(t, "42", bot["project_id"], "importing into an existing profile must not rewrite its entry") + assert.Equal(t, "human", cfgFile["default_profile"]) +} + +func TestAuthLoginWithTokenRejectsBadStdin(t *testing.T) { + for name, tc := range map[string]struct { + in string + want string + }{ + "empty": {"", "No token on stdin"}, + "whitespace": {" \n\t", "No token on stdin"}, + "two lines": {"bc_at_one\nbc_at_two\n", "single line"}, + "inner space": {"bc_at one", "single line"}, + "control char": {"bc_at\x1bone", "single line"}, + "oversized": {strings.Repeat("x", maxTokenBytes+1), "longer than"}, + } { + t.Run(name, func(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + app.Flags.Account = "999" + + _, err := runLogin(t, app, strings.NewReader(tc.in), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), tc.want) + assert.NotContains(t, err.Error(), "bc_at_", "the rejected input must not be echoed") + assert.Empty(t, srv.seenBearers()) + }) + } +} + +func TestAuthLoginWithTokenRefusesTerminalStdin(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("no /dev/ptmx on Windows") + } + pty, err := os.OpenFile("/dev/ptmx", os.O_RDWR, 0) + if err != nil { + t.Skipf("open /dev/ptmx: %v", err) + } + defer pty.Close() + if !stdinarg.IsTerminal(pty) { + t.Skip("this environment's pty is not a terminal") + } + + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + app.Flags.Account = "999" + + _, err = runLogin(t, app, pty, "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), "stdin is a terminal") + assert.Contains(t, err.Error(), `op read "op:////credential" | basecamp auth login --with-token -P --account `) + assert.Empty(t, srv.seenBearers()) +} + +func TestAuthLoginWithTokenRefusesEnvToken(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + app.Flags.Account = "999" + t.Setenv("BASECAMP_TOKEN", "env-token") + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), "BASECAMP_TOKEN is set") + assert.Empty(t, srv.seenBearers()) +} + +func TestAuthLoginWithTokenRejectsNonNumericExpectIdentity(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + app.Flags.Account = "999" + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token", "--expect-identity", "clawdito") + require.Error(t, err) + assert.Contains(t, err.Error(), "numeric identity ID") + assert.Empty(t, srv.seenBearers()) +} + +func TestAuthLoginWithTokenIsExclusiveWithInteractiveFlags(t *testing.T) { + for _, flag := range []string{"--device-code", "--remote", "--local", "--no-browser", "--login-hint=x@y"} { + t.Run(flag, func(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token", flag) + require.Error(t, err) + assert.Contains(t, err.Error(), "with-token") + }) + } +} + +// TestAuthLoginRefusesMachineOutputForInteractiveFlows covers the machine-mode +// half of #669: a browser or device login under --json/--agent used to print +// prose and block on approval. It now refuses before touching the network. +func TestAuthLoginRefusesMachineOutputForInteractiveFlows(t *testing.T) { + for name, set := range map[string]func(*appctx.App){ + "json": func(a *appctx.App) { a.Flags.JSON = true }, + "agent": func(a *appctx.App) { a.Flags.Agent = true }, + "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) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{}) + set(app) + _, err := runLogin(t, app, strings.NewReader(""), "--device-code") + require.Error(t, err) + assert.Contains(t, err.Error(), "machine output mode") + assert.Contains(t, err.Error(), "--with-token") + assert.Empty(t, srv.seenBearers()) + }) + } +} + +func TestAuthLoginUnknownProfileNeedsCreateOrToken(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "ghost"}) + + _, err := runLogin(t, app, strings.NewReader(""), "--device-code") + require.Error(t, err) + assert.Contains(t, err.Error(), `Profile "ghost" does not exist`) + assert.Contains(t, err.Error(), "basecamp profile create ghost") + assert.Contains(t, err.Error(), "--with-token -P ghost") +} + +func TestAuthLoginExpectIdentityRefusesEnvToken(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{}) + t.Setenv("BASECAMP_TOKEN", "env-token") + + _, err := runLogin(t, app, strings.NewReader(""), "--device-code", "--expect-identity", "1") + require.Error(t, err) + assert.Contains(t, err.Error(), "BASECAMP_TOKEN is set") +} diff --git a/internal/commands/profile.go b/internal/commands/profile.go index 81b65865..feaa75b9 100644 --- a/internal/commands/profile.go +++ b/internal/commands/profile.go @@ -269,43 +269,8 @@ Examples: profileCfg.Scope = loginResult.Scope } - configPath := filepath.Join(config.GlobalConfigDir(), "config.json") - if err := os.MkdirAll(config.GlobalConfigDir(), 0700); err != nil { - return fmt.Errorf("failed to create config directory: %w", err) - } - - configData := make(map[string]any) - if data, err := os.ReadFile(configPath); err == nil { //nolint:gosec // G304: Path is from trusted config location - _ = json.Unmarshal(data, &configData) - } - - // Get or create profiles map - profilesMap, _ := configData["profiles"].(map[string]any) - if profilesMap == nil { - profilesMap = make(map[string]any) - } - - // Add profile with effective scope - profileEntry := map[string]any{ - "base_url": profileCfg.BaseURL, - } - if profileCfg.AccountID != "" { - profileEntry["account_id"] = profileCfg.AccountID - } - if profileCfg.Scope != "" { - profileEntry["scope"] = profileCfg.Scope - } - profilesMap[name] = profileEntry - configData["profiles"] = profilesMap - - // If this is the first profile, set it as default - isDefault := len(profilesMap) == 1 - if isDefault { - configData["default_profile"] = name - } - - // Write config atomically - if err := atomicWriteJSON(configPath, configData); err != nil { + isDefault, err := registerProfile(name, profileCfg) + if err != nil { return err } @@ -378,27 +343,7 @@ func newProfileDeleteCmd() *cobra.Command { fmt.Fprintf(cmd.ErrOrStderr(), "Warning: could not delete credentials for profile %q: %v\n", name, err) } - // Update config file - configPath := filepath.Join(config.GlobalConfigDir(), "config.json") - configData := make(map[string]any) - if data, err := os.ReadFile(configPath); err == nil { //nolint:gosec // G304: Path is from trusted config location - _ = json.Unmarshal(data, &configData) - } - - if profilesMap, ok := configData["profiles"].(map[string]any); ok { - delete(profilesMap, name) - if len(profilesMap) == 0 { - delete(configData, "profiles") - } - } - - // Clear default_profile if it was this profile - if dp, ok := configData["default_profile"].(string); ok && dp == name { - delete(configData, "default_profile") - } - - // Write config back atomically - if err := atomicWriteJSON(configPath, configData); err != nil { + if err := unregisterProfile(name); err != nil { return err } @@ -453,6 +398,69 @@ func newProfileSetDefaultCmd() *cobra.Command { } } +// registerProfile adds a profile entry to the global config file. The first +// profile registered becomes the default; isDefault reports whether this one +// did. The in-memory config is the caller's to update. +func registerProfile(name string, p *config.ProfileConfig) (isDefault bool, err error) { + configPath := filepath.Join(config.GlobalConfigDir(), "config.json") + if err := os.MkdirAll(config.GlobalConfigDir(), 0700); err != nil { + return false, fmt.Errorf("failed to create config directory: %w", err) + } + + configData := make(map[string]any) + if data, err := os.ReadFile(configPath); err == nil { //nolint:gosec // G304: Path is from trusted config location + _ = json.Unmarshal(data, &configData) + } + + profilesMap, _ := configData["profiles"].(map[string]any) + if profilesMap == nil { + profilesMap = make(map[string]any) + } + + entry := map[string]any{ + "base_url": p.BaseURL, + } + if p.AccountID != "" { + entry["account_id"] = p.AccountID + } + if p.Scope != "" { + entry["scope"] = p.Scope + } + profilesMap[name] = entry + configData["profiles"] = profilesMap + + isDefault = len(profilesMap) == 1 + if isDefault { + configData["default_profile"] = name + } + + return isDefault, atomicWriteJSON(configPath, configData) +} + +// unregisterProfile removes a profile entry from the global config file, +// clearing default_profile when it named this profile. Credentials are the +// caller's to remove. +func unregisterProfile(name string) error { + configPath := filepath.Join(config.GlobalConfigDir(), "config.json") + configData := make(map[string]any) + if data, err := os.ReadFile(configPath); err == nil { //nolint:gosec // G304: Path is from trusted config location + _ = json.Unmarshal(data, &configData) + } + + if profilesMap, ok := configData["profiles"].(map[string]any); ok { + delete(profilesMap, name) + if len(profilesMap) == 0 { + delete(configData, "profiles") + } + } + + if dp, ok := configData["default_profile"].(string); ok && dp == name { + delete(configData, "default_profile") + } + + return atomicWriteJSON(configPath, configData) +} + // validProfileName matches letters, numbers, hyphens, and underscores. var validProfileName = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9_-]*$`) diff --git a/internal/stdinarg/stdinarg.go b/internal/stdinarg/stdinarg.go index 2666f131..66fecfd2 100644 --- a/internal/stdinarg/stdinarg.go +++ b/internal/stdinarg/stdinarg.go @@ -94,6 +94,16 @@ func IsPiped(r io.Reader) bool { return fi.Mode()&os.ModeCharDevice == 0 } +// IsTerminal reports whether the reader is an interactive terminal. A +// non-*os.File reader — the cmd.SetIn test seam — never is. Unlike IsPiped +// this asks term.IsTerminal, so /dev/null (a character device that delivers +// nothing) is not a terminal: a secret redirected from it should read as +// empty, not be refused as typed. +func IsTerminal(r io.Reader) bool { + f, ok := r.(*os.File) + return ok && isTerminal(f) +} + // InteractiveStdio reports whether both stdout and stdin are terminals — the // floor for launching anything that draws to the terminal and reads // keystrokes. A TUI (picker, wizard) reads key events from stdin, so a pipe or diff --git a/internal/stdinarg/stdinarg_test.go b/internal/stdinarg/stdinarg_test.go index 78d935a7..c6abf5be 100644 --- a/internal/stdinarg/stdinarg_test.go +++ b/internal/stdinarg/stdinarg_test.go @@ -161,3 +161,25 @@ func TestPredicatesDisagreeOnTheStreamTheyAskAbout(t *testing.T) { assert.False(t, InteractiveStdio(), "a piped stdout is no place for a picker") assert.True(t, InteractivePrompt(), "but a form draws to stderr, which is still a terminal") } + +// TestIsTerminal: only a real terminal counts. A buffer (the cmd.SetIn +// seam), a pipe, and /dev/null — a character device that delivers nothing — +// are all "not a terminal", so a secret redirected from any of them is read +// rather than refused as typed. +func TestIsTerminal(t *testing.T) { + assert.False(t, IsTerminal(strings.NewReader("token"))) + + devnull, err := os.Open(os.DevNull) + require.NoError(t, err) + defer devnull.Close() + assert.False(t, IsTerminal(devnull)) + + pipeR, pipeW, err := os.Pipe() + require.NoError(t, err) + defer pipeR.Close() + defer pipeW.Close() + assert.False(t, IsTerminal(pipeR)) + + pty := openPTY(t) + assert.True(t, IsTerminal(pty)) +} diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index ca867626..c1fe7a95 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -1317,6 +1317,8 @@ basecamp auth login # Re-authenticate basecamp auth login --scope full # Full access (the default; ignored by Launchpad) basecamp auth login --scope read # Read-only access (ignored by Launchpad) basecamp auth login --device-code # Headless authentication with manual browser instructions +basecamp auth login --with-token -P bot --account # Import a personal access token from stdin (pipe it in) +basecamp auth login --expect-identity # Discard the login unless it authenticated as this identity ``` **Network errors / localhost URLs:** From 14c33b8a1a129636590b0c0e4073ac9ed3413580 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 2 Sep 2026 13:17:38 -0700 Subject: [PATCH 2/8] Verify a login before storing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the token import found the rollback doing the wrong job: the credential was written under the live profile key, checked, and then restored or deleted — a window where another command could pick up an unverified token, a restore that could itself fail, and a success line printed before the check. Turn it around: the candidate token gets a client of its own (App.SDKClientFor, same transport, hooks and user agent as the real one), proves itself, and only then is stored. LoginOptions.Verify runs that check inside the device and Launchpad flows before their store.Save, so --expect-identity on a browser login stores nothing on a mismatch; the snapshot/restore code is gone. The check now covers the account, not only the identity. Production serves /my/profile.json only under an account prefix (the unscoped path 404s — the old "Logged in as" line had been failing silently), so the person lookup goes through the account-scoped People().Me, and that account must be numeric, must be the effective one (--account or BASECAMP_ACCOUNT_ID override the profile binding at runtime, so the mismatch guard compares against that), must appear non-expired in the authorization document, and for a new profile must have been given explicitly rather than inherited from the operator's config. A reported scope outside read/full is refused rather than stored. Smaller findings from the same round: --scope and --expect-identity are validated before stdin is consumed; --expect-identity compares parsed integers; the envelope reports the identity's own email and marks an existing default profile as default; server-supplied names, the login hint and the pinned issuer are reduced to one line before reaching the terminal; an unregistered profile name is no longer used as a cache path component; the e2e cases assert exact error and code with BASECAMP_PROFILE cleared; README and the doctor skill describe the token import, the identity assertion, and that --login-hint is announced, not sent. --- README.md | 15 +- e2e/auth.bats | 10 +- internal/appctx/context.go | 33 ++- internal/auth/auth.go | 60 +++- internal/auth/auth_test.go | 68 +++++ internal/auth/device_test.go | 80 ++++- internal/cli/root.go | 6 +- internal/cli/root_test.go | 28 ++ internal/commands/auth.go | 358 +++++++++++++---------- internal/commands/auth_login_test.go | 419 +++++++++++++++++++++------ skills/basecamp-doctor/SKILL.md | 6 +- 11 files changed, 787 insertions(+), 296 deletions(-) diff --git a/README.md b/README.md index eb710ae4..131f29f5 100644 --- a/README.md +++ b/README.md @@ -194,18 +194,21 @@ basecamp auth login --scope full # Full read+write access (default; ignored by L basecamp auth token # Print token for scripts ``` -`--expect-identity ` makes any login assert who it authenticated as: on a -mismatch the new credential is discarded (a profile's previous credential is -kept) and the command exits non-zero. `--login-hint ` suggests which -account to sign in as on the device-flow approval page. +`--expect-identity ` makes any login assert who it authenticated as: the +new credential is checked before it is stored, and on a mismatch nothing is +written (a profile's previous credential is untouched) and the command exits +non-zero. `--login-hint ` names the account to sign in as on the +device-flow approval page; this build tells you the hint rather than sending +it to the server, so the approval page is not preselected. ### Personal access tokens A [personal access token](https://3.basecamp.com/my/access_tokens) can be imported instead of running OAuth — the shape for bots, CI, and any machine that should never sign in interactively. The token is read from stdin (never -an argument), stored as a non-expiring credential under a named profile, and -verified against the server before anything is kept: +an argument), verified against the server — who it authenticates as, and that +it can reach the profile's account — and only then stored as a non-expiring +credential under a named profile: ```bash op read "op://Vault/Item/credential" | basecamp auth login --with-token -P bot --account 999 diff --git a/e2e/auth.bats b/e2e/auth.bats index 8c25aa44..69abd686 100644 --- a/e2e/auth.bats +++ b/e2e/auth.bats @@ -100,17 +100,17 @@ load test_helper } @test "basecamp auth login --with-token requires a profile" { - run basecamp auth login --with-token //credential" | basecamp auth login --with-token -P bot --account 999 ... | basecamp auth login --with-token -P bot --account 999 --expect-identity 12345 --json ---with-token stores the token under the named profile, creating the profile -when --account is given, then verifies who it authenticates as.`, +--with-token verifies the token against the server — who it authenticates as, +and that it can reach the profile's account — before storing it under the +named profile, creating the profile when --account is given. + +--login-hint names the account to sign in as on the device-flow approval page. +This build tells you the hint instead of sending it to the server.`, Annotations: map[string]string{AnnotationProfileMayCreate: "true"}, RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) @@ -274,8 +279,13 @@ when --account is given, then verifies who it authenticates as.`, return fmt.Errorf("app not initialized") } + expect, err := parseExpectIdentity(expectIdentity) + if err != nil { + return err + } + if withToken { - return runLoginWithToken(cmd, app, scope, expectIdentity) + return runLoginWithToken(cmd, app, scope, expect) } if app.Flags.JQFilter != "" { @@ -287,8 +297,8 @@ when --account is given, then verifies who it authenticates as.`, "Check credentials with `basecamp auth status`, or import a token headlessly: "+ "`... | basecamp auth login --with-token -P --account --json`.") } - if err := requireIdentityCheckable(expectIdentity); err != nil { - return err + if expect != 0 && os.Getenv("BASECAMP_TOKEN") != "" { + return errEnvTokenShadows("--expect-identity cannot be checked while BASECAMP_TOKEN is set") } if name := app.Config.ActiveProfile; name != "" { if _, ok := app.Config.Profiles[name]; !ok { @@ -310,8 +320,10 @@ when --account is given, then verifies who it authenticates as.`, fmt.Fprintln(w, r.Summary.Render("Starting Basecamp authentication...")) } - restore := credentialRestorer(app) - + // With an expectation the login is assertive: the token is + // checked before it is stored, and a mismatch stores nothing. + // Without one the identity line stays informational. + verifier := &loginVerifier{app: app, expectIdentity: expect, account: app.Config.AccountID, strict: expect != 0} result, err := app.Auth.Login(cmd.Context(), auth.LoginOptions{ Scope: scope, NoBrowser: noBrowser, @@ -319,6 +331,7 @@ when --account is given, then verifies who it authenticates as.`, Local: local, LoginHint: loginHint, Logger: func(msg string) { fmt.Fprintln(w, msg) }, + Verify: verifier.verify, }) if err != nil { return err @@ -331,15 +344,8 @@ when --account is given, then verifies who it authenticates as.`, fmt.Fprintln(w, r.Muted.Render(fmt.Sprintf("Access: %s", result.Scope))) } - // Without an expectation the identity line is informational, as - // it always was; with one, a credential that cannot be verified - // is not kept. - who, err := verifyLoginIdentity(cmd.Context(), app, expectIdentity, expectIdentity != "") - if err != nil { - restore(cmd.ErrOrStderr()) - return err - } - if who != nil { + if who := verifier.who; who != nil { + _ = app.Auth.SetUserIdentity(strconv.FormatInt(who.PersonID, 10), who.Email) fmt.Fprintln(w, r.Data.Render("Logged in as: "+who.label())) } @@ -355,8 +361,8 @@ when --account is given, then verifies who it authenticates as.`, cmd.Flags().BoolVar(&local, "local", false, "Force local mode (override SSH auto-detection)") cmd.Flags().BoolVar(&deviceCode, "device-code", false, "Headless authentication with manual browser instructions") cmd.Flags().BoolVar(&withToken, "with-token", false, "Read a personal access token from stdin instead of running OAuth (requires --profile)") - cmd.Flags().StringVar(&expectIdentity, "expect-identity", "", "Identity ID the login must authenticate as; otherwise discard the credentials") - cmd.Flags().StringVar(&loginHint, "login-hint", "", "Email address to sign in as (device flow only; ignored by Launchpad)") + cmd.Flags().StringVar(&expectIdentity, "expect-identity", "", "Identity ID the login must authenticate as; otherwise store nothing") + cmd.Flags().StringVar(&loginHint, "login-hint", "", "Email address to sign in as (device flow only; announced, not yet sent; ignored by Launchpad)") cmd.MarkFlagsMutuallyExclusive("remote", "local") cmd.MarkFlagsMutuallyExclusive("device-code", "local") for _, flag := range []string{"device-code", "remote", "local", "no-browser", "login-hint"} { @@ -367,16 +373,19 @@ when --account is given, then verifies who it authenticates as.`, } // runLoginWithToken imports a personal access token from stdin as the -// active profile's credential. The profile is registered only after the -// token has proven who it authenticates as, and a token that fails that -// check leaves whatever credential the profile had before. -func runLoginWithToken(cmd *cobra.Command, app *appctx.App, scope, expectIdentity string) error { - if err := requireIdentityCheckable(expectIdentity); err != nil { - return err +// active profile's credential. Everything that can be checked without the +// token is checked before stdin is read; the token is then verified through +// a client of its own — identity, and access to the profile's account — +// and only a token that passes is stored and its profile registered. +func runLoginWithToken(cmd *cobra.Command, app *appctx.App, scope string, expect int64) error { + if scope == "" { + scope = "full" + } + if scope != "read" && scope != "full" { + return output.ErrUsage("Invalid scope. Use 'read' or 'full'") } if os.Getenv("BASECAMP_TOKEN") != "" { - return output.ErrUsageHint("BASECAMP_TOKEN is set", - "Every request, including the identity check, would use it instead of the imported token. Unset it and retry.") + return errEnvTokenShadows("BASECAMP_TOKEN is set") } name := app.Config.ActiveProfile @@ -387,53 +396,51 @@ func runLoginWithToken(cmd *cobra.Command, app *appctx.App, scope, expectIdentit if !isValidProfileName(name) { return output.ErrUsage(fmt.Sprintf("Invalid profile name %q: use only letters, numbers, hyphens, and underscores", name)) } - if scope == "" { - scope = "full" - } + // The effective account is what every later command will address, so + // it is what the token must be able to reach: the profile's binding, + // overridden by --account / BASECAMP_ACCOUNT_ID exactly as at runtime. + account := app.Config.AccountID existing := app.Config.Profiles[name] var created *config.ProfileConfig switch { - case existing == nil && app.Flags.Account == "": + case existing == nil && !accountGivenExplicitly(app): return output.ErrUsageHint(fmt.Sprintf("Profile %q does not exist", name), "Pass --account to create it alongside the imported token.") case existing == nil: - created = &config.ProfileConfig{BaseURL: app.Config.BaseURL, AccountID: app.Flags.Account} - case app.Flags.Account != "" && existing.AccountID != "" && existing.AccountID != app.Flags.Account: - return output.ErrUsageHint(fmt.Sprintf("Profile %q is bound to account %s, not %s", name, existing.AccountID, app.Flags.Account), - "Import into a different profile, or pass the account the profile is bound to.") + created = &config.ProfileConfig{BaseURL: app.Config.BaseURL, AccountID: account} + case existing.AccountID != "" && account != existing.AccountID: + return output.ErrUsageHint(fmt.Sprintf("Profile %q is bound to account %s, not %s", name, existing.AccountID, account), + "Import into a different profile, or drop the --account / BASECAMP_ACCOUNT_ID override.") } - - token, err := readTokenFromStdin(cmd) - if err != nil { + if err := requireNumericAccount(account); err != nil { return err } - restore := credentialRestorer(app) - if err := app.Auth.ImportToken(token, scope); err != nil { + token, err := readTokenFromStdin(cmd) + if err != nil { return err } - who, err := verifyLoginIdentity(cmd.Context(), app, expectIdentity, true) - if err != nil { - restore(cmd.ErrOrStderr()) + verifier := &loginVerifier{app: app, expectIdentity: expect, account: account, strict: true} + if err := verifier.verify(cmd.Context(), token, "bc5"); err != nil { return err } + who := verifier.who // The server's word on the token's scope beats the caller's declaration. - if who.Scope != "" && who.Scope != scope { + if who.Scope != "" { scope = who.Scope - if err := setStoredScope(app, scope); err != nil { - restore(cmd.ErrOrStderr()) - return err - } } - isDefault := false + if err := app.Auth.ImportToken(token, scope, strconv.FormatInt(who.PersonID, 10), who.Email); err != nil { + return err + } + + isDefault := app.Config.DefaultProfile == name if created != nil { created.Scope = scope if isDefault, err = registerProfile(name, created); err != nil { - restore(cmd.ErrOrStderr()) - return err + return fmt.Errorf("the token was stored for profile %q but the profile entry could not be written (rerun the import): %w", name, err) } if app.Config.Profiles == nil { app.Config.Profiles = make(map[string]*config.ProfileConfig) @@ -441,27 +448,18 @@ func runLoginWithToken(cmd *cobra.Command, app *appctx.App, scope, expectIdentit app.Config.Profiles[name] = created } - accountID := app.Flags.Account - if existing != nil && existing.AccountID != "" { - accountID = existing.AccountID - } - data := map[string]any{ "profile": name, + "account_id": account, "base_url": app.Config.BaseURL, "source": "token", "oauth_type": "bc5", "scope": scope, "expires_at": nil, + "identity": map[string]any{"id": who.IdentityID, "email": who.IdentityEmail}, "person": map[string]any{"id": who.PersonID, "name": who.Name, "email": who.Email}, "profile_created": created != nil, } - if accountID != "" { - data["account_id"] = accountID - } - if who.IdentityID != 0 { - data["identity"] = map[string]any{"id": who.IdentityID, "email": who.Email} - } if isDefault { data["default"] = true } @@ -473,9 +471,9 @@ func runLoginWithToken(cmd *cobra.Command, app *appctx.App, scope, expectIdentit w := cmd.OutOrStdout() r := output.NewRendererWithTheme(w, false, tui.ResolveTheme(tui.DetectDark())) fmt.Fprintln(w, r.Success.Render("Logged in as "+who.label())) - fmt.Fprintln(w, r.Muted.Render(fmt.Sprintf("Profile: %s · Access: %s · Token: personal access token (does not expire)", name, scope))) + fmt.Fprintln(w, r.Muted.Render(fmt.Sprintf("Profile: %s · Account: %s · Access: %s · Token: personal access token (does not expire)", name, account, scope))) if created != nil { - line := fmt.Sprintf("Created profile %q for account %s", name, created.AccountID) + line := fmt.Sprintf("Created profile %q for account %s", name, account) if isDefault { line += " (default)" } @@ -484,6 +482,30 @@ func runLoginWithToken(cmd *cobra.Command, app *appctx.App, scope, expectIdentit return nil } +// accountGivenExplicitly reports whether the effective account came from +// this invocation (--account or BASECAMP_ACCOUNT_ID) rather than a config +// file: a new bot profile must not inherit whatever account the operator's +// own configuration happens to name. +func accountGivenExplicitly(app *appctx.App) bool { + src := config.Source(app.Config.Sources["account_id"]) + return app.Config.AccountID != "" && (src == config.SourceFlag || src == config.SourceEnv) +} + +// requireNumericAccount mirrors App.RequireAccount for an account the login +// is about to bind a profile to: anything but ASCII digits would leave a +// profile every account-scoped command rejects. +func requireNumericAccount(account string) error { + if account == "" { + return output.ErrUsage("Account ID required. Set via --account flag or BASECAMP_ACCOUNT_ID env.") + } + for _, c := range account { + if c < '0' || c > '9' { + return output.ErrUsage(fmt.Sprintf("Invalid account ID %q: must contain only digits", account)) + } + } + return nil +} + // readTokenFromStdin reads one access token from the command's stdin. A // terminal is refused outright — a secret typed at a prompt lands in shell // and terminal history, and the command exists to be piped from a secret @@ -514,19 +536,6 @@ func readTokenFromStdin(cmd *cobra.Command) (string, error) { return token, nil } -// setStoredScope rewrites the scope on the active credential, keeping -// everything else the login recorded on it. -func setStoredScope(app *appctx.App, scope string) error { - store := app.Auth.GetStore() - key := app.Auth.CredentialKey() - creds, err := store.Load(key) - if err != nil { - return err - } - creds.Scope = scope - return store.Save(key, creds) -} - // machineOutputFlagSet reports whether an explicit output flag asked for a // machine format. The config-driven formats are deliberately excluded: a // configured format=json must not lock a person out of an interactive login. @@ -534,63 +543,45 @@ func machineOutputFlagSet(app *appctx.App) bool { return app.Flags.Agent || app.Flags.JSON || app.Flags.Quiet || app.Flags.IDsOnly || app.Flags.Count } -// requireIdentityCheckable rejects an --expect-identity that could not be -// honored: a malformed ID, or a BASECAMP_TOKEN that every request — the -// identity check included — would use instead of the credential being -// verified. -func requireIdentityCheckable(expectIdentity string) error { - if expectIdentity == "" { - return nil +// parseExpectIdentity parses --expect-identity: 0 when absent, the identity +// ID otherwise. Identity IDs are positive, so 0 is free to mean "none". +func parseExpectIdentity(raw string) (int64, error) { + if raw == "" { + return 0, nil } - if _, err := strconv.ParseInt(expectIdentity, 10, 64); err != nil { - return output.ErrUsage(fmt.Sprintf("Invalid --expect-identity %q: expected a numeric identity ID", expectIdentity)) + id, err := strconv.ParseInt(raw, 10, 64) + if err != nil || id <= 0 { + return 0, output.ErrUsage(fmt.Sprintf("Invalid --expect-identity %q: expected a numeric identity ID", raw)) } - if os.Getenv("BASECAMP_TOKEN") != "" { - return output.ErrUsageHint("--expect-identity cannot be checked while BASECAMP_TOKEN is set", - "The identity check would run as the environment token, not the new credential. Unset it and retry.") - } - return nil + return id, nil } -// credentialRestorer snapshots the active credential and returns a function -// that puts it back — or removes what a login stored when there was none — -// so a login that fails verification leaves the profile as it found it. -func credentialRestorer(app *appctx.App) func(warn io.Writer) { - store := app.Auth.GetStore() - key := app.Auth.CredentialKey() - prev, err := store.Load(key) - if err != nil { - prev = nil - } - return func(warn io.Writer) { - var restoreErr error - if prev != nil { - restoreErr = store.Save(key, prev) - } else { - restoreErr = store.Delete(key) - } - if restoreErr != nil { - fmt.Fprintf(warn, "Warning: could not restore credentials for %s: %v\n", key, restoreErr) - } - } +// errEnvTokenShadows names the reason BASECAMP_TOKEN blocks a verified +// login: every request, the verification included, would carry the +// environment token instead of the credential being checked. +func errEnvTokenShadows(msg string) error { + return output.ErrUsageHint(msg, + "Every request, including the identity check, would use the environment token instead of the new credential. Unset it and retry.") } -// loginIdentity is who a freshly stored credential authenticates as: the -// account-scoped person (from /my/profile.json) and the account-independent -// identity (from the authorization endpoint), which is what --expect-identity -// compares against. +// loginIdentity is who a credential authenticates as: the account-independent +// identity from the authorization endpoint — what --expect-identity compares +// against — and the person within the verified account. type loginIdentity struct { - PersonID int64 - Name string - Email string - IdentityID int64 - Scope string + IdentityID int64 + IdentityEmail string + PersonID int64 + Name string + Email string + Scope string } +// label renders the identity for a one-line terminal sink. Name and email +// are server-supplied, so they are reduced to single lines first. func (l *loginIdentity) label() string { - label := l.Name - if l.Email != "" { - label += " <" + l.Email + ">" + label := richtext.SanitizeSingleLine(l.Name) + if email := richtext.SanitizeSingleLine(l.Email); email != "" { + label += " <" + email + ">" } parts := []string{} if l.IdentityID != 0 { @@ -602,57 +593,102 @@ func (l *loginIdentity) label() string { if len(parts) > 0 { label += " (" + strings.Join(parts, ", ") + ")" } - return label + return strings.TrimSpace(label) } -// verifyLoginIdentity resolves who the active credential authenticates as -// and records it on the credential. With strict set, a lookup failure is the -// caller's error to act on; otherwise it is reported as no identity (nil, -// nil), as the post-login line has always been best-effort. A non-empty -// expectIdentity must match the identity ID, and is checked strictly. -func verifyLoginIdentity(ctx context.Context, app *appctx.App, expectIdentity string, strict bool) (*loginIdentity, error) { - resp, err := app.SDK.Get(ctx, "/my/profile.json") +// loginVerifier proves a credential before it is stored. verify runs as +// LoginOptions.Verify (and directly for token imports) with a client bound +// to the candidate token, so nothing it learns comes from a stored +// credential or from BASECAMP_TOKEN. +// +// Strict mode is the assertive login: the authorization endpoint must +// answer, the effective account (when there is one) must be among the +// accounts the token can reach, and the identity must match any +// expectation. Non-strict mode keeps the informational "Logged in as" line +// best-effort: a lookup failure leaves who nil and the login proceeds. +type loginVerifier struct { + app *appctx.App + expectIdentity int64 + account string + strict bool + + who *loginIdentity +} + +func (v *loginVerifier) verify(ctx context.Context, accessToken, oauthType string) error { + client := v.app.SDKClientFor(&basecamp.StaticTokenProvider{Token: accessToken}) + + endpoint, err := v.app.Auth.AuthorizationEndpointFor(oauthType) if err != nil { - if !strict { - return nil, nil + return err + } + info, err := client.Authorization().GetInfo(ctx, &basecamp.GetInfoOptions{Endpoint: endpoint, FilterProduct: "bc3"}) + if err != nil { + if !v.strict { + return nil } - return nil, convertSDKError(err) + return output.ErrAuth(fmt.Sprintf("Could not verify the new credential: %v", err)) } - var person struct { - ID int64 `json:"id"` - Name string `json:"name"` - Email string `json:"email_address"` + + who := &loginIdentity{ + IdentityID: info.Identity.ID, + IdentityEmail: info.Identity.EmailAddress, + Email: info.Identity.EmailAddress, + Name: strings.TrimSpace(info.Identity.FirstName + " " + info.Identity.LastName), + Scope: info.Scope, } - if err := resp.UnmarshalData(&person); err != nil { - if !strict { - return nil, nil - } - return nil, output.ErrAPI(0, fmt.Sprintf("unexpected /my/profile.json response: %v", err)) + if who.Scope != "" && who.Scope != "read" && who.Scope != "full" { + return output.ErrAuth(fmt.Sprintf("The server reports scope %q for this credential; only read or full can be stored", richtext.SanitizeSingleLine(who.Scope))) + } + if v.expectIdentity != 0 && who.IdentityID != v.expectIdentity { + return output.ErrAuth(fmt.Sprintf("Authenticated as %s, not identity %d; nothing was stored", who.label(), v.expectIdentity)) } - who := &loginIdentity{PersonID: person.ID, Name: person.Name, Email: person.Email} - endpoint, err := app.Auth.AuthorizationEndpoint(ctx) - if err == nil { - var info *basecamp.AuthorizationInfo - info, err = app.SDK.Authorization().GetInfo(ctx, &basecamp.GetInfoOptions{Endpoint: endpoint, FilterProduct: "bc3"}) - if err == nil { - who.IdentityID = info.Identity.ID - who.Scope = info.Scope - if who.Email == "" { - who.Email = info.Identity.EmailAddress - } + // The person record is account-scoped (/{account}/my/profile.json), so + // it doubles as the proof the token reaches the account it is about to + // be used for. The authorization document is checked first: a missing + // account there is a clearer answer than a 404 from the person lookup. + if v.account != "" { + if !authorizesAccount(info, v.account) { + return output.ErrAuth(fmt.Sprintf("%s cannot access account %s (authorized: %s); nothing was stored", who.label(), v.account, authorizedAccountIDs(info))) + } + person, err := client.ForAccount(v.account).People().Me(ctx) + if err != nil { + return output.ErrAuth(fmt.Sprintf("Could not verify %s on account %s: %v", who.label(), v.account, err)) + } + who.PersonID = person.ID + who.Name = person.Name + if person.EmailAddress != "" { + who.Email = person.EmailAddress } - } - if err != nil && expectIdentity != "" { - return nil, output.ErrAuth(fmt.Sprintf("Could not verify the identity of the new credential: %v", err)) } - if expectIdentity != "" && strconv.FormatInt(who.IdentityID, 10) != expectIdentity { - return nil, output.ErrAuth(fmt.Sprintf("Authenticated as %s, not identity %s; the credential was not kept", who.label(), expectIdentity)) + v.who = who + return nil +} + +// authorizesAccount reports whether the authorization document names the +// account as reachable (and not expired) by the credential. +func authorizesAccount(info *basecamp.AuthorizationInfo, account string) bool { + for _, acct := range info.Accounts { + if strconv.FormatInt(acct.ID, 10) == account && !acct.Expired { + return true + } } + return false +} - _ = app.Auth.SetUserIdentity(strconv.FormatInt(person.ID, 10), who.Email) - return who, nil +func authorizedAccountIDs(info *basecamp.AuthorizationInfo) string { + ids := make([]string, 0, len(info.Accounts)) + for _, acct := range info.Accounts { + if !acct.Expired { + ids = append(ids, strconv.FormatInt(acct.ID, 10)) + } + } + if len(ids) == 0 { + return "none" + } + return strings.Join(ids, ", ") } // buildLogoutCmd constructs a logout command with the given Use name. diff --git a/internal/commands/auth_login_test.go b/internal/commands/auth_login_test.go index a41f5675..7082959a 100644 --- a/internal/commands/auth_login_test.go +++ b/internal/commands/auth_login_test.go @@ -89,39 +89,47 @@ func TestAuthLoginDeviceCodeForcesRemoteMode(t *testing.T) { "remote mode must not attempt a browser launch") } -// loginIdentityServer is a mock resource server for the post-login identity -// check: /my/profile.json and /authorization.json answer for exactly one -// bearer token and record what they were sent. +// loginIdentityServer is a mock resource server for the pre-store identity +// check: /authorization.json and the account-scoped /{account}/my/profile.json +// answer for exactly one bearer token and record what they were sent. type loginIdentityServer struct { srv *httptest.Server mu sync.Mutex bearers []string - // identityID is what /authorization.json reports; scope is optional. + paths []string + // identityID is what /authorization.json reports; scope is optional; + // accounts are the authorized account IDs (999 by default). identityID int64 scope string + accounts []int64 // authorizationStatus overrides the /authorization.json status when non-zero. authorizationStatus int + // personName lets a test plant hostile content in the person record. + personName string } func startLoginIdentityServer(t *testing.T, wantToken string) *loginIdentityServer { t.Helper() - s := &loginIdentityServer{identityID: 28142355} + s := &loginIdentityServer{identityID: 28142355, accounts: []int64{999}, personName: "Clawdito"} mux := http.NewServeMux() record := func(r *http.Request) bool { bearer := strings.TrimPrefix(r.Header.Get("Authorization"), "Bearer ") s.mu.Lock() s.bearers = append(s.bearers, bearer) + s.paths = append(s.paths, r.URL.Path) s.mu.Unlock() return bearer == wantToken } - mux.HandleFunc("/my/profile.json", func(w http.ResponseWriter, r *http.Request) { + mux.HandleFunc("/999/my/profile.json", func(w http.ResponseWriter, r *http.Request) { if !record(r) { w.WriteHeader(http.StatusUnauthorized) return } w.Header().Set("Content-Type", "application/json") - fmt.Fprint(w, `{"id":51177542,"name":"Clawdito","email_address":"clawdito@example.com"}`) + require.NoError(t, json.NewEncoder(w).Encode(map[string]any{ + "id": 51177542, "name": s.personName, "email_address": "clawdito@example.com", + })) }) mux.HandleFunc("/authorization.json", func(w http.ResponseWriter, r *http.Request) { if !record(r) { @@ -132,14 +140,25 @@ func startLoginIdentityServer(t *testing.T, wantToken string) *loginIdentityServ w.WriteHeader(s.authorizationStatus) return } - w.Header().Set("Content-Type", "application/json") - scope := "" + accounts := make([]map[string]any, 0, len(s.accounts)) + for _, id := range s.accounts { + accounts = append(accounts, map[string]any{"id": id, "name": "Acme", "href": fmt.Sprintf("%s/%d", s.srv.URL, id), "product": "bc3"}) + } + body := map[string]any{ + "identity": map[string]any{"id": s.identityID, "first_name": "Claw", "last_name": "Dito", "email_address": "identity@example.com"}, + "accounts": accounts, + "expires_at": "2036-01-01T00:00:00Z", + } if s.scope != "" { - scope = fmt.Sprintf(`,"scope":%q`, s.scope) + body["scope"] = s.scope } - fmt.Fprintf(w, `{"identity":{"id":%d,"email_address":"clawdito@example.com"},"accounts":[{"id":999,"name":"Acme","href":"%s/999","product":"bc3"}]%s,"expires_at":"2036-01-01T00:00:00Z"}`, s.identityID, s.srv.URL, scope) + w.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(w).Encode(body)) + }) + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + record(r) + http.NotFound(w, r) }) - mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { http.NotFound(w, r) }) s.srv = httptest.NewServer(mux) t.Cleanup(s.srv.Close) return s @@ -151,9 +170,16 @@ func (s *loginIdentityServer) seenBearers() []string { return append([]string(nil), s.bearers...) } -// managerTokenProvider feeds the SDK client from the auth manager, as the -// production authAdapter does, so the identity check runs as whatever the -// login just stored. +func (s *loginIdentityServer) seenPaths() []string { + s.mu.Lock() + defer s.mu.Unlock() + return append([]string(nil), s.paths...) +} + +// managerTokenProvider feeds the app's SDK client from the auth manager, as +// the production authAdapter does. The login never verifies through it — +// verification rides a client bound to the candidate token — so a test that +// sees it used has found a regression. type managerTokenProvider struct{ mgr *auth.Manager } func (p *managerTokenProvider) AccessToken(ctx context.Context) (string, error) { @@ -161,8 +187,8 @@ func (p *managerTokenProvider) AccessToken(ctx context.Context) (string, error) } // loginTestApp is an App wired for --with-token tests: file credential -// store under a temp XDG_CONFIG_HOME, SDK client pointed at the identity -// server, JSON output captured in buf. +// store under a temp XDG_CONFIG_HOME, SDK client (and SDKClientFor) pointed +// at the identity server, JSON output captured in buf. func loginTestApp(t *testing.T, srv *loginIdentityServer, cfg *config.Config) (*appctx.App, *bytes.Buffer) { t.Helper() t.Setenv("BASECAMP_NO_KEYRING", "1") @@ -177,22 +203,34 @@ func loginTestApp(t *testing.T, srv *loginIdentityServer, cfg *config.Config) (* } authMgr := auth.NewManager(cfg, srv.srv.Client()) authMgr.SetStore(auth.NewStore(config.GlobalConfigDir())) - sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: srv.srv.URL}, &managerTokenProvider{mgr: authMgr}, + sdkOptions := []basecamp.ClientOption{ basecamp.WithTransport(srv.srv.Client().Transport), basecamp.WithMaxRetries(1), - ) + } + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: srv.srv.URL}, &managerTokenProvider{mgr: authMgr}, sdkOptions...) buf := &bytes.Buffer{} app := &appctx.App{ - Config: cfg, - Auth: authMgr, - SDK: sdkClient, - Output: output.New(output.Options{Format: output.FormatJSON, Writer: buf}), + Config: cfg, + Auth: authMgr, + SDK: sdkClient, + SDKOptions: sdkOptions, + Output: output.New(output.Options{Format: output.FormatJSON, Writer: buf}), } return app, buf } -// runLogin executes `auth login` with stdin set to in and returns the error -// and everything the command wrote to stdout/stderr. +// withAccount sets the effective account the way the root pre-run would +// from the given source ("flag", "env", "profile", "global"). +func withAccount(app *appctx.App, id, source string) { + app.Config.AccountID = id + app.Config.Sources["account_id"] = source + if source == "flag" { + app.Flags.Account = id + } +} + +// runLogin executes `auth login` with stdin set to in and returns everything +// the command wrote to stdout/stderr, and the error. func runLogin(t *testing.T, app *appctx.App, in io.Reader, args ...string) (string, error) { t.Helper() cmd := NewAuthCmd() @@ -220,10 +258,18 @@ func readGlobalConfig(t *testing.T) map[string]any { return cfg } +func assertNothingStored(t *testing.T, app *appctx.App, name string) { + t.Helper() + _, loadErr := app.Auth.GetStore().Load("profile:" + name) + assert.Error(t, loadErr, "no credential may be stored for a rejected token") + assert.NotContains(t, readGlobalConfig(t), "profiles", "no profile entry may be registered for a rejected token") + assert.NotContains(t, app.Config.Profiles, name) +} + func TestAuthLoginWithTokenCreatesProfileAndVerifiesIdentity(t *testing.T) { srv := startLoginIdentityServer(t, "bc_at_secret") app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) - app.Flags.Account = "999" + withAccount(app, "999", "flag") // A trailing newline is what `op read` and `echo` deliver; it must not // reach the Authorization header. @@ -236,6 +282,8 @@ func TestAuthLoginWithTokenCreatesProfileAndVerifiesIdentity(t *testing.T) { for _, bearer := range srv.seenBearers() { assert.Equal(t, "bc_at_secret", bearer) } + assert.Equal(t, []string{"/authorization.json", "/999/my/profile.json"}, srv.seenPaths(), + "the person lookup is account-scoped, after the authorization document") creds, err := app.Auth.GetStore().Load("profile:bot") require.NoError(t, err) @@ -266,7 +314,7 @@ func TestAuthLoginWithTokenCreatesProfileAndVerifiesIdentity(t *testing.T) { func TestAuthLoginWithTokenJSONEnvelope(t *testing.T) { srv := startLoginIdentityServer(t, "bc_at_secret") app, buf := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) - app.Flags.Account = "999" + withAccount(app, "999", "flag") app.Flags.JSON = true out, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token", "--expect-identity", "28142355") @@ -290,39 +338,51 @@ func TestAuthLoginWithTokenJSONEnvelope(t *testing.T) { assert.Equal(t, true, data["profile_created"]) assert.Equal(t, true, data["default"]) assert.Equal(t, float64(28142355), data["identity"].(map[string]any)["id"]) - assert.Equal(t, "clawdito@example.com", data["identity"].(map[string]any)["email"]) + assert.Equal(t, "identity@example.com", data["identity"].(map[string]any)["email"], "the identity's own email, not the person's") assert.Equal(t, float64(51177542), data["person"].(map[string]any)["id"]) assert.Equal(t, "Clawdito", data["person"].(map[string]any)["name"]) + assert.Equal(t, "clawdito@example.com", data["person"].(map[string]any)["email"]) assert.NotContains(t, buf.String(), "bc_at_secret") } -func TestAuthLoginWithTokenExpectIdentityMismatchLeavesNothingBehind(t *testing.T) { +func TestAuthLoginWithTokenExpectIdentityMismatchStoresNothing(t *testing.T) { srv := startLoginIdentityServer(t, "bc_at_secret") app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) - app.Flags.Account = "999" + withAccount(app, "999", "flag") out, err := runLogin(t, app, strings.NewReader("bc_at_secret\n"), "--with-token", "--expect-identity", "1") require.Error(t, err) - assert.Contains(t, err.Error(), "Authenticated as Clawdito (identity 28142355, person 51177542), not identity 1") + assert.Contains(t, err.Error(), "Authenticated as Claw Dito (identity 28142355), not identity 1") assert.NotContains(t, err.Error()+out, "bc_at_secret") var outErr *output.Error require.ErrorAs(t, err, &outErr) assert.Equal(t, output.CodeAuth, outErr.Code) - _, loadErr := app.Auth.GetStore().Load("profile:bot") - assert.Error(t, loadErr, "the rejected credential must be deleted") - assert.NotContains(t, readGlobalConfig(t), "profiles", "no profile entry may be registered for a rejected credential") - assert.NotContains(t, app.Config.Profiles, "bot") + assertNothingStored(t, app, "bot") + assert.Equal(t, []string{"/authorization.json"}, srv.seenPaths(), "a rejected identity is not looked up any further") +} + +func TestAuthLoginWithTokenExpectIdentityAcceptsNumericSpellings(t *testing.T) { + for _, spelling := range []string{"28142355", "028142355", "+28142355"} { + t.Run(spelling, func(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + out, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token", "--expect-identity", spelling) + require.NoError(t, err, out) + }) + } } -func TestAuthLoginWithTokenMismatchRestoresPreviousCredentials(t *testing.T) { +func TestAuthLoginWithTokenMismatchLeavesPreviousCredentialUntouched(t *testing.T) { srv := startLoginIdentityServer(t, "bc_at_new") cfg := &config.Config{ ActiveProfile: "bot", Profiles: map[string]*config.ProfileConfig{"bot": {AccountID: "999"}}, } app, _ := loginTestApp(t, srv, cfg) + withAccount(app, "999", "profile") prev := &auth.Credentials{AccessToken: "bc_at_old", RefreshToken: "old-refresh", OAuthType: "bc5", Scope: "full", ExpiresAt: 4102444800} require.NoError(t, app.Auth.GetStore().Save("profile:bot", prev)) @@ -330,75 +390,152 @@ func TestAuthLoginWithTokenMismatchRestoresPreviousCredentials(t *testing.T) { require.Error(t, err) creds, loadErr := app.Auth.GetStore().Load("profile:bot") - require.NoError(t, loadErr, "the profile's previous credential must survive a rejected import") - assert.Equal(t, prev, creds) + require.NoError(t, loadErr) + assert.Equal(t, prev, creds, "the previous credential is never touched by a rejected import") + for _, bearer := range srv.seenBearers() { + assert.Equal(t, "bc_at_new", bearer, "verification must never run as the stored credential") + } } -func TestAuthLoginWithTokenFailsClosedWhenIdentityUnverifiable(t *testing.T) { +func TestAuthLoginWithTokenFailsClosedWhenUnverifiable(t *testing.T) { srv := startLoginIdentityServer(t, "bc_at_secret") srv.authorizationStatus = http.StatusUnauthorized app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) - app.Flags.Account = "999" + withAccount(app, "999", "flag") - _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token", "--expect-identity", "28142355") + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") require.Error(t, err) - assert.Contains(t, err.Error(), "Could not verify the identity") - _, loadErr := app.Auth.GetStore().Load("profile:bot") - assert.Error(t, loadErr) - - // Without an expectation the identity endpoint is informational: the - // person from /my/profile.json is enough to keep the credential. - app, _ = loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) - app.Flags.Account = "999" - out, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") - require.NoError(t, err, out) - assert.Contains(t, out, "Logged in as Clawdito (person 51177542)") + assert.Contains(t, err.Error(), "Could not verify the new credential") + assertNothingStored(t, app, "bot") } -func TestAuthLoginWithTokenRejectedTokenIsNotKept(t *testing.T) { +func TestAuthLoginWithTokenRejectedTokenIsNotStored(t *testing.T) { srv := startLoginIdentityServer(t, "bc_at_secret") app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) - app.Flags.Account = "999" + withAccount(app, "999", "flag") _, err := runLogin(t, app, strings.NewReader("bc_at_wrong"), "--with-token") require.Error(t, err) - _, loadErr := app.Auth.GetStore().Load("profile:bot") - assert.Error(t, loadErr, "a token the server rejects must not be stored") - assert.NotContains(t, readGlobalConfig(t), "profiles") + assertNothingStored(t, app, "bot") +} + +func TestAuthLoginWithTokenRequiresAccessToTheAccount(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + srv.accounts = []int64{111, 222} + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), "cannot access account 999 (authorized: 111, 222)") + assertNothingStored(t, app, "bot") + assert.NotContains(t, srv.seenPaths(), "/999/my/profile.json") +} + +func TestAuthLoginWithTokenRejectsPersonLookupFailure(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + srv.accounts = []int64{999, 1000} + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "1000", "flag") + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err, "the authorization document lists 1000 but the account-scoped lookup 404s") + assert.Contains(t, err.Error(), "on account 1000") + assertNothingStored(t, app, "bot") } func TestAuthLoginWithTokenScopeReportedByServerWins(t *testing.T) { srv := startLoginIdentityServer(t, "bc_at_secret") srv.scope = "read" app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) - app.Flags.Account = "999" + withAccount(app, "999", "flag") out, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") require.NoError(t, err, out) creds, err := app.Auth.GetStore().Load("profile:bot") require.NoError(t, err) assert.Equal(t, "read", creds.Scope) - assert.Equal(t, "51177542", creds.UserID, "identity survives the scope correction") + assert.Equal(t, "51177542", creds.UserID) assert.Equal(t, "read", app.Config.Profiles["bot"].Scope) } +func TestAuthLoginWithTokenRejectsUnknownReportedScope(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + srv.scope = "admin\x1b[31m" + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), "only read or full can be stored") + assert.NotContains(t, err.Error(), "\x1b") + assertNothingStored(t, app, "bot") +} + +func TestAuthLoginWithTokenRejectsInvalidScopeBeforeReadingStdin(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + + in := strings.NewReader("bc_at_secret") + _, err := runLogin(t, app, in, "--with-token", "--scope", "admin") + require.Error(t, err) + assert.Contains(t, err.Error(), "Invalid scope") + assert.Equal(t, 12, in.Len(), "stdin must not be consumed by a usage error") + assert.Empty(t, srv.seenBearers()) +} + func TestAuthLoginWithTokenRequiresAccountToCreateProfile(t *testing.T) { + t.Run("no account anywhere", func(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), `Profile "bot" does not exist`) + assert.Contains(t, err.Error(), "--account") + assert.Empty(t, srv.seenBearers(), "nothing may be sent before the profile question is settled") + assertNothingStored(t, app, "bot") + }) + + t.Run("a config-file account does not count", func(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "global") + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), "--account") + assert.Empty(t, srv.seenBearers()) + }) + + t.Run("BASECAMP_ACCOUNT_ID counts", func(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "env") + + out, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.NoError(t, err, out) + assert.Equal(t, "999", app.Config.Profiles["bot"].AccountID) + }) +} + +func TestAuthLoginWithTokenRejectsNonNumericAccount(t *testing.T) { srv := startLoginIdentityServer(t, "bc_at_secret") app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "abc", "flag") _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") require.Error(t, err) - assert.Contains(t, err.Error(), `Profile "bot" does not exist`) - assert.Contains(t, err.Error(), "--account") - assert.Empty(t, srv.seenBearers(), "nothing may be sent before the profile question is settled") - _, loadErr := app.Auth.GetStore().Load("profile:bot") - assert.Error(t, loadErr) + assert.Contains(t, err.Error(), `Invalid account ID "abc"`) + assert.Empty(t, srv.seenBearers()) + assertNothingStored(t, app, "bot") } func TestAuthLoginWithTokenRequiresProfile(t *testing.T) { srv := startLoginIdentityServer(t, "bc_at_secret") app, _ := loginTestApp(t, srv, &config.Config{}) - app.Flags.Account = "999" + withAccount(app, "999", "flag") _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") require.Error(t, err) @@ -408,39 +545,54 @@ func TestAuthLoginWithTokenRequiresProfile(t *testing.T) { } func TestAuthLoginWithTokenRejectsAccountMismatchOnExistingProfile(t *testing.T) { - srv := startLoginIdentityServer(t, "bc_at_secret") - cfg := &config.Config{ - ActiveProfile: "bot", - Profiles: map[string]*config.ProfileConfig{"bot": {AccountID: "111"}}, + for _, source := range []string{"flag", "env"} { + t.Run(source, func(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + cfg := &config.Config{ + ActiveProfile: "bot", + Profiles: map[string]*config.ProfileConfig{"bot": {AccountID: "111"}}, + } + app, _ := loginTestApp(t, srv, cfg) + // The root pre-run re-applies flag and env over the profile + // binding, so the effective account is the override. + withAccount(app, "222", source) + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), `Profile "bot" is bound to account 111, not 222`) + assert.Empty(t, srv.seenBearers()) + }) } - app, _ := loginTestApp(t, srv, cfg) - app.Flags.Account = "222" - - _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") - require.Error(t, err) - assert.Contains(t, err.Error(), `Profile "bot" is bound to account 111, not 222`) - assert.Empty(t, srv.seenBearers()) } func TestAuthLoginWithTokenExistingProfileKeepsItsEntry(t *testing.T) { srv := startLoginIdentityServer(t, "bc_at_secret") cfg := &config.Config{ - ActiveProfile: "bot", - Profiles: map[string]*config.ProfileConfig{"bot": {AccountID: "999", ProjectID: "42"}}, + ActiveProfile: "bot", + DefaultProfile: "bot", + Profiles: map[string]*config.ProfileConfig{"bot": {AccountID: "999", ProjectID: "42"}}, } - app, _ := loginTestApp(t, srv, cfg) + app, buf := loginTestApp(t, srv, cfg) + withAccount(app, "999", "profile") + app.Flags.JSON = true require.NoError(t, os.MkdirAll(config.GlobalConfigDir(), 0o700)) require.NoError(t, os.WriteFile(filepath.Join(config.GlobalConfigDir(), "config.json"), - []byte(`{"profiles":{"bot":{"base_url":"https://3.basecampapi.com","account_id":"999","project_id":"42"}},"default_profile":"human"}`), 0o600)) + []byte(`{"profiles":{"bot":{"base_url":"https://3.basecampapi.com","account_id":"999","project_id":"42"}},"default_profile":"bot"}`), 0o600)) out, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") require.NoError(t, err, out) - assert.NotContains(t, out, "Created profile") cfgFile := readGlobalConfig(t) bot := cfgFile["profiles"].(map[string]any)["bot"].(map[string]any) assert.Equal(t, "42", bot["project_id"], "importing into an existing profile must not rewrite its entry") - assert.Equal(t, "human", cfgFile["default_profile"]) + assert.Equal(t, "bot", cfgFile["default_profile"]) + + var envelope struct { + Data map[string]any `json:"data"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope)) + assert.Equal(t, false, envelope.Data["profile_created"]) + assert.Equal(t, true, envelope.Data["default"], "an existing default profile reports default") } func TestAuthLoginWithTokenRejectsBadStdin(t *testing.T) { @@ -458,7 +610,7 @@ func TestAuthLoginWithTokenRejectsBadStdin(t *testing.T) { t.Run(name, func(t *testing.T) { srv := startLoginIdentityServer(t, "bc_at_secret") app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) - app.Flags.Account = "999" + withAccount(app, "999", "flag") _, err := runLogin(t, app, strings.NewReader(tc.in), "--with-token") require.Error(t, err) @@ -484,7 +636,7 @@ func TestAuthLoginWithTokenRefusesTerminalStdin(t *testing.T) { srv := startLoginIdentityServer(t, "bc_at_secret") app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) - app.Flags.Account = "999" + withAccount(app, "999", "flag") _, err = runLogin(t, app, pty, "--with-token") require.Error(t, err) @@ -496,7 +648,7 @@ func TestAuthLoginWithTokenRefusesTerminalStdin(t *testing.T) { func TestAuthLoginWithTokenRefusesEnvToken(t *testing.T) { srv := startLoginIdentityServer(t, "bc_at_secret") app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) - app.Flags.Account = "999" + withAccount(app, "999", "flag") t.Setenv("BASECAMP_TOKEN", "env-token") _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") @@ -505,15 +657,22 @@ func TestAuthLoginWithTokenRefusesEnvToken(t *testing.T) { assert.Empty(t, srv.seenBearers()) } -func TestAuthLoginWithTokenRejectsNonNumericExpectIdentity(t *testing.T) { - srv := startLoginIdentityServer(t, "bc_at_secret") - app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) - app.Flags.Account = "999" +func TestAuthLoginRejectsNonNumericExpectIdentity(t *testing.T) { + for _, bad := range []string{"clawdito", "0", "-5", ""} { + if bad == "" { + continue + } + t.Run(bad, func(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") - _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token", "--expect-identity", "clawdito") - require.Error(t, err) - assert.Contains(t, err.Error(), "numeric identity ID") - assert.Empty(t, srv.seenBearers()) + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token", "--expect-identity", bad) + require.Error(t, err) + assert.Contains(t, err.Error(), "numeric identity ID") + assert.Empty(t, srv.seenBearers()) + }) + } } func TestAuthLoginWithTokenIsExclusiveWithInteractiveFlags(t *testing.T) { @@ -528,6 +687,29 @@ func TestAuthLoginWithTokenIsExclusiveWithInteractiveFlags(t *testing.T) { } } +// TestAuthLoginSanitizesServerSuppliedName: the person's name is server +// data headed for a one-line terminal sink; the JSON field keeps it verbatim. +func TestAuthLoginSanitizesServerSuppliedName(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + srv.personName = "Claw\x1b]8;;https://evil.example\x07dito\r\nEvil" + app, buf := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + + out, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.NoError(t, err, out) + assert.Contains(t, out, "Logged in as Clawdito Evil ") + assert.NotContains(t, out, "\x1b") + assert.NotContains(t, out, "evil.example") + + app2, buf2 := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app2, "999", "flag") + app2.Flags.JSON = true + _, err = runLogin(t, app2, strings.NewReader("bc_at_secret"), "--with-token") + require.NoError(t, err) + _ = buf + assert.Contains(t, buf2.String(), `evil.example`, "structured output carries the field verbatim") +} + // TestAuthLoginRefusesMachineOutputForInteractiveFlows covers the machine-mode // half of #669: a browser or device login under --json/--agent used to print // prose and block on approval. It now refuses before touching the network. @@ -572,3 +754,54 @@ func TestAuthLoginExpectIdentityRefusesEnvToken(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "BASECAMP_TOKEN is set") } + +// TestAuthLoginDeviceFlowExpectIdentityStoresNothingOnMismatch: the OAuth +// flows verify through the same pre-store hook. A device login whose token +// belongs to someone else never reaches the credential store, and the +// success line is never printed. +func TestAuthLoginDeviceFlowExpectIdentityStoresNothingOnMismatch(t *testing.T) { + srv := startLoginIdentityServer(t, "dev-tok") + // A pinned issuer served by the same mux: the device grant hands out + // dev-tok, which the identity server then answers for. + srv.srv.Config.Handler = deviceGrantThen(t, srv.srv.Config.Handler) + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot", Profiles: map[string]*config.ProfileConfig{"bot": {AccountID: "999"}}}) + withAccount(app, "999", "profile") + t.Setenv("BASECAMP_OAUTH_ISSUER", srv.srv.URL) + t.Setenv("SSH_CONNECTION", "") + t.Setenv("SSH_CLIENT", "") + t.Setenv("SSH_TTY", "") + + out, err := runLogin(t, app, strings.NewReader(""), "--device-code", "--expect-identity", "1") + require.Error(t, err) + assert.Contains(t, err.Error(), "not identity 1") + assert.NotContains(t, out, "Authentication successful", "no success line before the check") + _, loadErr := app.Auth.GetStore().Load("profile:bot") + assert.Error(t, loadErr, "a device login that fails --expect-identity stores nothing") + + // The same grant with the right expectation is stored and labeled. + out, err = runLogin(t, app, strings.NewReader(""), "--device-code", "--expect-identity", "28142355") + require.NoError(t, err, out) + assert.Contains(t, out, "Authentication successful") + assert.Contains(t, out, "Logged in as: Clawdito (identity 28142355, person 51177542)") + creds, err := app.Auth.GetStore().Load("profile:bot") + require.NoError(t, err) + assert.Equal(t, "dev-tok", creds.AccessToken) + assert.Equal(t, "51177542", creds.UserID) +} + +// deviceGrantThen wraps a handler with a BC5 device grant that issues +// dev-tok immediately, at the paths a pinned issuer derives. +func deviceGrantThen(t *testing.T, next http.Handler) http.Handler { + t.Helper() + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/oauth/device_authorizations": + fmt.Fprintf(w, `{"device_code":"dc","user_code":"ABCD-EFGH","verification_uri":%q,"expires_in":600,"interval":1}`, "http://"+r.Host+"/verify") + case "/oauth/tokens": + fmt.Fprint(w, `{"access_token":"dev-tok","refresh_token":"dev-ref","token_type":"bearer","expires_in":3600,"scope":"full"}`) + default: + next.ServeHTTP(w, r) + } + }) +} diff --git a/skills/basecamp-doctor/SKILL.md b/skills/basecamp-doctor/SKILL.md index 913399fd..eafda447 100644 --- a/skills/basecamp-doctor/SKILL.md +++ b/skills/basecamp-doctor/SKILL.md @@ -20,7 +20,11 @@ Interpret every check by status: Report failures and warnings with their `hint` fields. Also inspect the top-level `breadcrumbs` array and preserve its structured `cmd` next steps, because a breadcrumb can provide a more specific action than a check hint. Use these common remediations when relevant: -- Basecamp authentication: `basecamp auth login` +- Basecamp authentication: `basecamp auth login`. Guard a replacement login + with `--expect-identity ` so a browser signed in as someone else cannot + become the profile. A bot or CI profile that must never sign in interactively + imports a personal access token instead: + `op read "op:////credential" | basecamp auth login --with-token -P --account --expect-identity ` - Agent plugin installation or version: `basecamp setup agents` (honors `BASECAMP_SETUP_AGENT`) - Codex plugin specifically: `basecamp setup codex` - Claude Code plugin specifically: `basecamp setup claude` From a2e8a69624d0c96082e4be8575a805fa5e837e65 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 2 Sep 2026 14:00:53 -0700 Subject: [PATCH 3/8] Bind what the token was verified for, and keep ordinary logins best-effort MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The import verifies a token for one account and base URL and stores it under a profile; those must be the same place. An existing profile's base URL must match the effective one (a BASECAMP_BASE_URL override would verify against one host and store for another), an unbound profile is bound to the explicitly given account alongside the token (other fields preserved), and account comparisons are numeric like the rest of the package. The profile entry is written before the credential, so a failure between the two leaves a visibly unauthenticated profile rather than an orphaned secret. Strict verification refuses an authorization document with no identity id. An ordinary browser or device login without --expect-identity is not the assertive kind: an account the token cannot reach, or a person lookup that fails, no longer discards the login — the identity line falls back to the authorization document and no person id is fabricated (nothing is written as user_id 0 either). Stdin accepts exactly one trailing line ending, the shape a pipe delivers, rather than trimming whatever whitespace surrounds the secret. --- internal/auth/auth.go | 6 +- internal/commands/auth.go | 77 ++++++++++++---- internal/commands/auth_login_test.go | 131 +++++++++++++++++++++++++-- internal/commands/profile.go | 18 ++++ 4 files changed, 206 insertions(+), 26 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index dd730143..7896501e 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -875,8 +875,10 @@ const oauthIssuerEnv = "BASECAMP_OAUTH_ISSUER" // issuer origin, deriving the endpoints Basecamp mounts under /oauth. The // issuer is operator-supplied environment, so it passes the same endpoint // checks a discovery document would, and the derived endpoints are checked -// again by loginDevice before any POST. The value is never echoed: like a -// base URL, it can carry userinfo or a query string. +// again by loginDevice before any POST. A rejected value is never echoed +// (like a base URL, it can carry userinfo or a query string); the accepted +// one is announced, reduced to one line, so the operator sees what was +// pinned — as the discovered issuer is. func pinnedIssuerDiscovery(issuer string, log func(string)) (*discovery, error) { issuer = strings.TrimRight(strings.TrimSpace(issuer), "/") u, err := url.Parse(issuer) diff --git a/internal/commands/auth.go b/internal/commands/auth.go index d4637a7b..fc941e6a 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -345,7 +345,9 @@ This build tells you the hint instead of sending it to the server.`, } if who := verifier.who; who != nil { - _ = app.Auth.SetUserIdentity(strconv.FormatInt(who.PersonID, 10), who.Email) + if who.PersonID != 0 { + _ = app.Auth.SetUserIdentity(strconv.FormatInt(who.PersonID, 10), who.Email) + } fmt.Fprintln(w, r.Data.Render("Logged in as: "+who.label())) } @@ -397,19 +399,31 @@ func runLoginWithToken(cmd *cobra.Command, app *appctx.App, scope string, expect return output.ErrUsage(fmt.Sprintf("Invalid profile name %q: use only letters, numbers, hyphens, and underscores", name)) } - // The effective account is what every later command will address, so - // it is what the token must be able to reach: the profile's binding, - // overridden by --account / BASECAMP_ACCOUNT_ID exactly as at runtime. + // The effective account and base URL are what every later command will + // address under this profile, so they are what the token must be + // verified for — and they must be the profile's own. A --account / + // BASECAMP_ACCOUNT_ID / BASECAMP_BASE_URL override that disagrees with + // the binding would verify the token for one place and store it for + // another. account := app.Config.AccountID existing := app.Config.Profiles[name] var created *config.ProfileConfig + bindAccount := false switch { case existing == nil && !accountGivenExplicitly(app): return output.ErrUsageHint(fmt.Sprintf("Profile %q does not exist", name), "Pass --account to create it alongside the imported token.") case existing == nil: created = &config.ProfileConfig{BaseURL: app.Config.BaseURL, AccountID: account} - case existing.AccountID != "" && account != existing.AccountID: + case existing.BaseURL != "" && config.NormalizeBaseURL(existing.BaseURL) != config.NormalizeBaseURL(app.Config.BaseURL): + return output.ErrUsageHint(fmt.Sprintf("Profile %q is bound to %s, not %s", name, existing.BaseURL, app.Config.BaseURL), + "Import into a different profile, or drop the BASECAMP_BASE_URL override.") + case existing.AccountID == "" && !accountGivenExplicitly(app): + return output.ErrUsageHint(fmt.Sprintf("Profile %q has no account", name), + "Pass --account to bind it alongside the imported token.") + case existing.AccountID == "": + bindAccount = true + case !accountIDsEqual(account, existing.AccountID): return output.ErrUsageHint(fmt.Sprintf("Profile %q is bound to account %s, not %s", name, existing.AccountID, account), "Import into a different profile, or drop the --account / BASECAMP_ACCOUNT_ID override.") } @@ -432,20 +446,29 @@ func runLoginWithToken(cmd *cobra.Command, app *appctx.App, scope string, expect scope = who.Scope } - if err := app.Auth.ImportToken(token, scope, strconv.FormatInt(who.PersonID, 10), who.Email); err != nil { - return err - } - + // The profile entry goes first: an entry without a credential is a + // visible, harmless state (profile list shows it unauthenticated), where + // a stored secret without an entry would be an orphan. isDefault := app.Config.DefaultProfile == name - if created != nil { + switch { + case created != nil: created.Scope = scope if isDefault, err = registerProfile(name, created); err != nil { - return fmt.Errorf("the token was stored for profile %q but the profile entry could not be written (rerun the import): %w", name, err) + return err } if app.Config.Profiles == nil { app.Config.Profiles = make(map[string]*config.ProfileConfig) } app.Config.Profiles[name] = created + case bindAccount: + if err := bindProfileAccount(name, account); err != nil { + return err + } + existing.AccountID = account + } + + if err := app.Auth.ImportToken(token, scope, strconv.FormatInt(who.PersonID, 10), who.Email); err != nil { + return fmt.Errorf("profile %q is registered but the token could not be stored (rerun the import): %w", name, err) } data := map[string]any{ @@ -525,13 +548,18 @@ func readTokenFromStdin(cmd *cobra.Command) (string, error) { return "", output.ErrUsage(fmt.Sprintf("Token on stdin is longer than %d bytes; expected a single access token", maxTokenBytes)) } - token := strings.TrimSpace(string(data)) + // A pipe delivers the token with one line ending (`op read`, `echo`, a + // CRLF file on Windows); that one is stripped and nothing else is — + // any other whitespace is not a token, and quietly trimming it would + // hide a malformed secret rather than the secret store's exact value. + token := strings.TrimSuffix(string(data), "\n") + token = strings.TrimSuffix(token, "\r") if token == "" { return "", output.ErrUsageHint("No token on stdin", "Pipe it in from a secret store: `op read \"op:////credential\" | basecamp auth login --with-token -P --account `.") } if strings.IndexFunc(token, func(c rune) bool { return unicode.IsSpace(c) || unicode.IsControl(c) }) >= 0 { - return "", output.ErrUsage("Token on stdin must be a single line with no whitespace or control characters") + return "", output.ErrUsage("Token on stdin must be a single line with no whitespace or control characters (one trailing line ending is allowed)") } return token, nil } @@ -602,10 +630,12 @@ func (l *loginIdentity) label() string { // credential or from BASECAMP_TOKEN. // // Strict mode is the assertive login: the authorization endpoint must -// answer, the effective account (when there is one) must be among the -// accounts the token can reach, and the identity must match any -// expectation. Non-strict mode keeps the informational "Logged in as" line -// best-effort: a lookup failure leaves who nil and the login proceeds. +// answer with an identity, the effective account (when there is one) must +// be among the accounts the token can reach and its person record must +// resolve, and the identity must match any expectation. Non-strict mode +// keeps the informational "Logged in as" line best-effort: a lookup failure +// leaves who nil or without a person, and the login proceeds. A scope the +// CLI cannot store and a stated expectation are refused in either mode. type loginVerifier struct { app *appctx.App expectIdentity int64 @@ -640,6 +670,9 @@ func (v *loginVerifier) verify(ctx context.Context, accessToken, oauthType strin if who.Scope != "" && who.Scope != "read" && who.Scope != "full" { return output.ErrAuth(fmt.Sprintf("The server reports scope %q for this credential; only read or full can be stored", richtext.SanitizeSingleLine(who.Scope))) } + if v.strict && who.IdentityID <= 0 { + return output.ErrAuth("The authorization endpoint did not report an identity for the new credential; nothing was stored") + } if v.expectIdentity != 0 && who.IdentityID != v.expectIdentity { return output.ErrAuth(fmt.Sprintf("Authenticated as %s, not identity %d; nothing was stored", who.label(), v.expectIdentity)) } @@ -650,10 +683,18 @@ func (v *loginVerifier) verify(ctx context.Context, accessToken, oauthType strin // account there is a clearer answer than a 404 from the person lookup. if v.account != "" { if !authorizesAccount(info, v.account) { + if !v.strict { + v.who = who + return nil + } return output.ErrAuth(fmt.Sprintf("%s cannot access account %s (authorized: %s); nothing was stored", who.label(), v.account, authorizedAccountIDs(info))) } person, err := client.ForAccount(v.account).People().Me(ctx) if err != nil { + if !v.strict { + v.who = who + return nil + } return output.ErrAuth(fmt.Sprintf("Could not verify %s on account %s: %v", who.label(), v.account, err)) } who.PersonID = person.ID @@ -671,7 +712,7 @@ func (v *loginVerifier) verify(ctx context.Context, accessToken, oauthType strin // account as reachable (and not expired) by the credential. func authorizesAccount(info *basecamp.AuthorizationInfo, account string) bool { for _, acct := range info.Accounts { - if strconv.FormatInt(acct.ID, 10) == account && !acct.Expired { + if !acct.Expired && accountIDsEqual(strconv.FormatInt(acct.ID, 10), account) { return true } } diff --git a/internal/commands/auth_login_test.go b/internal/commands/auth_login_test.go index 7082959a..6c55ec96 100644 --- a/internal/commands/auth_login_test.go +++ b/internal/commands/auth_login_test.go @@ -600,12 +600,16 @@ func TestAuthLoginWithTokenRejectsBadStdin(t *testing.T) { in string want string }{ - "empty": {"", "No token on stdin"}, - "whitespace": {" \n\t", "No token on stdin"}, - "two lines": {"bc_at_one\nbc_at_two\n", "single line"}, - "inner space": {"bc_at one", "single line"}, - "control char": {"bc_at\x1bone", "single line"}, - "oversized": {strings.Repeat("x", maxTokenBytes+1), "longer than"}, + "empty": {"", "No token on stdin"}, + "bare newline": {"\n", "No token on stdin"}, + "whitespace": {" \n\t", "single line"}, + "leading space": {" bc_at_secret", "single line"}, + "trailing space": {"bc_at_secret ", "single line"}, + "two trailing newlines": {"bc_at_secret\n\n", "single line"}, + "two lines": {"bc_at_one\nbc_at_two\n", "single line"}, + "inner space": {"bc_at one", "single line"}, + "control char": {"bc_at\x1bone", "single line"}, + "oversized": {strings.Repeat("x", maxTokenBytes+1), "longer than"}, } { t.Run(name, func(t *testing.T) { srv := startLoginIdentityServer(t, "bc_at_secret") @@ -805,3 +809,118 @@ func deviceGrantThen(t *testing.T, next http.Handler) http.Handler { } }) } + +func TestAuthLoginWithTokenAcceptsOneTrailingLineEnding(t *testing.T) { + for name, in := range map[string]string{"LF": "bc_at_secret\n", "CRLF": "bc_at_secret\r\n", "none": "bc_at_secret"} { + t.Run(name, func(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + out, err := runLogin(t, app, strings.NewReader(in), "--with-token") + require.NoError(t, err, out) + for _, bearer := range srv.seenBearers() { + assert.Equal(t, "bc_at_secret", bearer) + } + }) + } +} + +func TestAuthLoginWithTokenFailsClosedWithoutAnIdentity(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + srv.identityID = 0 + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), "did not report an identity") + assertNothingStored(t, app, "bot") +} + +func TestAuthLoginWithTokenBindsAnUnboundExistingProfile(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + cfg := &config.Config{ + ActiveProfile: "bot", + Profiles: map[string]*config.ProfileConfig{"bot": {ProjectID: "42"}}, + } + app, _ := loginTestApp(t, srv, cfg) + require.NoError(t, os.MkdirAll(config.GlobalConfigDir(), 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(config.GlobalConfigDir(), "config.json"), + []byte(`{"profiles":{"bot":{"base_url":"`+srv.srv.URL+`","project_id":"42"}}}`), 0o600)) + + t.Run("without an explicit account the import is refused", func(t *testing.T) { + withAccount(app, "999", "global") + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), `Profile "bot" has no account`) + assert.Empty(t, srv.seenBearers()) + }) + + t.Run("an explicit account is bound alongside the token", func(t *testing.T) { + withAccount(app, "999", "flag") + out, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.NoError(t, err, out) + bot := readGlobalConfig(t)["profiles"].(map[string]any)["bot"].(map[string]any) + assert.Equal(t, "999", bot["account_id"], "the verified account is persisted on the profile") + assert.Equal(t, "42", bot["project_id"], "the rest of the entry is preserved") + assert.Equal(t, "999", app.Config.Profiles["bot"].AccountID) + }) +} + +func TestAuthLoginWithTokenRejectsBaseURLOverrideOnExistingProfile(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + cfg := &config.Config{ + ActiveProfile: "bot", + Profiles: map[string]*config.ProfileConfig{"bot": {BaseURL: "https://staging.example/", AccountID: "999"}}, + } + // loginTestApp points the effective base URL at the identity server, + // which is what a BASECAMP_BASE_URL override would do. + app, _ := loginTestApp(t, srv, cfg) + withAccount(app, "999", "profile") + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), `Profile "bot" is bound to https://staging.example/, not `+srv.srv.URL) + assert.Empty(t, srv.seenBearers()) + _, loadErr := app.Auth.GetStore().Load("profile:bot") + assert.Error(t, loadErr) +} + +func TestAuthLoginWithTokenComparesAccountsNumerically(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + cfg := &config.Config{ + ActiveProfile: "bot", + Profiles: map[string]*config.ProfileConfig{"bot": {AccountID: "0999"}}, + } + app, _ := loginTestApp(t, srv, cfg) + withAccount(app, "999", "flag") + + out, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.NoError(t, err, out) +} + +// TestAuthLoginDeviceFlowWithoutExpectationKeepsBestEffortIdentity: an +// ordinary login must not be lost to the informational lookups. When the +// configured account is not one the token reaches, the credential is still +// stored, the identity line falls back to the authorization document, and +// no person id is fabricated. +func TestAuthLoginDeviceFlowWithoutExpectationKeepsBestEffortIdentity(t *testing.T) { + srv := startLoginIdentityServer(t, "dev-tok") + srv.accounts = []int64{111} + srv.srv.Config.Handler = deviceGrantThen(t, srv.srv.Config.Handler) + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot", Profiles: map[string]*config.ProfileConfig{"bot": {AccountID: "999"}}}) + withAccount(app, "999", "profile") + t.Setenv("BASECAMP_OAUTH_ISSUER", srv.srv.URL) + t.Setenv("SSH_CONNECTION", "") + t.Setenv("SSH_CLIENT", "") + t.Setenv("SSH_TTY", "") + + out, err := runLogin(t, app, strings.NewReader(""), "--device-code") + require.NoError(t, err, out) + assert.Contains(t, out, "Authentication successful") + assert.Contains(t, out, "Logged in as: Claw Dito (identity 28142355)") + creds, err := app.Auth.GetStore().Load("profile:bot") + require.NoError(t, err) + assert.Equal(t, "dev-tok", creds.AccessToken) + assert.Empty(t, creds.UserID, "no person was resolved, so none is recorded") +} diff --git a/internal/commands/profile.go b/internal/commands/profile.go index feaa75b9..57b21a9f 100644 --- a/internal/commands/profile.go +++ b/internal/commands/profile.go @@ -437,6 +437,24 @@ func registerProfile(name string, p *config.ProfileConfig) (isDefault bool, err return isDefault, atomicWriteJSON(configPath, configData) } +// bindProfileAccount sets the account on an existing profile entry in the +// global config file, leaving every other field of the entry as it is. +func bindProfileAccount(name, account string) error { + configPath := filepath.Join(config.GlobalConfigDir(), "config.json") + configData := make(map[string]any) + if data, err := os.ReadFile(configPath); err == nil { //nolint:gosec // G304: Path is from trusted config location + _ = json.Unmarshal(data, &configData) + } + + profilesMap, _ := configData["profiles"].(map[string]any) + entry, _ := profilesMap[name].(map[string]any) + if entry == nil { + return output.ErrUsage(fmt.Sprintf("Profile %q not found in %s", name, configPath)) + } + entry["account_id"] = account + return atomicWriteJSON(configPath, configData) +} + // unregisterProfile removes a profile entry from the global config file, // clearing default_profile when it named this profile. Credentials are the // caller's to remove. From a2defe825cae57c81401da469085b4d976303de4 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 2 Sep 2026 14:27:04 -0700 Subject: [PATCH 4/8] Keep the server's expiry, refuse stray arguments and non-global bindings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The authorization document reports when the token expires; the import now stores that instead of claiming the token never does, and the envelope and status line report it. With no refresh token, AccessToken refuses the token near expiry with the existing "No refresh token" error, so the remedy — import again — is named rather than discovered from failing requests. A document without an expiry still stores zero. The login command takes no positional arguments, so a token pasted after --with-token is refused instead of being ignored while it sits in shell history. Binding an accountless profile rewrites the global config file, so a profile that arrived from a system, repo or local config is refused before stdin is read rather than left shadowing a binding it never sees. --- README.md | 4 +- internal/auth/auth.go | 27 +++++----- internal/auth/device_test.go | 14 +++++- internal/commands/auth.go | 27 ++++++++-- internal/commands/auth_login_test.go | 74 ++++++++++++++++++++++++---- 5 files changed, 120 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index 131f29f5..72b2b323 100644 --- a/README.md +++ b/README.md @@ -217,7 +217,9 @@ op read "op://Vault/Item/credential" | basecamp auth login --with-token -P bot - `--account` is required when the profile does not exist yet. `--json` returns an envelope with the profile, account, identity and person, `oauth_type`, -`scope`, and `expires_at: null`. +`scope`, and `expires_at` (the expiry the server reports for the token, or +`null` when it reports none). A token has no refresh token, so near a reported +expiry the CLI refuses it and asks for a fresh import. ### Multiple Identities diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 7896501e..ed838677 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -773,25 +773,30 @@ func validVerificationURL(raw string) string { // ImportToken stores an externally issued BC5 access token (a personal // access token) as the current credential, in one write, together with the -// identity it was verified to authenticate as. The token has no refresh -// token and no token endpoint, and ExpiresAt stays zero — the non-expiring -// path AccessToken already takes, so refresh is never attempted; when the -// server stops honoring it the next request fails as unauthenticated and -// the token is imported again. Scope is what the token was verified or -// declared to carry. -func (m *Manager) ImportToken(token, scope, userID, userEmail string) error { +// identity it was verified to authenticate as and the expiry the server +// reported for it (zero when it reported none). The token has no refresh +// token and no token endpoint: a zero expiry is the non-expiring path +// AccessToken already takes, and a reported one makes AccessToken refuse +// the token near expiry with "No refresh token available" rather than +// letting requests start failing — either way the remedy is to import +// again. Scope is what the token was verified or declared to carry. +func (m *Manager) ImportToken(token, scope, userID, userEmail string, expiresAt time.Time) error { if scope != scopeRead && scope != scopeFull { return output.ErrUsage("Invalid scope. Use 'read' or 'full'") } - m.mu.Lock() - defer m.mu.Unlock() - return m.store.Save(m.credentialKey(), &Credentials{ + creds := &Credentials{ AccessToken: token, OAuthType: oauthTypeBC5, Scope: scope, UserID: userID, UserEmail: userEmail, - }) + } + if !expiresAt.IsZero() { + creds.ExpiresAt = expiresAt.Unix() + } + m.mu.Lock() + defer m.mu.Unlock() + return m.store.Save(m.credentialKey(), creds) } // Logout removes stored credentials. diff --git a/internal/auth/device_test.go b/internal/auth/device_test.go index 092535ba..7fc5d05c 100644 --- a/internal/auth/device_test.go +++ b/internal/auth/device_test.go @@ -1088,7 +1088,7 @@ func TestImportToken(t *testing.T) { m := newDeviceTestManager(t, "https://3.basecampapi.com") m.cfg.ActiveProfile = "bot" - require.NoError(t, m.ImportToken("bc_at_secret", "full", "51177542", "bot@example.com")) + require.NoError(t, m.ImportToken("bc_at_secret", "full", "51177542", "bot@example.com", time.Time{})) creds, err := m.store.Load("profile:bot") require.NoError(t, err) @@ -1108,9 +1108,19 @@ func TestImportToken(t *testing.T) { require.Error(t, err, "an explicit refresh has nothing to refresh with") assert.Contains(t, err.Error(), "No refresh token") - err = m.ImportToken("bc_at_secret", "admin", "", "") + err = m.ImportToken("bc_at_secret", "admin", "", "", time.Time{}) require.Error(t, err) assert.Contains(t, err.Error(), "Invalid scope") + + // A reported expiry is kept, and near it the token is refused rather + // than served: there is no refresh token to renew it with. + require.NoError(t, m.ImportToken("bc_at_short", "full", "", "", time.Now().Add(time.Minute))) + creds, err = m.store.Load("profile:bot") + require.NoError(t, err) + assert.Positive(t, creds.ExpiresAt) + _, err = m.AccessToken(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "No refresh token") } // TestLoginDevice_VerifyRunsBeforeStore: a Verify hook sees the freshly diff --git a/internal/commands/auth.go b/internal/commands/auth.go index fc941e6a..9cb35cdd 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -273,6 +273,9 @@ named profile, creating the profile when --account is given. --login-hint names the account to sign in as on the device-flow approval page. This build tells you the hint instead of sending it to the server.`, Annotations: map[string]string{AnnotationProfileMayCreate: "true"}, + // No positional arguments, so a token pasted after --with-token is + // refused rather than silently ignored while it sits in shell history. + Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) if app == nil { @@ -421,6 +424,11 @@ func runLoginWithToken(cmd *cobra.Command, app *appctx.App, scope string, expect case existing.AccountID == "" && !accountGivenExplicitly(app): return output.ErrUsageHint(fmt.Sprintf("Profile %q has no account", name), "Pass --account to bind it alongside the imported token.") + case existing.AccountID == "" && config.Source(app.Config.Sources["profiles"]) != config.SourceGlobal: + // Binding rewrites the global config file; a profile that arrived + // from a system, repo or local config would keep shadowing it. + return output.ErrUsageHint(fmt.Sprintf("Profile %q has no account and is defined outside the global config", name), + fmt.Sprintf("Add account_id to its entry in the %s config, then rerun the import.", app.Config.Sources["profiles"])) case existing.AccountID == "": bindAccount = true case !accountIDsEqual(account, existing.AccountID): @@ -467,10 +475,17 @@ func runLoginWithToken(cmd *cobra.Command, app *appctx.App, scope string, expect existing.AccountID = account } - if err := app.Auth.ImportToken(token, scope, strconv.FormatInt(who.PersonID, 10), who.Email); err != nil { + if err := app.Auth.ImportToken(token, scope, strconv.FormatInt(who.PersonID, 10), who.Email, who.ExpiresAt); err != nil { return fmt.Errorf("profile %q is registered but the token could not be stored (rerun the import): %w", name, err) } + var expiresAt any + tokenLine := "personal access token (does not expire)" + if !who.ExpiresAt.IsZero() { + expiresAt = who.ExpiresAt.UTC().Format(time.RFC3339) + tokenLine = "expires " + who.ExpiresAt.Local().Format("2006-01-02") + } + data := map[string]any{ "profile": name, "account_id": account, @@ -478,7 +493,7 @@ func runLoginWithToken(cmd *cobra.Command, app *appctx.App, scope string, expect "source": "token", "oauth_type": "bc5", "scope": scope, - "expires_at": nil, + "expires_at": expiresAt, "identity": map[string]any{"id": who.IdentityID, "email": who.IdentityEmail}, "person": map[string]any{"id": who.PersonID, "name": who.Name, "email": who.Email}, "profile_created": created != nil, @@ -494,7 +509,7 @@ func runLoginWithToken(cmd *cobra.Command, app *appctx.App, scope string, expect w := cmd.OutOrStdout() r := output.NewRendererWithTheme(w, false, tui.ResolveTheme(tui.DetectDark())) fmt.Fprintln(w, r.Success.Render("Logged in as "+who.label())) - fmt.Fprintln(w, r.Muted.Render(fmt.Sprintf("Profile: %s · Account: %s · Access: %s · Token: personal access token (does not expire)", name, account, scope))) + fmt.Fprintln(w, r.Muted.Render(fmt.Sprintf("Profile: %s · Account: %s · Access: %s · Token: %s", name, account, scope, tokenLine))) if created != nil { line := fmt.Sprintf("Created profile %q for account %s", name, account) if isDefault { @@ -602,6 +617,9 @@ type loginIdentity struct { Name string Email string Scope string + // ExpiresAt is the expiry the authorization document reported for the + // credential; zero when it reported none. + ExpiresAt time.Time } // label renders the identity for a one-line terminal sink. Name and email @@ -667,6 +685,9 @@ func (v *loginVerifier) verify(ctx context.Context, accessToken, oauthType strin Name: strings.TrimSpace(info.Identity.FirstName + " " + info.Identity.LastName), Scope: info.Scope, } + if expiry, ok := info.Expiry(); ok { + who.ExpiresAt = expiry + } if who.Scope != "" && who.Scope != "read" && who.Scope != "full" { return output.ErrAuth(fmt.Sprintf("The server reports scope %q for this credential; only read or full can be stored", richtext.SanitizeSingleLine(who.Scope))) } diff --git a/internal/commands/auth_login_test.go b/internal/commands/auth_login_test.go index 6c55ec96..ee1722d5 100644 --- a/internal/commands/auth_login_test.go +++ b/internal/commands/auth_login_test.go @@ -107,11 +107,13 @@ type loginIdentityServer struct { authorizationStatus int // personName lets a test plant hostile content in the person record. personName string + // expiresAt is what /authorization.json reports; empty omits the field. + expiresAt string } func startLoginIdentityServer(t *testing.T, wantToken string) *loginIdentityServer { t.Helper() - s := &loginIdentityServer{identityID: 28142355, accounts: []int64{999}, personName: "Clawdito"} + s := &loginIdentityServer{identityID: 28142355, accounts: []int64{999}, personName: "Clawdito", expiresAt: "2036-01-01T00:00:00Z"} mux := http.NewServeMux() record := func(r *http.Request) bool { bearer := strings.TrimPrefix(r.Header.Get("Authorization"), "Bearer ") @@ -145,9 +147,11 @@ func startLoginIdentityServer(t *testing.T, wantToken string) *loginIdentityServ accounts = append(accounts, map[string]any{"id": id, "name": "Acme", "href": fmt.Sprintf("%s/%d", s.srv.URL, id), "product": "bc3"}) } body := map[string]any{ - "identity": map[string]any{"id": s.identityID, "first_name": "Claw", "last_name": "Dito", "email_address": "identity@example.com"}, - "accounts": accounts, - "expires_at": "2036-01-01T00:00:00Z", + "identity": map[string]any{"id": s.identityID, "first_name": "Claw", "last_name": "Dito", "email_address": "identity@example.com"}, + "accounts": accounts, + } + if s.expiresAt != "" { + body["expires_at"] = s.expiresAt } if s.scope != "" { body["scope"] = s.scope @@ -288,15 +292,14 @@ func TestAuthLoginWithTokenCreatesProfileAndVerifiesIdentity(t *testing.T) { creds, err := app.Auth.GetStore().Load("profile:bot") require.NoError(t, err) assert.Equal(t, "bc_at_secret", creds.AccessToken) - assert.Zero(t, creds.ExpiresAt, "a personal access token is stored as non-expiring") + assert.Equal(t, time.Date(2036, 1, 1, 0, 0, 0, 0, time.UTC).Unix(), creds.ExpiresAt, "the expiry the server reported is kept") assert.Empty(t, creds.RefreshToken) assert.Equal(t, "bc5", creds.OAuthType) assert.Equal(t, "full", creds.Scope) assert.Equal(t, "51177542", creds.UserID) assert.Equal(t, "clawdito@example.com", creds.UserEmail) - // Non-expiring: AccessToken serves it without attempting a refresh - // (there is no refresh token or token endpoint to attempt one with). + // Far from expiry: AccessToken serves it without attempting a refresh. tok, err := app.Auth.AccessToken(context.Background()) require.NoError(t, err) assert.Equal(t, "bc_at_secret", tok) @@ -333,8 +336,7 @@ func TestAuthLoginWithTokenJSONEnvelope(t *testing.T) { assert.Equal(t, "token", data["source"]) assert.Equal(t, "bc5", data["oauth_type"]) assert.Equal(t, "full", data["scope"]) - assert.Contains(t, data, "expires_at") - assert.Nil(t, data["expires_at"]) + assert.Equal(t, "2036-01-01T00:00:00Z", data["expires_at"]) assert.Equal(t, true, data["profile_created"]) assert.Equal(t, true, data["default"]) assert.Equal(t, float64(28142355), data["identity"].(map[string]any)["id"]) @@ -844,6 +846,7 @@ func TestAuthLoginWithTokenBindsAnUnboundExistingProfile(t *testing.T) { Profiles: map[string]*config.ProfileConfig{"bot": {ProjectID: "42"}}, } app, _ := loginTestApp(t, srv, cfg) + app.Config.Sources["profiles"] = "global" require.NoError(t, os.MkdirAll(config.GlobalConfigDir(), 0o700)) require.NoError(t, os.WriteFile(filepath.Join(config.GlobalConfigDir(), "config.json"), []byte(`{"profiles":{"bot":{"base_url":"`+srv.srv.URL+`","project_id":"42"}}}`), 0o600)) @@ -924,3 +927,56 @@ func TestAuthLoginDeviceFlowWithoutExpectationKeepsBestEffortIdentity(t *testing assert.Equal(t, "dev-tok", creds.AccessToken) assert.Empty(t, creds.UserID, "no person was resolved, so none is recorded") } + +func TestAuthLoginWithTokenWithoutReportedExpiryIsNonExpiring(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + srv.expiresAt = "" + app, buf := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + app.Flags.JSON = true + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.NoError(t, err) + creds, err := app.Auth.GetStore().Load("profile:bot") + require.NoError(t, err) + assert.Zero(t, creds.ExpiresAt) + + var envelope struct { + Data map[string]any `json:"data"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope)) + assert.Contains(t, envelope.Data, "expires_at") + assert.Nil(t, envelope.Data["expires_at"]) +} + +func TestAuthLoginWithTokenRefusesToBindANonGlobalProfile(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + cfg := &config.Config{ + ActiveProfile: "bot", + Profiles: map[string]*config.ProfileConfig{"bot": {}}, + Sources: map[string]string{"profiles": "repo"}, + } + app, _ := loginTestApp(t, srv, cfg) + withAccount(app, "999", "flag") + + in := strings.NewReader("bc_at_secret") + _, err := runLogin(t, app, in, "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), "defined outside the global config") + assert.Contains(t, err.Error(), "repo config") + assert.Equal(t, 12, in.Len(), "refused before stdin is read") + assert.Empty(t, srv.seenBearers()) +} + +func TestAuthLoginRejectsPositionalArguments(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + + in := strings.NewReader("bc_at_secret") + _, err := runLogin(t, app, in, "--with-token", "bc_at_pasted") + require.Error(t, err) + assert.Contains(t, err.Error(), "unknown command") + assert.Equal(t, 12, in.Len()) + assert.Empty(t, srv.seenBearers()) +} From 17cad546787b77b07f8a57ef1323bc66bb63300e Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 2 Sep 2026 14:49:58 -0700 Subject: [PATCH 5/8] Judge a profile binding by the global file's own entry, refuse near-expiry tokens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Config layers merge profiles per name and record one aggregate source for the whole map, so cfg.Sources["profiles"] cannot say where a particular profile came from — a global unbound profile was refused whenever a repo or local config contributed any other profile. Read the global file's entry instead: binding proceeds when that entry exists and is unbound, and is refused (before stdin) when it is missing or already bound, since the accountless effective profile then came from another layer and would keep shadowing whatever is written. An imported token has nothing to refresh with, so one that the server reports as expiring inside the refresh window would be refused by its first command; the import now declines it up front. RefreshWindow is exported from the auth package for that check instead of repeating the five-minute literal. --- internal/auth/auth.go | 13 +++-- internal/commands/auth.go | 17 +++++-- internal/commands/auth_login_test.go | 76 +++++++++++++++++++++++----- internal/commands/profile.go | 44 ++++++++++++++-- 4 files changed, 126 insertions(+), 24 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index ed838677..622d1b82 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -55,6 +55,13 @@ const ( scopeFull = "full" ) +// RefreshWindow is how long before its expiry a stored access token stops +// being served as-is: AccessToken refreshes inside this window, and a +// credential with nothing to refresh with (an imported token) is refused +// there instead. Exported so an import can decline a token that would be +// unusable from its first command. +const RefreshWindow = 5 * time.Minute + // Default OAuth callback address and redirect URI. const ( defaultCallbackAddr = "127.0.0.1:8976" @@ -137,7 +144,7 @@ func (m *Manager) AccessToken(ctx context.Context) (string, error) { // Check if token is expired (with 5 minute buffer). // ExpiresAt==0 means non-expiring token (e.g., from BASECAMP_TOKEN env var), // so only refresh if ExpiresAt > 0 and is within the expiry window. - if creds.ExpiresAt > 0 && time.Now().Unix() >= creds.ExpiresAt-300 { + if creds.ExpiresAt > 0 && time.Now().Unix() >= creds.ExpiresAt-int64(RefreshWindow.Seconds()) { if err := m.refreshLocked(ctx, credKey, creds); err != nil { return "", err } @@ -168,8 +175,8 @@ func (m *Manager) StoredAccessToken(ctx context.Context) (string, error) { return "", output.ErrAuth(fmt.Sprintf("No stored credentials for %s: %v", credKey, err)) } - // Check if token is expired (with 5 minute buffer) - if creds.ExpiresAt > 0 && time.Now().Unix() >= creds.ExpiresAt-300 { + // Check if token is expired (with the refresh-window buffer) + if creds.ExpiresAt > 0 && time.Now().Unix() >= creds.ExpiresAt-int64(RefreshWindow.Seconds()) { if err := m.refreshLocked(ctx, credKey, creds); err != nil { // Preserve the original error type (API, network, etc.) return "", err diff --git a/internal/commands/auth.go b/internal/commands/auth.go index 9cb35cdd..db1bbc4a 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -424,11 +424,13 @@ func runLoginWithToken(cmd *cobra.Command, app *appctx.App, scope string, expect case existing.AccountID == "" && !accountGivenExplicitly(app): return output.ErrUsageHint(fmt.Sprintf("Profile %q has no account", name), "Pass --account to bind it alongside the imported token.") - case existing.AccountID == "" && config.Source(app.Config.Sources["profiles"]) != config.SourceGlobal: - // Binding rewrites the global config file; a profile that arrived - // from a system, repo or local config would keep shadowing it. - return output.ErrUsageHint(fmt.Sprintf("Profile %q has no account and is defined outside the global config", name), - fmt.Sprintf("Add account_id to its entry in the %s config, then rerun the import.", app.Config.Sources["profiles"])) + case existing.AccountID == "" && !globalProfileIsUnbound(name): + // Binding rewrites the global config file. The effective profile + // is accountless, so if the global entry is missing or already + // carries an account, the accountless one came from a system, repo + // or local config and would keep shadowing whatever is written. + return output.ErrUsageHint(fmt.Sprintf("Profile %q has no account and is not the global config's entry", name), + "Add account_id to the config file that defines it, then rerun the import.") case existing.AccountID == "": bindAccount = true case !accountIDsEqual(account, existing.AccountID): @@ -694,6 +696,11 @@ func (v *loginVerifier) verify(ctx context.Context, accessToken, oauthType strin if v.strict && who.IdentityID <= 0 { return output.ErrAuth("The authorization endpoint did not report an identity for the new credential; nothing was stored") } + // An imported token has nothing to refresh with, so one already inside + // the refresh window could not serve a single command once stored. + if v.strict && !who.ExpiresAt.IsZero() && time.Until(who.ExpiresAt) <= auth.RefreshWindow { + return output.ErrAuth(fmt.Sprintf("The credential expires at %s, within the %s the CLI keeps clear of expiry; nothing was stored — mint a fresh token", who.ExpiresAt.UTC().Format(time.RFC3339), auth.RefreshWindow)) + } if v.expectIdentity != 0 && who.IdentityID != v.expectIdentity { return output.ErrAuth(fmt.Sprintf("Authenticated as %s, not identity %d; nothing was stored", who.label(), v.expectIdentity)) } diff --git a/internal/commands/auth_login_test.go b/internal/commands/auth_login_test.go index ee1722d5..d964da71 100644 --- a/internal/commands/auth_login_test.go +++ b/internal/commands/auth_login_test.go @@ -949,23 +949,73 @@ func TestAuthLoginWithTokenWithoutReportedExpiryIsNonExpiring(t *testing.T) { assert.Nil(t, envelope.Data["expires_at"]) } -func TestAuthLoginWithTokenRefusesToBindANonGlobalProfile(t *testing.T) { - srv := startLoginIdentityServer(t, "bc_at_secret") - cfg := &config.Config{ - ActiveProfile: "bot", - Profiles: map[string]*config.ProfileConfig{"bot": {}}, - Sources: map[string]string{"profiles": "repo"}, +// TestAuthLoginWithTokenBindsOnlyTheGlobalConfigsOwnEntry: config layers +// merge per profile name, so cfg.Sources["profiles"] is aggregate and says +// nothing about where this profile came from. The global file's entry is +// the evidence: bind when it exists and is unbound, refuse otherwise — +// before stdin is read. +func TestAuthLoginWithTokenBindsOnlyTheGlobalConfigsOwnEntry(t *testing.T) { + cases := map[string]struct { + global string + sources string + wantErr string + }{ + "global file has no such profile": { + global: `{"profiles":{"other":{"base_url":"https://3.basecampapi.com"}}}`, + sources: "repo", + wantErr: "not the global config's entry", + }, + "global entry is already bound (shadowed by an accountless layer)": { + global: `{"profiles":{"bot":{"base_url":"https://3.basecampapi.com","account_id":"111"}}}`, + sources: "local", + wantErr: "not the global config's entry", + }, + "global entry is unbound even though another layer contributed profiles": { + global: `{"profiles":{"bot":{"base_url":"BASE","project_id":"42"}}}`, + sources: "repo", + }, } - app, _ := loginTestApp(t, srv, cfg) + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + cfg := &config.Config{ + ActiveProfile: "bot", + Profiles: map[string]*config.ProfileConfig{"bot": {}}, + Sources: map[string]string{"profiles": tc.sources}, + } + app, _ := loginTestApp(t, srv, cfg) + withAccount(app, "999", "flag") + require.NoError(t, os.MkdirAll(config.GlobalConfigDir(), 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(config.GlobalConfigDir(), "config.json"), + []byte(strings.ReplaceAll(tc.global, "BASE", srv.srv.URL)), 0o600)) + + in := strings.NewReader("bc_at_secret") + out, err := runLogin(t, app, in, "--with-token") + if tc.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErr) + assert.Equal(t, 12, in.Len(), "refused before stdin is read") + assert.Empty(t, srv.seenBearers()) + return + } + require.NoError(t, err, out) + bot := readGlobalConfig(t)["profiles"].(map[string]any)["bot"].(map[string]any) + assert.Equal(t, "999", bot["account_id"]) + assert.Equal(t, "42", bot["project_id"]) + }) + } +} + +func TestAuthLoginWithTokenRefusesATokenInsideTheRefreshWindow(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + srv.expiresAt = time.Now().Add(2 * time.Minute).UTC().Format(time.RFC3339) + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) withAccount(app, "999", "flag") - in := strings.NewReader("bc_at_secret") - _, err := runLogin(t, app, in, "--with-token") + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") require.Error(t, err) - assert.Contains(t, err.Error(), "defined outside the global config") - assert.Contains(t, err.Error(), "repo config") - assert.Equal(t, 12, in.Len(), "refused before stdin is read") - assert.Empty(t, srv.seenBearers()) + assert.Contains(t, err.Error(), "within the 5m0s the CLI keeps clear of expiry") + assertNothingStored(t, app, "bot") } func TestAuthLoginRejectsPositionalArguments(t *testing.T) { diff --git a/internal/commands/profile.go b/internal/commands/profile.go index 57b21a9f..793f9274 100644 --- a/internal/commands/profile.go +++ b/internal/commands/profile.go @@ -7,6 +7,7 @@ import ( "path/filepath" "regexp" "sort" + "strconv" "github.com/spf13/cobra" @@ -437,17 +438,41 @@ func registerProfile(name string, p *config.ProfileConfig) (isDefault bool, err return isDefault, atomicWriteJSON(configPath, configData) } -// bindProfileAccount sets the account on an existing profile entry in the -// global config file, leaving every other field of the entry as it is. -func bindProfileAccount(name, account string) error { +// loadGlobalConfigFile returns the global config file's contents and path (an +// empty map when the file is absent or unreadable). +func loadGlobalConfigFile() (map[string]any, string) { configPath := filepath.Join(config.GlobalConfigDir(), "config.json") configData := make(map[string]any) if data, err := os.ReadFile(configPath); err == nil { //nolint:gosec // G304: Path is from trusted config location _ = json.Unmarshal(data, &configData) } + return configData, configPath +} +// globalProfileEntry returns the named profile's entry in the global config +// file, or nil when the file has none. +func globalProfileEntry(configData map[string]any, name string) map[string]any { profilesMap, _ := configData["profiles"].(map[string]any) entry, _ := profilesMap[name].(map[string]any) + return entry +} + +// globalProfileIsUnbound reports whether the global config file defines the +// profile without an account — the one shape bindProfileAccount can act on. +// Config layers merge per profile name, so the effective profile being +// accountless says nothing about which file it came from; the global entry +// itself is the evidence. +func globalProfileIsUnbound(name string) bool { + configData, _ := loadGlobalConfigFile() + entry := globalProfileEntry(configData, name) + return entry != nil && getStringOrNumber(entry, "account_id") == "" +} + +// bindProfileAccount sets the account on an existing profile entry in the +// global config file, leaving every other field of the entry as it is. +func bindProfileAccount(name, account string) error { + configData, configPath := loadGlobalConfigFile() + entry := globalProfileEntry(configData, name) if entry == nil { return output.ErrUsage(fmt.Sprintf("Profile %q not found in %s", name, configPath)) } @@ -455,6 +480,19 @@ func bindProfileAccount(name, account string) error { return atomicWriteJSON(configPath, configData) } +// getStringOrNumber reads a config value that may be stored as a string or +// a JSON number, as config.loadFromFile accepts for IDs. +func getStringOrNumber(m map[string]any, key string) string { + switch v := m[key].(type) { + case string: + return v + case float64: + return strconv.FormatFloat(v, 'f', -1, 64) + default: + return "" + } +} + // unregisterProfile removes a profile entry from the global config file, // clearing default_profile when it named this profile. Credentials are the // caller's to remove. From 709798bd9072eaf4531290d7041090a4aabb63e5 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 2 Sep 2026 15:20:23 -0700 Subject: [PATCH 6/8] Refuse to rewrite a malformed config, and limit the expiry refusal to imports Every writer of the global config file read it with parse errors ignored and then wrote the map back, so a token import that registered a profile could replace an operator's malformed-but-present config with a partial decode. loadGlobalConfigFile now treats only a missing file as empty and returns read and parse errors; register, unregister, bind and set-default all go through it and refuse before writing. The near-expiry refusal exists because an imported token has nothing to refresh with. An asserted OAuth login (--expect-identity) reaches the same verifier with a refresh token in hand, so the check is keyed to the import (noRefresh) rather than to strict mode. Imported credentials record source: "token", and auth status and profile show report it, so a machine consumer can tell a non-refreshable personal token from an OAuth credential. --- internal/auth/auth.go | 5 +++ internal/auth/device_test.go | 2 +- internal/auth/keyring.go | 5 +++ internal/commands/auth.go | 24 +++++++++-- internal/commands/auth_login_test.go | 62 ++++++++++++++++++++++++++++ internal/commands/profile.go | 62 +++++++++++++++++----------- 6 files changed, 130 insertions(+), 30 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 622d1b82..1551d6e4 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -55,6 +55,10 @@ const ( scopeFull = "full" ) +// CredentialSourceToken marks a credential imported from a personal access +// token rather than obtained through an OAuth flow. +const CredentialSourceToken = "token" + // RefreshWindow is how long before its expiry a stored access token stops // being served as-is: AccessToken refreshes inside this window, and a // credential with nothing to refresh with (an imported token) is refused @@ -797,6 +801,7 @@ func (m *Manager) ImportToken(token, scope, userID, userEmail string, expiresAt Scope: scope, UserID: userID, UserEmail: userEmail, + Source: CredentialSourceToken, } if !expiresAt.IsZero() { creds.ExpiresAt = expiresAt.Unix() diff --git a/internal/auth/device_test.go b/internal/auth/device_test.go index 7fc5d05c..47da09a8 100644 --- a/internal/auth/device_test.go +++ b/internal/auth/device_test.go @@ -1092,7 +1092,7 @@ func TestImportToken(t *testing.T) { creds, err := m.store.Load("profile:bot") require.NoError(t, err) - assert.Equal(t, &Credentials{AccessToken: "bc_at_secret", OAuthType: "bc5", Scope: "full", UserID: "51177542", UserEmail: "bot@example.com"}, creds) + assert.Equal(t, &Credentials{AccessToken: "bc_at_secret", OAuthType: "bc5", Scope: "full", UserID: "51177542", UserEmail: "bot@example.com", Source: "token"}, creds) assert.Zero(t, creds.ExpiresAt) // Non-expiring: served as-is, with no refresh attempted (there is no diff --git a/internal/auth/keyring.go b/internal/auth/keyring.go index 8fb20fcc..19c88a4f 100644 --- a/internal/auth/keyring.go +++ b/internal/auth/keyring.go @@ -22,6 +22,11 @@ type Credentials struct { UserID string `json:"user_id,omitempty"` UserEmail string `json:"user_email,omitempty"` + // Source records how the credential was obtained when that is not the + // OAuth flow: "token" for an imported personal access token, which has + // no refresh material. Empty means an OAuth login. + Source string `json:"source,omitempty"` + // Resource is the RFC 8707 resource indicator the tokens are bound to // (BC5: urn:bc:account:). BC5 device logins as the trusted // basecamp-cli client mint MULTI-ACCOUNT refresh tokens, and the refresh diff --git a/internal/commands/auth.go b/internal/commands/auth.go index db1bbc4a..2c9c84ae 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -100,9 +100,13 @@ func newAuthStatusCmd() *cobra.Command { effectiveScope = "" } + source := "oauth" + if creds.Source != "" { + source = creds.Source + } status := map[string]any{ "authenticated": true, - "source": "oauth", + "source": source, "oauth_type": creds.OAuthType, } if effectiveScope != "" { @@ -410,6 +414,14 @@ func runLoginWithToken(cmd *cobra.Command, app *appctx.App, scope string, expect // another. account := app.Config.AccountID existing := app.Config.Profiles[name] + globalUnbound := false + if existing != nil && existing.AccountID == "" { + unbound, err := globalProfileIsUnbound(name) + if err != nil { + return err + } + globalUnbound = unbound + } var created *config.ProfileConfig bindAccount := false switch { @@ -424,7 +436,7 @@ func runLoginWithToken(cmd *cobra.Command, app *appctx.App, scope string, expect case existing.AccountID == "" && !accountGivenExplicitly(app): return output.ErrUsageHint(fmt.Sprintf("Profile %q has no account", name), "Pass --account to bind it alongside the imported token.") - case existing.AccountID == "" && !globalProfileIsUnbound(name): + case existing.AccountID == "" && !globalUnbound: // Binding rewrites the global config file. The effective profile // is accountless, so if the global entry is missing or already // carries an account, the accountless one came from a system, repo @@ -446,7 +458,7 @@ func runLoginWithToken(cmd *cobra.Command, app *appctx.App, scope string, expect return err } - verifier := &loginVerifier{app: app, expectIdentity: expect, account: account, strict: true} + verifier := &loginVerifier{app: app, expectIdentity: expect, account: account, strict: true, noRefresh: true} if err := verifier.verify(cmd.Context(), token, "bc5"); err != nil { return err } @@ -661,6 +673,10 @@ type loginVerifier struct { expectIdentity int64 account string strict bool + // noRefresh marks a credential with nothing to refresh with (an + // imported token): one already inside the refresh window is refused, + // where an OAuth login's refresh token would simply renew it. + noRefresh bool who *loginIdentity } @@ -698,7 +714,7 @@ func (v *loginVerifier) verify(ctx context.Context, accessToken, oauthType strin } // An imported token has nothing to refresh with, so one already inside // the refresh window could not serve a single command once stored. - if v.strict && !who.ExpiresAt.IsZero() && time.Until(who.ExpiresAt) <= auth.RefreshWindow { + if v.noRefresh && !who.ExpiresAt.IsZero() && time.Until(who.ExpiresAt) <= auth.RefreshWindow { return output.ErrAuth(fmt.Sprintf("The credential expires at %s, within the %s the CLI keeps clear of expiry; nothing was stored — mint a fresh token", who.ExpiresAt.UTC().Format(time.RFC3339), auth.RefreshWindow)) } if v.expectIdentity != 0 && who.IdentityID != v.expectIdentity { diff --git a/internal/commands/auth_login_test.go b/internal/commands/auth_login_test.go index d964da71..b36af0be 100644 --- a/internal/commands/auth_login_test.go +++ b/internal/commands/auth_login_test.go @@ -298,6 +298,7 @@ func TestAuthLoginWithTokenCreatesProfileAndVerifiesIdentity(t *testing.T) { assert.Equal(t, "full", creds.Scope) assert.Equal(t, "51177542", creds.UserID) assert.Equal(t, "clawdito@example.com", creds.UserEmail) + assert.Equal(t, "token", creds.Source) // Far from expiry: AccessToken serves it without attempting a refresh. tok, err := app.Auth.AccessToken(context.Background()) @@ -1030,3 +1031,64 @@ func TestAuthLoginRejectsPositionalArguments(t *testing.T) { assert.Equal(t, 12, in.Len()) assert.Empty(t, srv.seenBearers()) } + +func TestAuthStatusReportsAnImportedToken(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, buf := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.NoError(t, err) + + cmd := NewAuthCmd() + cmd.SetArgs([]string{"status"}) + cmd.SetContext(appctx.WithApp(context.Background(), app)) + cmd.SetOut(&bytes.Buffer{}) + require.NoError(t, cmd.Execute()) + var envelope struct { + Data map[string]any `json:"data"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope), buf.String()) + assert.Equal(t, "token", envelope.Data["source"], "a machine consumer can tell an imported token from an OAuth login") + assert.Equal(t, "bc5", envelope.Data["oauth_type"]) +} + +func TestAuthLoginWithTokenRefusesToRewriteMalformedConfig(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + require.NoError(t, os.MkdirAll(config.GlobalConfigDir(), 0o700)) + configPath := filepath.Join(config.GlobalConfigDir(), "config.json") + require.NoError(t, os.WriteFile(configPath, []byte("{ not json"), 0o600)) + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), "not valid JSON") + data, readErr := os.ReadFile(configPath) + require.NoError(t, readErr) + assert.Equal(t, "{ not json", string(data), "the operator's file is left exactly as it was") + _, loadErr := app.Auth.GetStore().Load("profile:bot") + assert.Error(t, loadErr, "the entry comes before the credential, so nothing is stored") +} + +// TestAuthLoginDeviceFlowExpectIdentityKeepsAShortLivedAccessToken: the +// near-expiry refusal is for imports, which cannot refresh. An asserted +// OAuth login whose access token is about to expire has a refresh token +// and is stored. +func TestAuthLoginDeviceFlowExpectIdentityKeepsAShortLivedAccessToken(t *testing.T) { + srv := startLoginIdentityServer(t, "dev-tok") + srv.expiresAt = time.Now().Add(2 * time.Minute).UTC().Format(time.RFC3339) + srv.srv.Config.Handler = deviceGrantThen(t, srv.srv.Config.Handler) + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot", Profiles: map[string]*config.ProfileConfig{"bot": {AccountID: "999"}}}) + withAccount(app, "999", "profile") + t.Setenv("BASECAMP_OAUTH_ISSUER", srv.srv.URL) + t.Setenv("SSH_CONNECTION", "") + t.Setenv("SSH_CLIENT", "") + t.Setenv("SSH_TTY", "") + + out, err := runLogin(t, app, strings.NewReader(""), "--device-code", "--expect-identity", "28142355") + require.NoError(t, err, out) + creds, err := app.Auth.GetStore().Load("profile:bot") + require.NoError(t, err) + assert.Equal(t, "dev-ref", creds.RefreshToken) + assert.Empty(t, creds.Source) +} diff --git a/internal/commands/profile.go b/internal/commands/profile.go index 793f9274..80dc23c2 100644 --- a/internal/commands/profile.go +++ b/internal/commands/profile.go @@ -157,6 +157,9 @@ func newProfileShowCmd() *cobra.Command { if err == nil && creds.AccessToken != "" { result["authenticated"] = true result["oauth_type"] = creds.OAuthType + if creds.Source != "" { + result["source"] = creds.Source + } isLaunchpad = creds.OAuthType == "launchpad" // Suppress credential scope for Launchpad (scopes not supported) @@ -378,13 +381,10 @@ func newProfileSetDefaultCmd() *cobra.Command { return output.ErrUsage(fmt.Sprintf("Profile %q not found", name)) } - // Update config file - configPath := filepath.Join(config.GlobalConfigDir(), "config.json") - configData := make(map[string]any) - if data, err := os.ReadFile(configPath); err == nil { //nolint:gosec // G304: Path is from trusted config location - _ = json.Unmarshal(data, &configData) + configData, configPath, err := loadGlobalConfigFile() + if err != nil { + return err } - configData["default_profile"] = name if err := atomicWriteJSON(configPath, configData); err != nil { @@ -403,14 +403,12 @@ func newProfileSetDefaultCmd() *cobra.Command { // profile registered becomes the default; isDefault reports whether this one // did. The in-memory config is the caller's to update. func registerProfile(name string, p *config.ProfileConfig) (isDefault bool, err error) { - configPath := filepath.Join(config.GlobalConfigDir(), "config.json") if err := os.MkdirAll(config.GlobalConfigDir(), 0700); err != nil { return false, fmt.Errorf("failed to create config directory: %w", err) } - - configData := make(map[string]any) - if data, err := os.ReadFile(configPath); err == nil { //nolint:gosec // G304: Path is from trusted config location - _ = json.Unmarshal(data, &configData) + configData, configPath, err := loadGlobalConfigFile() + if err != nil { + return false, err } profilesMap, _ := configData["profiles"].(map[string]any) @@ -438,15 +436,24 @@ func registerProfile(name string, p *config.ProfileConfig) (isDefault bool, err return isDefault, atomicWriteJSON(configPath, configData) } -// loadGlobalConfigFile returns the global config file's contents and path (an -// empty map when the file is absent or unreadable). -func loadGlobalConfigFile() (map[string]any, string) { +// loadGlobalConfigFile returns the global config file's contents and path. +// A missing file is an empty config; a file that cannot be read or parsed +// is an error, since every caller is about to write the map back and would +// otherwise replace whatever the operator had with a partial decode. +func loadGlobalConfigFile() (map[string]any, string, error) { configPath := filepath.Join(config.GlobalConfigDir(), "config.json") configData := make(map[string]any) - if data, err := os.ReadFile(configPath); err == nil { //nolint:gosec // G304: Path is from trusted config location - _ = json.Unmarshal(data, &configData) + data, err := os.ReadFile(configPath) //nolint:gosec // G304: Path is from trusted config location + if os.IsNotExist(err) { + return configData, configPath, nil + } + if err != nil { + return nil, configPath, fmt.Errorf("failed to read config file %s: %w", configPath, err) + } + if err := json.Unmarshal(data, &configData); err != nil { + return nil, configPath, fmt.Errorf("config file %s is not valid JSON, refusing to rewrite it: %w", configPath, err) } - return configData, configPath + return configData, configPath, nil } // globalProfileEntry returns the named profile's entry in the global config @@ -462,16 +469,22 @@ func globalProfileEntry(configData map[string]any, name string) map[string]any { // Config layers merge per profile name, so the effective profile being // accountless says nothing about which file it came from; the global entry // itself is the evidence. -func globalProfileIsUnbound(name string) bool { - configData, _ := loadGlobalConfigFile() +func globalProfileIsUnbound(name string) (bool, error) { + configData, _, err := loadGlobalConfigFile() + if err != nil { + return false, err + } entry := globalProfileEntry(configData, name) - return entry != nil && getStringOrNumber(entry, "account_id") == "" + return entry != nil && getStringOrNumber(entry, "account_id") == "", nil } // bindProfileAccount sets the account on an existing profile entry in the // global config file, leaving every other field of the entry as it is. func bindProfileAccount(name, account string) error { - configData, configPath := loadGlobalConfigFile() + configData, configPath, err := loadGlobalConfigFile() + if err != nil { + return err + } entry := globalProfileEntry(configData, name) if entry == nil { return output.ErrUsage(fmt.Sprintf("Profile %q not found in %s", name, configPath)) @@ -497,10 +510,9 @@ func getStringOrNumber(m map[string]any, key string) string { // clearing default_profile when it named this profile. Credentials are the // caller's to remove. func unregisterProfile(name string) error { - configPath := filepath.Join(config.GlobalConfigDir(), "config.json") - configData := make(map[string]any) - if data, err := os.ReadFile(configPath); err == nil { //nolint:gosec // G304: Path is from trusted config location - _ = json.Unmarshal(data, &configData) + configData, configPath, err := loadGlobalConfigFile() + if err != nil { + return err } if profilesMap, ok := configData["profiles"].(map[string]any); ok { From 283c4f97a436c44f17b8e8d6758b108ea054284a Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 2 Sep 2026 15:54:43 -0700 Subject: [PATCH 7/8] Pin an origin only, refuse a non-object profiles value, and name app.basecamp.com MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BASECAMP_OAUTH_ISSUER derives its endpoints by appending /oauth/... to the value, so a path would produce endpoints nothing serves; only an origin is accepted now (no path, no opaque form). The global config writers refuse a "profiles" value that is not an object for the same reason they refuse a parse failure, and loadGlobalConfigFile creates the config directory so every writer — set-default included — can run before a global config exists. The README names app.basecamp.com for the token page and the pinned issuer. --- README.md | 4 +- internal/auth/auth.go | 4 +- internal/auth/device_test.go | 17 +++++--- internal/commands/auth_login_test.go | 18 +++++++++ internal/commands/profile.go | 60 +++++++++++++++++++--------- internal/commands/profile_test.go | 15 +++++++ 6 files changed, 89 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index 72b2b323..fb22d382 100644 --- a/README.md +++ b/README.md @@ -203,7 +203,7 @@ it to the server, so the approval page is not preselected. ### Personal access tokens -A [personal access token](https://3.basecamp.com/my/access_tokens) can be +A [personal access token](https://app.basecamp.com/my/access_tokens) can be imported instead of running OAuth — the shape for bots, CI, and any machine that should never sign in interactively. The token is read from stdin (never an argument), verified against the server — who it authenticates as, and that @@ -245,7 +245,7 @@ To use your own OAuth app (e.g., a custom Launchpad integration): Both `BASECAMP_OAUTH_CLIENT_ID` and `BASECAMP_OAUTH_CLIENT_SECRET` must be set together. -`BASECAMP_OAUTH_ISSUER=https://3.basecamp.com` pins the OAuth authorization +`BASECAMP_OAUTH_ISSUER=https://app.basecamp.com` pins the OAuth authorization server and skips discovery, so `basecamp auth login` reaches a server that is serving piloted clients but not yet advertising itself (discovery still 404s). It is a temporary escape hatch for that dark pilot, not a configuration diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 1551d6e4..a4169c57 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -899,8 +899,8 @@ const oauthIssuerEnv = "BASECAMP_OAUTH_ISSUER" func pinnedIssuerDiscovery(issuer string, log func(string)) (*discovery, error) { issuer = strings.TrimRight(strings.TrimSpace(issuer), "/") u, err := url.Parse(issuer) - if err != nil || !isSecureEndpointURL(u) || u.RawQuery != "" || u.ForceQuery || u.Fragment != "" { - return nil, output.ErrAuth("invalid " + oauthIssuerEnv + ": must be an absolute https URL (or http on loopback) with a hostname and no userinfo, query, or fragment") + if err != nil || u.Opaque != "" || !isSecureEndpointURL(u) || u.Path != "" || u.RawQuery != "" || u.ForceQuery || u.Fragment != "" { + return nil, output.ErrAuth("invalid " + oauthIssuerEnv + ": must be an origin — an absolute https URL (or http on loopback) with a hostname and no path, userinfo, query, or fragment") } deviceEndpoint := issuer + "/oauth/device_authorizations" cfg := &oauth.Config{ diff --git a/internal/auth/device_test.go b/internal/auth/device_test.go index 47da09a8..7f9e7915 100644 --- a/internal/auth/device_test.go +++ b/internal/auth/device_test.go @@ -1046,6 +1046,8 @@ func TestDiscoverOAuth_PinnedIssuerIsCheckedLikeAnEndpoint(t *testing.T) { "file scheme": "file:///etc/passwd", "query string": "https://as.example/?token=hunter2", "fragment": "https://as.example/#frag", + "path": "https://as.example/some/path", + "opaque": "https:as.example", } { t.Run(name, func(t *testing.T) { resource, discoveryHits := countingServer(t) @@ -1184,15 +1186,18 @@ func TestLoginDevice_LoginHintIsSanitizedForTheTerminal(t *testing.T) { } func TestDiscoverOAuth_PinnedIssuerIsSanitizedForTheTerminal(t *testing.T) { - as := startDeviceAS(t) resource, _ := countingServer(t) m := newDeviceTestManager(t, resource.URL) - // A C1 control in the path survives url.Parse and the endpoint checks; - // the log line must not carry it to the terminal. - t.Setenv("BASECAMP_OAUTH_ISSUER", as.srv.URL+"/\u0085x") + // Only an origin is accepted now, so a control character can only ride + // in the host — where url.Parse or the endpoint check refuses it. Either + // way the terminal never sees it: refused values are not echoed, and an + // accepted one is announced through the sanitizer. + t.Setenv("BASECAMP_OAUTH_ISSUER", "http://127.0.0.1\u0085:3001") cl := &collectLogger{} - _, _ = m.Login(context.Background(), LoginOptions{Remote: true, Logger: cl.log, deviceOptions: []oauth.DeviceOption{instantSleep()}}) - assert.Contains(t, cl.joined(), "pinned by BASECAMP_OAUTH_ISSUER") + _, err := m.Login(context.Background(), LoginOptions{Remote: true, Logger: cl.log, deviceOptions: []oauth.DeviceOption{instantSleep()}}) + require.Error(t, err) + assert.Contains(t, err.Error(), "BASECAMP_OAUTH_ISSUER") + assert.NotContains(t, err.Error(), "\u0085") assert.NotContains(t, cl.joined(), "\u0085") } diff --git a/internal/commands/auth_login_test.go b/internal/commands/auth_login_test.go index b36af0be..26a5f69d 100644 --- a/internal/commands/auth_login_test.go +++ b/internal/commands/auth_login_test.go @@ -1092,3 +1092,21 @@ func TestAuthLoginDeviceFlowExpectIdentityKeepsAShortLivedAccessToken(t *testing assert.Equal(t, "dev-ref", creds.RefreshToken) assert.Empty(t, creds.Source) } + +func TestAuthLoginWithTokenRefusesToRewriteANonObjectProfilesValue(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{ActiveProfile: "bot"}) + withAccount(app, "999", "flag") + require.NoError(t, os.MkdirAll(config.GlobalConfigDir(), 0o700)) + configPath := filepath.Join(config.GlobalConfigDir(), "config.json") + require.NoError(t, os.WriteFile(configPath, []byte(`{"profiles":[],"format":"json"}`), 0o600)) + + _, err := runLogin(t, app, strings.NewReader("bc_at_secret"), "--with-token") + require.Error(t, err) + assert.Contains(t, err.Error(), `"profiles" value that is not an object`) + data, readErr := os.ReadFile(configPath) + require.NoError(t, readErr) + assert.Equal(t, `{"profiles":[],"format":"json"}`, string(data)) + _, loadErr := app.Auth.GetStore().Load("profile:bot") + assert.Error(t, loadErr) +} diff --git a/internal/commands/profile.go b/internal/commands/profile.go index 80dc23c2..e6415e38 100644 --- a/internal/commands/profile.go +++ b/internal/commands/profile.go @@ -403,17 +403,13 @@ func newProfileSetDefaultCmd() *cobra.Command { // profile registered becomes the default; isDefault reports whether this one // did. The in-memory config is the caller's to update. func registerProfile(name string, p *config.ProfileConfig) (isDefault bool, err error) { - if err := os.MkdirAll(config.GlobalConfigDir(), 0700); err != nil { - return false, fmt.Errorf("failed to create config directory: %w", err) - } configData, configPath, err := loadGlobalConfigFile() if err != nil { return false, err } - - profilesMap, _ := configData["profiles"].(map[string]any) - if profilesMap == nil { - profilesMap = make(map[string]any) + profilesMap, err := globalProfilesMap(configData, configPath) + if err != nil { + return false, err } entry := map[string]any{ @@ -426,7 +422,6 @@ func registerProfile(name string, p *config.ProfileConfig) (isDefault bool, err entry["scope"] = p.Scope } profilesMap[name] = entry - configData["profiles"] = profilesMap isDefault = len(profilesMap) == 1 if isDefault { @@ -436,11 +431,15 @@ func registerProfile(name string, p *config.ProfileConfig) (isDefault bool, err return isDefault, atomicWriteJSON(configPath, configData) } -// loadGlobalConfigFile returns the global config file's contents and path. -// A missing file is an empty config; a file that cannot be read or parsed -// is an error, since every caller is about to write the map back and would -// otherwise replace whatever the operator had with a partial decode. +// loadGlobalConfigFile returns the global config file's contents and path, +// with the config directory in place for the write every caller is about +// to make. A missing file is an empty config; a file that cannot be read or +// parsed is an error, since writing the map back would otherwise replace +// whatever the operator had with a partial decode. func loadGlobalConfigFile() (map[string]any, string, error) { + if err := os.MkdirAll(config.GlobalConfigDir(), 0700); err != nil { + return nil, "", fmt.Errorf("failed to create config directory: %w", err) + } configPath := filepath.Join(config.GlobalConfigDir(), "config.json") configData := make(map[string]any) data, err := os.ReadFile(configPath) //nolint:gosec // G304: Path is from trusted config location @@ -456,8 +455,27 @@ func loadGlobalConfigFile() (map[string]any, string, error) { return configData, configPath, nil } +// globalProfilesMap returns the config's "profiles" object, creating it in +// the map when absent. A present value of any other shape is refused for +// the same reason a parse failure is: the caller is about to write the map +// back, and replacing an unexpected value would destroy operator config. +func globalProfilesMap(configData map[string]any, configPath string) (map[string]any, error) { + raw, present := configData["profiles"] + if !present { + profilesMap := make(map[string]any) + configData["profiles"] = profilesMap + return profilesMap, nil + } + profilesMap, ok := raw.(map[string]any) + if !ok { + return nil, fmt.Errorf("config file %s has a \"profiles\" value that is not an object, refusing to rewrite it", configPath) + } + return profilesMap, nil +} + // globalProfileEntry returns the named profile's entry in the global config -// file, or nil when the file has none. +// file, or nil when the file has none (or its profiles value is not an +// object — which the writers refuse separately). func globalProfileEntry(configData map[string]any, name string) map[string]any { profilesMap, _ := configData["profiles"].(map[string]any) entry, _ := profilesMap[name].(map[string]any) @@ -485,6 +503,9 @@ func bindProfileAccount(name, account string) error { if err != nil { return err } + if _, err := globalProfilesMap(configData, configPath); err != nil { + return err + } entry := globalProfileEntry(configData, name) if entry == nil { return output.ErrUsage(fmt.Sprintf("Profile %q not found in %s", name, configPath)) @@ -514,12 +535,13 @@ func unregisterProfile(name string) error { if err != nil { return err } - - if profilesMap, ok := configData["profiles"].(map[string]any); ok { - delete(profilesMap, name) - if len(profilesMap) == 0 { - delete(configData, "profiles") - } + profilesMap, err := globalProfilesMap(configData, configPath) + if err != nil { + return err + } + delete(profilesMap, name) + if len(profilesMap) == 0 { + delete(configData, "profiles") } if dp, ok := configData["default_profile"].(string); ok && dp == name { diff --git a/internal/commands/profile_test.go b/internal/commands/profile_test.go index 917e9929..cd76f3c4 100644 --- a/internal/commands/profile_test.go +++ b/internal/commands/profile_test.go @@ -1134,3 +1134,18 @@ func TestProfileCreateWithNilProfilesMap(t *testing.T) { require.True(t, ok, "expected profiles map in config") assert.Contains(t, profiles, "new-profile", "new profile should be created") } + +// TestProfileSetDefaultCreatesTheConfigDir: the global config directory may +// not exist yet when the profiles came from another config layer. +func TestProfileSetDefaultCreatesTheConfigDir(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + t.Setenv("XDG_CONFIG_HOME", filepath.Join(t.TempDir(), "fresh")) + + app, _ := setupTestApp(t) + app.Config.Profiles = map[string]*config.ProfileConfig{"bot": {BaseURL: "https://3.basecampapi.com"}} + + require.NoError(t, executeCommand(NewProfileCmd(), app, "set-default", "bot")) + data, err := os.ReadFile(filepath.Join(config.GlobalConfigDir(), "config.json")) + require.NoError(t, err) + assert.Contains(t, string(data), `"default_profile": "bot"`) +} From 45bc77b385929690bedee490b828a8881189788a Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 2 Sep 2026 16:25:23 -0700 Subject: [PATCH 8/8] Prove the config can take a profile before profile create logs in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit profile create registers its entry after the OAuth login, and the global config writers now refuse a malformed file, so a refusal there would have left a live credential with no profile. The file is checked for writability before the login runs; nothing is written by the check. The README no longer calls an imported token non-expiring — its expiry is whatever the server reports. --- README.md | 4 ++-- internal/commands/profile.go | 19 +++++++++++++++++++ internal/commands/profile_test.go | 23 +++++++++++++++++++++++ 3 files changed, 44 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index fb22d382..7567959d 100644 --- a/README.md +++ b/README.md @@ -207,8 +207,8 @@ A [personal access token](https://app.basecamp.com/my/access_tokens) can be imported instead of running OAuth — the shape for bots, CI, and any machine that should never sign in interactively. The token is read from stdin (never an argument), verified against the server — who it authenticates as, and that -it can reach the profile's account — and only then stored as a non-expiring -credential under a named profile: +it can reach the profile's account — and only then stored under a named +profile, with whatever expiry the server reports for it: ```bash op read "op://Vault/Item/credential" | basecamp auth login --with-token -P bot --account 999 diff --git a/internal/commands/profile.go b/internal/commands/profile.go index e6415e38..9c4fc0e0 100644 --- a/internal/commands/profile.go +++ b/internal/commands/profile.go @@ -236,6 +236,13 @@ Examples: profileCfg.AccountID = accountID } + // The entry is written only after the login succeeds, so prove + // the config file can take it before a credential exists to + // orphan: a malformed file is refused here, not after OAuth. + if _, err := writableGlobalProfiles(); err != nil { + return err + } + // Snapshot in-memory config before mutation prevActiveProfile := app.Config.ActiveProfile prevBaseURL := app.Config.BaseURL @@ -473,6 +480,18 @@ func globalProfilesMap(configData map[string]any, configPath string) (map[string return profilesMap, nil } +// writableGlobalProfiles reports whether the global config file can take a +// profile entry — readable, parseable, with an object (or absent) profiles +// value — without writing anything. Callers that obtain a credential before +// registering its profile run this first. +func writableGlobalProfiles() (map[string]any, error) { + configData, configPath, err := loadGlobalConfigFile() + if err != nil { + return nil, err + } + return globalProfilesMap(configData, configPath) +} + // globalProfileEntry returns the named profile's entry in the global config // file, or nil when the file has none (or its profiles value is not an // object — which the writers refuse separately). diff --git a/internal/commands/profile_test.go b/internal/commands/profile_test.go index cd76f3c4..eee79b32 100644 --- a/internal/commands/profile_test.go +++ b/internal/commands/profile_test.go @@ -1149,3 +1149,26 @@ func TestProfileSetDefaultCreatesTheConfigDir(t *testing.T) { require.NoError(t, err) assert.Contains(t, string(data), `"default_profile": "bot"`) } + +// TestProfileCreateRefusesAMalformedConfigBeforeLogin: registration happens +// after OAuth, so a config file that cannot take the entry must be refused +// before a credential exists to orphan. +func TestProfileCreateRefusesAMalformedConfigBeforeLogin(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + require.NoError(t, os.MkdirAll(config.GlobalConfigDir(), 0o700)) + configPath := filepath.Join(config.GlobalConfigDir(), "config.json") + require.NoError(t, os.WriteFile(configPath, []byte(`{"profiles":[]}`), 0o600)) + + // Any network use would be a login attempt; the no-network transport + // fails instantly, and the assertion below is on the refusal wording. + app, _ := setupTestApp(t) + err := executeCommand(NewProfileCmd(), app, "create", "bot", "--device-code") + require.Error(t, err) + assert.Contains(t, err.Error(), `"profiles" value that is not an object`) + data, readErr := os.ReadFile(configPath) + require.NoError(t, readErr) + assert.Equal(t, `{"profiles":[]}`, string(data)) + _, loadErr := app.Auth.GetStore().Load("profile:bot") + assert.Error(t, loadErr, "no login may have run") +}