Sign in with Ethereum: domain and origin should compare without port#18296
Sign in with Ethereum: domain and origin should compare without port#18296yonigoldberg wants to merge 4 commits intoMetaMask:developfrom
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
|
I have read the CLA Document and I hereby sign the CLA |
d839df4 to
9b0beb7
Compare
|
Amazing, came to the repo to make this exact change. The hostname will always map to the domain and drop the port, preventing the domain binding bug from happening on localhost. This change looks great to me. |
2db2321 to
b97c019
Compare
digiwand
left a comment
There was a problem hiding this comment.
Lgtm! Thanks for adding the fix and test for this @yonigoldberg!
I think it'd be a good idea to add another test to check the case where domain contains the port number as well. I don't think this is a blocker for this PR, though I think we should add it here or in a follow-up PR
The parsed SIWE domain might or might not have a port, therefore when it is compared with the origin it should handle both cases.
digiwand
left a comment
There was a problem hiding this comment.
I think that's a good idea to limit the scope to localhost. Blocking this PR now to apply the change and continue the discussion above.
…e.js Co-authored-by: Zbyszek Tenerowicz <naugtur@gmail.com>
Codecov Report
@@ Coverage Diff @@
## develop #18296 +/- ##
===========================================
- Coverage 64.55% 64.55% -0.00%
===========================================
Files 919 919
Lines 35445 35446 +1
Branches 9119 9120 +1
===========================================
- Hits 22880 22879 -1
- Misses 12565 12567 +2
... and 2 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
Left some thoughts @ #18188. PTAL. |
|
Hey @yonigoldberg : can you plz fix lint issue. |
There was a problem hiding this comment.
having an interesting conversation with @legobeat.
@yonigoldberg, we will need to hold off on merging this right now while we figure this out. Thank you for your patience with this. I am curious to know if you have found a workaround the match the port numbers in the meantime. Hoping you have
For clarification, I'm realizing the issue ticket that is currently related to this PR is only concerned with the http/https protocol rather than the port. maybe we can unrelate the tickets or combine the issues
Other related tickets:
|
Per the conversation in #18188 I think we can close this PR. |
|
Thank you for opening this PR, your input, and keeping the conversation moving @yonigoldberg |
|
^ +1 |
Explanation
The parsed SIWE domain doesn't have a port (e.g loclhost) therefore when it is compared with the origin, the host from the origin should be without a port as well. That is achieved by using
hostnameinstead ofhost.Screenshots/Screencaps
Before
### After
## Manual Testing Steps - Run any local server and try to login with SIWEPre-merge author checklist
Pre-merge reviewer checklist
If further QA is required (e.g. new feature, complex testing steps, large refactor), add the
Extension QA Boardlabel.In this case, a QA Engineer approval will be be required.