Import personal access tokens with auth login --with-token - #681
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
github-actions
Bot
added
commands
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 <profile> --account <id>` 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 <id>` 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 <email>` 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.
0282f11 to
b1ddc87
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
You’re at about 97% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0282f1166d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🟡 Changes recommended
Critical account-scoped verification and non-atomic credential rollback issues must be resolved before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds personal access-token login, identity assertions, OAuth issuer pinning, and safer non-interactive authentication behavior.
Changes:
- Adds PAT import through named profiles with identity verification and JSON output.
- Adds login hints, expected-identity checks, and machine-output guards.
- Updates profile handling, documentation, tests, and CLI surface metadata.
File summaries
| File | Description |
|---|---|
skills/basecamp/SKILL.md |
Documents token imports and identity assertions. |
README.md |
Documents PAT imports and issuer pinning. |
internal/stdinarg/stdinarg.go |
Adds terminal-input detection. |
internal/stdinarg/stdinarg_test.go |
Tests terminal detection. |
internal/commands/profile.go |
Extracts profile registration helpers. |
internal/commands/auth.go |
Implements token login and identity verification. |
internal/commands/auth_login_test.go |
Tests token-login behavior and failures. |
internal/cli/root.go |
Allows login to create named profiles. |
internal/cli/root_test.go |
Tests profile creation resolution. |
internal/auth/device_test.go |
Tests issuer pinning, hints, and token credentials. |
internal/auth/auth.go |
Adds token import and OAuth options. |
internal/auth/auth_test.go |
Tests Launchpad hint handling. |
e2e/auth.bats |
Adds CLI-level authentication tests. |
.surface |
Records the new login flags. |
Review details
Suppressed comments (3)
README.md:200
- This implies the approval page receives
--login-hint, but the current implementation only prints the email locally and sends no hint to the server. Clarify that limitation so operators do not rely on the page being preselected.
`--expect-identity <id>` 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 <email>` suggests which
account to sign in as on the device-flow approval page.
internal/auth/auth.go:859
- The comment says the issuer is never echoed, but the success path logs the full validated value at line 873 (and the test requires that output). Either remove the value from the log to honor the promise or clarify that only rejected values are not echoed; the current security documentation is self-contradictory.
issuer = strings.TrimRight(strings.TrimSpace(issuer), "/")
u, err := url.Parse(issuer)
internal/commands/auth_login_test.go:381
- This assertion cannot pass for an existing profile: the server scope is written only to the credential by
setStoredScope;existing.Scopeis never mutated. Since the PR also promises that an existing profile entry stays untouched, assert that behavior here rather than expecting an in-memory rewrite.
assert.Equal(t, "51177542", creds.UserID, "identity survives the scope correction")
- Files reviewed: 14/14 changed files
- Comments generated: 14
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved verification, persistence, account-scoping, and output-sanitization issues can violate the authentication guarantees.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (9)
README.md:200
- This currently implies the hint reaches the approval page, but this release only prints the hint locally; the pending SDK change is what will put
login_hinton the device authorization request. State that limitation here so operators do not assume the server will select the intended identity.
kept) and the command exits non-zero. `--login-hint <email>` suggests which
account to sign in as on the device-flow approval page.
internal/auth/auth.go:350
- This API comment says
LoginHintsteers the sign-in page, but the current implementation explicitly only announces it locally; the SDK support that would send it is still pending. Document the current behavior so callers do not rely on a hint reaching the authorization server.
// LoginHint names the account (email address) the user should sign in
// as; it steers the sign-in page and never authenticates on its own.
// Device flow only — Launchpad's authorization-code flow ignores it.
internal/auth/auth.go:856
- This comment says the issuer is never echoed, but line 870 logs every validated issuer and the success test asserts that output. Clarify that only rejected values are suppressed.
// again by loginDevice before any POST. The value is never echoed: like a
// base URL, it can carry userinfo or a query string.
internal/commands/auth.go:614
/my/profile.jsonis account-scoped, but the raw SDK client does not add an account prefix (internal/appctx/context.go:121explicitly requiresapp.Account()for such calls;internal/names/resolver_test.go:607expects/{account}/my/profile.json). In production this requests the resource root and the strict token-import path rejects every valid token. Resolve and validate the effective account, then fetch the person through the account client; the tests should assert the account-prefixed path.
resp, err := app.SDK.Get(ctx, "/my/profile.json")
internal/commands/auth.go:564
- Every
Loaderror is treated as “no previous credential.” A transient keyring/read error can therefore be followed by a successful overwrite, and a failed identity check then deletes the newly written value instead of restoring the previous credential, violating the rollback guarantee. Distinguish credential-not-found from other load errors and abort before login/import when the snapshot cannot be read.
prev, err := store.Load(key)
if err != nil {
prev = nil
}
internal/commands/auth.go:593
NameandEmailcome from server responses and this label is written directly to terminal output, so embedded OSC/CSI or newlines can inject terminal content. Sanitize both fields withrichtext.SanitizeSingleLinebefore composing the one-line label, consistent withinternal/richtext/sanitize.go:35-48.
label := l.Name
if l.Email != "" {
label += " <" + l.Email + ">"
internal/commands/auth.go:315
Logindurably replaces the profile credential before--expect-identityis evaluated. If the process receives SIGINT/SIGTERM or crashes after OAuth completes but before verification/rollback, the possibly wrong browser identity remains stored—the exact shared-browser case this assertion is meant to prevent. Stage the new credential (for example via a temporary/in-memory token provider), verify it, and commit it only after the expectation passes.
restore := credentialRestorer(app)
result, err := app.Auth.Login(cmd.Context(), auth.LoginOptions{
internal/commands/auth.go:417
- The imported token is persisted before either identity request runs. A termination or crash during verification leaves an unverified token behind (and permanently overwrites an existing profile credential), despite the command's guarantee that nothing is kept until verification succeeds. Verify through a temporary token provider first, then save the credential as the final commit step.
restore := credentialRestorer(app)
if err := app.Auth.ImportToken(token, scope); err != nil {
return err
}
who, err := verifyLoginIdentity(cmd.Context(), app, expectIdentity, true)
internal/commands/auth.go:340
- All mismatch-and-rollback tests added here invoke
--with-token; the new browser/device--expect-identitybranch is untested. Add a command-level OAuth test that completes a device or browser login with a mismatching identity and verifies the previous credential is restored, since this is a core behavior advertised for every login flow.
who, err := verifyLoginIdentity(cmd.Context(), app, expectIdentity, expectIdentity != "")
if err != nil {
restore(cmd.ErrOrStderr())
return err
- Files reviewed: 14/14 changed files
- Comments generated: 6
- Review effort level: Balanced
0f1a9be to
b4d1259
Compare
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.
b4d1259 to
14c33b8
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Four moderate authentication correctness and credential-cleanup issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 4
- Review effort level: Balanced
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14c33b8a1a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🟡 Changes recommended
Five moderate authentication and validation issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
internal/commands/auth.go:653
- This account verification is unconditional even when
strictis false. As a result, an ordinary login without--expect-identitynow aborts before storing its credential whenever the authorization document omits the configured account or the subsequent person lookup fails, contradicting the intended best-effort identity lookup and regressing previously successful logins. Only return these verification errors in strict mode; otherwise retain the authorization identity and continue.
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)))
internal/commands/auth.go:348
- When no account is configured, verification can populate
whofrom/authorization.jsonbut leavesPersonIDat zero. This then overwrites the newly stored OAuth credential'suser_idwith the literal"0", whichauth statusandprofile showexpose as if it were a real person ID. Preserve the previous best-effort behavior by writing identity metadata only after an account-scoped person was resolved.
_ = app.Auth.SetUserIdentity(strconv.FormatInt(who.PersonID, 10), who.Email)
internal/commands/auth.go:674
- This textual comparison also rejects a valid non-canonical account spelling such as
--account 0999, because the authorization document's numeric ID is formatted as999. Compare numerically withaccountIDsEqual; otherwise token import can fail even though the token authorizes the requested account.
if strconv.FormatInt(acct.ID, 10) == account && !acct.Expired {
- Files reviewed: 16/16 changed files
- Comments generated: 3
- Review effort level: Balanced
…ffort 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.
There was a problem hiding this comment.
🟡 Changes recommended
Token arguments must be rejected, and authorization-document expiry must be handled correctly.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
One critical and three moderate issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
internal/commands/auth.go:573
- A bare trailing
\ris currently stripped and accepted, even though the documented/validated allowance is only one LF or CRLF. This lets control-character-bearing input bypass the rejection below. Strip\ronly as part of a CRLF pair.
internal/commands/profile.go:273 - The PR says the failing unscoped person lookup in
profile createis addressed, but immediately after this call the command still usesapp.SDK.Get("/my/profile.json"). On production that remains a 404, so a newly created profile still silently omits its user identity. Use the account-scoped candidate-token verification path here as well and cover profile creation with an account-scoped regression test.
README.md:210 - This calls the stored credential non-expiring, but the implementation preserves a server-reported expiry and the next paragraph documents that behavior. Avoid the contradiction by describing it simply as a stored credential.
- Files reviewed: 16/16 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17cad54678
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… 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.
…basecamp.com 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.
There was a problem hiding this comment.
🟡 Changes recommended
Moderate credential-persistence, profile-binding, and token-input validation issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
internal/commands/auth.go:585
- These sequential trims also accept a bare trailing
\r, although the documented/verified contract is exactly one LF or CRLF. That silently changes malformed secret input instead of letting the control-character check reject it; strip the two allowed endings as units.
README.md:250
- The release note and usage example specify
BASECAMP_OAUTH_ISSUER=https://3.basecamp.com, while this documentation useshttps://app.basecamp.com(and the PAT link above changes the same host). Reconcile these before release so the manually pasted release instructions and README direct operators to the same production endpoint.
`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).
internal/commands/profile.go:455
- A top-level JSON
nullunmarshals successfully but setsconfigDatato a nil map. Every writer then panics when assigningprofilesordefault_profile, so this does not fail closed as intended. Rejectnullas a non-object config (and add it to the malformed-config matrix).
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, nil
internal/commands/profile.go:352
unregisterProfilecan now fail on malformed/non-object or unreadable config, but the credential has already been deleted. In that failure path the command returns an error while the profile remains configured and its credential is irreversibly lost. Update the config first, then perform the credential deletion (which is already best-effort).
if err := unregisterProfile(name); err != nil {
return err
}
- Files reviewed: 18/18 changed files
- Comments generated: 3
- Review effort level: Balanced
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.
There was a problem hiding this comment.
🟡 Changes recommended
Critical token-disclosure risks and profile/config consistency defects remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
internal/commands/auth.go:585
- This accepts a bare trailing carriage return because
\ris stripped even when no\npreceded it. That contradicts the single-line validation and lets control-character-bearing input through; strip exactly one CRLF or LF ending so a lone CR reaches the rejection below.
internal/commands/profile.go:462 - A config file containing the valid JSON value
nulldecodes with no error but setsconfigDatato a nil map. Every caller then assigns into it (globalProfilesMaporset-default) and panics instead of refusing the malformed top-level shape. Reject a nil result before returning it.
internal/commands/profile.go:243
- This preflight does not actually check writability:
writableGlobalProfilesonly creates/reads the directory and parses the JSON. With a readable config in an unwritable directory, it succeeds, OAuth storesprofile:<name>, and the laterregisterProfilefails atCreateTemp, leaving the orphaned credential this check is intended to prevent. Reserve/register the profile before OAuth with rollback, or perform an equivalent write/rename check before starting login.
// 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
- Files reviewed: 18/18 changed files
- Comments generated: 4
- Review effort level: Balanced
| func (a *App) SDKClientFor(provider basecamp.TokenProvider) *basecamp.Client { | ||
| cfg := a.SDK.Config() | ||
| return basecamp.NewClient(&cfg, provider, a.SDKOptions...) |
There was a problem hiding this comment.
Not doing this — and it is the third variation of one finding (this thread, auth.go:696 and auth.go:742 all say: a server that reflects the bearer token into an error body could get the candidate token into an error message or a trace file). The failure it would prevent needs the operator's own configured base URL to be a server that echoes Authorization headers back; Basecamp does not, and an operator pointing the CLI at a hostile host has already handed it the token in the request. The same error text reaches verbose output and trace files for every command the CLI runs with the stored credential, so a verification-only redaction would leave the class where it already lives; if the class is worth closing, the place is the observability hooks (redact bearer-shaped substrings once), not three sites in this PR. Leaving this open for Jeremy's call on whether hook-level redaction is wanted.
| if !v.strict { | ||
| return nil | ||
| } | ||
| return output.ErrAuth(fmt.Sprintf("Could not verify the new credential: %v", err)) |
There was a problem hiding this comment.
Not doing this — same class as the appctx.go:175 thread (server-reflected bearer in error text), see the reasoning there. The guarantee this PR makes is that the CLI never prints the token it read from stdin; the SDK error here is what convertSDKError surfaces for every command, and flattening it to a fixed message would drop the one thing an operator needs (401 vs network vs 5xx) to fix a failed import. Left unresolved for Jeremy.
| v.who = who | ||
| return nil | ||
| } | ||
| return output.ErrAuth(fmt.Sprintf("Could not verify %s on account %s: %v", who.label(), v.account, err)) |
There was a problem hiding this comment.
Not doing this — third variation of the server-reflected-bearer class; reasoning on the appctx.go:175 thread. Left unresolved for Jeremy.
| if err := unregisterProfile(name); err != nil { | ||
| return err |
There was a problem hiding this comment.
Flagged for human review — this is the fourth finding in one class: the round-6 decision to refuse a malformed or non-object global config (rather than rewrite it) keeps surfacing new orderings in the pre-existing profile commands. Added so far: loadGlobalConfigFile refuses parse failures (round 6), globalProfilesMap refuses a non-object profiles value (round 7), profile create preflights the file before OAuth (round 8), and this thread asks profile delete to update config before deleting the credential. The reorder is small and I am not disputing it, but I am not writing the next one in this series: the question for Jeremy is whether the refusal belongs in this PR at all (it was pre-existing clobber behavior in profile create/delete/set-default that the review asked to harden) or should move to its own change that reorders all three commands together. Leaving unresolved.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45bc77b385
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // 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} |
There was a problem hiding this comment.
When --expect-identity is used for an existing profile alongside a mismatching --account/BASECAMP_ACCOUNT_ID or BASECAMP_BASE_URL, this verifier validates the token against the overridden authority and Login then saves it under the unchanged profile key. Once the override is removed, the profile reverts to its bound account/server, so a credential verified only for the override can replace a working credential and may subsequently be sent to the wrong server. Fresh evidence beyond the resolved token-import comments is that this ordinary OAuth branch still bypasses the profile-binding checks in runLoginWithToken; apply equivalent checks before starting or storing the OAuth login.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Flagged for human review — this extends the profile-binding-versus-runtime-override class from the token import to the OAuth flows, and it is past the third variation (account override, env account override, base URL override, unbound binding, cross-layer provenance, now OAuth). The failure is real but narrow: an asserted OAuth login (--expect-identity) under a --account / BASECAMP_ACCOUNT_ID / BASECAMP_BASE_URL override verifies against the override and stores under the profile key, and a BC5 token is account-bound, so once the override is dropped the profile's bound account gets a token that may not reach it. It is also pre-existing for plain auth login -P and profile create, which never had a binding check. The right fix is one shared guard (the import's switch, lifted into a helper) applied to every login path that names a profile — a change to the OAuth flows beyond this PR's brief. Leaving unresolved for Jeremy to decide whether that lands here or as a follow-up.
| return err | ||
| } | ||
|
|
||
| token, err := readTokenFromStdin(cmd) |
There was a problem hiding this comment.
When --with-token is creating a profile, this reads and verifies the secret before registerProfile discovers that the global config is unreadable, malformed, or has a non-object profiles value. The command therefore consumes pipeline input and performs both credential-bearing verification requests even though a deterministic local error already makes success impossible; unlike profile create, this path never calls writableGlobalProfiles first. Run that preflight after deciding created != nil and before reading stdin.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Flagged for human review — same config-file-hardening series as the profile.go:358 thread (parse-failure refusal, non-object refusal, profile create preflight, delete ordering, and now the import preflight before stdin). The one-call preflight is genuinely small and consistent with round 8, but per the loop rule I am not adding the fifth variation; if Jeremy wants the series finished it is writableGlobalProfiles() before readTokenFromStdin when created != nil, plus the delete reorder, in one commit. Leaving unresolved.
basecamp-sdk#841 (47e7ca38) adds oauth.WithDeviceLoginHint, so the device flow now carries --login-hint as Basecamp's login_hint extension to the device authorization request instead of telling the user the hint. The option is appended only when a hint was given, so an unqualified login's form is unchanged. Launchpad still logs that it ignores the flag. make bump-sdk REF=main: v0.15.0 → v0.16.1-0.20260903193203-47e7ca381a49, provenance updated; the catalog parity tests pass against the new SDK.
4b075ed to
313c5b0
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Critical config handling and several credential consistency, profile binding, input validation, and SDK-sync issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (8)
Previously missed (3) — in code that hasn't changed since the last review.
internal/commands/auth.go:447
- This can bind the wrong layer. A trusted repo/local config may redefine the same accountless profile name after the global file;
globalProfileIsUnboundsees the shadowed global entry, this branch updates it, and the command succeeds, but the next load replaces it with the still-unbound higher-precedence entry. Track provenance per profile (or otherwise refuse a shadowed definition) before mutating the global entry.
internal/commands/auth.go:497 - An omitted server expiry only means no expiry was reported; it does not prove the revocable token never expires. The terminal output should preserve that distinction, as the JSON
nulland README already do.
internal/commands/auth.go:585 - The two independent suffix trims also accept a bare trailing
\r, although the documented contract permits only one LF or CRLF. Preserve CRLF handling as a unit so a lone carriage return reaches the control-character rejection below.
go.mod:9
- The release-note block and
--login-hintsection still say the hint is only announced locally pending an SDK bump, but this bump is the merged SDK change that addsWithDeviceLoginHint, andauth.gonow sends it. Update the PR description before release prep so it does not publish the old behavior.
github.com/basecamp/basecamp-sdk/go v0.16.1-0.20260903193203-47e7ca381a49
internal/auth/auth.go:658
- The release-note block and the
--login-hintsection of the PR description say this release only announces the hint and sends nothing pending an SDK bump. This code now uses the post-v0.16 SDK option and does transmitlogin_hint, as the linked SDK change specifies. Update the PR/release description before its requested manual publication so it does not describe the opposite behavior.
if opts.LoginHint != "" {
devOpts = append(devOpts, oauth.WithDeviceLoginHint(opts.LoginHint))
}
internal/commands/profile.go:285
- If this write fails after OAuth succeeds (for example because the config directory is read-only or changes during the flow),
Loginhas already savedprofile:<name>and this returns without removing it, leaving the orphaned credential this preflight is intended to prevent. Persist the profile before login with rollback on login failure, or explicitly remove the newly stored credential and restore in-memory state here.
isDefault, err := registerProfile(name, profileCfg)
if err != nil {
return err
internal/commands/profile.go:357
- The credential is deleted before the newly strict config loader runs. If
config.jsonis malformed or has a non-objectprofilesvalue, the command now returns an error after permanently deleting the credential while leaving the profile entry in place. Preflight the global config before mutating the credential store.
if err := unregisterProfile(name); err != nil {
skills/basecamp-doctor/SKILL.md:27
- Using
<id>for both flags implies the account and identity IDs are the same, which can make this remediation reject the intended bot login. Give the two independent identifiers distinct placeholders.
`op read "op://<vault>/<item>/credential" | basecamp auth login --with-token -P <profile> --account <id> --expect-identity <id>`
- Files reviewed: 20/21 changed files
- Comments generated: 2
- Review effort level: Balanced
| 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, nil |
| charm.land/bubbletea/v2 v2.0.9 | ||
| charm.land/lipgloss/v2 v2.0.6 | ||
| github.com/basecamp/basecamp-sdk/go v0.15.0 | ||
| github.com/basecamp/basecamp-sdk/go v0.16.1-0.20260903193203-47e7ca381a49 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 313c5b0205
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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 { |
There was a problem hiding this comment.
When config.json contains the valid JSON value null, this unmarshal succeeds but sets configData to a nil map. The next call to globalProfilesMap assigns configData["profiles"] and panics, so a new auth login --with-token -P bot --account 999 crashes after consuming and verifying the secret instead of returning a configuration error. Explicitly reject a nil/non-object top-level value before returning from this helper.
Useful? React with 👍 / 👎.
| 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 store nothing") |
There was a problem hiding this comment.
When the user needs to create a profile through browser or device OAuth, profile create still calls Auth.Login without a Verify callback and exposes no --expect-identity flag, so it can store credentials for the wrong browser session despite the new assertion feature. The asserted auth login path cannot substitute for this because it rejects an unknown profile at lines 310-314; expose the assertion on profile creation or route that flow through the same verifier before storing the credential.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🟡 Changes recommended
Moderate SDK coverage, token validation, and profile-config safety issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
go.mod:9
- This SDK bump crosses v0.16.0, which the PR description says adds to-do list template-library, recent-projects, and spotlight service methods, but the diff adds no corresponding CLI/catalog/registration work or API-COVERAGE rows. AGENTS.md:131-149 makes those artifacts mandatory for every new SDK service method; deferring them leaves the SDK sync incomplete. Either include that command/coverage work or pin a revision that adds
login_hintwithout introducing uncovered services.
github.com/basecamp/basecamp-sdk/go v0.16.1-0.20260903193203-47e7ca381a49
internal/commands/auth.go:497
- An omitted
expires_atmeans only that the server did not report an expiry; it does not establish that the token never expires. The README and JSON envelope preserve this distinction, but the human success output currently makes the stronger and potentially misleading claim. Report the absence instead.
tokenLine := "personal access token (does not expire)"
internal/commands/profile.go:462
- A config file containing the valid JSON value
nullunmarshals without error but setsconfigDatato a nil map. Every writer then assigns into that map (globalProfilesMap,set-default, etc.), causing a runtime panic instead of a controlled refusal. Reject a nil top-level map before returning.
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, nil
internal/commands/profile.go:357
unregisterProfilecan now fail while reading malformed JSON or a non-objectprofilesvalue, but this call occurs after the credential has already been deleted. In that caseprofile deletereturns an error while irreversibly deleting the secret and leaving the profile entry intact. Validate the global profiles file before deleting credentials so these predictable failures are non-destructive.
if err := unregisterProfile(name); err != nil {
- Files reviewed: 20/21 changed files
- Comments generated: 1
- Review effort level: Balanced
| token := strings.TrimSuffix(string(data), "\n") | ||
| token = strings.TrimSuffix(token, "\r") |
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 (
bc_at_…, revocable, full/read scope), so the CLI can take one from a secret store and refuse to keep it unless it authenticates as the expected identity. This is the CLI half of the Clawdito setup work in basecamp/coworker; the operator-facing flow isWhat
--with-token(internal/commands/auth.go)op read "op://…" | basecamp auth login --with-token -P <profile> --account <id>shape. Empty, multi-line, whitespace-bearing, control-character-bearing, or >4 KiB input is rejected without echoing it. The token is never written to output or errors.-P/--profileorBASECAMP_PROFILE; a single/default profile resolves too). When the profile does not exist,--account <id>is required and the profile is created with the current base URL. An existing profile keeps its entry untouched; passing an--accountthat differs from the profile's binding is a usage error rather than a silent rebind.App.SDKClientFor(StaticTokenProvider)— same transport, hooks and user agent as the real client) and must pass three checks before anything is written: the authorization document answers for it (identity id, email, reported scope — anything butread/fullis refused); the effective account (--account/BASECAMP_ACCOUNT_IDoverride the profile binding at runtime, so that is what is compared; a new profile must get its account explicitly, not from the operator's config file; digits only) appears non-expired in that document; and the account-scoped person lookupForAccount(acct).People().Mesucceeds. Only thenauth.Manager.ImportTokenstores{AccessToken, OAuthType: bc5, Scope, UserID, UserEmail}in one write and the profile is registered. A rejected token touches neither the store norconfig.json, and a profile's previous credential is never read or written. The server-reported expiry is stored with it (ExpiresAt0 when none is reported, the pathAccessTokennever refreshes); near a reported expiry the token is refused with "No refresh token available" and must be imported again./my/profile.jsononly under an account prefix (verified live: unscoped → 404,/2914079/my/profile.json→ 200), which is why the person lookup is account-scoped. The pre-existing "Logged in as" step inlogin/profile createused the unscoped path and had been failing silently.BASECAMP_TOKENin the environment is refused up front for--with-tokenand--expect-identity, since every request would otherwise carry it instead of the credential being checked.profile createintoregisterProfile/unregisterProfile(profile.go), used by create, delete, and the import. The root pre-run gains anAnnotationProfileMayCreatecarve-out so-P <new-name>reaches the command instead of failing with "unknown profile"; an unregistered name is not used as a cache-path component.--expect-identity <id>— the same verifier runs inside the browser and device flows through a newauth.LoginOptions.Verifyhook, called with the freshly issued token beforestore.Save. A mismatch stores nothing, prints no success line, and exits 3 (auth); a pre-existing credential is untouched. Without an expectation the "Logged in as" line stays best-effort. Output:Logged in as Clawdito <clawdito@…> (identity 28142355, person 51177542)(server-supplied name/email reduced to one line for the terminal; JSON verbatim).--json— envelope for the import:profile,account_id,base_url,source: "token",oauth_type: "bc5",scope,expires_at: null,identity: {id, email},person: {id, name, email},profile_created,default. Interactive flows under--json/--agent/--quiet/--ids-only/--countnow refuse before any network call (the machine-mode half of #669;BASECAMP_NONINTERACTIVEis deliberately not gated — a device login prints instructions and waits on a browser, not on stdin, which is a valid headless shape).BASECAMP_OAUTH_ISSUER(internal/auth/auth.go) — when set,discoverOAuthskips discovery and buildsoauth.Config{TokenEndpoint: issuer+"/oauth/tokens", DeviceAuthorizationEndpoint: issuer+"/oauth/device_authorizations"}(the paths bc3 mounts). The value passes the sameisSecureEndpointURLchecks as a discovered endpoint plus a no-query/no-fragment rule, is never echoed, and never falls back to Launchpad. Documented in the README as the dark-pilot escape hatch to remove after go-live.--login-hint <email>— threaded throughLoginOptions.LoginHintand sent by the device flow as Basecamp'slogin_hintextension viaoauth.WithDeviceLoginHint(basecamp/basecamp-sdk#841, merged as 47e7ca38). The option is appended only when a hint was given, so an unqualified login's form is unchanged; Launchpad logs that it ignores the flag. The SDK is bumped withmake bump-sdk REF=main(v0.15.0 → v0.16.1-0.20260903193203-47e7ca381a49, provenance updated, catalog parity tests green); the bump also carries the SDK's own 0.16.0 additions (to-do list template library, recent projects, spotlight), for which the CLI commands and API-COVERAGE rows are a separate SDK-sync task, not this PR.Usage
Verification
go build ./...,go vet ./...— clean.go test ./internal/auth ./internal/commands ./internal/cli ./internal/stdinarg— green. New coverage: token import happy path through a realhttptestidentity server (trailing newline trimmed before theAuthorizationheader; storedExpiresAt0;AccessTokenserves it with no refresh);--jsonenvelope shape;--expect-identitymismatch deletes the credential and registers no profile; mismatch on a pre-existing profile restores its previous credential byte-for-byte; unverifiable identity fails closed only with an expectation; a server-rejected token is not kept; server-reported scope wins; missing--account, missing profile, account mismatch, existing profile's entry (project_id,default_profile) untouched; bad stdin matrix (empty, whitespace, two lines, inner space, control char, oversized) with no echo; terminal stdin refusal (PTY);BASECAMP_TOKENrefusal; non-numeric identity; flag exclusivity; machine-mode refusal matrix; unknown profile without--with-token.internal/auth: pinned issuer skips discovery (0 hits on the resource server) and stores the trimmed/oauth/tokensendpoint; rejected issuer matrix (plain http, userinfo, no host, bad port, file scheme, query, fragment) never echoes and never falls back; login hint sent on the device flow (since round 9) and ignored on Launchpad;ImportTokenround trip and refresh behavior.internal/cli: may-create pass-through for--profileandBASECAMP_PROFILE, refusal otherwise.internal/stdinarg:IsTerminalover buffer, pipe,/dev/null, PTY.make fmt-check vet check-surface check-skill-drift check-bare-groups tidy-check— green;.surfaceregenerated with the six new flag entries (auth loginand theloginshortcut).make test-e2e— green, including four newauth.batscases (help text, profile required,--accountrequired,--with-tokenexcludes--device-code).bin/cigreen on Linux/x86 (thelio, Go 1.26.7) at a2defe8: fmt, vet, lint (0 issues), unit tests, 446 e2e, naming, surface, skill drift, bare groups, lint lockstep, smoke coverage, provenance, tidy. The PTY-class tests (TestIsTerminal,TestAuthLoginWithTokenRefusesTerminalStdin,TestInteractiveStdio,TestIsInteractiveTTY…, …) pass there; on this Mac's sandbox they fail identically onmainbecause/dev/ptmxis not a terminal, and its golangci-lint predates the Go 1.27 export-data format, so Linux is the reference run.--scope/--expect-identityvalidated before stdin is consumed (a test asserts stdin is left unread); device-flow--expect-identitymismatch stores nothing and prints no success line (end-to-end through a pinned issuer serving the grant); sanitization tests for the login hint, the pinned issuer (U+0085) and a server-supplied name (OSC hyperlink + CRLF); unregistered profile name kept out of the cache path (--profile ../../outside); e2e cases assert exact.error/.codeunderenv -u BASECAMP_PROFILE; doctor skill carries the token-import and--expect-identityremediation.user_id0; stdin accepts exactly one trailing LF/CRLF.Args: cobra.NoArgs; binding an accountless profile defined outside the global config is refused before stdin.source: "token", reported byauth statusandprofile show.BASECAMP_OAUTH_ISSUERaccepts an origin only; global config writers refuse a non-objectprofilesvalue and create the config directory first; README names app.basecamp.com.profile createproves the global config can take an entry before running OAuth (no credential left without a profile on a malformed file); README stops calling an imported token non-expiring.--login-hintsent on the wire (TestLoginDevice_LoginHintIsSentasserts the form carrieslogin_hint, and omits it when no hint is given). No review fixes this round: six threads left open on purpose — three server-reflected-bearer findings declined on merit, two config-file-hardening and one OAuth-binding finding flagged for Jeremy as the third-plus variation of settled classes.op-coworker read clawdito … | basecamp auth login --with-token -P clawdito --account 2914079 --expect-identity 28142355) andBASECAMP_OAUTH_ISSUER=https://app.basecamp.com basecamp auth login --device-codewait on the bc3 pilot deploy (work package A/B).Not doing here
BASECAMP_NONINTERACTIVE(the other half ofbasecamp loginblocks and emits prose under machine output / non-interactive environments #669) — see above.profile:<name>.Summary by cubic
Adds
basecamp auth login --with-tokenso bots and CI can import a personal access token from stdin instead of running interactive OAuth, and makes--expect-identityassert who any login authenticated as before saving. The token is verified before it is stored; rejected tokens leave existing credentials untouched.Token import
--accountwhen creating one; refuses mismatched account or base URL bindings.read/fullscope, expiry, andsource: "token";--json,auth status, andprofile showreport them.BASECAMP_TOKENand tokens expiring within the refresh window because imported tokens have no refresh token.profile createand can create a named profile; a config-write failure after verification can leave a new profile without a credential.Authentication and config
--expect-identity <id>verifies browser, device, and token logins before saving; mismatches preserve the previous credential and exit non-zero.--json,--agent,--quiet,--ids-only, and--countbefore network calls, covering the machine-mode half ofbasecamp loginblocks and emits prose under machine output / non-interactive environments #669.github.com/basecamp/basecamp-sdk/goso--login-hintis sent on the wire;BASECAMP_OAUTH_ISSUERaccepts only an origin for temporary device-flow pinning.profile createchecks writability before starting OAuth.Written for commit 4b075ed. Summary will update on new commits.