-
Notifications
You must be signed in to change notification settings - Fork 407
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
Use commit from PR #6855 to enable --unsafe-cors early #224
Conversation
Codecov Report
@@ Coverage Diff @@
## master #224 +/- ##
==========================================
- Coverage 72.41% 72.34% -0.08%
==========================================
Files 27 27
Lines 2632 2632
==========================================
- Hits 1906 1904 -2
- Misses 614 616 +2
Partials 112 112
Continue to review full report at Codecov.
|
cosmos/cosmos-sdk#6853 was merged. Now pin to that commit (on upstream repo), somewhere between v0.39.0 and v0.39.1 |
This does not seem to be working. Demonet settings okayThis sends a preflight request as browsers do
Same command against this patch
|
Rrr.... time for manual testing |
@webmaster128 what was the command-line you used for the rest-server? |
I wonder if after the Cosmos SDK patch, the |
I can reproduce, with the simple case - no node even, just checking the rest api: Run this:
Returns
Returns 400: |
I don't see any relevant difference between your |
You can look at the code and the defaults here: https://github.com/gorilla/handlers/blob/v1.4.2/cors.go#L169-L173 I made a patch here to explicitly allow all origins: cosmos/cosmos-sdk#6855 but that still didn't help (local tests in wasmd) |
Do those defaults look right? https://github.com/gorilla/handlers/blob/v1.4.2/cors.go#L28-L33 Something I need to adjust? |
Acording to https://github.com/gorilla/handlers/blob/v1.4.2/cors.go#L119-L120, an empty list represent no restrictions |
Ah... this is it: without
|
See: https://github.com/gorilla/handlers/blob/v1.4.2/cors.go#L191-L192 I will make this adjustment to allow Content-Type |
Ah, nice one.
We can also try if an empty list works as all headers |
67b9203
to
111c45e
Compare
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.
Changes look good. untested 👍
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 tested 67b9203 and it works
I cancelled this (CircleCI has a backlog). I will merge after cleaning up go.mod |
Okay, my PR got into the launchpad branch. Pointed to that and cleaned up go.mod. Then time to merge and tag a release. |
update to 0.39.1 when ready
for now, this let's cosmjs tests pass
Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (
docs/
)Added relevant
godoc
comments.Added a relevant changelog entry to the
Unreleased
section inCHANGELOG.md
Re-reviewed
Files changed
in the Github PR explorerFor admin use:
WIP
,R4R
,docs
, etc)