Review fixes: denylist-conditional revoke, Ory secret wiring, exp guard test
CI / full-gate (push) Successful in 2m40s
CI / full-gate (push) Successful in 2m40s
This commit is contained in:
@@ -854,6 +854,10 @@ blocks a clean clone:
|
|||||||
| `SECRETS_SYSTEM` | hydra env | encrypts OAuth2 tokens + consent at rest |
|
| `SECRETS_SYSTEM` | hydra env | encrypts OAuth2 tokens + consent at rest |
|
||||||
| `POSTGRES_USER` / `POSTGRES_PASSWORD` | compose env | the Ory databases (default `ory`/`ory`) |
|
| `POSTGRES_USER` / `POSTGRES_PASSWORD` | compose env | the Ory databases (default `ory`/`ory`) |
|
||||||
|
|
||||||
|
`CSRF_SECRET` and the Postgres pair are interpolated from the host environment. The three
|
||||||
|
Ory secrets are **not**: `compose.yml` passes only `DSN` to `kratos`/`hydra`, so add them to
|
||||||
|
those services' `environment:` (or an `env_file:`) or they silently stay on the throwaways.
|
||||||
|
|
||||||
2. **SSO provider client id/secret** — **optional**; password login works without them.
|
2. **SSO provider client id/secret** — **optional**; password login works without them.
|
||||||
Supplying a provider's creds via env activates it; no creds ⇒ no SSO button (see
|
Supplying a provider's creds via env activates it; no creds ⇒ no SSO button (see
|
||||||
[Social sign-in (SSO)](#social-sign-in-sso)).
|
[Social sign-in (SSO)](#social-sign-in-sso)).
|
||||||
@@ -1017,12 +1021,12 @@ what defends what, and which guarantees are deliberately not offered.
|
|||||||
- **The browser is not trusted.** Cookies, form fields, URLs and headers are attacker-controlled
|
- **The browser is not trusted.** Cookies, form fields, URLs and headers are attacker-controlled
|
||||||
until verified or escaped. Nothing is believed because of where it arrived from.
|
until verified or escaped. Nothing is believed because of where it arrived from.
|
||||||
- **The session JWT is trusted only after verification** — signature against the JWKS key its
|
- **The session JWT is trusted only after verification** — signature against the JWKS key its
|
||||||
`kid` names (or the sole key, when the set has one), then a **mandatory** `exp`, plus `nbf`
|
`kid` names (or the sole key, when the token carries no `kid`), then a **mandatory** `exp`, plus `nbf`
|
||||||
and the optional `iss`/`aud`. Before that it is bytes.
|
and the optional `iss`/`aud`. Before that it is bytes.
|
||||||
- **The private container network is the *only* thing guarding the Ory APIs.** Kratos admin
|
- **The private container network is the *only* thing guarding the Ory APIs.** Kratos admin
|
||||||
(`4434`), Hydra admin (`4445`) and Keto write (`4467`) authenticate no one — reaching them
|
(`4434`), Hydra admin (`4445`) and Keto write (`4467`) authenticate no one — reaching them
|
||||||
*is* full identity and permission control. Keto **read** (`4466`) cannot write, but discloses
|
*is* full identity and permission control. Keto **read** (`4466`) cannot write, but discloses
|
||||||
the entire authorization graph, so treat it the same. `compose.yml` publishes none of the six
|
the entire authorization graph, so treat it the same. `compose.yml` publishes none of the six Ory ports
|
||||||
(guarded by `src/compose.test.ts`); dev publishes only the two Ory ports a browser must reach.
|
(guarded by `src/compose.test.ts`); dev publishes only the two Ory ports a browser must reach.
|
||||||
Never expose one, and never front one with a proxy that lacks its own auth.
|
Never expose one, and never front one with a proxy that lacks its own auth.
|
||||||
- **Plugins are trusted code**, in-process and unsandboxed — a plugin can do anything the host
|
- **Plugins are trusted code**, in-process and unsandboxed — a plugin can do anything the host
|
||||||
@@ -1045,7 +1049,7 @@ Never put anything in a claim you wouldn't show them.
|
|||||||
| Token minted for another deployment | optional `JWT_ISSUER` / `JWT_AUDIENCE` pinning |
|
| Token minted for another deployment | optional `JWT_ISSUER` / `JWT_AUDIENCE` pinning |
|
||||||
| Stolen session cookie | `HttpOnly`, `SameSite=Lax`, `Secure` (`SECURE_COOKIES`) — but see the real session lifetime below |
|
| Stolen session cookie | `HttpOnly`, `SameSite=Lax`, `Secure` (`SECURE_COOKIES`) — but see the real session lifetime below |
|
||||||
| CSRF on our own forms | signed double-submit token, **opt-in per handler** via `ctx.verifyCsrf` (`src/auth/csrf.ts`) + `SameSite=Lax`; Kratos' flows carry Kratos' own token |
|
| CSRF on our own forms | signed double-submit token, **opt-in per handler** via `ctx.verifyCsrf` (`src/auth/csrf.ts`) + `SameSite=Lax`; Kratos' flows carry Kratos' own token |
|
||||||
| XSS | EJS `<%= %>` escapes; CSP `script-src 'self'` with no `'unsafe-inline'` (`src/http/security-headers.ts`) — the `*.html` slots stay [raw by contract](#routes--handlers) |
|
| XSS | EJS `<%= %>` escapes; the CSP blocks inline script ([headers](#production--deployment)) — the `*.html` slots stay [raw by contract](#escaping--the-trust-boundary) |
|
||||||
| Clickjacking | `frame-ancestors 'none'` + `X-Frame-Options: DENY` |
|
| Clickjacking | `frame-ancestors 'none'` + `X-Frame-Options: DENY` |
|
||||||
| Open redirect via `return_to` | validated host-relative (`localPath`, `src/http/safe-url.ts`) |
|
| Open redirect via `return_to` | validated host-relative (`localPath`, `src/http/safe-url.ts`) |
|
||||||
| Privilege escalation | roles authored only in Keto, re-read at every mint — see [login & the session JWT](#login-and-the-session-jwt) |
|
| Privilege escalation | roles authored only in Keto, re-read at every mint — see [login & the session JWT](#login-and-the-session-jwt) |
|
||||||
@@ -1060,11 +1064,13 @@ two cookies obey `SECURE_COOKIES`; the Kratos one takes its flags from Kratos' o
|
|||||||
**Fail closed — with one deliberate exception.** A token that cannot be verified (missing,
|
**Fail closed — with one deliberate exception.** A token that cannot be verified (missing,
|
||||||
malformed, bad signature, wrong `iss`/`aud`) yields *anonymous*, never a partly-trusted user,
|
malformed, bad signature, wrong `iss`/`aud`) yields *anonymous*, never a partly-trusted user,
|
||||||
and anonymous or under-privileged is denied (`requireSession` bounces to `/login`; `can`/`check`
|
and anonymous or under-privileged is denied (`requireSession` bounces to `/login`; `can`/`check`
|
||||||
return `false`). An **expired or revoked** token instead triggers a re-mint against the live
|
return `false`). An **expired** token instead triggers a re-mint: re-validation against the live
|
||||||
Kratos session — full re-authentication with roles re-read from Keto, or a cleared cookie if
|
Kratos session, roles re-read from Keto, or a cleared cookie if that session is dead; Ory
|
||||||
that session is dead; Ory unreachable ⇒ anonymous. Revoking a role therefore *downgrades* a
|
unreachable ⇒ anonymous. None of this is a session kill — a *revoked* state exists only with the
|
||||||
user promptly without signing them out; **ending a session outright means deactivating or
|
[denylist](#instant-revoke-the-optional-denylist) on (off by default), and it resolves through
|
||||||
deleting the identity.**
|
that same re-mint. **Offboarding:** with the denylist on, revoking a role downgrades the user at
|
||||||
|
once and deactivating or deleting the identity ends the session; with it off, both land within
|
||||||
|
one token TTL.
|
||||||
|
|
||||||
**Not guaranteed** — accepted, and stated where each mechanism is: role changes
|
**Not guaranteed** — accepted, and stated where each mechanism is: role changes
|
||||||
[lag up to one token TTL and sign-in needs Ory up](#two-trade-offs--both-deliberate), and the
|
[lag up to one token TTL and sign-in needs Ory up](#two-trade-offs--both-deliberate), and the
|
||||||
@@ -1474,8 +1480,7 @@ container-relative; with the dev bind-mount they edit the real file).
|
|||||||
2. **Restart Kratos** so it signs with the new first key: `docker compose restart kratos`.
|
2. **Restart Kratos** so it signs with the new first key: `docker compose restart kratos`.
|
||||||
(web needs no restart — it hot-reloads the file. The hot path verifies JWTs locally, so a
|
(web needs no restart — it hot-reloads the file. The hot path verifies JWTs locally, so a
|
||||||
brief Kratos blip only touches login/re-mint.)
|
brief Kratos blip only touches login/re-mint.)
|
||||||
3. **Verify** new logins mint the new `kid` — decode the `plainpages_jwt` cookie
|
3. **Verify** new logins mint the new `kid` — decode the `plainpages_jwt` cookie's JWT header, or watch web's logs for a `jwks reload on kid miss` debug line as old clients
|
||||||
header, or watch web's logs for a `jwks reload on kid miss` debug line as old clients
|
|
||||||
present the new key.
|
present the new key.
|
||||||
4. **Wait ~12 min**, then **prune** the superseded key:
|
4. **Wait ~12 min**, then **prune** the superseded key:
|
||||||
```bash
|
```bash
|
||||||
|
|||||||
@@ -29,8 +29,10 @@ test("verifyToken: a valid token → User, selecting the verify key by kid acros
|
|||||||
assert.deepEqual(user, { email: "a@b.c", id: "u1", roles: ["admin"] });
|
assert.deepEqual(user, { email: "a@b.c", id: "u1", roles: ["admin"] });
|
||||||
});
|
});
|
||||||
|
|
||||||
test("verifyToken rejects expiry and future nbf, with clock-skew leeway", async () => {
|
test("verifyToken requires exp, rejects expiry and future nbf, with clock-skew leeway", async () => {
|
||||||
const opts = { clockSkewSec: 60, now: NOW };
|
const opts = { clockSkewSec: 60, now: NOW };
|
||||||
|
// No exp ⇒ rejected outright: an exp-less token must never read as eternal.
|
||||||
|
await assert.rejects(verifyToken(mint(k1.privateKey, "k1", { ...valid, exp: undefined }), jwks, opts), /missing exp/);
|
||||||
await assert.rejects(verifyToken(mint(k1.privateKey, "k1", { ...valid, exp: NOW - 120 }), jwks, opts), /expired/);
|
await assert.rejects(verifyToken(mint(k1.privateKey, "k1", { ...valid, exp: NOW - 120 }), jwks, opts), /expired/);
|
||||||
// exp 30s in the past but inside the 60s skew → still accepted.
|
// exp 30s in the past but inside the 60s skew → still accepted.
|
||||||
await verifyToken(mint(k1.privateKey, "k1", { ...valid, exp: NOW - 30 }), jwks, opts);
|
await verifyToken(mint(k1.privateKey, "k1", { ...valid, exp: NOW - 30 }), jwks, opts);
|
||||||
|
|||||||
@@ -15,7 +15,7 @@
|
|||||||
- [x] CI/CD - When renovate updates a dependency - also release a new version of plainpages based on what got updated with Renovate. Major typescript? New apiVersion + new major. A tiny patch to ejs? Only patch release etc. Before implementing, explain in detail how you will solve this. (`renovate.yml` gains an `auto-release` job (`needs: renovate`) that cuts one `vX.Y.Z` tag per run for what Renovate merged; level = highest `Release-Bump:` trailer Renovate stamps via `commitBody`, any dep's major/minor/patch mapped straight through (default patch). Decoupled from `apiVersion` (tag-only, `HOST_API_VERSION` untouched — a "major" is just a bigger image tag, never a plugin break); pre-1.0 shifts down so nothing auto-crosses into 1.0.0. Pure `auto-release/next-version.ts` + unit tests; tag pushed with renovate-bot's PAT so `release.yml` fires; documented in README → CI/CD.)
|
- [x] CI/CD - When renovate updates a dependency - also release a new version of plainpages based on what got updated with Renovate. Major typescript? New apiVersion + new major. A tiny patch to ejs? Only patch release etc. Before implementing, explain in detail how you will solve this. (`renovate.yml` gains an `auto-release` job (`needs: renovate`) that cuts one `vX.Y.Z` tag per run for what Renovate merged; level = highest `Release-Bump:` trailer Renovate stamps via `commitBody`, any dep's major/minor/patch mapped straight through (default patch). Decoupled from `apiVersion` (tag-only, `HOST_API_VERSION` untouched — a "major" is just a bigger image tag, never a plugin break); pre-1.0 shifts down so nothing auto-crosses into 1.0.0. Pure `auto-release/next-version.ts` + unit tests; tag pushed with renovate-bot's PAT so `release.yml` fires; documented in README → CI/CD.)
|
||||||
- [x] Add an e2e test for the admin plugin's OAuth2-clients (Hydra) screen. The full-flow e2e suite runs without Hydra (compose.full.yml), so /admin/clients register/detail/delete is only unit-covered (src/http/app.test.ts); wire Hydra into an e2e stack and drive the screen in the browser. (compose.full.yml now includes Hydra (`serve all --dev`) and full-flow.spec.ts drives /admin/clients register → one-time secret → list → detail → delete in the browser; documented in README → Testing.)
|
- [x] Add an e2e test for the admin plugin's OAuth2-clients (Hydra) screen. The full-flow e2e suite runs without Hydra (compose.full.yml), so /admin/clients register/detail/delete is only unit-covered (src/http/app.test.ts); wire Hydra into an e2e stack and drive the screen in the browser. (compose.full.yml now includes Hydra (`serve all --dev`) and full-flow.spec.ts drives /admin/clients register → one-time secret → list → detail → delete in the browser; documented in README → Testing.)
|
||||||
- [x] Build and publish docker image as CI/CD. (Duplicate of the CI/CD items above: `ci.yml` builds and pushes `gitea.larvit.se/larvit/plainpages:<commit hash>` behind the green gate, `release.yml` re-tags it to semver and syncs those tags to Docker Hub.)
|
- [x] Build and publish docker image as CI/CD. (Duplicate of the CI/CD items above: `ci.yml` builds and pushes `gitea.larvit.se/larvit/plainpages:<commit hash>` behind the green gate, `release.yml` re-tags it to semver and syncs those tags to Docker Hub.)
|
||||||
- [x] The human developer understands the security model in the auth in this project. (README → Auth → [Security model](README.md#security-model): trust boundaries — browser untrusted, JWT untrusted until verified, the private network as the *only* guard on the unauthenticated Ory admin APIs, plugins trusted and unsandboxed, row rules upstream — plus a threat→defense table, the fail-closed rule, and pointers to the limits that are deliberately not guaranteed. Signed-not-encrypted is called out so nothing secret lands in a claim, and the JWT's ~10m TTL is separated from the 30-day Kratos session that re-mints it. Every row of the threat table is enforced by an existing test; the trust-boundary bullets are not testable claims. Review also corrected the hardening checklist — `REQUIRE_SECURE_SECRETS` guards only `CSRF_SECRET`, so the committed Kratos/Hydra/Postgres dev secrets are now listed in "What you must supply".)
|
- [x] The human developer understands the security model in the auth in this project. (README → Auth → [Security model](README.md#security-model): trust boundaries — browser untrusted, JWT untrusted until verified, the private network as the *only* guard on the unauthenticated Ory admin APIs, plugins trusted and unsandboxed, row rules upstream — plus a threat→defense table, the fail-closed rule, and pointers to the limits that are deliberately not guaranteed. Signed-not-encrypted is called out so nothing secret lands in a claim, and the JWT's ~10m TTL is separated from the 30-day Kratos session that re-mints it. Every row of the threat table is enforced by a test — the mandatory-`exp` guard was the one gap, now asserted in `src/auth/jwt-middleware.test.ts`; the trust-boundary bullets are not testable claims. Review also corrected the hardening checklist — `REQUIRE_SECURE_SECRETS` guards only `CSRF_SECRET`, so the committed Kratos/Hydra/Postgres dev secrets are now listed in "What you must supply".)
|
||||||
- [ ] Add i18n support.
|
- [ ] Add i18n support.
|
||||||
|
|
||||||
## Architectural review findings (2026-07-02)
|
## Architectural review findings (2026-07-02)
|
||||||
|
|||||||
Reference in New Issue
Block a user