Skip to content

fix(security): bind desktop OAuth callbacks - #6114

Open
bokelley wants to merge 4 commits into
mainfrom
security-wave-11-desktop-oauth-binding
Open

fix(security): bind desktop OAuth callbacks#6114
bokelley wants to merge 4 commits into
mainfrom
security-wave-11-desktop-oauth-binding

Conversation

@bokelley

Copy link
Copy Markdown
Contributor

Summary

  • replace the desktop bearer-in-deep-link login with a state- and PKCE-bound native OAuth v2 flow
  • persist pending desktop state in the system keyring before opening the browser and reject legacy, mismatched, malformed, expired, or replayed callbacks
  • issue only short-lived one-time authorization grants through the private URI scheme; exchange them over HTTPS for the server-derived WorkOS session and identity
  • hash transient identifiers, encrypt pending verifiers and sealed sessions at rest, and consume pending/grant rows atomically in PostgreSQL
  • add strict request parsing, no-store responses, rate limits, bounded streaming response reads, single-instance deep-link forwarding, and dedicated Rust CI

Security properties

  • no bearer session or user PII is placed in callback URLs or auth-event logs
  • desktop state and PKCE verifier are durable before browser launch
  • WorkOS and desktop PKCE bindings are independent and one-time
  • client ID, redirect URI, state, PKCE challenge, issuer, expiry, and protocol version are exact-bound
  • invalid callbacks preserve the existing authenticated session
  • v1 bearer-in-URI callbacks are explicitly rejected

Validation

  • cargo fmt --check
  • cargo test --locked --lib (12 passed)
  • cargo clippy --locked --all-targets -- -D warnings
  • targeted native-auth unit suite (38 passed; PostgreSQL-only tests skipped locally)
  • real-PostgreSQL integration coverage for concurrent single-use consumption and expiry
  • full server TypeScript typecheck
  • migration validation
  • Semgrep: 0 findings across the new native-auth boundary
  • independent security, Rust/desktop, and server/testing reviews: CLEAN

Notes

  • No root package.json or package-lock.json changes.
  • Migration 530 follows migration 529 on current main.

@bokelley
bokelley marked this pull request as ready for review July 31, 2026 09:08

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ladon verdict: Escalate to human review

Escalate — gated-path change requires human/CODEOWNERS review.

This PR replaces the bearer-in-deep-link login for the Addie desktop native OAuth with a state+PKCE, server-brokered one-time-grant flow. The reviewer found no Critical/High/Medium findings: callback validation fails closed (issuer-bind at auth_flow.rs:212, constant-time state compare at :206), grants are single-use and verifier-bound via atomic DELETE..RETURNING PKCE check (native-auth.ts:197), external auth HTTP calls are bounded (connect_timeout 5s + timeout 15s, redirect Policy::none at auth.rs:85), cleanup timer is unref'd, and migration 530 follows 529 without collision. No protocol surface (static/schemas/source/**) touched, so no changeset needed.

Despite the clean diff, the PR adds .github/workflows/desktop-rust.yml, which matches the repo's ## Gated Paths glob (.github/workflows/**). This is a hard, deterministic approval gate. review_decision is REVIEW_REQUIRED (not APPROVED), so decision-table row 2 fires: outcome must be escalate until a human/CODEOWNERS approval satisfies the gate.

No blocking findings otherwise. Once a required reviewer approves, this PR should fall through to a normal approve.

Why human review

  • Gated path touched: .github/workflows/desktop-rust.yml matches .github/workflows/** (hard approval gate) and review_decision is REVIEW_REQUIRED, not APPROVED — human/CODEOWNERS review required.
  • This PR touches a path under a hard, non-overridable approval gate (.github/workflows/desktop-rust.yml (added) matches .github/workflows/**) and the current GitHub review decision is 'REVIEW_REQUIRED', not APPROVED. This is a hard gate enforced in code — Ladon cannot auto-approve until a human/CODEOWNERS approval is recorded, regardless of how clean the diff is.

@aao-secretariat aao-secretariat Bot added the ladon/needs-human-review Ladon has escalated this PR for human review. label Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ladon/needs-human-review Ladon has escalated this PR for human review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant