fix(auth): validate every OIDC provider response at the boundary #216

Sammanfogat

supernaut sammanfogade 2 incheckningar från fix/validate-oidc-boundary in i main

2026-08-12 19:31:47 +00:00

supernaut kommenterad

2026-08-12 19:08:31 +00:00

Ägare

The security review in #36 listed three controls as follow-up work. None of them are in main.
This is that follow-up. Two commits.

Validate every OIDC provider response at the boundary

The discovery document, the JWKS, the token-endpoint response, the id_token header and claims,
and the userinfo body were each parsed and then asserted into a type nobody had checked
(as Discovery). An assertion is not a check. It makes the code read as though the shape were
known while leaving it as unknown as before, in the path that decides who is logged in.

All five are now validated at runtime, once, at the edge, with zod (src/lib/auth/oidc-schemas.ts).
oidc.ts works only with the validated results. The exported types are inferred from the schemas,
so the two cannot drift.

Schemas are strict about the fields the client depends on and lenient about the rest. Unknown
fields are dropped rather than rejected, since the live provider document carries 24 fields and 5
are read. A JWKS entry the client cannot use is skipped during key selection instead of failing the
whole set. Failures fail closed, naming the offending field but never its value, because these
messages are logged and the values include tokens. Bodies are read as text and parsed here rather
than via res.json(), whose SyntaxError quotes a fragment of the body.

Three fail-open defects the validation exposed, fixed here:

userinfo_endpoint was assumed present, though the spec makes it only RECOMMENDED. A provider
omitting it would have produced a fetch against undefined mid-login. Discovery now rejects such
a document up front.
A non-finite id_token exp never expired. typeof exp === "number" admits Infinity, and
Infinity * 1000 <= now is false, so 1e999 passed the expiry check for ever.
A session cookie whose payload lacked exp never expired either, for the same reason. The HMAC
proves a cookie was minted with our secret but says nothing about its shape. The payload is now
shape-checked after the MAC check. A mismatch yields no session, exactly like a bad signature.

Endpoints from the discovery document must now be absolute https URLs, with http allowed only on a
loopback host for a locally-run provider. The client secret travels to token_endpoint and the
access token to userinfo_endpoint.

Read environment values as checked strings

The last untyped values feeding logic came from import.meta.env, which Astro types with an any
index signature. Every key it does not know about arrives as any and flows on unchecked. One had
.trim() called on it, another was handed to encodeURIComponent, another to
TextEncoder.encode as an HMAC secret.

src/lib/env.ts now does that check once. envString reads process.env first and falls back to
import.meta.env for server-only runtime values. stringOr takes the value rather than a key, so
consts.ts keeps the static import.meta.env.PUBLIC_* access Vite needs to inline at build time.
An empty PUBLIC_* value now falls back to the default rather than being used verbatim.

With that, the four no-unsafe-* rules go back on and gate the repo. The last of 39 findings are
gone, and gone because the values are checked, not because a type assertion claimed they were.

Review notes

Adds one runtime dependency: zod 4.4.3, MIT.
docs/design/oidc-security-review.md is updated. The three follow-up rows now read as done.
Tests feed a malformed or hostile payload to each boundary. One asserts that a validation error
over a credential-bearing body does not echo the credential. The ES256/EC P-256 path the live
provider actually signs with is covered.
The feature stays inert until OIDC_* and SESSION_SECRET are set and a bitborg-web OAuth2
client exists in Kanidm. Merging this exposes nothing on its own.

Verified

pnpm test: 361 passed, up 56 from main's 305
pnpm check: 0 errors, 0 warnings
pnpm eslint, pnpm stylelint, pnpm format:check, pnpm mdlint, pnpm lang-check: all clean

Closes #36

The security review in #36 listed three controls as follow-up work. None of them are in `main`. This is that follow-up. Two commits. ## Validate every OIDC provider response at the boundary The discovery document, the JWKS, the token-endpoint response, the id_token header and claims, and the userinfo body were each parsed and then asserted into a type nobody had checked (`as Discovery`). An assertion is not a check. It makes the code read as though the shape were known while leaving it as unknown as before, in the path that decides who is logged in. All five are now validated at runtime, once, at the edge, with zod (`src/lib/auth/oidc-schemas.ts`). `oidc.ts` works only with the validated results. The exported types are inferred from the schemas, so the two cannot drift. Schemas are strict about the fields the client depends on and lenient about the rest. Unknown fields are dropped rather than rejected, since the live provider document carries 24 fields and 5 are read. A JWKS entry the client cannot use is skipped during key selection instead of failing the whole set. Failures fail closed, naming the offending field but never its value, because these messages are logged and the values include tokens. Bodies are read as text and parsed here rather than via `res.json()`, whose SyntaxError quotes a fragment of the body. Three fail-open defects the validation exposed, fixed here: - `userinfo_endpoint` was assumed present, though the spec makes it only RECOMMENDED. A provider omitting it would have produced a fetch against `undefined` mid-login. Discovery now rejects such a document up front. - A non-finite id_token `exp` never expired. `typeof exp === "number"` admits Infinity, and `Infinity * 1000 <= now` is false, so `1e999` passed the expiry check for ever. - A session cookie whose payload lacked `exp` never expired either, for the same reason. The HMAC proves a cookie was minted with our secret but says nothing about its shape. The payload is now shape-checked after the MAC check. A mismatch yields no session, exactly like a bad signature. Endpoints from the discovery document must now be absolute https URLs, with http allowed only on a loopback host for a locally-run provider. The client secret travels to `token_endpoint` and the access token to `userinfo_endpoint`. ## Read environment values as checked strings The last untyped values feeding logic came from `import.meta.env`, which Astro types with an `any` index signature. Every key it does not know about arrives as `any` and flows on unchecked. One had `.trim()` called on it, another was handed to `encodeURIComponent`, another to `TextEncoder.encode` as an HMAC secret. `src/lib/env.ts` now does that check once. `envString` reads `process.env` first and falls back to `import.meta.env` for server-only runtime values. `stringOr` takes the value rather than a key, so `consts.ts` keeps the static `import.meta.env.PUBLIC_*` access Vite needs to inline at build time. An empty `PUBLIC_*` value now falls back to the default rather than being used verbatim. With that, the four `no-unsafe-*` rules go back on and gate the repo. The last of 39 findings are gone, and gone because the values are checked, not because a type assertion claimed they were. ## Review notes - Adds one runtime dependency: `zod` 4.4.3, MIT. - `docs/design/oidc-security-review.md` is updated. The three follow-up rows now read as done. - Tests feed a malformed or hostile payload to each boundary. One asserts that a validation error over a credential-bearing body does not echo the credential. The ES256/EC P-256 path the live provider actually signs with is covered. - The feature stays inert until `OIDC_*` and `SESSION_SECRET` are set and a `bitborg-web` OAuth2 client exists in Kanidm. Merging this exposes nothing on its own. ## Verified - `pnpm test`: 361 passed, up 56 from main's 305 - `pnpm check`: 0 errors, 0 warnings - `pnpm eslint`, `pnpm stylelint`, `pnpm format:check`, `pnpm mdlint`, `pnpm lang-check`: all clean Closes #36

