From 9002d4012b60060a5c6365863f9a4e350f6627f8 Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Mon, 7 Sep 2026 23:56:01 +0300 Subject: [PATCH 01/11] feat(config): add the OAuth dance keys behind an all-or-nothing gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The backend becomes a confidential OAuth client, so it needs the provider's client_secret, this instance's external redirect_uri and the provider's two endpoints. None carries an in-code default: an endpoint baked into the binary is one an operator cannot see when they need to know where their sign-ins are going. OAuthDanceEnabled is both-or-neither. A config naming a client_secret but no endpoints would otherwise register the routes and then send the browser to a URL with no host — a failure that arrives as a 302 and reads as success in every access log. It never aborts startup: an instance that configures no dance boots exactly as before. app.oauth_cookie_path and app.oauth_callback_path are in the gate for a sharper reason than the rest. Both are empty-is-not-inert: an empty cookie path makes net/http omit the attribute, and the browser then scopes the cookie to the internal route behind the proxy — precisely the value that never comes back. Co-Authored-By: Claude Opus 5 --- deployment/maintmode/dev/app.config.yaml | 58 +++++++++ .../maintmode/dev/app.secrets.sample.yaml | 24 +++- deployment/maintmode/local/app.config.yaml | 58 +++++++++ .../maintmode/local/app.secrets.sample.yaml | 24 +++- deployment/maintmode/prod/app.config.yaml | 58 +++++++++ .../maintmode/prod/app.secrets.sample.yaml | 25 ++-- deployment/maintmode/test/app.config.yaml | 58 +++++++++ .../maintmode/test/app.secrets.sample.yaml | 15 ++- internal/config/app_config.go | 114 ++++++++++++++++-- 9 files changed, 403 insertions(+), 31 deletions(-) diff --git a/deployment/maintmode/dev/app.config.yaml b/deployment/maintmode/dev/app.config.yaml index 7e34cb5..14a4caa 100644 --- a/deployment/maintmode/dev/app.config.yaml +++ b/deployment/maintmode/dev/app.config.yaml @@ -5,6 +5,33 @@ app: # Moved in from the auth config — the merged process serves the auth # routes, so it needs this or those redirects target an empty URL. frontend_url: http://localhost:3000 + # The frontend route the OAuth dance sends the browser back to, appended to + # frontend_url. It belongs to the frontend rather than to this service, so a + # rename there needs no backend release. + # + # Required whenever the dance is armed, with no in-code default: a value from + # config and another from the binary would mean two places to look when a + # redirect lands somewhere unexpected. Leaving it empty leaves the dance + # unregistered rather than redirecting to the frontend's bare origin. + oauth_callback_path: /auth/oauth/callback + + # The Path attribute of the two dance cookies, and it must be the EXTERNAL + # prefix the browser sees. Caddy serves this backend under + # `handle_path /auth/*` and STRIPS the prefix, so the app sees + # /api/v1/login/oauth/... while the browser's URL space is + # /auth/api/v1/login/oauth/... A cookie scoped to what the app sees is never + # sent back on the callback, and the dance then dies as a 302 that reads as + # success in every access log. + # + # Keep it the narrowest prefix covering both /start and /callback. Widening to + # "/" also works and only means two httpOnly, minutes-long cookies travel more + # than they need; narrowing past the shared prefix breaks every sign-in. + # + # Required whenever the dance is armed, with no in-code default: leaving it + # empty is not inert — the attribute is then omitted and the browser scopes + # the cookie to the internal request path, which is exactly the value that + # never comes back. So an empty value leaves the dance unregistered instead. + oauth_cookie_path: /auth/api/v1/login/oauth auth: # Open self-registration: an unknown, uninvited user signs in as guest. ON in @@ -15,6 +42,12 @@ auth: # email is guessable in principle, and this window plus the attempt ceiling # below are what bound that. otp_ttl: 5m + # How long a signed OAuth-dance state stays valid: the span from pressing + # "sign in" to finishing a consent screen, password prompt and second factor + # included. Enforced by the signature rather than by the cookie's MaxAge, so + # lengthening it widens a real window — a captured (state, cookie) pair is + # replayable for exactly this long. + oauth_dance_state_ttl: 10m # The minimum time BOTH one-time-code endpoints take to answer, whatever they # did. This is a security knob, not a throttle, and its VALUE is the property: # an address with no account is refused after a single indexed SELECT, while a @@ -133,6 +166,31 @@ oauth_providers: use_stub: true google: client_id: + # Backend-driven OAuth dance (RUK-291) — OFF by default. + # + # Uncommenting ALL FOUR keys registers /login/oauth/{provider}/start, + # /callback and /code/exchange. Leaving them commented keeps this instance + # exactly as it was: the BFF path at /login/oauth/exchange/google stays the + # only way in. Setting some but not all logs a warning and registers + # nothing — a dance that cannot reach the provider is worse than no dance. + # + # Before uncommenting client_secret, make sure the key exists in the secret + # store this stand reads: the resolver hard-fails on a missing key, so the + # reference alone stops the instance booting. + # + # redirect_uri must be the EXTERNAL url — Caddy serves this backend under + # `handle_path /auth/*` and strips the prefix, so the app sees /api/v1/... + # while Google must be told /auth/api/v1/... Registering the internal form + # yields a redirect_uri_mismatch that never reaches our logs. + # client_secret: + # redirect_uri: http://localhost:9000/auth/api/v1/login/oauth/google/callback + # auth_url and token_url are the provider's endpoints. They carry no + # in-code default on purpose: an endpoint baked into the binary is one an + # operator cannot see when they need to know where their sign-ins are + # going. The values below are Google's own; a stand pointing at a local + # fake overrides them here. + # auth_url: https://accounts.google.com/o/oauth2/v2/auth + # token_url: https://oauth2.googleapis.com/token jwtverifier: jwks_url: https://www.googleapis.com/oauth2/v3/certs jwt_issuers: diff --git a/deployment/maintmode/dev/app.secrets.sample.yaml b/deployment/maintmode/dev/app.secrets.sample.yaml index 6e9d250..fc12250 100644 --- a/deployment/maintmode/dev/app.secrets.sample.yaml +++ b/deployment/maintmode/dev/app.secrets.sample.yaml @@ -19,11 +19,21 @@ valkey/password: "" # Google OAuth — client_id ONLY. # -# There is deliberately no client_secret: the BFF (maintmode-ui, NextAuth) owns -# the authorization-code exchange with Google. The backend only verifies the -# resulting id_token offline against Google's JWKS, using this client_id as the -# expected audience. Do not add a client_secret here — the code no longer reads -# one, and a second copy of a credential is a second thing to leak. +# Until RUK-291 there was deliberately no client_secret here: the BFF +# (maintmode-ui, NextAuth) owned the authorization-code exchange and the backend +# only verified the resulting id_token offline against Google's JWKS. +# +# RUK-291 made the backend a confidential OAuth client, so a client_secret is now +# meaningful — but ONLY for the backend-driven dance, which is OFF unless +# app.config.yaml also sets oauth_providers.google.client_secret and +# redirect_uri. The BFF path still needs nothing but the client_id. +# +# ORDER MATTERS. The secret resolver hard-fails on a missing key, so a +# reference in app.config.yaml without the matching entry below +# stops the instance booting. Add the key here FIRST, then the config reference. +# Note `make secrets` only copies this sample when app.secrets.yaml is ABSENT — +# on a machine that already has one, add the key by hand or the next run dies at +# config load. # # The client_id is not a secret (it ships in every browser redirect), but it # must match the client the BFF uses or every token fails audience validation. @@ -31,6 +41,10 @@ valkey/password: "" # MAINTMODE_GOOGLE_OAUTH_CLIENT_ID. "oauth/google/client_id": "CHANGE_ME.apps.googleusercontent.com" +# Backend-driven OAuth dance (RUK-291). Leave as-is unless +# app.config.yaml opts in; the routes stay unregistered without it. +"oauth/google/client_secret": "CHANGE_ME-google-client-secret" + # JWT signing — the merged binary mints and verifies its own tokens. # issuer_private_key hex-encoded raw P-256 private key # issuer_kid key id advertised in the in-process JWKS diff --git a/deployment/maintmode/local/app.config.yaml b/deployment/maintmode/local/app.config.yaml index dccf9dc..30b2b0b 100644 --- a/deployment/maintmode/local/app.config.yaml +++ b/deployment/maintmode/local/app.config.yaml @@ -5,6 +5,33 @@ app: # Moved in from the auth config — the merged process serves the auth # routes, so it needs this or those redirects target an empty URL. frontend_url: http://localhost:9000 + # The frontend route the OAuth dance sends the browser back to, appended to + # frontend_url. It belongs to the frontend rather than to this service, so a + # rename there needs no backend release. + # + # Required whenever the dance is armed, with no in-code default: a value from + # config and another from the binary would mean two places to look when a + # redirect lands somewhere unexpected. Leaving it empty leaves the dance + # unregistered rather than redirecting to the frontend's bare origin. + oauth_callback_path: /auth/oauth/callback + + # The Path attribute of the two dance cookies, and it must be the EXTERNAL + # prefix the browser sees. Caddy serves this backend under + # `handle_path /auth/*` and STRIPS the prefix, so the app sees + # /api/v1/login/oauth/... while the browser's URL space is + # /auth/api/v1/login/oauth/... A cookie scoped to what the app sees is never + # sent back on the callback, and the dance then dies as a 302 that reads as + # success in every access log. + # + # Keep it the narrowest prefix covering both /start and /callback. Widening to + # "/" also works and only means two httpOnly, minutes-long cookies travel more + # than they need; narrowing past the shared prefix breaks every sign-in. + # + # Required whenever the dance is armed, with no in-code default: leaving it + # empty is not inert — the attribute is then omitted and the browser scopes + # the cookie to the internal request path, which is exactly the value that + # never comes back. So an empty value leaves the dance unregistered instead. + oauth_cookie_path: /auth/api/v1/login/oauth auth: # Open self-registration: an unknown, uninvited user signs in as guest. ON in @@ -15,6 +42,12 @@ auth: # email is guessable in principle, and this window plus the attempt ceiling # below are what bound that. otp_ttl: 5m + # How long a signed OAuth-dance state stays valid: the span from pressing + # "sign in" to finishing a consent screen, password prompt and second factor + # included. Enforced by the signature rather than by the cookie's MaxAge, so + # lengthening it widens a real window — a captured (state, cookie) pair is + # replayable for exactly this long. + oauth_dance_state_ttl: 10m # The minimum time BOTH one-time-code endpoints take to answer, whatever they # did. This is a security knob, not a throttle, and its VALUE is the property: # an address with no account is refused after a single indexed SELECT, while a @@ -133,6 +166,31 @@ oauth_providers: use_stub: false google: client_id: + # Backend-driven OAuth dance (RUK-291) — OFF by default. + # + # Uncommenting ALL FOUR keys registers /login/oauth/{provider}/start, + # /callback and /code/exchange. Leaving them commented keeps this instance + # exactly as it was: the BFF path at /login/oauth/exchange/google stays the + # only way in. Setting some but not all logs a warning and registers + # nothing — a dance that cannot reach the provider is worse than no dance. + # + # Before uncommenting client_secret, make sure the key exists in the secret + # store this stand reads: the resolver hard-fails on a missing key, so the + # reference alone stops the instance booting. + # + # redirect_uri must be the EXTERNAL url — Caddy serves this backend under + # `handle_path /auth/*` and strips the prefix, so the app sees /api/v1/... + # while Google must be told /auth/api/v1/... Registering the internal form + # yields a redirect_uri_mismatch that never reaches our logs. + # client_secret: + # redirect_uri: http://localhost:9000/auth/api/v1/login/oauth/google/callback + # auth_url and token_url are the provider's endpoints. They carry no + # in-code default on purpose: an endpoint baked into the binary is one an + # operator cannot see when they need to know where their sign-ins are + # going. The values below are Google's own; a stand pointing at a local + # fake overrides them here. + # auth_url: https://accounts.google.com/o/oauth2/v2/auth + # token_url: https://oauth2.googleapis.com/token jwtverifier: jwks_url: https://www.googleapis.com/oauth2/v3/certs jwt_issuers: diff --git a/deployment/maintmode/local/app.secrets.sample.yaml b/deployment/maintmode/local/app.secrets.sample.yaml index 13d0a41..a09c2b2 100644 --- a/deployment/maintmode/local/app.secrets.sample.yaml +++ b/deployment/maintmode/local/app.secrets.sample.yaml @@ -21,11 +21,21 @@ # Google OAuth — client_id ONLY. # -# There is deliberately no client_secret: the BFF (maintmode-ui, NextAuth) owns -# the authorization-code exchange with Google. The backend only verifies the -# resulting id_token offline against Google's JWKS, using this client_id as the -# expected audience. Do not add a client_secret here — the code no longer reads -# one, and a second copy of a credential is a second thing to leak. +# Until RUK-291 there was deliberately no client_secret here: the BFF +# (maintmode-ui, NextAuth) owned the authorization-code exchange and the backend +# only verified the resulting id_token offline against Google's JWKS. +# +# RUK-291 made the backend a confidential OAuth client, so a client_secret is now +# meaningful — but ONLY for the backend-driven dance, which is OFF unless +# app.config.yaml also sets oauth_providers.google.client_secret and +# redirect_uri. The BFF path still needs nothing but the client_id. +# +# ORDER MATTERS. The secret resolver hard-fails on a missing key, so a +# reference in app.config.yaml without the matching entry below +# stops the instance booting. Add the key here FIRST, then the config reference. +# Note `make secrets` only copies this sample when app.secrets.yaml is ABSENT — +# on a machine that already has one, add the key by hand or the next run dies at +# config load. # # The client_id is not a secret (it ships in every browser redirect), but it # must match the client the BFF uses or every token fails audience validation. @@ -33,6 +43,10 @@ # MAINTMODE_GOOGLE_OAUTH_CLIENT_ID. "oauth/google/client_id": "CHANGE_ME.apps.googleusercontent.com" +# Backend-driven OAuth dance (RUK-291). Leave as-is unless +# app.config.yaml opts in; the routes stay unregistered without it. +"oauth/google/client_secret": "CHANGE_ME-google-client-secret" + # JWT signing — the merged binary mints and verifies its own tokens. # issuer_private_key hex-encoded raw P-256 private key # issuer_kid key id advertised in the in-process JWKS diff --git a/deployment/maintmode/prod/app.config.yaml b/deployment/maintmode/prod/app.config.yaml index c7e2ca2..00f3162 100644 --- a/deployment/maintmode/prod/app.config.yaml +++ b/deployment/maintmode/prod/app.config.yaml @@ -5,6 +5,33 @@ app: # Moved in from the auth config — the merged process serves the auth # routes, so it needs this or those redirects target an empty URL. frontend_url: https://maintmode.example.com + # The frontend route the OAuth dance sends the browser back to, appended to + # frontend_url. It belongs to the frontend rather than to this service, so a + # rename there needs no backend release. + # + # Required whenever the dance is armed, with no in-code default: a value from + # config and another from the binary would mean two places to look when a + # redirect lands somewhere unexpected. Leaving it empty leaves the dance + # unregistered rather than redirecting to the frontend's bare origin. + oauth_callback_path: /auth/oauth/callback + + # The Path attribute of the two dance cookies, and it must be the EXTERNAL + # prefix the browser sees. Caddy serves this backend under + # `handle_path /auth/*` and STRIPS the prefix, so the app sees + # /api/v1/login/oauth/... while the browser's URL space is + # /auth/api/v1/login/oauth/... A cookie scoped to what the app sees is never + # sent back on the callback, and the dance then dies as a 302 that reads as + # success in every access log. + # + # Keep it the narrowest prefix covering both /start and /callback. Widening to + # "/" also works and only means two httpOnly, minutes-long cookies travel more + # than they need; narrowing past the shared prefix breaks every sign-in. + # + # Required whenever the dance is armed, with no in-code default: leaving it + # empty is not inert — the attribute is then omitted and the browser scopes + # the cookie to the internal request path, which is exactly the value that + # never comes back. So an empty value leaves the dance unregistered instead. + oauth_cookie_path: /auth/api/v1/login/oauth auth: # Invite-only: an unknown user without an invitation is rejected with 403 @@ -16,6 +43,12 @@ auth: # email is guessable in principle, and this window plus the attempt ceiling # below are what bound that. otp_ttl: 5m + # How long a signed OAuth-dance state stays valid: the span from pressing + # "sign in" to finishing a consent screen, password prompt and second factor + # included. Enforced by the signature rather than by the cookie's MaxAge, so + # lengthening it widens a real window — a captured (state, cookie) pair is + # replayable for exactly this long. + oauth_dance_state_ttl: 10m # The minimum time BOTH one-time-code endpoints take to answer, whatever they # did. This is a security knob, not a throttle, and its VALUE is the property: # an address with no account is refused after a single indexed SELECT, while a @@ -170,6 +203,31 @@ valkey: oauth_providers: google: client_id: + # Backend-driven OAuth dance (RUK-291) — OFF by default. + # + # Uncommenting ALL FOUR keys registers /login/oauth/{provider}/start, + # /callback and /code/exchange. Leaving them commented keeps this instance + # exactly as it was: the BFF path at /login/oauth/exchange/google stays the + # only way in. Setting some but not all logs a warning and registers + # nothing — a dance that cannot reach the provider is worse than no dance. + # + # Before uncommenting client_secret, make sure the key exists in the secret + # store this stand reads: the resolver hard-fails on a missing key, so the + # reference alone stops the instance booting. + # + # redirect_uri must be the EXTERNAL url — Caddy serves this backend under + # `handle_path /auth/*` and strips the prefix, so the app sees /api/v1/... + # while Google must be told /auth/api/v1/... Registering the internal form + # yields a redirect_uri_mismatch that never reaches our logs. + # client_secret: + # redirect_uri: https:///auth/api/v1/login/oauth/google/callback + # auth_url and token_url are the provider's endpoints. They carry no + # in-code default on purpose: an endpoint baked into the binary is one an + # operator cannot see when they need to know where their sign-ins are + # going. The values below are Google's own; a stand pointing at a local + # fake overrides them here. + # auth_url: https://accounts.google.com/o/oauth2/v2/auth + # token_url: https://oauth2.googleapis.com/token jwtverifier: jwks_url: https://www.googleapis.com/oauth2/v3/certs jwt_issuers: diff --git a/deployment/maintmode/prod/app.secrets.sample.yaml b/deployment/maintmode/prod/app.secrets.sample.yaml index 3c757f4..2a4a9a9 100644 --- a/deployment/maintmode/prod/app.secrets.sample.yaml +++ b/deployment/maintmode/prod/app.secrets.sample.yaml @@ -13,18 +13,27 @@ db/dsn: "replace-me-prod-value" valkey/password: "replace-me-prod-value" -# Google OAuth — client_id ONLY. +# Google OAuth. # -# There is deliberately no client_secret in prod, or anywhere else in this -# service. The BFF (maintmode-ui, NextAuth) owns the authorization-code -# exchange with Google and holds the only copy of the secret; the backend -# verifies the resulting id_token offline against Google's JWKS, using this -# client_id as the expected audience. +# Until RUK-291 there was deliberately no client_secret in prod, or anywhere +# else in this service: the BFF (maintmode-ui, NextAuth) owned the +# authorization-code exchange and held the only copy, while the backend merely +# verified the resulting id_token offline against Google's JWKS. # -# Must match the client the BFF uses (MAINTMODE_GOOGLE_OAUTH_CLIENT_ID) or -# every token fails audience validation. +# RUK-291 made the backend a confidential OAuth client, so the secret is now +# meaningful here — but ONLY for the backend-driven dance, which stays OFF +# unless app.config.yaml also sets oauth_providers.google.client_secret and +# redirect_uri. The BFF path still needs nothing but the client_id. +# +# client_id must match the client the BFF uses +# (MAINTMODE_GOOGLE_OAUTH_CLIENT_ID) or every token fails audience validation. oauth/google/client_id: "replace-me-prod-value" +# Required only when the dance is armed. This file documents the key set that +# whatever store feeds a real deploy must contain, and the resolver hard-fails +# on a missing key — so the store gets this BEFORE app.config.yaml references it. +oauth/google/client_secret: "replace-me-prod-value" + # Auth/JWT — the merged process mints and verifies its own tokens. # issuer_private_key hex-encoded raw P-256 private key (JWT signing key) # issuer_kid key id advertised in the in-process JWKS diff --git a/deployment/maintmode/test/app.config.yaml b/deployment/maintmode/test/app.config.yaml index e5d97bb..0a6142e 100644 --- a/deployment/maintmode/test/app.config.yaml +++ b/deployment/maintmode/test/app.config.yaml @@ -5,6 +5,33 @@ app: # Moved in from the auth config — the merged process serves the auth # routes, so it needs this or those redirects target an empty URL. frontend_url: http://localhost:9000 + # The frontend route the OAuth dance sends the browser back to, appended to + # frontend_url. It belongs to the frontend rather than to this service, so a + # rename there needs no backend release. + # + # Required whenever the dance is armed, with no in-code default: a value from + # config and another from the binary would mean two places to look when a + # redirect lands somewhere unexpected. Leaving it empty leaves the dance + # unregistered rather than redirecting to the frontend's bare origin. + oauth_callback_path: /auth/oauth/callback + + # The Path attribute of the two dance cookies, and it must be the EXTERNAL + # prefix the browser sees. Caddy serves this backend under + # `handle_path /auth/*` and STRIPS the prefix, so the app sees + # /api/v1/login/oauth/... while the browser's URL space is + # /auth/api/v1/login/oauth/... A cookie scoped to what the app sees is never + # sent back on the callback, and the dance then dies as a 302 that reads as + # success in every access log. + # + # Keep it the narrowest prefix covering both /start and /callback. Widening to + # "/" also works and only means two httpOnly, minutes-long cookies travel more + # than they need; narrowing past the shared prefix breaks every sign-in. + # + # Required whenever the dance is armed, with no in-code default: leaving it + # empty is not inert — the attribute is then omitted and the browser scopes + # the cookie to the internal request path, which is exactly the value that + # never comes back. So an empty value leaves the dance unregistered instead. + oauth_cookie_path: /auth/api/v1/login/oauth auth: # OFF in the API-test stack: TestMain seeds the bootstrap admin and suites @@ -15,6 +42,12 @@ auth: # email is guessable in principle, and this window plus the attempt ceiling # below are what bound that. otp_ttl: 5m + # How long a signed OAuth-dance state stays valid: the span from pressing + # "sign in" to finishing a consent screen, password prompt and second factor + # included. Enforced by the signature rather than by the cookie's MaxAge, so + # lengthening it widens a real window — a captured (state, cookie) pair is + # replayable for exactly this long. + oauth_dance_state_ttl: 10m # The minimum time BOTH one-time-code endpoints take to answer, whatever they # did. This is a security knob, not a throttle, and its VALUE is the property: # an address with no account is refused after a single indexed SELECT, while a @@ -132,6 +165,31 @@ oauth_providers: use_stub: true google: client_id: + # Backend-driven OAuth dance (RUK-291) — OFF by default. + # + # Uncommenting ALL FOUR keys registers /login/oauth/{provider}/start, + # /callback and /code/exchange. Leaving them commented keeps this instance + # exactly as it was: the BFF path at /login/oauth/exchange/google stays the + # only way in. Setting some but not all logs a warning and registers + # nothing — a dance that cannot reach the provider is worse than no dance. + # + # Before uncommenting client_secret, make sure the key exists in the secret + # store this stand reads: the resolver hard-fails on a missing key, so the + # reference alone stops the instance booting. + # + # redirect_uri must be the EXTERNAL url — Caddy serves this backend under + # `handle_path /auth/*` and strips the prefix, so the app sees /api/v1/... + # while Google must be told /auth/api/v1/... Registering the internal form + # yields a redirect_uri_mismatch that never reaches our logs. + # client_secret: + # redirect_uri: http://localhost:9000/auth/api/v1/login/oauth/google/callback + # auth_url and token_url are the provider's endpoints. They carry no + # in-code default on purpose: an endpoint baked into the binary is one an + # operator cannot see when they need to know where their sign-ins are + # going. The values below are Google's own; a stand pointing at a local + # fake overrides them here. + # auth_url: https://accounts.google.com/o/oauth2/v2/auth + # token_url: https://oauth2.googleapis.com/token jwtverifier: jwks_url: https://www.googleapis.com/oauth2/v3/certs jwt_issuers: diff --git a/deployment/maintmode/test/app.secrets.sample.yaml b/deployment/maintmode/test/app.secrets.sample.yaml index 6c00e7b..f8f7e50 100644 --- a/deployment/maintmode/test/app.secrets.sample.yaml +++ b/deployment/maintmode/test/app.secrets.sample.yaml @@ -19,16 +19,21 @@ db/dsn: "postgres://postgres:postgres@pg_doorman:6432/maintmode?sslmode=disable" valkey/password: "" -# Google OAuth — client_id ONLY. +# Google OAuth. # -# There is deliberately no client_secret: the BFF (maintmode-ui, NextAuth) owns -# the authorization-code exchange with Google; the backend only verifies the -# resulting id_token against Google's JWKS. Do not add a client_secret here — -# the code no longer reads one. +# Until RUK-291 there was deliberately no client_secret: the BFF (maintmode-ui, +# NextAuth) owned the authorization-code exchange and the backend only verified +# the resulting id_token against Google's JWKS. RUK-291 made the backend a +# confidential client, so the secret below is meaningful — but only once +# app.config.yaml uncomments its reference, which no stand does by default. # # Placeholder is fine: the stub provider short-circuits verification in test. "oauth/google/client_id": "test-client-id.apps.googleusercontent.com" +# Backend-driven OAuth dance (RUK-291). Leave as-is unless +# app.config.yaml opts in; the routes stay unregistered without it. +"oauth/google/client_secret": "CHANGE_ME-google-client-secret" + # JWT signing — the merged binary mints and verifies its own tokens. "jwt/issuer_private_key": "1be2f1f68285c972b750b7718b00d5453f2c08f88c7894d1b9013f75a439de20" "jwt/issuer_kid": "99d1e557df9b619fd046322c1e7f196e" diff --git a/internal/config/app_config.go b/internal/config/app_config.go index f8e9655..d6cef8e 100644 --- a/internal/config/app_config.go +++ b/internal/config/app_config.go @@ -185,18 +185,57 @@ type Valkey struct { DB int `mapstructure:"db"` } -// GoogleOauthProvider configures ID-token verification only. +// GoogleOauthProvider configures Google sign-in. // -// There is deliberately no client_secret, redirect_url or scopes: the BFF -// (maintmode-ui, NextAuth) owns the authorization-code exchange with Google -// and posts us the id_token. Those three configure a token-endpoint round -// trip this service never makes. We verify the token offline against Google's -// JWKS, and the only thing we need from the OAuth client is ClientID, as the -// expected audience. Do not reintroduce a secret here — it would be an unused -// copy of a credential that only the BFF needs. +// HISTORY, because the previous comment here forbade exactly what now follows: +// until RUK-291 this block held a client_id and nothing else, because the BFF +// (maintmode-ui, NextAuth) owned the authorization-code exchange and merely +// posted us the resulting id_token. A client_secret would have been an unused +// copy of a credential only the BFF needed. +// +// RUK-291 made the backend a confidential OAuth client: it now runs the dance +// itself (/login/oauth/{provider}/start + /callback), so ClientSecret, +// RedirectURI and the two endpoints are load-bearing. The BFF path +// (/login/oauth/exchange/google) is still live and still needs nothing but +// ClientID, which is why both sets of fields coexist here rather than one +// replacing the other. type GoogleOauthProvider struct { ClientID string `mapstructure:"client_id"` JWTVerify JWTVerifierConfig `mapstructure:"jwtverifier"` + + // ClientSecret is the confidential-client credential used at the token + // endpoint. Resolved from the secret store via a reference in + // app.config.yaml — never written literally into a config file. + ClientSecret string `mapstructure:"client_secret"` + // RedirectURI is this instance's EXTERNAL callback URL, and it must be the + // external form. Caddy serves the backend under `handle_path /auth/*`, which + // strips the prefix, so the app sees /api/v1/... while the browser and + // Google see /auth/api/v1/... Registering the internal form with Google + // yields a redirect_uri_mismatch that never reaches our logs. + RedirectURI string `mapstructure:"redirect_uri"` + // AuthURL and TokenURL override the provider's endpoints, so a stand can + // point the dance at a local fake. Empty means Google's own; the defaults + // live with the gateway that dials them, not here. + AuthURL string `mapstructure:"auth_url"` + TokenURL string `mapstructure:"token_url"` +} + +// danceConfigured reports whether every value the dance needs to reach the +// provider is present. All or nothing: a dance that cannot reach the token +// endpoint is worse than no dance. +// +// AuthURL and TokenURL are in here because there is no in-code default for +// them. Without this check a config naming a client_secret and a redirect_uri +// but no endpoints would register the routes and then send the browser to a +// URL with no host — a failure that arrives as a 302 and reads as success in +// every access log. +// +// RedirectURI is checked for presence only. It must be the EXTERNAL form — a +// proxy that strips a path prefix makes what the provider is told differ from +// the route the app sees — but getting that right is the operator's job, not +// something this predicate second-guesses. +func (g GoogleOauthProvider) danceConfigured() bool { + return g.ClientSecret != "" && g.RedirectURI != "" && g.AuthURL != "" && g.TokenURL != "" } // OauthProviders has no `stub` section: the stub short-circuits verification in @@ -269,6 +308,28 @@ type LoggerConfig struct { type App struct { FrontendURL string `mapstructure:"frontend_url"` + // OAuthCallbackPath is the frontend route the dance sends the browser back + // to, appended to FrontendURL. Required when the dance is armed — see + // OAuthDanceEnabled. + // + // It is configurable rather than a literal because the route belongs to the + // frontend, not to this service: RUK-292 owns that page, and a rename there + // should not need a backend release. + OAuthCallbackPath string `mapstructure:"oauth_callback_path"` + // OAuthCookiePath is the Path attribute of the two dance cookies, and it + // must be the EXTERNAL prefix the browser sees, not the route this service + // mounts. Caddy serves the backend under `handle_path /auth/*`, which strips + // the prefix: a cookie scoped to the internal route is never sent back on + // the callback, and the dance then dies as a 302 that reads as success. + // + // Stated outright rather than derived from redirect_uri because this is the + // value an operator reaches for when a sign-in silently fails, and reading + // it should not mean re-deriving it in your head. + // + // Required when the dance is armed — see OAuthDanceEnabled. Scope it to the + // narrowest prefix covering both /start and /callback; "/" works and only + // means the two httpOnly, minutes-long cookies travel more than they need. + OAuthCookiePath string `mapstructure:"oauth_cookie_path"` // InvitationTTL is how long a user invitation link stays valid. Zero falls // back to a 7-day default at wiring time. InvitationTTL time.Duration `mapstructure:"invitation_ttl"` @@ -291,6 +352,15 @@ type Auth struct { // email is guessable in principle, and its lifetime is the main control on // that until the per-code attempt ceiling exists. OTPTTL time.Duration `mapstructure:"otp_ttl"` + // OAuthDanceStateTTL is how long a signed OAuth-dance state stays valid: + // the span from pressing "sign in" to finishing a consent screen, password + // prompt and second factor included. Zero falls back to 10 minutes at wiring + // time. + // + // It is enforced by the signature rather than by the cookie's MaxAge, so + // lengthening it widens a real window: a captured (state, cookie) pair + // verifies for exactly this long. + OAuthDanceStateTTL time.Duration `mapstructure:"oauth_dance_state_ttl"` // OTPResponseFloor is the minimum time the one-time-code request endpoint // takes to answer, whatever it did. Zero falls back to 300ms at wiring time. // @@ -544,6 +614,34 @@ type AppConfig struct { Bootstrap BootstrapConfig `mapstructure:"bootstrap"` } +// OAuthDanceEnabled reports whether the backend-driven OAuth routes should be +// registered. It follows LicenseConfig.Enabled's both-or-neither shape: a +// partially configured block leaves the feature entirely off rather than +// half-armed. +// +// FrontendURL and OAuthCallbackPath are both part of the gate because every +// redirect out of /callback — success and failure alike — is built from the +// two together, so a dance missing either has nowhere to send the browser. +// There is deliberately no default for either path: one value from config and +// another from the binary would mean two places to look when a redirect lands +// somewhere unexpected. +// +// OAuthCookiePath is in the gate for a sharper reason than the other two. An +// empty Path is not inert — net/http omits the attribute and the browser then +// scopes the cookie to the request's own directory, which is the INTERNAL route +// behind the proxy. That is precisely the value that never comes back, so a +// missing key would arm a dance where every sign-in fails as a 302 reading as +// success. Refusing to register the routes surfaces the mistake at startup +// instead. +// +// It NEVER aborts startup: an instance that configures no OAuth dance boots +// exactly as it did before, whatever its frontend_url holds. +func (c AppConfig) OAuthDanceEnabled() bool { + return c.OauthProviders.Google.danceConfigured() && + c.App.FrontendURL != "" && c.App.OAuthCallbackPath != "" && + c.App.OAuthCookiePath != "" +} + // BootstrapConfig configures the break-glass admin sign-in — the emergency // login that breaks the "to configure a provider you must sign in, to sign in // you must configure a provider" loop. From f0366776582d72ed8fc604daa2cc4e1493e62f65 Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Mon, 7 Sep 2026 23:56:13 +0300 Subject: [PATCH 02/11] feat(auth): add the failure reasons and sentinels the dance needs A replayed state is a signal of an attack rather than a routine error, so it gets its own audit reason instead of sharing one with an expired dance. failedLoginDetails covers the case the renderer had no wording for: a refusal that never identified anyone. It used to render as "login failed for " with a dangling preposition and an empty actor. Co-Authored-By: Claude Opus 5 --- internal/apperr/auth.go | 22 ++++- internal/audit/render.go | 17 +++- internal/audit/render_test.go | 31 +++++++ internal/entity/audit.go | 88 +++++++++++++++++-- .../services/auth/login_failure_reason.go | 16 ++++ .../auth/login_failure_reason_test.go | 38 ++++++++ 6 files changed, 205 insertions(+), 7 deletions(-) diff --git a/internal/apperr/auth.go b/internal/apperr/auth.go index ee29810..f5d3190 100644 --- a/internal/apperr/auth.go +++ b/internal/apperr/auth.go @@ -16,7 +16,27 @@ var ( ErrSuspiciousActivity = errors.New("suspicious activity detected") ErrLogoutAlready = errors.New("logout already") ErrUnsupportedProvider = errors.New("unsupported provider") - ErrAuthUnavailable = errors.New("auth unavailable") + // ErrOAuthExchangeFailed marks the provider failing to hand us something we + // can use: a spent or forged code, a PKCE verifier that does not match the + // challenge, a redirect_uri the provider does not recognize, a provider-side + // fault, or an id_token that will not verify. The distinction between those + // is deliberately not carried in the error type — the browser is mid-redirect + // and gets one fixed code, and the detail lives in our logs instead. + ErrOAuthExchangeFailed = errors.New("oauth code exchange failed") + // ErrOAuthProviderDenied marks the provider ending the dance in the redirect + // itself — the user declined consent, or the provider reported an error of + // its own. Distinct from ErrOAuthExchangeFailed, which is the back-channel + // call failing after the browser already came back. + ErrOAuthProviderDenied = errors.New("oauth provider denied") + // ErrOAuthDanceStateInvalid marks a callback that cannot be shown to belong + // to a dance this backend began: no state cookie, a signature that does not + // verify, one that has expired, or a missing PKCE verifier or code. + // + // The causes are deliberately not distinguished. With the state in a cookie + // an abandoned tab and a replayed URL arrive identically, and telling a + // caller which half of its attempt was wrong would confirm half a guess. + ErrOAuthDanceStateInvalid = errors.New("oauth dance state invalid") + ErrAuthUnavailable = errors.New("auth unavailable") // ErrUserBlocked marks a blocked user trying to obtain or use an access // token. Issuance (login/refresh/re-issue) and introspection both reject it, // so blocking a user cuts off both new tokens and live ones on the next diff --git a/internal/audit/render.go b/internal/audit/render.go index adcc9b6..59d24f9 100644 --- a/internal/audit/render.go +++ b/internal/audit/render.go @@ -80,7 +80,7 @@ func fillAuthPayload(payload *entity.ProcessorTaskPayloadAuditWrite, action Acti case LoginFailed: setActor(payload, a.User) payload.EntityID = failedLoginEntityID(a.User) - payload.Details = fmt.Sprintf("login failed for %s", a.User.Email) + payload.Details = failedLoginDetails(a.User) payload.Metadata = sanitizeMetadata(a.Meta) case LogoutSuccess: setActor(payload, a.User) @@ -255,6 +255,21 @@ func fillMaintStepAction( // does retire one assumption worth stating: entity_type "user" no longer implies // entity_id parses as a UUID, and a reader joining it to users.id must filter // those rows out rather than assume. +// failedLoginDetails renders the human-readable half of a failed login. +// +// An attempt that failed before any identity was established has no address to +// name — the OAuth dance's refusals are all like this — and "login failed for " +// with a dangling preposition reads like a truncated record rather than a +// complete one. The metadata carries what such a row is actually found by: IP, +// user agent, failure reason. +func failedLoginDetails(actor *entity.User) string { + if actor.Email == "" { + return "login failed for an unidentified caller" + } + + return fmt.Sprintf("login failed for %s", actor.Email) +} + func failedLoginEntityID(actor *entity.User) string { if actor.ID == uuid.Nil { return actor.Email diff --git a/internal/audit/render_test.go b/internal/audit/render_test.go index 6e72d98..b54c934 100644 --- a/internal/audit/render_test.go +++ b/internal/audit/render_test.go @@ -266,3 +266,34 @@ func TestRender_IdentifiedLoginFailureKeepsTheUserID(t *testing.T) { require.Equal(t, user.ID.String(), payload.EntityID) require.Equal(t, user.Email, payload.Actor) } + +// TestRenderLoginFailedAttributesAnAnonymousAttempt pins what a pre-identity +// failure looks like once rendered. +// +// The publishers in services/auth pass a synthetic &entity.User{} rather than +// nil, because setActor dereferences the actor unconditionally — and it does so +// HERE, in the processor, asynchronously, long after the request that caused it +// returned a tidy 302. A handler test cannot see that: those assert on the +// queued row, which is written before anything renders it. +// +// Asserting the panic on nil was tried and rejected: it holds only while the +// renderer stays nil-hostile, so adding a guard there would make the assertion +// pass while saying nothing. This pins the outcome instead — the zero UUID is +// the documented representation of "failed before the user was known", and a +// test asserting an empty string would be asserting the wrong thing. +func TestRenderLoginFailedAttributesAnAnonymousAttempt(t *testing.T) { + t.Parallel() + + r := fixedRenderer(uuid.New(), time.Now()) + + payload, err := r.Render(LoginFailed{ + User: &entity.User{}, + Meta: &entity.AuditMetadata{IP: "203.0.113.1", FailureReason: entity.AuditFailureSessionMismatch}, + }) + require.NoError(t, err) + + require.Equal(t, uuid.Nil.String(), payload.ActorID) + require.Empty(t, payload.Actor, "an anonymous attempt has no address to attribute") + require.Equal(t, entity.AuditFailureSessionMismatch, payload.Metadata.FailureReason, + "the reason is what makes such a row findable at all") +} diff --git a/internal/entity/audit.go b/internal/entity/audit.go index ee9c7d0..6f0765d 100644 --- a/internal/entity/audit.go +++ b/internal/entity/audit.go @@ -115,6 +115,19 @@ const ( AuditFailureUserProvisioning AuditFailureReason = "user provisioning failed" //nolint:gosec // G101 false positive: a human-readable failure reason, not a credential AuditFailureTokenIssuance AuditFailureReason = "token issuance failed" + // AuditFailureUserBlocked marks a blocked account that got as far as token + // issuance: the identity verified and the user row resolved, and only then + // did IssueTokenPair refuse. + // + // It is deliberately NOT filed under AuditFailureTokenIssuance, which it + // would otherwise share a branch with. That reason means "this deployment + // could not mint a token" — an incident an operator is expected to act on. + // This one means "the system did exactly what it was configured to do", and + // collapsing the two would make a run of ordinary blocked-user sign-ins + // indistinguishable from a failing token service. Same distinction + // AuditFailureSignupDisabled draws one step earlier, and the same one + // apperr's ErrInvalidCredentials doc insists on for refused accounts. + AuditFailureUserBlocked AuditFailureReason = "user blocked" // AuditFailureSignupDisabled marks an OAuth login of an unknown user rejected // because neither an invitation nor open signup authorized creating the account. AuditFailureSignupDisabled AuditFailureReason = "signup disabled" @@ -143,11 +156,29 @@ const ( // AuditFailureAttemptsExhausted marks a guess refused because the code had // already spent its ceiling. The code itself is never compared. AuditFailureAttemptsExhausted AuditFailureReason = "attempts exhausted" - // AuditFailureSessionMismatch marks a correct-shaped attempt whose session - // nonce did not match the one bound to the code. It is a risk signal rather - // than a routine error: the ordinary cause is a user who closed the tab - // while the mail was in flight, but the same event is what a code relayed to - // a third party looks like. + // AuditFailureSessionMismatch marks a correct-shaped attempt that could not + // prove it came from the browser the flow began in. It is a risk signal + // rather than a routine error: the ordinary cause is a user who closed the + // tab mid-flow, but the same event is what a secret relayed to a third party + // looks like. + // + // It serves two flows, and they establish that proof differently. + // + // For one-time codes a nonce travels in the request body and is compared + // against the one bound to the attempt — a per-attempt value, spent once. + // The web client calls this backend server-side, so a cookie would bind that + // server rather than the user's browser. + // + // For the OAuth dance there is no stored value to compare against: /start + // hands the browser an HMAC signature over the state it sent the provider, + // and this reason records that the signature did not verify — absent, + // forged, expired, or for a different state or provider. Note what that does + // NOT mean here: a signature is deterministic, so unlike the nonce it is not + // a one-shot value, and a mismatch says the pair failed to authenticate + // rather than that something was spent twice. + // + // Different mechanisms, one meaning — the request could not prove its + // origin — which is why this is one reason and not two. AuditFailureSessionMismatch AuditFailureReason = "session nonce mismatch" // AuditFailureUnknown covers a rejection whose cause the failing layer could // not name -- in practice an infrastructural error, where the request failed @@ -166,6 +197,53 @@ const ( // AuditFailureCodeExpired marks a code presented after its expiry. //nolint:gosec // G101 false positive: a human-readable failure reason, not a credential AuditFailureCodeExpired AuditFailureReason = "code expired" + + // A dance that dies at the state check is pre-identification in the + // strongest sense in this file: it carries no identity at all, not even a + // claimed address. Those rows are identified by their metadata — IP and user + // agent — and their actor fields are zero. + // + // There is deliberately no "code reused" value for the dance's one-time + // code: AuditFailureInvalidCode above already covers an unknown code and + // losing the race to consume one, which is exactly that case. A second + // symbol would split one documented meaning across two names. + // + // There were once two more reasons here, "oauth state expired" and "oauth + // state reused", which told an abandoned tab from a replay by consulting a + // tombstone the store wrote when it consumed a state. The dance no longer + // stores anything: the state rides in a signed cookie, so a lapsed dance and + // a replayed URL both arrive as a signature that does not verify, and no + // mechanism can separate them. Both now record as + // AuditFailureSessionMismatch. Do not reintroduce the distinction without + // reintroducing something that can actually observe it. + + // AuditFailureProviderUnavailable marks the provider refusing or failing the + // back-channel exchange: the token endpoint rejected the code or the + // client_secret, timed out, or returned an id_token that would not verify. + // + // It is its own reason rather than a reuse of AuditFailureInvalidCredentials, + // which is documented as a password that did not match. The two would be + // indistinguishable in the trail while meaning opposite things: a run of + // "invalid credentials" reads as someone guessing passwords, while a run of + // this one reads as a rotated client_secret or an unreachable Google — a + // different incident with a different runbook. §10 makes the audit trail the + // only signal until RUK-292 adds counters, which is exactly why it must not + // be blurred. + // + // It differs from AuditFailureProviderDenied in who ended the dance: that one + // is the provider telling us up front, in the redirect, that it will not + // proceed; this one is the back-channel call failing after the browser has + // already come back to us. + AuditFailureProviderUnavailable AuditFailureReason = "oauth provider unavailable" + // AuditFailureProviderDenied marks the provider ending the dance: the user + // declined consent, or the provider returned an OAuth error of its own. + // + // It does not belong to the group above and is not filed under its framing: + // in the common case nothing failed and nobody is being attacked — a person + // clicked "cancel". It is recorded because a sudden run of denials usually + // means a broken consent screen or a misconfigured client, which is + // invisible otherwise. + AuditFailureProviderDenied AuditFailureReason = "oauth provider denied" ) type AuditLogoutKind string diff --git a/internal/services/auth/login_failure_reason.go b/internal/services/auth/login_failure_reason.go index ee5bbd6..661b6db 100644 --- a/internal/services/auth/login_failure_reason.go +++ b/internal/services/auth/login_failure_reason.go @@ -17,3 +17,19 @@ func provisioningFailureReason(err error) entity.AuditFailureReason { } return entity.AuditFailureUserProvisioning } + +// issuanceFailureReason is the same distinction one step later. +// +// A blocked user does not fail provisioning — the account resolves fine — it +// fails at IssueTokenPair, which is where ErrUserBlocked is raised. Filing that +// under AuditFailureTokenIssuance would put two unrelated events under one +// reason: "this deployment cannot mint tokens", which is an incident, and "this +// person is blocked", which is the system working as configured. An operator +// reading the trail has to be able to tell those apart, and apperr's own +// doctrine for ErrInvalidCredentials says exactly that about refused accounts. +func issuanceFailureReason(err error) entity.AuditFailureReason { + if errors.Is(err, apperr.ErrUserBlocked) { + return entity.AuditFailureUserBlocked + } + return entity.AuditFailureTokenIssuance +} diff --git a/internal/services/auth/login_failure_reason_test.go b/internal/services/auth/login_failure_reason_test.go index 1679325..3b37ee6 100644 --- a/internal/services/auth/login_failure_reason_test.go +++ b/internal/services/auth/login_failure_reason_test.go @@ -25,3 +25,41 @@ func TestProvisioningFailureReason(t *testing.T) { require.Equal(t, entity.AuditFailureUserProvisioning, provisioningFailureReason(errors.New("db down"))) }) } + +// TestIssuanceFailureReason pins the distinction an operator reads the audit +// trail for. +// +// Both branches end the same way for the user — no token — so nothing in the +// response tells them apart. Collapsing this mapper to a constant leaves every +// handler test green while a run of ordinary blocked-user sign-ins becomes +// indistinguishable from a token service that has stopped working. +func TestIssuanceFailureReason(t *testing.T) { + t.Parallel() + + tests := map[string]struct { + err error + want entity.AuditFailureReason + }{ + "blocked user is not an incident": { + err: apperr.ErrUserBlocked, + want: entity.AuditFailureUserBlocked, + }, + // IssueTokenPair wraps, so the sentinel arrives buried. + "blocked user survives wrapping": { + err: fmt.Errorf("issue access token: %w", apperr.ErrUserBlocked), + want: entity.AuditFailureUserBlocked, + }, + "anything else is a genuine issuance failure": { + err: errors.New("database is down"), + want: entity.AuditFailureTokenIssuance, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + + require.Equal(t, tt.want, issuanceFailureReason(tt.err)) + }) + } +} From 98ac6b2bd3b533e8bc6e211a5c1d6dd859ab90dd Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Mon, 7 Sep 2026 23:56:22 +0300 Subject: [PATCH 03/11] feat(auth): add the Google token-exchange gateway on x/oauth2 Redeems an authorization code at the provider's token endpoint with the client secret and the PKCE verifier, and returns the id_token. Outgoing HTTP goes through xhttp with the shared sanitizer, per project convention. Endpoints come from config with no fallback, so a stand can point the dance at a local fake and an operator can see where sign-ins go. Co-Authored-By: Claude Opus 5 --- go.mod | 1 + go.sum | 2 + internal/gateways/googleoauth/client.go | 85 +++++++++++++++++++++++ internal/gateways/googleoauth/exchange.go | 48 +++++++++++++ 4 files changed, 136 insertions(+) create mode 100644 internal/gateways/googleoauth/client.go create mode 100644 internal/gateways/googleoauth/exchange.go diff --git a/go.mod b/go.mod index a43b8b7..e390ab0 100644 --- a/go.mod +++ b/go.mod @@ -44,6 +44,7 @@ require ( go.uber.org/mock v0.6.0 go.uber.org/zap v1.28.0 golang.org/x/crypto v0.54.0 + golang.org/x/oauth2 v0.36.0 golang.org/x/sync v0.22.0 golang.org/x/text v0.40.0 golang.org/x/time v0.15.0 diff --git a/go.sum b/go.sum index 5e161e4..d6bf771 100644 --- a/go.sum +++ b/go.sum @@ -348,6 +348,8 @@ golang.org/x/net v0.10.0/go.mod h1:0qNGK6F8kojg2nk9dLZ2mShWaEBan6FAoqfSigmmuDg= golang.org/x/net v0.21.0/go.mod h1:bIjVDfnllIU7BJ2DNgfnXvpSvtn8VRwhlsaeUTyUS44= golang.org/x/net v0.57.0 h1:K5+3DljvIuDG9/Jv9rvyMywYNFCQ9RSUY6OOTTkT+tE= golang.org/x/net v0.57.0/go.mod h1:KpXc8iv+r3XplLAG/f7Jsf9RPszJzdR0f58q9vGOuEU= +golang.org/x/oauth2 v0.36.0 h1:peZ/1z27fi9hUOFCAZaHyrpWG5lwe0RJEEEeH0ThlIs= +golang.org/x/oauth2 v0.36.0/go.mod h1:YDBUJMTkDnJS+A4BP4eZBjCqtokkg1hODuPjwiGPO7Q= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20220722155255-886fb9371eb4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.1.0/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= diff --git a/internal/gateways/googleoauth/client.go b/internal/gateways/googleoauth/client.go new file mode 100644 index 0000000..a7578ab --- /dev/null +++ b/internal/gateways/googleoauth/client.go @@ -0,0 +1,85 @@ +// Package googleoauth is the gateway to Google's OAuth token endpoint: one +// exchange per sign-in trades an authorization code for an id_token. +// +// It exists because RUK-291 made this backend a confidential OAuth client. The +// verification half of Google sign-in lives in services/authmethod/googleoauth +// and talks to nobody — it checks an id_token offline against Google's JWKS. +// This package is the other half, and the only one that needs the client secret. +// +// The exchange itself is golang.org/x/oauth2 rather than a hand-rolled POST: +// the grant type, the form encoding, the RFC 6749 error shape and the PKCE +// parameter are all its business, and reimplementing them buys nothing. What +// this package adds is the transport — oauth2 takes its *http.Client from the +// context, so the project's xhttp client goes in there and the exchange inherits +// the same timeout and log-redaction policy as every other outbound call. +package googleoauth + +import ( + "net/http" + "time" + + "golang.org/x/oauth2" + + "github.com/ruko1202/xhttp/client" + + "github.com/ruko1202/maintmode/internal/config" + "github.com/ruko1202/maintmode/internal/utils/xsanitize" +) + +// exchangeTimeout bounds one token-endpoint round trip. +// +// Deliberately far more generous than the license gateway's one-second +// fallback: that client calls our own Console over a short hop, this crosses +// the public internet. A tight ceiling would manufacture failures out of +// ordinary latency, and an aborted exchange costs the user the whole dance. +// +// The other bound is the server's 60s context timeout — without a deadline +// here, an unresponsive provider pins the callback handler for that minute. +const exchangeTimeout = 10 * time.Second + +// Client talks to Google's token endpoint. +type Client struct { + cfg oauth2.Config + httpc *http.Client +} + +// NewClient builds the token-exchange client. The request timeout is the +// gateway's own business, not the caller's — see exchangeTimeout. +func NewClient(cfg config.GoogleOauthProvider) *Client { + return &Client{ + cfg: oauth2.Config{ + ClientID: cfg.ClientID, + ClientSecret: cfg.ClientSecret, + RedirectURL: cfg.RedirectURI, + Endpoint: oauth2.Endpoint{ + // Both come from config, with no in-code default. An endpoint + // baked into the binary is one an operator cannot see when they + // need to know where their sign-ins are going, and it would let + // a stand run against Google while its config says otherwise. + AuthURL: cfg.AuthURL, + TokenURL: cfg.TokenURL, + }, + Scopes: []string{"openid", "email", "profile"}, + }, + httpc: client.NewClient( + client.WithTimeout(exchangeTimeout), + client.WithSanitizer(xsanitize.New()), + ), + } +} + +// AuthCodeURL builds the provider redirect /start sends the browser to. +// +// It lives here rather than in the handler because oauth2.Config already holds +// the client id, the redirect URI and the endpoint: assembling the same URL by +// hand in the API layer meant a second copy of all three, plus the response_type +// and challenge-method literals the library sets itself. +// +// Only the CHALLENGE goes out; the verifier stays in Valkey, which is what stops +// an intercepted authorization code from being redeemable. +func (c *Client) AuthCodeURL(state, verifier string) string { + return c.cfg.AuthCodeURL(state, + oauth2.AccessTypeOnline, + oauth2.S256ChallengeOption(verifier), + ) +} diff --git a/internal/gateways/googleoauth/exchange.go b/internal/gateways/googleoauth/exchange.go new file mode 100644 index 0000000..89d7921 --- /dev/null +++ b/internal/gateways/googleoauth/exchange.go @@ -0,0 +1,48 @@ +package googleoauth + +import ( + "context" + "fmt" + + "github.com/ruko1202/xlog" + "golang.org/x/oauth2" + + "github.com/ruko1202/maintmode/internal/apperr" +) + +// Exchange trades an authorization code for the id_token Google minted with it. +// +// The client secret and the PKCE verifier both travel in the request, which is +// what makes this a confidential-client exchange: possession of the code alone +// is not enough to redeem it. Only the id_token is returned — Google's own +// access token is for calling Google's APIs, which this service never does, so +// it is deliberately never read out of the response. +func (c *Client) Exchange(ctx context.Context, code, codeVerifier string) (string, error) { + ctx, span := xlog.WithOperationSpan(ctx, "gateway.GoogleOAuth.Exchange") + defer span.End() + + // oauth2 reads its HTTP client from the context. Handing it the project's + // xhttp client is what keeps this call under the same timeout and the same + // log-redaction policy as every other outbound request; the library's own + // default would be http.DefaultClient, which has neither. + ctx = context.WithValue(ctx, oauth2.HTTPClient, c.httpc) + + token, err := c.cfg.Exchange(ctx, code, oauth2.VerifierOption(codeVerifier)) + if err != nil { + return "", fmt.Errorf("%w: %w", apperr.ErrOAuthExchangeFailed, err) + } + + // The id_token is an extra field rather than part of oauth2.Token: the + // library models OAuth 2.0, and an id_token is OIDC on top of it. + idToken, _ := token.Extra("id_token").(string) + + // A successful exchange carrying no id_token is not a success with a missing + // field: there is nothing to verify, and returning "" here would push a + // confusing failure into the verifier instead of reporting it where it + // happened. + if idToken == "" { + return "", fmt.Errorf("%w: token response carried no id_token", apperr.ErrOAuthExchangeFailed) + } + + return idToken, nil +} From a4f24afc99c60601e49c4bd663bf2824199eac49 Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Mon, 7 Sep 2026 23:56:32 +0300 Subject: [PATCH 04/11] feat(auth): add the Valkey store behind the one-time dance code The code the callback hands the frontend is redeemed exactly once. GETDEL is what makes that true: two racing exchanges of the same code produce exactly one winner, which a GET followed by a DEL would not. Keys are the SHA-256 of the code, so a Valkey dump never carries a live one. Co-Authored-By: Claude Opus 5 --- internal/storages/oauthdance/consume_code.go | 45 +++++ internal/storages/oauthdance/main_test.go | 33 ++++ internal/storages/oauthdance/put_code.go | 38 ++++ internal/storages/oauthdance/store.go | 51 +++++ internal/storages/oauthdance/store_test.go | 193 +++++++++++++++++++ 5 files changed, 360 insertions(+) create mode 100644 internal/storages/oauthdance/consume_code.go create mode 100644 internal/storages/oauthdance/main_test.go create mode 100644 internal/storages/oauthdance/put_code.go create mode 100644 internal/storages/oauthdance/store.go create mode 100644 internal/storages/oauthdance/store_test.go diff --git a/internal/storages/oauthdance/consume_code.go b/internal/storages/oauthdance/consume_code.go new file mode 100644 index 0000000..3a89f71 --- /dev/null +++ b/internal/storages/oauthdance/consume_code.go @@ -0,0 +1,45 @@ +package oauthdance + +import ( + "context" + "encoding/json" + "errors" + "fmt" + + valkeylib "github.com/redis/go-redis/v9" + "github.com/ruko1202/xlog" + + "github.com/ruko1202/maintmode/internal/entity" +) + +// ConsumeCode redeems a one-time opaque code, returning nil when there is +// nothing to redeem. +// +// A nil pair covers unknown, expired and already-redeemed alike, and the caller +// answers all three with one identical 401: telling them apart would tell an +// attacker which of their guesses was structurally right. The audit trail is +// where the distinction is kept. +// +// GETDEL, not GET-then-DEL. The single round trip is what makes the code truly +// single-use: with two calls, N concurrent redemptions all read the same live +// value before any of them deletes it, and every one of them gets a token pair. +func (s *Store) ConsumeCode(ctx context.Context, code string) (*entity.TokenPair, error) { + ctx, span := xlog.WithOperationSpan(ctx, "store.OAuthDance.ConsumeCode") + defer span.End() + + encoded, err := s.db.GetDel(ctx, codeKey(code)).Result() + if err != nil { + if errors.Is(err, valkeylib.Nil) { + return nil, nil //nolint:nilnil // "no code to redeem" is not an error condition here; see the doc comment. + } + + return nil, fmt.Errorf("consume dance code: %w", err) + } + + pair := new(entity.TokenPair) + if err := json.Unmarshal([]byte(encoded), pair); err != nil { + return nil, fmt.Errorf("unmarshal token pair: %w", err) + } + + return pair, nil +} diff --git a/internal/storages/oauthdance/main_test.go b/internal/storages/oauthdance/main_test.go new file mode 100644 index 0000000..d6639d0 --- /dev/null +++ b/internal/storages/oauthdance/main_test.go @@ -0,0 +1,33 @@ +package oauthdance_test + +import ( + "os" + "testing" + + valkeyDB "github.com/redis/go-redis/v9" + + "github.com/ruko1202/maintmode/internal/config" + "github.com/ruko1202/maintmode/internal/utils/closer" + testconfigutils "github.com/ruko1202/maintmode/test/utils/config" + testdbconnutils "github.com/ruko1202/maintmode/test/utils/db/conn" +) + +// These tests run against a LIVE Valkey, deliberately. The store's single-use +// guarantee rests on GETDEL being one atomic round trip; an in-memory fake would +// happily pass a GET-then-DEL implementation, which is precisely the bug these +// tests exist to catch. +var ( + valkey *valkeyDB.Client + cfg *config.AppConfig +) + +func TestMain(m *testing.M) { + cfg = testconfigutils.LoadAuthConfig() + + valkey = testdbconnutils.NewValkeyClient(cfg) + closer.Add(valkey.Close) + + code := m.Run() + + os.Exit(code) +} diff --git a/internal/storages/oauthdance/put_code.go b/internal/storages/oauthdance/put_code.go new file mode 100644 index 0000000..30ce1f5 --- /dev/null +++ b/internal/storages/oauthdance/put_code.go @@ -0,0 +1,38 @@ +package oauthdance + +import ( + "context" + "encoding/json" + "fmt" + + "github.com/ruko1202/xlog" + + "github.com/ruko1202/maintmode/internal/entity" +) + +// PutCode parks a minted token pair under the hash of the one-time opaque code +// the browser carries to the frontend. +// +// The pair is encoded from the ENTITY, deliberately, and never from the API +// response DTO. entity.TokenPair.SessionID is documented as never appearing in +// the API response, so a DTO round trip would drop it silently — and SessionID +// is what ties a login to its audit record. +func (s *Store) PutCode(ctx context.Context, code string, pair *entity.TokenPair) error { + ctx, span := xlog.WithOperationSpan(ctx, "store.OAuthDance.PutCode") + defer span.End() + + // The pair genuinely is credential material: that is what a one-time code + // redeems. It is held for 60 seconds under a hashed key, which is the whole + // design, so the marshal-a-secret warning has nothing to add here. + //nolint:gosec // G117: storing the token pair IS the purpose of this store + encoded, err := json.Marshal(pair) + if err != nil { + return fmt.Errorf("marshal token pair: %w", err) + } + + if err := s.db.Set(ctx, codeKey(code), encoded, s.codeTTL).Err(); err != nil { + return fmt.Errorf("put dance code: %w", err) + } + + return nil +} diff --git a/internal/storages/oauthdance/store.go b/internal/storages/oauthdance/store.go new file mode 100644 index 0000000..96bf9c7 --- /dev/null +++ b/internal/storages/oauthdance/store.go @@ -0,0 +1,51 @@ +// Package oauthdance stores the one thing a backend-driven OAuth dance cannot +// keep in the browser: the token pair waiting behind a one-time code. +// +// The state and the PKCE verifier are NOT here. They live in cookies, signed +// rather than stored, so no replica needs to know what another one issued — +// see the auth handlers. This entry exists only because the frontend is a +// different origin in production, so a cookie cannot carry the pair across. +// +// Everything here lives in Valkey and nothing in Postgres: the entry is +// worthless a minute after it is written. +// +// Keys are the SHA-256 of the secret, never the secret itself. An operator +// running KEYS, a slow-log entry or a memory dump must not hand anyone a +// replayable credential. +package oauthdance + +import ( + "time" + + valkeylib "github.com/redis/go-redis/v9" + + "github.com/ruko1202/maintmode/internal/utils/xhash" +) + +const codePrefix = "oauth:code:" + +// Store is the Valkey-backed dance store. +// +// The TTL lives on the store rather than traveling per call: it is policy, +// identical for every dance, and a per-call value would be shared mutable state +// on a struct that concurrent requests share. +type Store struct { + db *valkeylib.Client + codeTTL time.Duration +} + +// codeTTL bounds how long a one-time code is redeemable. Short because the code +// is a bearer credential with no second factor: whoever reads it inside the +// window can redeem it. Sixty seconds is the span between the browser receiving +// the redirect and the frontend exchanging it. +const codeTTL = 60 * time.Second + +// NewStore creates an OAuth dance store. +func NewStore(db *valkeylib.Client) *Store { + return &Store{db: db, codeTTL: codeTTL} +} + +// codeKey is the ONE place the hashing scheme lives: both store methods address +// Valkey through it. The code is never a key itself, so a KEYS scan or a memory +// dump yields nothing redeemable. +func codeKey(code string) string { return codePrefix + xhash.HashSha256([]byte(code)) } diff --git a/internal/storages/oauthdance/store_test.go b/internal/storages/oauthdance/store_test.go new file mode 100644 index 0000000..a9b0784 --- /dev/null +++ b/internal/storages/oauthdance/store_test.go @@ -0,0 +1,193 @@ +package oauthdance_test + +import ( + "context" + "sync" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/ruko1202/maintmode/internal/entity" + "github.com/ruko1202/maintmode/internal/storages/oauthdance" + "github.com/ruko1202/maintmode/internal/utils/xhash" + "github.com/ruko1202/maintmode/internal/utils/xuuid" +) + +// newStore builds a store against the live Valkey. Codes are a per-run random +// secret, so parallel runs (make tloc uses -count 2) never collide on a key. +func newStore(t *testing.T) *oauthdance.Store { + t.Helper() + + return oauthdance.NewStore(valkey) +} + +func randomSecret(t *testing.T) string { + t.Helper() + + return xuuid.NewString() +} + +// TestConsumeCodeRoundTripPreservesSessionID guards the serialization choice. +// entity.TokenPair.SessionID is documented as never appearing in the API +// response, so encoding through the response DTO would drop it silently and the +// login audit would lose its session correlation. +func TestConsumeCodeRoundTripPreservesSessionID(t *testing.T) { + t.Parallel() + + ctx := context.Background() + store := newStore(t) + code := randomSecret(t) + + want := &entity.TokenPair{ + AccessToken: "access", + RefreshToken: "refresh", + ExpiresIn: 900, + SessionID: uuid.New(), + } + require.NoError(t, store.PutCode(ctx, code, want)) + + got, err := store.ConsumeCode(ctx, code) + require.NoError(t, err) + require.NotNil(t, got) + + assert.Equal(t, want.AccessToken, got.AccessToken) + assert.Equal(t, want.RefreshToken, got.RefreshToken) + assert.Equal(t, want.ExpiresIn, got.ExpiresIn) + assert.Equal(t, want.SessionID, got.SessionID, "SessionID must survive the round trip") +} + +func TestConsumeCodeSingleUse(t *testing.T) { + t.Parallel() + + ctx := context.Background() + store := newStore(t) + code := randomSecret(t) + + require.NoError(t, store.PutCode(ctx, code, &entity.TokenPair{AccessToken: "a"})) + + got, err := store.ConsumeCode(ctx, code) + require.NoError(t, err) + require.NotNil(t, got) + + got, err = store.ConsumeCode(ctx, code) + require.NoError(t, err) + assert.Nil(t, got, "a redeemed code must never be redeemable again") +} + +// TestConsumeCodeConcurrent is the test a GET-then-DEL implementation fails and +// an in-memory fake would not: N racers, exactly one token pair handed out. +// TestConsumeCodeConcurrent is the test a GET-then-DEL implementation must +// fail, and the reason it is written this way rather than the obvious way. +// +// The obvious version — spawn N goroutines, hope they collide — PASSES against +// a GET-then-DEL store, verified by mutation ten runs out of ten. Goroutines do +// not start together: the first one completes its GET and its DEL before the +// others reach their GET, so the race window never opens and the test proves +// nothing. +// +// Two things fix that. Every racer blocks on a shared channel and is released +// at once, so they enter ConsumeCode inside the same instant. And the whole +// thing repeats: a race is a probability, not a certainty, and one round can +// legitimately serialize on its own. +func TestConsumeCodeConcurrent(t *testing.T) { + t.Parallel() + + ctx := context.Background() + store := newStore(t) + + const ( + racers = 8 + rounds = 20 + ) + + for round := range rounds { + code := randomSecret(t) + require.NoError(t, store.PutCode(ctx, code, &entity.TokenPair{AccessToken: "only-one"})) + + var ( + wg sync.WaitGroup + mu sync.Mutex + winners int + ) + + start := make(chan struct{}) + + wg.Add(racers) + for range racers { + go func() { + defer wg.Done() + + <-start // every racer waits here, so they all enter together + + pair, err := store.ConsumeCode(ctx, code) + assert.NoError(t, err) + + if pair != nil { + mu.Lock() + winners++ + mu.Unlock() + } + }() + } + + close(start) + wg.Wait() + + require.Equal(t, 1, winners, + "exactly one racer may redeem the code (round %d)", round) + } +} + +// TestKeysAreHashed proves the code itself never becomes a Valkey key. An +// operator running KEYS, a slow-log entry or a memory dump must not yield +// anything replayable. +func TestKeysAreHashed(t *testing.T) { + t.Parallel() + + ctx := context.Background() + store := newStore(t) + code := randomSecret(t) + + require.NoError(t, store.PutCode(ctx, code, &entity.TokenPair{AccessToken: "a"})) + + found, err := valkey.Keys(ctx, "*"+code+"*").Result() + require.NoError(t, err) + assert.Empty(t, found, "the raw code must not appear in any key") +} + +// TestCodeExpires proves the stored pair carries an expiry at all. +// +// That is the whole assertion, and it is not a weak one: a key written with no +// TTL lives forever, and an immortal one-time code is the exact opposite of +// what this store is for. Verified by mutation — dropping the TTL from the +// constructor fails only here. +// +// It deliberately does NOT compare against the configured value. Reading the +// same constant the code wrote and asserting they match is a tautology: it +// stays green when the lifetime is stretched from a minute to half an hour, +// which is the change that would actually matter. +func TestCodeExpires(t *testing.T) { + t.Parallel() + + ctx := context.Background() + code := randomSecret(t) + + require.NoError(t, newStore(t).PutCode(ctx, code, &entity.TokenPair{AccessToken: "a"})) + + ttl, err := valkey.TTL(ctx, codeKeyForTest(code)).Result() + require.NoError(t, err) + + assert.Positive(t, ttl, "a one-time code with no expiry is redeemable forever") +} + +// codeKeyForTest rebuilds the store's key so this test can look the entry up. +// +// It duplicates the scheme on purpose rather than exporting it: a helper the +// production code also used would agree with itself by construction, including +// when both are wrong. TestKeysAreHashed is what pins the scheme, by searching +// for the raw code instead of computing anything. +func codeKeyForTest(code string) string { + return "oauth:code:" + xhash.HashSha256([]byte(code)) +} From 720e7a3d4efad14ea5c82fe066e4cb83d404ee04 Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Mon, 7 Sep 2026 23:59:25 +0300 Subject: [PATCH 05/11] refactor(auth): give failed logins one publisher Every sign-in path built its own audit failure event. The dance adds three more callers, and copying the construction a fourth time is how the actor and the reason drift apart between paths. signInWithVerifiedClaims is extracted for the same reason: the dance resolves a user from verified id_token claims exactly as the BFF exchange does, and that resolution is the part worth sharing. Co-Authored-By: Claude Opus 5 --- internal/services/auth/exchange_id_token.go | 63 ++------ internal/services/auth/login_with_otp.go | 21 +-- internal/services/auth/login_with_password.go | 36 ++--- internal/services/auth/service.go | 29 ++++ .../auth/sign_in_with_verified_claims.go | 71 +++++++++ .../auth/sign_in_with_verified_claims_test.go | 138 ++++++++++++++++++ .../authmethod/googleoauth/provider.go | 16 +- internal/services/authmethod/provider.go | 3 + 8 files changed, 278 insertions(+), 99 deletions(-) create mode 100644 internal/services/auth/sign_in_with_verified_claims.go create mode 100644 internal/services/auth/sign_in_with_verified_claims_test.go diff --git a/internal/services/auth/exchange_id_token.go b/internal/services/auth/exchange_id_token.go index fd2f8ee..56c2b43 100644 --- a/internal/services/auth/exchange_id_token.go +++ b/internal/services/auth/exchange_id_token.go @@ -7,8 +7,6 @@ import ( "github.com/ruko1202/xlog" "github.com/ruko1202/xlog/xfield" - "github.com/ruko1202/maintmode/internal/audit" - "github.com/ruko1202/maintmode/internal/entity" ) @@ -20,23 +18,16 @@ func (s *Service) ExchangeIDToken(ctx context.Context, cmd *entity.ExchangeIDTok ctx, span := xlog.WithOperationSpan(ctx, "service.Auth.ExchangeIDToken") defer span.End() - pair, user, err := s.exchangeIDToken(ctx, cmd) + // Both the success and the failure records are published inside + // SignInWithVerifiedClaims, which this path reaches through exchangeIDToken. + // Cases that fail before identification (an invalid token) cannot be tied to + // a user and are logged rather than audited. + pair, _, err := s.exchangeIDToken(ctx, cmd) if err != nil { - // Login-failed audit is recorded inside exchangeIDToken once we - // have a user identity; cases that fail before identification - // (e.g. invalid token) cannot be tied to a user. xlog.Error(ctx, "exchange id token failed", xfield.Error(err)) return nil, err } - s.publishAudit(ctx, audit.LoginSuccess{ - User: user, - Meta: &entity.AuditMetadata{ - IP: cmd.ClientIP, - UserAgent: cmd.UserAgent, - SessionID: pair.SessionID.String(), - }, - }) return pair, nil } @@ -53,44 +44,14 @@ func (s *Service) exchangeIDToken(ctx context.Context, cmd *entity.ExchangeIDTok // TestRoles are filled only by the dev component of the API layer; in prod // the field is always empty, so creation falls back to bootstrap/open-signup. - user, err := s.usersSrv.GetOrCreateByAuthInfo(ctx, cmd.Provider, &entity.OAuthProviderUserInfo{ - ID: claims.Subject, - Email: claims.Email, - Name: claims.Name, - }, entity.UserCreationPolicy{ + // The policy is derived HERE rather than inside SignInWithVerifiedClaims, + // because the other caller of that method — the backend OAuth dance — has no + // X-Test-Roles header to derive it from and must pass the zero value. + return s.SignInWithVerifiedClaims(ctx, cmd.Provider, claims, entity.UserCreationPolicy{ AllowCreate: len(cmd.TestRoles) > 0, GrantRoles: cmd.TestRoles, + }, &entity.AuditMetadata{ + IP: cmd.ClientIP, + UserAgent: cmd.UserAgent, }) - if err != nil { - // login_failed is a security-relevant record. It is published to the - // durable audit outbox: the write survives a crash, at the cost of being - // eventually-consistent rather than persisted before we return. - s.publishAudit(ctx, audit.LoginFailed{ - User: &entity.User{ - Email: claims.Email, - Name: claims.Name, - }, - Meta: &entity.AuditMetadata{ - IP: cmd.ClientIP, - UserAgent: cmd.UserAgent, - FailureReason: provisioningFailureReason(err), - }, - }) - return nil, nil, fmt.Errorf("get or create user: %w", err) - } - - pair, err := s.IssueTokenPair(ctx, user, cmd.ClientIP) - if err != nil { - s.publishAudit(ctx, audit.LoginFailed{ - User: user, - Meta: &entity.AuditMetadata{ - IP: cmd.ClientIP, - UserAgent: cmd.UserAgent, - FailureReason: entity.AuditFailureTokenIssuance, - }, - }) - return nil, nil, fmt.Errorf("issue token pair: %w", err) - } - - return pair, user, nil } diff --git a/internal/services/auth/login_with_otp.go b/internal/services/auth/login_with_otp.go index ee8f7c2..f3e02e8 100644 --- a/internal/services/auth/login_with_otp.go +++ b/internal/services/auth/login_with_otp.go @@ -54,14 +54,9 @@ func (s *Service) LoginWithOTP(ctx context.Context, cmd *entity.VerifyOTPCmd) (* if err != nil { // A blocked user lands here: the guard inside IssueAccessToken refuses // the token even though the code was correct. - s.publishAudit(ctx, audit.LoginFailed{ - User: user, - Meta: &entity.AuditMetadata{ - IP: cmd.ClientIP, - UserAgent: cmd.UserAgent, - FailureReason: entity.AuditFailureTokenIssuance, - }, - }) + s.publishLoginFailure(ctx, user, + &entity.AuditMetadata{IP: cmd.ClientIP, UserAgent: cmd.UserAgent}, + entity.AuditFailureTokenIssuance) return nil, fmt.Errorf("issue token pair: %w", err) } @@ -94,12 +89,6 @@ func (s *Service) publishOTPLoginFailure( user = &entity.User{Email: cmd.Email} } - s.publishAudit(ctx, audit.LoginFailed{ - User: user, - Meta: &entity.AuditMetadata{ - IP: cmd.ClientIP, - UserAgent: cmd.UserAgent, - FailureReason: reason, - }, - }) + s.publishLoginFailure(ctx, user, + &entity.AuditMetadata{IP: cmd.ClientIP, UserAgent: cmd.UserAgent}, reason) } diff --git a/internal/services/auth/login_with_password.go b/internal/services/auth/login_with_password.go index d9df838..cd056bd 100644 --- a/internal/services/auth/login_with_password.go +++ b/internal/services/auth/login_with_password.go @@ -143,14 +143,9 @@ func (s *Service) loginWithStoredPassword( issued, err := s.IssueTokenPair(ctx, user, cmd.ClientIP) if err != nil { - s.publishAudit(ctx, audit.LoginFailed{ - User: user, - Meta: &entity.AuditMetadata{ - IP: cmd.ClientIP, - UserAgent: cmd.UserAgent, - FailureReason: entity.AuditFailureTokenIssuance, - }, - }) + s.publishLoginFailure(ctx, user, + &entity.AuditMetadata{IP: cmd.ClientIP, UserAgent: cmd.UserAgent}, + entity.AuditFailureTokenIssuance) return nil, nil, true, fmt.Errorf("issue token pair: %w", err) } @@ -226,14 +221,9 @@ func (s *Service) loginWithSeed( if err != nil { // A blocked bootstrap admin lands here: the guard inside IssueAccessToken // refuses the token, so blocking cuts off break-glass too. - s.publishAudit(ctx, audit.LoginFailed{ - User: user, - Meta: &entity.AuditMetadata{ - IP: cmd.ClientIP, - UserAgent: cmd.UserAgent, - FailureReason: entity.AuditFailureTokenIssuance, - }, - }) + s.publishLoginFailure(ctx, user, + &entity.AuditMetadata{IP: cmd.ClientIP, UserAgent: cmd.UserAgent}, + entity.AuditFailureTokenIssuance) return nil, nil, fmt.Errorf("issue token pair: %w", err) } @@ -258,21 +248,13 @@ func (s *Service) burnDecoyHash(ctx context.Context, password string) { } // publishPasswordLoginFailure records a failure that happened before a user was -// resolved. The audit renderer dereferences the actor unconditionally, so the -// event carries a synthetic user rather than nil; its zero ID is the documented -// representation of "a login that failed before the user was known". +// resolved. The claimed address is the only attribution such an attempt has. func (s *Service) publishPasswordLoginFailure( ctx context.Context, cmd *entity.LoginWithPasswordCmd, email string, reason entity.AuditFailureReason, ) { - s.publishAudit(ctx, audit.LoginFailed{ - User: &entity.User{Email: email}, - Meta: &entity.AuditMetadata{ - IP: cmd.ClientIP, - UserAgent: cmd.UserAgent, - FailureReason: reason, - }, - }) + s.publishLoginFailure(ctx, &entity.User{Email: email}, + &entity.AuditMetadata{IP: cmd.ClientIP, UserAgent: cmd.UserAgent}, reason) } diff --git a/internal/services/auth/service.go b/internal/services/auth/service.go index 8ad70d8..eb0d4fd 100644 --- a/internal/services/auth/service.go +++ b/internal/services/auth/service.go @@ -93,6 +93,35 @@ func NewService( } } +// publishLoginFailure records a login that failed, with whatever attribution the +// attempt had. +// +// actor must never be nil: the audit renderer dereferences its fields without a +// guard, so a nil one panics the audit processor — asynchronously, long after +// the request that caused it returned cleanly. Pass the resolved user when there +// is one, &entity.User{Email: claimed} when an address was offered but matched +// nothing, and a bare &entity.User{} when the attempt carried no identity at +// all. The zero ID is the documented representation of "failed before the user +// was known", and such rows are found by their metadata instead: IP, user agent, +// failure reason. +func (s *Service) publishLoginFailure( + ctx context.Context, + actor *entity.User, + meta *entity.AuditMetadata, + reason entity.AuditFailureReason, +) { + // Copied rather than written in place: callers reuse one metadata value + // across several branches, and mutating it would leak one branch's reason + // into another's record. + failed := entity.AuditMetadata{FailureReason: reason} + if meta != nil { + failed = *meta + failed.FailureReason = reason + } + + s.publishAudit(ctx, audit.LoginFailed{User: actor, Meta: &failed}) +} + // publishAudit publishes an audited action to the durable outbox. A failed // enqueue is logged, not propagated: the user's auth action must not fail // because the audit publish hiccuped. The durability guarantee is "once diff --git a/internal/services/auth/sign_in_with_verified_claims.go b/internal/services/auth/sign_in_with_verified_claims.go new file mode 100644 index 0000000..7761169 --- /dev/null +++ b/internal/services/auth/sign_in_with_verified_claims.go @@ -0,0 +1,71 @@ +package auth + +import ( + "context" + "fmt" + + "github.com/ruko1202/xlog" + + "github.com/ruko1202/maintmode/internal/audit" + "github.com/ruko1202/maintmode/internal/entity" +) + +// SignInWithVerifiedClaims resolves an already-verified external identity to a +// user and mints a token pair for them. +// +// It takes CLAIMS rather than a token, and that is the whole point of it +// existing. Two paths reach this code: the BFF exchange, which verifies an +// id_token posted by the frontend, and the backend OAuth dance, which verifies +// the id_token it fetched from the provider itself. Passing a raw token here +// would force the dance to verify twice — and duplicating the body instead +// would let one identity resolve to two different users depending on which path +// a person took, which is precisely the bug the two paths running side by side +// would otherwise invite. +// +// It owns the whole audit trail for the sign-in — LoginFailed on both failure +// branches and LoginSuccess at the end — like every other service in this +// codebase. An earlier version left the success record to each caller on the +// theory that the two paths differed; they did not, and the two records were +// identical down to the SessionID. +func (s *Service) SignInWithVerifiedClaims( + ctx context.Context, + provider entity.AuthMethod, + claims *entity.OAuthIDTokenClaims, + policy entity.UserCreationPolicy, + meta *entity.AuditMetadata, +) (*entity.TokenPair, *entity.User, error) { + ctx, span := xlog.WithOperationSpan(ctx, "service.Auth.SignInWithVerifiedClaims") + defer span.End() + + user, err := s.usersSrv.GetOrCreateByAuthInfo(ctx, provider, &entity.OAuthProviderUserInfo{ + ID: claims.Subject, + Email: claims.Email, + Name: claims.Name, + }, policy) + if err != nil { + // login_failed is a security-relevant record. It is published to the + // durable audit outbox: the write survives a crash, at the cost of being + // eventually-consistent rather than persisted before we return. + s.publishLoginFailure(ctx, &entity.User{Email: claims.Email, Name: claims.Name}, + meta, provisioningFailureReason(err)) + + return nil, nil, fmt.Errorf("get or create user: %w", err) + } + + pair, err := s.IssueTokenPair(ctx, user, meta.IP) + if err != nil { + s.publishLoginFailure(ctx, user, meta, issuanceFailureReason(err)) + + return nil, nil, fmt.Errorf("issue token pair: %w", err) + } + + // Published here rather than by the caller: the SessionID that correlates a + // login only exists once the pair is minted, and this is the first point + // where both it and the user are in hand. + success := *meta + success.SessionID = pair.SessionID.String() + + s.publishAudit(ctx, audit.LoginSuccess{User: user, Meta: &success}) + + return pair, user, nil +} diff --git a/internal/services/auth/sign_in_with_verified_claims_test.go b/internal/services/auth/sign_in_with_verified_claims_test.go new file mode 100644 index 0000000..712d997 --- /dev/null +++ b/internal/services/auth/sign_in_with_verified_claims_test.go @@ -0,0 +1,138 @@ +package auth + +import ( + "context" + "testing" + + "github.com/ruko1202/xlog" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + "go.uber.org/zap/zaptest" + + "github.com/ruko1202/maintmode/internal/entity" + "github.com/ruko1202/maintmode/internal/utils/xuuid" +) + +// TestSignInWithVerifiedClaims covers the method the OAuth dance calls directly, +// and which ExchangeIDToken now calls after verifying an id_token. +// +// It exists as an extraction rather than a copy because both paths run side by +// side in production: one identity must resolve to one user regardless of which +// path a person took, and a duplicated provisioning body is how that quietly +// stops being true. +func TestSignInWithVerifiedClaims(t *testing.T) { + t.Parallel() + ctx := xlog.ContextWithLogger(context.Background(), xlog.NewZapAdapter(zaptest.NewLogger(t))) + + t.Run("mints a pair carrying a session id", func(t *testing.T) { + t.Parallel() + + srv, _ := initService(t) + + claims := &entity.OAuthIDTokenClaims{ + Subject: xuuid.NewString(), + Email: xuuid.NewString() + "@example.com", + Name: "Dancer", + } + + pair, user, err := srv.SignInWithVerifiedClaims(ctx, entity.AuthMethodGoogle, claims, + entity.UserCreationPolicy{AllowCreate: true}, + &entity.AuditMetadata{IP: "10.0.0.9", UserAgent: "Mozilla/5.0"}, + ) + require.NoError(t, err) + require.NotNil(t, user) + require.NotEmpty(t, pair.AccessToken) + require.NotEmpty(t, pair.RefreshToken) + + // SessionID ties the login to its audit row and is absent from the API + // response DTO, so a caller serializing the wrong shape loses it + // silently. It must be populated where it is minted. + require.NotEqual(t, entity.TokenPair{}.SessionID, pair.SessionID) + + // The user is returned, not just the pair: the caller needs it to + // publish LoginSuccess, which is why the signature carries three values. + require.Equal(t, claims.Email, user.Email) + }) + + // The property that would break if the dance grew its own provisioning copy. + t.Run("the same subject resolves to the same user", func(t *testing.T) { + t.Parallel() + + srv, _ := initService(t) + + claims := &entity.OAuthIDTokenClaims{ + Subject: xuuid.NewString(), + Email: xuuid.NewString() + "@example.com", + Name: "Repeat", + } + + _, first, err := srv.SignInWithVerifiedClaims(ctx, entity.AuthMethodGoogle, claims, + entity.UserCreationPolicy{AllowCreate: true}, &entity.AuditMetadata{IP: "10.0.0.1"}) + require.NoError(t, err) + + _, second, err := srv.SignInWithVerifiedClaims(ctx, entity.AuthMethodGoogle, claims, + entity.UserCreationPolicy{}, &entity.AuditMetadata{IP: "10.0.0.2"}) + require.NoError(t, err) + + require.Equal(t, first.ID, second.ID, "one identity must resolve to one user across both paths") + }) + + // The creation policy is a parameter precisely so the two callers can differ: + // the BFF path derives AllowCreate from the dev-only X-Test-Roles header, + // while a redirect arriving from Google carries no such header. Hard-coding + // either choice inside the shared method would silently change the other. + // + // This asserts the policy is FORWARDED, not that it is decisive on its own: + // GetOrCreateByAuthInfo resolves in the documented order + // bootstrap > policy.AllowCreate > open signup > refuse, so on a stand with + // no active admin the bootstrap branch creates the user whatever the policy + // says. Asserting a refusal here would be asserting the test database's + // state, not this method's contract. + t.Run("forwards the creation policy rather than inventing one", func(t *testing.T) { + t.Parallel() + + srv, _ := initService(t) + + claims := &entity.OAuthIDTokenClaims{ + Subject: xuuid.NewString(), + Email: xuuid.NewString() + "@example.com", + Name: "Granted", + } + + _, user, err := srv.SignInWithVerifiedClaims(ctx, entity.AuthMethodGoogle, claims, + entity.UserCreationPolicy{AllowCreate: true, GrantRoles: []entity.Role{entity.RoleEditor}}, + &entity.AuditMetadata{IP: "10.0.0.3"}) + require.NoError(t, err) + require.Contains(t, user.Roles, entity.RoleEditor, + "GrantRoles must reach GetOrCreateByAuthInfo; a dropped policy would silently change both callers") + }) +} + +// TestExchangeIDTokenStillDerivesAllowCreateFromTestRoles is the AC7 regression +// guard for the extraction: the old path's dev-only auto-creation must survive +// unchanged, and it is exactly what a hard-coded policy inside the shared method +// would have removed. +func TestExchangeIDTokenStillDerivesAllowCreateFromTestRoles(t *testing.T) { + t.Parallel() + ctx := xlog.ContextWithLogger(context.Background(), xlog.NewZapAdapter(zaptest.NewLogger(t))) + + srv, mocks := initService(t) + + claims := &entity.OAuthIDTokenClaims{ + Subject: xuuid.NewString(), + Email: xuuid.NewString() + "@example.com", + Name: "Tester", + } + mocks.authMethod.EXPECT(). + Authenticate(gomock.Any(), gomock.Any()). + Return(claims, nil) + + pair, err := srv.ExchangeIDToken(ctx, &entity.ExchangeIDTokenCmd{ + Provider: entity.AuthMethodGoogle, + IDToken: "tok", + ClientIP: "10.0.0.1", + TestRoles: []entity.Role{entity.RoleAdmin}, + }) + require.NoError(t, err, "X-Test-Roles must still authorize creating an unknown user") + require.NotEmpty(t, pair.AccessToken) +} diff --git a/internal/services/authmethod/googleoauth/provider.go b/internal/services/authmethod/googleoauth/provider.go index 38768ec..8cbcb96 100644 --- a/internal/services/authmethod/googleoauth/provider.go +++ b/internal/services/authmethod/googleoauth/provider.go @@ -14,11 +14,17 @@ import ( "github.com/ruko1202/maintmode/internal/utils/xtime" ) -// Service verifies Google-issued ID tokens. The authorization-code exchange -// lives in the BFF (maintmode-ui), so this provider never talks to Google's -// token endpoint: no client secret, no redirect URL, no HTTP client for the -// token or userinfo endpoints. The one thing it needs from the OAuth client is -// the client_id, which every ID token must carry as its audience. +// Service verifies Google-issued ID tokens, and only that. It never talks to +// Google's token endpoint: no client secret, no HTTP client for the token or +// userinfo endpoints. The one thing it needs from the OAuth client is the +// client_id, which every ID token must carry as its audience. +// +// It used to be true that nothing in this service talked to that endpoint, +// because the BFF owned the authorization-code exchange. RUK-291 changed that: +// gateways/googleoauth is now the confidential-client half and holds the +// secret. The split is deliberate — verification is offline and stateless, +// exchange is a network call with a credential — and this half stays reachable +// for BOTH login paths, so the audience and issuer checks have one home. // // The verifier state is held flat, mirroring jwtverifier.Service: cfg plus the // keyfunc it feeds, plus the timestamp its refresh-failure callback writes. diff --git a/internal/services/authmethod/provider.go b/internal/services/authmethod/provider.go index 937bdf9..8927eca 100644 --- a/internal/services/authmethod/provider.go +++ b/internal/services/authmethod/provider.go @@ -20,6 +20,9 @@ import ( // owns the OAuth dance with Google and posts us the resulting id_token. The // backend only verifies that token offline against the provider's JWKS, which // is why it needs a client_id (the expected audience) but no client_secret. +// (RUK-291 gave the backend a client_secret for the authorization-code dance, +// but that lives in gateways/googleoauth; verification through this interface +// stays offline and credential-free). // // The credential is typed as a plain string because a password and an emailed // code will sit behind this same interface. The return type is From 2aaf85609cc31b98f4513d8169078988fd3ec1f4 Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Tue, 8 Sep 2026 00:00:08 +0300 Subject: [PATCH 06/11] feat(entity): add the dance types shared across layers DanceStart and DanceCallback cross the API/service boundary, so they live here rather than in either side. DanceProvider is the allow-list: the route takes a path segment, and github, email, stub and bootstrap must never open a dance. Co-Authored-By: Claude Opus 5 --- internal/entity/oauth_dance.go | 69 +++++++++++++++++++++++++++++ internal/entity/oauth_dance_test.go | 47 ++++++++++++++++++++ 2 files changed, 116 insertions(+) create mode 100644 internal/entity/oauth_dance.go create mode 100644 internal/entity/oauth_dance_test.go diff --git a/internal/entity/oauth_dance.go b/internal/entity/oauth_dance.go new file mode 100644 index 0000000..4f4acce --- /dev/null +++ b/internal/entity/oauth_dance.go @@ -0,0 +1,69 @@ +package entity + +import "time" + +// supportedDanceProviders is the allow-list the backend-driven OAuth dance +// accepts, checked before any other work. +// +// A set rather than a map to AuthMethod: the value would only ever be the key +// again. It is narrower than ParseAuthMethod on purpose — that one also accepts +// github and email, which have no authorization-code flow behind them — and it +// is a closed list rather than a registry lookup because the dance route's +// {provider} segment shares a path space with the static +// /login/oauth/exchange/google, so an unvalidated parameter is how a request for +// one route ends up served by another. +var supportedDanceProviders = map[AuthMethod]struct{}{ + AuthMethodGoogle: {}, +} + +// DanceProvider resolves a {provider} path segment to the method that serves it, +// reporting whether the dance supports it at all. +func DanceProvider(segment string) (AuthMethod, bool) { + method, ok := ParseAuthMethod(segment) + if !ok { + return "", false + } + + if _, supported := supportedDanceProviders[method]; !supported { + return "", false + } + + return method, true +} + +// DanceStart is what /start hands the browser. +type DanceStart struct { + // State goes to the provider in the redirect, in the clear. + State string + // StateSignature goes to the browser as a cookie. The browser never sees + // the state and the provider never sees the signature; a callback needs + // both, which is the whole binding. + StateSignature string + // Verifier is the PKCE secret. It never travels to the provider — only its + // S256 challenge does. + Verifier string + // AuthorizationURL is where the browser is sent, built so the client id, + // redirect URI, scopes and challenge method come from one place. + AuthorizationURL string + // TTL is how long the signature stays valid, handed out so the transport can + // match the cookies' MaxAge to it without knowing the number. It is a hint + // to the browser either way: the enforced deadline is inside the signature. + TTL time.Duration +} + +// DanceCallback is what a browser brings back from the provider. +// +// Every field is attacker-controlled: the query values come from the redirect, +// the cookie values from whatever the client chose to send. +type DanceCallback struct { + // Provider is the {provider} path segment, unvalidated. + Provider string + // ProviderError is the provider reporting its own failure, if it did. + ProviderError string + // State and Code arrive in the query; StateSignature and Verifier in the + // cookies /start planted. + State string + Code string + StateSignature string + Verifier string +} diff --git a/internal/entity/oauth_dance_test.go b/internal/entity/oauth_dance_test.go new file mode 100644 index 0000000..6057b22 --- /dev/null +++ b/internal/entity/oauth_dance_test.go @@ -0,0 +1,47 @@ +package entity_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/ruko1202/maintmode/internal/entity" +) + +// TestDanceProvider pins the allow-list, which is narrower than +// ParseAuthMethod's on purpose. +// +// github and email parse as login methods and will one day be real ones, but +// neither has an authorization-code flow behind it today. Accepting them here +// would register a dance the backend cannot finish, and the {provider} segment +// shares a path space with the static /login/oauth/exchange/google — so an +// unvalidated parameter is how a request for one route ends up served by +// another. +func TestDanceProvider(t *testing.T) { + t.Parallel() + + tests := map[string]struct { + segment string + want entity.AuthMethod + ok bool + }{ + "google is the only dance provider today": {segment: "google", want: entity.AuthMethodGoogle, ok: true}, + "github parses but has no dance": {segment: "github"}, + "email parses but has no dance": {segment: "email"}, + "stub is never accepted from a request": {segment: "stub"}, + "bootstrap likewise": {segment: "bootstrap"}, + "unknown": {segment: "definitely-not-a-provider"}, + "empty": {segment: ""}, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + + got, ok := entity.DanceProvider(tt.segment) + + assert.Equal(t, tt.ok, ok) + assert.Equal(t, tt.want, got) + }) + } +} From 00daa83a37ac48df8d2857fd0aba644d2be2ae98 Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Tue, 8 Sep 2026 00:00:23 +0300 Subject: [PATCH 07/11] feat(auth): run the OAuth dance in the auth service, with state in signed cookies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nothing about a dance in flight is stored server-side. The provider is sent the plaintext state and the browser holds only its HMAC signature: whoever observes the redirect URL holds one half and not the other, and that asymmetry is the binding. The verifier never travels — only its S256 challenge does. The deadline is inside the signature rather than in the cookie's MaxAge, because MaxAge is a hint a browser may ignore and an attacker replaying a captured pair with curl honours nothing at all. Ordering in verifyDanceOrigin is load-bearing twice over. The emptiness checks sit AFTER signature verification, or a stranger with curl learns which half of their attempt was wrong. And the explicit empty-signature branch looks redundant against Verify — it is not: falling through would take the audited path and let anyone knocking fill the audit trail. Co-Authored-By: Claude Opus 5 --- internal/services/auth/dance_state_signer.go | 122 +++++++++ .../services/auth/dance_state_signer_test.go | 227 ++++++++++++++++ .../services/auth/dance_state_ttl_test.go | 38 +++ internal/services/auth/oauth_dance.go | 39 +++ internal/services/auth/oauth_dance_state.go | 250 ++++++++++++++++++ internal/services/auth/service.go | 52 ++++ 6 files changed, 728 insertions(+) create mode 100644 internal/services/auth/dance_state_signer.go create mode 100644 internal/services/auth/dance_state_signer_test.go create mode 100644 internal/services/auth/dance_state_ttl_test.go create mode 100644 internal/services/auth/oauth_dance.go create mode 100644 internal/services/auth/oauth_dance_state.go diff --git a/internal/services/auth/dance_state_signer.go b/internal/services/auth/dance_state_signer.go new file mode 100644 index 0000000..b6d5301 --- /dev/null +++ b/internal/services/auth/dance_state_signer.go @@ -0,0 +1,122 @@ +package auth + +import ( + "crypto/hmac" + "crypto/rand" + "crypto/sha256" + "encoding/base64" + "fmt" + "strconv" + "strings" + "time" +) + +const danceSecretBytes = 32 + +// newDanceSecret mints one dance secret: the state, the PKCE verifier or the +// one-time code. +// +// RawURLEncoding, not the padded form xcripto's helper uses: a trailing "=" is +// invalid in an unquoted cookie value, and neither http.SetCookie nor c.Cookie +// encodes it. RFC 7636 requires the same unpadded alphabet for a PKCE verifier, +// so one helper serves all three secrets. +func newDanceSecret() (string, error) { + b := make([]byte, danceSecretBytes) + if _, err := rand.Read(b); err != nil { + return "", fmt.Errorf("generate dance secret: %w", err) + } + + return base64.RawURLEncoding.EncodeToString(b), nil +} + +// danceStateSigner signs and verifies the state the browser carries, so the +// dance needs no server-side store: /start hands out a signature, /callback +// re-derives it from what the provider returned. +// +// It buys authenticity and a deadline, NOT uniqueness. The signature is +// deterministic, so a captured (state, cookie) pair verifies as often as it is +// presented inside the window; single-use comes from the provider burning the +// authorization code instead. +type danceStateSigner struct { + key []byte +} + +// stateSignatureLabel lets a future scheme coexist with signatures already in +// flight rather than colliding with them. +const stateSignatureLabel = "oauth-state-v1" + +// stateSignatureSeparator is safe because neither signed field can contain it: +// the state is base64url and the provider comes from a validated allow-list. +const stateSignatureSeparator = "." + +// newDanceStateSigner derives the signing key from the two secrets, so rotating +// EITHER one ends every dance in flight with no store to purge. +// +// jwtPrivateKey must be config.JWT.PrivateKey as the hex STRING, never the +// parsed key or its D bytes: at exactly 64 characters it sits on HMAC-SHA256's +// block boundary and is zero-padded, while the decoded bytes are padded +// differently. Swapping the form silently invalidates dances in flight, and a +// sign-then-verify test inside one process passes under either — hence the test +// that pins it. +// +// The secrets go through a KDF rather than concatenation: with both operands +// variable-length, ("ab","cdef") and ("abc","def") derive the same key, so a +// client-secret rotation that shifts the boundary could reproduce a +// pre-rotation key. +// +// Operational coupling: rotating the JWT key is already the "sign everyone out" +// lever and now also ends dances in flight, so do not sequence one into the +// middle of an OAuth rollout. +func newDanceStateSigner(jwtPrivateKey, clientSecret string) danceStateSigner { + mac := hmac.New(sha256.New, []byte(jwtPrivateKey)) + mac.Write([]byte(stateSignatureLabel + clientSecret)) + + return danceStateSigner{key: mac.Sum(nil)} +} + +// Sign returns the cookie value "exp.signature", the signature covering the +// provider, the state and that same exp. +// +// The expiry is inside the signed material because cookie MaxAge is an +// instruction the browser may ignore and an attacker ignores by definition. +func (s danceStateSigner) Sign(provider, state string, exp time.Time) string { + unix := strconv.FormatInt(exp.Unix(), 10) + + return unix + stateSignatureSeparator + s.signature(provider, state, unix) +} + +// Verify reports whether cookieValue authenticates this state for this provider +// and has not expired. Every failure is the same false; the causes stay tellable +// apart only in the audit trail. +func (s danceStateSigner) Verify(cookieValue, provider, state string, now time.Time) bool { + // Cut splits on the FIRST separator, so anything appended after an otherwise + // valid cookie lands in the signature half and fails the comparison below + // rather than being quietly ignored. + unix, signature, found := strings.Cut(cookieValue, stateSignatureSeparator) + if !found { + return false + } + + exp, err := strconv.ParseInt(unix, 10, 64) + if err != nil { + return false + } + + // No skew window: one instance both signs and verifies, and any allowance + // here would silently widen the lifetime. + if !now.Before(time.Unix(exp, 0)) { + return false + } + + // Must stay hmac.Equal: == returns the same booleans, so no test can catch + // the swap — only the timing differs. This comment is the guard, as it is in + // bootstrapauth.Authenticate and otp.verify. + return hmac.Equal([]byte(signature), []byte(s.signature(provider, state, unix))) +} + +func (s danceStateSigner) signature(provider, state, unix string) string { + mac := hmac.New(sha256.New, s.key) + mac.Write([]byte(provider + stateSignatureSeparator + state + stateSignatureSeparator + unix)) + + return base64.RawURLEncoding.EncodeToString(mac.Sum(nil)) +} diff --git a/internal/services/auth/dance_state_signer_test.go b/internal/services/auth/dance_state_signer_test.go new file mode 100644 index 0000000..20e7803 --- /dev/null +++ b/internal/services/auth/dance_state_signer_test.go @@ -0,0 +1,227 @@ +package auth + +import ( + "encoding/hex" + "strconv" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The two secrets the signature is seeded from. Both are arbitrary here; what +// matters is that changing either one invalidates a signature made with the +// other, which is the property the rotation tests below pin. +const ( + testJWTKey = "1be2f1f68285c972b750b7718b00d5453f2c08f88c7894d1b9013f75a439de20" + testStateSecret = "dance-client-secret" + testStateValue = "Zm9vYmFyLXN0YXRlLXZhbHVl" + testStateExpFrom = 10 * time.Minute +) + +func testSigner() danceStateSigner { + return newDanceStateSigner(testJWTKey, testStateSecret) +} + +// TestStateSignatureRoundTrips is the happy path: what Sign produced verifies +// for the same provider and state inside the window. +func TestStateSignatureRoundTrips(t *testing.T) { + t.Parallel() + + now := time.Now() + signer := testSigner() + + cookie := signer.Sign("google", testStateValue, now.Add(testStateExpFrom)) + + assert.True(t, signer.Verify(cookie, "google", testStateValue, now)) +} + +// TestStateSignatureIsNotTheState guards the shape of the cookie itself. +// +// A rewrite that "simplifies" the cookie down to the state it signs would still +// pass a round-trip test — Sign and Verify would agree — while handing whoever +// reads the redirect URL everything they need to forge a callback. The cookie +// must not contain the state, in any form. +func TestStateSignatureIsNotTheState(t *testing.T) { + t.Parallel() + + cookie := testSigner().Sign("google", testStateValue, time.Now().Add(testStateExpFrom)) + + assert.NotContains(t, cookie, testStateValue, + "the cookie must carry a signature over the state, never the state itself") +} + +// TestStateSignatureRefusesTamperedInput covers every way the three signed +// fields can fail to match. +func TestStateSignatureRefusesTamperedInput(t *testing.T) { + t.Parallel() + + now := time.Now() + signer := testSigner() + cookie := signer.Sign("google", testStateValue, now.Add(testStateExpFrom)) + + tests := map[string]struct { + provider string + state string + }{ + // RUK-293 and RUK-295 add providers. A dance begun for one must not be + // completable as another, whose id_token this instance would verify + // against different keys. + "another provider": {provider: "github", state: testStateValue}, + "another state": {provider: "google", state: "some-other-state"}, + "empty state": {provider: "google", state: ""}, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + + assert.False(t, signer.Verify(cookie, tt.provider, tt.state, now)) + }) + } +} + +// TestStateSignatureExpires proves the deadline is enforced by the SERVER. +// +// Cookie MaxAge is an instruction to the browser, not a rule anyone enforces: an +// attacker who keeps the cookie value replays it whenever they like. The expiry +// is inside the signed material precisely so this check exists, and a mutation +// dropping exp from the signature has to fail here. +func TestStateSignatureExpires(t *testing.T) { + t.Parallel() + + now := time.Now() + signer := testSigner() + cookie := signer.Sign("google", testStateValue, now.Add(testStateExpFrom)) + + assert.True(t, signer.Verify(cookie, "google", testStateValue, now.Add(testStateExpFrom-time.Second)), + "a signature one second before its expiry is still good") + assert.False(t, signer.Verify(cookie, "google", testStateValue, now.Add(testStateExpFrom+time.Second)), + "a signature past its expiry must be refused, whatever the browser did with MaxAge") +} + +// TestStateSignatureCoversItsOwnExpiry is the test that gives `exp` its reason +// to be inside the signed material, and it is a different claim from the one +// above. +// +// Verify reads exp FROM the cookie and checks the deadline before comparing +// signatures, so the expiry test passes whether or not exp is signed. What only +// this test catches: the cookie is entirely attacker-controlled, so if exp is +// not covered by the signature, anyone holding a captured cookie can rewrite the +// timestamp to any future value and the signature still matches. The 10-minute +// window would then be decorative. +// +// Dropping exp from the signed message must fail here. +func TestStateSignatureCoversItsOwnExpiry(t *testing.T) { + t.Parallel() + + now := time.Now() + signer := testSigner() + cookie := signer.Sign("google", testStateValue, now.Add(testStateExpFrom)) + + _, signature, found := strings.Cut(cookie, ".") + require.True(t, found) + + // The same signature, re-presented with a far-future expiry — exactly what an + // attacker who kept the cookie would send. + extended := strconv.FormatInt(now.Add(365*24*time.Hour).Unix(), 10) + "." + signature + + assert.False(t, signer.Verify(extended, "google", testStateValue, now), + "an attacker-rewritten expiry must break the signature, or the lifetime is unenforced") +} + +// TestStateSignatureDiesWithEitherSecret is the property that justifies mixing +// the client secret into the key at all: rotating EITHER secret invalidates every +// dance in flight at once, with no store to purge. +// +// Without this test the client secret could be dropped from the key derivation +// entirely and every other test here would still pass. +func TestStateSignatureDiesWithEitherSecret(t *testing.T) { + t.Parallel() + + now := time.Now() + exp := now.Add(testStateExpFrom) + cookie := testSigner().Sign("google", testStateValue, exp) + + t.Run("client secret rotated", func(t *testing.T) { + t.Parallel() + + rotated := newDanceStateSigner(testJWTKey, "a-rotated-client-secret") + assert.False(t, rotated.Verify(cookie, "google", testStateValue, now)) + }) + + t.Run("jwt issuer key rotated", func(t *testing.T) { + t.Parallel() + + // Rotating this key is already the "sign everyone out" lever; after this + // ticket it also invalidates dances in flight. + rotated := newDanceStateSigner( + "0000000000000000000000000000000000000000000000000000000000000001", testStateSecret) + assert.False(t, rotated.Verify(cookie, "google", testStateValue, now)) + }) +} + +// TestStateSignatureRefusesMalformedCookies covers what arrives from the wire. +// +// The cookie value is entirely attacker-controlled, so every shape that is not +// "exp.signature" has to be refused rather than parsed optimistically. A +// forgotten error branch here reads as an accept. +func TestStateSignatureRefusesMalformedCookies(t *testing.T) { + t.Parallel() + + now := time.Now() + signer := testSigner() + valid := signer.Sign("google", testStateValue, now.Add(testStateExpFrom)) + signature := valid[strings.Index(valid, ".")+1:] + + tests := map[string]string{ + "empty": "", + "no separator": signature, + "only a separator": ".", + "missing signature": strconv.FormatInt(now.Add(testStateExpFrom).Unix(), 10) + ".", + "missing exp": "." + signature, + "non-numeric exp": "not-a-number." + signature, + "extra separator": valid + ".extra", + "forged signature": strconv.FormatInt(now.Add(testStateExpFrom).Unix(), 10) + ".AAAA", + "whitespace padding": " " + valid, + } + + for name, cookie := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + + assert.False(t, signer.Verify(cookie, "google", testStateValue, now)) + }) + } +} + +// TestStateSignatureSeedIsTheConfiguredString pins the FORM of the JWT seed. +// +// config.JWT.PrivateKey is 64 hex characters — exactly HMAC-SHA256's block size, +// so it is zero-padded rather than hashed, while the parsed key's 32 raw bytes +// are a different input entirely. Both are secure; they are not the same key. A +// later cleanup from one form to the other would silently invalidate every dance +// in flight, and a sign/verify test inside one process passes under either. +// +// This test is what makes that cleanup fail: the hex string and the bytes it +// decodes to must not produce the same signature. +func TestStateSignatureSeedIsTheConfiguredString(t *testing.T) { + t.Parallel() + + now := time.Now() + exp := now.Add(testStateExpFrom) + + fromString := newDanceStateSigner(testJWTKey, testStateSecret). + Sign("google", testStateValue, exp) + + decoded, err := hex.DecodeString(testJWTKey) + require.NoError(t, err) + + fromBytes := newDanceStateSigner(string(decoded), testStateSecret). + Sign("google", testStateValue, exp) + + assert.NotEqual(t, fromString, fromBytes, + "the hex string and its decoded bytes are different keys; the config string is the pinned one") +} diff --git a/internal/services/auth/dance_state_ttl_test.go b/internal/services/auth/dance_state_ttl_test.go new file mode 100644 index 0000000..0e74794 --- /dev/null +++ b/internal/services/auth/dance_state_ttl_test.go @@ -0,0 +1,38 @@ +package auth + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" + + "github.com/ruko1202/maintmode/internal/config" +) + +// TestDanceStateTTL pins the fallback, which is the branch that matters. +// +// Config blocks carry no viper defaults, so an absent or half-filled auth block +// arrives as a bare Go zero — and a zero TTL would sign every state as already +// expired, refusing every callback on a stand nobody thought they had +// misconfigured. Falling back rather than installing that is the whole point of +// having a resolver instead of reading the field directly. +func TestDanceStateTTL(t *testing.T) { + t.Parallel() + + tests := map[string]struct { + configured time.Duration + want time.Duration + }{ + "configured value wins": {configured: 3 * time.Minute, want: 3 * time.Minute}, + "unset falls back": {configured: 0, want: defaultDanceStateTTL}, + "negative falls back too": {configured: -time.Minute, want: defaultDanceStateTTL}, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + + require.Equal(t, tt.want, danceStateTTL(config.Auth{OAuthDanceStateTTL: tt.configured})) + }) + } +} diff --git a/internal/services/auth/oauth_dance.go b/internal/services/auth/oauth_dance.go new file mode 100644 index 0000000..e0b7793 --- /dev/null +++ b/internal/services/auth/oauth_dance.go @@ -0,0 +1,39 @@ +package auth + +import ( + "context" + "fmt" + + "github.com/ruko1202/xlog" + + "github.com/ruko1202/maintmode/internal/entity" +) + +// verifyProviderIDToken verifies an id_token the backend fetched itself. +// +// It routes through the SAME authmethod registry the BFF exchange uses, rather +// than reaching into a provider package directly, for two reasons: the audience +// and issuer checks then have exactly one implementation to drift from, and the +// dev/test `use_stub` substitution keeps applying to both paths at once. A dance +// that bypassed the registry would verify against real Google on a stand where +// every other login is stubbed. +func (s *Service) verifyProviderIDToken( + ctx context.Context, + provider entity.AuthMethod, + idToken string, +) (*entity.OAuthIDTokenClaims, error) { + ctx, span := xlog.WithOperationSpan(ctx, "service.Auth.VerifyProviderIDToken") + defer span.End() + + authMethod, err := s.authMethods.Get(ctx, provider) + if err != nil { + return nil, fmt.Errorf("get oauth provider: %w", err) + } + + claims, err := authMethod.Authenticate(ctx, idToken) + if err != nil { + return nil, fmt.Errorf("verify provider id token: %w", err) + } + + return claims, nil +} diff --git a/internal/services/auth/oauth_dance_state.go b/internal/services/auth/oauth_dance_state.go new file mode 100644 index 0000000..ecd707c --- /dev/null +++ b/internal/services/auth/oauth_dance_state.go @@ -0,0 +1,250 @@ +package auth + +import ( + "context" + "fmt" + "time" + + "github.com/ruko1202/xlog" + "github.com/ruko1202/xlog/xfield" + + "github.com/ruko1202/maintmode/internal/apperr" + "github.com/ruko1202/maintmode/internal/config" + "github.com/ruko1202/maintmode/internal/entity" +) + +// defaultDanceStateTTL is the signed state's lifetime when +// auth.oauth_dance_state_ttl is unset: the span from pressing "sign in" to +// finishing a consent screen, password prompt and second factor included. +const defaultDanceStateTTL = 10 * time.Minute + +// danceStateTTL returns the configured lifetime, or the default when unset. +// +// Config blocks carry no viper defaults, so an absent or half-filled auth block +// arrives as a bare Go zero — and a zero TTL here would sign every state as +// already expired, refusing every callback. Falling back rather than installing +// that is the same fail-open choice otp.TTL documents. +func danceStateTTL(cfg config.Auth) time.Duration { + if cfg.OAuthDanceStateTTL <= 0 { + return defaultDanceStateTTL + } + + return cfg.OAuthDanceStateTTL +} + +// StartDance mints one dance's secrets, signs its state and builds the +// authorization URL. The handler's job is to put the results in cookies and a +// redirect. +func (s *Service) StartDance(ctx context.Context, provider entity.AuthMethod) (*entity.DanceStart, error) { + _, span := xlog.WithOperationSpan(ctx, "service.Auth.StartDance") + defer span.End() + + state, err := newDanceSecret() + if err != nil { + return nil, fmt.Errorf("mint oauth state: %w", err) + } + + verifier, err := newDanceSecret() + if err != nil { + return nil, fmt.Errorf("mint pkce verifier: %w", err) + } + + return &entity.DanceStart{ + State: state, + StateSignature: s.danceSigner.Sign(string(provider), state, time.Now().Add(s.danceStateTTL)), + Verifier: verifier, + AuthorizationURL: s.danceGateway.AuthCodeURL(state, verifier), + TTL: s.danceStateTTL, + }, nil +} + +// RedeemDanceCode trades a one-time opaque code for the pair parked behind it. +// A nil pair with no error means nothing to redeem — unknown, expired or spent, +// deliberately indistinguishable. The consume is atomic, so N concurrent +// redemptions yield one winner. +func (s *Service) RedeemDanceCode(ctx context.Context, code string) (*entity.TokenPair, error) { + ctx, span := xlog.WithOperationSpan(ctx, "service.Auth.RedeemDanceCode") + defer span.End() + + pair, err := s.danceCodes.ConsumeCode(ctx, code) + if err != nil { + return nil, fmt.Errorf("consume one-time dance code: %w", err) + } + + return pair, nil +} + +// CompleteDance turns a callback into a one-time code the frontend can redeem, +// or an error saying why it could not. +// +// The ORDER below is the security property: the provider is resolved before the +// signature is re-derived because it is part of the signed material, and the +// signature is checked before anything reaches the provider so an unverified +// caller cannot spend our outbound requests. +// +// Every refusal audits itself — deciding "this is not a dance we began" and +// deciding "that is worth a row" are one decision. +func (s *Service) CompleteDance( + ctx context.Context, + callback entity.DanceCallback, + meta *entity.AuditMetadata, +) (string, error) { + ctx, span := xlog.WithOperationSpan(ctx, "service.Auth.CompleteDance") + defer span.End() + + // The provider reporting a failure of its own comes first. The user declined + // or the provider stumbled; either way this dance is over and they start a + // fresh one, which is one click on the same login page. + if callback.ProviderError != "" { + xlog.Warn(ctx, "oauth provider reported an error", + xfield.String("provider_error", callback.ProviderError)) + s.publishLoginFailure(ctx, &entity.User{}, meta, entity.AuditFailureProviderDenied) + + return "", fmt.Errorf("%w: %s", apperr.ErrOAuthProviderDenied, callback.ProviderError) + } + + provider, err := s.verifyDanceOrigin(ctx, callback, meta) + if err != nil { + return "", err + } + + idToken, err := s.danceGateway.Exchange(ctx, callback.Code, callback.Verifier) + if err != nil { + xlog.Error(ctx, "oauth code exchange failed", xfield.Error(err)) + s.publishLoginFailure(ctx, &entity.User{}, meta, entity.AuditFailureProviderUnavailable) + + return "", fmt.Errorf("%w: %w", apperr.ErrOAuthExchangeFailed, err) + } + + return s.issueDanceCode(ctx, provider, idToken, meta) +} + +// verifyDanceOrigin decides whether this callback belongs to a dance this +// backend began. +// +// All four refusals answer identically on purpose: with the state in a cookie +// an abandoned tab and a replayed URL are the same event, and telling a caller +// which half was wrong would confirm half a guess. +func (s *Service) verifyDanceOrigin( + ctx context.Context, + callback entity.DanceCallback, + meta *entity.AuditMetadata, +) (entity.AuthMethod, error) { + // Unaudited: a callback naming a provider we do not serve, or carrying no + // state cookie at all, is a stranger knocking. /callback is unauthenticated + // and reachable by anyone, so auditing that would fill the trail with rows + // whose only content is an IP — and bury the rows that mean something. + provider, ok := entity.DanceProvider(callback.Provider) + if !ok { + xlog.Warn(ctx, "oauth dance callback for an unsupported provider", + xfield.String("provider", callback.Provider)) + + return "", fmt.Errorf("%w: %s", apperr.ErrUnsupportedProvider, callback.Provider) + } + + // Verify would reject an empty signature on its own, so this looks + // redundant — it is not. Falling through would take the audited branch + // below, and "no cookie at all" is the one shape any scanner produces for + // free. The check is here to keep it OUT of the trail, not to keep it out + // of Verify. + if callback.StateSignature == "" { + xlog.Warn(ctx, "oauth dance callback arrived with no state cookie") + + return "", apperr.ErrOAuthDanceStateInvalid + } + + // Audited from here on. A signature that is PRESENT and wrong is not a + // stranger: either a real user whose cookie was mangled in transit — the + // failure mode this design added, and one that exits as a 302 that reads as + // success — or someone replaying a captured pair. Both are worth a row, and + // a burst of them with no matching LoginSuccess is the signal that cookie + // delivery has broken. + if !s.danceSigner.Verify(callback.StateSignature, string(provider), callback.State, time.Now()) { + return "", s.refuseDance(ctx, meta, "unverifiable state", apperr.ErrOAuthDanceStateInvalid) + } + + // Past the signature the caller has proved the dance is ours, so detail + // costs nothing — and a request that got this far and is still malformed is + // odd enough to record. + // + // An absent verifier is refused; a merely stale one is NOT — nothing + // enforces its lifetime server-side, so a stale one falls through and the + // provider rejects the exchange, which is a provider failure rather than a + // state one. An absent code is refused here too, saving an outbound request + // guaranteed to fail. + if callback.Verifier == "" { + return "", s.refuseDance(ctx, meta, "no pkce verifier", apperr.ErrOAuthDanceStateInvalid) + } + + if callback.Code == "" { + return "", s.refuseDance(ctx, meta, "no authorization code", apperr.ErrOAuthDanceStateInvalid) + } + + return provider, nil +} + +// refuseDance records one refusal and returns the error to answer it with. +// +// One helper because these refusals ARE one answer: they share a reason, an +// error and a redirect code, and only the log line differs. Keeping them +// separate invited the reader to think the distinctions reached the caller, +// which is exactly what must not happen — telling a caller which half of its +// attempt was wrong confirms half a guess. +// +// The audit row is the only record that anything happened: the browser gets a +// 302, which reads as success in every access log. +func (s *Service) refuseDance( + ctx context.Context, + meta *entity.AuditMetadata, + why string, + err error, +) error { + xlog.Warn(ctx, "oauth dance callback refused", xfield.String("reason", why)) + s.publishLoginFailure(ctx, &entity.User{}, meta, entity.AuditFailureSessionMismatch) + + return err +} + +// issueDanceCode verifies the provider's id_token, resolves the user, mints the +// token pair and parks it behind a one-time code — the code CompleteDance hands +// back for the frontend to redeem later through RedeemDanceCode. +// +// Named for what it produces rather than for the step it sits in: an earlier +// name, redeemDance, read as the opposite of what it does and sat one screen +// above RedeemDanceCode, which genuinely redeems. +func (s *Service) issueDanceCode( + ctx context.Context, + provider entity.AuthMethod, + idToken string, + meta *entity.AuditMetadata, +) (string, error) { + // The SAME verifier the BFF path uses: a second one would be a second place + // for the audience and issuer checks to drift. + claims, err := s.verifyProviderIDToken(ctx, provider, idToken) + if err != nil { + s.publishLoginFailure(ctx, &entity.User{}, meta, entity.AuditFailureProviderUnavailable) + + return "", fmt.Errorf("%w: %w", apperr.ErrOAuthExchangeFailed, err) + } + + // Empty creation policy: AllowCreate comes from the dev-only X-Test-Roles + // header, and a provider redirect carries no header of ours. + pair, _, err := s.SignInWithVerifiedClaims(ctx, provider, claims, entity.UserCreationPolicy{}, meta) + if err != nil { + return "", fmt.Errorf("sign in with verified claims: %w", err) + } + + code, err := newDanceSecret() + if err != nil { + return "", fmt.Errorf("mint one-time dance code: %w", err) + } + + // Minting and storing stay in one function: the code is worthless without + // the entry and the entry unreachable without the code, so a caller able to + // do one without the other could only get it wrong. + if err := s.danceCodes.PutCode(ctx, code, pair); err != nil { + return "", fmt.Errorf("store one-time dance code: %w", err) + } + + return code, nil +} diff --git a/internal/services/auth/service.go b/internal/services/auth/service.go index eb0d4fd..a761e9b 100644 --- a/internal/services/auth/service.go +++ b/internal/services/auth/service.go @@ -3,6 +3,7 @@ package auth import ( "context" "fmt" + "time" "github.com/google/uuid" @@ -63,6 +64,57 @@ type Service struct { otpVerifier OTPVerifier otpRequester OTPRequester passwords PasswordCredentials + // danceSigner and danceCodes are zero until WithDance is called, which only + // happens when the backend-driven OAuth dance is configured. Every dance + // route is behind the same config gate, so neither is ever reached unset. + danceSigner danceStateSigner + danceCodes DanceCodeStore + danceGateway DanceGateway + danceStateTTL time.Duration +} + +// DanceCodeStore is the one piece of Valkey the dance still needs: the token +// pair waiting behind a one-time code. Everything else the dance carries rides +// in signed cookies. +type DanceCodeStore interface { + PutCode(ctx context.Context, code string, pair *entity.TokenPair) error + ConsumeCode(ctx context.Context, code string) (*entity.TokenPair, error) +} + +// DanceGateway is the provider side of the dance: where /start sends the +// browser, and where the callback redeems the code it comes back with. +// +// Both halves are one interface because both are the provider's contract rather +// than ours — the authorization URL carries the same client id, redirect URI and +// endpoint the exchange does, and splitting them would mean assembling that set +// twice. +type DanceGateway interface { + AuthCodeURL(state, verifier string) string + Exchange(ctx context.Context, code, codeVerifier string) (string, error) +} + +// WithDance enables the backend-driven OAuth dance. +// +// It is a separate step rather than more constructor parameters because the +// dance is optional: an instance that configures no client_secret never +// registers its routes, and every existing caller of NewService keeps working +// unchanged. +// +// clientSecret is mixed into the signing key so that rotating EITHER it or the +// JWT issuer key ends every dance in flight — see newDanceStateSigner for why +// the two are combined through a KDF rather than concatenated. +func (s *Service) WithDance( + authCfg config.Auth, + clientSecret string, + codes DanceCodeStore, + gateway DanceGateway, +) *Service { + s.danceSigner = newDanceStateSigner(s.cfg.PrivateKey, clientSecret) + s.danceCodes = codes + s.danceGateway = gateway + s.danceStateTTL = danceStateTTL(authCfg) + + return s } func NewService( From 69803fa4302592872e8d1daf2786de3ec4493007 Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Tue, 8 Sep 2026 00:00:41 +0300 Subject: [PATCH 08/11] fix(auth): sanitize credentials out of request logs The dance puts an authorization code and a one-time code in query strings and request bodies that the access log would otherwise record verbatim. A log line is the wrong place for either: both are redeemable, and logs outlive the seconds they are valid for. Co-Authored-By: Claude Opus 5 --- internal/server/middlewares/base.go | 4 +- .../server/middlewares/request_sanitizer.go | 199 ++++++++++++++ .../middlewares/request_sanitizer_test.go | 246 ++++++++++++++++++ 3 files changed, 447 insertions(+), 2 deletions(-) create mode 100644 internal/server/middlewares/request_sanitizer.go create mode 100644 internal/server/middlewares/request_sanitizer_test.go diff --git a/internal/server/middlewares/base.go b/internal/server/middlewares/base.go index 6132acb..a41db44 100644 --- a/internal/server/middlewares/base.go +++ b/internal/server/middlewares/base.go @@ -32,7 +32,7 @@ func BaseAPIMiddlewares(env config.Environment, meta *buildmeta.AppBuildMeta) [] echootel.NewMiddleware(meta.AppName), middleware.RequestIDWithConfig(middleware.RequestIDConfig{Generator: xuuid.NewString}), TraceMiddleware(), - xhttpserver.RequestLoggingMiddleware(), + xhttpserver.RequestLoggingMiddlewareWithSanitizer(NewRequestSanitizer()), middleware.ContextTimeout(60*time.Second), middleware.GzipWithConfig(middleware.GzipConfig{}), ) @@ -47,7 +47,7 @@ func BaseAPIMiddlewares(env config.Environment, meta *buildmeta.AppBuildMeta) [] ) if !env.IsPerformanceTest() { mw = append(mw, - xhttpserver.BodyDumpLoggingMiddleware(), + xhttpserver.BodyDumpLoggingMiddlewareWithSanitizer(NewRequestSanitizer()), ) } } diff --git a/internal/server/middlewares/request_sanitizer.go b/internal/server/middlewares/request_sanitizer.go new file mode 100644 index 0000000..e1b5111 --- /dev/null +++ b/internal/server/middlewares/request_sanitizer.go @@ -0,0 +1,199 @@ +package middlewares + +import ( + "encoding/json" + "net/url" + "strings" + + "github.com/ruko1202/xhttp/sanitize" + + "github.com/ruko1202/maintmode/internal/utils/xsanitize" +) + +// redacted replaces any value we refuse to write to a log. +const redacted = "[REDACTED]" + +// sensitiveQueryParams are masked in logged request URIs. +// +// This is a blocklist rather than an allow-list because the query namespace of +// THIS service is bounded and known: these four names are the only ones that +// ever carry a credential, and masking every parameter would cost the +// diagnostics that make a 404 on the wrong route distinguishable from a 500 on +// the right one — the exact trade the shared sanitizer's doc comment records +// having already been made once. +var sensitiveQueryParams = map[string]struct{}{ + // The provider's authorization code. Live until redeemed, and redeemable by + // whoever holds it plus our client secret. + "code": {}, + // The dance's CSRF state. Half of the pair that completes a callback: the + // other half is the signature in the browser's cookie, and a log line + // holding the state narrows an attacker's problem to stealing one cookie. + "state": {}, + // A provider id_token, if one ever reaches a query string. + "id_token": {}, + // The PKCE verifier. It should never appear in a URL at all; masking it + // costs nothing and covers a debug redirect that puts it there. + "code_verifier": {}, +} + +// sensitiveBodyFields are masked in logged request and response bodies. +var sensitiveBodyFields = map[string]struct{}{ + "access_token": {}, + "refresh_token": {}, + "id_token": {}, + "code": {}, + "password": {}, + "session_nonce": {}, +} + +var _ sanitize.Sanitizer = RequestSanitizer{} + +// RequestSanitizer is the redaction policy for INBOUND request logging. +// +// It is a wrapper rather than a change to xsanitize, and that follows this +// repository's own documented decision: xsanitize deliberately leaves path and +// query readable, having once masked them and rolled that back as "paying a real +// diagnostic price", and its comment prescribes wrapping for a caller whose +// secret does live in the URL — as the Telegram gateway already does. That +// instance is handed to every outbound client, so widening it to fix an inbound +// problem would regress diagnostics for license, Slack and JWKS. +// +// RUK-291 is the first caller whose inbound URLs carry credentials: an OAuth +// callback arrives as /callback?code=&state=<...>. +// +// Embedding xsanitize.Sanitizer keeps the header policy shared, so a sensitive +// header added there reaches this type too. +type RequestSanitizer struct { + xsanitize.Sanitizer +} + +// NewRequestSanitizer returns the sanitizer for the request-logging middleware. +func NewRequestSanitizer() RequestSanitizer { + return RequestSanitizer{} +} + +// SanitizeURL masks credential-bearing query parameters, leaving the path and +// every other parameter readable. +// +// The embedded implementation is called first so userinfo stripping keeps +// applying; only the query is rewritten here. +func (s RequestSanitizer) SanitizeURL(rawURL string) string { + base := s.Sanitizer.SanitizeURL(rawURL) + if base == redacted || !strings.Contains(base, "?") { + return base + } + + u, err := url.Parse(base) + if err != nil { + // Logging is a side effect and must never be the reason a secret + // escapes, so an unparseable URL yields the marker rather than itself. + return redacted + } + + // url.Query() drops everything after a parse error (a bare semicolon, since + // Go 1.17). Falling through to `base` would then log the original URI + // UNREDACTED, and silently returning the partial query would drop + // parameters from the log without saying so. Neither is acceptable for a + // value that may carry a live authorization code, so an unparseable query + // redacts wholesale. + if _, err := url.ParseQuery(u.RawQuery); err != nil { + u.RawQuery = redacted + + return u.String() + } + + q := u.Query() + masked := false + + for name := range q { + if _, sensitive := sensitiveQueryParams[strings.ToLower(name)]; sensitive { + q.Set(name, redacted) + masked = true + } + } + + if !masked { + return base + } + + u.RawQuery = q.Encode() + + return u.String() +} + +// SanitizeBody masks credential fields in a logged body. +// +// Body logging is dev-only, but the test stand declares environment: dev, so +// without this the dance's own tests would write whole token pairs — access and +// refresh — into the log at Debug. +// +// A body that is not a JSON object is returned unchanged: it cannot be +// inspected field by field, and a blanket redaction would delete the diagnostic +// value that turning body logging on was for. +func (s RequestSanitizer) SanitizeBody(b []byte) []byte { + if len(b) == 0 { + return b + } + + var decoded any + if err := json.Unmarshal(b, &decoded); err != nil { + return b + } + + // Recursive, not top-level only. Today's bodies are flat, so a shallow pass + // would leak nothing — but "flat" is a property of the current endpoints + // rather than of this function, and a wrapped response ({"data":{...}}) is + // exactly the shape someone adds later without thinking about the logger. + masked := maskSensitive(decoded) + if !masked { + return b + } + + out, err := json.Marshal(decoded) + if err != nil { + // Re-encoding failed, so the safe direction is to drop the body rather + // than fall back to the unmasked original. + return []byte(redacted) + } + + return out +} + +// maskSensitive walks a decoded JSON value in place, replacing the values of +// blocked keys wherever they appear. It reports whether anything was masked, so +// an untouched body can be returned verbatim rather than re-encoded — a +// re-encode reorders keys and reformats numbers, which costs log readability for +// nothing. +func maskSensitive(node any) bool { + switch typed := node.(type) { + case map[string]any: + masked := false + + for name, value := range typed { + if _, sensitive := sensitiveBodyFields[strings.ToLower(name)]; sensitive { + typed[name] = redacted + masked = true + + continue + } + + if maskSensitive(value) { + masked = true + } + } + + return masked + case []any: + masked := false + + for _, value := range typed { + if maskSensitive(value) { + masked = true + } + } + + return masked + default: + return false + } +} diff --git a/internal/server/middlewares/request_sanitizer_test.go b/internal/server/middlewares/request_sanitizer_test.go new file mode 100644 index 0000000..3245b74 --- /dev/null +++ b/internal/server/middlewares/request_sanitizer_test.go @@ -0,0 +1,246 @@ +package middlewares_test + +import ( + "net/url" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/ruko1202/maintmode/internal/server/middlewares" +) + +// TestSanitizeURLMasksDanceCredentials covers the leak RUK-291 introduces: an +// OAuth callback arrives with a live authorization code in its query string, and +// the request logger writes the URI verbatim. +func TestSanitizeURLMasksDanceCredentials(t *testing.T) { + t.Parallel() + + s := middlewares.NewRequestSanitizer() + + tests := map[string]struct { + in string + mustNotHave []string + mustHave []string + }{ + // One case per parameter, deliberately. An earlier version asserted the + // code and the state together, and a mutation removing ONLY "code" from + // the blocklist still passed, because the state was masked and the + // combined assertion could not tell which secret had survived. + "oauth callback code": { + in: "/auth/api/v1/login/oauth/google/callback?code=4/0AY0e-live&state=abc123", + mustNotHave: []string{"4/0AY0e-live"}, + // The route must stay readable: masking it wholesale is what the + // shared sanitizer already rolled back once. + mustHave: []string{"/auth/api/v1/login/oauth/google/callback", "code="}, + }, + "oauth callback state": { + in: "/auth/api/v1/login/oauth/google/callback?code=4/0AY0e-live&state=abc123", + mustNotHave: []string{"abc123"}, + mustHave: []string{"state="}, + }, + "pkce verifier": { + in: "/callback?code_verifier=super-secret-verifier", + mustNotHave: []string{"super-secret-verifier"}, + }, + "id token": { + in: "/x?id_token=eyJhbGciOi.payload.sig", + mustNotHave: []string{"eyJhbGciOi.payload.sig"}, + }, + "benign parameters survive untouched": { + in: "/api/v1/users?limit=50&search=alice", + mustHave: []string{"limit=50", "search=alice"}, + }, + "error redirects stay diagnosable": { + in: "/auth/oauth/callback?error=state_reused", + mustHave: []string{"error=state_reused"}, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + + got := s.SanitizeURL(tt.in) + + // Compare against the DECODED form, not the raw string. Encode() + // percent-escapes the output, so a leaked Google code appears as + // "4%2F0AY0e-live" rather than "4/0AY0e-live" — and a NotContains + // on the literal passes while the secret sits in the log. A + // mutation removing "code" from the blocklist caught this test + // doing exactly that. + decoded, err := url.QueryUnescape(got) + require.NoError(t, err) + + for _, secret := range tt.mustNotHave { + assert.NotContains(t, decoded, secret, "a credential reached the log") + } + for _, keep := range tt.mustHave { + assert.Contains(t, got, keep, "diagnostics were masked away") + } + }) + } +} + +// TestSanitizeBodyMasksTokenPairs is the third leak site. Body logging is +// dev-only, but the test stand declares environment: dev, so without this the +// dance's own tests write whole token pairs into the log. +func TestSanitizeBodyMasksTokenPairs(t *testing.T) { + t.Parallel() + + s := middlewares.NewRequestSanitizer() + + t.Run("a token pair response is masked", func(t *testing.T) { + t.Parallel() + + got := string(s.SanitizeBody([]byte(`{"access_token":"live-access","refresh_token":"live-refresh","expires_in":900}`))) + + assert.NotContains(t, got, "live-access") + assert.NotContains(t, got, "live-refresh") + // A non-secret field survives, so the record still says what it was. + assert.Contains(t, got, "900") + }) + + t.Run("a one-time code request is masked", func(t *testing.T) { + t.Parallel() + + got := string(s.SanitizeBody([]byte(`{"code":"one-time-opaque"}`))) + assert.NotContains(t, got, "one-time-opaque") + }) + + // One subtest per field. Asserting several together lets a mutation that + // drops one entry pass, because the others are still masked — the same shape + // of hole the URL test had. password matters most of the three: the + // break-glass sign-in posts it, and body logging is on in dev. + t.Run("every blocked field is masked independently", func(t *testing.T) { + t.Parallel() + + fields := map[string]string{ + "access_token": `{"access_token":"SECRET-VALUE"}`, + "refresh_token": `{"refresh_token":"SECRET-VALUE"}`, + "id_token": `{"id_token":"SECRET-VALUE"}`, + "code": `{"code":"SECRET-VALUE"}`, + "password": `{"password":"SECRET-VALUE"}`, + "session_nonce": `{"session_nonce":"SECRET-VALUE"}`, + } + + for field, body := range fields { + t.Run(field, func(t *testing.T) { + t.Parallel() + + assert.NotContains(t, string(s.SanitizeBody([]byte(body))), "SECRET-VALUE", + "%s reached the log", field) + }) + } + }) + + t.Run("a non-JSON body is left alone", func(t *testing.T) { + t.Parallel() + + const body = "plain text, not a credential" + assert.Equal(t, body, string(s.SanitizeBody([]byte(body)))) + }) + + t.Run("a body with nothing sensitive is untouched", func(t *testing.T) { + t.Parallel() + + const body = `{"name":"alice","limit":10}` + assert.Equal(t, body, string(s.SanitizeBody([]byte(body)))) + }) +} + +// TestSanitizerKeepsTheSharedHeaderPolicy proves the embedding is real: adding a +// sensitive header name to xsanitize must reach this type, which a copied +// blocklist would not do. +func TestSanitizerKeepsTheSharedHeaderPolicy(t *testing.T) { + t.Parallel() + + headers := middlewares.NewRequestSanitizer().SanitizeHeaders(map[string][]string{ + "Authorization": {"Bearer live-token"}, + "Cookie": {"oauth_dance_nonce=live-nonce"}, + "Accept": {"application/json"}, + }) + + require.NotNil(t, headers) + assert.NotContains(t, headers["Authorization"], "Bearer live-token") + assert.NotContains(t, headers["Cookie"], "oauth_dance_nonce=live-nonce") + assert.Equal(t, []string{"application/json"}, headers["Accept"]) +} + +// TestSanitizeURLFailsSafeOnAnUnparseableQuery covers the branch where Go's own +// parser gives up. A bare semicolon has been a parse error since Go 1.17, and +// url.Query() answers by returning what it managed — so a naive implementation +// either logs the original URI unredacted or drops parameters without saying so. +func TestSanitizeURLFailsSafeOnAnUnparseableQuery(t *testing.T) { + t.Parallel() + + s := middlewares.NewRequestSanitizer() + + got := s.SanitizeURL("/auth/api/v1/login/oauth/google/callback?code=LIVE-CODE;b=c") + + assert.NotContains(t, got, "LIVE-CODE", "an unparseable query must never leak its values") + // The route survives: knowing WHICH endpoint was hit is the diagnostic this + // sanitizer exists to preserve. + assert.Contains(t, got, "/auth/api/v1/login/oauth/google/callback") +} + +// TestSanitizeURLRedactsWhateverGoRefusesToParse covers the first of SanitizeURL's +// two fail-safe branches, and records something I got wrong while writing it. +// +// The branches are NOT independent, and a mutation test proves it: removing the +// url.Parse guard entirely leaves every assertion here green, because whatever +// slips past it fails at ParseQuery a few lines later and is redacted there +// instead. So this test pins the OUTCOME — no secret in the output — rather than +// which guard produced it, since no input distinguishes them from the outside. +// +// Worth knowing which input hits which, because it is easy to assume wrongly: a +// broken percent-escape like %zz parses fine as a URL and fails at ParseQuery, +// while a raw control character fails at url.Parse. Both are reachable from the +// wire, and a mistyped callback URL carrying a live code lands on the +// RouteNotFound logger — which is why AC6 says no authorization code appears in +// ANY log line. +func TestSanitizeURLRedactsWhateverGoRefusesToParse(t *testing.T) { + t.Parallel() + + s := middlewares.NewRequestSanitizer() + + for name, uri := range map[string]string{ + // Fails at ParseQuery; url.Parse accepts it. + "broken percent escape": "/login/oauth/google/callback?code=LIVE-CODE&x=%zz", + // Fails at url.Parse itself. + "control character": "/login/oauth/google/callback?code=LIVE-CODE&x=" + string(rune(0x7f)), + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + assert.NotContains(t, s.SanitizeURL(uri), "LIVE-CODE", + "a URI Go will not parse must never reach the log carrying its values") + }) + } +} + +// TestSanitizeBodyRecursesIntoNestedShapes guards a property that is currently +// invisible: today's bodies are flat, so a top-level-only pass would leak +// nothing and every existing test would still pass. A wrapped response is +// exactly what someone adds later without thinking about the logger. +func TestSanitizeBodyRecursesIntoNestedShapes(t *testing.T) { + t.Parallel() + + s := middlewares.NewRequestSanitizer() + + tests := map[string]string{ + "nested object": `{"data":{"access_token":"SECRET-VALUE"}}`, + "array of objects": `[{"code":"SECRET-VALUE"}]`, + "deeply nested": `{"a":{"b":{"c":{"refresh_token":"SECRET-VALUE"}}}}`, + "array inside object": `{"items":[{"password":"SECRET-VALUE"}]}`, + "sibling of a safe key": `{"name":"alice","session":{"id_token":"SECRET-VALUE"}}`, + } + + for name, body := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + + assert.NotContains(t, string(s.SanitizeBody([]byte(body))), "SECRET-VALUE") + }) + } +} From 6b4592219cf19554b8473e7053dfe453440782dc Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Tue, 8 Sep 2026 00:00:41 +0300 Subject: [PATCH 09/11] feat(auth): expose the dance over three endpoints behind the config gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /start redirects to the provider, /callback completes the dance and hands the frontend a one-time code, /code/exchange redeems that code for a token pair. The BFF path at /login/oauth/exchange/google stays live and untouched — the two routes coexist by design. The callback answers with a redirect carrying an error code, never a JSON error: the browser arrives here as a top-level navigation from the provider, and there is a frontend page to send it to. A code rather than the token pair itself crosses the redirect, because the frontend is a separate origin in production and a redirect can only carry things in the URL — where a 720h refresh token would outlive the session in logs, history and Referer headers. Cookie attributes come from the external redirect_uri and the configured cookie path, not from the mounted route: Caddy serves this backend under `handle_path /auth/*` and strips the prefix, so a cookie scoped to what the handler sees is never sent back — and no handler test would notice, because there is no proxy in one. Secure follows that URI's SCHEME rather than the environment name. IsDev() covers dev, local and performance_test, and the deployed dev stand runs environment: dev behind Caddy on https — so an environment-keyed flag shipped the state signature and the PKCE verifier without Secure on a live HTTPS stand. Co-Authored-By: Claude Opus 5 --- cmd/maintmode/main.go | 49 +- internal/app/api/public/auth/app.go | 34 + internal/app/api/public/auth/models/oauth.go | 12 + .../app/api/public/auth/oauth_callback.go | 61 ++ .../api/public/auth/oauth_callback_test.go | 597 ++++++++++++++++++ .../api/public/auth/oauth_code_exchange.go | 64 ++ .../public/auth/oauth_code_exchange_test.go | 155 +++++ .../api/public/auth/oauth_dance_cookies.go | 105 +++ .../api/public/auth/oauth_dance_main_test.go | 192 ++++++ .../api/public/auth/oauth_dance_redirect.go | 110 ++++ internal/app/api/public/auth/oauth_start.go | 67 ++ .../app/api/public/auth/oauth_start_test.go | 258 ++++++++ internal/server/api_server.go | 49 +- 13 files changed, 1741 insertions(+), 12 deletions(-) create mode 100644 internal/app/api/public/auth/models/oauth.go create mode 100644 internal/app/api/public/auth/oauth_callback.go create mode 100644 internal/app/api/public/auth/oauth_callback_test.go create mode 100644 internal/app/api/public/auth/oauth_code_exchange.go create mode 100644 internal/app/api/public/auth/oauth_code_exchange_test.go create mode 100644 internal/app/api/public/auth/oauth_dance_cookies.go create mode 100644 internal/app/api/public/auth/oauth_dance_main_test.go create mode 100644 internal/app/api/public/auth/oauth_dance_redirect.go create mode 100644 internal/app/api/public/auth/oauth_start.go create mode 100644 internal/app/api/public/auth/oauth_start_test.go diff --git a/cmd/maintmode/main.go b/cmd/maintmode/main.go index 4187ec4..ce4b085 100644 --- a/cmd/maintmode/main.go +++ b/cmd/maintmode/main.go @@ -45,8 +45,43 @@ import ( "github.com/ruko1202/maintmode/internal/utils/xecho" "github.com/ruko1202/maintmode/internal/config" + googleoauthgw "github.com/ruko1202/maintmode/internal/gateways/googleoauth" + "github.com/ruko1202/maintmode/internal/storages/oauthdance" ) +// newAuthHandlers builds the auth API component, attaching the backend OAuth +// dance only when it is configured. +// +// The gate is checked here as well as at route registration, and the redundancy +// is deliberate: registration decides whether the endpoints exist, this decides +// whether a token-exchange client holding a client secret is constructed at all. +// An unconfigured instance ends up with neither. +func newAuthHandlers(cfg *config.AppConfig, services *bootstrap.Services, valkeyClient *valkeylib.Client) *apiauth.Implementation { + impl := apiauth.New( + cfg.Auth, + services.Auth, + services.Token, + services.User, + services.OTP, + ) + + if !cfg.OAuthDanceEnabled() { + return impl + } + + // Two halves of arming the dance, and they sit in different layers on + // purpose: the service owns the state signature (it already holds the JWT + // issuer key the signature is seeded from), the handler owns the transport. + services.Auth.WithDance( + cfg.Auth, + cfg.OauthProviders.Google.ClientSecret, + oauthdance.NewStore(valkeyClient), + googleoauthgw.NewClient(cfg.OauthProviders.Google), + ) + + return impl.WithOAuthDance(cfg.OauthProviders.Google, cfg.App) +} + func main() { ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGTERM, @@ -140,13 +175,12 @@ func startAPIServer( Integrations: integrationapi.New(services.Integration, services.UserSummary), UserPicker: userpickerapi.New(services.UserPicker), - Auth: apiauth.New( - cfg.Auth, - services.Auth, - services.Token, - services.User, - services.OTP, - ), + // The dance dependencies attach only when the feature is + // configured. On an unconfigured instance the routes are never + // registered either, so nothing here is ever read — but wiring a + // gateway holding an empty client secret would be a live object + // waiting for a routing mistake. + Auth: newAuthHandlers(cfg, services, valkeyClient), Roles: apiroles.New(services.User), Users: apiusers.New(services.User, services.License), Invitations: apiinvitations.New(services.Invitation), @@ -159,6 +193,7 @@ func startAPIServer( License: services.License, }, valkeyClient, + cfg.OAuthDanceEnabled(), xhttpserver.WithLogger(xecho.NewSlogAdapter(logger)), ) s.BindRouters(cfg.Environment, meta) diff --git a/internal/app/api/public/auth/app.go b/internal/app/api/public/auth/app.go index 7cb21cd..ba21f4c 100644 --- a/internal/app/api/public/auth/app.go +++ b/internal/app/api/public/auth/app.go @@ -16,6 +16,19 @@ type Implementation struct { tokenSrv *token.Service userSrv *user.Service otpSrv *otp.Service + + // Backend-driven OAuth dance. These are zero when the dance is not + // configured, in which case its routes are never registered (see the + // config gate) and none of them is read. + // danceCookiePath is the EXTERNAL cookie scope, taken from config rather + // than from the mounted route — the proxy strips a prefix the handler never + // sees. The config gate requires it, so it is never empty here. + danceCookiePath string + // danceCookieSecure follows the redirect_uri's scheme, not the environment + // name — see the cookie builder for why the environment name was wrong. + danceCookieSecure bool + frontendURL string + frontendCallbackPath string // otpResponseFloor is the minimum time RequestOTP takes to answer. It closes // a timing oracle rather than throttling anything; see acceptedOTPRequest. otpResponseFloor time.Duration @@ -53,3 +66,24 @@ func New( otpResponseFloor: otpResponseFloorFrom(cfg), } } + +// WithOAuthDance attaches the backend-driven dance dependencies. +// +// It is a separate constructor step rather than more parameters on New because +// the dance is optional: an instance that configures no client_secret never +// registers these routes, and every existing caller of New keeps working +// unchanged. +// +// The state signature is NOT wired here: it belongs to the auth service, which +// derives it from the JWT issuer key it already holds — see WithDanceSigner. +func (i *Implementation) WithOAuthDance( + googleCfg config.GoogleOauthProvider, + appCfg config.App, +) *Implementation { + i.danceCookiePath = appCfg.OAuthCookiePath + i.danceCookieSecure = danceCookieSecure(googleCfg.RedirectURI) + i.frontendURL = appCfg.FrontendURL + i.frontendCallbackPath = appCfg.OAuthCallbackPath + + return i +} diff --git a/internal/app/api/public/auth/models/oauth.go b/internal/app/api/public/auth/models/oauth.go new file mode 100644 index 0000000..d4c74c0 --- /dev/null +++ b/internal/app/api/public/auth/models/oauth.go @@ -0,0 +1,12 @@ +package apiauthmodels + +// ExchangeOAuthCodeRequest redeems the one-time code an OAuth dance callback +// placed in the redirect to the frontend. +// +// The code is the only field: an earlier design also carried a nonce, which was +// dropped because it would have traveled in the same redirect URL as the code +// it was meant to protect, and so bought nothing against the one observer that +// binding was for. +type ExchangeOAuthCodeRequest struct { + Code string `json:"code"` +} diff --git a/internal/app/api/public/auth/oauth_callback.go b/internal/app/api/public/auth/oauth_callback.go new file mode 100644 index 0000000..351b20b --- /dev/null +++ b/internal/app/api/public/auth/oauth_callback.go @@ -0,0 +1,61 @@ +package auth + +import ( + "net/url" + + "github.com/labstack/echo/v5" + "github.com/ruko1202/xlog" + "github.com/ruko1202/xlog/xfield" + + "github.com/ruko1202/maintmode/internal/entity" +) + +// OAuthDanceCallback godoc +// @Summary Complete the backend-driven OAuth dance +// @Description Verifies the signed state carried in the oauth_state cookie, exchanges the authorization code for tokens using the client secret and the PKCE verifier from the oauth_code_verifier cookie, resolves the user and redirects to the frontend with a one-time code. Both cookies are cleared on every exit. Always answers 302, success or failure: the user's browser is sitting on this URL, so a JSON error body would be a dead end. +// @Tags Auth +// @Produce json +// @Param provider path string true "Provider id" Enums(google) +// @Param code query string false "Authorization code from the provider" +// @Param state query string false "The state issued by /start" +// @Param error query string false "Error reported by the provider" +// @Success 302 "Redirect to the frontend carrying a one-time code" +// @Failure 429 {object} httperrors.ErrorResponse "Rate limit exceeded" +// @Router /api/v1/login/oauth/{provider}/callback [get] +func (i *Implementation) OAuthDanceCallback(c *echo.Context) error { + ctx, span := xlog.WithOperationSpan(c.Request().Context(), "api.Auth.OAuthDance.Callback") + defer span.End() + + meta := &entity.AuditMetadata{IP: c.RealIP(), UserAgent: c.Request().UserAgent()} + + // STEP 0, before any check: read both cookies and queue their removal. + // + // Everything below works from these two locals. Re-reading the request after + // the expiry has been queued is how a handler ends up with two different + // answers for one cookie; and doing the clearing here rather than at the + // point of success is what makes "cleared on every exit" true by + // construction, instead of in seven branches that each had to remember. + // + // What it buys is bounded: a COOPERATING browser cannot carry a spent dance + // into the next attempt. An attacker replaying with curl never honors + // Set-Cookie, and the signature's deadline is what covers them. + signature := danceCookieValue(c, oauthStateCookie) + verifier := danceCookieValue(c, oauthVerifierCookie) + + i.expireDanceCookies(c) + + code, err := i.authSrv.CompleteDance(ctx, entity.DanceCallback{ + Provider: c.Param("provider"), + ProviderError: c.QueryParam(paramError), + State: c.QueryParam(paramState), + Code: c.QueryParam(paramCode), + StateSignature: signature, + Verifier: verifier, + }, meta) + if err != nil { + xlog.Warn(ctx, "oauth dance did not complete", xfield.Error(err)) + return i.redirectFailure(c, danceFailureCode(err)) + } + + return i.redirectHome(c, url.Values{paramCode: {code}}) +} diff --git a/internal/app/api/public/auth/oauth_callback_test.go b/internal/app/api/public/auth/oauth_callback_test.go new file mode 100644 index 0000000..ecea5a9 --- /dev/null +++ b/internal/app/api/public/auth/oauth_callback_test.go @@ -0,0 +1,597 @@ +package auth + +import ( + "crypto/sha256" + "encoding/base64" + "net/http" + "net/http/httptest" + "net/url" + "strconv" + "strings" + "testing" + "time" + + "github.com/labstack/echo/v5" + "github.com/labstack/echo/v5/echotest" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/ruko1202/maintmode/internal/entity" +) + +// danceRun carries what /start handed the browser, so a callback test can +// present the same state and cookies a real browser would. +type danceRun struct { + state string + signature string + verifier string +} + +// runStart drives /start and collects both halves of the dance, so the callback +// tests exercise the real pairing rather than a hand-built one. +func runStart(t *testing.T, impl *Implementation) danceRun { + t.Helper() + + rec := startRequest(t, impl) + + target, err := url.Parse(rec.Header().Get(echo.HeaderLocation)) + require.NoError(t, err) + + cookies := danceCookies(t, rec) + require.Len(t, cookies, 2) + + return danceRun{ + state: target.Query().Get("state"), + signature: cookies[oauthStateCookie].Value, + verifier: cookies[oauthVerifierCookie].Value, + } +} + +// callbackRequest drives /callback for the google provider. +func callbackRequest(t *testing.T, impl *Implementation, query url.Values, run danceRun) *httptest.ResponseRecorder { + t.Helper() + + return callbackRequestAs(t, impl, string(entity.AuthMethodGoogle), query, run) +} + +// callbackRequestAs drives /callback for an arbitrary {provider} segment. +// +// A danceRun field left empty means the browser sent no such cookie, which is a +// distinct request from sending one with an empty value — see +// callbackWithEmptyStateCookie for that case. +func callbackRequestAs( + t *testing.T, + impl *Implementation, + provider string, + query url.Values, + run danceRun, +) *httptest.ResponseRecorder { + t.Helper() + + cookies := make([]*http.Cookie, 0, 2) + if run.signature != "" { + cookies = append(cookies, &http.Cookie{Name: oauthStateCookie, Value: run.signature}) + } + + if run.verifier != "" { + cookies = append(cookies, &http.Cookie{Name: oauthVerifierCookie, Value: run.verifier}) + } + + return driveCallback(t, impl, provider, query, cookies) +} + +// callbackWithEmptyStateCookie sends the state cookie PRESENT but empty. +// +// It cannot be expressed through danceRun, whose empty field means "send no +// cookie at all" — a different request exercising a different branch of +// danceCookieValue. +func callbackWithEmptyStateCookie( + t *testing.T, + impl *Implementation, + query url.Values, + run danceRun, + empty bool, +) *httptest.ResponseRecorder { + t.Helper() + + if !empty { + return callbackRequest(t, impl, query, run) + } + + return driveCallback(t, impl, string(entity.AuthMethodGoogle), query, []*http.Cookie{ + {Name: oauthStateCookie, Value: ""}, + {Name: oauthVerifierCookie, Value: run.verifier}, + }) +} + +// driveCallback is the one place a callback request is built and run. +func driveCallback( + t *testing.T, + impl *Implementation, + provider string, + query url.Values, + cookies []*http.Cookie, +) *httptest.ResponseRecorder { + t.Helper() + + request := httptest.NewRequest(http.MethodGet, + "/login/oauth/"+provider+"/callback?"+query.Encode(), http.NoBody) + + // A per-request User-Agent, so an audit assertion can find exactly this + // request's rows in a database every other test is also writing to. `make + // tloc` runs the suite twice over one database, so the name alone is not + // unique either. + request.Header.Set("User-Agent", testAgent(t)) + + for _, cookie := range cookies { + request.AddCookie(cookie) + } + + rec := httptest.NewRecorder() + c := echotest.ContextConfig{ + Request: request, + Response: rec, + PathValues: echo.PathValues{{Name: "provider", Value: provider}}, + }.ToContext(t) + + require.NoError(t, impl.OAuthDanceCallback(c)) + + return rec +} + +// redirectResult reads the outcome of a callback redirect: the frontend target +// plus whichever of code/error it carried. +func redirectResult(t *testing.T, rec *httptest.ResponseRecorder) (*url.URL, url.Values) { + t.Helper() + + require.Equal(t, http.StatusFound, rec.Code, "a callback must always redirect, never answer JSON") + + target, err := url.Parse(rec.Header().Get(echo.HeaderLocation)) + require.NoError(t, err) + + // The named local is what gocritic's evalOrder wants here, not a stylistic + // preference: it will not accept target.Query() evaluated inside the return. + query := target.Query() + + return target, query +} + +func TestCallbackHappyPathRedirectsWithAOneTimeCode(t *testing.T) { + impl := initDanceImpl(t) + run := runStart(t, impl) + + rec := callbackRequest(t, impl, url.Values{ + "code": {"provider-auth-code"}, + "state": {run.state}, + }, run) + + target, q := redirectResult(t, rec) + + assert.Equal(t, "frontend.example.com", target.Host, "the target comes only from FrontendURL") + assert.Equal(t, "/auth/oauth/callback", target.Path, "RUK-292 implements this exact route") + assert.NotEmpty(t, q.Get("code")) + assert.Empty(t, q.Get("error")) + + // The opaque code must be redeemable exactly once, and for the pair this + // dance minted. + pair, err := impl.authSrv.RedeemDanceCode(t.Context(), q.Get("code")) + require.NoError(t, err) + require.NotNil(t, pair) + assert.NotEmpty(t, pair.AccessToken) + assert.NotEqual(t, entity.TokenPair{}.SessionID, pair.SessionID) + + // The redirect must carry the opaque code and no token material — a refresh + // token in a query string outlives the browser session in access logs, + // history and Referer, and prod's refresh TTL is 720h. + // + // Asserted against the raw Location, NOT q.Get("access_token"). A named-key + // assertion is blind to the key it is not named after: leaking the pair as + // ?tok= keeps every q.Get() empty and the test green. Verified + // by mutation — that is exactly what happened here. + location := rec.Header().Get(echo.HeaderLocation) + assert.NotContains(t, location, pair.AccessToken, "no access token may ride the redirect") + assert.NotContains(t, location, pair.RefreshToken, "no refresh token may ride the redirect") + assert.NotContains(t, location, pair.SessionID.String(), "not even the session id") +} + +// TestCallbackClearsBothCookiesOnEveryExit covers the invariant step 0 exists +// to create. +// +// Asserting it only on the happy path is what the store-based version did, and +// it left every failure branch free to leak a live cookie into the next attempt. +// Each branch is driven separately here for that reason. +func TestCallbackClearsBothCookiesOnEveryExit(t *testing.T) { + happy := url.Values{"code": {"provider-auth-code"}} + + tests := map[string]struct { + query url.Values + mutate func(*danceRun) + wantErr string + }{ + "success": {query: happy}, + "provider error": {query: url.Values{"error": {"access_denied"}}, wantErr: "access_denied"}, + "no state cookie": {query: happy, mutate: func(r *danceRun) { r.signature = "" }, wantErr: "state_invalid"}, + "forged signature": {query: happy, mutate: func(r *danceRun) { r.signature = "1799999999.forged" }, wantErr: "state_invalid"}, + "no verifier": {query: happy, mutate: func(r *danceRun) { r.verifier = "" }, wantErr: "state_invalid"}, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + impl := initDanceImpl(t) + run := runStart(t, impl) + + query := url.Values{} + for k, v := range tt.query { + query[k] = v + } + if query.Get("error") == "" { + query.Set("state", run.state) + } + + if tt.mutate != nil { + tt.mutate(&run) + } + + rec := callbackRequest(t, impl, query, run) + + _, q := redirectResult(t, rec) + assert.Equal(t, tt.wantErr, q.Get("error")) + + expired := expiredDanceCookies(t, rec) + assert.Contains(t, expired, oauthStateCookie, "the state cookie must not survive this exit") + assert.Contains(t, expired, oauthVerifierCookie, "the verifier cookie must not survive this exit") + + // The expiry has to match the original's scope, or the browser keeps + // the live cookie alongside the tombstone. + assert.Equal(t, "/auth/api/v1/login/oauth", expired[oauthStateCookie].Path, + "an expiry with a different Path leaves the original cookie in place") + }) + } +} + +// TestCallbackRefusesAnUnverifiableState is the ticket's own criterion, in the +// form the cookie design can actually deliver. +// +// The store gave single-use through an atomic consume. A signature cannot: it is +// deterministic, so the same (state, cookie) pair verifies as often as it is +// presented inside the window. What IS guaranteed is that a callback which +// cannot present a matching signature does not complete — which covers every +// attacker who has the redirect URL but not the browser. +func TestCallbackRefusesAnUnverifiableState(t *testing.T) { + valid := url.Values{"code": {"c"}} + + tests := map[string]struct { + query url.Values + // mutate rewrites what the browser presents. omitStateParam is a field + // rather than a check on the case's NAME: the earlier version compared + // name against a literal, so renaming a case silently changed what it + // tested. + mutate func(*danceRun) + omitStateParam bool + // emptyCookie sends the cookie WITH an empty value, which is a different + // request from sending none. Both must be refused, and the earlier + // version could not tell them apart: it set signature = "" for both, and + // the request helper skips a cookie whose value is empty, so the two + // cases were byte-for-byte identical. + emptyCookie bool + }{ + "no cookie at all": {query: valid, mutate: func(r *danceRun) { r.signature = "" }}, + "empty cookie value": {query: valid, emptyCookie: true}, + "forged signature": {query: valid, mutate: func(r *danceRun) { r.signature = "1799999999.AAAA" }}, + "malformed cookie": {query: valid, mutate: func(r *danceRun) { r.signature = "not-a-cookie" }}, + "state from elsewhere": {query: url.Values{"code": {"c"}, "state": {"never-issued"}}}, + "no state in query": {query: valid, omitStateParam: true}, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + impl := initDanceImpl(t) + run := runStart(t, impl) + + query := url.Values{} + for k, v := range tt.query { + query[k] = v + } + if _, pinned := tt.query["state"]; !pinned && !tt.omitStateParam { + query.Set("state", run.state) + } + + if tt.mutate != nil { + tt.mutate(&run) + } + + rec := callbackWithEmptyStateCookie(t, impl, query, run, tt.emptyCookie) + + _, q := redirectResult(t, rec) + + assert.Equal(t, "state_invalid", q.Get("error")) + assert.Empty(t, q.Get("code"), "a refused state must never yield a redeemable code") + }) + } +} + +// TestCallbackStaleVerifierReachesTheProvider pins §6.2 step 3's asymmetry: an +// ABSENT verifier is refused here, a merely stale one is not. +// +// Nothing enforces the verifier's lifetime server-side, so a wrong-but-present +// verifier must fall through to the token endpoint and surface as +// provider_error. Adding a server-side freshness check would look like a +// tightening and would in fact reclassify a provider failure as a state +// failure, which is a different row in the audit trail and a different message +// to the user. Without this test that change passes. +func TestCallbackStaleVerifierReachesTheProvider(t *testing.T) { + gateway := newFakeGateway(t, "", errProviderRefused) + impl := initDanceImplWith(t, testEnv(), gateway) + run := runStart(t, impl) + + // Present, well-formed, and not the one this dance minted. + run.verifier = "a-stale-but-perfectly-shaped-verifier" + + _, q := redirectResult(t, callbackRequest(t, impl, url.Values{ + "code": {"c"}, + "state": {run.state}, + }, run)) + + assert.Equal(t, "provider_error", q.Get("error"), + "a stale verifier is the provider's verdict to give, not ours") + assert.Equal(t, 1, gateway.calls, "the exchange must actually be attempted") +} + +// TestCallbackRefusesAnEmptyAuthorizationCode stops the one branch where +// unvalidated input reaches the network. +// +// A callback with a valid signature but no code used to be handed straight to +// Google's token endpoint, where it could only ever fail. Refusing it here costs +// the attacker their outbound request and costs us nothing. +func TestCallbackRefusesAnEmptyAuthorizationCode(t *testing.T) { + gateway := newFakeGateway(t, "stub-id-token", nil) + impl := initDanceImplWith(t, testEnv(), gateway) + run := runStart(t, impl) + + _, q := redirectResult(t, callbackRequest(t, impl, url.Values{ + "state": {run.state}, + }, run)) + + assert.Equal(t, "state_invalid", q.Get("error")) + assert.Equal(t, 0, gateway.calls, "an empty code must never reach the provider") +} + +// TestCallbackRedirectsAreNotCacheable covers the header that keeps a live +// one-time code out of a shared proxy. +// +// A 302 carrying no cache directives is heuristically cacheable (RFC 9111 +// §4.2.2), and this one carries a redeemable code in its Location. Every other +// credential-bearing response in this package already sets no-store; asserting +// it here is what stops this one drifting back out of line. +func TestCallbackRedirectsAreNotCacheable(t *testing.T) { + impl := initDanceImpl(t) + + t.Run("success carries the code", func(t *testing.T) { + run := runStart(t, impl) + + rec := callbackRequest(t, impl, url.Values{ + "code": {"provider-auth-code"}, + "state": {run.state}, + }, run) + + require.NotEmpty(t, rec.Header().Get(echo.HeaderLocation)) + assert.Equal(t, "no-store", rec.Header().Get(echo.HeaderCacheControl)) + }) + + t.Run("failure, for symmetry", func(t *testing.T) { + run := runStart(t, impl) + run.signature = "" + + rec := callbackRequest(t, impl, url.Values{ + "code": {"c"}, + "state": {run.state}, + }, run) + + assert.Equal(t, "no-store", rec.Header().Get(echo.HeaderCacheControl)) + }) +} + +// TestCallbackIgnoresARedirectTargetFromTheRequest pins a prohibition rather +// than a behavior, which is why nothing else covers it. +// +// SPEC §6.2 is explicit: the redirect target is built ONLY from +// config.FrontendURL, with no return_to parameter "in this ticket or as a hook +// for a later one". Today the code honors that — frontendURLWith never reads +// the request. But a rule held only by a comment is one refactor from being +// broken, and the failure mode is the worst available here: a return_to hook on +// the success path carries the one-time code, which exchanges for a full token +// pair, to an attacker's host. +// +// So this asserts the negative: whatever the request asks for, the browser goes +// to FrontendURL. It is a regression test for a hole that does not exist yet. +func TestCallbackIgnoresARedirectTargetFromTheRequest(t *testing.T) { + const attacker = "https://evil.example" + + t.Run("on success, where the one-time code rides along", func(t *testing.T) { + impl := initDanceImpl(t) + run := runStart(t, impl) + + target, q := redirectResult(t, callbackRequest(t, impl, url.Values{ + "code": {"provider-auth-code"}, + "state": {run.state}, + "return_to": {attacker}, + "redirect": {attacker}, + "next": {attacker}, + }, run)) + + assert.Equal(t, "frontend.example.com", target.Host, + "the one-time code must never leave the configured frontend") + assert.NotEmpty(t, q.Get("code"), "and this must be the success path, or the assertion is vacuous") + }) + + t.Run("on failure", func(t *testing.T) { + impl := initDanceImpl(t) + run := runStart(t, impl) + run.signature = "" + + target, q := redirectResult(t, callbackRequest(t, impl, url.Values{ + "code": {"c"}, + "state": {run.state}, + "return_to": {attacker}, + }, run)) + + assert.Equal(t, "frontend.example.com", target.Host) + assert.NotEmpty(t, q.Get("error"), "and this must be the failure path") + }) +} + +// TestCallbackRefusesADanceFromAnotherBrowser is the binding, stated as the +// property that survives the rewrite: whoever observes the redirect URL holds +// the state but not the httpOnly cookie. +func TestCallbackRefusesADanceFromAnotherBrowser(t *testing.T) { + impl := initDanceImpl(t) + + victim := runStart(t, impl) + attacker := runStart(t, impl) + + // The attacker's own cookies, presented against the victim's state — which + // is all an observer of the redirect URL could ever have. + _, q := redirectResult(t, callbackRequest(t, impl, url.Values{ + "code": {"c"}, + "state": {victim.state}, + }, attacker)) + + assert.Equal(t, "state_invalid", q.Get("error")) + assert.Empty(t, q.Get("code")) +} + +// TestCallbackRefusesARewrittenExpiry is the attack the signed exp exists to +// stop, driven end to end through the handler. +// +// The cookie is entirely attacker-controlled. If exp were merely carried rather +// than signed, anyone holding a captured cookie could extend it indefinitely and +// the 10-minute window would be decorative. +func TestCallbackRefusesARewrittenExpiry(t *testing.T) { + impl := initDanceImpl(t) + run := runStart(t, impl) + + _, signature, found := strings.Cut(run.signature, ".") + require.True(t, found) + + run.signature = strconv.FormatInt(time.Now().Add(365*24*time.Hour).Unix(), 10) + "." + signature + + _, q := redirectResult(t, callbackRequest(t, impl, url.Values{ + "code": {"c"}, + "state": {run.state}, + }, run)) + + assert.Equal(t, "state_invalid", q.Get("error")) +} + +// TestCallbackRejectsAProviderSwap covers the check that gives the provider its +// place in the signed material. +// +// Every other callback test drives google against a google signature, so this +// branch never evaluates true and deleting it leaves the suite green. A dance is +// pinned to one provider at /start precisely so a callback cannot carry it into +// another — a provider whose id_token this instance would verify against +// different keys. +func TestCallbackRejectsAProviderSwap(t *testing.T) { + impl := initDanceImpl(t) + run := runStart(t, impl) + + // A path provider that is NOT the one the dance began with, and one the + // allow-list rejects too: both halves must refuse, and neither may fall + // through to the token exchange. + _, q := redirectResult(t, callbackRequestAs(t, impl, "stub", url.Values{ + "code": {"c"}, + "state": {run.state}, + }, run)) + + assert.Equal(t, "state_invalid", q.Get("error")) + assert.Empty(t, q.Get("code"), "a provider swap must never yield a redeemable code") +} + +func TestCallbackProviderErrors(t *testing.T) { + tests := map[string]struct { + providerErr string + wantError string + }{ + // The provider reporting a declined consent screen. Nothing is broken; + // the user simply starts again. + "user declined": {providerErr: "access_denied", wantError: "access_denied"}, + "provider stumbled": {providerErr: "temporarily_unavailable", wantError: "provider_error"}, + "provider misbehave": {providerErr: "server_error", wantError: "provider_error"}, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + impl := initDanceImpl(t) + run := runStart(t, impl) + + _, q := redirectResult(t, callbackRequest(t, impl, + url.Values{"error": {tt.providerErr}}, run)) + + assert.Equal(t, tt.wantError, q.Get("error")) + assert.Empty(t, q.Get("code"), "a failure must never carry a redeemable code") + }) + } +} + +// TestCallbackProviderExchangeFailure covers the whole downstream leg: a token +// endpoint that refuses must not strand the browser on a JSON error. +func TestCallbackProviderExchangeFailure(t *testing.T) { + impl := initDanceImplWith(t, testEnv(), newFakeGateway(t, "", errProviderRefused)) + run := runStart(t, impl) + + _, q := redirectResult(t, callbackRequest(t, impl, url.Values{ + "code": {"c"}, + "state": {run.state}, + }, run)) + + assert.Equal(t, "provider_error", q.Get("error")) + assert.Empty(t, q.Get("code")) +} + +// TestCallbackPassesTheCookieVerifierToTheProvider proves PKCE is actually +// wired: the verifier minted at /start, which never reaches the provider, must +// be the one presented at the token endpoint. +func TestCallbackPassesTheCookieVerifierToTheProvider(t *testing.T) { + gateway := newFakeGateway(t, "stub-id-token", nil) + impl := initDanceImplWith(t, testEnv(), gateway) + run := runStart(t, impl) + + callbackRequest(t, impl, url.Values{ + "code": {"provider-auth-code"}, + "state": {run.state}, + }, run) + + require.Equal(t, 1, gateway.calls) + assert.Equal(t, "provider-auth-code", gateway.gotCode) + assert.Equal(t, run.verifier, gateway.gotVerf, "the cookie's verifier must reach the token endpoint") + assert.NotEqual(t, run.state, gateway.gotVerf, "the verifier is a separate secret from the state") +} + +// TestStartAdvertisesTheS256TransformOfTheCookieVerifier closes the PKCE +// downgrade hole: asserting the challenge is merely non-empty cannot tell S256 +// from `plain`, and `plain` means the challenge IS the verifier — which defeats +// the point, since whoever intercepts the redirect then holds both halves. +func TestStartAdvertisesTheS256TransformOfTheCookieVerifier(t *testing.T) { + impl := initDanceImpl(t) + + rec := startRequest(t, impl) + + target, err := url.Parse(rec.Header().Get(echo.HeaderLocation)) + require.NoError(t, err) + + challenge := target.Query().Get("code_challenge") + require.NotEmpty(t, challenge) + require.Equal(t, "S256", target.Query().Get("code_challenge_method")) + + verifier := danceCookies(t, rec)[oauthVerifierCookie] + require.NotNil(t, verifier) + + // Computed here from the RFC rather than by calling the implementation's own + // helper: a test that reuses the function under test agrees with it by + // construction, including when both are wrong. + sum := sha256.Sum256([]byte(verifier.Value)) + assert.Equal(t, base64.RawURLEncoding.EncodeToString(sum[:]), challenge, + "the advertised challenge must be the S256 transform of the cookie's verifier") + assert.NotEqual(t, verifier.Value, challenge, + "a challenge equal to the verifier is the `plain` method, which PKCE exists to avoid") +} diff --git a/internal/app/api/public/auth/oauth_code_exchange.go b/internal/app/api/public/auth/oauth_code_exchange.go new file mode 100644 index 0000000..9eb451b --- /dev/null +++ b/internal/app/api/public/auth/oauth_code_exchange.go @@ -0,0 +1,64 @@ +package auth + +import ( + "net/http" + + "github.com/labstack/echo/v5" + "github.com/ruko1202/xlog" + "github.com/ruko1202/xlog/xfield" + + "github.com/ruko1202/maintmode/internal/app/api/httperrors" + apiauthmodels "github.com/ruko1202/maintmode/internal/app/api/public/auth/models" + "github.com/ruko1202/maintmode/internal/apperr" +) + +// ExchangeOAuthDanceCode godoc +// @Summary Redeem the one-time code from an OAuth dance +// @Description Trades the short-lived opaque code the callback put in the redirect for the token pair it stands for. Single-use: the second attempt with the same code fails like any other. Every failure — unknown, expired, already redeemed, malformed — answers the same 401, so a caller cannot learn which of its guesses was closer. +// @Tags Auth +// @Accept json +// @Produce json +// @Param request body apiauthmodels.ExchangeOAuthCodeRequest true "The one-time code" +// @Success 200 {object} apiauthmodels.TokenPairResponse +// @Failure 401 {object} httperrors.ErrorResponse "The code is not redeemable" +// @Failure 429 {object} httperrors.ErrorResponse "Rate limit exceeded" +// @Router /api/v1/login/oauth/code/exchange [post] +func (i *Implementation) ExchangeOAuthDanceCode(c *echo.Context) error { + ctx, span := xlog.WithOperationSpan(c.Request().Context(), "api.Auth.OAuthDance.ExchangeCode") + defer span.End() + op := "oauth dance code exchange" + + body := new(apiauthmodels.ExchangeOAuthCodeRequest) + if err := c.Bind(body); err != nil { + // A malformed body answers exactly as a wrong code does. Distinguishing + // them would tell a prober that its JSON was at least well-formed, which + // is the first bit of a guess. + xlog.Warn(ctx, "failed to bind oauth code exchange request", xfield.Error(err)) + + return httperrors.ToAPIError(c, op, apperr.ErrInvalidAccessToken) + } + + if body.Code == "" { + return httperrors.ToAPIError(c, op, apperr.ErrInvalidAccessToken) + } + + pair, err := i.authSrv.RedeemDanceCode(ctx, body.Code) + if err != nil { + xlog.Error(ctx, "failed to consume the one-time dance code", xfield.Error(err)) + + // Even a store fault answers 401 rather than 500: the uniform response + // is the property, and a 500 here would mark the one code whose lookup + // misbehaved. + return httperrors.ToAPIError(c, op, apperr.ErrInvalidAccessToken) + } + + // Unknown, expired and already-redeemed are one case by design. The audit + // trail is where they stay tellable apart. + if pair == nil { + return httperrors.ToAPIError(c, op, apperr.ErrInvalidAccessToken) + } + + c.Response().Header().Set(echo.HeaderCacheControl, "no-store") + + return c.JSON(http.StatusOK, apiauthmodels.ToAPITokenPairResponse(pair)) +} diff --git a/internal/app/api/public/auth/oauth_code_exchange_test.go b/internal/app/api/public/auth/oauth_code_exchange_test.go new file mode 100644 index 0000000..5ce701d --- /dev/null +++ b/internal/app/api/public/auth/oauth_code_exchange_test.go @@ -0,0 +1,155 @@ +package auth + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "sync" + "testing" + + "github.com/labstack/echo/v5" + "github.com/labstack/echo/v5/echotest" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + apiauthmodels "github.com/ruko1202/maintmode/internal/app/api/public/auth/models" +) + +// exchangeCode drives POST /login/oauth/code/exchange with the given body. +func exchangeCode(t *testing.T, impl *Implementation, body string) *httptest.ResponseRecorder { + t.Helper() + + request := httptest.NewRequest(http.MethodPost, "/login/oauth/code/exchange", strings.NewReader(body)) + request.Header.Set(echo.HeaderContentType, echo.MIMEApplicationJSON) + + rec := httptest.NewRecorder() + c := echotest.ContextConfig{Request: request, Response: rec}.ToContext(t) + + _ = impl.ExchangeOAuthDanceCode(c) + + return rec +} + +// danceToCode runs a full dance and returns the one-time code the browser would +// carry to the frontend. +func danceToCode(t *testing.T, impl *Implementation) string { + t.Helper() + + run := runStart(t, impl) + rec := callbackRequest(t, impl, url.Values{"code": {"c"}, "state": {run.state}}, run) + + _, q := redirectResult(t, rec) + code := q.Get("code") + require.NotEmpty(t, code) + + return code +} + +func TestExchangeCodeReturnsThePair(t *testing.T) { + impl := initDanceImpl(t) + code := danceToCode(t, impl) + + rec := exchangeCode(t, impl, `{"code":"`+code+`"}`) + require.Equal(t, http.StatusOK, rec.Code) + + var pair apiauthmodels.TokenPairResponse + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &pair)) + assert.NotEmpty(t, pair.AccessToken) + assert.NotEmpty(t, pair.RefreshToken) + + // The pair is credential material: a cache would put it in a shared proxy. + assert.Equal(t, "no-store", rec.Header().Get(echo.HeaderCacheControl)) +} + +// TestExchangeCodeIsSingleUse is the ticket's second named acceptance criterion. +func TestExchangeCodeIsSingleUse(t *testing.T) { + impl := initDanceImpl(t) + code := danceToCode(t, impl) + + require.Equal(t, http.StatusOK, exchangeCode(t, impl, `{"code":"`+code+`"}`).Code) + + second := exchangeCode(t, impl, `{"code":"`+code+`"}`) + assert.Equal(t, http.StatusUnauthorized, second.Code, "a redeemed code must never be redeemable again") + assert.NotContains(t, second.Body.String(), "access_token") +} + +// TestExchangeCodeConcurrentRedemption is what a GET-then-DEL store would fail: +// N callers race for one code and exactly one may leave with a token pair. +// TestExchangeCodeConcurrentRedemption drives the race through the HANDLER. +// +// Written with a release barrier and repeated rounds for the reason the store's +// own concurrency test spells out: goroutines spawned in a loop do not collide +// on their own, and the naive version passes against a store with a genuine +// GET-then-DEL race. Verified by mutation. +func TestExchangeCodeConcurrentRedemption(t *testing.T) { + impl := initDanceImpl(t) + + const ( + racers = 8 + rounds = 10 + ) + + for round := range rounds { + code := danceToCode(t, impl) + + var ( + wg sync.WaitGroup + mu sync.Mutex + granted int + ) + + start := make(chan struct{}) + + wg.Add(racers) + for range racers { + go func() { + defer wg.Done() + + <-start + + if exchangeCode(t, impl, `{"code":"`+code+`"}`).Code == http.StatusOK { + mu.Lock() + granted++ + mu.Unlock() + } + }() + } + + close(start) + wg.Wait() + + require.Equal(t, 1, granted, + "exactly one racer may redeem a one-time code (round %d)", round) + } +} + +// TestExchangeCodeFailuresAreIndistinguishable pins the uniform answer. An +// attacker probing codes must not learn which of their guesses was structurally +// closer; the audit trail is where the cases stay tellable apart. +func TestExchangeCodeFailuresAreIndistinguishable(t *testing.T) { + impl := initDanceImpl(t) + + spent := danceToCode(t, impl) + require.Equal(t, http.StatusOK, exchangeCode(t, impl, `{"code":"`+spent+`"}`).Code) + + bodies := map[string]string{ + "unknown code": `{"code":"never-issued-at-all"}`, + "already spent": `{"code":"` + spent + `"}`, + "empty code": `{"code":""}`, + "absent field": `{}`, + "malformed": `{not json`, + } + + seen := map[string]struct{}{} + for name, body := range bodies { + t.Run(name, func(t *testing.T) { + rec := exchangeCode(t, impl, body) + assert.Equal(t, http.StatusUnauthorized, rec.Code) + seen[rec.Body.String()] = struct{}{} + }) + } + + assert.Len(t, seen, 1, "every failure must answer with one identical body") +} diff --git a/internal/app/api/public/auth/oauth_dance_cookies.go b/internal/app/api/public/auth/oauth_dance_cookies.go new file mode 100644 index 0000000..da46707 --- /dev/null +++ b/internal/app/api/public/auth/oauth_dance_cookies.go @@ -0,0 +1,105 @@ +package auth + +import ( + "net/http" + "net/url" + "time" + + "github.com/labstack/echo/v5" +) + +// The two cookies that carry a dance; nothing about it is stored server-side. +// +// The split is the binding: the provider gets the plaintext state and the +// browser only its SIGNATURE, so whoever observes the redirect URL holds one +// half and not the other. The verifier never travels at all — only its S256 +// challenge does. +const ( + oauthStateCookie = "oauth_state" + oauthVerifierCookie = "oauth_code_verifier" +) + +// danceCookieValue reads one dance cookie, treating "absent" and "empty" as the +// same thing: neither can complete a dance, and no caller has a use for the +// difference. +func danceCookieValue(c *echo.Context, name string) string { + cookie, err := c.Cookie(name) + if err != nil { + return "" + } + + return cookie.Value +} + +// setDanceCookie writes one of the dance cookies. The lifetime comes from the +// service alongside the values themselves — MaxAge is only a hint to the +// browser, since the deadline that is enforced sits inside the signature. +func (i *Implementation) setDanceCookie(c *echo.Context, name, value string, ttl time.Duration) { + http.SetCookie(c.Response(), i.danceCookie(name, value, int(ttl.Seconds()))) +} + +// expireDanceCookies queues the removal of both cookies. +// +// The callback calls this FIRST, before any check, which is what makes "cleared +// on every exit" true by construction rather than by remembering it in seven +// branches. +// +// What it buys is worth stating precisely, because the obvious reading +// overstates it: a COOPERATING browser cannot carry a spent dance into the next +// attempt. An attacker replaying a captured pair with curl never honors +// Set-Cookie at all, so this bounds nothing for them — the signature's deadline +// and the provider burning the authorization code are what cover that case. +func (i *Implementation) expireDanceCookies(c *echo.Context) { + for _, name := range []string{oauthStateCookie, oauthVerifierCookie} { + http.SetCookie(c.Response(), i.danceCookie(name, "", -1)) + } +} + +// danceCookie builds a dance cookie. Expiry reuses it rather than hand-rolling +// a second cookie, because a browser only replaces one whose name, Path and +// Domain all match — an expiry with a different Path leaves the original alive. +// +// Path is configured outright (app.oauth_cookie_path) and must be the EXTERNAL +// prefix: Caddy strips /auth before the handler sees the request, so a cookie +// scoped to the internal route is never sent back and no handler test would +// notice. +// +// Secure is NOT configured alongside it. It follows the redirect_uri's SCHEME, +// because the two failure directions are not symmetric: a missing-or-wrong Path +// breaks sign-in visibly enough to chase, while a Secure that is false when it +// should be true leaks the signature and the verifier and looks like nothing at +// all. Deriving it removes the chance to get it wrong. It is emphatically not +// keyed on the environment NAME: IsDev() covers dev, local and +// performance_test, and the deployed dev stand runs environment: dev behind +// Caddy on https — so an environment-keyed flag shipped both halves of a dance +// without Secure on a live HTTPS stand, with no HSTS to fall back on. +// +//nolint:gosec // G124: Secure is conditional by design, see the note above. +func (i *Implementation) danceCookie(name, value string, maxAge int) *http.Cookie { + return &http.Cookie{ + Name: name, + Value: value, + Path: i.danceCookiePath, + MaxAge: maxAge, + HttpOnly: true, + Secure: i.danceCookieSecure, + // Lax, not Strict: the callback is a top-level navigation from the + // provider, and Strict withholds cookies on exactly that. + SameSite: http.SameSiteLaxMode, + } +} + +// danceCookieSecure reports whether the dance cookies must be HTTPS-only, +// judged by the scheme of the configured external redirect_uri. +// +// Defaults to true: an unparseable or scheme-less redirect_uri is a +// misconfiguration, and the safe direction to fail is a cookie the browser +// withholds over plain HTTP rather than one it leaks. +func danceCookieSecure(redirectURI string) bool { + u, err := url.Parse(redirectURI) + if err != nil { + return true + } + + return u.Scheme != "http" +} diff --git a/internal/app/api/public/auth/oauth_dance_main_test.go b/internal/app/api/public/auth/oauth_dance_main_test.go new file mode 100644 index 0000000..c8c0eb8 --- /dev/null +++ b/internal/app/api/public/auth/oauth_dance_main_test.go @@ -0,0 +1,192 @@ +package auth + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "sync" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/ruko1202/maintmode/internal/app/bootstrap" + "github.com/ruko1202/maintmode/internal/config" + googleoauthgw "github.com/ruko1202/maintmode/internal/gateways/googleoauth" + "github.com/ruko1202/maintmode/internal/services/auth" + "github.com/ruko1202/maintmode/internal/storages/oauthdance" + "github.com/ruko1202/maintmode/internal/utils/xuuid" +) + +const ( + testClientID = "dance-client-id" + testRedirectURI = "https://host/auth/api/v1/login/oauth/google/callback" + testFrontendURL = "https://frontend.example.com" + // testCookiePath is the EXTERNAL scope, carrying the /auth prefix the proxy + // strips. Spelled out here rather than read from the stand's config so the + // assertions pin a value this package controls. + testCookiePath = "/auth/api/v1/login/oauth" +) + +// fakeDanceGateway stands in for Google's token endpoint. The dance's own +// contract with the provider is covered by the gateway package's httptest +// suite; here the interesting behavior is what the handler does with the +// gateway's answer, so this only has to be steerable. +type fakeDanceGateway struct { + idToken string + err error + calls int + gotCode string + gotVerf string + + // real builds the authorization URL, so the /start tests assert against the + // URL production would emit rather than one this fake invented. Only the + // network call is faked; the contract with the provider is not. + real *googleoauthgw.Client +} + +func (f *fakeDanceGateway) AuthCodeURL(state, verifier string) string { + return f.real.AuthCodeURL(state, verifier) +} + +func (f *fakeDanceGateway) Exchange(_ context.Context, code, verifier string) (string, error) { + f.calls++ + f.gotCode = code + f.gotVerf = verifier + + return f.idToken, f.err +} + +// testEnv names the redirect_uri the dance handler tests run against. HTTPS, so +// the cookies carry Secure and the assertions match what production emits. +func testEnv() string { return testRedirectURI } + +// danceCookies indexes the LIVE dance cookies a response set, by name. +// +// A cookie the handler expired is deliberately excluded. Clearing a cookie is +// itself a Set-Cookie header, so a test counting the raw slice would pass +// against a handler that set two cookies and immediately cleared both — which +// is exactly what the callback does on every exit. +func danceCookies(t *testing.T, rec *httptest.ResponseRecorder) map[string]*http.Cookie { + t.Helper() + + live := map[string]*http.Cookie{} + + for _, cookie := range rec.Result().Cookies() { + if cookie.Name != oauthStateCookie && cookie.Name != oauthVerifierCookie { + continue + } + + if cookie.MaxAge < 0 { + continue + } + + live[cookie.Name] = cookie + } + + return live +} + +// expiredDanceCookies is the complement: the cookies this response told the +// browser to drop. +func expiredDanceCookies(t *testing.T, rec *httptest.ResponseRecorder) map[string]*http.Cookie { + t.Helper() + + expired := map[string]*http.Cookie{} + + for _, cookie := range rec.Result().Cookies() { + if cookie.MaxAge < 0 { + expired[cookie.Name] = cookie + } + } + + return expired +} + +// testAgent is a User-Agent unique to one request, used to find its audit rows +// in a shared database. Registered on the test so repeated calls within one test +// share it — an assertion covers the whole test, not one call. +func testAgent(t *testing.T) string { + t.Helper() + + agent, ok := testAgents.Load(t.Name()) + if !ok { + agent, _ = testAgents.LoadOrStore(t.Name(), t.Name()+"/"+xuuid.NewString()) + } + + return agent.(string) +} + +var testAgents sync.Map + +// errProviderRefused is a stand-in provider failure for tests that only care that +// the handler treats the exchange as failed. +var errProviderRefused = errors.New("provider refused the exchange") + +func initDanceImpl(t *testing.T) *Implementation { + t.Helper() + + return initDanceImplForRedirectURI(t, testRedirectURI) +} + +// initDanceImplForRedirectURI builds a dance whose cookie attributes follow the +// scheme of the given redirect_uri — which is what decides Secure. +func initDanceImplForRedirectURI(t *testing.T, redirectURI string) *Implementation { + t.Helper() + + return initDanceImplWith(t, redirectURI, newFakeGateway(t, "stub-id-token", nil)) +} + +// newFakeGateway builds a gateway whose exchange is faked but whose +// authorization URL is the real one. +func newFakeGateway(t *testing.T, idToken string, err error) *fakeDanceGateway { + t.Helper() + + return &fakeDanceGateway{ + idToken: idToken, + err: err, + real: googleoauthgw.NewClient(danceProviderConfig()), + } +} + +// danceProviderConfig is the provider block the dance tests run against. +func danceProviderConfig() config.GoogleOauthProvider { + return config.GoogleOauthProvider{ + ClientID: testClientID, + ClientSecret: "dance-client-secret", + RedirectURI: testRedirectURI, + // Spelled out because the gateway has no in-code default: these are the + // values a stand's app.config.yaml carries, and the /start assertions + // below check the redirect actually points at them. + AuthURL: "https://accounts.google.com/o/oauth2/v2/auth", + TokenURL: "https://oauth2.googleapis.com/token", + } +} + +// initDanceImplWith builds a dance-enabled handler against the REAL Valkey +// store. The store is where single-use lives, and substituting a fake here +// would quietly retire the property these handler tests most need to hold. +func initDanceImplWith(t *testing.T, redirectURI string, gateway auth.DanceGateway) *Implementation { + t.Helper() + + stores, err := bootstrap.NewStores(db, valkey) + require.NoError(t, err) + + services, err := bootstrap.NewServices(t.Context(), cfg, stores) + require.NoError(t, err) + + provider := danceProviderConfig() + provider.RedirectURI = redirectURI + + // The signer lives on the service now, so the dance is armed in two places: + // the service gets the signing secret, the handler gets the transport. + impl := New(cfg.Auth, + services.Auth.WithDance(cfg.Auth, provider.ClientSecret, oauthdance.NewStore(valkey), gateway), + services.Token, services.User, services.OTP) + + return impl.WithOAuthDance(provider, config.App{ + FrontendURL: testFrontendURL, + OAuthCallbackPath: cfg.App.OAuthCallbackPath, + OAuthCookiePath: testCookiePath, + }) +} diff --git a/internal/app/api/public/auth/oauth_dance_redirect.go b/internal/app/api/public/auth/oauth_dance_redirect.go new file mode 100644 index 0000000..8451b34 --- /dev/null +++ b/internal/app/api/public/auth/oauth_dance_redirect.go @@ -0,0 +1,110 @@ +package auth + +import ( + "errors" + "net/http" + "net/url" + "strings" + + "github.com/labstack/echo/v5" + + "github.com/ruko1202/maintmode/internal/apperr" +) + +// Query parameters the dance exchanges with the provider and the frontend. +const ( + paramCode = "code" + paramState = "state" + paramError = "error" +) + +// Redirect codes the frontend renders: a closed, stable set that RUK-292 maps +// to messages, so adding one is a cross-ticket contract change. +// +// Distinct failures collapse onto one code deliberately — the browser learns +// only that the dance did not complete, and which cause stays in the audit +// trail. errCodeStateInvalid in particular covers every way a callback can fail +// to present a state this backend signed. +const ( + errCodeAccessDenied = "access_denied" + errCodeStateInvalid = "state_invalid" + errCodeProvider = "provider_error" + errCodeInternal = "internal_error" +) + +// danceFailureCode maps a failed dance to the code the browser is sent home +// with: the service reports what went wrong, this decides what the frontend is +// told. +// +// A refused account is the MOST LIKELY failure in production, where signup is +// invite-only, and reads as a denial rather than an internal fault — nothing is +// broken, the person needs an invitation. +// +// A free function so the mapping can be tested without the whole auth service: +// the local stand runs open signup and never produces that error, so a mutation +// collapsing this to internal_error otherwise passes every handler test. +func danceFailureCode(err error) string { + switch { + case errors.Is(err, apperr.ErrOAuthProviderDenied): + return providerErrorCode(err) + case errors.Is(err, apperr.ErrSignupDisabled), errors.Is(err, apperr.ErrUserBlocked): + return errCodeAccessDenied + case errors.Is(err, apperr.ErrOAuthDanceStateInvalid), errors.Is(err, apperr.ErrUnsupportedProvider): + return errCodeStateInvalid + case errors.Is(err, apperr.ErrOAuthExchangeFailed): + return errCodeProvider + default: + return errCodeInternal + } +} + +// providerErrorCode separates a user declining consent from a provider +// misbehaving: "I changed my mind" and "the provider is broken" are different +// things to whoever reads the trail, and only the first is a denial. +func providerErrorCode(err error) string { + if strings.Contains(err.Error(), errCodeAccessDenied) { + return errCodeAccessDenied + } + + return errCodeProvider +} + +// redirectFailure sends the browser home with a readable code. Never JSON: the +// user is mid-navigation, and a JSON body would strand them on a blank page. +func (i *Implementation) redirectFailure(c *echo.Context, code string) error { + return i.redirectHome(c, url.Values{paramError: {code}}) +} + +// redirectHome is the one place this handler emits a 302, so the cache +// directive cannot be forgotten on the branch that matters: the success +// redirect carries a live one-time code, and a 302 with no directives is +// heuristically cacheable by a shared proxy (RFC 9111 §4.2.2). +func (i *Implementation) redirectHome(c *echo.Context, q url.Values) error { + c.Response().Header().Set(echo.HeaderCacheControl, "no-store") + + return c.Redirect(http.StatusFound, i.frontendURLWith(q)) +} + +// frontendURLWith builds the redirect target from CONFIGURED values only. There +// is no return_to parameter, in this ticket or as a hook for a later one: a +// redirect target taken from the request is an open redirect. +// +// Built with net/url rather than by concatenation: JoinPath settles the slash +// between origin and path — the earlier version trimmed one by hand — and +// String() escapes what needs escaping instead of trusting the inputs to be +// clean. +func (i *Implementation) frontendURLWith(q url.Values) string { + target, err := url.Parse(i.frontendURL) + if err != nil { + // Unreachable in practice: the config gate refuses to register these + // routes without a frontend_url, and a value that will not parse would + // have failed at wiring. Falling back to the raw string keeps a + // misconfigured stand pointing somewhere visible rather than at "". + return i.frontendURL + } + + target = target.JoinPath(i.frontendCallbackPath) + target.RawQuery = q.Encode() + + return target.String() +} diff --git a/internal/app/api/public/auth/oauth_start.go b/internal/app/api/public/auth/oauth_start.go new file mode 100644 index 0000000..12cb1c2 --- /dev/null +++ b/internal/app/api/public/auth/oauth_start.go @@ -0,0 +1,67 @@ +package auth + +import ( + "fmt" + "net/http" + + "github.com/labstack/echo/v5" + "github.com/ruko1202/xlog" + "github.com/ruko1202/xlog/xfield" + + "github.com/ruko1202/maintmode/internal/app/api/httperrors" + "github.com/ruko1202/maintmode/internal/apperr" + "github.com/ruko1202/maintmode/internal/entity" +) + +// StartOAuthDance godoc +// @Summary Begin the backend-driven OAuth dance +// @Description Mints CSRF state and a PKCE verifier, hands the browser the state's signature and the verifier as two httpOnly cookies, and redirects to the provider. Nothing is stored server-side. Answers 302 on success; a provider outside the supported set answers 400. This is the backend-owned alternative to the BFF flow behind /login/oauth/exchange/google, which stays live. +// @Tags Auth +// @Produce json +// @Param provider path string true "Provider id" Enums(google) +// @Success 302 "Redirect to the provider's authorization endpoint" +// @Failure 400 {object} httperrors.ErrorResponse "Unsupported provider" +// @Failure 429 {object} httperrors.ErrorResponse "Rate limit exceeded" +// @Failure 500 {object} httperrors.ErrorResponse "Internal error" +// @Router /api/v1/login/oauth/{provider}/start [get] +func (i *Implementation) StartOAuthDance(c *echo.Context) error { + ctx, span := xlog.WithOperationSpan(c.Request().Context(), "api.Auth.OAuthDance.Start") + defer span.End() + op := "oauth dance start" + + // The allow-list runs FIRST, before any secret is minted or stored. + // + // Ordering matters twice over: an unknown provider must cost nothing, and + // this route's {provider} segment shares a path space with the static + // /login/oauth/exchange/google, so an unchecked parameter is how a request + // for one route ends up handled by another. + provider, ok := entity.DanceProvider(c.Param("provider")) + if !ok { + xlog.Warn(ctx, "oauth dance requested for an unsupported provider", + xfield.String("provider", c.Param("provider"))) + + // JSON, not a redirect. There is no trusted frontend target to send an + // error to at this point, and echoing an arbitrary path segment into a + // Location header is how open redirects start. + return httperrors.ToAPIError(c, op, fmt.Errorf("%w: %s", apperr.ErrUnsupportedProvider, c.Param("provider"))) + } + + dance, err := i.authSrv.StartDance(ctx, provider) + if err != nil { + xlog.Error(ctx, "failed to start the oauth dance", xfield.Error(err)) + return httperrors.ToAPIError(c, op, err) + } + + // Nothing is stored. The browser carries the whole dance: the state's + // SIGNATURE and the verifier itself, both httpOnly. + // + // The provider receives the plaintext state, so whoever observes the + // redirect URL holds one half of the pair and not the other. That asymmetry + // is the binding — with the honest limit that a signature authenticates a + // callback without making it one-shot, since re-signing the same state + // inside the window yields the same value every time. + i.setDanceCookie(c, oauthStateCookie, dance.StateSignature, dance.TTL) + i.setDanceCookie(c, oauthVerifierCookie, dance.Verifier, dance.TTL) + + return c.Redirect(http.StatusFound, dance.AuthorizationURL) +} diff --git a/internal/app/api/public/auth/oauth_start_test.go b/internal/app/api/public/auth/oauth_start_test.go new file mode 100644 index 0000000..d632194 --- /dev/null +++ b/internal/app/api/public/auth/oauth_start_test.go @@ -0,0 +1,258 @@ +package auth + +import ( + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/labstack/echo/v5" + "github.com/labstack/echo/v5/echotest" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/ruko1202/maintmode/internal/entity" +) + +// startRequest drives the /start handler with the given provider path param and +// returns the recorder, so a test can read both the redirect and the cookie. +func startRequest(t *testing.T, impl *Implementation) *httptest.ResponseRecorder { + t.Helper() + + rec := httptest.NewRecorder() + c := danceContext(t, rec, string(entity.AuthMethodGoogle)) + + require.NoError(t, impl.StartOAuthDance(c)) + + return rec +} + +// danceContext builds an echo context carrying the {provider} path value. +func danceContext(t *testing.T, rec *httptest.ResponseRecorder, provider string) *echo.Context { + t.Helper() + + return echotest.ContextConfig{ + Request: httptest.NewRequest(http.MethodGet, "/login/oauth/"+provider+"/start", http.NoBody), + Response: rec, + PathValues: echo.PathValues{{Name: "provider", Value: provider}}, + }.ToContext(t) +} + +func TestStartRedirectsToTheProvider(t *testing.T) { + impl := initDanceImpl(t) + + rec := startRequest(t, impl) + + require.Equal(t, http.StatusFound, rec.Code) + + target, err := url.Parse(rec.Header().Get(echo.HeaderLocation)) + require.NoError(t, err) + + assert.Equal(t, "https", target.Scheme) + assert.Equal(t, "accounts.google.com", target.Host) + + q := target.Query() + assert.Equal(t, "code", q.Get("response_type")) + assert.Equal(t, testClientID, q.Get("client_id")) + assert.Equal(t, testRedirectURI, q.Get("redirect_uri")) + assert.Equal(t, "openid email profile", q.Get("scope")) + assert.NotEmpty(t, q.Get("state")) + + // PKCE. The challenge must be the S256 transform, never the verifier itself + // ("plain"), which would make the whole exercise decorative. + assert.Equal(t, "S256", q.Get("code_challenge_method")) + assert.NotEmpty(t, q.Get("code_challenge")) + + // The verifier is the secret half and must never leave this service. + assert.NotContains(t, rec.Header().Get(echo.HeaderLocation), "code_verifier") +} + +// TestStartSetsBothDanceCookies is the assertion that would otherwise only fail +// in production. +// +// Caddy serves this backend under `handle_path /auth/*`, which STRIPS the +// prefix: the handler sees /api/v1/... while the browser's URL space is +// /auth/api/v1/... A cookie scoped to the path the handler sees is never sent +// back on the callback, and no handler-level test would notice because there is +// no Caddy in one. So the Path must be the external value from config +// (app.oauth_cookie_path), and this pins that it reaches the cookie unaltered +// rather than being replaced by the mounted route. +func TestStartSetsBothDanceCookies(t *testing.T) { + impl := initDanceImpl(t) + + rec := startRequest(t, impl) + cookies := danceCookies(t, rec) + + require.Len(t, cookies, 2, "the dance needs the state signature and the verifier, and nothing else") + + for _, name := range []string{oauthStateCookie, oauthVerifierCookie} { + cookie, ok := cookies[name] + require.True(t, ok, "missing cookie %s", name) + + assert.NotEmpty(t, cookie.Value) + assert.True(t, cookie.HttpOnly, "a dance cookie must be unreadable from JS") + assert.Equal(t, http.SameSiteLaxMode, cookie.SameSite, + "Strict would withhold the cookie on the redirect back from Google, which is exactly when it is needed") + assert.Equal(t, testCookiePath, cookie.Path, + "the cookie must carry the configured EXTERNAL path; Caddy strips the /auth prefix before the handler sees it") + // A real lifetime, not a token one. Collapsing MaxAge to a second makes + // the browser drop the cookie before the consent screen is done, so + // every dance dies at the callback — as a 302 that reads as success. + // The exact number is the service's to choose and is not pinned here. + assert.Greater(t, cookie.MaxAge, 60, + "a cookie the browser discards mid-consent breaks every dance") + + // Padded base64 would put "=" in a cookie value, which http.SetCookie + // does not encode and c.Cookie does not decode. + assert.NotContains(t, cookie.Value, "=") + } +} + +// TestStartCookieCarriesTheSignatureNotTheState is the one that stops a later +// "simplification" of the cookie down to the value it protects. +// +// The provider is sent the plaintext state and the browser holds only its +// signature; that asymmetry IS the binding. A cookie holding the state itself +// would hand whoever reads the redirect URL both halves at once, while every +// round-trip test stayed green. +func TestStartCookieCarriesTheSignatureNotTheState(t *testing.T) { + impl := initDanceImpl(t) + + rec := startRequest(t, impl) + + target, err := url.Parse(rec.Header().Get(echo.HeaderLocation)) + require.NoError(t, err) + + state := target.Query().Get("state") + require.NotEmpty(t, state) + + cookies := danceCookies(t, rec) + stateCookie := cookies[oauthStateCookie] + require.NotNil(t, stateCookie) + + assert.NotContains(t, stateCookie.Value, state, + "the cookie must carry a signature over the state, never the state itself") + + // And it must be a signature the callback will accept. Asked by driving the + // real callback rather than by reaching into internals: a start that mints a + // signature its own callback refuses is the failure worth catching, and only + // the round trip proves it does not happen. + _, q := redirectResult(t, callbackRequest(t, impl, url.Values{ + "code": {"provider-auth-code"}, + "state": {state}, + }, danceRun{state: state, signature: stateCookie.Value, verifier: cookies[oauthVerifierCookie].Value})) + + assert.NotEmpty(t, q.Get("code"), "the callback must accept the signature /start issued") + assert.Empty(t, q.Get("error")) +} + +// TestStartSecureFlagFollowsTheRedirectScheme pins the attribute whose earlier +// version shipped a real hole. +// +// It used to be !IsDev(), with a comment claiming IsDev() is false only in prod. +// That is wrong: IsDev() covers dev, local and performance_test, and the +// deployed dev stand runs environment: dev behind Caddy on https — so a live +// HTTPS stand was handing out the state signature and the PKCE verifier without +// Secure. Any plain-HTTP request the attacker could provoke to that host would +// have carried both halves of a dance, and there is no HSTS in this deployment +// to fall back on. +// +// Deriving it from the redirect_uri's scheme ties the flag to how the instance +// is actually reached rather than to what its environment is called, so a new +// stand cannot reintroduce the hole by picking a name. +func TestStartSecureFlagFollowsTheRedirectScheme(t *testing.T) { + tests := map[string]struct { + redirectURI string + want bool + }{ + "https stand carries Secure": { + redirectURI: "https://host/auth/api/v1/login/oauth/google/callback", + want: true, + }, + // The case the old gate got wrong: an HTTPS stand that is NOT prod. + "https dev stand carries Secure too": { + redirectURI: "https://dev.maintmode.dev/auth/api/v1/login/oauth/google/callback", + want: true, + }, + // Plain HTTP on localhost is the only reason this flag is conditional at + // all: hard-coding it true makes every local dance fail silently. + "plain http localhost does not": { + redirectURI: "http://localhost:9000/auth/api/v1/login/oauth/google/callback", + want: false, + }, + // A misconfigured value fails towards the safe side: a cookie the + // browser withholds beats one it leaks. + "an unparseable redirect_uri defaults to Secure": { + redirectURI: "://not-a-url", + want: true, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + impl := initDanceImplForRedirectURI(t, tt.redirectURI) + + cookies := danceCookies(t, startRequest(t, impl)) + require.Len(t, cookies, 2) + + for cookieName, cookie := range cookies { + assert.Equal(t, tt.want, cookie.Secure, cookieName) + } + }) + } +} + +// TestStartRejectsUnknownProviderBeforeAnyWork covers the ordering that keeps +// the new param route from swallowing its static neighbors. The allow-list runs +// first, and an unknown provider gets a 400 JSON rather than a redirect: there +// is no trusted frontend target yet, and echoing an arbitrary path segment into +// a Location header is how open redirects begin. +func TestStartRejectsUnknownProviderBeforeAnyWork(t *testing.T) { + impl := initDanceImpl(t) + + for _, provider := range []string{"github", "exchange", "", "../etc"} { + t.Run("provider="+provider, func(t *testing.T) { + rec := httptest.NewRecorder() + c := danceContext(t, rec, provider) + + err := impl.StartOAuthDance(c) + if err != nil { + // Echo's error handler is not wired in this bare context, so a + // returned error is the handler refusing — which is the point. + assert.NotEmpty(t, err.Error()) + return + } + + assert.Equal(t, http.StatusBadRequest, rec.Code) + assert.Empty(t, rec.Header().Get(echo.HeaderLocation), "a refusal must never redirect") + assert.Empty(t, rec.Result().Cookies(), "a refusal must not set a dance cookie") + }) + } +} + +// TestStartIssuesAFreshStatePerCall guards against a constant or reused state, +// which would let one captured callback URL be replayed forever. +func TestStartIssuesAFreshStatePerCall(t *testing.T) { + impl := initDanceImpl(t) + + seen := map[string]bool{} + for range 5 { + rec := startRequest(t, impl) + + target, err := url.Parse(rec.Header().Get(echo.HeaderLocation)) + require.NoError(t, err) + + state := target.Query().Get("state") + require.NotEmpty(t, state) + assert.False(t, seen[state], "state must be unique per dance") + seen[state] = true + + // The verifier is the secret half of PKCE and must never travel in the + // same channel as anything the provider sees. + verifier := danceCookies(t, rec)[oauthVerifierCookie] + require.NotNil(t, verifier) + assert.False(t, strings.Contains(rec.Header().Get(echo.HeaderLocation), verifier.Value), + "the pkce verifier must not travel in the redirect it protects") + } +} diff --git a/internal/server/api_server.go b/internal/server/api_server.go index 55c330d..897ebd5 100644 --- a/internal/server/api_server.go +++ b/internal/server/api_server.go @@ -80,6 +80,13 @@ type APIServer struct { // user). Each limiter degrades to a per-replica in-memory bucket when valkey // is unreachable. valkey *valkeylib.Client + // oauthDanceEnabled decides whether the backend-driven dance routes are + // registered at all. It arrives already computed (config.OAuthDanceEnabled) + // rather than as the config block itself: the router needs the decision, not + // the credentials behind it, and passing the whole AppConfig here to answer + // one boolean would hand the routing layer the client secret it must never + // touch. + oauthDanceEnabled bool } func NewAPIServer( @@ -87,6 +94,7 @@ func NewAPIServer( handlers APIServerHandlers, security APIServerSecurity, rdb *valkeylib.Client, + oauthDanceEnabled bool, opts ...xhttpserver.Option, ) *APIServer { timeouts := cfg.Timeouts.TimeoutsOrDefault() @@ -104,10 +112,11 @@ func NewAPIServer( WriteTimeout: timeouts.Write, IdleTimeout: timeouts.Idle, }, opts...), - cfg: cfg, - handlers: handlers, - security: security, - valkey: rdb, + cfg: cfg, + handlers: handlers, + security: security, + valkey: rdb, + oauthDanceEnabled: oauthDanceEnabled, } } @@ -115,7 +124,8 @@ func (s *APIServer) BindRouters(env config.Environment, meta *buildmeta.AppBuild rootGr := s.Echo().Group("") rootGr.Use(middlewares.BaseAPIMiddlewares(env, meta)...) - rootGr.RouteNotFound("/*", xhttpserver.NotFoundHandler, xhttpserver.RequestLoggingMiddleware()) + rootGr.RouteNotFound("/*", xhttpserver.NotFoundHandler, + xhttpserver.RequestLoggingMiddlewareWithSanitizer(middlewares.NewRequestSanitizer())) // The /api/v1 base group carries NO blanket access-token gate: the auth module // exposes public routes (login/oauth, refresh, jwks, invitation preview/accept) @@ -187,6 +197,7 @@ func (s *APIServer) authPublicV1Group(gr *echo.Group, _ config.Environment, meta middleware.RateLimiter(NewRateLimiter(meta.AppName, s.valkey, s.cfg.RateLimiter)), ) loginOAuthGr.Add(http.MethodPost, "/exchange/google", s.handlers.Auth.ExchangeGoogleToken) + s.oauthDanceRoutes(loginOAuthGr) // The break-glass password sign-in. Registered in every environment: it is // what breaks the "to configure a provider you must sign in" loop on a fresh @@ -217,6 +228,34 @@ func (s *APIServer) authPublicV1Group(gr *echo.Group, _ config.Environment, meta invitesGr.Add(http.MethodPost, "/accept", s.handlers.Invitations.AcceptInvitation) } +// oauthDanceRoutes registers the backend-driven OAuth dance, and only when it is +// configured. +// +// The gate is what makes this ticket safe to deploy: an instance that sets no +// client_secret keeps exactly the routing it had before, and the BFF path at +// /exchange/google — registered above, unconditionally — stays the only way in. +// A half-configured block leaves these unregistered too, because a dance that +// cannot reach the token endpoint is worse than no dance. +// +// The routes share the group's per-IP limiter with /exchange/google. That is +// deliberate: they are the same surface for the same anonymous caller, and +// NewRateLimiter keys on the client IP with no route component anyway. +// +// STATIC BEFORE PARAM. /code/exchange is a literal segment in the same position +// as {provider}, and echo prefers static segments, but the ordering is written +// this way so the dependency is visible rather than accidental. The handler's +// own allow-list is the real guard: it rejects anything that is not a known +// provider before minting a secret. +func (s *APIServer) oauthDanceRoutes(loginOAuthGr *echo.Group) { + if !s.oauthDanceEnabled { + return + } + + loginOAuthGr.Add(http.MethodPost, "/code/exchange", s.handlers.Auth.ExchangeOAuthDanceCode) + loginOAuthGr.Add(http.MethodGet, "/:provider/start", s.handlers.Auth.StartOAuthDance) + loginOAuthGr.Add(http.MethodGet, "/:provider/callback", s.handlers.Auth.OAuthDanceCallback) +} + // otpRoutes registers the one-time-code endpoints behind all three limiter // tiers. // From 57ce4130ac59c5ec158dad976e7d0377042bc367 Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Tue, 8 Sep 2026 00:00:41 +0300 Subject: [PATCH 10/11] chore(auth): regenerate the OpenAPI spec and client for the dance Output of `make swag` and the client codegen. Kept apart from the handwritten change so the diff that needs reading stays readable. Co-Authored-By: Claude Opus 5 --- docs/auth/swagger.json | 180 ++++++ docs/auth/swagger.yaml | 112 ++++ .../pkg/generated/clients/auth/client.gen.go | 520 ++++++++++++++++++ 3 files changed, 812 insertions(+) diff --git a/docs/auth/swagger.json b/docs/auth/swagger.json index deeb661..2a9fa1d 100644 --- a/docs/auth/swagger.json +++ b/docs/auth/swagger.json @@ -238,6 +238,14 @@ }, "type": "object" }, + "apiauthmodels.ExchangeOAuthCodeRequest": { + "properties": { + "code": { + "type": "string" + } + }, + "type": "object" + }, "apiauthmodels.JWKSResponse": { "properties": { "keys": { @@ -1093,6 +1101,58 @@ ] } }, + "/api/v1/login/oauth/code/exchange": { + "post": { + "description": "Trades the short-lived opaque code the callback put in the redirect for the token pair it stands for. Single-use: the second attempt with the same code fails like any other. Every failure — unknown, expired, already redeemed, malformed — answers the same 401, so a caller cannot learn which of its guesses was closer.", + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/apiauthmodels.ExchangeOAuthCodeRequest" + } + } + }, + "description": "The one-time code", + "required": true + }, + "responses": { + "200": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/apiauthmodels.TokenPairResponse" + } + } + }, + "description": "OK" + }, + "401": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/httperrors.ErrorResponse" + } + } + }, + "description": "The code is not redeemable" + }, + "429": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/httperrors.ErrorResponse" + } + } + }, + "description": "Rate limit exceeded" + } + }, + "summary": "Redeem the one-time code from an OAuth dance", + "tags": [ + "Auth" + ] + } + }, "/api/v1/login/oauth/exchange/google": { "post": { "requestBody": { @@ -1164,6 +1224,126 @@ ] } }, + "/api/v1/login/oauth/{provider}/callback": { + "get": { + "description": "Verifies the signed state carried in the oauth_state cookie, exchanges the authorization code for tokens using the client secret and the PKCE verifier from the oauth_code_verifier cookie, resolves the user and redirects to the frontend with a one-time code. Both cookies are cleared on every exit. Always answers 302, success or failure: the user's browser is sitting on this URL, so a JSON error body would be a dead end.", + "parameters": [ + { + "description": "Provider id", + "in": "path", + "name": "provider", + "required": true, + "schema": { + "enum": [ + "google" + ], + "type": "string" + } + }, + { + "description": "Authorization code from the provider", + "in": "query", + "name": "code", + "schema": { + "type": "string" + } + }, + { + "description": "The state issued by /start", + "in": "query", + "name": "state", + "schema": { + "type": "string" + } + }, + { + "description": "Error reported by the provider", + "in": "query", + "name": "error", + "schema": { + "type": "string" + } + } + ], + "responses": { + "302": { + "description": "Redirect to the frontend carrying a one-time code" + }, + "429": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/httperrors.ErrorResponse" + } + } + }, + "description": "Rate limit exceeded" + } + }, + "summary": "Complete the backend-driven OAuth dance", + "tags": [ + "Auth" + ] + } + }, + "/api/v1/login/oauth/{provider}/start": { + "get": { + "description": "Mints CSRF state and a PKCE verifier, hands the browser the state's signature and the verifier as two httpOnly cookies, and redirects to the provider. Nothing is stored server-side. Answers 302 on success; a provider outside the supported set answers 400. This is the backend-owned alternative to the BFF flow behind /login/oauth/exchange/google, which stays live.", + "parameters": [ + { + "description": "Provider id", + "in": "path", + "name": "provider", + "required": true, + "schema": { + "enum": [ + "google" + ], + "type": "string" + } + } + ], + "responses": { + "302": { + "description": "Redirect to the provider's authorization endpoint" + }, + "400": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/httperrors.ErrorResponse" + } + } + }, + "description": "Unsupported provider" + }, + "429": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/httperrors.ErrorResponse" + } + } + }, + "description": "Rate limit exceeded" + }, + "500": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/httperrors.ErrorResponse" + } + } + }, + "description": "Internal error" + } + }, + "summary": "Begin the backend-driven OAuth dance", + "tags": [ + "Auth" + ] + } + }, "/api/v1/login/otp/request": { "post": { "description": "Emails a one-time code to the address, if it belongs to an account. Answers 202 in every case — unknown address, blocked account, malformed body — so the response never reveals whether an account exists. The returned session_nonce binds the code to the client that asked for it and must be presented when the code is verified; it is never emailed.", diff --git a/docs/auth/swagger.yaml b/docs/auth/swagger.yaml index 2a441d7..9b57b44 100644 --- a/docs/auth/swagger.yaml +++ b/docs/auth/swagger.yaml @@ -171,6 +171,11 @@ components: description: IDToken is the upstream provider's signed JWT. type: string type: object + apiauthmodels.ExchangeOAuthCodeRequest: + properties: + code: + type: string + type: object apiauthmodels.JWKSResponse: properties: keys: @@ -822,6 +827,113 @@ paths: summary: Get licensed seat usage tags: - Users + /api/v1/login/oauth/{provider}/callback: + get: + description: 'Verifies the signed state carried in the oauth_state cookie, exchanges the authorization code for tokens using the client secret and the PKCE verifier from the oauth_code_verifier cookie, resolves the user and redirects to the frontend with a one-time code. Both cookies are cleared on every exit. Always answers 302, success or failure: the user''s browser is sitting on this URL, so a JSON error body would be a dead end.' + parameters: + - description: Provider id + in: path + name: provider + required: true + schema: + enum: + - google + type: string + - description: Authorization code from the provider + in: query + name: code + schema: + type: string + - description: The state issued by /start + in: query + name: state + schema: + type: string + - description: Error reported by the provider + in: query + name: error + schema: + type: string + responses: + "302": + description: Redirect to the frontend carrying a one-time code + "429": + content: + application/json: + schema: + $ref: '#/components/schemas/httperrors.ErrorResponse' + description: Rate limit exceeded + summary: Complete the backend-driven OAuth dance + tags: + - Auth + /api/v1/login/oauth/{provider}/start: + get: + description: Mints CSRF state and a PKCE verifier, hands the browser the state's signature and the verifier as two httpOnly cookies, and redirects to the provider. Nothing is stored server-side. Answers 302 on success; a provider outside the supported set answers 400. This is the backend-owned alternative to the BFF flow behind /login/oauth/exchange/google, which stays live. + parameters: + - description: Provider id + in: path + name: provider + required: true + schema: + enum: + - google + type: string + responses: + "302": + description: Redirect to the provider's authorization endpoint + "400": + content: + application/json: + schema: + $ref: '#/components/schemas/httperrors.ErrorResponse' + description: Unsupported provider + "429": + content: + application/json: + schema: + $ref: '#/components/schemas/httperrors.ErrorResponse' + description: Rate limit exceeded + "500": + content: + application/json: + schema: + $ref: '#/components/schemas/httperrors.ErrorResponse' + description: Internal error + summary: Begin the backend-driven OAuth dance + tags: + - Auth + /api/v1/login/oauth/code/exchange: + post: + description: 'Trades the short-lived opaque code the callback put in the redirect for the token pair it stands for. Single-use: the second attempt with the same code fails like any other. Every failure — unknown, expired, already redeemed, malformed — answers the same 401, so a caller cannot learn which of its guesses was closer.' + requestBody: + content: + application/json: + schema: + $ref: '#/components/schemas/apiauthmodels.ExchangeOAuthCodeRequest' + description: The one-time code + required: true + responses: + "200": + content: + application/json: + schema: + $ref: '#/components/schemas/apiauthmodels.TokenPairResponse' + description: OK + "401": + content: + application/json: + schema: + $ref: '#/components/schemas/httperrors.ErrorResponse' + description: The code is not redeemable + "429": + content: + application/json: + schema: + $ref: '#/components/schemas/httperrors.ErrorResponse' + description: Rate limit exceeded + summary: Redeem the one-time code from an OAuth dance + tags: + - Auth /api/v1/login/oauth/exchange/google: post: requestBody: diff --git a/internal/pkg/generated/clients/auth/client.gen.go b/internal/pkg/generated/clients/auth/client.gen.go index 8ca17e7..37fee76 100644 --- a/internal/pkg/generated/clients/auth/client.gen.go +++ b/internal/pkg/generated/clients/auth/client.gen.go @@ -235,6 +235,36 @@ func (e GetApiV1AuditLogParamsAction) Valid() bool { } } +// Defines values for GetApiV1LoginOauthProviderCallbackParamsProvider. +const ( + GetApiV1LoginOauthProviderCallbackParamsProviderGoogle GetApiV1LoginOauthProviderCallbackParamsProvider = "google" +) + +// Valid indicates whether the value is a known member of the GetApiV1LoginOauthProviderCallbackParamsProvider enum. +func (e GetApiV1LoginOauthProviderCallbackParamsProvider) Valid() bool { + switch e { + case GetApiV1LoginOauthProviderCallbackParamsProviderGoogle: + return true + default: + return false + } +} + +// Defines values for GetApiV1LoginOauthProviderStartParamsProvider. +const ( + GetApiV1LoginOauthProviderStartParamsProviderGoogle GetApiV1LoginOauthProviderStartParamsProvider = "google" +) + +// Valid indicates whether the value is a known member of the GetApiV1LoginOauthProviderStartParamsProvider enum. +func (e GetApiV1LoginOauthProviderStartParamsProvider) Valid() bool { + switch e { + case GetApiV1LoginOauthProviderStartParamsProviderGoogle: + return true + default: + return false + } +} + // Defines values for PostApiV1MeProvidersProviderConnectParamsProvider. const ( PostApiV1MeProvidersProviderConnectParamsProviderGithub PostApiV1MeProvidersProviderConnectParamsProvider = "github" @@ -378,6 +408,11 @@ type ApiauthmodelsExchangeIDTokenRequest struct { IdToken *string `json:"id_token,omitempty"` } +// ApiauthmodelsExchangeOAuthCodeRequest defines model for apiauthmodels.ExchangeOAuthCodeRequest. +type ApiauthmodelsExchangeOAuthCodeRequest struct { + Code *string `json:"code,omitempty"` +} + // ApiauthmodelsJWKSResponse defines model for apiauthmodels.JWKSResponse. type ApiauthmodelsJWKSResponse struct { Keys *[]EntityJWK `json:"keys,omitempty"` @@ -674,6 +709,24 @@ type GetApiV1AuditLogParams struct { // GetApiV1AuditLogParamsAction defines parameters for GetApiV1AuditLog. type GetApiV1AuditLogParamsAction string +// GetApiV1LoginOauthProviderCallbackParams defines parameters for GetApiV1LoginOauthProviderCallback. +type GetApiV1LoginOauthProviderCallbackParams struct { + // Code Authorization code from the provider + Code *string `form:"code,omitempty" json:"code,omitempty"` + + // State The state issued by /start + State *string `form:"state,omitempty" json:"state,omitempty"` + + // Error Error reported by the provider + Error *string `form:"error,omitempty" json:"error,omitempty"` +} + +// GetApiV1LoginOauthProviderCallbackParamsProvider defines parameters for GetApiV1LoginOauthProviderCallback. +type GetApiV1LoginOauthProviderCallbackParamsProvider string + +// GetApiV1LoginOauthProviderStartParamsProvider defines parameters for GetApiV1LoginOauthProviderStart. +type GetApiV1LoginOauthProviderStartParamsProvider string + // PostApiV1LogoutParams defines parameters for PostApiV1Logout. type PostApiV1LogoutParams struct { // Authorization Bearer access token @@ -728,6 +781,9 @@ type GetApiV1UsersListParams struct { Active *bool `form:"active,omitempty" json:"active,omitempty"` } +// PostApiV1LoginOauthCodeExchangeJSONRequestBody defines body for PostApiV1LoginOauthCodeExchange for application/json ContentType. +type PostApiV1LoginOauthCodeExchangeJSONRequestBody = ApiauthmodelsExchangeOAuthCodeRequest + // PostApiV1LoginOauthExchangeGoogleJSONRequestBody defines body for PostApiV1LoginOauthExchangeGoogle for application/json ContentType. type PostApiV1LoginOauthExchangeGoogleJSONRequestBody = ApiauthmodelsExchangeIDTokenRequest @@ -861,11 +917,22 @@ type ClientInterface interface { // GetApiV1LicenseSeats request GetApiV1LicenseSeats(ctx context.Context, reqEditors ...RequestEditorFn) (*http.Response, error) + // PostApiV1LoginOauthCodeExchangeWithBody request with any body + PostApiV1LoginOauthCodeExchangeWithBody(ctx context.Context, contentType string, body io.Reader, reqEditors ...RequestEditorFn) (*http.Response, error) + + PostApiV1LoginOauthCodeExchange(ctx context.Context, body PostApiV1LoginOauthCodeExchangeJSONRequestBody, reqEditors ...RequestEditorFn) (*http.Response, error) + // PostApiV1LoginOauthExchangeGoogleWithBody request with any body PostApiV1LoginOauthExchangeGoogleWithBody(ctx context.Context, contentType string, body io.Reader, reqEditors ...RequestEditorFn) (*http.Response, error) PostApiV1LoginOauthExchangeGoogle(ctx context.Context, body PostApiV1LoginOauthExchangeGoogleJSONRequestBody, reqEditors ...RequestEditorFn) (*http.Response, error) + // GetApiV1LoginOauthProviderCallback request + GetApiV1LoginOauthProviderCallback(ctx context.Context, provider GetApiV1LoginOauthProviderCallbackParamsProvider, params *GetApiV1LoginOauthProviderCallbackParams, reqEditors ...RequestEditorFn) (*http.Response, error) + + // GetApiV1LoginOauthProviderStart request + GetApiV1LoginOauthProviderStart(ctx context.Context, provider GetApiV1LoginOauthProviderStartParamsProvider, reqEditors ...RequestEditorFn) (*http.Response, error) + // PostApiV1LoginOtpRequestWithBody request with any body PostApiV1LoginOtpRequestWithBody(ctx context.Context, contentType string, body io.Reader, reqEditors ...RequestEditorFn) (*http.Response, error) @@ -1026,6 +1093,30 @@ func (c *Client) GetApiV1LicenseSeats(ctx context.Context, reqEditors ...Request return c.Client.Do(req) } +func (c *Client) PostApiV1LoginOauthCodeExchangeWithBody(ctx context.Context, contentType string, body io.Reader, reqEditors ...RequestEditorFn) (*http.Response, error) { + req, err := NewPostApiV1LoginOauthCodeExchangeRequestWithBody(c.Server, contentType, body) + if err != nil { + return nil, err + } + req = req.WithContext(ctx) + if err := c.applyEditors(ctx, req, reqEditors); err != nil { + return nil, err + } + return c.Client.Do(req) +} + +func (c *Client) PostApiV1LoginOauthCodeExchange(ctx context.Context, body PostApiV1LoginOauthCodeExchangeJSONRequestBody, reqEditors ...RequestEditorFn) (*http.Response, error) { + req, err := NewPostApiV1LoginOauthCodeExchangeRequest(c.Server, body) + if err != nil { + return nil, err + } + req = req.WithContext(ctx) + if err := c.applyEditors(ctx, req, reqEditors); err != nil { + return nil, err + } + return c.Client.Do(req) +} + func (c *Client) PostApiV1LoginOauthExchangeGoogleWithBody(ctx context.Context, contentType string, body io.Reader, reqEditors ...RequestEditorFn) (*http.Response, error) { req, err := NewPostApiV1LoginOauthExchangeGoogleRequestWithBody(c.Server, contentType, body) if err != nil { @@ -1050,6 +1141,30 @@ func (c *Client) PostApiV1LoginOauthExchangeGoogle(ctx context.Context, body Pos return c.Client.Do(req) } +func (c *Client) GetApiV1LoginOauthProviderCallback(ctx context.Context, provider GetApiV1LoginOauthProviderCallbackParamsProvider, params *GetApiV1LoginOauthProviderCallbackParams, reqEditors ...RequestEditorFn) (*http.Response, error) { + req, err := NewGetApiV1LoginOauthProviderCallbackRequest(c.Server, provider, params) + if err != nil { + return nil, err + } + req = req.WithContext(ctx) + if err := c.applyEditors(ctx, req, reqEditors); err != nil { + return nil, err + } + return c.Client.Do(req) +} + +func (c *Client) GetApiV1LoginOauthProviderStart(ctx context.Context, provider GetApiV1LoginOauthProviderStartParamsProvider, reqEditors ...RequestEditorFn) (*http.Response, error) { + req, err := NewGetApiV1LoginOauthProviderStartRequest(c.Server, provider) + if err != nil { + return nil, err + } + req = req.WithContext(ctx) + if err := c.applyEditors(ctx, req, reqEditors); err != nil { + return nil, err + } + return c.Client.Do(req) +} + func (c *Client) PostApiV1LoginOtpRequestWithBody(ctx context.Context, contentType string, body io.Reader, reqEditors ...RequestEditorFn) (*http.Response, error) { req, err := NewPostApiV1LoginOtpRequestRequestWithBody(c.Server, contentType, body) if err != nil { @@ -1749,6 +1864,46 @@ func NewGetApiV1LicenseSeatsRequest(server string) (*http.Request, error) { return req, nil } +// NewPostApiV1LoginOauthCodeExchangeRequest calls the generic PostApiV1LoginOauthCodeExchange builder with application/json body +func NewPostApiV1LoginOauthCodeExchangeRequest(server string, body PostApiV1LoginOauthCodeExchangeJSONRequestBody) (*http.Request, error) { + var bodyReader io.Reader + buf, err := json.Marshal(body) + if err != nil { + return nil, err + } + bodyReader = bytes.NewReader(buf) + return NewPostApiV1LoginOauthCodeExchangeRequestWithBody(server, "application/json", bodyReader) +} + +// NewPostApiV1LoginOauthCodeExchangeRequestWithBody generates requests for PostApiV1LoginOauthCodeExchange with any type of body +func NewPostApiV1LoginOauthCodeExchangeRequestWithBody(server string, contentType string, body io.Reader) (*http.Request, error) { + var err error + + serverURL, err := url.Parse(server) + if err != nil { + return nil, err + } + + operationPath := fmt.Sprintf("/api/v1/login/oauth/code/exchange") + if operationPath[0] == '/' { + operationPath = "." + operationPath + } + + queryURL, err := serverURL.Parse(operationPath) + if err != nil { + return nil, err + } + + req, err := http.NewRequest(http.MethodPost, queryURL.String(), body) + if err != nil { + return nil, err + } + + req.Header.Add("Content-Type", contentType) + + return req, nil +} + // NewPostApiV1LoginOauthExchangeGoogleRequest calls the generic PostApiV1LoginOauthExchangeGoogle builder with application/json body func NewPostApiV1LoginOauthExchangeGoogleRequest(server string, body PostApiV1LoginOauthExchangeGoogleJSONRequestBody) (*http.Request, error) { var bodyReader io.Reader @@ -1789,6 +1944,125 @@ func NewPostApiV1LoginOauthExchangeGoogleRequestWithBody(server string, contentT return req, nil } +// NewGetApiV1LoginOauthProviderCallbackRequest generates requests for GetApiV1LoginOauthProviderCallback +func NewGetApiV1LoginOauthProviderCallbackRequest(server string, provider GetApiV1LoginOauthProviderCallbackParamsProvider, params *GetApiV1LoginOauthProviderCallbackParams) (*http.Request, error) { + var err error + + var pathParam0 string + + pathParam0, err = runtime.StyleParamWithOptions("simple", false, "provider", provider, runtime.StyleParamOptions{ParamLocation: runtime.ParamLocationPath, Type: "string", Format: ""}) + if err != nil { + return nil, err + } + + serverURL, err := url.Parse(server) + if err != nil { + return nil, err + } + + operationPath := fmt.Sprintf("/api/v1/login/oauth/%s/callback", pathParam0) + if operationPath[0] == '/' { + operationPath = "." + operationPath + } + + queryURL, err := serverURL.Parse(operationPath) + if err != nil { + return nil, err + } + + if params != nil { + // queryValues collects non-styled parameters (passthrough, JSON) + // that are safe to round-trip through url.Values.Encode(). + queryValues := queryURL.Query() + // rawQueryFragments collects pre-encoded query fragments from + // styled parameters, preserving literal commas as delimiters + // per the OpenAPI spec (e.g. "color=blue,black,brown"). + var rawQueryFragments []string + + if params.Code != nil { + + if queryFrag, err := runtime.StyleParamWithOptions("form", true, "code", *params.Code, runtime.StyleParamOptions{ParamLocation: runtime.ParamLocationQuery, Type: "string", Format: ""}); err != nil { + return nil, err + } else { + for _, qp := range strings.Split(queryFrag, "&") { + rawQueryFragments = append(rawQueryFragments, qp) + } + } + + } + + if params.State != nil { + + if queryFrag, err := runtime.StyleParamWithOptions("form", true, "state", *params.State, runtime.StyleParamOptions{ParamLocation: runtime.ParamLocationQuery, Type: "string", Format: ""}); err != nil { + return nil, err + } else { + for _, qp := range strings.Split(queryFrag, "&") { + rawQueryFragments = append(rawQueryFragments, qp) + } + } + + } + + if params.Error != nil { + + if queryFrag, err := runtime.StyleParamWithOptions("form", true, "error", *params.Error, runtime.StyleParamOptions{ParamLocation: runtime.ParamLocationQuery, Type: "string", Format: ""}); err != nil { + return nil, err + } else { + for _, qp := range strings.Split(queryFrag, "&") { + rawQueryFragments = append(rawQueryFragments, qp) + } + } + + } + + if encoded := queryValues.Encode(); encoded != "" { + rawQueryFragments = append(rawQueryFragments, encoded) + } + queryURL.RawQuery = strings.Join(rawQueryFragments, "&") + } + + req, err := http.NewRequest(http.MethodGet, queryURL.String(), nil) + if err != nil { + return nil, err + } + + return req, nil +} + +// NewGetApiV1LoginOauthProviderStartRequest generates requests for GetApiV1LoginOauthProviderStart +func NewGetApiV1LoginOauthProviderStartRequest(server string, provider GetApiV1LoginOauthProviderStartParamsProvider) (*http.Request, error) { + var err error + + var pathParam0 string + + pathParam0, err = runtime.StyleParamWithOptions("simple", false, "provider", provider, runtime.StyleParamOptions{ParamLocation: runtime.ParamLocationPath, Type: "string", Format: ""}) + if err != nil { + return nil, err + } + + serverURL, err := url.Parse(server) + if err != nil { + return nil, err + } + + operationPath := fmt.Sprintf("/api/v1/login/oauth/%s/start", pathParam0) + if operationPath[0] == '/' { + operationPath = "." + operationPath + } + + queryURL, err := serverURL.Parse(operationPath) + if err != nil { + return nil, err + } + + req, err := http.NewRequest(http.MethodGet, queryURL.String(), nil) + if err != nil { + return nil, err + } + + return req, nil +} + // NewPostApiV1LoginOtpRequestRequest calls the generic PostApiV1LoginOtpRequest builder with application/json body func NewPostApiV1LoginOtpRequestRequest(server string, body PostApiV1LoginOtpRequestJSONRequestBody) (*http.Request, error) { var bodyReader io.Reader @@ -2990,11 +3264,22 @@ type ClientWithResponsesInterface interface { // GetApiV1LicenseSeatsWithResponse request GetApiV1LicenseSeatsWithResponse(ctx context.Context, reqEditors ...RequestEditorFn) (*GetApiV1LicenseSeatsResponse, error) + // PostApiV1LoginOauthCodeExchangeWithBodyWithResponse request with any body + PostApiV1LoginOauthCodeExchangeWithBodyWithResponse(ctx context.Context, contentType string, body io.Reader, reqEditors ...RequestEditorFn) (*PostApiV1LoginOauthCodeExchangeResponse, error) + + PostApiV1LoginOauthCodeExchangeWithResponse(ctx context.Context, body PostApiV1LoginOauthCodeExchangeJSONRequestBody, reqEditors ...RequestEditorFn) (*PostApiV1LoginOauthCodeExchangeResponse, error) + // PostApiV1LoginOauthExchangeGoogleWithBodyWithResponse request with any body PostApiV1LoginOauthExchangeGoogleWithBodyWithResponse(ctx context.Context, contentType string, body io.Reader, reqEditors ...RequestEditorFn) (*PostApiV1LoginOauthExchangeGoogleResponse, error) PostApiV1LoginOauthExchangeGoogleWithResponse(ctx context.Context, body PostApiV1LoginOauthExchangeGoogleJSONRequestBody, reqEditors ...RequestEditorFn) (*PostApiV1LoginOauthExchangeGoogleResponse, error) + // GetApiV1LoginOauthProviderCallbackWithResponse request + GetApiV1LoginOauthProviderCallbackWithResponse(ctx context.Context, provider GetApiV1LoginOauthProviderCallbackParamsProvider, params *GetApiV1LoginOauthProviderCallbackParams, reqEditors ...RequestEditorFn) (*GetApiV1LoginOauthProviderCallbackResponse, error) + + // GetApiV1LoginOauthProviderStartWithResponse request + GetApiV1LoginOauthProviderStartWithResponse(ctx context.Context, provider GetApiV1LoginOauthProviderStartParamsProvider, reqEditors ...RequestEditorFn) (*GetApiV1LoginOauthProviderStartResponse, error) + // PostApiV1LoginOtpRequestWithBodyWithResponse request with any body PostApiV1LoginOtpRequestWithBodyWithResponse(ctx context.Context, contentType string, body io.Reader, reqEditors ...RequestEditorFn) (*PostApiV1LoginOtpRequestResponse, error) @@ -3235,6 +3520,38 @@ func (r GetApiV1LicenseSeatsResponse) ContentType() string { return "" } +type PostApiV1LoginOauthCodeExchangeResponse struct { + Body []byte + HTTPResponse *http.Response + JSON200 *ApiauthmodelsTokenPairResponse + JSON401 *HttperrorsErrorResponse + JSON429 *HttperrorsErrorResponse +} + +// Status returns HTTPResponse.Status +func (r PostApiV1LoginOauthCodeExchangeResponse) Status() string { + if r.HTTPResponse != nil { + return r.HTTPResponse.Status + } + return http.StatusText(0) +} + +// StatusCode returns HTTPResponse.StatusCode +func (r PostApiV1LoginOauthCodeExchangeResponse) StatusCode() int { + if r.HTTPResponse != nil { + return r.HTTPResponse.StatusCode + } + return 0 +} + +// ContentType is a convenience method to retrieve the Content-Type value from the HTTP response headers +func (r PostApiV1LoginOauthCodeExchangeResponse) ContentType() string { + if r.HTTPResponse != nil { + return r.HTTPResponse.Header.Get("Content-Type") + } + return "" +} + type PostApiV1LoginOauthExchangeGoogleResponse struct { Body []byte HTTPResponse *http.Response @@ -3269,6 +3586,68 @@ func (r PostApiV1LoginOauthExchangeGoogleResponse) ContentType() string { return "" } +type GetApiV1LoginOauthProviderCallbackResponse struct { + Body []byte + HTTPResponse *http.Response + JSON429 *HttperrorsErrorResponse +} + +// Status returns HTTPResponse.Status +func (r GetApiV1LoginOauthProviderCallbackResponse) Status() string { + if r.HTTPResponse != nil { + return r.HTTPResponse.Status + } + return http.StatusText(0) +} + +// StatusCode returns HTTPResponse.StatusCode +func (r GetApiV1LoginOauthProviderCallbackResponse) StatusCode() int { + if r.HTTPResponse != nil { + return r.HTTPResponse.StatusCode + } + return 0 +} + +// ContentType is a convenience method to retrieve the Content-Type value from the HTTP response headers +func (r GetApiV1LoginOauthProviderCallbackResponse) ContentType() string { + if r.HTTPResponse != nil { + return r.HTTPResponse.Header.Get("Content-Type") + } + return "" +} + +type GetApiV1LoginOauthProviderStartResponse struct { + Body []byte + HTTPResponse *http.Response + JSON400 *HttperrorsErrorResponse + JSON429 *HttperrorsErrorResponse + JSON500 *HttperrorsErrorResponse +} + +// Status returns HTTPResponse.Status +func (r GetApiV1LoginOauthProviderStartResponse) Status() string { + if r.HTTPResponse != nil { + return r.HTTPResponse.Status + } + return http.StatusText(0) +} + +// StatusCode returns HTTPResponse.StatusCode +func (r GetApiV1LoginOauthProviderStartResponse) StatusCode() int { + if r.HTTPResponse != nil { + return r.HTTPResponse.StatusCode + } + return 0 +} + +// ContentType is a convenience method to retrieve the Content-Type value from the HTTP response headers +func (r GetApiV1LoginOauthProviderStartResponse) ContentType() string { + if r.HTTPResponse != nil { + return r.HTTPResponse.Header.Get("Content-Type") + } + return "" +} + type PostApiV1LoginOtpRequestResponse struct { Body []byte HTTPResponse *http.Response @@ -4191,6 +4570,23 @@ func (c *ClientWithResponses) GetApiV1LicenseSeatsWithResponse(ctx context.Conte return ParseGetApiV1LicenseSeatsResponse(rsp) } +// PostApiV1LoginOauthCodeExchangeWithBodyWithResponse request with arbitrary body returning *PostApiV1LoginOauthCodeExchangeResponse +func (c *ClientWithResponses) PostApiV1LoginOauthCodeExchangeWithBodyWithResponse(ctx context.Context, contentType string, body io.Reader, reqEditors ...RequestEditorFn) (*PostApiV1LoginOauthCodeExchangeResponse, error) { + rsp, err := c.PostApiV1LoginOauthCodeExchangeWithBody(ctx, contentType, body, reqEditors...) + if err != nil { + return nil, err + } + return ParsePostApiV1LoginOauthCodeExchangeResponse(rsp) +} + +func (c *ClientWithResponses) PostApiV1LoginOauthCodeExchangeWithResponse(ctx context.Context, body PostApiV1LoginOauthCodeExchangeJSONRequestBody, reqEditors ...RequestEditorFn) (*PostApiV1LoginOauthCodeExchangeResponse, error) { + rsp, err := c.PostApiV1LoginOauthCodeExchange(ctx, body, reqEditors...) + if err != nil { + return nil, err + } + return ParsePostApiV1LoginOauthCodeExchangeResponse(rsp) +} + // PostApiV1LoginOauthExchangeGoogleWithBodyWithResponse request with arbitrary body returning *PostApiV1LoginOauthExchangeGoogleResponse func (c *ClientWithResponses) PostApiV1LoginOauthExchangeGoogleWithBodyWithResponse(ctx context.Context, contentType string, body io.Reader, reqEditors ...RequestEditorFn) (*PostApiV1LoginOauthExchangeGoogleResponse, error) { rsp, err := c.PostApiV1LoginOauthExchangeGoogleWithBody(ctx, contentType, body, reqEditors...) @@ -4208,6 +4604,24 @@ func (c *ClientWithResponses) PostApiV1LoginOauthExchangeGoogleWithResponse(ctx return ParsePostApiV1LoginOauthExchangeGoogleResponse(rsp) } +// GetApiV1LoginOauthProviderCallbackWithResponse request returning *GetApiV1LoginOauthProviderCallbackResponse +func (c *ClientWithResponses) GetApiV1LoginOauthProviderCallbackWithResponse(ctx context.Context, provider GetApiV1LoginOauthProviderCallbackParamsProvider, params *GetApiV1LoginOauthProviderCallbackParams, reqEditors ...RequestEditorFn) (*GetApiV1LoginOauthProviderCallbackResponse, error) { + rsp, err := c.GetApiV1LoginOauthProviderCallback(ctx, provider, params, reqEditors...) + if err != nil { + return nil, err + } + return ParseGetApiV1LoginOauthProviderCallbackResponse(rsp) +} + +// GetApiV1LoginOauthProviderStartWithResponse request returning *GetApiV1LoginOauthProviderStartResponse +func (c *ClientWithResponses) GetApiV1LoginOauthProviderStartWithResponse(ctx context.Context, provider GetApiV1LoginOauthProviderStartParamsProvider, reqEditors ...RequestEditorFn) (*GetApiV1LoginOauthProviderStartResponse, error) { + rsp, err := c.GetApiV1LoginOauthProviderStart(ctx, provider, reqEditors...) + if err != nil { + return nil, err + } + return ParseGetApiV1LoginOauthProviderStartResponse(rsp) +} + // PostApiV1LoginOtpRequestWithBodyWithResponse request with arbitrary body returning *PostApiV1LoginOtpRequestResponse func (c *ClientWithResponses) PostApiV1LoginOtpRequestWithBodyWithResponse(ctx context.Context, contentType string, body io.Reader, reqEditors ...RequestEditorFn) (*PostApiV1LoginOtpRequestResponse, error) { rsp, err := c.PostApiV1LoginOtpRequestWithBody(ctx, contentType, body, reqEditors...) @@ -4731,6 +5145,46 @@ func ParseGetApiV1LicenseSeatsResponse(rsp *http.Response) (*GetApiV1LicenseSeat return response, nil } +// ParsePostApiV1LoginOauthCodeExchangeResponse parses an HTTP response from a PostApiV1LoginOauthCodeExchangeWithResponse call +func ParsePostApiV1LoginOauthCodeExchangeResponse(rsp *http.Response) (*PostApiV1LoginOauthCodeExchangeResponse, error) { + bodyBytes, err := io.ReadAll(rsp.Body) + defer func() { _ = rsp.Body.Close() }() + if err != nil { + return nil, err + } + + response := &PostApiV1LoginOauthCodeExchangeResponse{ + Body: bodyBytes, + HTTPResponse: rsp, + } + + switch { + case strings.Contains(rsp.Header.Get("Content-Type"), "json") && rsp.StatusCode == 200: + var dest ApiauthmodelsTokenPairResponse + if err := json.Unmarshal(bodyBytes, &dest); err != nil { + return nil, err + } + response.JSON200 = &dest + + case strings.Contains(rsp.Header.Get("Content-Type"), "json") && rsp.StatusCode == 401: + var dest HttperrorsErrorResponse + if err := json.Unmarshal(bodyBytes, &dest); err != nil { + return nil, err + } + response.JSON401 = &dest + + case strings.Contains(rsp.Header.Get("Content-Type"), "json") && rsp.StatusCode == 429: + var dest HttperrorsErrorResponse + if err := json.Unmarshal(bodyBytes, &dest); err != nil { + return nil, err + } + response.JSON429 = &dest + + } + + return response, nil +} + // ParsePostApiV1LoginOauthExchangeGoogleResponse parses an HTTP response from a PostApiV1LoginOauthExchangeGoogleWithResponse call func ParsePostApiV1LoginOauthExchangeGoogleResponse(rsp *http.Response) (*PostApiV1LoginOauthExchangeGoogleResponse, error) { bodyBytes, err := io.ReadAll(rsp.Body) @@ -4785,6 +5239,72 @@ func ParsePostApiV1LoginOauthExchangeGoogleResponse(rsp *http.Response) (*PostAp return response, nil } +// ParseGetApiV1LoginOauthProviderCallbackResponse parses an HTTP response from a GetApiV1LoginOauthProviderCallbackWithResponse call +func ParseGetApiV1LoginOauthProviderCallbackResponse(rsp *http.Response) (*GetApiV1LoginOauthProviderCallbackResponse, error) { + bodyBytes, err := io.ReadAll(rsp.Body) + defer func() { _ = rsp.Body.Close() }() + if err != nil { + return nil, err + } + + response := &GetApiV1LoginOauthProviderCallbackResponse{ + Body: bodyBytes, + HTTPResponse: rsp, + } + + switch { + case strings.Contains(rsp.Header.Get("Content-Type"), "json") && rsp.StatusCode == 429: + var dest HttperrorsErrorResponse + if err := json.Unmarshal(bodyBytes, &dest); err != nil { + return nil, err + } + response.JSON429 = &dest + + } + + return response, nil +} + +// ParseGetApiV1LoginOauthProviderStartResponse parses an HTTP response from a GetApiV1LoginOauthProviderStartWithResponse call +func ParseGetApiV1LoginOauthProviderStartResponse(rsp *http.Response) (*GetApiV1LoginOauthProviderStartResponse, error) { + bodyBytes, err := io.ReadAll(rsp.Body) + defer func() { _ = rsp.Body.Close() }() + if err != nil { + return nil, err + } + + response := &GetApiV1LoginOauthProviderStartResponse{ + Body: bodyBytes, + HTTPResponse: rsp, + } + + switch { + case strings.Contains(rsp.Header.Get("Content-Type"), "json") && rsp.StatusCode == 400: + var dest HttperrorsErrorResponse + if err := json.Unmarshal(bodyBytes, &dest); err != nil { + return nil, err + } + response.JSON400 = &dest + + case strings.Contains(rsp.Header.Get("Content-Type"), "json") && rsp.StatusCode == 429: + var dest HttperrorsErrorResponse + if err := json.Unmarshal(bodyBytes, &dest); err != nil { + return nil, err + } + response.JSON429 = &dest + + case strings.Contains(rsp.Header.Get("Content-Type"), "json") && rsp.StatusCode == 500: + var dest HttperrorsErrorResponse + if err := json.Unmarshal(bodyBytes, &dest); err != nil { + return nil, err + } + response.JSON500 = &dest + + } + + return response, nil +} + // ParsePostApiV1LoginOtpRequestResponse parses an HTTP response from a PostApiV1LoginOtpRequestWithResponse call func ParsePostApiV1LoginOtpRequestResponse(rsp *http.Response) (*PostApiV1LoginOtpRequestResponse, error) { bodyBytes, err := io.ReadAll(rsp.Body) From bfd04ebaa343ea3aaad787d7b2153d42256ecb14 Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Tue, 8 Sep 2026 00:00:41 +0300 Subject: [PATCH 11/11] docs(agents): rule out plumbing tests and configuration logging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Route registration, config predicates and logging resolved configuration back at the operator kept getting written and kept getting deleted by hand. Test the output — the pure functions — and log what was done, not what was said. Co-Authored-By: Claude Opus 5 --- .agents/project/conventions.md | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/.agents/project/conventions.md b/.agents/project/conventions.md index 99402fe..913f860 100644 --- a/.agents/project/conventions.md +++ b/.agents/project/conventions.md @@ -165,6 +165,30 @@ The async task queue is goque. Tasks are registered by type in must key by user and therefore must sit after the token gate), which is not observable in either layer alone. +- Do not write tests about configuration plumbing. Same principle as route + wiring, one layer down: a test that builds an `AppConfig` literal and asserts + a predicate over it restates the predicate in a second syntax. That includes + tables enumerating which combinations of keys switch a feature on, and tests + that a defaulted value defaults. + + A misconfigured instance announces itself the moment it runs: the feature is + off, the endpoint 404s, the process refuses to boot. Test the *derivation* + that has somewhere to be wrong -- a URL rewritten into a cookie scope, a path + prefix stripped by a proxy -- as a pure function on its inputs, in the package + that owns it. `absoluteURL(FrontendURL)` is worth pinning; `Enabled() == true + when both keys are set` is not. + +- Do not add logging to describe configuration back to the operator. A startup + line that prints a value the operator just typed into a file they are looking + at tells them nothing they cannot read faster from the file, and it goes stale + silently when the key is renamed. Log what the process *did* -- a connection + established, a processor registered, a credential resolved from the store -- + not what it was told to do. + + The exception is a value the process *derived* and the operator cannot + predict: a resolved hostname, a computed path, a generated password. If the + operator can grep it out of a config file, it does not need a log line. + ## Generated Code - Do not manually edit generated files unless the generator output itself is