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

add Access-Control-Allow-Credentials header #416

Merged
merged 2 commits into from
Sep 26, 2023
Merged

Conversation

st-h
Copy link
Contributor

@st-h st-h commented Apr 18, 2022

fixes #415

@gaul
Copy link
Owner

gaul commented Apr 18, 2022

@reimannf could you review this?

@reimannf
Copy link
Contributor

I think CORS does not include cookies/credentials on cross-origin requests by default. Both, client and server has to opt-in if they want to do this. So instead of always sending that header, which might open some attacking space, I suggest to introduce a config property PROPERTY_CORS_ALLOW_CREDENTIAL here:

public static final String PROPERTY_CORS_ALLOW_HEADERS =
"s3proxy.cors-allow-headers";

You might want to send the header always with the value configured in PROPERTY_CORS_ALLOW_CREDENTIAL unless you default to something different than true and make this an active decision.

Repository owner deleted a comment from reimannf Jun 12, 2022
@gaul
Copy link
Owner

gaul commented Sep 26, 2022

@st-h Could you address the review comment so I can merge this?

@st-h
Copy link
Contributor Author

st-h commented Sep 27, 2022

@gaul thanks for the reminder. I totally forgot this PR was still open and I was chugging along with my local docker image. Would have been up for a surprise when switching to a different computer. I just added the requested property.

@st-h
Copy link
Contributor Author

st-h commented Jun 23, 2023

@gaul Any chance we can still merge and release this?

@gaul gaul merged commit b134e81 into gaul:master Sep 26, 2023
@gaul
Copy link
Owner

gaul commented Sep 26, 2023

Thank you for your contribution and patience @st-h!

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.

[cors] missing Access-Control-Allow-Credentials header
3 participants