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

fix: Cors headers #119

Merged
merged 4 commits into from
Oct 12, 2017
Merged

fix: Cors headers #119

merged 4 commits into from
Oct 12, 2017

Conversation

rkostrzewski
Copy link
Collaborator

@developit this change allows only https://localhost:port origin to access served app.

@rkostrzewski rkostrzewski requested a review from developit June 17, 2017 12:49
return [
simplehttp2server,
'-cors', '*',
'-cors', `https://localhost:${port}`,
Copy link
Member

Choose a reason for hiding this comment

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

Can we allow an options.cors that gets appended to this as a CSV?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here you go. FYI simplehttp2server is able to add only one origin.

@rkostrzewski
Copy link
Collaborator Author

@developit this is a breaking change - as it can break stuff on things like now.sh

Copy link
Member

@developit developit left a comment

Choose a reason for hiding this comment

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

lgtm!

@developit
Copy link
Member

@rkostrzewski maybe we should check if there is an env var we can watch for?

@rkostrzewski
Copy link
Collaborator Author

@developit we can check for NOW variable on now.sh, but I don't really think it's worth the trouble as far more providers exist out there. Furthermore setting CORS to * even on hosting provider doesn't seem like a good idea.

@rkostrzewski
Copy link
Collaborator Author

I'd wait for @developit since IMHO this is a major version bump and maybe something smaller will come along.

@developit
Copy link
Member

I think I overestimated the breakage this would cause initially - really the only time someone would run into an issue is if they are trying to fetch assets from their preact-cli output on a page with a differing origin. I think that's a fairly small set of people - I've had such a usecase before, but I would expect to have to muck around with hostnames/headers a bit to make things work.

Should we wait for a bit of breakage to accumulate and make a 2.0.0, or merge and cross our fingers? haha

@lukeed
Copy link
Member

lukeed commented Jul 1, 2017

TBH, I'm not fully aware of the implications, but I have a feeling that (in development) that would be a tiny tiny subset of people.

In production, I'd assume that the requesting origins would be known & specified... or just reverted back to "*".

@developit
Copy link
Member

developit commented Jul 1, 2017

The last time I ran into this was proxying my initial bundle, but leaving it to reference its associated webpack assets from a CDN. The thing is, I had to manually configure Webpack to set the public path to the CDN.

Maybe we could generate a list of allowed origins using some more information:

  • the user's own ip
  • 127.0.0.1
  • localhost
  • any domain found in publicPath
  • the cors CLI arg
  • $HOSTNAME

The combination of these would handle a whole bunch of cases, really leaving only a few where there's not enough information to determine safety.

Also good point - if someone found this to be an issue they can still do --cors *.

FWIW I believe this change only applies to the production web server(s) bundled in.

@reznord reznord added this to the 1.4.0 milestone Jul 10, 2017
@rkostrzewski
Copy link
Collaborator Author

Simple http2server doesn't support multiple origins 😢

@developit
Copy link
Member

(continuing from slack lol) When you suggested setting the header from firebase.json - do you know if that works with simplehttp2server?

@reznord reznord modified the milestones: 2.0, 1.4.0 Jul 16, 2017
@developit developit merged commit 9e2dd15 into master Oct 12, 2017
@lukeed lukeed deleted the fix/cors branch October 20, 2017 16:41
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.

4 participants