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(urlUtils): make IPv6 URL's hostname wrapped in square brackets in IE/Edge #16715

Merged
merged 1 commit into from Oct 18, 2018

Conversation

@mgol
Copy link
Member

mgol commented Oct 11, 2018

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

A bug fix.

What is the current behavior? (You can also link to an open issue here)

In IE 9-11 and Edge 16-17 (not in 18 Preview) $location.host() incorrectly returns hostnames of IPv6 addresses without wrapping in square brackets.

This is caused by and IE/Edge bug which incorrectly don't wrap IPv6 addresses' hostnames in square brackets when parsed out of an anchor element which is used by $location for the purpose of URL parsing.

Issue: #16692

What is the new behavior (if this is a feature change)?

For all supported browsers $location.host() returns an IPv6 address wrapped in square brackets.

Does this PR introduce a breaking change?

No

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Fix/Feature: Docs have been added/updated
  • Fix/Feature: Tests have been added; existing tests pass

Other information:

@mgol mgol added this to the 1.7.x milestone Oct 11, 2018
@mgol mgol self-assigned this Oct 11, 2018
@googlebot googlebot added the cla: yes label Oct 11, 2018
@mgol mgol force-pushed the mgol:ipv6-ie-edge branch from a6c3fb7 to 4955a2b Oct 11, 2018
mgol added a commit to mgol/angular.js that referenced this pull request Oct 11, 2018
… IE/Edge

IE 9-11 and Edge 16-17 (fixed in 18 Preview) incorrectly don't wrap IPv6
addresses' hostnames in square brackets when parsed out of an anchor element.

Fixes angular#16692
Closes angular#16715
@mgol

This comment has been minimized.

Copy link
Member Author

mgol commented Oct 11, 2018

I added a support test to avoid an overhead of additional regex testing per each URL passing through urlResolve.

@@ -72,13 +78,19 @@ function urlResolve(url) {

urlParsingNode.setAttribute('href', href);

var hostname = urlParsingNode.hostname;

if (!ipv6InBrackets && /[\da-e:]+/.test(hostname)) {

This comment has been minimized.

Copy link
@gkalpak

gkalpak Oct 11, 2018

Member

Why not a-f in the RegExp?
Also, you probably meant /^[...]+$/ (to match the whole hostname, not just a part of it).

This comment has been minimized.

Copy link
@mgol

mgol Oct 11, 2018

Author Member

Off-by-one error! I broke https://google.com parsing. 😱 Fixed.

@mgol mgol force-pushed the mgol:ipv6-ie-edge branch from 4955a2b to 60a0aa6 Oct 11, 2018
mgol added a commit to mgol/angular.js that referenced this pull request Oct 11, 2018
… IE/Edge

IE 9-11 and Edge 16-17 (fixed in 18 Preview) incorrectly don't wrap IPv6
addresses' hostnames in square brackets when parsed out of an anchor element.

Fixes angular#16692
Closes angular#16715
Copy link
Member

gkalpak left a comment

LGTM

src/ng/urlUtils.js Outdated Show resolved Hide resolved
src/ng/urlUtils.js Outdated Show resolved Hide resolved
… IE/Edge

IE 9-11 and Edge 16-17 (fixed in 18 Preview) incorrectly don't wrap IPv6
addresses' hostnames in square brackets when parsed out of an anchor element.

Fixes #16692
Closes #16715
@mgol mgol force-pushed the mgol:ipv6-ie-edge branch from 60a0aa6 to d1e5a65 Oct 17, 2018
@mgol mgol merged commit b4e409b into angular:master Oct 18, 2018
2 checks passed
2 checks passed
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mgol mgol deleted the mgol:ipv6-ie-edge branch Oct 18, 2018
mgol added a commit that referenced this pull request Oct 18, 2018
… IE/Edge

IE 9-11 and Edge 16-17 (fixed in 18 Preview) incorrectly don't wrap IPv6
addresses' hostnames in square brackets when parsed out of an anchor element.

Fixes #16692
Closes #16715
@GulajavaMinistudio GulajavaMinistudio mentioned this pull request Oct 18, 2018
0 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.