Skip to content

feat(auth): migrate CLI login to scoped access tokens - #275

Merged
yuaanlin merged 2 commits into
mainfrom
feat/access-token-login
Sep 1, 2026
Merged

yuaanlin merged 2 commits into
mainfrom
feat/access-token-login

Conversation

@yuaanlin

@yuaanlin yuaanlin commented Sep 1, 2026 •

Copy link
Copy Markdown
Member

Summary

Legacy sk- API keys are deprecated. This switches the CLI login flow to the new scoped personal access tokens (zat_) issued by createAccessToken.

The CLI itself never mints a credential: it opens the dashboard confirm page, which mints in the browser session and POSTs back to a localhost callback. The gateway accepts both prefixes as a Bearer token, so the wire format is unchanged. The matching dashboard change is in zeabur/dashboard (confirm page now calls createAccessToken and posts access_token).

Changes

  • pkg/auth/token.go: token prefix constants, IsLegacyAPIKey, shared deprecation message.
  • pkg/auth/callback.go: read access_token form field, fall back to api_key so the CLI works against dashboard builds that predate the change.
  • pkg/auth/const.go: endpoint renamed to ZeaburAccessTokenConfirmEndpoint; URL path unchanged so old CLI builds keep resolving it.
  • auth login: interactive login with a stored legacy key re-runs the browser flow instead of reporting "already logged in". Non-interactive login with a legacy ZEABUR_TOKEN warns but proceeds.
  • Root pre-run and auth status: warn once per command when the stored token is legacy, suppressed in --json mode.
  • Unit tests for the prefix check and the form-field fallback.

Rollout

  1. Ship this CLI first. It still works against the current dashboard via the api_key fallback.
  2. Ship the dashboard change. From then on both new and old CLIs receive zat_ tokens.
  3. Existing users see the deprecation warning until they run zeabur auth login again.

Test plan

  • go build ./..., go vet, go test ./...
  • Live browser login after the dashboard change ships, confirm stored token starts with zat_
  • zeabur auth status with an old sk- token shows the deprecation warning

🤖 Generated with Claude Code

https://claude.ai/code/session_011g8BAHTy8eMxCcZcw9MDG9


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

The dashboard confirm page now mints a `zat_` personal access token
instead of the deprecated unscoped `sk-` API key. The callback accepts
the new `access_token` form field and falls back to `api_key` so the
CLI keeps working against dashboard builds that predate the change.

Users still holding a legacy key get a deprecation warning on every
authenticated command (suppressed in --json mode), and an interactive
`zeabur auth login` re-runs the browser flow instead of reporting
"already logged in" so they can migrate in one step.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011g8BAHTy8eMxCcZcw9MDG9
@opencodezebra

opencodezebra Bot commented Sep 1, 2026

Copy link
Copy Markdown

Review Council started (round 1).

Baseline:

  • Scope: 8 files changed (+93/−5)
  • CI/checks: pending (0 contexts) at f7f6f16

The council is reviewing this pull request; the verdict will follow as a separate comment when the round closes.

@opencodezebra

opencodezebra Bot commented Sep 1, 2026

Copy link
Copy Markdown

CHANGES REQUESTED ⚠️ — The scoped-token migration is sound and secure, but the non-interactive login path silently skips the deprecation warning it claims to emit.
Reviewed at f7f6f16 (round 1)

What This PR Does

Migrates the CLI login flow from legacy sk- API keys to the new scoped personal access tokens (zat_). The CLI never mints a credential itself — the browser session mints it and POSTs it back to the localhost callback. It adds a shared pkg/auth token classifier (IsLegacyAPIKey) and deprecation message, teaches the callback to read the new access_token form field with an api_key fallback for older dashboard builds, renames the confirm endpoint constant (URL path unchanged), and surfaces a legacy-key deprecation warning across auth login, auth status, and the root pre-run. Includes unit tests for the pure helpers.

How It Works

  • pkg/auth/token.go: prefix constants, IsLegacyAPIKey, shared deprecation message — used only for warnings, never for authorization.
  • pkg/auth/callback.go: tokenFromForm prefers access_token, falls back to api_key; state/CSRF validation is unchanged and still gates token acceptance.
  • pkg/auth/const.go: endpoint constant renamed, wire path preserved so old CLIs keep resolving it.
  • auth login / auth status / root pre-run: emit the deprecation warning when a stored token is a legacy key (suppressed in --json); auth's subtree sets DisableAuthCheck so the warning fires once, not twice.

Findings

ID Severity Finding Location
F1 🟡 Non-interactive login with a stored valid legacy sk- token returns "already logged in" before reaching the deprecation warning, so it never warns — contradicting the PR body's "warns but proceeds" claim. No test covers RunLogin branch ordering. (raised by: rev-claude) internal/cmd/auth/login/login.go:62
Finding Details

🟡 F1: Non-interactive already-logged-in legacy path skips the deprecation warning

For a non-interactive zeabur auth login where the config already holds a valid legacy sk- token, the first branch at login.go:60 is skipped (it requires f.Interactive), and control falls to else if f.LoggedIn() at login.go:62. A successful GetUserInfo there prints "Already logged in" and return nil at login.go:78 — before the warning block at login.go:102-104 can run. So the common case (a user/CI with a stored legacy token) gets no login-time deprecation nudge, which contradicts the PR body's claim that "Non-interactive login with a legacy ZEABUR_TOKEN warns but proceeds" (that warning only fires when the token comes from env/flag without an already-logged-in state).

Impact is bounded: the classifier is warning-only and never affects authorization (confirmed by rev-codex), and auth status warns unconditionally (status.go:58-61), so users are not left entirely without signal. Concrete action: either emit auth.LegacyAPIKeyDeprecationMessage in the else if f.LoggedIn() branch before returning, or correct the PR description to match the actual behavior. rev-claude additionally recommends a RunLogin test with a mocked already-valid legacy token in non-interactive mode, since the current token_test.go only exercises the pure helpers in isolation.

What's Good (🟢)
  • Callback remains loopback-only, uses cryptographically random state, and validates state before accepting the token; preferring access_token with an api_key fallback does not weaken that validation (rev-codex).
  • Token-prefix classification affects only migration warnings, not authorization decisions — no auth-bypass surface (rev-codex).
  • Endpoint rename introduces no stale ZeaburApiKeyConfirmEndpoint references and no compile break; wire path preserved for old CLIs (rev-claude).
  • auth status warning is single-fire — auth's subtree sets DisableAuthCheck, so root's pre-run check does not double-print (rev-claude).
  • Backward-compatible rollout: CLI ships first and keeps working against the current dashboard via the api_key fallback.
Baseline Check
  • Scope: 8 files, +93/−5. New pkg/auth helpers plus deprecation warnings across login/status/root.
  • CI/checks: no commit statuses reported at review time (rollup pending; mergeable_state: blocked).
  • Net-new value: enables the zat_ scoped-token migration while remaining compatible with the pre-change dashboard.
  • Test plan's live-login and auth status checkboxes are correctly left unchecked pending the companion zeabur/dashboard PR — not a gap in this PR.
Review Metadata
  • Reviewers: rev-codex (security — approve, no findings), rev-claude (correctness/tests — raised F1)
  • Consensus: split (one approve, one finding)
  • Absent reviewers: none

🔴×0 🟡×1 🟢×5 · 💬 Comment @opencodezebra <question> for a follow-up · 🔁 Push new commits or comment @opencodezebra review <fix notes> to re-run the council

@opencodezebra opencodezebra Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Council request_changes — 🔴0 🟡1 🟢5. Reviewed at f7f6f16. Full report: #275 (comment)

…testpackage lint

Extract reportExistingLogin so the already-logged-in path also emits the
deprecation warning when the stored token is a legacy sk- key. Move the
pkg/auth tests to an external package and exercise the callback server
end to end instead of the unexported helper.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011g8BAHTy8eMxCcZcw9MDG9
@yuaanlin

yuaanlin commented Sep 1, 2026

Copy link
Copy Markdown
Member Author

@opencodezebra review F1 fixed: extracted reportExistingLogin so the already-logged-in non-interactive path now emits the legacy deprecation warning before returning, with login_test.go covering both the legacy and access-token cases. Also fixed the testpackage lint failure by moving pkg/auth tests to an external package.

@opencodezebra

opencodezebra Bot commented Sep 1, 2026

Copy link
Copy Markdown

Review Council started (round 2).

Baseline:

  • Scope: 9 files changed (+223/−19)
  • CI/checks: pending (0 contexts) at 122e69d

The council is reviewing this pull request; the verdict will follow as a separate comment when the round closes.

@opencodezebra

opencodezebra Bot commented Sep 1, 2026

