fix: block static client registration read action (edgiest of cases)

panva · Jul 8, 2019 · 18db430 · 18db430
1 parent 74a0f04
commit 18db430
Show file tree

Hide file tree

Showing 4 changed files with 24 additions and 9 deletions.
diff --git a/lib/actions/registration.js b/lib/actions/registration.js
@@ -148,6 +148,10 @@ module.exports = {
     ...validateRegistrationAccessToken,
 
     async function clientReadResponse(ctx, next) {
+      if (ctx.oidc.client.noManage) {
+        throw new InvalidRequest('client does not have permission to read its record', 403);
+      }
+
       ctx.body = ctx.oidc.client.metadata();
 
       Object.assign(ctx.body, {
@@ -181,24 +185,22 @@ module.exports = {
     async function equalChecks(ctx, next) {
       ctx.assert(ctx.oidc.body.client_id === ctx.oidc.client.clientId, new InvalidRequest('provided client_id does not match the authenticated client\'s one'));
 
-      if (typeof ctx.oidc.body.client_secret === 'string') {
+      if ('client_secret' in ctx.oidc.body) {
         const clientSecretValid = constantEquals(
-          ctx.oidc.body.client_secret,
-          ctx.oidc.client.clientSecret,
+          typeof ctx.oidc.body.client_secret === 'string' ? ctx.oidc.body.client_secret : '',
+          ctx.oidc.client.clientSecret || '',
           1000,
         );
 
         ctx.assert(clientSecretValid, new InvalidRequest('provided client_secret does not match the authenticated client\'s one'));
-      } else if ('client_secret' in ctx.oidc.body) {
-        throw new InvalidRequest('provided client_secret does not match the authenticated client\'s one');
       }
 
       await next();
     },
 
     async function clientUpdateResponse(ctx, next) {
       if (ctx.oidc.client.noManage) {
-        throw new InvalidRequest('this client is not allowed to update its records', 403);
+        throw new InvalidRequest('client does not have permission to update its record', 403);
       }
 
       const properties = chain({}).defaults({
@@ -275,7 +277,7 @@ module.exports = {
 
     async function clientRemoveResponse(ctx, next) {
       if (ctx.oidc.client.noManage) {
-        throw new InvalidRequest('this client is not allowed to delete itself', 403);
+        throw new InvalidRequest('client does not have permission to delete its record', 403);
       }
 
       const { oidc: { provider } } = ctx;

diff --git a/test/dynamic_registration/dynamic_registration.config.js b/test/dynamic_registration/dynamic_registration.config.js
@@ -6,4 +6,9 @@ config.features = { registration: { enabled: true } };
 
 module.exports = {
   config,
+  client: {
+    client_id: 'client',
+    client_secret: 'secret',
+    redirect_uris: ['https://client.example.com/cb'],
+  },
 };
diff --git a/test/dynamic_registration/dynamic_registration.test.js b/test/dynamic_registration/dynamic_registration.test.js
@@ -406,5 +406,13 @@ describe('registration features', () => {
           expect(spy.firstCall.args[0].clientId).to.equal(this.clientId);
         });
     });
+
+    it('cannot read non-dynamic clients', async function () {
+      const rat = new (this.provider.RegistrationAccessToken)({ clientId: 'client' });
+      const bearer = await rat.save();
+      return this.agent.get('/reg/client')
+        .auth(bearer, { type: 'bearer' })
+        .expect(this.failWith(403, 'invalid_request', 'client does not have permission to read its record'));
+    });
   });
 });
diff --git a/test/registration_management/registration_management.test.js b/test/registration_management/registration_management.test.js
@@ -204,7 +204,7 @@ describe('OAuth 2.0 Dynamic Client Registration Management Protocol', () => {
         .send(updateProperties(client.metadata(), {
           redirect_uris: ['https://client.example.com/foobar/cb'],
         }))
-        .expect(this.failWith(403, 'invalid_request', 'this client is not allowed to update its records'));
+        .expect(this.failWith(403, 'invalid_request', 'client does not have permission to update its record'));
     });
 
     describe('rotateRegistrationAccessToken', () => {
@@ -322,7 +322,7 @@ describe('OAuth 2.0 Dynamic Client Registration Management Protocol', () => {
       const bearer = await rat.save();
       return this.agent.del('/reg/client')
         .auth(bearer, { type: 'bearer' })
-        .expect(this.failWith(403, 'invalid_request', 'this client is not allowed to delete itself'));
+        .expect(this.failWith(403, 'invalid_request', 'client does not have permission to delete its record'));
     });
   });
 });