Skip to content

fix(security): use PKCE (S256) on the OAuth authorization-code flow - #432

Draft
mptfire wants to merge 1 commit into
LNVPS:masterfrom
mptfire:audit/oauth-pkce
Draft

mptfire wants to merge 1 commit into
LNVPS:masterfrom
mptfire:audit/oauth-pkce

Conversation

@mptfire

@mptfire mptfire commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #431 (stacked on it — merge that first; I'll rebase after). Found by the same independent audit (2026-11-01).

What

The OAuth authorization-code flow now uses PKCE with S256 (RFC 7636):

  • every login generates a 32-byte base64url code_verifier (43 chars, RFC charset);
  • the authorization request carries code_challenge + code_challenge_method=S256;
  • the verifier rides the signed, short-lived state token — the flow stays stateless (no server-side session table), and a state minted before PKCE simply carries none, so the token request omits it;
  • the token exchange presents the verifier when the state carried one.

Why

Authorization-code interception (RFC 9700 §4.6): a code leaked through redirects, referrer headers, proxy logs or browser history is useless without the verifier, which never leaves the server and the provider's browser session. RFC 9700 recommends PKCE for all OAuth clients regardless of client type.

Standards-compliant providers (Google, Apple, generic OIDC) accept PKCE; GitHub supports it. A provider that rejected the extra authorization parameters would fail login closed rather than degrade security — docs/config.md says so.

Validation

  • 3 new unit tests: the RFC 7636 appendix B test vector for the S256 challenge; verifier shape (43 chars, base64url charset) across generations; verifier round-trip through the signed state.
  • Full oauth suite green (19 tests), lnvps_api_common green (638), build clean.
  • Not covered: a live end-to-end login against a real provider (needs registered OAuth apps; the token-request change is exercised only at the body-construction level).

Follow-up to the id_token hardening (LNVPS#431). Every authorization request now
carries a code_challenge and the token exchange presents the code_verifier
(RFC 7636, S256). The verifier rides the signed, short-lived state token,
so the flow stays stateless; states minted before PKCE simply carry no
verifier and the token request omits it.

Mitigates authorization-code interception (RFC 9700 section 4.6): a code
stolen from redirects, referrers or logs is useless without the verifier,
which never leaves the server and the provider's browser session.

docs/config.md documents the behavior; API_CHANGELOG.md records it.
Found by the 2026-11-01 security audit (LNVM-013 follow-up).

@v0l v0l 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.

This PR contains all of #431 (it was branched from it), so please rebase onto master once #431 lands and keep only the PKCE commit here. Reviewing as one change makes it hard to land either.

On the PKCE part itself:

  1. Facebook does not support PKCE for the confidential web flow the way Google/GitHub/Apple do, and providers that reject unknown params will now fail login closed (your changelog says so). Please confirm each built-in provider (google, github, facebook, apple) still logs in with the code_challenge params present, or make PKCE a per-flavour default with an oidc opt-out. A note on which ones you actually tested end-to-end would settle it.
  2. The verifier sits in the state query param, which is visible to the browser, the provider and any log of the callback URL. With a confidential client that also sends client_secret, PKCE still adds code-injection protection, but say so in the doc rather than calling the verifier "Secret". Alternatively store it server side keyed by the nonce.
  3. Doc comments: we keep comments out of new code (see docs/agents/code-style.md); the new /// blocks on pkce_verifier, pkce_challenge, VerifiedState and the tests can go.

@v0l

v0l commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

Correction to point 1: Facebook does support PKCE (https://developers.facebook.com/documentation/facebook-login/guides/advanced/oidc-token/). The ask stands as a smoke test: confirm each built-in provider still completes a login with the PKCE params. The rebase onto #431 is the main blocker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants