Skip to content

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

Merged
bokelley merged 5 commits into
mainfrom
security-wave-11-desktop-oauth-binding
Aug 8, 2026
Merged

bokelley merged 5 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
@bokelley
bokelley force-pushed the security-wave-11-desktop-oauth-binding branch from 860cf86 to f839245 Compare August 8, 2026 14:05
@bokelley
bokelley enabled auto-merge (squash) August 8, 2026 14:06

@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 under hard approval gate, review not yet approved.

This PR adds .github/workflows/desktop-rust.yml, which matches the repo's ## Gated Paths glob .github/workflows/**. gated_paths is true and review_decision is REVIEW_REQUIRED (not APPROVED), so decision-table row 2 fires: Ladon cannot auto-approve until a human/CODEOWNERS approval is recorded, regardless of how clean the diff is.

The reviewer found no Critical/High/Medium findings. The desktop OAuth v2 surface reviewed well: validate_callback fails closed (exact scheme/host/path/fragment checks, duplicate/legacy-param rejection, constant-time state compare, timestamp integrity, issuer binding); native-auth.ts binds the grant and consumes once via DELETE...RETURNING with a correct RFC 7636 S256 derivation; state DB hashes opaque ids/codes and encrypts verifier + sealed session at rest (AES-256-GCM); reqwest client has connect/read timeouts and a bounded 64KB body reader; migration 538 correctly succeeds 537 and is IF NOT EXISTS idempotent. No dist/** artifact is touched, so immutability is clean.

This is consistent with the prior escalate — the block is purely the unsatisfied gated-path approval gate, not any code defect. Once a human/CODEOWNERS approval is recorded (review_decision: APPROVED), a re-run with the same clean findings falls through to approve.

Escalation reasons

  • Adds .github/workflows/desktop-rust.yml — matches gated path .github/workflows/** (hard approval gate); review_decision is REVIEW_REQUIRED, not APPROVED. Human/CODEOWNERS review required.

Why human review

  • Adds .github/workflows/desktop-rust.yml, which matches gated path glob .github/workflows/** (hard, non-overridable approval gate), and review_decision is REVIEW_REQUIRED — not APPROVED. Human/CODEOWNERS approval is required before this can merge.
  • 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.

@bokelley
bokelley disabled auto-merge August 8, 2026 19:47
@bokelley
bokelley merged commit 38f492d into main Aug 8, 2026
16 checks passed
@bokelley
bokelley deleted the security-wave-11-desktop-oauth-binding branch August 8, 2026 19:47
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