Fix IE default redirect url #1373

luisrudge · 2018-05-10T14:46:55Z

lbalmaceda

💯

lbalmaceda · 2018-05-11T20:49:38Z

src/__tests__/core/index.test.js

@@ -37,6 +37,15 @@ describe('setup', () => {
    const model = mock.calls[0][1].toJS();
    expect(model.auth.redirectUrl).toBe('https://test.com/path/');
  });
+  it('default redirectUrl should work when `window.location.origin` is not available', () => {
+    setURL('https://test.com/path/#not-this-part', { noOrigin: true });
+    const options = {};


any reason why you have the declaration separate from the usage, since you're hardcoding the rest of the parameters too?

easy to copy/paste later if needed 😬

lbalmaceda · 2018-05-11T20:52:24Z

src/__tests__/utils/url_utils.test.js

+      var url = 'https://test.com/example';
+      expect(getOriginFromUrl(url)).toBe('https://test.com');
+    });
+    it('should use add the `port` when available', function() {


should ~~use~~ add

~~use add~~ include

lbalmaceda · 2018-05-11T20:53:04Z

src/__tests__/utils/url_utils.test.js

+    const mapping = {
+      'https://localhost:3000/foo?id=1': {
+        href: 'https://localhost:3000/foo?id=1',
+        protocol: 'https:',


protocol should not include the :.

Actually, the function does return the protocol with a colon:

// http://banana.com host:"banana.com" hostname:"banana.com" href:"http://banana.com" protocol:"http:"

Wikipedia says that protocol is:

A non-empty scheme component followed by a colon

... which tells me that protocol should not include it. That said, type this into your console:

> window.location ... < protocol: "https:"

The built-in URL object (MDN) has the colon so I think this is the right implementation. Again, in the console:

> var url = new URL(window.location.toString()); url; ... < protocol: "https:"

lucho is discovering all the greatness of javascript. our boy is growing 😝

lbalmaceda · 2018-05-11T20:53:48Z

src/__tests__/utils/url_utils.test.js

+        protocol: 'https:',
+        host: 'auth0.com',
+        hostname: 'auth0.com',
+        port: undefined,


Should you return undefined or 80 (default)?

It's text parsing, silly! No port in the text === no port found

lbalmaceda · 2018-05-11T20:56:46Z

src/__tests__/utils/url_utils.test.js

+        search: '',
+        hash: ''
+      },
+      'https://auth0.com#access_token=foo': {


If I enter manage.auth0.com, the # is put after the /. .e.g.: https://manage.auth0.com/#/applications

/# and # is the same thing

I tried:

https://manage.auth0.com/#/applications -> fine

https://manage.auth0.com#/applications -> redirs to https://manage.auth0.com/#/applications

https://manage.auth0.com/#applications -> no content in the frame

https://manage.auth0.com#applications -> no content in the frame

So as how I see it, I don't think they are the same. Maybe I'm wrong? Anyway, just add an extra case to test that scenario where the # is after the / such as https://manage.auth0.com/#/applications.

lbalmaceda · 2018-05-11T20:57:05Z

src/__tests__/utils/url_utils.test.js

+        port: undefined,
+        pathname: '',
+        search: '',
+        hash: '#access_token=foo'


why does hash includes the #? Same as port property, it shouldn't. It's already implied in the property name 😂

Same as protocol above, browser parsing with URL includes the #

yeah, it's what window.location.hash returns

lbalmaceda · 2018-05-11T20:58:04Z

src/core/index.js

@@ -273,7 +273,8 @@ function extractAuthOptions(options) {
  // if responseType was not set and there is a redirectUrl, it defaults to code. Otherwise token.
  responseType = typeof responseType === 'string' ? responseType : redirectUrl ? 'code' : 'token';
  // now we set the default because we already did the validation
-  redirectUrl = redirectUrl || `${window.location.origin}${window.location.pathname}`;
+  redirectUrl =
+    redirectUrl || `${getOriginFromUrl(window.location.href)}${window.location.pathname}`;


does this alternative of not using window.location.origin work in every scenario?

yes, if anything, it's way more reliable, since works across all browsers.

lbalmaceda · 2018-05-11T21:01:08Z

src/__tests__/utils/url_utils.test.js

+      expect(getOriginFromUrl(null)).toBe(undefined);
+    });
+    it('should use an anchor to parse the url and return the origin', function() {
+      var url = 'https://test.com/example';


what about http URLs?

pushed some tests with http

joshcanhelp · 2018-05-11T21:22:43Z

src/utils/url_utils.js

@@ -1,5 +1,29 @@
-export function parseUrl(str) {


Is it breaking to remove this?

nope. internal use only

joshcanhelp · 2018-05-11T21:29:37Z

src/__tests__/utils/url_utils.test.js

+    });
+  });
+  describe('getLocationFromUrl', function() {
+    const mapping = {


Might want to add a subdomain test here

joshcanhelp · 2018-05-11T21:47:56Z

src/__tests__/utils/url_utils.test.js

+    const mapping = {
+      'https://localhost:3000/foo?id=1': {
+        href: 'https://localhost:3000/foo?id=1',
+        protocol: 'https:',


Actually, the function does return the protocol with a colon:

// http://banana.com host:"banana.com" hostname:"banana.com" href:"http://banana.com" protocol:"http:"

Wikipedia says that protocol is:

A non-empty scheme component followed by a colon

... which tells me that protocol should not include it. That said, type this into your console:

> window.location ... < protocol: "https:"

The built-in URL object (MDN) has the colon so I think this is the right implementation. Again, in the console:

> var url = new URL(window.location.toString()); url; ... < protocol: "https:"

joshcanhelp · 2018-05-11T21:51:00Z

src/__tests__/utils/url_utils.test.js

+      var url = 'https://test.com/example';
+      expect(getOriginFromUrl(url)).toBe('https://test.com');
+    });
+    it('should use add the `port` when available', function() {


~~use add~~ include

joshcanhelp · 2018-05-11T21:51:53Z

src/__tests__/utils/url_utils.test.js

+        protocol: 'https:',
+        host: 'auth0.com',
+        hostname: 'auth0.com',
+        port: undefined,


It's text parsing, silly! No port in the text === no port found

joshcanhelp · 2018-05-11T21:54:28Z

src/__tests__/utils/url_utils.test.js

+        port: undefined,
+        pathname: '',
+        search: '',
+        hash: '#access_token=foo'


Same as protocol above, browser parsing with URL includes the #

joshcanhelp · 2018-05-11T21:58:30Z

src/utils/url_utils.js

+}
+
+export function getOriginFromUrl(url) {
+  if (!url) {


You're catching an empty string here but ... 👇

joshcanhelp · 2018-05-11T22:00:19Z

src/utils/url_utils.js

+    return undefined;
+  }
+  var parsed = getLocationFromUrl(url);
+  var origin = parsed.protocol + '//' + parsed.hostname;


... this is also a source of an error if you just pass in a string:

> getLocationFromUrl('banana') < null

If there is no match then null is returned.

good catch. pushed a fix.

lbalmaceda

🎉

luisrudge added the CH: Fixed label May 10, 2018

lbalmaceda suggested changes May 11, 2018

View reviewed changes

joshcanhelp reviewed May 11, 2018

View reviewed changes

luisrudge added 3 commits May 23, 2018 15:28

Fix IE default redirect url

2ea60f6

adding http tests + invalid url handling

fbbfa62

fixing tests

361a693

luisrudge force-pushed the fix-origin-ie branch from 1bff085 to 361a693 Compare May 23, 2018 18:28

lbalmaceda approved these changes May 23, 2018

View reviewed changes

luisrudge merged commit a28f31b into master May 23, 2018

luisrudge deleted the fix-origin-ie branch May 23, 2018 18:34

luisrudge added this to the v11.7.0 milestone May 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix IE default redirect url #1373

Fix IE default redirect url #1373

luisrudge commented May 10, 2018

lbalmaceda left a comment

lbalmaceda May 11, 2018

luisrudge May 14, 2018

lbalmaceda May 11, 2018

joshcanhelp May 11, 2018

lbalmaceda May 11, 2018

joshcanhelp May 11, 2018

luisrudge May 14, 2018 •

edited

Loading

lbalmaceda May 11, 2018

joshcanhelp May 11, 2018

lbalmaceda May 11, 2018

luisrudge May 14, 2018

lbalmaceda May 23, 2018

lbalmaceda May 11, 2018

joshcanhelp May 11, 2018

luisrudge May 14, 2018

lbalmaceda May 11, 2018

luisrudge May 14, 2018

lbalmaceda May 11, 2018

luisrudge May 14, 2018

joshcanhelp May 11, 2018

luisrudge May 14, 2018

joshcanhelp May 11, 2018

joshcanhelp May 11, 2018

joshcanhelp May 11, 2018

joshcanhelp May 11, 2018

joshcanhelp May 11, 2018

joshcanhelp May 11, 2018

joshcanhelp May 11, 2018

luisrudge May 14, 2018

lbalmaceda left a comment

Fix IE default redirect url #1373

Fix IE default redirect url #1373

Conversation

luisrudge commented May 10, 2018

lbalmaceda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisrudge May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbalmaceda left a comment

Choose a reason for hiding this comment

luisrudge May 14, 2018 •

edited

Loading