fix: registered native loopback redirect_uris do not get normalized

panva · Dec 2, 2019 · 96e035f · 96e035f
1 parent d7b927d
commit 96e035f
Show file tree

Hide file tree

Showing 4 changed files with 28 additions and 34 deletions.
diff --git a/lib/consts/client_attributes.js b/lib/consts/client_attributes.js
@@ -147,7 +147,7 @@ const HTTPS_URI = [
   'sector_identifier_uri',
 ];
 
-const LOOPBACKS = ['localhost', '127.0.0.1', '[::1]'];
+const LOOPBACKS = new Set(['localhost', '127.0.0.1', '[::1]']);
 
 const ENUM = {
   application_type: () => ['native', 'web'],

diff --git a/lib/helpers/client_schema.js b/lib/helpers/client_schema.js
@@ -248,7 +248,6 @@ module.exports = function getSchema(provider) {
       this.postLogoutRedirectUris();
       this.redirectUris();
       this.webMessageUris();
-      this.normalizeNativeAppUris();
       this.checkContacts();
 
       // max_age and client_secret_expires_at format
@@ -473,21 +472,6 @@ module.exports = function getSchema(provider) {
       this.response_types = this.response_types.map((type) => [...new Set(type.split(' '))].sort().join(' '));
     }
 
-    normalizeNativeAppUris() {
-      if (this.application_type === 'web') return;
-
-      this.redirect_uris = this.redirect_uris.map((redirectUri) => {
-        const parsed = new url.URL(redirectUri);
-        // remove the port component, making dynamic ports allowed for loopback uris
-        if (parsed.protocol === 'http:' && LOOPBACKS.includes(parsed.hostname)) {
-          parsed.port = 80; // http + 80 = no port part in the string
-          return parsed.href;
-        }
-
-        return redirectUri;
-      });
-    }
-
     postLogoutRedirectUris() {
       this.post_logout_redirect_uris.forEach((uri) => {
         try {
@@ -554,12 +538,12 @@ module.exports = function getSchema(provider) {
           case 'native': {
             switch (protocol) {
               case 'http:': // Loopback Interface Redirection
-                if (!LOOPBACKS.includes(hostname)) {
+                if (!LOOPBACKS.has(hostname)) {
                   this.invalidate('redirect_uris for native clients using http as a protocol can only use loopback addresses as hostnames');
                 }
                 break;
               case 'https:': // Claimed HTTPS URI Redirection
-                if (LOOPBACKS.includes(hostname)) {
+                if (LOOPBACKS.has(hostname)) {
                   this.invalidate(`redirect_uris for native clients using claimed HTTPS URIs must not be using ${hostname} as hostname`);
                 }
                 break;

diff --git a/lib/models/client.js b/lib/models/client.js
@@ -436,22 +436,32 @@ module.exports = function getClient(provider) {
       return this.grantTypes.includes(type);
     }
 
-    redirectUriAllowed(redirectUri) {
-      let checkedUri = redirectUri;
+    redirectUriAllowed(value) {
+      let parsed;
+      try {
+        parsed = new URL(value);
+      } catch (err) {
+        return false;
+      }
+
+      const match = this.redirectUris.includes(value);
       if (
-        this.applicationType === 'native'
-        && redirectUri.startsWith('http:')
+        match
+        || this.applicationType !== 'native'
+        || !parsed.protocol === 'http:'
+        || !LOOPBACKS.has(parsed.hostname)
       ) {
-        try {
-          const parsed = new URL(redirectUri);
-          if (LOOPBACKS.includes(parsed.hostname)) {
-            parsed.port = 80;
-            checkedUri = parsed.href;
-          }
-        } catch (err) {}
+        return match;
       }
 
-      return this.redirectUris.includes(checkedUri);
+      parsed.port = '';
+
+      return !!this.redirectUris
+        .find((registeredUri) => {
+          const registered = new URL(registeredUri);
+          registered.port = '';
+          return parsed.href === registered.href;
+        });
     }
 
     checkSessionOriginAllowed(origin) {

diff --git a/test/oauth_native_apps/oauth_native_apps.test.js b/test/oauth_native_apps/oauth_native_apps.test.js
@@ -87,7 +87,7 @@ describe('OAuth 2.0 for Native Apps Best Current Practice features', () => {
           token_endpoint_auth_method: 'none',
           redirect_uris: ['http://localhost:2355/op/callback'],
         }).then((client) => {
-          expect(client.redirectUris).to.contain('http://localhost/op/callback');
+          expect(client.redirectUris).to.contain('http://localhost:2355/op/callback');
           expect(client.redirectUriAllowed('http://localhost/op/callback')).to.be.true;
           expect(client.redirectUriAllowed('http://localhost:80/op/callback')).to.be.true;
           expect(client.redirectUriAllowed('http://localhost:443/op/callback')).to.be.true;
@@ -123,7 +123,7 @@ describe('OAuth 2.0 for Native Apps Best Current Practice features', () => {
           token_endpoint_auth_method: 'none',
           redirect_uris: ['http://127.0.0.1:2355/op/callback'],
         }).then((client) => {
-          expect(client.redirectUris).to.contain('http://127.0.0.1/op/callback');
+          expect(client.redirectUris).to.contain('http://127.0.0.1:2355/op/callback');
           expect(client.redirectUriAllowed('http://127.0.0.1/op/callback')).to.be.true;
           expect(client.redirectUriAllowed('http://127.0.0.1:80/op/callback')).to.be.true;
           expect(client.redirectUriAllowed('http://127.0.0.1:443/op/callback')).to.be.true;
@@ -159,7 +159,7 @@ describe('OAuth 2.0 for Native Apps Best Current Practice features', () => {
           token_endpoint_auth_method: 'none',
           redirect_uris: ['http://[::1]:2355/op/callback'],
         }).then((client) => {
-          expect(client.redirectUris).to.contain('http://[::1]/op/callback');
+          expect(client.redirectUris).to.contain('http://[::1]:2355/op/callback');
           expect(client.redirectUriAllowed('http://[::1]/op/callback')).to.be.true;
           expect(client.redirectUriAllowed('http://[::1]:80/op/callback')).to.be.true;
           expect(client.redirectUriAllowed('http://[::1]:443/op/callback')).to.be.true;