Skip to main content

max / makenotwork

server: harden OAuth code redemption + challenge length (ultra-fuzz Run #1 Security LOWs) Two PKCE hardening items on the synckit OAuth flow: - The /token handler consumed (burned) the authorization code before validating client_id, redirect_uri, and the PKCE verifier, so a failed check left the code spent and forced the legitimate client to restart. Add peek_oauth_code (non-consuming read), validate against it, then atomically consume only after all checks pass. Concurrency stays race-safe via the existing used_at IS NULL guard on the consume. - /authorize accepted an empty or wrong-length code_challenge: the GET path had no length check at all (and its prompt=none branch issues a code directly), and POST only capped the upper bound. Enforce the exact S256 range (43-44 chars) on both, rejecting the empty string. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-06-19 21:10 UTC
Commit: 0268eb440d602a1de6128e0c617e432f7546d140
Parent: 8669659
2 files changed, +50 insertions, -5 deletions
M server/src/db/oauth.rs +25
@@ -160,6 +160,31 @@ pub async fn cleanup_expired_refresh_tokens(pool: &PgPool) -> Result<u64> {
160 160 Ok(result.rows_affected())
161 161 }
162 162
163 + /// Fetch a still-valid authorization code WITHOUT consuming it.
164 + ///
165 + /// Lets the token handler validate client_id / redirect_uri / PKCE against the
166 + /// code's stored values before burning it, so a failed validation leaves the
167 + /// code usable for the legitimate client's retry (ultra-fuzz Run #1 Security
168 + /// LOW: the code was previously marked used before any of those checks). The
169 + /// atomic `consume_oauth_code` below is still what actually claims the code, so
170 + /// concurrent redemptions remain race-safe — this is only a pre-flight read.
171 + #[tracing::instrument(skip_all)]
172 + pub async fn peek_oauth_code(pool: &PgPool, code: &str) -> Result<Option<DbOAuthCode>> {
173 + let row = sqlx::query_as::<_, DbOAuthCode>(
174 + r#"
175 + SELECT * FROM oauth_authorization_codes
176 + WHERE code = $1
177 + AND used_at IS NULL
178 + AND expires_at > NOW()
179 + "#,
180 + )
181 + .bind(code)
182 + .fetch_optional(pool)
183 + .await?;
184 +
185 + Ok(row)
186 + }
187 +
163 188 /// Atomically consume an authorization code: mark it used and return it in one step.
164 189 ///
165 190 /// Returns `Some(code)` if the code was valid and successfully consumed,
M server/src/routes/oauth.rs +25 -5
@@ -284,6 +284,15 @@ async fn authorize_get(
284 284 return Err(AppError::BadRequest("code_challenge_method must be 'S256'".to_string()));
285 285 }
286 286
287 + // An S256 challenge is base64url-nopad of a SHA-256: exactly 43 chars (44
288 + // if a stray `=` is included). Reject anything outside that range —
289 + // including the empty string from `?code_challenge=` — so the prompt=none
290 + // branch below never issues a code bound to an unsatisfiable challenge
291 + // (ultra-fuzz Run #1 Security LOW). The POST consent path enforces the same.
292 + if !(43..=44).contains(&code_challenge.len()) {
293 + return Err(AppError::BadRequest("code_challenge has invalid length".to_string()));
294 + }
295 +
287 296 // Look up app by client_id (= sync_apps.api_key)
288 297 let app = db::synckit::get_sync_app_by_api_key(&state.db, client_id)
289 298 .await?
@@ -363,9 +372,10 @@ async fn authorize_post(
363 372 return Err(AppError::BadRequest("state parameter too long (max 1024 bytes)".to_string()));
364 373 }
365 374 // S256 challenges are exactly 43 base64url chars (no padding). Allow 44
366 - // for clients that include the trailing `=`. Anything wildly larger is a
367 - // malformed challenge that would never verify.
368 - if form.code_challenge.len() > 44 {
375 + // for clients that include the trailing `=`. Reject anything outside that
376 + // range — including the empty string — as a malformed challenge that would
377 + // never verify (ultra-fuzz Run #1 Security LOW: empty was not rejected).
378 + if !(43..=44).contains(&form.code_challenge.len()) {
369 379 return Err(AppError::BadRequest("code_challenge has invalid length".to_string()));
370 380 }
371 381
@@ -629,8 +639,11 @@ async fn token_authorization_code(
629 639 .as_deref()
630 640 .ok_or_else(|| AppError::BadRequest("code_verifier is required".to_string()))?;
631 641
632 - // Atomically consume code (must exist, not expired, not used).
633 - let oauth_code = db::oauth::consume_oauth_code(&state.db, code)
642 + // Peek the code (does NOT consume it) so a failed client_id / redirect_uri
643 + // / PKCE check leaves it usable for the legitimate client's retry instead of
644 + // burning it (ultra-fuzz Run #1 Security LOW). The atomic consume below is
645 + // what actually claims it, so concurrent redemptions stay race-safe.
646 + let oauth_code = db::oauth::peek_oauth_code(&state.db, code)
634 647 .await?
635 648 .ok_or(AppError::BadRequest("Invalid or expired authorization code".to_string()))?;
636 649
@@ -662,6 +675,13 @@ async fn token_authorization_code(
662 675 return Err(AppError::BadRequest("PKCE verification failed".to_string()));
663 676 }
664 677
678 + // All checks passed — now atomically claim the code. A None here means a
679 + // concurrent request already redeemed it (or it expired in the gap); the
680 + // `used_at IS NULL` guard makes double-redemption impossible.
681 + let oauth_code = db::oauth::consume_oauth_code(&state.db, code)
682 + .await?
683 + .ok_or(AppError::BadRequest("Invalid or expired authorization code".to_string()))?;
684 +
665 685 // Re-check account liveness at redemption. A user suspended or deactivated
666 686 // between authorize and code->token must not receive a token; the refresh
667 687 // grant already applies this gate, but the code->token path skipped it,