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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2e8a69624
ℹ️ 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".
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.
There was a problem hiding this comment.
🟡 Changes recommended
Three moderate authentication and profile-handling issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
internal/commands/auth.go:466
- This assignment discards the
app.Config.DefaultProfile == nameresult computed above. Ifdefault_profilealready names this missing profile and another profile exists, registration leaves that default in the file but returnsfalsebecause the profile is not the first one, so both the JSONdefaultfield and human “(default)” label are wrong. Preserve either source of default status and add this stale-default creation case to the tests.
internal/commands/auth.go:571 - These two trims also remove a bare trailing
\r, so input such asbc_at_secret\rbypasses the control-character check and is sent to the server even though only LF/CRLF is allowed. Strip\ronly when it is part of a CRLF ending; the bad-stdin matrix should include a bare-CR case.
- Files reviewed: 16/16 changed files
- Comments generated: 1
- 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: a2defe825c
ℹ️ 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".
…xpiry tokens 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.
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...) |
| if !v.strict { | ||
| return nil | ||
| } | ||
| return output.ErrAuth(fmt.Sprintf("Could not verify the new credential: %v", err)) |
| v.who = who | ||
| return nil | ||
| } | ||
| return output.ErrAuth(fmt.Sprintf("Could not verify %s on account %s: %v", who.label(), v.account, err)) |
| if err := unregisterProfile(name); err != nil { | ||
| return err |
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 👍 / 👎.
| 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 👍 / 👎.
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.LoginHintinto the device flow. The pinned SDK (v0.15.0) has no way to putlogin_hinton the wire; basecamp/basecamp-sdk#841 addsoauth.WithDeviceLoginHint. Until the bump,deviceLoginHintOptionstells the user which account to sign in as and sends nothing; Launchpad logs that the hint is ignored. Noreplacedirective: CONTRIBUTING prescribesgo.workfor local SDK work andmake bump-sdk(which acceptsREF=main, so the bump can follow the SDK merge without waiting for a tag).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 announced on the device flow 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.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
login_hinton the wire — lands with the SDK bump after Send login_hint with the device authorization request basecamp-sdk#841.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-tokento import personal access tokens from stdin instead of using an interactive OAuth login. The CLI verifies the token before storing it; rejected tokens leave the existing credential untouched, while a later config-write failure can leave a newly bound profile without a credential.Token import
readorfullscope, expiry, andsource: "token"in the existing credential store;--json,auth status, andprofile showreport these values.BASECAMP_TOKENand tokens that expire within the refresh window because imported tokens have no refresh token.profile create, and allows the import command to create a named profile.Authentication and config
--expect-identity <id>now 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 portion ofbasecamp loginblocks and emits prose under machine output / non-interactive environments #669.BASECAMP_OAUTH_ISSUERaccepts only an origin for temporary device-flow pinning, while--login-hintis announced locally until SDK support sends it.profile createchecks writability before starting OAuth.Written for commit 45bc77b. Summary will update on new commits.