supernaut lade till 2 incheckningar

2026-08-12 19:08:31 +00:00

fix(auth): validate every OIDC provider response at the boundary 37114216e8

The discovery document, the JWKS, the token-endpoint response, the id_token
header and claims, and the userinfo body were each parsed and then asserted
into a type nobody had checked (`as Discovery`). An assertion is not a check:
it makes the code read as though the shape were known while leaving it exactly
as unknown as before, in the path that decides who is logged in.

All five are now validated at runtime, once, at the edge, with zod
(src/lib/auth/oidc-schemas.ts); oidc.ts works only with the validated results,
and the exported types are inferred from the schemas so the two cannot drift.
Schemas are strict about the fields the client depends on and lenient about
everything else — unknown fields are dropped rather than rejected (the live
provider document carries 24 fields, 5 of which are read), and a JWKS entry the
client cannot use is skipped during key selection instead of failing the whole
set. Failures fail closed naming the offending field, never its value, since
these messages are logged and the values include tokens. Bodies are read as
text and parsed here rather than via res.json(), whose SyntaxError quotes a
fragment of the body.

Three fail-open defects the validation exposed, fixed here:

- userinfo_endpoint was assumed present, though the spec makes it only
  RECOMMENDED. A provider omitting it would have produced a fetch against
  `undefined` mid-login; discovery now rejects such a document up front.
- A non-finite id_token `exp` never expired: `typeof exp === "number"` admits
  Infinity, and `Infinity * 1000 <= now` is false, so `1e999` passed the expiry
  check for ever.
- A session cookie whose payload lacked `exp` never expired either, for the
  same reason. The HMAC proves a cookie was minted with our secret but says
  nothing about its shape, so the payload is now shape-checked after the MAC
  check; a mismatch yields no session, exactly like a bad signature.

Endpoints from the discovery document must now be absolute https URLs (http
only on a loopback host, for a locally-run provider): the client secret travels
to token_endpoint and the access token to userinfo_endpoint.

Tests feed a malformed or hostile payload to each boundary, assert that a
validation error over a credential-bearing body does not echo the credential,
and cover the ES256/EC P-256 path that the live provider actually signs with.

refactor(env): read environment values as checked strings, gate no-unsafe-*

ci / ci (pull_request) Successful in 1m25s

Detaljer

d93c0714fb

The last untyped values feeding logic came from `import.meta.env`, which Astro
types with an `any` index signature: every key it does not know about arrives as
`any` and flows on unchecked — `.trim()` called on one, another handed to
encodeURIComponent, another to TextEncoder.encode as an HMAC secret. src/lib/env.ts
now does that check once. `envString` reads process.env first and falls back to
import.meta.env for server-only runtime values; `stringOr` takes the value rather
than a key, so consts.ts keeps the static `import.meta.env.PUBLIC_*` access Vite
needs to inline at build time. An empty PUBLIC_* value now falls back to the
default rather than being used verbatim.

Also narrows a post-Array.isArray element read to `unknown`, so the string check
guarding it does real work instead of being vacuous against `any[]`.

With that, the four no-unsafe-* rules go back on and gate the repo — the last of
39 findings are gone, and gone because the values are checked, not because a type
assertion claimed they were. The reasoning recorded where they were switched off
is replaced with a note on where the checking now happens, and on the fact that a
future finding means an unchecked value, not a rule to disable.

supernaut refererade till denna ändringsförfrågan

2026-08-12 19:10:11 +00:00

Control panel: JWKS id_token verification + security review #36

supernaut sammanfogade incheckning 1e02af5282 till main

2026-08-12 19:31:47 +00:00

supernaut tog bort grenen fix/validate-oidc-boundary

2026-08-12 19:31:47 +00:00

supernaut refererade denna ändringsförfrågan från en incheckning

2026-08-12 19:31:49 +00:00

fix(auth): validate every OIDC provider response at the boundary (#216)

supernaut refererade till denna ändringsförfrågan

2026-08-12 19:43:02 +00:00