Skip to main content

max / makenotwork

server: hash OAuth authorization codes at rest (ultra-fuzz Run #1 --deep Phase 2) OAuth authorization codes were stored plaintext while every other persistent credential (refresh tokens, git PATs, sessions) is SHA-256 hashed — a DB read exposed live, redeemable codes for their 10-min TTL. Store and look up the code by its hash instead: create/peek/consume now take a `code_hash`, the route hashes the plaintext on both issuance and redemption, and the plaintext only ever travels in the redirect to the RP. Mirrors oauth_refresh_tokens.token_hash; no migration (the column already held a 64-char hex value). Document the deliberate no-constant-time-compare on git PAT resolve_active (hash-indexed lookup, the key is already a SHA-256) so the asymmetry reads as intentional. Gate: oauth/auth/passkeys/sso/password_reset workflows 42, lib units 56 green; clippy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-06-22 21:56 UTC
Commit: cd46703af774119a7f5b6237bbfca98298adcd32
Parent: 2219313
4 files changed, +42 insertions, -19 deletions
M server/src/db/git_access_tokens.rs +6
@@ -84,6 +84,12 @@ pub async fn revoke(pool: &PgPool, id: GitAccessTokenId, user_id: UserId) -> Res
84 84 /// Resolve a presented token's hash to its (user, push-scope), if the token
85 85 /// exists and has not expired. Stamps `last_used_at` on a hit. Returns `None`
86 86 /// for unknown or expired tokens — callers must treat that as unauthenticated.
87 + ///
88 + /// No constant-time compare here, deliberately: `token_hash` is already the
89 + /// SHA-256 of the secret, so the lookup is an indexed equality on a value an
90 + /// attacker cannot steer toward a stored secret via timing (a hash preimage is
91 + /// infeasible). Constant-time matters where the *secret itself* is byte-compared
92 + /// (e.g. CSRF/PKCE); a hash-indexed credential lookup is the correct asymmetry.
87 93 #[tracing::instrument(skip_all)]
88 94 pub async fn resolve_active(pool: &PgPool, token_hash: &str) -> Result<Option<ResolvedToken>> {
89 95 // UPDATE...RETURNING does the expiry filter and the last_used_at stamp in
M server/src/db/models/git.rs +3
@@ -53,6 +53,9 @@ pub struct SshKeyUserLookup {
53 53 #[allow(dead_code)] // Fields read via sqlx queries and in route handlers
54 54 pub struct DbOAuthCode {
55 55 pub id: OAuthCodeId,
56 + /// SHA-256 hex of the authorization code, never the plaintext. Stored and
57 + /// looked up by this hash (see `db::oauth`), so a DB read can't surface a
58 + /// live, redeemable code — same at-rest contract as refresh-token hashes.
56 59 pub code: String,
57 60 pub app_id: SyncAppId,
58 61 pub user_id: UserId,
M server/src/db/oauth.rs +23 -16
@@ -9,11 +9,16 @@ use super::{SyncAppId, UserId};
9 9 use crate::error::Result;
10 10
11 11 /// Store a new OAuth authorization code, carrying the granted scope.
12 + ///
13 + /// `code_hash` is the SHA-256 hex of the opaque code, never the plaintext —
14 + /// the `code` column holds the hash, looked up by hash in [`peek_oauth_code`] /
15 + /// [`consume_oauth_code`], so a DB read never exposes a live, redeemable code.
16 + /// Same at-rest contract as `oauth_refresh_tokens.token_hash`.
12 17 #[allow(clippy::too_many_arguments)]
13 18 #[tracing::instrument(skip_all)]
14 19 pub async fn create_oauth_code(
15 20 pool: &PgPool,
16 - code: &str,
21 + code_hash: &str,
17 22 app_id: SyncAppId,
18 23 user_id: UserId,
19 24 code_challenge: &str,
@@ -30,7 +35,7 @@ pub async fn create_oauth_code(
30 35 RETURNING *
31 36 "#,
32 37 )
33 - .bind(code)
38 + .bind(code_hash)
34 39 .bind(app_id)
35 40 .bind(user_id)
36 41 .bind(code_challenge)
@@ -162,14 +167,16 @@ pub async fn cleanup_expired_refresh_tokens(pool: &PgPool) -> Result<u64> {
162 167
163 168 /// Fetch a still-valid authorization code WITHOUT consuming it.
164 169 ///
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.
170 + /// `code_hash` is the SHA-256 hex of the presented code (the column stores the
171 + /// hash, not the plaintext). Lets the token handler validate client_id /
172 + /// redirect_uri / PKCE against the code's stored values before burning it, so a
173 + /// failed validation leaves the code usable for the legitimate client's retry
174 + /// (ultra-fuzz Run #1 Security LOW: the code was previously marked used before
175 + /// any of those checks). The atomic `consume_oauth_code` below is still what
176 + /// actually claims the code, so concurrent redemptions remain race-safe — this
177 + /// is only a pre-flight read.
171 178 #[tracing::instrument(skip_all)]
172 - pub async fn peek_oauth_code(pool: &PgPool, code: &str) -> Result<Option<DbOAuthCode>> {
179 + pub async fn peek_oauth_code(pool: &PgPool, code_hash: &str) -> Result<Option<DbOAuthCode>> {
173 180 let row = sqlx::query_as::<_, DbOAuthCode>(
174 181 r#"
175 182 SELECT * FROM oauth_authorization_codes
@@ -178,7 +185,7 @@ pub async fn peek_oauth_code(pool: &PgPool, code: &str) -> Result<Option<DbOAuth
178 185 AND expires_at > NOW()
179 186 "#,
180 187 )
181 - .bind(code)
188 + .bind(code_hash)
182 189 .fetch_optional(pool)
183 190 .await?;
184 191
@@ -187,12 +194,12 @@ pub async fn peek_oauth_code(pool: &PgPool, code: &str) -> Result<Option<DbOAuth
187 194
188 195 /// Atomically consume an authorization code: mark it used and return it in one step.
189 196 ///
190 - /// Returns `Some(code)` if the code was valid and successfully consumed,
191 - /// or `None` if the code was already used, expired, or does not exist.
192 - /// Because this is a single UPDATE with `used_at IS NULL` in the WHERE clause,
193 - /// concurrent requests for the same code will never both succeed.
197 + /// `code_hash` is the SHA-256 hex of the presented code. Returns `Some(code)` if
198 + /// it was valid and successfully consumed, or `None` if already used, expired, or
199 + /// nonexistent. Because this is a single UPDATE with `used_at IS NULL` in the
200 + /// WHERE clause, concurrent requests for the same code will never both succeed.
194 201 #[tracing::instrument(skip_all)]
195 - pub async fn consume_oauth_code(pool: &PgPool, code: &str) -> Result<Option<DbOAuthCode>> {
202 + pub async fn consume_oauth_code(pool: &PgPool, code_hash: &str) -> Result<Option<DbOAuthCode>> {
196 203 let row = sqlx::query_as::<_, DbOAuthCode>(
197 204 r#"
198 205 UPDATE oauth_authorization_codes
@@ -203,7 +210,7 @@ pub async fn consume_oauth_code(pool: &PgPool, code: &str) -> Result<Option<DbOA
203 210 RETURNING *
204 211 "#,
205 212 )
206 - .bind(code)
213 + .bind(code_hash)
207 214 .fetch_optional(pool)
208 215 .await?;
209 216
M server/src/routes/oauth.rs +10 -3
@@ -161,9 +161,12 @@ async fn issue_authorization_code(
161 161 let expires_at =
162 162 chrono::Utc::now() + chrono::Duration::seconds(constants::OAUTH_CODE_EXPIRY_SECS);
163 163
164 + // Store only the hash; the plaintext code goes to the RP in the redirect and
165 + // is never persisted (same at-rest contract as refresh tokens). A DB read
166 + // therefore can't surface a live, redeemable code.
164 167 db::oauth::create_oauth_code(
165 168 pool,
166 - &code,
169 + &hash_token(&code),
167 170 app_id,
168 171 user_id,
169 172 code_challenge,
@@ -639,11 +642,15 @@ async fn token_authorization_code(
639 642 .as_deref()
640 643 .ok_or_else(|| AppError::BadRequest("code_verifier is required".to_string()))?;
641 644
645 + // Codes are stored hashed; hash the presented plaintext once and use that for
646 + // both the peek and the atomic consume below.
647 + let code_hash = hash_token(code);
648 +
642 649 // Peek the code (does NOT consume it) so a failed client_id / redirect_uri
643 650 // / PKCE check leaves it usable for the legitimate client's retry instead of
644 651 // burning it (ultra-fuzz Run #1 Security LOW). The atomic consume below is
645 652 // what actually claims it, so concurrent redemptions stay race-safe.
646 - let oauth_code = db::oauth::peek_oauth_code(&state.db, code)
653 + let oauth_code = db::oauth::peek_oauth_code(&state.db, &code_hash)
647 654 .await?
648 655 .ok_or(AppError::BadRequest("Invalid or expired authorization code".to_string()))?;
649 656
@@ -678,7 +685,7 @@ async fn token_authorization_code(
678 685 // All checks passed — now atomically claim the code. A None here means a
679 686 // concurrent request already redeemed it (or it expired in the gap); the
680 687 // `used_at IS NULL` guard makes double-redemption impossible.
681 - let oauth_code = db::oauth::consume_oauth_code(&state.db, code)
688 + let oauth_code = db::oauth::consume_oauth_code(&state.db, &code_hash)
682 689 .await?
683 690 .ok_or(AppError::BadRequest("Invalid or expired authorization code".to_string()))?;
684 691