11744: CORS: echo request Origin and add Vary: Origin; sanitize CSV lists; prefer comma-separated origins; rely on JVM options/MicroProfile only; add tests and release notes

ErykKul · ErykKul · commit 618ee4fd72bf · 2025-08-14T15:09:21.000+02:00
diff --git a/doc/release-notes/11744-cors-echo-origin-vary.md b/doc/release-notes/11744-cors-echo-origin-vary.md
@@ -0,0 +1,62 @@
+# 11744: CORS header handling fixes (echo single Origin, add Vary: Origin, multi-origin allow, sanitization)
+
+This branch adjusts the CORS filter so browser clients work correctly when multiple origins are allowed.
+
+## What changed
+- Access-Control-Allow-Origin (ACAO) now echoes the single request `Origin` when it matches an allowlist from `dataverse.cors.origin`.
+- `Vary: Origin` is added when echoing a specific origin to keep caches correct across different origins.
+- Comma-separated origin lists are supported; surrounding quotes in CSV configs are stripped.
+- Sanitization is applied to CORS header lists (methods/allow/expose) to avoid quoted values that can break preflight checks.
+- Deprecated DB fallback for enabling CORS is removed; CORS is considered enabled only when `dataverse.cors.origin` is set as a JVM options/Microprofile setting.
+
+## Upgrade / run notes (non-SQL)
+To keep CORS working after pulling this branch:
+
+1) Configure origins as JVM options/Microprofile settings (no quotes):
+- Single origin:
+  - `dataverse.cors.origin=https://example.org`
+- Multiple origins (comma-separated):
+  - `dataverse.cors.origin=https://libis.github.io,https://gdcc.github.io`
+- Wildcard:
+  - `dataverse.cors.origin=*`
+  - Note: Browsers reject `*` when credentialed requests are used (cookies/Authorization headers). Prefer explicit origins for those cases.
+
+2) Optional headers/methods lists (unquoted, comma-separated CSV):
+- `dataverse.cors.methods`
+- `dataverse.cors.headers.allow`
+- `dataverse.cors.headers.expose`
+
+Avoid surrounding values with quotes (e.g., do not use `"Accept, Content-Type"`). Quotes will be stripped but may cause confusion.
+
+3) If you previously relied on the database setting to enable CORS (deprecated `AllowCors`), set `dataverse.cors.origin` instead. The DB fallback is no longer used.
+
+4) Reverse proxies/caches: `Vary: Origin` is now emitted. Ensure your proxy does not drop this header.
+
+## Verify
+Preflight (replace DV_URL with your base URL):
+
+```bash
+curl -i -X OPTIONS \
+  -H "Origin: https://libis.github.io" \
+  -H "Access-Control-Request-Method: GET" \
+  "${DV_URL}/api/info/version"
+```
+
+Expected:
+- `Access-Control-Allow-Origin: https://libis.github.io`
+- `Vary: Origin` present
+
+Actual request:
+
+```bash
+curl -i \
+  -H "Origin: https://libis.github.io" \
+  "${DV_URL}/api/info/version"
+```
+
+Expected:
+- Same ACAO echo as above
+
+## Backward compatibility
+- Instances relying on the deprecated DB-based CORS enablement must set `dataverse.cors.origin` to keep CORS enabled.
+- Quoted CORS configuration values may behave differently; remove quotes going forward.
diff --git a/src/main/java/edu/harvard/iq/dataverse/filter/CorsFilter.java b/src/main/java/edu/harvard/iq/dataverse/filter/CorsFilter.java
@@ -1,69 +1,130 @@
 package edu.harvard.iq.dataverse.filter;
 
-import jakarta.inject.Inject;
 import jakarta.servlet.*;
 import jakarta.servlet.annotation.WebFilter;
+import jakarta.servlet.http.HttpServletRequest;
 import jakarta.servlet.http.HttpServletResponse;
 import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.stream.Collectors;
 
 import edu.harvard.iq.dataverse.settings.JvmSettings;
-import edu.harvard.iq.dataverse.settings.SettingsServiceBean;
 
 /**
  * CorsFilter is a servlet filter that handles Cross-Origin Resource Sharing (CORS) for the Dataverse application.
  * It configures and applies CORS headers to HTTP responses based on application settings.
  * 
  * This filter:
- * 1. Reads CORS configuration from JVM settings or (deprecated) the SettingsServiceBean. See the Dataverse Configuration Guide for more details.
+ * 1. Reads CORS configuration from JVM options/Microprofile settings (e.g. dataverse.cors.*).
  * 2. Determines whether CORS should be allowed based on these settings.
- * 3. If CORS is allowed, it adds the appropriate CORS headers to all HTTP responses. The JVMSettings allow customization of the header contents if desired.
+ * 3. If CORS is allowed, it adds the appropriate CORS headers to all HTTP responses. The JvmSettings allow customization of the header contents if desired.
  * 
  * The filter is applied to all paths ("/*") in the application.
  */
 
 @WebFilter("/*")
 public class CorsFilter implements Filter {
-
-    @Inject
-    private SettingsServiceBean settingsSvc;
-
     private boolean allowCors;
-    private String origin;
+    private String origin; // raw configured origin value
     private String methods;
     private String allowHeaders;
     private String exposeHeaders;
+    private Set<String> allowedOrigins = Collections.emptySet();
+    private boolean allowAllOrigins = false;
 
     @Override
     public void init(FilterConfig filterConfig) throws ServletException {
-        origin = JvmSettings.CORS_ORIGIN.lookupOptional().orElse(null);
-        boolean corsSetting = settingsSvc.isTrueForKey(SettingsServiceBean.Key.AllowCors, true);
-
-        if (origin == null && !corsSetting) {
-            allowCors = false;
-        } else {
-            allowCors = true;
-            origin = (origin != null) ? origin : "*";
-        }
+        origin = sanitize(JvmSettings.CORS_ORIGIN.lookupOptional().orElse(null));
+        allowCors = origin != null && !origin.trim().isEmpty();
 
         if (allowCors) {
-            methods = JvmSettings.CORS_METHODS.lookupOptional().orElse("PUT, GET, POST, DELETE, OPTIONS");
-            allowHeaders = JvmSettings.CORS_ALLOW_HEADERS.lookupOptional()
-                    .orElse("Accept, Content-Type, X-Dataverse-key, Range");
-            exposeHeaders = JvmSettings.CORS_EXPOSE_HEADERS.lookupOptional()
-                    .orElse("Accept-Ranges, Content-Range, Content-Encoding");
+            methods = sanitizeCsv(JvmSettings.CORS_METHODS.lookupOptional().orElse("GET, POST, OPTIONS, PUT, DELETE"));
+            allowHeaders = sanitizeCsv(JvmSettings.CORS_ALLOW_HEADERS.lookupOptional()
+                    .orElse("Accept, Content-Type, X-Dataverse-key, Range"));
+            exposeHeaders = sanitizeCsv(JvmSettings.CORS_EXPOSE_HEADERS.lookupOptional()
+                    .orElse("Accept-Ranges, Content-Range, Content-Encoding"));
+
+            // Initialize allowed origins (documented as comma-separated list)
+            String configured = origin != null ? origin.trim() : null;
+            if (configured == null || configured.isEmpty() || "*".equals(configured)) {
+                allowAllOrigins = true;
+                allowedOrigins = Collections.emptySet();
+            } else {
+                // Parse configured origins; code is tolerant of whitespace but
+                // docs recommend a comma-separated list (no quotes)
+                allowedOrigins = Arrays.stream(configured.split(","))
+                        .flatMap(s -> Arrays.stream(s.split("[\n\r\t ]+")))
+                        .map(String::trim)
+                        .filter(s -> !s.isEmpty())
+                        .collect(Collectors.toCollection(HashSet::new));
+                // handle a single '"*"' that might slip through incorrectly
+                if (allowedOrigins.size() == 1 && allowedOrigins.contains("*")) {
+                    allowAllOrigins = true;
+                    allowedOrigins = Collections.emptySet();
+                }
+            }
         }
     }
 
     @Override
     public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, FilterChain chain)
             throws IOException, ServletException {
         if (allowCors) {
+            HttpServletRequest request = (HttpServletRequest) servletRequest;
             HttpServletResponse response = (HttpServletResponse) servletResponse;
-            response.addHeader("Access-Control-Allow-Origin", origin);
-            response.addHeader("Access-Control-Allow-Methods", methods);
-            response.addHeader("Access-Control-Allow-Headers", allowHeaders);
-            response.addHeader("Access-Control-Expose-Headers", exposeHeaders);
+
+            String requestOrigin = sanitize(request.getHeader("Origin"));
+
+            // Decide ACAO value
+            if (allowAllOrigins) {
+                // Note: Browsers will reject '*' for credentialed requests; this is by design.
+                response.setHeader("Access-Control-Allow-Origin", "*");
+            } else if (requestOrigin != null && allowedOrigins.contains(requestOrigin)) {
+                response.setHeader("Access-Control-Allow-Origin", requestOrigin);
+                // Help caches vary based on Origin
+                response.setHeader("Vary", appendVary(response.getHeader("Vary"), "Origin"));
+            }
+
+            response.setHeader("Access-Control-Allow-Methods", methods);
+            response.setHeader("Access-Control-Allow-Headers", allowHeaders);
+            response.setHeader("Access-Control-Expose-Headers", exposeHeaders);
         }
         chain.doFilter(servletRequest, servletResponse);
     }
+
+    /** Remove surrounding quotes and collapse internal quotes. */
+    private String sanitize(String value) {
+        if (value == null) return null;
+        String v = value.trim();
+        if (v.length() >= 2 && v.startsWith("\"") && v.endsWith("\"")) {
+            v = v.substring(1, v.length() - 1);
+        }
+        return v.replace("\"", "").trim();
+    }
+
+    /** Remove quotes from CSV-like header value and trim spaces around commas. */
+    private String sanitizeCsv(String value) {
+        String v = sanitize(value);
+        // Normalize separators and trim tokens
+        return Arrays.stream(v.split(","))
+                .map(String::trim)
+                .filter(s -> !s.isEmpty())
+                .collect(Collectors.joining(", "));
+    }
+
+    private String appendVary(String existing, String token) {
+        if (existing == null || existing.isEmpty()) {
+            return token;
+        }
+        // Avoid duplicate tokens in Vary
+        Set<String> tokens = Arrays.stream(existing.split(","))
+                .map(String::trim)
+                .filter(s -> !s.isEmpty())
+                .collect(Collectors.toCollection(HashSet::new));
+        tokens.add(token);
+        return String.join(", ", tokens);
+    }
 }
diff --git a/src/test/java/edu/harvard/iq/dataverse/filter/CorsFilterTest.java b/src/test/java/edu/harvard/iq/dataverse/filter/CorsFilterTest.java
@@ -0,0 +1,170 @@
+package edu.harvard.iq.dataverse.filter;
+
+import jakarta.servlet.FilterChain;
+import jakarta.servlet.ServletRequest;
+import jakarta.servlet.ServletResponse;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.ArgumentCaptor;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.junit.jupiter.api.Assertions.*;
+import static org.mockito.Mockito.*;
+
+class CorsFilterTest {
+
+    private final Map<String, String> sysPropsBackup = new HashMap<>();
+
+    @BeforeEach
+    void setUp() {
+        // backup potentially touched props
+        backupAndClear("dataverse.cors.origin");
+        backupAndClear("dataverse.cors.methods");
+        backupAndClear("dataverse.cors.headers.allow");
+        backupAndClear("dataverse.cors.headers.expose");
+    }
+
+    @AfterEach
+    void tearDown() {
+        restore("dataverse.cors.origin");
+        restore("dataverse.cors.methods");
+        restore("dataverse.cors.headers.allow");
+        restore("dataverse.cors.headers.expose");
+    }
+
+    @Test
+    void wildcardOrigin_allowsAny_noVary() throws Exception {
+        System.setProperty("dataverse.cors.origin", "*");
+
+        CorsFilter sut = new CorsFilter();
+        injectSettingsAllowCors(sut, true);
+        sut.init(null);
+
+        HttpServletRequest req = mock(HttpServletRequest.class);
+        when(req.getHeader("Origin")).thenReturn("https://a.example");
+        HttpServletResponse res = mock(HttpServletResponse.class);
+        FilterChain chain = mock(FilterChain.class);
+
+        sut.doFilter(req, res, chain);
+
+        verify(res).setHeader("Access-Control-Allow-Origin", "*");
+        // By design, Vary not required for wildcard
+        verify(res, never()).setHeader(eq("Vary"), anyString());
+        verify(chain).doFilter(any(ServletRequest.class), any(ServletResponse.class));
+    }
+
+    @Test
+    void singleOrigin_echoesAndAddsVary() throws Exception {
+        System.setProperty("dataverse.cors.origin", "https://libis.github.io");
+
+        CorsFilter sut = new CorsFilter();
+        injectSettingsAllowCors(sut, true);
+        sut.init(null);
+
+        HttpServletRequest req = mock(HttpServletRequest.class);
+        when(req.getHeader("Origin")).thenReturn("https://libis.github.io");
+        HttpServletResponse res = mock(HttpServletResponse.class);
+        when(res.getHeader("Vary")).thenReturn(null);
+        FilterChain chain = mock(FilterChain.class);
+
+        sut.doFilter(req, res, chain);
+
+        verify(res).setHeader("Access-Control-Allow-Origin", "https://libis.github.io");
+
+        ArgumentCaptor<String> varyVal = ArgumentCaptor.forClass(String.class);
+        verify(res).setHeader(eq("Vary"), varyVal.capture());
+        assertTrue(varyVal.getValue().contains("Origin"));
+        verify(chain).doFilter(any(ServletRequest.class), any(ServletResponse.class));
+    }
+
+    @Test
+    void multipleOrigins_echoesMatch_onlyWhenAllowed() throws Exception {
+        // Comma-separated list as set via JVM options/Microprofile
+        System.setProperty("dataverse.cors.origin", "https://a.example, https://b.example");
+
+        CorsFilter sut = new CorsFilter();
+        injectSettingsAllowCors(sut, true);
+        sut.init(null);
+
+        // allowed origin
+        HttpServletRequest reqAllowed = mock(HttpServletRequest.class);
+        when(reqAllowed.getHeader("Origin")).thenReturn("https://b.example");
+        HttpServletResponse resAllowed = mock(HttpServletResponse.class);
+        FilterChain chain = mock(FilterChain.class);
+
+        sut.doFilter(reqAllowed, resAllowed, chain);
+        verify(resAllowed).setHeader("Access-Control-Allow-Origin", "https://b.example");
+        verify(resAllowed).setHeader(eq("Vary"), contains("Origin"));
+
+        // not allowed origin -> no ACAO header set
+        HttpServletRequest reqDenied = mock(HttpServletRequest.class);
+        when(reqDenied.getHeader("Origin")).thenReturn("https://c.example");
+        HttpServletResponse resDenied = mock(HttpServletResponse.class);
+
+        sut.doFilter(reqDenied, resDenied, chain);
+        verify(resDenied, never()).setHeader(eq("Access-Control-Allow-Origin"), anyString());
+    }
+
+    @Test
+    void sanitizesQuotedHeaderLists() throws Exception {
+        System.setProperty("dataverse.cors.origin", "https://x.example");
+        System.setProperty("dataverse.cors.headers.allow", "\"Accept, X-Dataverse-key\"");
+        System.setProperty("dataverse.cors.headers.expose", "\"Accept-Ranges, Content-Range\"");
+        System.setProperty("dataverse.cors.methods", "GET, POST, OPTIONS");
+
+        CorsFilter sut = new CorsFilter();
+        injectSettingsAllowCors(sut, true);
+        sut.init(null);
+
+        HttpServletRequest req = mock(HttpServletRequest.class);
+        when(req.getHeader("Origin")).thenReturn("https://x.example");
+        HttpServletResponse res = mock(HttpServletResponse.class);
+
+        sut.doFilter(req, res, mock(FilterChain.class));
+
+        verify(res).setHeader("Access-Control-Allow-Headers", "Accept, X-Dataverse-key");
+        verify(res).setHeader("Access-Control-Expose-Headers", "Accept-Ranges, Content-Range");
+        verify(res).setHeader("Access-Control-Allow-Methods", "GET, POST, OPTIONS");
+    }
+
+    @Test
+    void disabledCors_skipsHeaders() throws Exception {
+        // no origin set -> CORS disabled
+        CorsFilter sut = new CorsFilter();
+        sut.init(null);
+
+        HttpServletRequest req = mock(HttpServletRequest.class);
+        when(req.getHeader("Origin")).thenReturn("https://any.example");
+        HttpServletResponse res = mock(HttpServletResponse.class);
+
+        sut.doFilter(req, res, mock(FilterChain.class));
+
+        verify(res, never()).setHeader(eq("Access-Control-Allow-Origin"), anyString());
+        verify(res, never()).setHeader(eq("Access-Control-Allow-Methods"), anyString());
+        verify(res, never()).setHeader(eq("Access-Control-Allow-Headers"), anyString());
+        verify(res, never()).setHeader(eq("Access-Control-Expose-Headers"), anyString());
+    }
+
+    // No-op since filter no longer depends on SettingsServiceBean
+    private void injectSettingsAllowCors(CorsFilter sut, boolean allowCors) { /* legacy path removed */ }
+
+    private void backupAndClear(String key) {
+        String old = System.getProperty(key);
+        if (old != null) {
+            sysPropsBackup.put(key, old);
+        }
+        System.clearProperty(key);
+    }
+
+    private void restore(String key) {
+        System.clearProperty(key);
+        if (sysPropsBackup.containsKey(key)) {
+            System.setProperty(key, sysPropsBackup.get(key));
+        }
+    }
+}
