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

Enable permissive CORS on static file serving #1397

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

marius851000
Copy link
Contributor

@marius851000 marius851000 commented Jul 23, 2024

I had this issue while experimenting with CI for MR preview of static site with Hydra. The same issue also impact the Nix and Hydra manual, where font icons are not displayed (at least with recent Firefox and Chromium).

(https://hydra.nixos.org/build/263397466/download/1/manual/)

I’m not really sure why CORS is needed for some request and not other ones. I suspect this is due to the Content-Security-Policy=sandbox header, as the same program I have work well on another deployment that don’t send that header.

This should stay secure, as only static files are served, but will allow other sites to embed built content served by Hydra.

PS: MDN confirm that this won’t send Cookies or other Authentification information (in https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Credentials): "Credentials are cookies, TLS client certificates, or authentication headers containing a username and password. By default, these credentials are not sent in cross-origin requests, and doing so can make a site vulnerable to CSRF attacks."

@Mindavi
Copy link
Contributor

Mindavi commented Jul 27, 2024

Generally speaking, I wouldn't recommend using hydra to serve static content. Instead upload the data somewhere else before serving it.

Otherwise, CORS is not really something I know a lot about, so I'll defer reviewing safety of this to someone else.

@@ -237,6 +237,7 @@ sub serveFile {
# Have the hosted data considered its own origin to avoid being a giant
# XSS hole.
$c->response->header('Content-Security-Policy' => 'sandbox allow-scripts');
$c->response->header('Access-Control-Allow-Origin', '*');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hsjobeki Can this be abused in any way?

@Mic92
Copy link
Member

Mic92 commented Aug 21, 2024

PS: MDN confirm that this won’t send Cookies or other Authentification information (in https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Credentials): "Credentials are cookies, TLS client certificates, or authentication headers containing a username and password. By default, these credentials are not sent in cross-origin requests, and doing so can make a site vulnerable to CSRF attacks.

This is only for cross-domain requests, but our problem is that hydra.nixos.org is the same domain as the build. The only way I can see how can be made secure is by having builds on a different subdomain (maybe for even each build even)

@marius851000
Copy link
Contributor Author

marius851000 commented Aug 21, 2024 via email

@dasJ
Copy link
Member

dasJ commented Aug 21, 2024

Since I hope you don't run Hydra without a reverse proxy like nginx, why not set the header there?

@hsjobeki
Copy link

hsjobeki commented Aug 21, 2024

So i think we're kind of misunderstanding each other.

Problem: https://hydra.nixos.org/build/263397466/download/1/manual/ has broken fonts and icons somehow because of cors.

Another problem:

Same origin policy (as of right now) allows any build page to access any other build resource.

I.e. https://hydra.nixos.org/build/263397466/download/1/manual/ could access https://hydra.nixos.org/priviate because of the same origin.

To solve this the build-id could become part of the origin? (If accessing cross resources is a security issue).

https://263397466.hydra.nixos.org

The proposed solution for the first problem:

'Access-Control-Allow-Origin', '*')

Solves the issue, but also opens up even more cross site scripting. Although no-credentials would be sen, as mentioned.
I didn't figure out why the fonts files couldn't be loaded strangely. Maybe you can find a narrower setting than "*" ?

@marius851000
Copy link
Contributor Author

marius851000 commented Aug 21, 2024 via email

@Mic92
Copy link
Member

Mic92 commented Aug 21, 2024

Since I hope you don't run Hydra without a reverse proxy like nginx, why not set the header there?

I think this was specifically about hydra.nixos.org.

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.

5 participants