Set the Secure and HttpOnly cookie flags from creation

[vharseko]

The cookie cluster of the CodeQL medium triage: java/insecure-cookie ×19 and java/sensitive-cookie-not-httponly ×7.

What the findings are about

OpenAM sets Secure and HttpOnly from configuration (com.iplanet.am.cookie.secure, com.sun.identity.cookie.httponly), which CodeQL cannot follow as such: it accepts a cookie as secure only when it flows from setSecure(true) or setSecure(request.isSecure()), and as HttpOnly when it flows from a setHttpOnly(x) with x not literally false. Reading the sinks against that model turned up two real gaps rather than only a modelling mismatch:

• CookieUtils.newCookie set Secure from the property but left HttpOnly to addCookieToResponse, which applied it by reflection — Cookie.class.getMethod("setHttpOnly"), a Servlet 2.5 leftover that has been dead code since the move to Jakarta Servlet. 20 of the 26 sinks are cookies built by createCookie → newCookie and handed straight to response.addCookie (mostly the logout deletions), which therefore went out without HttpOnly even when it is configured.
• The LoginServlet and CDCServlet "reset" paths re-added the request's own Cookie object with an empty value. A request cookie carries no path, domain or flags, so the deletion the browser received did not match the cookie that had been set with path / (and, for CDC, the auth-URL cookie domain).

The change

• CookieUtils.newCookie sets both flags on the cookie as it is created — if (isCookieSecure()) setSecure(true), if (isCookieHttpOnly()) setHttpOnly(true) — so every cookie the helper builds carries them whatever path adds it to the response. addCookieToResponse calls Cookie.setHttpOnly directly; the hand-built Set-Cookie header is kept for the SameSite case only, which the servlet Cookie cannot express. The IdP discovery war's copy of CookieUtils sets both flags in newCookie the same way, but its addCookieToResponse keeps writing an HttpOnly cookie as a hand-built header: that WAR has no SameSite setting, and its configurator lets a deployment choose a cookie domain such as .example.com or an unencoded, space-separated preferred-IdP value, both of which Tomcat's Rfc6265CookieProcessor rejects in response.addCookie.
• LoginServlet: the host-only deletion is built with AuthUtils.createCookie(name, "", null) + maxAge 0, the same way the per-domain deletions next to it already are; the AMTESTCOOKIE probe goes through CookieUtils (only the server reads it back).
• CDCServlet: the auth-URL cookie is cleared by name, path / and its configured domain instead of re-adding the request object.
• The Oracle Access Manager and SiteMinder sample adapters (src/main/integrations, not part of the build but scanned under build-mode: none) set their session cookie Secure on an HTTPS request and HttpOnly; their agents read the cookie from the request header, not from script.

No configuration semantics change: a deployment that sets neither property gets exactly the cookies it got before, minus nothing.

Verification

Ten new tests, five in openam-shared and five in openam-idpdiscovery. Run against the base sources, two of them fail — newCookieCarriesTheHttpOnlyFlagTheDeploymentConfigures in each module (expected [true] but found [false]) — and pin the fix. The other eight pass at the base too: they are characterization tests of the routing and of the Secure flag, which the base already handled. The idpdiscovery HttpOnly-off row does go red against an unconditional setHttpOnly(true) in addCookieToResponse. openam-shared 1242 tests, openam-idpdiscovery 13 — 0 failures, on the current master; openam-core 2130 and openam-federation-library 177 — 0 failures, OpenFM and openam-server-auth-ui compile, on the first round's base. The LoginServlet/CDCServlet wiring has no unit test (no servlet harness in those modules), and the two adapters cannot be compiled without the vendor SDKs.

Expected effect on the scan

All 19 insecure-cookie and 7 sensitive-cookie-not-httponly alerts should close: the cookies now flow from a literal setSecure(true) / setHttpOnly(true) in newCookie. Anything left after this PR's scan is a sink that receives a cookie built outside newCookie, to be looked at individually.

[maximthomas]

praise: The flags now live where the cookies are made, which is where the gap was.

• CookieUtils.newCookie (openam-shared) sets HttpOnly at creation, so the createCookie → response.addCookie sinks (the logout deletions) carry it; newCookieCarriesTheHttpOnlyFlagTheDeploymentConfigures is red at the base and green at the head.
• The dead reflective Cookie.class.getMethod("setHttpOnly") in addCookieToResponse is gone.
• CDCServlet.java:675 now deletes the auth-URL cookie by name, path / and its configured domain instead of re-adding the request's pathless Cookie.

---

issue (blocking): In the IdP discovery copy of addCookieToResponse, an HttpOnly cookie now goes through response.addCookie, which Tomcat validates, instead of the hand-written header, which it does not.

openam-federation/openam-idpdiscovery/src/main/java/com/sun/identity/saml2/idpdiscovery/CookieUtils.java:495, CookieWriterServlet.java:281, :306, :368

The standalone IdP discovery WAR has no SameSite key in libIDPDiscoveryConfig.properties, so getCookieSameSite() is null there and every HttpOnly=True deployment takes the new road into Tomcat's Rfc6265CookieProcessor. That processor throws IllegalArgumentException for a cookie domain .example.com ("An invalid domain"), which then fails on every _saml_idp write. It also throws for the value resetPreferredIDPCookie builds with encodeCookie=False: it joins IdPs with a literal space, so the second preferred IdP fails with "invalid character [32]". Measured against tomcat-embed-core 10.1.50 and 11.0.20. Response.addCookie has no handler, and doGetPost catches only IOException, so /saml2writer answers HTTP 500. At the base both configurations worked, because !isCookieHttpOnly() && … sent them down the addHeader road. Both choices are radio buttons or a text field in Configurator.jsp. The OpenAM server WAR is not affected, because it defaults to samesite=Lax.

        if (!isCookieHttpOnly() && getCookieSameSite() == null) {
            response.addCookie(cookie);
            return;
        }

