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.

Sign up for GitHub

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
Open

feat: CORS headers for served assets #490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/middleware.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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!


response.setHeader('Cache-Control', 'private, max-age=0, must-revalidate');
response.setHeader('Content-Length', stat.size);
response.setHeader('Content-Type', mime.contentType(path.extname(filename)));
Expand Down
2 changes: 2 additions & 0 deletions test/server_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,8 @@ describe('server', function() {
expect(body).to.eql('Hello');
expect(headers['last-modified']).to.eql('Fri, 27 Jul 2018 17:23:02 GMT');
expect(headers['cache-control']).to.eql('private, max-age=0, must-revalidate');
expect(headers['access-control-allow-origin']).to.eql('*');
expect(headers['access-control-allow-methods']).to.eql('GET, OPTIONS');
expect(headers['content-length']).to.eql('5');
expect(headers['content-type']).to.eql('text/plain; charset=utf-8');
}
Expand Down