fix(auth): bind OAuth callback to loopback + harden keychain storage#6
Conversation
…rage Addresses three security findings against the OAuth login path and the keychain-backed token store. OAuth callback server (was: bound to all interfaces during login) Bumps SwiftWebServer to 0.2.0 and passes host: "localhost" to listen() so the callback socket is constrained to dual-stack loopback (127.0.0.1 + ::1) per RFC 8252 §7.3. Without this, the server was reachable from any host on the same network while a login was in flight, giving a LAN peer a reliable login-denial primitive (and a stretched code-injection race) against any consumer app. Defense in depth: the callback handler now compares the incoming state query parameter against the value generated for this flow and returns HTTP 400 on mismatch *without* burning the single-shot resume — a forged callback can no longer kill the legitimate browser one. Updated isResolvedPortBindFailure to also match IPv6 bind failures so the existing port-race retry loop still triggers when ::1 loses the race after the INADDR_ANY reservation closed. KeychainTokenStorage init (was: silent fallback to a global service name) KeychainTokenStorage.init now throws when both servicePrefix and Bundle.main.bundleIdentifier are nil, so a CLI/test host can't end up with a globally-shared "com.codingplan.auth" keychain service name and inadvertently share credentials between unrelated tools sharing a process bundle. AuthService.init no longer has a default storage. Callers must construct storage explicitly so the failure surface is at construction time, not first save/load. Tests - All 49 existing tests pass against SwiftWebServer 0.2.0. - Bind-failure regression test reserves the conflicting port via INADDR_LOOPBACK rather than INADDR_ANY (with SO_REUSEADDR a more- specific loopback bind can succeed against an INADDR_ANY reservation at the same port — the test would otherwise no longer observe the conflict it asserts on).
|
@claude review this PR |
|
Claude finished @atom2ueki's task in 1m 59s —— View job PR Review: fix(auth): bind OAuth callback to loopback + harden keychain storage
Summary: The three security fixes are sound and the implementation is correct. A few items worth addressing before merging — one medium gap (no tests for the new state validation), one stale comment, and a minor docs nit. Nothing that blocks merge if the manual smoke test is green, but the test gap in particular is worth closing for a security-critical path. FindingsMedium — State validation logic has no test coverage
Without these, a future refactor of the Minor — Stale version in
|
| Loopback bind (RFC 8252 §7.3) | ✅ Correct |
| State CSRF guard | ✅ Logic correct, |
| Retry loop dual-stack fix | ✅ Correct |
| Test port reservation fix | ✅ Correct |
KeychainTokenStorage fail-fast |
✅ Correct |
AuthService storage default removed |
✅ Correct |
| Manual smoke test | ⬜ Not yet (noted in PR) |
There was a problem hiding this comment.
Pull request overview
This PR hardens the OAuth local callback server and keychain-backed token storage to address security findings in the SDK’s login and credential persistence paths.
Changes:
- Bind the local OAuth callback server to loopback only (
host: "localhost") and addstatevalidation to reject forged/stale callbacks without consuming the single-shot resume. - Make
KeychainTokenStoragefail fast by throwing when neither an explicitservicePrefixnor a bundle identifier is available; require explicit storage construction inAuthService. - Update bind-failure detection for both IPv4/IPv6 and adjust the port-conflict test to reserve loopback rather than
INADDR_ANY.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/CodingPlanAuthTests/Infrastructure/LocalCallbackServerTests.swift | Ensures the bind-conflict test reserves loopback to match the server’s new loopback-only bind behavior. |
| Sources/CodingPlanAuth/Infrastructure/Storage/KeychainTokenStorage.swift | Makes the keychain storage initializer throwing to prevent silent global-service fallback in bundle-id-less hosts. |
| Sources/CodingPlanAuth/Infrastructure/Server/LocalCallbackServer.swift | Binds callback server to loopback and adds expected-state checking; expands bind-failure detection to IPv6. |
| Sources/CodingPlanAuth/Infrastructure/OAuth/OAuth2PKCEFlow.swift | Plumbs OAuth state into the callback server for defense-in-depth validation. |
| Sources/CodingPlanAuth/Documentation.docc/CodingPlanAuth.md | Updates DocC sample to compile with the new throwing KeychainTokenStorage initializer. |
| Sources/CodingPlanAuth/Application/AuthService.swift | Removes default storage parameter and updates docs to reflect explicit, throwing storage construction. |
| Package.swift | Bumps SwiftWebServer dependency to a version that supports host-bound listening. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add 4 unit tests for the new expectedState defense-in-depth check on
LocalCallbackServer (Claude review medium / Copilot 4):
- matching state completes the flow normally,
- mismatched state returns 400 and the legitimate browser callback
can still win the resume,
- missing state with expectedState set is rejected,
- no expectedState configured = back-compat (any state accepted).
Tests drive the actual local server with URLSession so the route
handler logic is exercised end to end.
- Update WebServerBox comment to reference SwiftWebServer 0.2.0
(Claude review minor). The @unchecked Sendable workaround is still
needed against 0.2.0; a follow-up will remove it once SwiftWebServer
0.3.0 ships with @mainactor.
- Replace try! KeychainTokenStorage() in the DocC overview with a
throwing init {} pattern so the snippet doesn't model a
crash-on-misconfiguration that copy-paste readers might inherit
(Claude minor / Copilot 3).
- Wrap the doc-comment snippets in AuthService and KeychainTokenStorage
in do/catch so the new throwing init is shown with explicit error
handling rather than a bare try in an implied throwing context
(Copilot 1, 2).
|
Re: @claude review — fix-up pushed in 02327cc.
Test count: 49 → 53. Will follow up with the |
|
Claude finished @atom2ueki's task in 1m 52s —— View job Re-review: fix-up commit 02327cc
All four findings from the previous review are correctly addressed. Details below. Medium —
|
| Test | What it proves |
|---|---|
callbackWithMatchingStateCompletesFlow |
Happy path: correct state + code → 200, start() returns the right params |
callbackWithMismatchedStateRejectsAndKeepsResumeAlive |
The critical security test. Forged callback → 400, genuine callback immediately after → 200, params.code == "genuine" (not "stolen") — directly proves the single-shot resume is preserved |
callbackWithMissingStateRejectsWhenExpectedStateSet |
No state query param → 400; server correctly stopped to avoid a hang |
callbackWithoutExpectedStateAcceptsAnyState |
Back-compat: unconfigured expectedState → any state accepted |
Logic and cleanup in each test is correct. The callbackGET helper targets 127.0.0.1 directly, which is correct since the server is bound to localhost (IPv4 loopback).
One cosmetic note: the callbackGET comment says "Don't follow redirects" but no redirect-prevention delegate is configured on URLSession.shared. This is harmless because the tested responses are 200/400 (not 3xx), but the comment could mislead a future reader. Worth a one-line fix at some point, not a blocker.
Minor — Stale WebServerBox comment ✅ Resolved
LocalCallbackServer.swift:19 now reads SwiftWebServer 0.2.0 still does not declare itself Sendable. The @unchecked Sendable justification is accurate for 0.2.0 and the follow-up plan (remove wrapper once 0.3.0 ships @MainActor) is documented in the commit message.
Minor — try! in DocC overview ✅ Resolved
CodingPlanAuth.md:28-40: the stored-property try! pattern is replaced with an init() throws that propagates the error. The inline comment explaining why the throw is safe for normal app targets (but still surfaces for CLI/test hosts) is a nice touch for copy-paste readers.
Minor (Copilot) — do/catch in doc-comment snippets ✅ Resolved
Both AuthService.swift and KeychainTokenStorage.swift doc snippets now wrap the throwing call in do { ... } catch { ... }. Syntactically valid Swift and models the correct error-handling pattern.
Verdict
All review items resolved. The PR is clean from a security-correctness and test-coverage perspective. The only remaining item before tagging a release is the manual smoke test called out in the PR checklist.
| Branch: security/fix-loopback-bind
SwiftWebServer 0.3.0 ships @mainactor isolation on the SwiftWebServer class, which makes it Sendable on its own — the WebServerBox @unchecked Sendable wrapper this SDK was carrying since 0.1.0 is no longer needed. Remove it; store SwiftWebServer directly on LocalCallbackServer. The constructor and route registration are now @mainactor, so makeServer() is annotated @mainactor and called via the LocalCallback actor's await hop. The immutable configuration fields the closure reads (responseHTML, redirectBaseURL, expectedState) are now `nonisolated private let` so the @mainactor helper can capture them without further hops. Mark the registered route handler closure @sendable. Without this, Swift 6 strict concurrency infers the closure as @mainactor (since server.get is @mainactor) and the runtime traps with "BUG IN CLIENT OF libdispatch" when SwiftWebServer's Connection invokes the handler from its background queue. @sendable forces non-isolation, matching how Connection actually calls it. 53/53 tests pass.
|
Follow-up: SwiftWebServer 0.3.0 just shipped (https://github.com/atom2ueki/SwiftWebServer/releases/tag/0.3.0) with Pushed
53/53 tests pass. |
|
@claude review this PR |
|
@codex review |
|
Claude finished @atom2ueki's task in 2m 6s —— View job PR Review: fix(auth): bind OAuth callback to loopback + harden keychain storage
All prior findings have been resolved. This review covers the final commit 9bc5530: @mainactor / @sendable / WebServerBox removal ✅
Remaining cosmetic item (not a blocker)
Full checklist
Verdict: All review items resolved across the three-commit series. The PR is clean — security-correct, test-covered, and Swift 6 strict-concurrency-clean. Ready to merge once the manual smoke test is green. |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Claude review noted the comment is inaccurate — `URLSession.shared` has no redirect-prevention delegate configured, so it would follow a 3xx if the route ever returned one. The tested responses are 200/400 only, so the comment was never load-bearing; just removing it rather than configuring a redirect-blocking session, which would be ceremony with no behavior change.
|
Cosmetic fix pushed in 5e73c10 — dropped the misleading 'Don't follow redirects' comment in the test helper. PR is ready from my side. Awaiting your manual smoke test of a real OAuth login on iOS/macOS before merging — that's the one thing I can't validate from here. Once you've confirmed the live flow still works, give the green light and I'll merge + tag the SDK release. |
- README Acknowledgements: list SwiftWebServer (built on) and the four projects that inspired this work — openai/codex, sugarforever/rn-ai-kit, Vercel AI SDK, badlogic/pi-mono. - README Quick Start: the example previously showed `AuthService(storage: KeychainTokenStorage())`, which no longer compiles after #6 made `KeychainTokenStorage.init` throwing and removed `AuthService.init`'s default storage. Move construction into a throwing `init() throws` on the SignIn class with an explanatory comment matching the DocC sample. - llms.txt: tighten the KeychainTokenStorage description to mention the throwing init and why it traps misconfigured hosts.
#7) - README Acknowledgements: list SwiftWebServer (built on) and the four projects that inspired this work — openai/codex, sugarforever/rn-ai-kit, Vercel AI SDK, badlogic/pi-mono. - README Quick Start: the example previously showed `AuthService(storage: KeychainTokenStorage())`, which no longer compiles after #6 made `KeychainTokenStorage.init` throwing and removed `AuthService.init`'s default storage. Move construction into a throwing `init() throws` on the SignIn class with an explanatory comment matching the DocC sample. - llms.txt: tighten the KeychainTokenStorage description to mention the throwing init and why it traps misconfigured hosts.
Summary
Fixes three security findings against the OAuth login path and the keychain-backed token store. All three came out of the security review on the SDK as it ships to third-party iOS/macOS app developers — anything weak here propagates into every consumer app.
1. OAuth callback server bound to all interfaces (High)
The local callback server inherited an
INADDR_ANY/in6addr_anybind fromSwiftWebServer 0.1.0, so during a login flow the server was reachable from any host on the same network. On a hostile LAN that's a reliable login-denial primitive (forge a callback faster than the browser, the single-shot server tears down before the legit callback lands) and a stretched code-injection race.Fix: Bumped the dependency to
SwiftWebServer 0.2.0(released today: https://github.com/atom2ueki/SwiftWebServer/releases/tag/0.2.0), which adds ahost:parameter tolisten(). Now passinghost: "localhost"inLocalCallbackServer.startListening()so the bind is constrained to dual-stack loopback (127.0.0.1+::1) per RFC 8252 §7.3. The browser can still reach the callback whether it resolveslocalhostto v4 or v6 first.Defense in depth: the route handler now compares the incoming
statequery parameter against the value generated for the in-flight handshake. Mismatched callbacks return HTTP 400 without burning the single-shot resume — a forged or stale callback can no longer kill the legitimate one. The legit browser callback (which carries the genuine state) still wins.2. KeychainTokenStorage silent fallback (Low)
KeychainTokenStorage.initsilently fell back to a hardcoded global service name"com.codingplan.auth"when bothservicePrefixandBundle.main.bundleIdentifierwere nil. In CLI tools, test harnesses, and app-extension hosts whereBundle.mainis the host shell, two unrelated tools could end up sharing each other's credentials.Fix:
KeychainTokenStorage.initnow throwsAuthError.storageErrorwhen neither a prefix nor a bundle id is available. The failure surface moves from "first save/load returns the wrong creds" to "construction fails fast."AuthService.initlost its default storage so the failure is visible at the call site rather than buried in a default expression.3. Adjacent improvements
isResolvedPortBindFailurenow matches bothIPv4andIPv6bind-failure messages so the existing port-race retry loop still triggers when the dual-stack listen loses the race on either family after theINADDR_ANYreservation closed.LocalCallbackServerTests.reserveLocalPortnow binds the conflicting reservation toINADDR_LOOPBACKinstead ofINADDR_ANY. WithSO_REUSEADDR, anINADDR_ANYreservation does not prevent a more-specific loopback bind at the same port, so the test would otherwise stop observing the conflict it asserts on.Breaking changes
This is a 0.x SDK; both breaks are intentional security posture improvements.
KeychainTokenStorage.init(servicePrefix:accessGroup:)is nowthrows. Existing callers addtry. Apps with aBundle.main.bundleIdentifier(the normal case) keep working unchanged once they add the keyword.AuthService.init(storage:)no longer has a default value. Callers must pass storage explicitly. The DocC example was already showing explicit construction, so this matches the documented usage.Test plan
swift buildclean.swift test— 49 tests, 0 failures, against the publishedSwiftWebServer 0.2.0.Notes
This is one clean commit on top of the latest
main(1c6a72f). NoPackage.resolvedchange is committed (gitignored in this repo). Ready to merge.