-
Notifications
You must be signed in to change notification settings - Fork 2
fix: add security headers to Java backend to address ZAP findings #416
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 8 commits
5a78d54
241bdd8
279c2e4
7674c0e
c904d82
9502885
c26ff76
92b3e40
bed232d
8c0c42c
ab370aa
f234c85
2723e8e
663a9f1
255af1a
df15ca3
88f6940
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,186 @@ | ||||||
| package ca.bc.gov.nrs.api.security; | ||||||
|
|
||||||
| import jakarta.ws.rs.container.ContainerRequestContext; | ||||||
| import jakarta.ws.rs.container.ContainerResponseContext; | ||||||
| import jakarta.ws.rs.container.ContainerResponseFilter; | ||||||
| import jakarta.ws.rs.core.MultivaluedMap; | ||||||
| import jakarta.ws.rs.ext.Provider; | ||||||
| import java.util.ArrayList; | ||||||
| import java.util.List; | ||||||
|
|
||||||
| /** | ||||||
| * Security headers filter to address ZAP penetration test findings. | ||||||
| * | ||||||
| * Addresses the following ZAP alerts: | ||||||
| * - Content Security Policy (CSP) Header Not Set [10038] | ||||||
| * - Missing Anti-clickjacking Header [10020] | ||||||
| * - Proxy Disclosure [40025] | ||||||
| * - Cookie with SameSite Attribute None [10054] | ||||||
| * - Permissions Policy Header Not Set [10063] | ||||||
| * - Strict-Transport-Security Header Not Set [10035] | ||||||
| * - X-Content-Type-Options Header Missing [10021] | ||||||
| * - Re-examine Cache-control Directives [10015] | ||||||
| * - Non-Storable Content [10049] | ||||||
| * - Storable and Cacheable Content [10049] | ||||||
| */ | ||||||
| @Provider | ||||||
| public class SecurityHeadersFilter implements ContainerResponseFilter { | ||||||
|
|
||||||
| // Headers to remove to prevent proxy/server disclosure | ||||||
| private static final String[] HEADERS_TO_REMOVE = { | ||||||
| "Server", | ||||||
| "X-Powered-By", | ||||||
| "Via", | ||||||
| "X-AspNet-Version", | ||||||
| "X-AspNetMvc-Version" | ||||||
| }; | ||||||
|
|
||||||
| @Override | ||||||
| public void filter( | ||||||
| ContainerRequestContext requestContext, ContainerResponseContext responseContext) { | ||||||
| MultivaluedMap<String, Object> headers = responseContext.getHeaders(); | ||||||
|
|
||||||
| // Security headers to address ZAP alerts | ||||||
|
|
||||||
| // X-Content-Type-Options: Prevents MIME type sniffing | ||||||
| // Addresses: X-Content-Type-Options Header Missing [10021] | ||||||
| headers.add("X-Content-Type-Options", "nosniff"); | ||||||
|
|
||||||
| // X-Frame-Options: Prevents clickjacking attacks | ||||||
| // Addresses: Missing Anti-clickjacking Header [10020] | ||||||
| headers.add("X-Frame-Options", "DENY"); | ||||||
|
|
||||||
| // Strict-Transport-Security: Enforces HTTPS | ||||||
| // Addresses: Strict-Transport-Security Header Not Set [10035] | ||||||
| // Only set HSTS when the request is served over HTTPS | ||||||
| // Check both direct HTTPS and proxy-forwarded HTTPS (for reverse proxy scenarios) | ||||||
| boolean isHttps = | ||||||
| requestContext.getUriInfo().getRequestUri().getScheme().equals("https") | ||||||
| || "https".equalsIgnoreCase( | ||||||
| requestContext.getHeaderString("X-Forwarded-Proto")); | ||||||
| if (isHttps) { | ||||||
| headers.add( | ||||||
| "Strict-Transport-Security", "max-age=31536000; includeSubDomains; preload"); | ||||||
| } | ||||||
|
|
||||||
| // Content-Security-Policy: Restrictive CSP for API endpoints | ||||||
| // Addresses: Content Security Policy (CSP) Header Not Set [10038] | ||||||
| // Note: This is a restrictive policy suitable for APIs. For web applications with | ||||||
| // inline scripts/styles or external resources, customize this policy accordingly. | ||||||
| headers.add("Content-Security-Policy", "default-src 'self'"); | ||||||
DerekRoberts marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
|
||||||
| // Permissions-Policy: Controls browser features | ||||||
| // Addresses: Permissions Policy Header Not Set [10063] | ||||||
| headers.add( | ||||||
| "Permissions-Policy", | ||||||
| "geolocation=(), microphone=(), camera=(), payment=(), usb=(), magnetometer=()," | ||||||
| + " gyroscope=(), speaker-selection=()"); | ||||||
|
||||||
| + " gyroscope=(), speaker-selection=()"); | |
| + "gyroscope=(), speaker-selection=()"); |
Fixed
Show fixed
Hide fixed
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation on line 120 is incomplete. It states "If SameSite is missing or set to None, replaces with Strict" but the implementation also replaces "Lax" with "Strict". The documentation should be updated to accurately reflect the actual behavior: "If SameSite is missing or set to None or Lax, replaces with Strict".
| * If SameSite is missing or set to None, replaces with Strict. | |
| * If SameSite is missing or set to None or Lax, replaces with Strict. |
DerekRoberts marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
DerekRoberts marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cookie attribute detection logic has a bug when checking for HttpOnly, Secure, and Path attributes. The code uses case-sensitive indexOf to find these attributes (e.g., cookie.indexOf("; HttpOnly")), which will fail to detect these attributes if they have different casing (e.g., "; httponly", "; HTTPONLY"). Cookie attributes are case-insensitive according to RFC 6265, so this could result in incorrect insertion position for the SameSite attribute. Consider using case-insensitive matching or converting to lowercase for comparison.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,139 @@ | ||
| package ca.bc.gov.nrs.api.security; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import java.lang.reflect.Method; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.*; | ||
|
|
||
| /** | ||
| * Unit tests for cookie SameSite attribute handling in SecurityHeadersFilter. | ||
| * Tests the fixCookieHeader method logic directly. | ||
| */ | ||
| class SecurityHeadersFilterCookieTest { | ||
|
|
||
| private SecurityHeadersFilter filter = new SecurityHeadersFilter(); | ||
|
|
||
| /** | ||
| * Uses reflection to access the private fixCookieHeader method for testing. | ||
| */ | ||
| private String fixCookieHeader(String cookie) throws Exception { | ||
| Method method = SecurityHeadersFilter.class.getDeclaredMethod("fixCookieHeader", String.class); | ||
| method.setAccessible(true); | ||
| return (String) method.invoke(filter, cookie); | ||
| } | ||
DerekRoberts marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| @Test | ||
| void testCookieWithoutSameSite() throws Exception { | ||
| String cookie = "sessionId=abc123"; | ||
| String result = fixCookieHeader(cookie); | ||
| assertTrue(result.contains("SameSite=Strict"), "Should add SameSite=Strict"); | ||
| assertEquals("sessionId=abc123; SameSite=Strict", result); | ||
| } | ||
|
|
||
| @Test | ||
| void testCookieWithSameSiteNone() throws Exception { | ||
| String cookie = "sessionId=abc123; SameSite=None"; | ||
| String result = fixCookieHeader(cookie); | ||
| assertTrue(result.contains("SameSite=Strict"), "Should replace SameSite=None with Strict"); | ||
| assertFalse(result.contains("SameSite=None"), "Should not contain SameSite=None"); | ||
| } | ||
|
|
||
| @Test | ||
| void testCookieWithSameSiteLax() throws Exception { | ||
| String cookie = "sessionId=abc123; SameSite=Lax"; | ||
| String result = fixCookieHeader(cookie); | ||
| assertTrue(result.contains("SameSite=Strict"), "Should replace SameSite=Lax with Strict"); | ||
| assertFalse(result.contains("SameSite=Lax"), "Should not contain SameSite=Lax"); | ||
| } | ||
|
|
||
| @Test | ||
| void testCookieWithSameSiteStrict() throws Exception { | ||
| String cookie = "sessionId=abc123; SameSite=Strict"; | ||
| String result = fixCookieHeader(cookie); | ||
| assertTrue(result.contains("SameSite=Strict"), "Should keep SameSite=Strict"); | ||
| // Should not duplicate | ||
| assertEquals(1, (result.length() - result.replace("SameSite=Strict", "").length()) / "SameSite=Strict".length()); | ||
DerekRoberts marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| @Test | ||
| void testCookieWithHttpOnly() throws Exception { | ||
| String cookie = "sessionId=abc123; HttpOnly"; | ||
| String result = fixCookieHeader(cookie); | ||
| assertTrue(result.contains("SameSite=Strict"), "Should add SameSite=Strict"); | ||
| assertTrue(result.contains("HttpOnly"), "Should preserve HttpOnly"); | ||
| // SameSite should come before HttpOnly | ||
| int sameSiteIndex = result.indexOf("SameSite=Strict"); | ||
| int httpOnlyIndex = result.indexOf("HttpOnly"); | ||
| assertTrue(sameSiteIndex < httpOnlyIndex, "SameSite should come before HttpOnly"); | ||
| } | ||
|
|
||
| @Test | ||
| void testCookieWithSecure() throws Exception { | ||
| String cookie = "sessionId=abc123; Secure"; | ||
| String result = fixCookieHeader(cookie); | ||
| assertTrue(result.contains("SameSite=Strict"), "Should add SameSite=Strict"); | ||
| assertTrue(result.contains("Secure"), "Should preserve Secure"); | ||
| // SameSite should come before Secure | ||
| int sameSiteIndex = result.indexOf("SameSite=Strict"); | ||
| int secureIndex = result.indexOf("Secure"); | ||
| assertTrue(sameSiteIndex < secureIndex, "SameSite should come before Secure"); | ||
| } | ||
|
|
||
| @Test | ||
| void testCookieWithPath() throws Exception { | ||
| String cookie = "sessionId=abc123; Path=/"; | ||
| String result = fixCookieHeader(cookie); | ||
| assertTrue(result.contains("SameSite=Strict"), "Should add SameSite=Strict"); | ||
| assertTrue(result.contains("Path=/"), "Should preserve Path"); | ||
| // SameSite should come before Path | ||
| int sameSiteIndex = result.indexOf("SameSite=Strict"); | ||
| int pathIndex = result.indexOf("Path="); | ||
| assertTrue(sameSiteIndex < pathIndex, "SameSite should come before Path"); | ||
| } | ||
|
|
||
| @Test | ||
| void testCookieWithMultipleAttributes() throws Exception { | ||
| // Test with HttpOnly, Secure, and Path - should insert before the earliest one | ||
| String cookie = "sessionId=abc123; Secure; HttpOnly; Path=/"; | ||
| String result = fixCookieHeader(cookie); | ||
| assertTrue(result.contains("SameSite=Strict"), "Should add SameSite=Strict"); | ||
| assertTrue(result.contains("Secure"), "Should preserve Secure"); | ||
| assertTrue(result.contains("HttpOnly"), "Should preserve HttpOnly"); | ||
| assertTrue(result.contains("Path=/"), "Should preserve Path"); | ||
| // SameSite should come before Secure (the earliest attribute) | ||
| int sameSiteIndex = result.indexOf("SameSite=Strict"); | ||
| int secureIndex = result.indexOf("Secure"); | ||
| assertTrue(sameSiteIndex < secureIndex, "SameSite should come before Secure"); | ||
| } | ||
|
|
||
| @Test | ||
| void testCookieWithCaseInsensitiveSameSite() throws Exception { | ||
| String cookie = "sessionId=abc123; samesite=none"; | ||
| String result = fixCookieHeader(cookie); | ||
| assertTrue(result.contains("SameSite=Strict"), "Should replace case-insensitive SameSite=None"); | ||
| assertFalse(result.toLowerCase().contains("samesite=none"), "Should not contain SameSite=None"); | ||
| } | ||
|
|
||
| @Test | ||
| void testCookieWithSpacesInSameSite() throws Exception { | ||
| String cookie = "sessionId=abc123; SameSite = None"; | ||
| String result = fixCookieHeader(cookie); | ||
| assertTrue(result.contains("SameSite=Strict"), "Should handle spaces in SameSite attribute"); | ||
| // Verify the original value with spaces was replaced, not just added | ||
| assertFalse(result.contains("= None"), "Should not contain original SameSite = None"); | ||
| assertFalse(result.toLowerCase().contains("samesite = none"), "Should not contain original SameSite = None (case-insensitive)"); | ||
| } | ||
|
|
||
| @Test | ||
| void testNullCookie() throws Exception { | ||
| String result = fixCookieHeader(null); | ||
| assertNull(result, "Should return null for null input"); | ||
| } | ||
|
|
||
| @Test | ||
| void testEmptyCookie() throws Exception { | ||
| String result = fixCookieHeader(""); | ||
| assertEquals("", result, "Should return empty string for empty input"); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.