Copy link
Copy Markdown

Review Council started (round 2).

Baseline:

  • Scope: 9 files changed (+223/−19)
  • CI/checks: pending (0 contexts) at 122e69d

The council is reviewing this pull request; the verdict will follow as a separate comment when the round closes.

@opencodezebra

opencodezebra Bot commented Sep 1, 2026

Copy link
Copy Markdown

LGTM ✅ — The round-1 finding F1 is fixed and independently verified; no new findings, both reviewers approve.
Reviewed at 122e69d (round 2)

Delta since f7f6f16

  • Extracted reportExistingLogin in internal/cmd/auth/login/login.go: the non-interactive "already logged in" path with a stored legacy sk- token now emits LegacyAPIKeyDeprecationMessage before returning (F1 fix).
  • Added internal/cmd/auth/login/login_test.go covering both the legacy-warns and access-token-no-warn non-interactive cases via a stub client/config.
  • Moved pkg/auth tests to an external auth_test package (uses only exported symbols) to clear the testpackage lint failure.

Findings

Resolved Severity Finding Fixed in
F1 🟡 Non-interactive already-logged-in legacy path skipped the deprecation warning 122e69d

No outstanding or new findings.

Finding Details

🟡 F1 (Resolved): Non-interactive already-logged-in legacy path skipped the deprecation warning

Round 1 found that a non-interactive zeabur auth login with a stored valid legacy sk- token returned "Already logged in" and return nil before the deprecation warning block could run, contradicting the PR body's "warns but proceeds" claim.

Verified fixed at head 122e69d. RunLogin now routes the already-logged-in case through reportExistingLogin (login.go:120-137): the interactive+legacy case takes the re-login branch, and every other already-logged-in case (including non-interactive legacy) calls reportExistingLogin, which prints "Already logged in as …" and then, when auth.IsLegacyAPIKey(token) is true, f.Log.Warn(auth.LegacyAPIKeyDeprecationMessage) before returning (true, nil). Unauthorized-detection and error-wrapping behavior are preserved via the extracted isUnauthorized helper. login_test.go adds TestRunLogin_NonInteractiveLegacyTokenWarns and ...AccessTokenDoesNotWarn, which assert the warning fires for sk- and not for zat_. Confirmed independently by the chair and by rev-claude (correctness) and rev-codex (security, who additionally confirmed the warn happens only after successful server-side token validation and changes no authorization decision).

What's Good (🟢)
  • Callback stays loopback-only and validates cryptographically random state before accepting the token; preferring access_token with an api_key fallback does not weaken that validation (rev-codex).
  • Token-prefix classification affects only migration warnings, never authorization — no auth-bypass surface (rev-codex).
  • The testpackage lint fix moves pkg/auth tests to an external package using only exported symbols; no stale ZeaburApiKeyConfirmEndpoint references after the rename (rev-claude).
  • New login_test.go exercises the actual fixed code path via a stub client/config, not just the pure helpers.
  • Backward-compatible rollout preserved: CLI ships first and keeps working against the current dashboard via the api_key fallback.
Baseline Check
  • Delta scope: 3 files touched since round 1 (login.go, new login_test.go, pkg/auth test package move); PR total 9 files, +223/−19.
  • CI/checks at head 122e69d: lint and build-test green; Analyze (go) in progress at read time (non-blocking). mergeable_state: blocked reflects the pending required review, not a failing check.
  • Net-new value: enables the zat_ scoped-token migration while remaining compatible with the pre-change dashboard.
  • Test-plan's live-login and auth status boxes remain correctly unchecked pending the companion zeabur/dashboard PR — not a gap in this PR.
Review Metadata
  • Reviewers: rev-codex (security — approve, no findings, F1 confirmed resolved), rev-claude (correctness/tests — approve, F1 confirmed resolved)
  • Consensus: approve
  • Absent reviewers: none

🔴×0 🟡×0 🟢×5 · 💬 Comment @opencodezebra <question> for a follow-up · 🔁 Push new commits or comment @opencodezebra review <fix notes> to re-run the council

@opencodezebra opencodezebra Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Council approve — 🔴0 🟡0 🟢5. Reviewed at 122e69d. Full report: #275 (comment)

@yuaanlin
yuaanlin merged commit 73e25aa into main Sep 1, 2026
6 checks passed
@yuaanlin
yuaanlin deleted the feat/access-token-login branch September 1, 2026 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant