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

Fix ENS resolution of .eth URLs with query strings #9674

Merged
merged 1 commit into from Oct 22, 2020

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Oct 22, 2020

Our ENS resolver for the browser address bar was incorrectly resolving addresses that included query strings. We were concatenating the path property with the search property, despite the fact that the path property already contains search. As a result, search was duplicated in the resolved addresses.

For example, if an IPFS content ID was found for this address, the resolved address for metamask.eth/?foo=bar would have the path /?foo=bar?foo=bar

The original intent was likely to use pathname in place of path. The resolver has been updated to use pathname, and the query string now appears only once in the resolved address.

Our ENS resolver for the browser address bar was incorrectly resolving
addresses that included query strings. We were concatenating the `path`
property with the `search` property, despite the fact that the `path`
property already contains `search`. As a result, `search` was
duplicated in the resolved addresses.

For example, if an IPFS content ID was found for this address, the
resolved address for `metamask.eth/?foo=bar` would have the path
`/?foo=bar?foo=bar`

The original intent was likely to use `pathname` in place of `path`.
The resolver has been updated to use `pathname`, and the query string
now appears only once in the resolved address.
@Gudahtt Gudahtt requested a review from a team as a code owner October 22, 2020 03:29
@Gudahtt Gudahtt requested a review from kumavis October 22, 2020 03:29
@Gudahtt
Copy link
Member Author

Gudahtt commented Oct 22, 2020

DevTools screenshots:

Before:

test-duplicate-search

After:

resolve-after

@metamaskbot
Copy link
Collaborator

Builds ready [445cf60]
Page Load Metrics (503 ± 62 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaintNaNNaNNaNNaNNaN
domContentLoaded33578350212862
load33778450312862
domInteractive33578250112862

@Gudahtt Gudahtt merged commit 07cffe9 into develop Oct 22, 2020
@Gudahtt Gudahtt deleted the fix-address-bar-ENS-resolution-search-parameters branch October 22, 2020 13:15
@github-actions github-actions bot locked and limited conversation to collaborators Oct 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants