Skip to content

Enable HttpOnly session cookies by default - #1059

Open
vharseko wants to merge 3 commits into
OpenIdentityPlatform:masterfrom
vharseko:features/httponly
Open

vharseko wants to merge 3 commits into
OpenIdentityPlatform:masterfrom
vharseko:features/httponly

Conversation

@vharseko

@vharseko vharseko commented Jun 20, 2026 •

Copy link
Copy Markdown
Member

Summary

Flip the default of com.sun.identity.cookie.httponly from false to true so OpenAM ships HttpOnly SSO/session cookies out of the box. The HttpOnly mechanism (server + XUI) already exists in the codebase — this PR makes it the default and updates the docs, tests, and e2e/CI matrix accordingly.

Background / Why

  • HttpOnly stops scripts from reading the SSO token via document.cookie, mitigating token theft through XSS.
  • The default used to be false because the XUI once read the token from document.cookie. The XUI now fully supports HttpOnly (it relies on the auto-sent cookie and an in-memory sentinel token), so the old "HttpOnly breaks XUI console login" rationale no longer applies.

Changes

Default flip (core)

  • openam-shared CookieUtils — default to true when the property is unset, via getAsBoolean(AM_COOKIE_HTTPONLY, true); isCookieHttpOnly() javadoc updated.
  • serverdefaults.properties — ship com.sun.identity.cookie.httponly=true; rewrite the stale comment (document the allowTokenInBody opt-in / opt-out).

IDP Discovery (separate war)

  • CookieUtils — default HttpOnly to true (null/empty → on, explicit false → off).
  • Configurator.jsp — default the "HTTP-Only Cookie" radio to True.

MCP server (openam-mcp-server)

  • AuthInterceptor — with HttpOnly on, /json/authenticate no longer returns tokenId in the body, so the MCP server read a null token. It now takes the token from tokenId in the body when present, otherwise from the last non-empty iPlanetDirectoryPro (openam.tokenHeader) Set-Cookie, and fails with a clear error when neither is there. Covers both the username/password and the OAuth path.
  • AuthInterceptorTest — HttpOnly-mode tests for both paths via MockRestServiceServer, plus body precedence, a clearing cookie and the no-token case.
  • README — allowTokenInBody is not needed; set OPENAM_TOKEN_HEADER if the session cookie is renamed.

Tests

  • RestAuthenticationHandlerTest — establish the token-readable baseline (setCookieHttpOnly(false)) in @BeforeMethod so legacy assertions stay independent of the production default and of test order.

Docs (asciidoc only)

  • admin-guide/chap-securing, deployment-planning/chap-deployments, reference/chap-config-ref (Default → true), dev-guide/chap-client-dev (serverinfo example).

CI / e2e (.github/workflows/build.yml, e2e/)

  • Invert the Playwright phases: test the new HttpOnly=true default first (XUI specs), then override to false via setenv.sh and run the full suite (oauth2/saml read tokenId from the body, which is suppressed in HttpOnly mode).
  • Auth steps read the SSO token from the iPlanetDirectoryPro Set-Cookie header (instead of jq .tokenId), picking the last non-empty value so a clearing (empty) Set-Cookie cannot win.
  • Verify successful logins via successUrl in the response body (present on every completed authentication, in both modes) instead of grep tokenId.
  • saml-test.spec.mjs — fix the now-incorrect "HttpOnly breaks XUI" comment; openam-commons.mjs — document that getAuthToken needs the token in the body.

Behavioral changes / migration

  • New installs and environments without an explicit value now get HttpOnly cookies. Existing installs are unchanged (the value is already persisted at install time).
  • In HttpOnly mode, /json/authenticate no longer echoes tokenId in the response body by default. The in-repo MCP server already handles this (reads the session cookie). Non-browser / raw-REST integrations that need it can set org.openidentityplatform.openam.httponly.allowTokenInBody=true, or disable HttpOnly entirely with com.sun.identity.cookie.httponly=false.

Testing

  • openam-shared and openam-idpdiscovery compile (offline).
  • openam-mcp-server: 30/30 tests pass; the new HttpOnly tests fail against the previous AuthInterceptor.
  • build.yml YAML validated; Set-Cookie token extraction and successUrl checks verified locally (incl. empty/clearing cookie and failed-auth cases).
  • The xui-httponly e2e spec covers login / logout / reload / step-up in both modes; the CI matrix now exercises HttpOnly on (default) and off.

A word from someone who's been bitten by cookie flags before 🪧

This one's green in CI, but HttpOnly-by-default is exactly the kind of change that passes every test and still surprises someone in prod. Before signing off, kick the tires on the things automation won't catch:

  • Real browser, not just curl. Log into the XUI console, then hard-refresh the page. The session has to survive a reload with zero JS access to the cookie — that's the whole point, and it's the first thing that breaks.
  • Fresh install vs. upgrade. The default only changes new installs; existing deployments keep whatever's already persisted. Verify both, and don't assume an upgraded box flipped.
  • Your non-browser clients will notice. Anything scraping tokenId out of the /json/authenticate body now gets nothing. That's by design — confirm the escape hatch (allowTokenInBody=true) actually brings it back.
  • The cross-domain stuff. SAML SP/IDP, CDSSO, policy agents, step-up/session upgrade — exercise at least one real flow end-to-end.
  • Mind the edges. Reverse proxy / load balancer (amlbcookie), HTTP vs HTTPS, and the Secure/SameSite interplay.

Green checkmarks tell you it didn't break the way we expected. Testing is how you find the way we didn't. 👀

Reference

@tsujiguchitky tsujiguchitky left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - enabling HttpOnly by default is the right call, and the change is consistent across code, defaults, CI, tests, and docs. Approving.

@vharseko vharseko added the security Security fix or hardening (CVE, GHSA, XSS/CSRF/SSRF) label Jul 8, 2026
@vharseko

Copy link
Copy Markdown
Member Author

Rebased/merged master into this branch to pick up recent changes — in particular #1084, which had independently reworded the same HttpOnly documentation/comment block (still defaulting to false) while this PR was open. Resolved the conflict by keeping this PR's "default true" rationale and escape hatches (allowTokenInBody, disabling via false), while preserving the two caveats #1084 added that this PR didn't have yet: the property is read once at server startup (needs a restart to take effect), and the OIDC session-management iframe (/oauth2/connect/checkSession) can't observe the session while HttpOnly is enabled. Same treatment in serverdefaults.properties, chap-securing.adoc, and chap-deployments.adoc. No functional changes beyond the doc/comment text — re-requesting review since the branch moved.

@tsujiguchitky

Copy link
Copy Markdown
Contributor

Thanks for all the work on this! I have to apologize for coming back with something new. In my earlier review, I didn't find any blockers, but I noticed an issue that I think should be addressed before merging.

openam-mcp-server gets its SSO token only from the /json/authenticate response body. AuthInterceptor#getUserNamePasswordToken and #getTokenIdFromAccessToken both do tokenResponse.get("tokenId"), and the RestClient has no cookie handling. RestAuthenticationHandler only puts tokenId in the body when !isCookieHttpOnly() || isHttpOnlyAllowTokenInBody(). So with the new defaults (HttpOnly on, allowTokenInBody off), a freshly installed OpenAM no longer returns it, and the MCP server ends up with a null token.

Could we have the MCP server read iPlanetDirectoryPro from the Set-Cookie header, the same way the CI step now does, with a test for the HttpOnly-mode response? If you'd rather handle that separately, then at least documenting in the MCP server README that allowTokenInBody=true is required would help. Thanks again!

Switch the default of com.sun.identity.cookie.httponly from false to true so
that OpenAM marks its SSO/session cookies HttpOnly out of the box. The XUI
already supports this mode (relies on the auto-sent cookie instead of reading
the token from document.cookie), so the previous "breaks XUI" rationale no
longer applies.