newCookie still sets the flag at creation, and the header road below already appends ;httponly. addCookieToResponseSetsHttpOnlyOnTheServletCookie (CookieUtilsTest.java:134) pins the routing this reverts and turns into a header row. The addCookie sink in this copy may then stay a CodeQL alert. Or: keep addCookie, and fall back to the header on IllegalArgumentException.

---

suggestion (non-blocking): The idpdiscovery suite has no HttpOnly-off row for addCookieToResponse, so making cookie.setHttpOnly(true) unconditional there stays green.

openam-federation/openam-idpdiscovery/src/test/java/com/sun/identity/saml2/idpdiscovery/CookieUtilsTest.java:134, :149

addCookieToResponse is called only under (false, true, null) and (true, true, "Lax"). The openam-shared suite has addCookieToResponseLeavesHttpOnlyAloneWhenNotConfigured, but this copy has no equivalent. The mutant is not equivalent: with httponly unset, CookieWriterServlet:281 would send _saml_idp with HttpOnly.

    @Test
    public void addCookieToResponseLeavesHttpOnlyAloneWhenNotConfigured() {
        withCookieSettings(false, false, null, () -> {
            HttpServletResponse response = mock(HttpServletResponse.class);
            Cookie cookie = new Cookie("_saml_idp", "aWRw");

            CookieUtils.addCookieToResponse(response, cookie);

            assertFalse(cookie.isHttpOnly());
            verify(response).addCookie(cookie);
        });
    }

Pin: this row goes red against the unconditional setHttpOnly(true), and it stays the only addCookie row once the blocking fix above lands.

---

nitpick (non-blocking): The Verification paragraph's "Every new test was watched failing first" is true of 3 of the 9 new tests, and openam-shared has 5 new tests, not 6.

openam-shared/src/test/java/com/sun/identity/shared/encode/CookieUtilsTest.java:96, openam-federation/openam-idpdiscovery/src/test/java/com/sun/identity/saml2/idpdiscovery/CookieUtilsTest.java:117, :149

I ran the head tests against the base sources. In openam-shared only newCookieCarriesTheHttpOnlyFlagTheDeploymentConfigures goes red, because the base already set HttpOnly by reflection, which resolves under Jakarta, and already set Secure in newCookie. In idpdiscovery the two HttpOnly tests go red, and the Secure and SameSite-header tests pass. The fix is still pinned. The green rows cover unchanged or equivalent code, so the paragraph should name the red ones and call the rest characterization tests.

[vharseko]

issue (blocking) — confirmed and fixed by reverting the routing, as suggested first. I checked the claim against the source and ran the cookie through Rfc6265CookieProcessor on tomcat-embed-core 10.1.48 and 11.0.20: .example.com → "An invalid domain", aWRw aWRwMg== → "An invalid character [32]". PREFERRED_COOKIE_SEPERATOR is " ", and the WAR's libIDPDiscoveryConfig.properties template has no SameSite key.

addCookieToResponse in the idpdiscovery copy is back to !isCookieHttpOnly() && getCookieSameSite() == null → response.addCookie. An HttpOnly cookie goes out as the hand-built header again, with a comment saying why. newCookie still sets both flags at creation. addCookieToResponseSetsHttpOnlyOnTheServletCookie became addCookieToResponseWritesTheHeaderItselfWhenHttpOnlyIsConfigured, with the .example.com domain in the expected header. It is red against the previous head.

On the CodeQL side, I don't expect this sink to come back. Its only alert, #29, is java/insecure-cookie, and the cookie still flows from the literal setSecure(true) in newCookie. The scan on this push will show.

One thing for the record, outside this PR: at the base, HttpOnly=False (the configurator default) already sent the cookie through response.addCookie, so .example.com or an unencoded two-IdP value already gives a 500 there. I left it as it was, to keep this PR free of configuration changes, and will open a separate issue for it.

suggestion — added as proposed: addCookieToResponseLeavesHttpOnlyAloneWhenNotConfigured. I checked the mutant: with cookie.setHttpOnly(true) inserted before response.addCookie, this row goes red.

nitpick — the Verification paragraph is rewritten. Ten new tests, five per module. Against the base sources, two fail (newCookieCarriesTheHttpOnlyFlagTheDeploymentConfigures in each module), and the other eight are called characterization tests. I re-ran both modules against the base myself, and the numbers match yours.

The branch is rebased onto the current master. openam-shared 1242 and openam-idpdiscovery 13 pass with 0 failures.

[vharseko]

The pre-existing HttpOnly=False failure mentioned above is now tracked as #1145.

[maximthomas]

praise: Round 2 fixes the idpdiscovery regression exactly where it was, and pins it.

• addCookieToResponse in the idpdiscovery copy of CookieUtils is back to !isCookieHttpOnly() && getCookieSameSite() == null (:495), with a comment that says why. addCookieToResponseWritesTheHeaderItselfWhenHttpOnlyIsConfigured asserts the hand-built header with a .example.com domain, so round 1's gate turns it red.
• CookieUtils.newCookie sets Secure and HttpOnly at creation (openam-shared/src/main/java/com/sun/identity/shared/encode/CookieUtils.java:402-407), so a cookie handed straight to response.addCookie carries them too.
• CDCServlet clears the auth-URL cookie by the setter's name, path and domain (CDCServlet.java:675-676) instead of re-adding the request's cookie.