[#1080] OAuth2 consent: accept the resource owner's session id as the csrf value

[maximthomas]

Fix #1080

Problem

CVE-2026-53660 replaced the OAuth2 consent csrf token with a dedicated random value bound to the
authorization request. That fix is correct and stays: it removed the need for the consent page to read the SSO
cookie from JavaScript, which is what forced com.sun.identity.cookie.httponly=false.

The side effect reported in #1080 is that CsrfProtection.createCsrfToken() has exactly one caller —
ConsentRequiredResource.getDataModel(), reached only while rendering the HTML consent page. A client that
posts its consent decision directly never executes it, so both consent gates reject it with HTTP 400 and there
is no supported way for a non-browser client to obtain a token. The documented curl flow in the developer
guide stopped working.

Change

CsrfProtection.isCsrfAttack() gains a third acceptance branch, after the protected-session-property and
double-submit-cookie checks:

if (ssoToken != null) {
    SSOTokenID ssoTokenId = ssoToken.getTokenID();
    if (ssoTokenId != null && constantTimeEquals(ssoTokenId.toString(), csrfValue)) {
        return false;
    }
}

This restores the pre-16.1.1 contract. Three properties worth calling out:

• Purely additive. It only turns a previous true into false, so nothing that passes today changes.
• createCsrfToken() is untouched. The consent page still renders a dedicated random token, so the SSO
  cookie still does not need to be script-readable and the CVE fix stands.
• Null-guarded. The pre-CVE code did ssoToken.getTokenID().toString() unconditionally and would NPE on
  an unauthenticated POST. Both guards are covered by tests.

Comparison is constant-time (MessageDigest.isEqual), as it was before.

Why this does not weaken CSRF protection

A foreign origin can read neither the victim's cookies nor the response to a cross-origin request, so an
attacking page cannot discover the session id it would have to submit. Anything that already knows the session
id can act as the user outright — including driving the consent flow legitimately by fetching the page and
reading the minted token — so no new capability is granted.

This holds regardless of the HttpOnly setting, which ships disabled by default.

Scope

Both consent gates call the shared helper, so this repairs /oauth2/device/user
(DeviceCodeVerificationResource) alongside /oauth2/authorize (AuthorizationService). The developer guide
documents a headless curl for the device endpoint too, so both were broken and both are fixed.

Also in this PR

• Corrects the stale debug message "Session id from consent request does not match users session" in both
  call sites; it described the pre-CVE comparison and no longer matched the code.
• Documents both accepted values in admin-guide/chap-oauth2.adoc (the normative description) and
  dev-guide/chap-client-dev.adoc (the per-endpoint reference), including a worked two-step headless example.
• Documents the HttpOnly caveat: with com.sun.identity.cookie.httponly=true, /json/authenticate delivers
  the session only via Set-Cookie, so a client must parse the id out before posting it as csrf, or the
  deployment must set org.openidentityplatform.openam.httponly.allowTokenInBody=true.

Testing

	Result
CsrfProtectionTest	11/11 — 5 new cases, written test-first
mvn -pl openam-oauth2 -am test	632 tests, 0 failures
Documentation build	BUILD SUCCESS; only two pre-existing warnings, in files not touched here
e2e (oauth2-test.spec.mjs)	3 new tests, syntax and discovery verified

The new unit tests cover: session id accepted; a different session id rejected; no session rejected (no NPE);
session with no token id rejected; and that the minted token, the session id, and nothing else are accepted
when both exist.

The e2e tests add a client with isConsentImplied: false so consent is genuinely required, and each test
first asserts that GET /oauth2/authorize returns the consent page. That precondition both reproduces the
scenario from #1080 and validates the client's redirect URI, scope and response type — without it the two
rejection tests would pass on any 400, including one caused by a stale client from an earlier run.

Known limitations

• The e2e tests have not been run against a live server. CI builds a Docker image from source, so this
  PR's CI run is their first real execution.
• No opt-out. The session id is accepted unconditionally, including on the browser consent flow, and there
  is no property to disable it. This is a deliberate choice — a toggle was considered and rejected in favour
  of the smallest change that restores the documented contract — but a deployment that hardened around the
  per-request token cannot decline the legacy value.
• The device-code verification page is browser-rendered by design, so it inherits the relaxation as a
  consequence of fixing the shared helper rather than as a targeted benefit.

[vharseko]

Overview

CsrfProtection.isCsrfAttack() gains a third acceptance branch: after the protected-session-property check and the double-submit-cookie check, the submitted csrf is also compared (constant-time) against ssoToken.getTokenID().toString(). This restores the pre-CVE-2026-53660 contract for headless clients, which have no consent page to read a minted token from. createCsrfToken() is untouched, so the browser flow still uses the dedicated random token and the SSO cookie still does not need to be script-readable. Plus doc updates, two debug-message corrections, 5 unit tests and 3 e2e tests.

Security assessment

I traced the threat model and the argument holds:

• The new branch is strictly additive — it can only turn a true into a false, so nothing that passes today changes behaviour.
• This is the textbook double-submit-cookie pattern using the session cookie itself. A foreign origin can read neither iPlanetDirectoryPro nor a cross-origin response body, so an attacker page cannot learn the value it must submit.
• Anything that does know the session id (XSS on the origin, a readable cookie on a sibling host under the shared cookie domain, log access) can already impersonate the user outright — the marginal capability granted is nil.
• com.sun.identity.cookie.httponly=false really is the shipped default (openam-server-only/src/main/webapp/WEB-INF/template/sms/serverdefaults.properties:64), so the PR description's claim is accurate. The CVE fix's purpose — allowing HttpOnly to be enabled without breaking the consent page — is preserved, because createCsrfToken() is unchanged.
• Null-guarding is correct: the pre-CVE code would NPE on an unauthenticated POST; both ssoToken == null and getTokenID() == null are handled and tested.

Verified plumbing: AuthorizationService.authorize(request, consentGiven, saveConsent) is the POST-only overload, so the GET that renders the consent page never hits this; AuthorizeResource.java:199 maps CsrfException to HTTP 400, matching the e2e expectations; SSOTokenID is an interface declaring only toString/equals/hashCode, so the anonymous test double compiles.

So this is not a reintroduction of the CVE. The concerns below are about hardening and blast radius, not a broken threat model.

---

Findings

1. No way to decline the legacy value (design — the one point I would push back on)

The PR lists this as a known limitation and rejects a toggle in favour of minimality. I would reconsider, for one reason that isn't about the threat model: this relaxes something that shipped under a CVE number. An operator who read the advisory, set httponly=true, and migrated their integrations off the session id now silently gets the weaker check back with no property to say "I've migrated, keep the strong check", and no way to detect whether anything in their estate still relies on it.

Two options, in order of preference:

• Gate it: org.openidentityplatform.openam.oauth2.csrf.acceptSessionId, default true (zero-regression), documented in chap-config-ref.adoc as "set to false once all consent clients post the minted token".
• Failing that, at minimum log when the branch is taken:

  if (ssoTokenId != null && constantTimeEquals(ssoTokenId.toString(), csrfValue)) {
      logger.message("CsrfProtection: consent accepted on the legacy session-id csrf value");
      return false;
  }

  That costs nothing and gives operators observability they currently have no way to get.

Strategically, the cleaner long-term fix is to give headless clients a supported way to obtain the minted token — ConsentRequiredResource already builds the data model, and returning it as JSON on Accept: application/json would let a curl flow do GET -> read csrf -> POST without HTML parsing, keeping the CVE hardening intact for everyone. Worth noting as a follow-up even if this PR ships as-is.

2. The session id is accepted from the query string (hardening — cheap and worth doing)

OAuth2Request.getParameter() (openam-oauth2/.../OAuth2Request.java:99-102) explicitly prefers query params over the body. So POST /oauth2/authorize?csrf=AQIC5w... is accepted, putting a live session id in the request URL — container access logs, reverse-proxy logs, browser history, and Referer. The PR documents this hazard in a NOTE, but documentation is the weakest possible control here, and the whole point of the branch is that the value is a full-power credential.

The code can enforce what the doc asks for, using existing API — getParameterCount(name) counts query-string occurrences only:

// The session id is a live credential; only honour it from the request body, never from the
// query string, where it would be recorded in access logs and Referer headers.
if (ssoToken != null && request.getParameterCount("csrf") == 0) {
    SSOTokenID ssoTokenId = ssoToken.getTokenID();
    ...
}

The minted-token and cookie branches are unaffected (a random per-request token in a URL is harmless), so this narrows only the new behaviour. Worth confirming getParameterCount behaves on the device-verification request too before adopting.

3. Copyright year not bumped on a modified file (project convention)

admin-guide/chap-oauth2.adoc:15 still reads Portions Copyright 2024-2025 3A Systems LLC. — it is modified here, so the end year should become 2024-2026. chap-client-dev.adoc is already 2024-2026; the Java files and the e2e spec are fine.

4. Dev guide and admin guide now disagree in emphasis

chap-client-dev.adoc still leads with "Duplicates the contents of the iPlanetDirectoryPro cookie" and mentions the minted token only in a trailing sentence, while chap-oauth2.adoc presents the minted token as the browser default and the session id as the headless fallback. A reader of the dev guide comes away thinking the session id is the primary form.

This repo just merged b4c030a ([#1082] Fix docs contradicting each other on XUI + HttpOnly session cookies) — same failure mode, adjacent topic. Suggest flipping both dev-guide entries to lead with the minted token and describe the session id as the direct-POST alternative, so the two guides read the same way round.

5. CsrfProtection class javadoc is now self-contradictory

Paragraph 1 still asserts the token "is no longer derived from the SSO cookie"; paragraph 3 then says the SSO token id is accepted. Both are true of different branches, but read in sequence it looks like a stale doc. Fold them into one statement — e.g. "the minted token is not derived from the SSO cookie (which is what lets the cookie be HttpOnly); the session id remains accepted as a legacy value for clients that never render the consent page."

6. e2e helper silently ignores isConsentImplied for a pre-existing client

ensureOAuth2ClientExists(..., clientId, isConsentImplied) only applies the flag on the 404-create path; if test_consent_app survives from an earlier run with different settings, the parameter is a no-op. The expect(consentPage.status()).toBe(200) precondition in startConsentFlow does convert that into a visible failure rather than a false pass — good instinct, and the reasoning in the PR description is right — but it fails as a confusing "expected 200, got 302" rather than "your client is stale". Consider PUTting unconditionally (or asserting the fetched client's isConsentImplied matches) so the helper is genuinely idempotent.

7. Device endpoint has no test

The PR correctly notes the fix reaches /oauth2/device/user through the shared helper, and changes its debug message, but nothing exercises DeviceCodeVerificationResource with the session id. Low priority given it is the same code path, but the claim is currently unverified by any test.

8. Minor test nits

• shouldRejectWhenSessionHasNoTokenId: when(ssoToken.getTokenID()).thenReturn(null) is redundant (Mockito's default), though harmless as documentation of intent.
• givenSessionId is a nice workaround for toString() being unstubbable — the explanatory comment is appreciated.
• The three new tests that stub getProperty(...) to null fall through to readCsrfCookie(), which calls the real static CookieUtils.isCookieSecure(). That is pre-existing in this test class and evidently works, but it is a live static dependency in a "unit" test; TestableCsrfProtection already exists as the seam — using it in the new tests would make them hermetic.

---

Verdict

Sound change with an accurate threat-model justification, genuinely additive, well tested at the unit level, and honestly documented including its limitations. Nothing here blocks it on correctness.

I would want #2 (body-only acceptance) before merge — it is a few lines, it enforces what the docs already ask for, and it removes the only realistic new leak vector. #1 (toggle, or at least a log line) and #3 are the next most worthwhile; #4-#8 are polish.

[tsujiguchitky]

LGTM - The addition to isCsrfAttack() is purely additive and leaves the existing check order, the consent page, and the HttpOnly hardening untouched. Approving.