Skip to content

Fix IE11 string comparison issue (ASH-54) #4

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

Merged
merged 3 commits into from
May 14, 2018

Conversation

uifox
Copy link
Collaborator

@uifox uifox commented May 10, 2018

When IE11 converted the redirect URL string using a URI parser it added a port even though the original URL didn't contain any. Since this port isn't present in the auth iframe's location, the authentication always timed out. This commit fixes this issue by omitting the parser technique.

…ed a port even though the original URL didn't contain any. Since this port isn't present in the auth iframe's location, the authentication always timed out. This commit fixes this issue by omitting the parser technique.
@uifox uifox self-assigned this May 10, 2018
@bsarvari bsarvari requested a review from mikewisian May 10, 2018 16:01
Copy link
Collaborator

@mikewisian mikewisian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like redirectUriParser was attempting to solve a problem. Do you know what that might be? My hesitation to approve this is that redirectUriParser was attempting to normalize inconsistencies of browser implementations. It's possible it to support a legacy browser that doxo no longer supports, but I just want to make sure that you've reasonably tested it with the browsers we support to make sure this is truly no longer needed before calling it good.

@bsarvari bsarvari requested a review from tylerritchie May 11, 2018 07:05
@bsarvari
Copy link
Owner

bsarvari commented May 11, 2018

@mikewisian
I think there is rationale behind your suspect. I studied the original solution a bit more and experimented with a few of the idiosyncratic browsers (IE11, old Safari) to see why they originally came up with a utility to normalize URLs. Most likely they did this because (the very least) IE11 has a somewhat inconsistent location API implementation, where it sets the port of location objects to 80 or 443 for the http and https protocols if the port is missing from the URL. This becomes an issue when the code compares the configured auth callback URL with the destination of the auth iframe.

Because the original solution was broken in old Safari (see ASH-11) I changed the getFullUrlPath() utility, which—as it turns out now—does not please IE.

Although removing the redirect URL parser from the auth loop fixes the IE11 issue, I think a better approach would be restoring the original getFullUrlPath() method so that it always include the default port numbers and handle the '0' port issue we experienced in Safari differently.

Stay tuned. A new checkin is on its way.

@uifox uifox changed the title Fix IE11 string comparison issue Fix IE11 string comparison issue (ASH-54) May 14, 2018
@uifox uifox merged commit e064148 into master May 14, 2018
@uifox uifox deleted the defect/IE11-string-comparison branch May 14, 2018 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants