fix(cli): implement PKCE for secretless OAuth authorization-code login#3460
Conversation
Generated auth login sent a bare authorization_code grant when no client secret was configured — no client authentication and no code_verifier — which token endpoints reject. Every printed CLI with authorization-code OAuth inherited the broken CLIENT_ID-only path. When no secret is set, the emitted flow now generates a per-attempt RFC 7636 verifier, sends an S256 code_challenge on the authorize request, and exchanges with code_verifier in place of client_secret. Secret-based flows are unchanged. The loopback redirect moves from localhost to 127.0.0.1 per RFC 8252 §7.3, and the token exchange gets its own 30s client instead of the shared default. Adds a generated-output test (compiled binary, verify-env URL assertions both ways, injected -race runtime tests incl. the RFC 7636 appendix B vector) and a golden case locking the authorization-code + PKCE shape, which previously had no golden coverage. Closes mvanhorn#3457. Refs mvanhorn/printing-press-library#1287. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Merge Protections🟢 All 2 merge protections satisfied — ready to merge. Show 2 satisfied protections🟢 require-ready-label-and-ci
🟢 🚦 Auto-queueWhen all merge protections are satisfied and these conditions match, this pull request will be queued automatically.
|
Greptile SummaryThis PR fixes the secretless OAuth2 authorization-code login path in the CLI generator template (
Confidence Score: 5/5Safe to merge. The change fixes a definitively broken code path — secretless authorization-code login was always rejected by spec-compliant providers — and the fix is narrowly scoped to the secretless branch, leaving confidential-client behavior untouched. The PKCE implementation is technically correct: 32-byte CSPRNG verifier, base64url-unpadded encoding (43 chars, within RFC 7636's 43–128 range), SHA-256 challenge derived from the ASCII verifier, S256 method declared. The localhost → 127.0.0.1 redirect change is low-risk and RFC 8252-prescribed. The token exchange upgrade to http.NewRequestWithContext(cmd.Context(), …) with a dedicated http.Client is strictly an improvement. Test coverage is thorough: RFC 7636 appendix B vector, verifier shape/uniqueness, both secretless and with-secret behavioral paths under the verify env, and a new golden case locking both auth.go and client.go contracts for the authorization-code template branch. No pre-existing goldens were modified. No files require special attention. The template (auth.go.tmpl) is the only functional change and its generated output is fully locked by the new golden case and the behavioral test. Important Files Changed
|
… test assertions Token exchange now uses http.NewRequestWithContext(cmd.Context(), ...) so Ctrl+C cancels an in-flight exchange, matching the refreshAccessToken idiom in client.go.tmpl. Split the whitespace-sensitive combined assertion into independent Contains checks; mutual exclusivity stays covered by the verify-env behavioral checks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@tmchow this should be ready to merge. |
Merge Queue Status
This pull request spent 6 minutes 44 seconds in the queue, including 6 minutes 29 seconds running CI. Required conditions to merge
|
Merging origin/main brought in the new generate-golden-api-oauth2-authcode case (added by the PKCE work, mvanhorn#3460) whose expected client.go predates this branch's header-driven rate limiter. Regenerated it so the golden reflects the merged engine (PKCE auth-code client + ObserveHeaders/NewAdaptiveLimiterAuto). Only this file changed; the rest of the golden regen was Windows CRLF/checksum noise, reverted. LF blob verified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…out, membership skip, composite path-param fix (#3429) * WIP(R6 port): header-driven+auto rate limiter + transport reuse into templates Ports the build-tree engine work into the generator templates (feat/plane-rate-limit-fanout, off origin/main): - cliutil_ratelimit.go.tmpl: full header-driven limiter (ObserveHeaders, ParseRateLimitHeaders, budgetRate, headerDriven, brakeSafetySeconds), NewAdaptiveLimiterAuto/newAdaptiveLimiter split, OnSuccess maxRate clamp, OnRateLimit budgetRate decay. - client.go.tmpl: maxIdleConnsPerHost=32 + Clone(DefaultTransport) in the plain newHTTPClient branch (Task 1); RateLimitAuto/defaultAutoStartRate/newRateLimiter; New + newTierLimiters route through newRateLimiter; ObserveHeaders wired into do() response handling. WIP CHECKPOINT — goldens NOT regenerated, remaining R6 pieces (root.go.tmpl default_rate_limit, sync.go.tmpl parallel fan-out + skip-non-member, defect B cherry-pick) NOT yet ported. Does not pass generate --validate yet. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(spec): add default_rate_limit capability (x-pp-default-rate-limit) APISpec.DefaultRateLimit ("auto" or a number) drives the generated CLI's --rate-limit default: "auto" wires the header-driven adaptive limiter (client.RateLimitAuto) and relabels --help; a number pins a fixed ceiling; empty keeps the legacy provenance rule (sniffed=2, else 0). Parsed from the yaml field or the OpenAPI x-pp-default-rate-limit extension; validated in APISpec.Validate. Parser + generator-emission tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(sync): parallel per-parent fan-out + membership-aware skip + composite path-param fix - Parallel dependent fan-out: extract syncOneParent + parentReport, run a bounded worker pool over parents (concurrency threaded from --concurrency), lockedWriter serializes concurrent event writes, drain barrier before counter reads, dry-run drain + ctx cancel. Removes the 100ms per-parent sleep (pacing is the rate limiter's job). Thread concurrency through syncDependentResources/Resource. - Membership-aware skip (generic, spec-driven via x-pp-membership-field, mirrors the tenant-scope plumbing): APISpec/profiler MembershipField + MembershipScopedParents, Store.NonMemberParents (json_valid-guarded), membershipScopedParents map, skip filter + reportSkippedParents English summary + parent_skipped events. All gated on {{if .MembershipScopedParents}} so non-annotated CLIs emit nothing. - Defect B: strip NUL-composite parent storage id via store.BareResourceID before substituting dependent path params (nginx 400 on %00). (folds bed9959f) - mcp defaultMCPRateLimit -> client.RateLimitAuto (header-driven pacing). Tests: TestMembershipScopedFanout (generate+compile+run NonMemberParents behavior), TestDependentPathParamStripsCompositeStorageID; existing dependent-fanout test green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(golden): regenerate goldens for R6 engine changes 16 Go goldens (sync.go/client.go/mcp tools.go/cliutil ratelimit.go/jobs.go/ things_list.go across fixtures) reflect the rate-limit auto default, header-driven limiter, transport reuse, parallel dependent fan-out, and composite path-param fix. 8 .printing-press.json spec_checksum updates are the real drift from the new Endpoint.MembershipField / APISpec.DefaultRateLimit fields (same class as R4's TenantScopeColumn drift). LF-normalized; scripts/golden.sh verify green (the 2 dogfood README/SKILL blank-line diffs are the known Windows-only artifact, left at origin/main so Linux CI stays green). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(sync): generated-run -race test for parallel per-parent fan-out pool Generates a CLI with a typed parent + paginated dependent so the template emits syncDependentResource + the bounded worker pool, injects an in-package concurrency test into internal/cli, and runs it under -race. Asserts each parent is fetched exactly once (no double-dial / dropped parent), counts aggregate across workers past the barrier, and the dry-run sentinel short-circuits under the pool. Mirrors the build-tree internal/cli/sync_concurrency_test.go so a race in the ported pool fails on the generated artifact, not just the hand-maintained tree. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(ci): repair golden drift, emitted-test arity, and limiter test contracts Linux CI surfaced failures the Windows-local run masked: - go-lint (modernize): drop unneeded `tc := tc` loop copy. - full-golden: revert Windows-flavored noise to origin/main — 8 .printing-press.json (spec_checksum was hashed over CRLF-checked-out fixtures) plus jobs.go / things_list.go (toolchain gofmt/goimports skew on surfaces this PR does not touch). - syncDependentResource arity: the generator's emitted in-package sync tests (batch4_sync_test / dependent_parent_fk_test / sync_html_extract_test / dependent_chained_params_test) now pass the new trailing `concurrency` arg. - emitted limiter test: RampsUpAfterSuccesses uses NewAdaptiveLimiterAuto (the no-ceiling contract) since NewAdaptiveLimiter now pins a hard ceiling. - ratelimit_toctou_test: assert the renamed `startRate` constructor param. - tier_routing_test: assert per-tier limiters route through newRateLimiter (auto tiers must not collapse to a nil/disabled limiter). - mcp_tools: revert defaultMCPRateLimit to a fixed 2, preserving upstream's deliberate MCP-politeness guard (auto would blind-ramp on header-less servers); reverts the 6 tools.go goldens accordingly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(mcp): sanitize nested-variadic positional into a valid tool schema key cobratree derived MCP positional property names from cmd.Use. A nested variadic token like `[<slug>...]` (e.g. `workspaces add "add <slug> [<slug>...]"`, `init`) went through `strings.Trim(raw,"<>[]")` + `TrimSuffix("...")`, which left the inner `>` intact (Trim only cuts ends; the `...` shields it) → schema key `slug>`. That violates Anthropic's `^[a-zA-Z0-9_.-]{1,64}$`, so once such a tool is in the `tools` array EVERY request 400s and the Claude session is unrecoverable. Fix in `positionalArgsForCommand`: strip decorations with a NewReplacer (all of `<>[]`) then TrimSuffix("..."), and dedup positionals by inputName so `<slug> [<slug>...]` collapses to one `slug` slot. New emitted test `TestPositionalVariadicNestedAngleBracketsSanitizesKey`. Regenerated the one affected golden (generate-golden-api cobratree/typemap.go). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(golden): add sanitized-variadic MCP test to generate-golden-api golden Follow-up to 0410335: the emitted cobratree/shellout_test.go golden also gains TestPositionalVariadicNestedAngleBracketsSanitizesKey so full-golden matches the regenerated CLI. (Hand-applied to preserve owner/CRLF normalization, verified code-identical to a fresh generate.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(sync): address Greptile P2s on the parallel dependent-sync path - reportSkippedParents: json.Marshal the parent table name in the sync_skipped_parents rollup event instead of raw %s, matching parentSkippedJSON and guarding against a name that needs JSON escaping. - parallel per-parent progress: workers consume parents out of order, so the human-mode "(n/N parents)" counter derived from the original job index jumped around. Use a monotonic atomic tick (mirrors the existing progressCount idiom) so it increases 1..N regardless of worker order; drop the now-unused job index. Regenerated the two affected sync.go goldens (generate-golden-api, sync-walker). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore: empty commit to green the fork conversation check_run Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(ratelimit): don't double-brake budgetRate on header-driven 429 For header-driven limiters, ObserveHeaders already paces down from the same 429's X-Ratelimit-Remaining/Reset headers (its low-remaining brake), so also halving budgetRate in OnRateLimit double-brakes. budgetRate is a high-water mark that self-corrects to limit/window once the bucket refills, so the decay wouldn't stick anyway. Guard the decay on !headerDriven; on the header-less path budgetRate is always 0, so the guard just makes the invariant explicit. Addresses Greptile review comment on #3429. Adds ObserveHeaders test coverage (previously none) plus a regression test pinning that OnRateLimit leaves a header-driven budgetRate intact. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(golden): regen oauth2-authcode client.go after merging origin/main Merging origin/main brought in the new generate-golden-api-oauth2-authcode case (added by the PKCE work, #3460) whose expected client.go predates this branch's header-driven rate limiter. Regenerated it so the golden reflects the merged engine (PKCE auth-code client + ObserveHeaders/NewAdaptiveLimiterAuto). Only this file changed; the rest of the golden regen was Windows CRLF/checksum noise, reverted. LF blob verified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Anton Sidorov aka anticodeguy <a@anticodeguy.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Intent
Generated OAuth2
auth login(fromauth.go.tmpl) never implements PKCE, so the documented CLIENT_ID-only (no client secret) login path fails at token exchange in every printed CLI with authorization-code auth: the request carries neither client authentication nor acode_verifier, and spec-compliant token endpoints reject it.Issue: Closes #3457. Refs mvanhorn/printing-press-library#1287 (downstream report against spotify-pp-cli with full reproduction; that CLI needs a regen + republish after this lands).
Approach
RFC 7636 PKCE, gated on the secretless path so confidential-client behavior is untouched:
runOAuthLogingenerates a per-attemptcode_verifier(32 CSPRNG bytes, base64url unpadded), sendscode_challenge+code_challenge_method=S256on the authorize request, and sendscode_verifierinstead ofclient_secreton the token exchange. The secretless path is guaranteed-broken today, so the guard can only improve it; with a secret present the emitted flow is byte-identical in behavior.generatePKCEVerifier,pkceCodeChallengeS256) so generated CLIs can unit-test them.http://localhost:<port>/callbacktohttp://127.0.0.1:<port>/callbackper RFC 8252 §7.3. Some providers (e.g. Spotify, per the downstream issue) rejectlocalhostoutright; the published spotify CLI already carries this as a hand-patch that regen would otherwise revert.http.PostForm's shared default client, per the timeout-separation rule inskills/printing-press/references/oauth2-pkce-cli-checklist.md(that checklist explicitly anticipates promoting these rules into the generator).client.go.tmpl'srefreshAccessTokenalready sendsclient_idand omits the secret when empty, which is the correct PKCE public-client refresh shape (locked by the new golden case'sclient.goartifact).Scope
Primary area:
internal/generator/templates/auth.go.tmpl(machine change)Why this belongs in this repo: the symptom surfaced in a printed CLI (spotify-pp-cli), but the broken file is generator output (
DO NOT EDIT) emitted for every authorization-code OAuth CLI. A printed-CLI patch would be overwritten on regen and would not fix the other printed CLIs with the same flaw.Risk
authorization_codegrant (non-compliant but conceivable) now receive PKCE parameters. RFC 6749 requires servers to ignore unrecognized parameters, and that path was failing against every mainstream provider anyway.localhost→127.0.0.1redirect change means OAuth apps registered with alocalhostredirect URI need127.0.0.1registered instead (RFC 8252 guidance; most providers treat loopback IP literals leniently, and several — including the provider in the downstream issue — already rejectlocalhost).golden.sh update(the only golden diff is the new case).Output Contract
TestOAuthLoginUsesPKCEWhenNoClientSecret(internal/generator/auth_oauth_pkce_test.go): asserts emitted PKCE wiring, builds the generated binary, exercises the real flag→authorize-URL behavior underPRINTING_PRESS_VERIFY=1with and without a secret, and injects runtime tests into the generated module run with-race— including the RFC 7636 appendix B challenge vector and verifier shape/uniqueness properties.generate-golden-api-oauth2-authcode(fixture + expectedinternal/cli/auth.goandinternal/client/client.go): locks the authorization-code template-selection branch and the PKCE/loopback/refresh contract. Previously no golden case coveredauth.go.tmpl's authorization-code output (only client_credentials and device_code).Verification
go test ./...(all packages, pass), new test compiles and runs the generated CLIrunOAuthLogin, which is unconditional inauth.go.tmpl; template is selected only whenAuthorizationURL != ""; refresh call site inclient.go.tmplaudited (already gated correctly)Commands run:
go test ./...— passscripts/golden.sh verify— pass (31 cases, including the new one)scripts/golden.sh update— only diff is the newgenerate-golden-api-oauth2-authcodeexpected dir; all pre-existing goldens byte-identicalscripts/verify-generator-output.sh generate-golden-api-oauth2-authcode— generated module tidies and buildsAI / Automation Disclosure
🤖 Generated with Claude Code