fix(oauth): validate that redirect param exists.

Fixes #1786
mozilla · Oct 28, 2014 · a8c63fd · a8c63fd
1 parent 8b31196
commit a8c63fd
Show file tree

Hide file tree

Showing 5 changed files with 86 additions and 10 deletions.
diff --git a/app/scripts/lib/constants.js b/app/scripts/lib/constants.js
@@ -22,6 +22,8 @@ define([], function () {
     CODE_LENGTH: 32,
     UID_LENGTH: 32,
 
+    OAUTH_CODE_LENGTH: 64,
+
     PASSWORD_MIN_LENGTH: 8,
 
     PROFILE_IMAGE_DISPLAY_SIZE: 240,

diff --git a/app/scripts/lib/validate.js b/app/scripts/lib/validate.js
@@ -45,7 +45,7 @@ define([
     },
 
     /**
-     * Check if a verification code is valid
+     * Check if an email verification code is valid
      */
     isCodeValid: function (code) {
       if (typeof code !== 'string') {
@@ -57,6 +57,19 @@ define([
              HEX_STRING.test(code);
     },
 
+    /**
+     * Check if an OAuth code is valid
+     */
+    isOAuthCodeValid: function (code) {
+      if (typeof code !== 'string') {
+        return false;
+      }
+
+      // codes are fixed length hex strings.
+      return code.length === Constants.OAUTH_CODE_LENGTH &&
+        HEX_STRING.test(code);
+    },
+
     /**
      * Check if a verification token is valid
      */

diff --git a/app/scripts/views/mixins/service-mixin.js b/app/scripts/views/mixins/service-mixin.js
@@ -18,9 +18,10 @@ define([
   'lib/config-loader',
   'lib/session',
   'lib/service-name',
-  'lib/channels'
+  'lib/channels',
+  'lib/validate'
 ], function (p, BaseView, progressIndicator, Url, OAuthClient, Assertion,
-    OAuthErrors, ConfigLoader, Session, ServiceName, Channels) {
+    OAuthErrors, ConfigLoader, Session, ServiceName, Channels, Validate) {
   /* jshint camelcase: false */
 
   var EXPECT_CHANNEL_RESPONSE_TIMEOUT = 5000;
@@ -85,10 +86,20 @@ define([
    */
   function _formatOAuthResult(result) {
     // get code and state from redirect params
-    var redirectParams = result.redirect.split('?')[1];
-    result.state = Url.searchParam('state', redirectParams);
-    result.code = Url.searchParam('code', redirectParams);
-    return p(result);
+    if (result && result.redirect) {
+      var redirectParams = result.redirect.split('?')[1];
+
+      result.state = Url.searchParam('state', redirectParams);
+      result.code = Url.searchParam('code', redirectParams);
+
+      if (! Validate.isOAuthCodeValid(result.code)) {
+        return p.reject(OAuthErrors.toError('UNEXPECTED_ERROR'));
+      }
+
+      return p(result);
+    } else {
+      return p.reject(OAuthErrors.toError('UNEXPECTED_ERROR'));
+    }
   }
 
   return {

diff --git a/app/tests/spec/lib/validate.js b/app/tests/spec/lib/validate.js
@@ -71,6 +71,28 @@ function (chai, _, Validate, Constants, TestHelpers) {
       });
     });
 
+    describe('isOAuthCodeValid', function () {
+      it('returns false for non-hex value', function () {
+        assert.isFalse(Validate.isOAuthCodeValid('this is a normal string'));
+      });
+
+      it('returns false for an empty string', function () {
+        assert.isFalse(Validate.isOAuthCodeValid(''));
+      });
+
+      it('returns false for one too few characters', function () {
+        assert.isFalse(Validate.isOAuthCodeValid(createRandomHexString(Constants.OAUTH_CODE_LENGTH - 1)));
+      });
+
+      it('returns false for one too many characters', function () {
+        assert.isFalse(Validate.isOAuthCodeValid(createRandomHexString(Constants.OAUTH_CODE_LENGTH + 1)));
+      });
+
+      it('returns true for just the right number', function () {
+        assert.isTrue(Validate.isOAuthCodeValid(createRandomHexString(Constants.OAUTH_CODE_LENGTH)));
+      });
+    });
+
     describe('isUidValid', function () {
       it('returns false for non-hex value', function () {
         assert.isFalse(Validate.isUidValid('this is a normal string'));

diff --git a/app/tests/spec/views/mixins/service-mixin.js b/app/tests/spec/views/mixins/service-mixin.js
@@ -19,16 +19,17 @@ define([
   'stache!templates/test_template',
   'models/reliers/relier',
   'models/reliers/fx-desktop',
-  'models/reliers/oauth'
+  'models/reliers/oauth',
+  'lib/oauth-errors'
 ], function (Chai, Backbone, _, p, MockOAuthServers, WindowMock, ChannelMock,
         TestHelpers, ServiceMixin, BaseView, Session, TestTemplate,
-        Relier, FxDesktopRelier, OAuthRelier) {
+        Relier, FxDesktopRelier, OAuthRelier, OAuthErrors) {
   var assert = Chai.assert;
 
 
   var CLIENT_ID = 'dcdb5ae7add825d2';
   var STATE = '123';
-  var CODE = 'code1';
+  var CODE = 'd74e7d9b2c5e8be20fbff978b7804f648369b3f1f87ba7426cbe96bc2c5e360f';
   var DEFAULT_REDIRECT_STRING = TestHelpers.toSearchString({
     code: CODE,
     state: STATE
@@ -89,6 +90,33 @@ define([
     });
 
     describe('_formatOAuthResult', function () {
+      it('throws on null', function () {
+        relier.set('state', STATE);
+        return view._formatOAuthResult().then(assert.fail, function (err) {
+          assert.isTrue(OAuthErrors.is(err, 'UNEXPECTED_ERROR'));
+        });
+      });
+
+      it('throws on an empty object', function () {
+        relier.set('state', STATE);
+        return view._formatOAuthResult({}).then(assert.fail, function (err) {
+          assert.isTrue(OAuthErrors.is(err, 'UNEXPECTED_ERROR'));
+        });
+      });
+
+      it('catches invalid codes', function () {
+        var redirectBadCode = TestHelpers.toSearchString({
+          code: 'a',
+          state: 'state1'
+        });
+        relier.set('state', STATE);
+        return view._formatOAuthResult({
+          redirect: redirectBadCode
+        }).then(assert.fail, function (err) {
+          assert.isTrue(OAuthErrors.is(err, 'UNEXPECTED_ERROR'));
+        });
+      });
+
       it('formats the redirect params', function () {
         relier.set('state', STATE);
         return view._formatOAuthResult({