AMP-31205 : PIDC-Security-Test-Report-fixes

[brianbrix]

…te restrictions

[Copilot]

🟡 Changes recommended

The cookie settings can break login and do not fully secure all authentication and session cookies.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This pull request hardens cookie security and updates Tomcat proxy/server configuration.

Changes:

• Adds HttpOnly and conditional Secure attributes to the language cookie.
• Configures proxy handling and server identification.
• Enables stricter container cookie policies.

File summaries

File	Summary and findings
amp/src/main/java/org/digijava/kernel/util/DgUtil.java	Hardens the language cookie. Moderate (1 vote): HttpOnly prevents existing JavaScript language updates from working.
amp/docker/server.xml	Updates proxy and server configuration.
amp/context.xml	Tightens cookie policies. Critical (3 votes): Strict SameSite breaks the cross-site login callback. Moderate (1 vote): Application-created authentication cookies remain without required flags. Moderate (1 vote): The container session cookie is not configured as Secure.

Review details

Suppressed comments (3)

amp/context.xml:3

• useHttpOnly applies to the container-managed session cookie; it does not add HttpOnly or Secure to cookies that the application creates directly. AMP still emits the authentication cookies digi_session_id and the remember-login cookie in HttpLoginManager without those flags (the latter contains a base64-encoded username and password), so this change does not enforce the cookie restriction for credentials. Set the flags at those cookie creation/removal sites as well, or centralize authentication-cookie creation.

<Context path="/" debug="0" reloadable="false" unloadDelay="4000" swallowOutput="true" useHttpOnly="true"

amp/context.xml:3

• useHttpOnly="true" only adds the HttpOnly attribute; it does not make the container's JSESSIONID Secure. WEB-INF/web.xml currently has only a session timeout and no <cookie-config><secure>true</secure></cookie-config>, so the actual session identifier can still be sent over plain HTTP. Add the secure session-cookie configuration (and keep the proxy scheme handling) if this change is intended to close the cookie security finding.

<Context path="/" debug="0" reloadable="false" unloadDelay="4000" swallowOutput="true" useHttpOnly="true"

amp/src/main/java/org/digijava/kernel/util/DgUtil.java:268

• digi_language is still assigned from JavaScript by both TEMPLATE/ampTemplate/dashboard/external.js:40 and TEMPLATE/ampTemplate/gisModule/external.js:40. After this response cookie becomes HttpOnly, browsers ignore document.cookie writes for that cookie name, so those client-side language updates no longer work. Either remove those writes and make the server endpoint the sole setter, or do not mark this client-managed cookie HttpOnly.

        cookie.setHttpOnly(true);

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Critical cookie vulnerabilities and compatibility issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

amp/context.xml:9

• Strict SameSite breaks the built-in cross-site login handoff: redirectToLoginSite sets the Digi session cookie before redirecting to the configured login site, and /um/newSession requires that cookie on the redirect back before accepting the session id in the URL. When the login site is a different site, browsers omit this cookie and the flow returns noSessionCookie.jsp; retain lax for top-level login redirects or replace this handoff with a token-based flow that does not depend on a cross-site cookie.

    <CookieProcessor sameSiteCookies="strict" />

amp/src/main/java/org/digijava/kernel/util/DgUtil.java:268

• digi_language is also written by browser JavaScript (document.cookie = ...) in both TEMPLATE/ampTemplate/dashboard/external.js and TEMPLATE/ampTemplate/gisModule/external.js. Making this cookie HttpOnly prevents the ?language= flow from updating the preference, leaving the old language in place; keep this non-sensitive preference cookie script-accessible or move those writes to a server-side Set-Cookie flow.

        cookie.setHttpOnly(true);

• Files reviewed: 4/4 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Unresolved moderate findings affect authentication-cookie security and the cross-site login handoff.

Review details

Suppressed comments (4)

amp/context.xml:3

• useHttpOnly only protects the container session cookie from script access; it does not mark JSESSIONID as Secure. This context and web.xml do not configure sessionCookieSecure/<cookie-config><secure>, so the authentication session cookie can still be sent over HTTP in an HTTPS deployment. Add a production-specific Secure session-cookie setting (and retain an explicit HTTP-only override for local development).

<Context path="/" debug="0" reloadable="false" unloadDelay="4000" swallowOutput="true" useHttpOnly="true"

amp/context.xml:9

• Changing this from lax to strict breaks the existing cross-site login handoff. HttpLoginManager creates the digi_session_id cookie before redirecting to the configured separate logon-site, and the return flow expects that cookie in NewSession; a top-level redirect back from the login host is cross-site, so SameSite=Strict omits it and the request is sent to the no-session-cookie path. Keep lax for this flow or replace the cookie-based handoff with an explicit state mechanism.

    <CookieProcessor sameSiteCookies="strict" />

amp/src/main/java/org/digijava/kernel/util/DgUtil.java:269

• This protects only digi_language; the authentication cookies issued by HttpLoginManager (digi_session_id and the credential-bearing login cookie) are still created without a Secure attribute. Thus an HTTPS request can still leave security-sensitive cookies usable over a later HTTP request, despite the new proxy-aware isSecure() handling. Apply the same conditional Secure policy to those auth-cookie creation paths, or enforce it centrally.

        cookie.setSecure(request.isSecure());

amp/src/main/java/org/digijava/kernel/util/DgUtil.java:268

• digi_language is intentionally written by client-side code: TEMPLATE/ampTemplate/dashboard/external.js:35-40 and TEMPLATE/ampTemplate/gisModule/external.js:35-40 assign it via document.cookie so ?language= works. Marking it HttpOnly makes that assignment unavailable, so those entry points can no longer persist the requested language. Keep HttpOnly on the session cookie, but leave this non-sensitive preference cookie script-readable.

        cookie.setHttpOnly(true);

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Cookie security flags and Strict SameSite behavior leave authentication and SSO flows unresolved.

Review details

Suppressed comments (3)

amp/context.xml:3

• useHttpOnly only affects container-managed session cookies; it does not add flags to arbitrary Cookie objects. HttpLoginManager.saveSessionIdToCookie still emits the login/session token digi_session_id without HttpOnly or Secure, and its remember-me cookie also carries base64 credentials without those flags, so this change leaves sensitive authentication cookies readable by scripts and sendable over HTTP. Set both flags where those cookies are created, with an explicit production HTTPS policy.

<Context path="/" debug="0" reloadable="false" unloadDelay="4000" swallowOutput="true" useHttpOnly="true"

amp/context.xml:9

• Setting SameSite=Strict breaks the existing cross-site login flow. HttpLoginManager.redirectToLoginSite sets the digi_session_id cookie before redirecting to the configured login site, then the return handler in NewSession requires that cookie before accepting the dgsessionid parameter; browsers omit a Strict cookie on this cross-site top-level return, so SSO ends at noSessionCookie.jsp. Keep Lax for this callback or redesign the callback so it does not depend on that cookie.

    <!-- Strict prevents session cookies from being sent in cross-site requests. -->
    <CookieProcessor sameSiteCookies="strict" />

amp/src/main/java/org/digijava/kernel/util/DgUtil.java:268

• This cookie is used by the existing client-side language bootstrap, but marking it HttpOnly prevents that code from updating it: both TEMPLATE/ampTemplate/dashboard/external.js:40 and TEMPLATE/ampTemplate/gisModule/external.js:40 assign document.cookie = "digi_language=...". As a result, links with ?language=... will no longer persist the selected language. Keep this non-HttpOnly (it only stores a locale), or move that update to a server-side endpoint and update both clients.

        cookie.setHttpOnly(true);

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite