-
-
Notifications
You must be signed in to change notification settings - Fork 536
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
Add Local Network Access support #833
Add Local Network Access support #833
Conversation
79e89e7
to
28f6c9c
Compare
This comment was marked as spam.
This comment was marked as spam.
28f6c9c
to
178e917
Compare
Hey @adamchainz 👋 Did you have a chance to take a look at this PR yet? Anything I can improve or change? |
This comment was marked as spam.
This comment was marked as spam.
This commit adds support for configuring private network access. It allows configuring whether a Django instance residing on an IP from the private IP range can be accessed by a browser enforcing private network access if the CORS origin server resides on a public IP. * Added config variable CORS_ALLOW_PRIVATE_NETWORK_ACCESS * Added code to middleware to send appropriate header if the browser asks for it * Added tests for middleware and config * Updated README and added a short info to the changelog
Changed test to different origin to make it clearer
for more information, see https://pre-commit.ci
* Removed unused import * Marked unreachable test code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updated PR @jjurgens0 . I didn't prioritize reviewing it because it seems like a niche feature that's still in draft but I'm updating and merging it now. I'll address the small comments I've made shortly.
@untidy-hair I don't appreciate comments like "Can you expedite this?" or "Any update?". If you want to help move a PR forwards, review it or add some information like "I have deployed this successfully and it passes my tests".
README.rst
Outdated
@@ -304,6 +304,18 @@ Change the setting to ``'None'`` if you need to bypass this security restriction | |||
|
|||
.. _SESSION_COOKIE_SAMESITE: https://docs.djangoproject.com/en/stable/ref/settings/#std:setting-SESSION_COOKIE_SAMESITE | |||
|
|||
``CORS_ALLOW_PRIVATE_NETWORK_ACCESS: bool`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the setting name is quite wordy, let's cut the word access
which isn't really adding any info and keeps the setting like the header says "allow private network"
README.rst
Outdated
@@ -304,6 +304,18 @@ Change the setting to ``'None'`` if you need to bypass this security restriction | |||
|
|||
.. _SESSION_COOKIE_SAMESITE: https://docs.djangoproject.com/en/stable/ref/settings/#std:setting-SESSION_COOKIE_SAMESITE | |||
|
|||
``CORS_ALLOW_PRIVATE_NETWORK_ACCESS: bool`` | |||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
underline must match title length in reStructuredText
README.rst
Outdated
It set to ``True``, browsers running in CORS mode and enforcing private network access will be | ||
still be allowed to access this Django instance if it is running on an IP from the private IP | ||
address space while the CORS origin's server is running on an IP from the public IP address space. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wordy sentence
CHANGELOG.rst
Outdated
@@ -2,6 +2,9 @@ | |||
Changelog | |||
========= | |||
|
|||
* Added support for `private network access <https://wicg.github.io/local-network-access/>`_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's mention the setting in the changelog
Hmmm.. there's a huge debate about the name "private" versus "local": WICG/private-network-access#91 . I guess we'll ship with "PRIVATE" and support it as an alias if necessary, but still annoying how preliminary the standards are here. |
178e917
to
16255d9
Compare
@adamchainz My apologies for having bothered you with my comments. I will keep it in mind and be more careful in the future. Thank you for the review and the library. |
This PR adds support for private network access. The feature was already added in PR #745, but unfortunately it hasn’t been merged yet. This PR builds on #745 but addresses the review comments in the original PR.
Details about private network access can be found here on the Chrome blog. The original release plan is to enforce private network access in version 111 (at the earliest), which is the next version to be released.
According to the implementation status, the feature might land in Chrome 113.