fix(auth): stop routing login through a Volcano Web page that doesn't exist for it#75
Conversation
tkkhq
changed the title
7104e49 to
10c5ca7
Compare
10c5ca7 to
a02f033
Compare
There was a problem hiding this comment.
Pull request overview
Restores direct device-flow login while retaining Volcano Web for signup.
Changes:
- Opens backend-provided verification URLs directly for login.
- Uses an API-derived Web origin and relative
/devicehandoff for signup. - Removes obsolete login URL helpers and updates tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
internal/config/config.go |
Adds Web URL derivation for resolved API URLs. |
internal/auth/auth.go |
Revises browser login and signup routing. |
internal/auth/auth_test.go |
Tests direct verification URL handling. |
internal/cmd/auth/auth_test.go |
Updates signup routing expectations. |
internal/api/auth.go |
Simplifies signup URL construction. |
internal/api/client_test.go |
Removes obsolete login helper tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // LoginWithBrowser runs the OAuth device flow and opens the browser at the | ||
| // verification URI the device-authorization response returned, unmodified. | ||
| // That page (for Volcano's first-party CLI client, the API's own managed | ||
| // hosted-auth page, action=device) is self-contained: it signs the user in | ||
| // and asks them to approve the code itself, so login needs no Volcano Web | ||
| // routing of its own (see docs/api-reference/cli-authentication.md and | ||
| // docs/authentication/managed-hosted-pages.md in volcano-hosting). |
There was a problem hiding this comment.
Fixed in 400da3a — CONTRIBUTING.md now says VOLCANO_WEB_URL is signup-page-only and explicitly notes login opens the verification URL directly instead, and WebURLOverride's doc comment no longer references the removed device-verification-URI derivation, clarifying it's only used by Signup.
tkkhq
left a comment
tkkhq
left a comment
There was a problem hiding this comment.
Code Review — PR #75: fix(auth): stop routing login through a Volcano Web page that doesn't exist for it
Verification: No user-level verificationBaselines are configured (pr_review_verify list returned enabled: false, zero baselines), so the extension-owned build/test verification lifecycle was not run; this run does not substitute a bash-driven checkout/build to avoid executing unsandboxed PR code outside the intended lifecycle. Correctness was instead confirmed by source-grounded review: repo-wide grep confirms zero remaining callers of the removed symbols (WebLoginURL, VerificationWebTarget, webPageURL, resolveWebTarget), and internal/config/config_test.go's existing WebURL() test suite (untouched by this diff) is byte-for-byte consistent with the new WebURL()/WebURLForAPIURL() split. Five parallel subagent passes (overview + 4 heavy P0-P2 lenses: state/lifecycle correctness, contracts/integration correctness, security, performance/resources) all completed; the four heavy passes returned no substantiated findings.
Overview
This PR reverts the core behavior change from #72: LoginWithBrowser (internal/auth/auth.go) stops routing the browser through a Volcano Web /login page and instead opens the device-authorization response's verification_uri_complete (falling back to verification_uri) directly, since that page is now the API's own self-contained managed-hosted-auth page for Volcano's first-party CLI client and no longer needs a Volcano Web hop. Signup keeps its Volcano Web /signup step (account creation genuinely lives there) but now derives its web origin from cfg.WebURLForAPIURL(apiURL) — a new exported method extracted from config.Config.WebURL() — instead of the device-authorization response's verification URI, and passes a synthesized same-origin /device?user_code=... next path via the new deviceApprovalPath helper rather than a value derived from that (different-origin, post-#417) response.
As cleanup, the PR removes now-dead code: api.WebLoginURL, the shared webPageURL helper (inlined back into WebSignupURL), api.VerificationWebTarget, and auth.resolveWebTarget, along with their now-obsolete tests. Doc comments throughout are rewritten to explain the current architecture and link to the backend docs that justify it.
The change is well-scoped and internally consistent: config.WebURL() is preserved as a thin wrapper over the new WebURLForAPIURL(c.APIURL()), so its existing behavior and test suite (untouched by this diff) remain valid unchanged, and Signup's fail-fast VOLCANO_WEB_URL validation and URL-building call sites were updated to match.
Strengths
- Exceptionally well-documented rationale: the rewritten doc comments on
LoginWithBrowser,Signup, andWebURLForAPIURLexplain why each design choice is correct and cite the specific backend docs/PRs that justify it, making the change easy to audit. - Clean, behavior-preserving refactor of
config.WebURL()intoWebURL()+WebURLForAPIURL(apiURL)— verified against the existing (untouched)config_test.gosuite, which still validates the exact same precedence rules. - Dead code removal is thorough and correctly scoped:
WebLoginURL,webPageURL,VerificationWebTarget, andresolveWebTargetare removed along with their tests, and repo-wide grep confirms no dangling callers remain. - Test updates track the behavior changes precisely — e.g.
TestLoginWithBrowserFallsBackToVerificationURIWhenCompleteMissingexercises the newVerificationUriComplete→VerificationUrifallback, and the signup tests were retargeted from a device-response-derived origin to an API-derived one with clear comments explaining why.
Findings — 0 total (0 inline, 0 summary-only)
No issues found.
Correctness / Security / Performance
- Correctness: All four heavy lenses (state/lifecycle, contracts/integration, security, performance/resources) independently traced the changed call paths (LoginWithBrowser's URI fallback, Signup's WebURLForAPIURL/deviceApprovalPath wiring, the WebURL split, and the removed symbols) and returned no substantiated defects; the one edge case discussed (both verification URI fields empty) requires a non-RFC-8628-compliant server and degrades no worse than the pre-PR path it replaces.
- Security: Opening the device-authorization response's verification URI directly in the user's browser is the standard RFC 8628 device-flow contract; it's only ever sourced from the trusted, TLS-terminated, first-party-client API response, not sent server-side (no SSRF), and Signup's synthesized
nextpath is properly percent-encoded and same-origin (an improvement over deriving it from the verification response). - Performance: Pure string/URL-building and doc-comment refactor; removes a query-map loop in favor of a single field check and introduces no new I/O, goroutines, or allocations at scale.
Verdict
Suggested verdict: approve
Four independent heavy-lens passes plus orchestrator source validation found no substantiated P0-P2 defects; the refactor is behavior-preserving where intended (WebURL split) and intentionally behavior-changing exactly as documented (login/signup routing), backed by updated tests and confirmed dead-code removal.
Confidence: 0.90
Tracking
LoginWithBrowserchange)Signupstill usescfg.WebURLForAPIURL)Summary
Kong/volcano-hosting) and web frontend (Kong/volcano-web) source and docs: for Volcano's first-party CLI client, the device-authorization response'sverification_uri/verification_uri_completealready point at the API's own self-contained managed-hosted-auth page (/projects/{id}/auth/hosted?action=device&user_code=...&anon_key=...), which signs the user in and asks them to approve the code itself — there is no Volcano Web/loginstep in this flow, and hasn't been sinceKong/volcano-hosting#417(2026-06-22)LoginWithBrowserno longer routes throughcfg.WebURLForAPIURL/WebLoginURL— it opensverification_uri_complete/verification_uridirectly again, exactly as it did before fix(auth): route browser login through Volcano Web's login page #72api.WebLoginURL(no longer called) and simplifiedWebSignupURLback to a single-purpose function now that it's the only caller of the old sharedwebPageURLhelperSignupkeepscfg.WebURLForAPIURL(apiURL)(from feat(config): derive VOLCANO_WEB_URL from VOLCANO_API_URL by convention #74) for its origin — Volcano Web's own/signuppage is still the right place to create an account — but no longer carries the device-authorization response's URI asnext:Kong/volcano-web's login/signup pages reject anynextthat isn't a same-origin relative path (isSafeInternalPath), and the verification URI is now a different-origin API page, so it could never have safely represented that hand-off —nextis a synthesized same-origin/device?user_code=...path insteadisSafeInternalPathrejects the absolute URI fix(auth): stop routing login through a Volcano Web page that doesn't exist for it #75 previously passed asnext, so Volcano Web would have silently dropped it and redirected to/dashboardafter a successful login/signup, regardless of the origin match this PR's earlier revision was trying to fixSignup's account-creation + device-approval hand-off may still have a real gap (Volcano Web's/devicepage's continued relevance, and whetheraction=signupon the hosted page can ever complete a device grant, are unconfirmed post-#417) — out of scope here, flagged separatelyVerification
go build ./...go vet ./...go test ./...go tool golangci-lint run ./...