-
Notifications
You must be signed in to change notification settings - Fork 937
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
Feature CORS-RFC1918 Support #4305
Feature CORS-RFC1918 Support #4305
Conversation
…itscher/firebase-tools into feature-cors-rfc1918-support
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). For more information, open the CLA check for this pull request. |
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.
This seems pretty reasonable to me. Could you take a look at the comments and re-request a review? I'll re-review and then make sure someone who knows the storage paths better than me takes a look (probably @yuchenshi ) :)
Could you also add an entry to the CHANGELOG? Something like
- Adds appropriate headers to Auth and Storage emulators when accessing them via a tunnel (#4227).
Co-authored-by: Bryan Kendall <bryan.a.kendall@gmail.com>
Co-authored-by: Bryan Kendall <bryan.a.kendall@gmail.com>
endOfLine rule was temporarily changed for prettier/prettier to allow tests to pass on Windows + VS Code. Reverting to original repo settings
Hey team. Anything holding this one up? Did you need anything more from me? |
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.
I've suggested the fix for the tests, but LGTM!
Head branch was pushed to by a user without write access
Hey @bkendall or @yuchenshi just wondering if you can help as to why these tests are failing? I can't seem to see any information in the details and I am not too sure what I have wront. Sorry first time doing PR here. |
@pklitscher looks like the formatter is failing. Running You should be able to run |
Thank you @bkendall I am hitting two issues.
If I change the rule and run
Sorry for my lack of knowledge here - not sure where I am going wrong and don't want to touch how your linting and tests are setup. |
You may be able to make the eslint change, run the formatter, then revert the eslint change. Alternatively, I think you may just have to add a newline to the CHANGELOG file - that might get you past that error. I'm not sure what the differences are here, unfortunately - most of the development environments I deal with are linux or macOS, so this kind of issue I don't hit regularly. I may be able to take your branch, fix the formatting and submit it. Would that be alright? |
Co-authored-by: Bryan Kendall <bryan.a.kendall@gmail.com>
Co-authored-by: Yuchen Shi <yuchenshi@google.com>
…itscher/firebase-tools into feature-cors-rfc1918-support
@bkendall I think I cleared the issue with CHANGELOG by adding a heading and a newline as you mentioned. Still seems to be failing on some of the integration tests so I am happy for you to grab the branch and submit. Apologies for the hassle for such a small change. Thank you for your help. |
Codecov ReportBase: 56.99% // Head: 56.99% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #4305 +/- ##
==========================================
- Coverage 56.99% 56.99% -0.01%
==========================================
Files 292 292
Lines 19508 19516 +8
Branches 3902 3904 +2
==========================================
+ Hits 11119 11123 +4
- Misses 7452 7456 +4
Partials 937 937
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
* Add support for cors rfc1918 * Remove null from next() Co-authored-by: Bryan Kendall <bryan.a.kendall@gmail.com> * Remove null from next() Co-authored-by: Bryan Kendall <bryan.a.kendall@gmail.com> * Revert incorrect linting changes endOfLine rule was temporarily changed for prettier/prettier to allow tests to pass on Windows + VS Code. Reverting to original repo settings * Update CHANGELOG.md * Update src/test/emulators/auth/rest.spec.ts Co-authored-by: Bryan Kendall <bryan.a.kendall@gmail.com> * Update CHANGELOG.md * Update CHANGELOG * Update src/emulator/storage/server.ts Co-authored-by: Yuchen Shi <yuchenshi@google.com> * Update CHANGELOG.md Co-authored-by: Bryan Kendall <bryan.a.kendall@gmail.com> Co-authored-by: Yuchen Shi <yuchenshi@google.com> Co-authored-by: Bryan Kendall <bkend@google.com>
Description
Adding CORS-RFC1918 Support to Auth and Storage.
Allows emulated firebase features to be accessed at localhost:#### from a site that is exposed via tunnel such as localtunnel or ngrok. See issue #4227 & https://wicg.github.io/private-network-access/#headers
Change adds
access-control-allow-private-network=true
header to api responses whenaccess-control-request-private-network
is present.This change should be replaced in favor of a expressjs/cors option if adopted. See expressjs/cors#236
If PR accepted please consider also adding
access-control-allow-private-network=true
header to RTDB and Firestore.Scenarios Tested
Attempt to create a user/sign in, and upload file when site is exposed via a tunnel.
Setup
lt -p 5000 -s firebase-emulator-cors-test
Test Authentication
Test Storage
Sample Commands
N/A