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

feat: CORS headers for served assets #490

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

Conversation

lifeart
Copy link
Contributor

@lifeart lifeart commented Sep 24, 2021

Use case:

  • Static Java application with it's own serving logic and port (port: 5000)
  • Broccoli server running in parallel, to simplify frontend development (port: 4200)

We point to broccoli-served assets from port 5000 to 4200 and getting CORS error.

This MR introduce CORS headers for static assets, and should solve this error.

@lifeart lifeart mentioned this pull request Sep 26, 2021
@lifeart
Copy link
Contributor Author

lifeart commented Sep 26, 2021

partially related #292, but i'm think it's simplier to land CORS, comparing to this stalled MR

@@ -134,6 +134,9 @@ function handleRequest(
return;
}

response.setHeader('Access-Control-Allow-Origin', '*');
response.setHeader('Access-Control-Allow-Methods', 'GET, OPTIONS');
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the original concern from @joliss was that if this was enabled by default, all broccoli development environments could leak information (anything files served by the development middleware) to malicious websites the developers navigates too while running a broccoli service. Although this may seem somewhat esoteric, it exposes a risk I do not feel comfortable with either.

Many security issues are the composition of several not super serious issues. For example, one very dangerous composition would be:

  1. if broccoli development environments had 'Access-Control-Allow-Origin:': '*'
  2. the static file serving middleware, or a user provided middleware, had a path traversal vulnerability.
    ...
  3. any file readable by the development server could be accessed (which compounds if users happen to run the broccoli process with sudo

Other scenarios would be well known files or endpoints that contain data someone malicious can predict, or fuz for.


The approach of allowing users to specify their own headers seems appropriate, as we provide a more secure default experience, and allow them to opt out if they want. We should also discourage 'Access-Control-Allow-Origin': '*', and encourage explicit development domains to guard against scenarios such as the one listed above

Copy link
Contributor

Choose a reason for hiding this comment

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

resurrecting #292 or something targeted specifically at CORS seems appropriate

Copy link
Contributor Author

@lifeart lifeart Sep 27, 2021

Choose a reason for hiding this comment

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

@stefanpenner could we have Access-Control-Allow-Origin for localhost, 0.0.0.0, 192.168.0.1 ? (imho, it will cover 90% usecases)

Copy link
Contributor Author

@lifeart lifeart Sep 27, 2021

Choose a reason for hiding this comment

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

or, we could add cli option --allowed-origins to support CORS (by default we could support only localhost and 192.168.0.1)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hosts that are aliases for localhost seems like a good compromise. Let me think about it through a touch more, and reach out to some other folks to confirm.

good idea!

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.

2 participants