Conversation
tsujiguchitky
left a comment
There was a problem hiding this comment.
LGTM - enabling HttpOnly by default is the right call, and the change is consistent across code, defaults, CI, tests, and docs. Approving.
|
Rebased/merged |
|
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.
Could we have the MCP server read |
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.
6e8f928 to
1700002
Compare
|
@tsujiguchitky Thanks — good catch, confirmed: with HttpOnly on and
I also rebased the branch onto current master (no functional change to the rest of the PR). Re-requesting review. |
tsujiguchitky
left a comment
There was a problem hiding this comment.
LGTM! Thanks for the quick fix — the MCP server now picks up the SSO token from the session cookie under the new HttpOnly default, and the tests cover both auth paths nicely.
Summary
Flip the default of
com.sun.identity.cookie.httponlyfromfalsetotrueso 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
document.cookie, mitigating token theft through XSS.falsebecause the XUI once read the token fromdocument.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-sharedCookieUtils— default totruewhen the property is unset, viagetAsBoolean(AM_COOKIE_HTTPONLY, true);isCookieHttpOnly()javadoc updated.serverdefaults.properties— shipcom.sun.identity.cookie.httponly=true; rewrite the stale comment (document theallowTokenInBodyopt-in / opt-out).IDP Discovery (separate war)
CookieUtils— default HttpOnly totrue(null/empty → on, explicitfalse→ off).Configurator.jsp— default the "HTTP-Only Cookie" radio to True.MCP server (
openam-mcp-server)AuthInterceptor— with HttpOnly on,/json/authenticateno longer returnstokenIdin the body, so the MCP server read a null token. It now takes the token fromtokenIdin the body when present, otherwise from the last non-emptyiPlanetDirectoryPro(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 viaMockRestServiceServer, plus body precedence, a clearing cookie and the no-token case.allowTokenInBodyis not needed; setOPENAM_TOKEN_HEADERif the session cookie is renamed.Tests
RestAuthenticationHandlerTest— establish the token-readable baseline (setCookieHttpOnly(false)) in@BeforeMethodso 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/)truedefault first (XUI specs), then override tofalseviasetenv.shand run the full suite (oauth2/samlreadtokenIdfrom the body, which is suppressed in HttpOnly mode).iPlanetDirectoryProSet-Cookieheader (instead ofjq .tokenId), picking the last non-empty value so a clearing (empty)Set-Cookiecannot win.successUrlin the response body (present on every completed authentication, in both modes) instead ofgrep tokenId.saml-test.spec.mjs— fix the now-incorrect "HttpOnly breaks XUI" comment;openam-commons.mjs— document thatgetAuthTokenneeds the token in the body.Behavioral changes / migration
/json/authenticateno longer echoestokenIdin 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 setorg.openidentityplatform.openam.httponly.allowTokenInBody=true, or disable HttpOnly entirely withcom.sun.identity.cookie.httponly=false.Testing
openam-sharedandopenam-idpdiscoverycompile (offline).openam-mcp-server: 30/30 tests pass; the new HttpOnly tests fail against the previousAuthInterceptor.build.ymlYAML validated;Set-Cookietoken extraction andsuccessUrlchecks verified locally (incl. empty/clearing cookie and failed-auth cases).xui-httponlye2e 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:
tokenIdout of the/json/authenticatebody now gets nothing. That's by design — confirm the escape hatch (allowTokenInBody=true) actually brings it back.amlbcookie), HTTP vs HTTPS, and theSecure/SameSiteinterplay.Green checkmarks tell you it didn't break the way we expected. Testing is how you find the way we didn't. 👀
Reference