Merge pull request #216 from anvilresearch/vsimonian-fix-unverified-r…

…edirects

Always verify redirect_uri before issuing redirect

oidc/validateAuthorizationParams.js

@@ -38,24 +38,6 @@ var responseModes = [
 function validateAuthorizationParams (req, res, next) {
  var params = req.connectParams
 
- // missing redirect uri
- if (!params.redirect_uri) {
- return next(new AuthorizationError({
- error: 'invalid_request',
- error_description: 'Missing redirect uri',
- statusCode: 400
- }))
- }
-
- // invalid redirect uri
- // if (!params.redirect_uri) { // HOW SHOULD WE VALIDATE THIS?
- // return next(new AuthorizationError({
- // error: 'invalid_request',
- // error_description: 'Invalid redirect uri',
- // statusCode: 400
- // }))
- // }
-
  // missing response type
  if (!params.response_type) {
  return next(new AuthorizationError({
@@ -87,15 +69,6 @@ function validateAuthorizationParams (req, res, next) {
  }))
  }
 
- // missing client id
- if (!params.client_id) {
- return next(new AuthorizationError({
- error: 'unauthorized_client',
- error_description: 'Missing client id',
- statusCode: 403
- }))
- }
-
  // missing scope
  if (!params.scope) {
  return next(new AuthorizationError({

oidc/verifyClient.js

@@ -17,6 +17,24 @@ var AuthorizationError = require('../errors/AuthorizationError')
 function verifyClient (req, res, next) {
  var params = req.connectParams
 
+ // missing redirect uri
+ if (!params.redirect_uri) {
+ return next(new AuthorizationError({
+ error: 'invalid_request',
+ error_description: 'Missing redirect uri',
+ statusCode: 400
+ }))
+ }
+
+ // missing client id
+ if (!params.client_id) {
+ return next(new AuthorizationError({
+ error: 'unauthorized_client',
+ error_description: 'Missing client id',
+ statusCode: 403
+ }))
+ }
+
  Client.get(params.client_id, {
  private: true
  }, function (err, client) {

routes/authorize.js

@@ -11,8 +11,8 @@ var oidc = require('../oidc')
 module.exports = function (server) {
  var handler = [
  oidc.selectConnectParams,
- oidc.validateAuthorizationParams,
  oidc.verifyClient,
+ oidc.validateAuthorizationParams,
  oidc.requireSignin,
  oidc.determineUserScope,
  oidc.promptToAuthorize,

routes/connect.js

@@ -19,8 +19,8 @@ module.exports = function (server) {
 
  server.get('/connect/:provider',
  oidc.selectConnectParams,
- oidc.validateAuthorizationParams,
  oidc.verifyClient,
+ oidc.validateAuthorizationParams,
  oidc.stashParams,
  oidc.determineProvider,
  function (req, res, next) {

routes/signin.js

@@ -27,8 +27,8 @@ module.exports = function (server) {
 
  server.get('/signin',
  oidc.selectConnectParams,
- oidc.validateAuthorizationParams,
  oidc.verifyClient,
+ oidc.validateAuthorizationParams,
  function (req, res, next) {
  res.render('signin', {
  params: qs.stringify(req.query),
@@ -45,8 +45,8 @@ module.exports = function (server) {
 
  var handler = [
  oidc.selectConnectParams,
- oidc.validateAuthorizationParams,
  oidc.verifyClient,
+ oidc.validateAuthorizationParams,
  oidc.determineProvider,
  oidc.enforceReferrer('/signin'),
  function (req, res, next) {

routes/signup.js

@@ -23,8 +23,8 @@ module.exports = function (server) {
 
  var getSignupHandler = [
  oidc.selectConnectParams,
- oidc.validateAuthorizationParams,
  oidc.verifyClient,
+ oidc.validateAuthorizationParams,
  function (req, res, next) {
  res.render('signup', {
  params: qs.stringify(req.query),
@@ -70,8 +70,8 @@ module.exports = function (server) {
 
  var postSignupHandler = [
  oidc.selectConnectParams,
- oidc.validateAuthorizationParams,
  oidc.verifyClient,
+ oidc.validateAuthorizationParams,
  usePasswordProvider,
  oidc.enforceReferrer('/signup'),
  createUser,

test/unit/oidc/validateAuthorizationParams.coffee

@@ -20,40 +20,6 @@ describe 'Validate Authorization Parameters', ->
 
  describe 'all requests', ->
 
- describe 'with missing redirect_uri', ->
-
- before (done) ->
- params = {}
- validateAuthorizationParams req(params), res, (error) ->
- err = error
- done()
-
- it 'should provide an AuthorizationError', ->
- err.name.should.equal 'AuthorizationError'
-
- it 'should provide an error code', ->
- err.error.should.equal 'invalid_request'
-
- it 'should provide an error description', ->
- err.error_description.should.equal 'Missing redirect uri'
-
- it 'should provide a status code', ->
- err.statusCode.should.equal 400
-
-
-
-
- describe 'with invalid redirect_uri', ->
-
- it 'should provide an AuthorizationError'
- it 'should provide an error code'
- it 'should provide an error description'
- it 'should provide a redirect_uri'
- it 'should provide a status code'
-
-
-
-
  describe 'with missing response_type', ->
 
  before (done) ->
@@ -170,32 +136,6 @@ describe 'Validate Authorization Parameters', ->
 
 
 
- describe 'with missing client_id', ->
-
- before (done) ->
- params =
- redirect_uri: 'https://redirect.uri'
- response_type: 'code'
-
- validateAuthorizationParams req(params), res, (error) ->
- err = error
- done()
-
- it 'should provide an AuthorizationError', ->
- err.name.should.equal 'AuthorizationError'
-
- it 'should provide an error code', ->
- err.error.should.equal 'unauthorized_client'
-
- it 'should provide an error description', ->
- err.error_description.should.equal 'Missing client id'
-
- it 'should provide a status code', ->
- err.statusCode.should.equal 403
-
-
-
-
  describe 'with missing scope', ->
 
  before (done) ->

test/unit/oidc/verifyClient.coffee

@@ -23,12 +23,63 @@ describe 'Verify Client', ->
 
  {req,res,next,err} = {}
 
+ describe 'with missing redirect_uri', ->
+
+ before (done) ->
+ req = { connectParams: {} }
+ verifyClient req, res, (error) ->
+ err = error
+ done()
+
+ it 'should provide an AuthorizationError', ->
+ err.name.should.equal 'AuthorizationError'
+
+ it 'should provide an error code', ->
+ err.error.should.equal 'invalid_request'
+
+ it 'should provide an error description', ->
+ err.error_description.should.equal 'Missing redirect uri'
+
+ it 'should provide a status code', ->
+ err.statusCode.should.equal 400
+
+
+
+
+ describe 'with missing client_id', ->
+
+ before (done) ->
+ req =
+ connectParams:
+ redirect_uri: 'https://redirect.uri'
+
+ verifyClient req, res, (error) ->
+ err = error
+ done()
+
+ it 'should provide an AuthorizationError', ->
+ err.name.should.equal 'AuthorizationError'
+
+ it 'should provide an error code', ->
+ err.error.should.equal 'unauthorized_client'
+
+ it 'should provide an error description', ->
+ err.error_description.should.equal 'Missing client id'
+
+ it 'should provide a status code', ->
+ err.statusCode.should.equal 403
+
+
+
 
  describe 'with unknown client id', ->
 
  before (done) ->
  sinon.stub(Client, 'get').callsArgWith(2, null, null)
- req = { connectParams: {} }
+ req =
+ connectParams:
+ redirect_uri: 'https://redirect.uri'
+ client_id: 'unknown'
  res = {}
  next = sinon.spy()
 
@@ -59,7 +110,10 @@ describe 'Verify Client', ->
  before (done) ->
  client = { redirect_uris: [] }
  sinon.stub(Client, 'get').callsArgWith(2, null, client)
- req = { connectParams: { redirect_uri: 'https://mismatching.uri/cb' } }
+ req =
+ connectParams:
+ redirect_uri: 'https://mismatching.uri/cb'
+ client_id: 'id'
  res = {}
  next = sinon.spy()