-
Notifications
You must be signed in to change notification settings - Fork 530
11744 CORS: echo request Origin + add Vary #11745
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: develop
Are you sure you want to change the base?
Conversation
…ists; prefer comma-separated origins; rely on JVM options/MicroProfile only; add tests and release notes
This comment has been minimized.
This comment has been minimized.
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.
This looks okay to me w.r.t. the CORS changes themselves. I made one comment about the possibility of simplifying some of the parsing. The other request I'd make is to check in the Guides where CORS is mentioned and see if this changes anything, e.g. in the settings documentation (e.g. https://guides.dataverse.org/en/latest/installation/config.html#cors-settings and settings list where minimally the :AllowsCors info should get deleted, but also the notes in the Big Data guide, etc.) There's more discussion of CORS at https://github.com/gdcc/dataverse-previewers/wiki/Using-Previewers-with-download-redirects-from-S3#if-you-are-interested-in-restricting-the-allowedorigins-and-allowedheaders - changes there wouldn't be part of this PR but it would be good to get updated guidance for that for Dataverse v6.9+.
W.r.t. testing, I think there's a lot of useful guidance, I'd also suggest regression testing to make sure that things like direct up/download and previewers that rely on CORS still work after this change and updating the config as described.
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.
Short follow-up summary (reason for additional changes):
While addressing the CORS review feedback, we noticed scattered ad‑hoc regex CSV splits (",\s" / ",\s*") across core code (settings, PID providers, anonymized fields, workflow, tests). We replaced every remaining instance with the existing CsvUtil to unify trimming, quote cleanup, empty‑token removal, and ordering. Added small JvmSettings CSV helpers and refactored CORS + PID provider code to consume them. This is a low‑risk mechanical cleanup done now to prevent further drift, reduce future review overhead, and give us one choke point (CsvUtil) for any future parsing tweaks. Behaviour only changes for pathological inputs (extra commas/quotes) which are now safely normalized.
Changes unknown |
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
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.
This PR now contains two useful additions - CORS changes and standardizing parsing of csv settings. I've left various comments about simplifying some code, removing references to the now unused :AllowCors setting.
I raised two comments in standup today
- are we OK with this as one PR (versus two)?
- are there any concerns about stripping double quote marks from settings? @pdurbin has added a comment trying to get clarification about why @ErykKul thinks this is useful/needed.
My guess is that we can accept this as one PR to avoid extra work, so I think addressing the comments and making a group decision about whether double quotes should be stripped are the main actions.
src/main/java/edu/harvard/iq/dataverse/dataaccess/GlobusOverlayAccessIO.java
Outdated
Show resolved
Hide resolved
…ling and CSV parsing
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
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.
Pull Request Overview
This PR modernizes CORS (Cross-Origin Resource Sharing) handling in Dataverse to support multiple browser origins without proxy changes. It implements proper origin echoing and caching behavior while standardizing CSV parsing across the codebase.
Key changes:
- CORS now echoes the request Origin when it matches the allowlist and adds
Vary: Origin
for proper caching behavior - Introduces standardized comma-separated value parsing via
ListSplitUtil
across the entire codebase - Removes deprecated database-based CORS configuration in favor of JVM/MicroProfile settings only
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/main/java/edu/harvard/iq/dataverse/filter/CorsFilter.java |
Core CORS logic rewritten to echo origins and add Vary header |
src/main/java/edu/harvard/iq/dataverse/util/ListSplitUtil.java |
New utility for consistent CSV parsing across the application |
src/main/java/edu/harvard/iq/dataverse/settings/JvmSettings.java |
Added methods for CSV-aware setting lookups |
src/test/java/edu/harvard/iq/dataverse/filter/CorsFilterTest.java |
Comprehensive test coverage for new CORS behavior |
Various implementation files | Updated to use standardized CSV parsing utility |
Documentation files | Updated configuration guides and release notes |
Comments suppressed due to low confidence (1)
src/main/java/edu/harvard/iq/dataverse/settings/JvmSettings.java:1
- Similar to the optional version, this method unnecessarily joins array values with commas only to split them again. Consider using Arrays.asList(values) or List.of(values) directly.
package edu.harvard.iq.dataverse.settings;
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/main/java/edu/harvard/iq/dataverse/settings/JvmSettings.java
Outdated
Show resolved
Hide resolved
src/test/java/edu/harvard/iq/dataverse/filter/CorsFilterTest.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
…cessing in JvmSettings
This comment has been minimized.
This comment has been minimized.
1 similar comment
📦 Pushed preview images as
🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |
What this PR does / why we need it:
Which issue(s) this PR closes:
Special notes for your reviewer:
Suggestions on how to test this:
Configure comma-separated origins via JVM options/MicroProfile, e.g.:
From a client, send requests with different Origin headers and verify:
Preflight check: Send OPTIONS with Origin + Access-Control-Request-Method. Verify Access-Control-Allow-Methods/Headers reflect sanitized config values.
Automated tests: run CorsFilterTest (JUnit 5) to validate the above behaviors.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
doc/release-notes/11744-cors-echo-origin-vary.md
.Additional documentation:
Verification (Manual Tests)
Replace DV_URL with your base URL and ORIGIN with one you configured.
Preflight (OPTIONS):
Expect:
HTTP/1.1 200
(or appropriate success)Access-Control-Allow-Origin: https://libis.github.io
Vary: Origin
Actual request:
Expect same echoed ACAO and
Vary: Origin
.Multi-origin sanity check (second origin):
Should echo
https://gdcc.github.io
.Wildcard mode check (after setting
*
):Should show
*
(and noVary: Origin
).