Core:
- CookieUtils (openam-shared): default to true when the property is unset, via
  getAsBoolean(AM_COOKIE_HTTPONLY, true); update isCookieHttpOnly() javadoc.
- serverdefaults.properties: ship com.sun.identity.cookie.httponly=true and
  rewrite the stale comment (document the allowTokenInBody opt-in / opt-out).

IDP Discovery:
- CookieUtils: default HttpOnly to true (null/empty -> on, explicit false -> off).
- Configurator.jsp: default the "HTTP-Only Cookie" radio to True.

CI / e2e:
- build.yml: invert the Playwright phases — test the new HttpOnly=true default
  first (xui specs), then override to false via setenv.sh and run the full suite
  (oauth2/saml read tokenId from the response body, suppressed in HttpOnly mode).
- saml-test.spec.mjs: fix the now-incorrect "HttpOnly breaks XUI" comment.
- openam-commons.mjs: document that getAuthToken needs the token in the body.

Tests:
- RestAuthenticationHandlerTest: set the token-readable baseline
  (setCookieHttpOnly(false)) in @BeforeMethod so legacy assertions are
  independent of the production default and test order.

Docs (asciidoc): update default to true and the serverinfo example in
chap-securing, chap-deployments, chap-config-ref, chap-client-dev.
With HttpOnly enabled by default, /json/authenticate no longer echoes the
tokenId in the response body, so the Docker/e2e steps that scraped it broke.

- Extract the admin SSO token from the iPlanetDirectoryPro Set-Cookie header
  (curl -D - -o /dev/null + sed) instead of jq .tokenId; pick the last
  non-empty value so a clearing (empty) Set-Cookie cannot win.
- Verify successful logins via "successUrl" in the response body (present on
  every completed authentication, in both HttpOnly and token-readable modes)
  instead of grepping tokenId — robust against cookie-clearing Set-Cookie.
  Applies to the IDP demo user, SP, and the multi-server test-openam1/2/3 checks.
With HttpOnly session cookies on by default, /json/authenticate no longer
returns tokenId in the body, so the MCP server got a null token on a fresh
OpenAM install. AuthInterceptor now takes tokenId from the body when present
and otherwise the last non-empty Set-Cookie named like openam.tokenHeader
(iPlanetDirectoryPro), failing with a clear error when neither is there.
Covers both the username/password and the OAuth path; README notes that
allowTokenInBody is not needed.
@vharseko vharseko added java Pull requests that update java code tests Test suite: coverage, fixtures, or test infrastructure documentation Documentation, README, or javadoc ci CI, GitHub Actions, or build pipeline labels Sep 25, 2026
@vharseko

Copy link
Copy Markdown
Member Author

@tsujiguchitky Thanks — good catch, confirmed: with HttpOnly on and allowTokenInBody off, both AuthInterceptor paths got a null token (and actually hit an NPE in the Caffeine cache on refresh). Fixed in this PR rather than separately, since it is this change that breaks it (1700002):

  • /json/authenticate responses are now read as a ResponseEntity; the token comes from tokenId in the body when present, otherwise from the last non-empty iPlanetDirectoryPro (openam.tokenHeader) Set-Cookie, with a clear error when neither is there.
  • Tests cover both the username/password and OAuth paths in HttpOnly mode via MockRestServiceServer, plus a clearing cookie and no token at all.
  • The MCP server README notes that allowTokenInBody isn't needed.

I also rebased the branch onto current master (no functional change to the rest of the PR). Re-requesting review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci CI, GitHub Actions, or build pipeline documentation Documentation, README, or javadoc help wanted java Pull requests that update java code needs testing security Security fix or hardening (CVE, GHSA, XSS/CSRF/SSRF) tests Test suite: coverage, fixtures, or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants