Skip to content
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

Adds ETag to Access-Control-Expose-Headers headers #668

Closed
wants to merge 3 commits into from

Conversation

twick00
Copy link

@twick00 twick00 commented Jul 30, 2024

I mentioned this in issue #667 but suffice to say that, while ETag is available as a header from many of the APIs in S3Proxy, the browser cannot access it since it's missing from the Access-Control-Expose-Headers list. To allow accessing this header from the browser, it needs to be added to the Access-Control-Expose-Headers header.

@gaul
Copy link
Owner

gaul commented Aug 4, 2024

@twick00 Please fix the stylistic issues.

@reimannf Does this seem correct to you? Sorry to bug you years after #287 but I don't have any background on this.

@twick00
Copy link
Author

twick00 commented Aug 4, 2024

I also created a codesandbox example that demonstrates the issue, but it has a few caveats:

  1. Ensure the endpoint and bucket values are pointed to an S3 proxy configured with SSL
  2. The S3Proxy should be running with s3proxy.cors-allow-all=true or properly configured SSL options

@reimannf
Copy link
Contributor

reimannf commented Aug 5, 2024

The extension to add Access-Control-Expose-Headers to the CORS response makes sense. Nevertheless I would make the list of Headers included in Access-Control-Expose-Headers configurable and would not hardcode ETag there.
@twick00 maybe you could look here https://github.com/gaul/s3proxy/blob/master/src/main/java/org/gaul/s3proxy/S3Proxy.java#L271-L272 as an example for allowed headers and pass it accordingly to CrossOriginResourceSharing extended constructor https://github.com/gaul/s3proxy/blob/master/src/main/java/org/gaul/s3proxy/CrossOriginResourceSharing.java#L62

@twick00
Copy link
Author

twick00 commented Aug 5, 2024

@reimannf, I had those changes ready to go locally so I created a PR (#674) instead of adding to this one, however, I'm not sure if it's the right solution. The fact that nobody has brought up the issue in the last 2 years shows that few people might care about customizing that Access-Control-Expose-Headers response so I decided to start with this PR since it seems like a much lighter solution. I didn't want to introduce a new configuration knob for people, IMO Access-Control-Expose-Headers should "just work".

Seems to me like the only 2 headers that most people care about are etag and location. Heres an example of the lib that works with S3Proxy; notice that it only cares about those two headers I mentioned. For my specific case, I only care about ETag, but I could easily add location to the list of exposed headers.

If you really feel like you would prefer a new configuration knob then feel free to move over to #674 instead of this and I'll close this one.

@reimannf
Copy link
Contributor

reimannf commented Aug 6, 2024

Understand your arguments, but as you described for this specific lib, one would need already 2 headers.
I am not up to date regarding AWS S3 default CORS policy, but I think the ETag is not included there as Access-Control-Expose-Headers.
I would prefer an option, to configure the Access-Control-Expose-Headers as it would also allow custom headers to be included.
The final word has @gaul as I am not a maintainer, but my preference is #674

@twick00
Copy link
Author

twick00 commented Aug 6, 2024

I'll close this for now and we can migrate to #674

@twick00 twick00 closed this Aug 6, 2024
@gaul
Copy link
Owner

gaul commented Aug 7, 2024

Thank you @reimannf for your feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants