Skip to content

fix(auth): retry stale scoped token once after 403#895

Open
steipete wants to merge 10 commits into
mainfrom
codex/fix-auth-scope-refresh
Open

fix(auth): retry stale scoped token once after 403#895
steipete wants to merge 10 commits into
mainfrom
codex/fix-auth-scope-refresh

Conversation

@steipete

@steipete steipete commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • retry one replayable Google API request after a 403 response explicitly reports insufficient authentication scopes
  • limit forced refresh/remint behavior to stored user OAuth; preserve ordinary 403s, direct tokens, ADC, service accounts, and non-replayable requests
  • serialize token refresh/read state, preserve inspected response bodies, and recognize Google's insufficientPermissions and singular Insufficient Permission response variants

Closes #889.

Supersedes #890 after maintainer hardening while retaining its original implementation commit.

Root cause

Since 1531013e, the CLI persists and reuses a still-valid access token. golang.org/x/oauth2 returns that cached token without contacting the token endpoint until expiry. If Google transiently rejects that token as under-scoped, the current retry transport treats the first non-429 4xx as terminal. The candidate now discards the rejected cached access token, mints through the stored refresh token, persists returned metadata, and replays once.

Final exact-head proof

Exact head: ac67db3a4dc4124d7659f96fc1152d0ca421bb32

  • make ci
  • focused scope/retry regressions passed 20 consecutive runs
  • focused race coverage passed
  • final branch autoreview clean; no accepted/actionable findings; confidence 0.82
  • built candidate: v0.31.1-18-gac67db3a; SHA-256 c03f2e3cff64f75bf661f07c087a3df316667b149b7e5b1f37abb6125016e1a4
  • authenticated dedicated-test-account Contacts and Calendar read-only calls exited 0 with valid JSON
  • source-blind behavior validation: 4 pass, 0 fail, 1 blocked; recovery/replay, ordinary-403, non-replayable-body, and live baseline clauses passed on the predecessor head; final follow-ups have exact detector-variant and failed-refresh response-preservation coverage

Review repairs

  • preserve the cached source when forced token minting fails
  • serialize ordinary token persistence with forced refresh so an in-flight stale token cannot overwrite a newer token
  • recognize Google's camelCase insufficientPermissions reason and singular permission message while leaving ordinary permission denials untouched
  • return the intact original Google 403 when forced refresh fails, preserving existing re-auth guidance; focused regression proves one refresh attempt, no replay, and exact status/body preservation
  • rebase onto current main, preserving the youtube videos list: add opt-in --parts all without changing defaults #871 changelog entry

Live-proof caveat

Google's intermittent first-response 403 cannot be safely or deterministically forced on the real provider. The exact recovery branch is covered by replay, refresh, concurrency, ordinary-403, non-replayable-body, detector-variant, and response-preservation tests; final-head authenticated Contacts and Calendar smoke is green. Merge should wait for an item-specific waiver of the unforceable provider-403 proof requirement, or for a naturally observed redacted reproduction.

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 189b8ab0be

ℹ️ 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".

Comment thread internal/googleapi/client_auth.go Outdated
@clawsweeper

clawsweeper Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 1, 2026, 9:53 AM ET / 13:53 UTC.

Summary
The PR adds a stored-user-OAuth-only one-time refresh-and-retry path for replayable Google API 403 insufficient-scope responses, plus token-source serialization and regression coverage.

Reproducibility: Do we have a high-confidence way to reproduce the issue? No: source and tests model the retry path, and authenticated smoke was reported green, but the real intermittent first-response Google 403 cannot be safely forced on demand.

Review metrics: 2 noteworthy metrics.

  • Auth files changed: 4 auth implementation/test files changed. The behavior touches shared Google API auth and retry plumbing rather than a single command surface.
  • Regression coverage added: 487 test-line additions across 2 test files. The expanded tests are the main evidence for an auth-provider branch that cannot be forced safely live.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #889
Summary: This PR is the current implementation candidate for the open intermittent Google insufficient-scope 403 issue, superseding the earlier closed generated PR.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Maintainer should explicitly record whether the unforceable intermittent Google 403 branch is waived for this PR.

Risk before merge

  • [P1] Merging changes stored OAuth token-source caching, forced remint behavior, and token metadata persistence; green CI cannot fully prove live Google auth-provider semantics.
  • [P1] The exact intermittent first-response insufficient-scope 403 could not be safely induced against Google, so maintainers must accept the harness/live-smoke evidence or wait for a natural redacted reproduction.
  • [P1] The PR author association is COLLABORATOR, so ClawSweeper cleanup should not close it automatically even if the patch now looks clean.

Maintainer options:

  1. Waive the unforceable provider branch (recommended)
    If maintainers accept the targeted transport tests, race coverage, authenticated smoke calls, and CI as sufficient, they can explicitly waive the live intermittent-403 proof gate and proceed with normal merge checks.
  2. Wait for natural live proof
    Keep the PR open until a redacted real Google response naturally hits the intermittent insufficient-scope 403 and demonstrates the one-retry recovery path.

Next step before merge

  • [P2] Human maintainer review is needed to waive or require live-provider proof for the intermittent Google 403 branch; there is no remaining narrow automated code repair visible on the latest head.

Security
Cleared: No concrete security or supply-chain regression was found in the diff; the remaining concern is auth-provider behavior and upgrade proof, not secret exposure or dependency execution.

Review details

Best possible solution:

A maintainer should either explicitly waive the unforceable live-provider branch after reviewing the focused tests and smoke proof, or wait for a natural redacted reproduction before merging the auth retry behavior.

Do we have a high-confidence way to reproduce the issue?

Do we have a high-confidence way to reproduce the issue? No: source and tests model the retry path, and authenticated smoke was reported green, but the real intermittent first-response Google 403 cannot be safely forced on demand.

Is this the best way to solve the issue?

Is this the best way to solve the issue? Unclear pending maintainer sign-off: a narrow stored-user-OAuth-only one-time replay is plausible and now covered, but accepting mock/harness proof for the provider branch is an auth-provider product and risk decision.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 621757c686ee.

Label changes

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body and follow-up comments provide exact-head CI, focused race/regression runs, authenticated Contacts/Calendar smoke, and an explicit caveat for the unforceable intermittent provider branch.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body and follow-up comments provide exact-head CI, focused race/regression runs, authenticated Contacts/Calendar smoke, and an explicit caveat for the unforceable intermittent provider branch.
  • remove rating: 🦐 gold shrimp: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.
  • remove status: ⏳ waiting on author: Current PR status label is status: 👀 ready for maintainer look.

Label justifications:

  • P2: This is a normal-priority auth reliability fix with limited scope but meaningful OAuth-provider behavior risk.
  • merge-risk: 🚨 auth-provider: The diff changes stored OAuth token refresh/retry behavior and token persistence around Google insufficient-scope 403 responses.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body and follow-up comments provide exact-head CI, focused race/regression runs, authenticated Contacts/Calendar smoke, and an explicit caveat for the unforceable intermittent provider branch.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body and follow-up comments provide exact-head CI, focused race/regression runs, authenticated Contacts/Calendar smoke, and an explicit caveat for the unforceable intermittent provider branch.
Evidence reviewed

What I checked:

  • Repository policy read: AGENTS.md was read fully and applied; its guidance to keep stdout parseable is not directly implicated, while the PR workflow note that CHANGELOG.md is release-owned informed that the changelog edit is not treated as a finding. (AGENTS.md:1, 621757c686ee)
  • PR diff: retry hook wiring: authenticatedTransportWithStoredScopeCheck now wires ForceRefresh into RetryTransport only when the stored token source exposes that method, keeping direct tokens/other auth paths outside the forced-refresh path. (internal/googleapi/client.go:148, ac67db3a4dc4)
  • PR diff: response preservation on refresh failure: RetryTransport inspects a 403 insufficient-scope response, attempts RefreshAuth before closing the response body, and returns the original response unchanged if refresh fails. (internal/googleapi/transport.go:140, ac67db3a4dc4)
  • PR diff: focused regressions: The new tests cover one retry for insufficient-scope 403s, no retry for ordinary 403s/non-replayable requests, detector variants, failed-refresh response preservation, and token refresh serialization. (internal/googleapi/transport_more_test.go:229, ac67db3a4dc4)
  • PR discussion: proof caveat: The PR body and maintainer comments report make ci, focused race/regression runs, authenticated Contacts/Calendar smoke, and hosted CI, but explicitly say the intermittent first-response Google 403 cannot be safely forced and needs a waiver or natural redacted reproduction. (ac67db3a4dc4)
  • History: cached token behavior root: git log shows commit 1531013 added cached-token persistence in internal/googleapi/client_auth.go, matching the PR body's root-cause claim that still-valid cached access tokens are reused until expiry. (internal/googleapi/client_auth.go:35, 1531013e9496)

Likely related people:

  • steipete: Current-main history shows steipete authored the cached-token persistence work tied to this retry behavior and has recent changes in the auth transport/client area. (role: recent area contributor; confidence: high; commits: 1531013e9496; files: internal/googleapi/client_auth.go, internal/googleapi/client.go)
  • clawsweeper[bot]: The closed predecessor implementation for the same issue was authored by clawsweeper[bot] and the current PR states it retains that original implementation commit before maintainer hardening. (role: initial candidate author; confidence: medium; commits: aed01d23ca7d; files: internal/googleapi/transport.go, internal/googleapi/client_auth.go)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. labels Jul 1, 2026
@steipete steipete force-pushed the codex/fix-auth-scope-refresh branch from 189b8ab to 2b213d8 Compare July 1, 2026 08:25
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jul 1, 2026
@steipete steipete force-pushed the codex/fix-auth-scope-refresh branch from 842b5b0 to b4d8e96 Compare July 1, 2026 10:24

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b4d8e96f14

ℹ️ 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".

Comment thread internal/googleapi/transport.go
@steipete

steipete commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator Author

Maintainer closeout on exact head b4d8e96f143bc35a0f9661e38934ac4d2aaa5b18:

  • rebased cleanly onto current main; branch is mergeable
  • local make ci, package race tests, and 20x serialization stress are green
  • final branch autoreview is clean with no actionable findings
  • authenticated dedicated-account Contacts and Calendar Meet create/read/delete smoke is green; cleanup verified
  • source-blind behavior validation: 4 pass, 0 fail, 1 blocked
  • exact-head hosted CI and Docker are green: CI, push CI, Docker

The single remaining gate is unchanged: Google's intermittent first-response insufficient-scope 403 did not occur naturally and cannot be safely induced against the real provider. Please explicitly waive that item-specific live-provider branch requirement to authorize merge, or provide a redacted natural reproduction. I have left the PR unmerged.

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jul 1, 2026
@steipete

steipete commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the later ClawSweeper P2 finding on exact head 070af732079cf435532c8868fb31b8ac7089164f:

  • internal/googleapi/transport.go now recognizes Google's insufficientPermissions reason plus singular/plural permission messages.
  • internal/googleapi/transport_more_test.go covers those variants and verifies the inspected response body remains readable.
  • Focused tests passed 20 consecutive runs; focused race coverage and full make ci are green.
  • Final branch autoreview reports no accepted/actionable findings (0.86).
  • Exact binary v0.31.1-17-g070af732, SHA-256 e2a8d20af0b29a5aa0f2cb17f82f86ece49eaca3068d78ed31aa2c74fe79461b, passed authenticated read-only Contacts and Calendar calls on the dedicated test account.

The one remaining gate is unchanged: Google's intermittent first-response under-scope 403 cannot be safely forced against the live provider. Please grant the item-specific live-proof waiver before merge, or provide a naturally observed redacted reproduction. I am leaving the clean, green PR unmerged pending that decision.

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 070af73207

ℹ️ 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".

Comment thread internal/googleapi/transport.go Outdated
Comment on lines +155 to +156
if err := t.RefreshAuth(req.Context()); err != nil {
return nil, fmt.Errorf("refresh auth after insufficient scopes response: %w", err)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Return the original 403 when forced refresh fails

When a stored grant is genuinely missing the requested scope, the first Google API call returns the useful insufficient-scope 403, but the forced refresh can then fail at the token endpoint with invalid_scope; this path discards the original response and returns only the refresh error. Callers such as wrapYouTubeForceSSLError look for the original insufficientPermissions / insufficient authentication scopes text to show the gog auth add ... --force-consent hint, so under-scoped grants now lose that actionable guidance. Preserve and return the original 403 when RefreshAuth fails, or translate that failure into the same reauth error.

Useful? React with 👍 / 👎.

@steipete

steipete commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@steipete

steipete commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the fresh ClawSweeper P2 on exact head ac67db3a4dc4124d7659f96fc1152d0ca421bb32:

  • internal/googleapi/transport.go:153-161 now attempts the forced remint before closing the inspected response; if remint fails, it returns the intact original Google 403 so existing wrappers retain their re-auth guidance.
  • internal/googleapi/transport_more_test.go:306 proves exact 403 status/body preservation, one refresh attempt, one transport call, and no replay on refresh failure.
  • Focused regressions passed 20 consecutive runs plus race coverage; full make ci is green.
  • Final branch autoreview has no accepted/actionable findings (0.82).
  • Exact binary v0.31.1-18-gac67db3a, SHA-256 c03f2e3cff64f75bf661f07c087a3df316667b149b7e5b1f37abb6125016e1a4, passed authenticated read-only Contacts and Calendar calls.
  • Exact-head hosted push/PR CI, Docker, and Socket checks are all green: push CI, PR CI, Docker.

The only remaining gate is the explicit item-specific waiver for the intermittent live-provider first-response 403, which cannot be safely induced.

@steipete

steipete commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jul 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Intermittent 403 'insufficient scopes' that succeeds on retry with same token (per-call scope mint?)

1 participant