pac4j session security: empty CSRF token accepted, session cookie never Secure, session map never evicted #63

New issue

Open

opened 2026-09-22 18:50:22 +00:00 by claude · 0 comments

claude commented

2026-09-22 18:50:22 +00:00

Member

Item 1: `ConstantTimeCsrfAuthorizer` should reject an empty request token

In pac4j-core 6.3.2, CsrfAuthorizer accepts a request token that equals either the current token or the previous one, and it reads an absent previous token as "".

With the stock DefaultCsrfTokenGenerator, no previous token exists before the session's first matched request. An empty pac4jCsrfToken therefore passes the check on that first request — which is exactly the request a cross-site attacker can make, because sending no token at all is the easy case.

MEDECA closes this in its own generator by writing previous = current, so a comparison against the previous token is never a comparison against "". That fix belongs in the library, not in each consumer.

Proposal: ConstantTimeCsrfAuthorizer rejects an empty or blank request token before any comparison. Add a test that sends an empty pac4jCsrfToken as the first matched request of a fresh session and expects a rejection.

Item 2: the `pac4jSessionId` cookie can never be `Secure`

Pac4jConfigFactory.scala:35 sets

secure = callbackBase.startsWith("https://")

but callbackBase is always a path, never a URL. Pac4jHttpSecurity.route mounts it as Router(s"${config.callbackBase}" -> ...), so the value is of the form /callback. The condition is therefore false in every deployment, including production behind TLS, and the session cookie is sent over plain HTTP whenever anything downgrades the connection.

This is a concrete security consequence of the naming problem recorded in #32 (BaseUri overloaded as path-prefix and as URL), but it can be fixed on its own.

Proposal: take the flag from explicit configuration rather than deriving it from a path, defaulting to true. A deployment that genuinely serves plain HTTP opts out.

Item 3: the in-memory pac4j session map never evicts

CacheSessionRepository.scala:10 in http4s-pac4j holds sessions in a plain map with no expiry and no size bound. A long-running server accumulates one entry per session forever.

This is upstream code, not iw-support's, so the fix here is a SessionRepository implementation with a TTL (and the option of a shared store), offered beside the in-memory one — not a patch to the dependency.

Proposal: ship an evicting SessionRepository in server/http and name it in docs/CSRF_GUIDE.md as the one to use for anything that runs longer than a development session.

Found while enabling pac4j CSRF in a downstream HTMX app (MEDECA-348). Three independent items, all in the pac4j session/CSRF area. Each can be fixed without the others. Companion to #62, which covers the `matchers`/`authorizers` hook and the 4-hour token expiry. ## Item 1: `ConstantTimeCsrfAuthorizer` should reject an empty request token In pac4j-core 6.3.2, `CsrfAuthorizer` accepts a request token that equals either the current token or the **previous** one, and it reads an absent previous token as `""`. With the stock `DefaultCsrfTokenGenerator`, no previous token exists before the session's first matched request. An empty `pac4jCsrfToken` therefore passes the check on that first request — which is exactly the request a cross-site attacker can make, because sending no token at all is the easy case. MEDECA closes this in its own generator by writing previous = current, so a comparison against the previous token is never a comparison against `""`. That fix belongs in the library, not in each consumer. **Proposal:** `ConstantTimeCsrfAuthorizer` rejects an empty or blank request token before any comparison. Add a test that sends an empty `pac4jCsrfToken` as the first matched request of a fresh session and expects a rejection. ## Item 2: the `pac4jSessionId` cookie can never be `Secure` `Pac4jConfigFactory.scala:35` sets ```scala secure = callbackBase.startsWith("https://") ``` but `callbackBase` is always a **path**, never a URL. `Pac4jHttpSecurity.route` mounts it as `Router(s"${config.callbackBase}" -> ...)`, so the value is of the form `/callback`. The condition is therefore false in every deployment, including production behind TLS, and the session cookie is sent over plain HTTP whenever anything downgrades the connection. This is a concrete security consequence of the naming problem recorded in #32 (`BaseUri` overloaded as path-prefix and as URL), but it can be fixed on its own. **Proposal:** take the flag from explicit configuration rather than deriving it from a path, defaulting to `true`. A deployment that genuinely serves plain HTTP opts out. ## Item 3: the in-memory pac4j session map never evicts `CacheSessionRepository.scala:10` in `http4s-pac4j` holds sessions in a plain map with no expiry and no size bound. A long-running server accumulates one entry per session forever. This is upstream code, not iw-support's, so the fix here is a `SessionRepository` implementation with a TTL (and the option of a shared store), offered beside the in-memory one — not a patch to the dependency. **Proposal:** ship an evicting `SessionRepository` in `server/http` and name it in `docs/CSRF_GUIDE.md` as the one to use for anything that runs longer than a development session.