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: handle private network requests #274

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

joostdebruijn
Copy link

@joostdebruijn joostdebruijn commented Aug 2, 2022

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)

lib/index.js Outdated Show resolved Hide resolved
@dougwilson
Copy link
Contributor

Besides the default issue above, it looks good, thank you for your work, it is really appreciated!

The option name itself I'm not sure Access postfix is really necessary, as all the options are about access. What about just allowPrivateNetwork?

@joostdebruijn
Copy link
Author

Thanks for your feedback! I pushed a new commit for the default value.

What about just allowPrivateNetwork?

Yes, better! Changed that as well.

@joostdebruijn
Copy link
Author

@dougwilson Did you have time to re-review this one?

@jaime-rivas
Copy link

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!

@edokan
Copy link

edokan commented Dec 29, 2022

Hello @dougwilson ,

Looking forward to this PR since I cannot test my ApolloServer when running on localhost. Thank you for your efforts @joostdebruijn

@gersonfs
Copy link

I'm also quite interested in this PR so we don't have any problems in the next chromium updates.

image

@dougwilson dougwilson self-assigned this Feb 10, 2023
@dougwilson
Copy link
Contributor

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?

@gersonfs
Copy link

I'm not a security expert. But particularly I find it more transparent to enable this setting explicitly in the client code.

@skambalin
Copy link

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.

@bbbates-tl
Copy link

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.

@trullock
Copy link

Please merge this

@dougwilson
Copy link
Contributor

Hi @trullock the PR should get changed so it is not on by default as per @bbbates-tl I think.

@joostdebruijn
Copy link
Author

@dougwilson I've changed the default value. Private network requests are disabled by default now.

@Sahib08
Copy link

Sahib08 commented Apr 19, 2024

Is this going to be merged anytime soon?

@deanylev
Copy link

Can this please be merged?

@bjohansebas bjohansebas requested review from jonchurch and removed request for dougwilson November 20, 2024 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Question / Feature Request] CORS-RFC1918 Support
10 participants