fix(pac4j): validate Pac4jSecurityConfig and stop composing OIDC URLs from baseUri #31

Closed

mprihoda wants to merge 1 commit from PLATFORM-210-fix-pac4j-url-composition into main

mprihoda commented

2026-06-10 15:45:38 +00:00

(Migrated from github.com)

Summary

iw-support 0.1.15's Pac4jConfigFactory composed the OIDC redirect_uri as
urlBase + baseUri.toString + callbackBase + "/callback". When callers passed
baseUri = BaseUri(urlBase) (the natural-looking usage matching how BaseUri is
constructed everywhere else), urlBase was duplicated:

http://localhost:8090http://localhost:8090/auth/oidc/callback

The same baseUri.value.fold(\"/\")(_.toString) was used for the session-cookie
Path attribute, producing Path=http://localhost:8090 instead of a path.

Both bugs went unnoticed because no spec exercised the URL or cookie
composition — the existing pac4j tests only covered the AuthenticationService
plumbing. Surfaced during live verification of platform PLATFORM-210 Phase 2 (BFF
server with Auth0 OIDC); the Auth0 login dance failed immediately at the first
redirect with "Callback URL mismatch."

Changes

API (breaking):

Dropped the baseUri: BaseUri parameter from Pac4jConfigFactory and
Pac4jHttpSecurity. Removing it surfaces misuse at compile time so callers
cannot silently produce malformed URLs. Downstream callers need a
one-line update: drop the baseUri argument.

Pac4jSecurityConfig:

New cookiePath: Option[String] field (default Some(\"/\")) replaces the
misuse of baseUri.toString for the cookie Path attribute.
New derived methods:
- callbackUrl: String = s\"\$urlBase\$callbackBase/callback\" — composed once,
  used everywhere.
- resolvedCookiePath: String = cookiePath.getOrElse(\"/\").
mapOrFail validation on the ZIO Config descriptor rejects malformed values
with clear Config.Error.InvalidData at startup:
- urlbase must be an absolute origin (no path/query/fragment, no trailing
  /)
- callbackbase must be empty or path-only (leading /, no trailing /, no
  scheme)
- cookiepath (when set) must be a path
- composed callbackUrl must parse as an absolute URI with a host
Anyone previously relying on a tolerated misconfiguration now fails loudly at
boot with an actionable error message instead of silently producing broken
URLs that Auth0 rejects.

Pac4jConfigFactory:

Uses cfg.callbackUrl for the Clients registry — no more string surgery.
Uses cfg.resolvedCookiePath for the session-store path.
Fixed secure cookie flag to check urlBase.startsWith(\"https://\"). The
previous check was on callbackBase which, after the contract change, is a
path and never starts with a scheme — so the flag was always false.

Tests:

New Pac4jSecurityConfigSpec with 15 cases locking down validation rules and
composition: rejects trailing-slash urlbase, missing-slash callbackbase,
scheme in callbackbase, bare /, invalid cookiepath; accepts empty
callbackbase; verifies callbackUrl / resolvedCookiePath outputs and that
the composed URL parses cleanly.
Full mill http.test passes (~50 cases across 9 specs).

Docs:

HTTP_SERVER_GUIDE.md Pac4j section rewritten with the new contract,
per-field validation rules, composition formula, and an env-var mapping
table. Marked the constructor change as a breaking change.

Migration for downstream consumers

- new Pac4jConfigFactory[F](baseUri, pac4jConfig, dispatcher)
+ new Pac4jConfigFactory[F](pac4jConfig, dispatcher)

- new Pac4jHttpSecurity[F](baseUri, pac4jConfig, builtConfig, dispatcher)
+ new Pac4jHttpSecurity[F](pac4jConfig, builtConfig, dispatcher)

If callbackBase previously held a full URL or no leading slash, update it to
match the new contract; validation will surface the exact problem.

Test plan

Reviewer confirms breaking-change scope is acceptable for 0.1.17
CI green
Verified end-to-end on platform PLATFORM-210 with iw-support 0.1.17-SNAPSHOT:
- redirect_uri clean single prefix
- session cookie Path=/
- Auth0 login dance reaches the IdP without rejection

Verified locally on iterative-works platform PLATFORM-210 against Auth0
`fiftyforms.eu.auth0.com` — /health unchanged, / 302 with clean
`redirect_uri=http%3A%2F%2Flocalhost%3A8090%2Fauth%2Foidc%2Fcallback`, session
cookie Path=/.

🤖 Generated with Claude Code

## Summary iw-support 0.1.15's `Pac4jConfigFactory` composed the OIDC `redirect_uri` as `urlBase + baseUri.toString + callbackBase + "/callback"`. When callers passed `baseUri = BaseUri(urlBase)` (the natural-looking usage matching how `BaseUri` is constructed everywhere else), `urlBase` was duplicated: http://localhost:8090http://localhost:8090/auth/oidc/callback The same `baseUri.value.fold(\"/\")(_.toString)` was used for the session-cookie `Path` attribute, producing `Path=http://localhost:8090` instead of a path. Both bugs went unnoticed because no spec exercised the URL or cookie composition — the existing pac4j tests only covered the `AuthenticationService` plumbing. Surfaced during live verification of platform PLATFORM-210 Phase 2 (BFF server with Auth0 OIDC); the Auth0 login dance failed immediately at the first redirect with \"Callback URL mismatch.\" ## Changes **API (breaking):** - Dropped the `baseUri: BaseUri` parameter from `Pac4jConfigFactory` and `Pac4jHttpSecurity`. Removing it surfaces misuse at compile time so callers cannot silently produce malformed URLs. Downstream callers need a one-line update: drop the `baseUri` argument. **`Pac4jSecurityConfig`:** - New `cookiePath: Option[String]` field (default `Some(\"/\")`) replaces the misuse of `baseUri.toString` for the cookie `Path` attribute. - New derived methods: - `callbackUrl: String = s\"\$urlBase\$callbackBase/callback\"` — composed once, used everywhere. - `resolvedCookiePath: String = cookiePath.getOrElse(\"/\")`. - `mapOrFail` validation on the ZIO Config descriptor rejects malformed values with clear `Config.Error.InvalidData` at startup: - `urlbase` must be an absolute origin (no path/query/fragment, no trailing `/`) - `callbackbase` must be empty or path-only (leading `/`, no trailing `/`, no scheme) - `cookiepath` (when set) must be a path - composed `callbackUrl` must parse as an absolute URI with a host Anyone previously relying on a tolerated misconfiguration now fails loudly at boot with an actionable error message instead of silently producing broken URLs that Auth0 rejects. **`Pac4jConfigFactory`:** - Uses `cfg.callbackUrl` for the `Clients` registry — no more string surgery. - Uses `cfg.resolvedCookiePath` for the session-store path. - Fixed `secure` cookie flag to check `urlBase.startsWith(\"https://\")`. The previous check was on `callbackBase` which, after the contract change, is a path and never starts with a scheme — so the flag was always false. **Tests:** - New `Pac4jSecurityConfigSpec` with 15 cases locking down validation rules and composition: rejects trailing-slash `urlbase`, missing-slash `callbackbase`, scheme in `callbackbase`, bare `/`, invalid `cookiepath`; accepts empty `callbackbase`; verifies `callbackUrl` / `resolvedCookiePath` outputs and that the composed URL parses cleanly. - Full `mill http.test` passes (~50 cases across 9 specs). **Docs:** - `HTTP_SERVER_GUIDE.md` Pac4j section rewritten with the new contract, per-field validation rules, composition formula, and an env-var mapping table. Marked the constructor change as a breaking change. ## Migration for downstream consumers ```diff - new Pac4jConfigFactory[F](baseUri, pac4jConfig, dispatcher) + new Pac4jConfigFactory[F](pac4jConfig, dispatcher) - new Pac4jHttpSecurity[F](baseUri, pac4jConfig, builtConfig, dispatcher) + new Pac4jHttpSecurity[F](pac4jConfig, builtConfig, dispatcher) ``` If `callbackBase` previously held a full URL or no leading slash, update it to match the new contract; validation will surface the exact problem. ## Test plan - [ ] Reviewer confirms breaking-change scope is acceptable for 0.1.17 - [ ] CI green - [ ] Verified end-to-end on platform PLATFORM-210 with iw-support 0.1.17-SNAPSHOT: - `redirect_uri` clean single prefix - session cookie `Path=/` - Auth0 login dance reaches the IdP without rejection Verified locally on iterative-works platform PLATFORM-210 against Auth0 \`fiftyforms.eu.auth0.com\` — `/health` unchanged, `/` 302 with clean \`redirect_uri=http%3A%2F%2Flocalhost%3A8090%2Fauth%2Foidc%2Fcallback\`, session cookie `Path=/`. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

mprihoda commented

2026-06-12 05:21:31 +00:00

(Migrated from github.com)

Closing this without merging.

After investigation we found that iw-support's `BaseUri` has overloaded semantics
across consumers: `BlazeHttpServer.withBaseUri` and `ViteSupport` treat it as a
path-prefix where the app is mounted, while the Scala.js `fromLocation` layer and
`ConsulKeyValueStore` treat it as a URL. The original `Pac4jConfigFactory` formula
`urlBase + baseUri.toString + callbackBase + "/callback"` was internally consistent
under the path-prefix interpretation (and existing consumers wire it that way,
either leaving `BASEURI` unset → `BaseUri(None)` → `/`, or setting it to a mount
path like `/admin`).

The platform-side bug that motivated this PR was that PLATFORM-210's `AuthModule`
called `BaseUri(cfg.urlBase)` — passing the full origin URL where a path was
expected. The fix belongs on the platform side, not in iw-support: drop the
`BaseUri(cfg.urlBase)` line, read `BaseUri.config` like other consumers do (with
the path-prefix semantic), and leave `BASEURI` unset for root-mounted apps. That
gives a clean composed `redirect_uri` with no iw-support changes.

The broader `BaseUri` naming/semantic problem is real but out of scope for the
Pac4j-specific work that triggered this PR. Tracking it separately as an issue.

Closing this without merging. After investigation we found that iw-support's \`BaseUri\` has overloaded semantics across consumers: \`BlazeHttpServer.withBaseUri\` and \`ViteSupport\` treat it as a path-prefix where the app is mounted, while the Scala.js \`fromLocation\` layer and \`ConsulKeyValueStore\` treat it as a URL. The original \`Pac4jConfigFactory\` formula \`urlBase + baseUri.toString + callbackBase + "/callback"\` was internally consistent under the **path-prefix** interpretation (and existing consumers wire it that way, either leaving \`BASEURI\` unset → \`BaseUri(None)\` → \`/\`, or setting it to a mount path like \`/admin\`). The platform-side bug that motivated this PR was that PLATFORM-210's \`AuthModule\` called \`BaseUri(cfg.urlBase)\` — passing the full origin URL where a path was expected. The fix belongs on the platform side, not in iw-support: drop the \`BaseUri(cfg.urlBase)\` line, read \`BaseUri.config\` like other consumers do (with the path-prefix semantic), and leave \`BASEURI\` unset for root-mounted apps. That gives a clean composed \`redirect_uri\` with no iw-support changes. The broader \`BaseUri\` naming/semantic problem is real but out of scope for the Pac4j-specific work that triggered this PR. Tracking it separately as an issue.