fix: origin check whatever the SameSite setting, no JWKS lock across the fetch, no refresh replay

.github/workflows/ci.yml

@@ -89,6 +89,9 @@ jobs:
       - name: Conformance
         env:
           SEAMLESS_API_DIR: ${{ github.workspace }}/seamless-auth-api
+          # This adapter ships the content-type gate, so hold it to the pending
+          # contract (seamless-cli verify/harness/lib/pendingContracts.ts).
+          SEAMLESS_VERIFY_CONTRACTS: content-type-gate
         run: node seamless-cli/dist/index.js verify --adapter-url=http://localhost:8080
       - name: Reference app log
         if: always()

AGENTS.md

@@ -64,8 +64,9 @@ so CI builds what was tested.
 - `src/adapter.rs`: `Adapter`, the router, the request pipeline, the upstream call, writing replies.
 - `src/routes.rs`: manifest routes, credential resolution (with silent refresh), session
   verification, what a cookie-transport body may contain.
-- `src/refresh.rs`: refresh sharing (one result per refresh token for 5 seconds, on its own task),
-  `POST /refresh`, logout.
+- `src/refresh.rs`: refresh sharing (one rotation per refresh token while in flight, on its own task,
+  and for cookie transport only a successful rotation kept for 5 seconds for other tabs; bearer
+  replays always reach the API), `POST /refresh`, logout.
 - `src/manifest.rs`: parsing, matching, the live and embedded manifest.
 - `src/guard.rs`: `require_auth`, `authenticate`, the `User` extractor.
 - `src/console.rs`: `console_router`, the admin dashboard proxy.

README.md

@@ -86,15 +86,19 @@ state come from that verified token, so a cookie forged with a leaked cookie sec
 claims of its own. When the JWKS cannot be fetched the session is refused.
 
 A request the guard admits on the session cookie also gets the auth routes' cross-site check. It
-applies only while cookies are `SameSite=None`, and only to state-changing methods (not `GET`,
-`HEAD` or `OPTIONS`); a refused request answers 403 `cross_site_request_blocked`:
+applies whatever the cookies' `SameSite` setting (a sibling subdomain is same-site, so `Lax` and
+`Strict` cookies still reach it), and only to state-changing methods (not `GET`, `HEAD` or
+`OPTIONS`); a refused request answers 403 `cross_site_request_blocked`:
 
+- A `null` `Origin` (in any case) is refused first, whatever `Sec-Fetch-Site` and
+  `allowed_origins` say.
 - `Sec-Fetch-Site: cross-site` is refused, and `same-origin` (or `none`) passes.
 - `Sec-Fetch-Site: same-site` also covers a sibling subdomain, so when `allowed_origins` is set
   the `Origin` must be on it, and a request with no `Origin` is refused. Without
   `allowed_origins` it passes.
-- With no `Sec-Fetch-Site` (an older browser or a server), a request with no `Origin` passes, a
-  `null` `Origin` is refused, and when `allowed_origins` is set the `Origin` must be on it.
+- With no `Sec-Fetch-Site` (an older browser or a server), a request with no `Origin` passes, and
+  when `allowed_origins` is set the `Origin` must be on it.
+- An empty `allowed_origins` is still set: it allows no cross-origin caller.
 
 A bearer token is never attached by a browser, so it is not checked.
 
@@ -138,7 +142,14 @@ get(|user: Option<User>| async move { /* signed in or not */ })
 The auth routes forward a request body to the auth API as JSON, so a body must be sent as
 `application/json` (or a `+json` type). Anything else answers 415 `unsupported_media_type`: a
 cross-site form can post a `text/plain` body shaped like JSON with no CORS preflight, and read as
-JSON it would be a sign-in the user never made. The client SDKs already send JSON.
+JSON it would be a sign-in the user never made. A state-changing request with no body (`POST`,
+`PUT`, `PATCH` or `DELETE`) to a manifest route must still send `Content-Type: application/json` or the
+`x-seamless-auth-transport` header, or it answers 415 too: either one forces a CORS preflight,
+which an HTML form on another site, sibling subdomains included, cannot send. The header may
+carry any value; only `bearer` selects bearer transport. `POST /refresh`, `DELETE /logout` and
+`DELETE /logout/all` are exempt from this rule, since the client SDKs send them with no body and no
+header, but they still get the cross-site check. Routes behind `require_auth` get the cross-site
+check only.
 
 ### Errors and logout
 
@@ -163,7 +174,7 @@ API cannot be reached.
 | `cookie_domain` | none | Cookie `Domain` |
 | `insecure_cookies` | `false` | Drops `Secure`, for local development over HTTP, and allows example secrets |
 | `same_site` | `None`, or `Lax` with `insecure_cookies` | Cookie `SameSite` |
-| `allowed_origins` | none | The only cross-origin (including same-site) callers allowed to change state while cookies are `SameSite=None` |
+| `allowed_origins` | none | The only cross-origin (including same-site) callers allowed to change state; an empty list allows none |
 | `session_cookie_names` | `seamless-access`, `seamless-refresh` | Session cookie names |
 | `flow_cookie_names` | `seamless-ephemeral` | Sign-in flow cookie names |
 | `deliver` | none | Sends OTP codes and magic links through your own transports |

src/adapter.rs

@@ -24,7 +24,7 @@ use crate::origin::check_origin;
 use crate::refresh::Refresher;
 use crate::service_token::ServiceTokens;
 
-const TRANSPORT_HEADER: &str = "x-seamless-auth-transport";
+pub(crate) const TRANSPORT_HEADER: &str = "x-seamless-auth-transport";
 pub(crate) const DELIVERY_MODE_HEADER: &str = "x-seamless-auth-delivery-mode";
 pub(crate) const DELIVERY_KINDS_HEADER: &str = "x-seamless-auth-delivery-kinds";
 pub(crate) const MAX_BODY_BYTES: usize = 1 << 20;

src/client_ip.rs

@@ -115,21 +115,53 @@ impl TrustedProxies {
             .collect();
 
         for hop in hops.iter().rev() {
-            match hop.parse::<IpAddr>() {
-                Ok(ip) if self.trusted(ip) => continue,
-                Ok(ip) => return Some(ip),
+            match hop_address(hop) {
+                Some(ip) if self.trusted(ip) => continue,
+                Some(ip) => return Some(ip),
                 // Not an address, so not a trusted proxy either: whoever wrote it
                 // is the client, and it has no usable address.
-                Err(_) => return None,
+                None => return None,
             }
         }
         match hops.first() {
-            Some(first) => first.parse().ok(),
+            Some(first) => hop_address(first),
             None => Some(peer),
         }
     }
 }
 
+/// A hop's address, read past the port some proxies append (`ip:port`,
+/// `[v6]:port`) and the brackets around a bare `[v6]`.
+fn hop_address(hop: &str) -> Option<IpAddr> {
+    if let Ok(ip) = hop.parse() {
+        return Some(ip);
+    }
+    let (host, port) = match hop.strip_prefix('[') {
+        Some(rest) => {
+            let (host, after) = rest.split_once(']')?;
+            match after {
+                "" => (host, None),
+                _ => (host, Some(after.strip_prefix(':')?)),
+            }
+        }
+        None => {
+            let (host, port) = hop.split_once(':')?;
+            // More than one colon without brackets is an IPv6 address, which
+            // already failed to parse above.
+            if port.contains(':') {
+                return None;
+            }
+            (host, Some(port))
+        }
+    };
+    if let Some(port) = port
+        && port.parse::<u16>().is_err()
+    {
+        return None;
+    }
+    host.parse().ok()
+}
+
 fn width(addr: IpAddr) -> u8 {
     match addr {
         IpAddr::V4(_) => 32,

src/jwks.rs

@@ -1,10 +1,10 @@
 use std::collections::HashMap;
+use std::sync::{Mutex, MutexGuard};
 use std::time::{Duration, Instant};
 
 use jsonwebtoken::{Algorithm, DecodingKey, Validation};
 use serde::Deserialize;
 use serde_json::Value;
-use tokio::sync::Mutex;
 
 use crate::jwt::{CLOCK_SKEW_SECS, Claims};
 
@@ -22,7 +22,10 @@ const MAX_JWKS_BYTES: usize = 256 * 1024;
 pub(crate) struct JwksCache {
     url: String,
     client: reqwest::Client,
+    /// Held only to read or update the key set, never across a fetch.
     state: Mutex<KeySet>,
+    /// Held across a fetch, so only one runs at a time.
+    fetching: tokio::sync::Mutex<()>,
 }
 
 #[derive(Default)]
@@ -63,6 +66,17 @@ struct JwkSet {
     keys: Vec<Jwk>,
 }
 
+/// What the key set holds for a kid.
+enum Lookup {
+    Fresh(Key),
+    /// A key from a set past its maximum age, or none, and whether a fetch is
+    /// due.
+    Cached {
+        key: Option<Key>,
+        due: bool,
+    },
+}
+
 impl JwksCache {
     pub fn new(auth_server_url: &str, client: reqwest::Client) -> Self {
         Self::at(format!("{auth_server_url}/.well-known/jwks.json"), client)
@@ -73,45 +87,82 @@ impl JwksCache {
             url,
             client,
             state: Mutex::new(KeySet::default()),
+            fetching: tokio::sync::Mutex::new(()),
         }
     }
 
     pub async fn key(&self, kid: &str) -> Result<Key, String> {
-        let mut state = self.state.lock().await;
+        let cached = match self.lookup(kid) {
+            Lookup::Fresh(key)
+            | Lookup::Cached {
+                key: Some(key),
+                due: false,
+            } => return Ok(key),
+            Lookup::Cached { key, .. } => key,
+        };
+        // A kid already cached is served while another caller fetches, so a slow
+        // JWKS endpoint never holds up tokens it would verify anyway. An unknown
+        // kid waits for the fetch in flight, which may bring it.
+        let _fetching = match cached {
+            Some(key) => match self.fetching.try_lock() {
+                Ok(guard) => guard,
+                Err(_) => return Ok(key),
+            },
+            None => self.fetching.lock().await,
+        };
+        match self.lookup(kid) {
+            Lookup::Fresh(key) => return Ok(key),
+            Lookup::Cached { key, due: false } => {
+                return key.ok_or_else(|| format!("unknown key id {kid:?}"));
+            }
+            Lookup::Cached { due: true, .. } => {}
+        }
+
+        self.lock_state().attempted_at = Some(Instant::now());
+        let fetched = self.fetch().await;
+        let mut state = self.lock_state();
+        match fetched {
+            Ok(keys) => {
+                state.keys = Some(keys);
+                state.fetched_at = Some(Instant::now());
+            }
+            // A stale key set keeps verifying while the API cannot be reached,
+            // rather than signing every user out.
+            Err(err) if state.keys.is_some() => {
+                tracing::warn!("[seamless-auth] Could not refresh the JWKS ({err}).");
+            }
+            Err(err) => return Err(err),
+        }
+        known(&state, kid)
+    }
 
+    fn lookup(&self, kid: &str) -> Lookup {
+        let state = self.lock_state();
         let fresh = state.fetched_at.is_some_and(|at| at.elapsed() < MAX_AGE);
-        let known = state.keys.as_ref().and_then(|keys| keys.get(kid)).cloned();
-        if let (true, Some(key)) = (fresh, &known) {
-            return Ok(key.clone());
+        let key = state.keys.as_ref().and_then(|keys| keys.get(kid)).cloned();
+        if let (true, Some(key)) = (fresh, &key) {
+            return Lookup::Fresh(key.clone());
         }
 
         let backoff = if state.keys.is_some() {
             REFETCH_INTERVAL
         } else {
             FAILED_FETCH_BACKOFF
         };
-        if state.attempted_at.is_none_or(|at| at.elapsed() >= backoff) {
-            state.attempted_at = Some(Instant::now());
-            match self.fetch().await {
-                Ok(keys) => {
-                    state.keys = Some(keys);
-                    state.fetched_at = Some(Instant::now());
-                }
-                // A stale key set keeps verifying while the API cannot be reached,
-                // rather than signing every user out.
-                Err(err) if state.keys.is_some() => {
-                    tracing::warn!("[seamless-auth] Could not refresh the JWKS ({err}).");
-                }
-                Err(err) => return Err(err),
-            }
-        }
+        let due = state.attempted_at.is_none_or(|at| at.elapsed() >= backoff);
+        Lookup::Cached { key, due }
+    }
 
-        state
-            .keys
-            .as_ref()
-            .and_then(|keys| keys.get(kid))
-            .cloned()
-            .ok_or_else(|| format!("unknown key id {kid:?}"))
+    fn lock_state(&self) -> MutexGuard<'_, KeySet> {
+        self.state.lock().unwrap_or_else(|e| e.into_inner())
+    }
+
+    /// Makes the last fetch attempt look older than the refetch interval, so the
+    /// next unknown kid fetches again.
+    #[cfg(test)]
+    pub(crate) async fn backdate_attempt(&self) {
+        self.lock_state().attempted_at =
+            Instant::now().checked_sub(REFETCH_INTERVAL + Duration::from_secs(1));
     }
 
     async fn fetch(&self) -> Result<HashMap<String, Key>, String> {
@@ -179,3 +230,12 @@ impl JwksCache {
             .map_err(|e| e.to_string())
     }
 }
+
+fn known(state: &KeySet, kid: &str) -> Result<Key, String> {
+    state
+        .keys
+        .as_ref()
+        .and_then(|keys| keys.get(kid))
+        .cloned()
+        .ok_or_else(|| format!("unknown key id {kid:?}"))
+}

src/options.rs

@@ -116,7 +116,7 @@ pub struct Builder {
     cookie_domain: Option<String>,
     insecure_cookies: bool,
     same_site: Option<SameSite>,
-    allowed_origins: Vec<String>,
+    allowed_origins: Option<Vec<String>>,
     access_cookie_name: String,
     registration_cookie_name: String,
     refresh_cookie_name: String,
@@ -149,7 +149,7 @@ impl Builder {
             cookie_domain: None,
             insecure_cookies: false,
             same_site: None,
-            allowed_origins: Vec::new(),
+            allowed_origins: None,
             access_cookie_name: "seamless-access".into(),
             registration_cookie_name: "seamless-ephemeral".into(),
             refresh_cookie_name: "seamless-refresh".into(),
@@ -219,15 +219,16 @@ impl Builder {
         self
     }
 
-    /// The only cross-origin callers allowed to change state while cookies are
-    /// `SameSite=None`. With it set, a `Sec-Fetch-Site: same-site` request (which
-    /// includes a sibling subdomain) needs an `Origin` on the list as well.
+    /// The only cross-origin callers allowed to change state. With it set, a
+    /// `Sec-Fetch-Site: same-site` request (which includes a sibling subdomain),
+    /// or a request without fetch metadata that carries an `Origin`, needs that
+    /// `Origin` on the list. An empty list allows no cross-origin caller.
     pub fn allowed_origins<I, S>(mut self, origins: I) -> Self
     where
         I: IntoIterator<Item = S>,
         S: Into<String>,
     {
-        self.allowed_origins = origins.into_iter().map(Into::into).collect();
+        self.allowed_origins = Some(origins.into_iter().map(Into::into).collect());
         self
     }
 
@@ -438,7 +439,8 @@ pub(crate) struct Config {
     pub cookie_domain: Option<String>,
     pub secure_cookies: bool,
     pub same_site: SameSite,
-    pub allowed_origins: Vec<String>,
+    /// `None` when not configured; an empty list is configured and allows none.
+    pub allowed_origins: Option<Vec<String>>,
     pub access_cookie_name: String,
     pub registration_cookie_name: String,
     pub refresh_cookie_name: String,