-
-
Notifications
You must be signed in to change notification settings - Fork 472
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: handle private network requests #274
base: master
Are you sure you want to change the base?
feat: handle private network requests #274
Conversation
Besides the The option name itself I'm not sure |
Thanks for your feedback! I pushed a new commit for the default value.
Yes, better! Changed that as well. |
@dougwilson Did you have time to re-review this one? |
It's great to see this PR as we are also very interested in this feature, thank you @joostdebruijn! @dougwilson looking forward to hearing an update from you. Thank you both! |
Hello @dougwilson , Looking forward to this PR since I cannot test my ApolloServer when running on localhost. Thank you for your efforts @joostdebruijn |
Thanks everyone. I will review it today and merge if no comments. In the meantime I do have a question about if this should be landed like this enabled by default; I'm not sure if thay is going to end up getting a sec vulun reported or not; I ask because I assume the point of the web browser change is that this should be opt-in? |
I'm not a security expert. But particularly I find it more transparent to enable this setting explicitly in the client code. |
Looking forward to this PR since we encountered problems in our automation recently after updating Chrome version. We run the automation in private network so we have to add support for the new headers, either by ourself but would be great to use this option. |
Any update on this? It's getting close to Chrome 117 release day 😬 From what I can tell from the conversation, there is an outstanding question of whether this setting should be on by default. IMO it should not be - the setting is there to allow a very specific use case that potentially opens up clients to possible exploitation, so those that enable it must be aware of the consequences of it. |
Please merge this |
Hi @trullock the PR should get changed so it is not on by default as per @bbbates-tl I think. |
@dougwilson I've changed the default value. Private network requests are disabled by default now. |
Is this going to be merged anytime soon? |
Can this please be merged? |
This PR adds support for Private Network Requests via CORS, as described in issue #236.
By default this library is allowing all origins, thus in this PR Private Network Requests are allowed by default as well.
(Closes #236)