From b2e2f33cf11273b07fc40a319cce744a90c3c28a Mon Sep 17 00:00:00 2001 From: Oliver Eyton-Williams Date: Sat, 7 Nov 2020 09:05:25 +0100 Subject: [PATCH] feat(api): allow redirects with a returnTo param (#40161) --- api-server/server/boot/authentication.js | 35 +++++------- api-server/server/component-passport.js | 29 +++++----- api-server/server/middleware.json | 1 - .../server/middlewares/add-return-to.js | 37 ------------- api-server/server/utils/get-return-to.js | 23 ++++++++ api-server/server/utils/get-return-to.test.js | 54 +++++++++++++++++++ client/src/client-only-routes/ShowSettings.js | 4 +- .../client-only-routes/ShowSettings.test.js | 4 +- 8 files changed, 111 insertions(+), 76 deletions(-) delete mode 100644 api-server/server/middlewares/add-return-to.js create mode 100644 api-server/server/utils/get-return-to.js create mode 100644 api-server/server/utils/get-return-to.test.js diff --git a/api-server/server/boot/authentication.js b/api-server/server/boot/authentication.js index 19f01f10e6..636667f2ea 100644 --- a/api-server/server/boot/authentication.js +++ b/api-server/server/boot/authentication.js @@ -2,12 +2,15 @@ import passport from 'passport'; import dedent from 'dedent'; import { check } from 'express-validator/check'; import { isEmail } from 'validator'; +import jwt from 'jsonwebtoken'; import { homeLocation } from '../../../config/env'; +import { jwtSecret } from '../../../config/secrets'; + import { createPassportCallbackAuthenticator, - saveResponseAuthCookies, - loginRedirect + devSaveResponseAuthCookies, + devLoginRedirect } from '../component-passport'; import { ifUserRedirectTo, ifNoUserRedirectTo } from '../utils/middleware'; import { wrapHandledError } from '../utils/create-handled-error.js'; @@ -37,8 +40,8 @@ module.exports = function enableAuthentication(app) { app.enableAuth(); const ifUserRedirect = ifUserRedirectTo(); const ifNoUserRedirectHome = ifNoUserRedirectTo(homeLocation); - const saveAuthCookies = saveResponseAuthCookies(); - const loginSuccessRedirect = loginRedirect(); + const devSaveAuthCookies = devSaveResponseAuthCookies(); + const devLoginSuccessRedirect = devLoginRedirect(); const api = app.loopback.Router(); // Use a local mock strategy for signing in if we are in dev mode. @@ -48,26 +51,14 @@ module.exports = function enableAuthentication(app) { api.get( '/signin', passport.authenticate('devlogin'), - saveAuthCookies, - loginSuccessRedirect + devSaveAuthCookies, + devLoginSuccessRedirect ); } else { - api.get( - '/signin', - (req, res, next) => { - if (req && req.query && req.query.returnTo) { - req.query.returnTo = `${homeLocation}/${req.query.returnTo}`; - } - return next(); - }, - ifUserRedirect, - (req, res, next) => { - const state = req.query.returnTo - ? Buffer.from(req.query.returnTo).toString('base64') - : null; - return passport.authenticate('auth0-login', { state })(req, res, next); - } - ); + api.get('/signin', ifUserRedirect, (req, res, next) => { + const state = jwt.sign({ returnTo: req.query.returnTo }, jwtSecret); + return passport.authenticate('auth0-login', { state })(req, res, next); + }); api.get( '/auth/auth0/callback', diff --git a/api-server/server/component-passport.js b/api-server/server/component-passport.js index fa12146774..1b7ad1ffb7 100644 --- a/api-server/server/component-passport.js +++ b/api-server/server/component-passport.js @@ -11,6 +11,8 @@ import { getUserById } from './utils/user-stats'; import { homeLocation } from '../../config/env'; import passportProviders from './passport-providers'; import { setAccessTokenToResponse } from './utils/getSetAccessToken'; +import { jwtSecret } from '../../config/secrets'; +import getReturnTo from './utils/get-return-to'; const passportOptions = { emailOptional: true, @@ -63,7 +65,7 @@ export function setupPassport(app) { }); } -export const saveResponseAuthCookies = () => { +export const devSaveResponseAuthCookies = () => { return (req, res, next) => { const user = req.user; @@ -78,12 +80,11 @@ export const saveResponseAuthCookies = () => { }; }; -export const loginRedirect = () => { +export const devLoginRedirect = () => { return (req, res) => { const successRedirect = req => { - if (!!req && req.session && req.session.returnTo) { - delete req.session.returnTo; - return `${homeLocation}/learn`; + if (req && req.query && req.query.returnTo) { + return req.query.returnTo; } return `${homeLocation}/learn`; }; @@ -101,10 +102,14 @@ export const createPassportCallbackAuthenticator = (strategy, config) => ( res, next ) => { - const returnTo = - req && req.query && req.query.state - ? Buffer.from(req.query.state, 'base64').toString('utf-8') - : `${homeLocation}/learn`; + const state = req && req.query && req.query.state; + const { returnTo } = getReturnTo(state, jwtSecret); + + // TODO: getReturnTo returns a {returnTo, success} object, so we can use + // 'success' to show a flash message, but currently it immediately gets + // overwritten by a second message. We should either change the message if + // !success or allow multiple messages to appear at once. + return passport.authenticate( strategy, { session: false }, @@ -116,7 +121,6 @@ export const createPassportCallbackAuthenticator = (strategy, config) => ( if (!user || !userInfo) { return res.redirect('/signin'); } - const redirect = `${returnTo}`; const { accessToken } = userInfo; const { provider } = config; @@ -140,9 +144,10 @@ we recommend using your email address: ${user.email} to sign in instead. setAccessTokenToResponse({ accessToken }, req, res); req.login(user); } - // TODO: enable 'returnTo' for sign-up + // TODO: handle returning to /email-sign-up without relying on + // homeLocation if (user.acceptedPrivacyTerms) { - return res.redirectWithFlash(redirect); + return res.redirectWithFlash(returnTo); } else { return res.redirectWithFlash(`${homeLocation}/email-sign-up`); } diff --git a/api-server/server/middleware.json b/api-server/server/middleware.json index 445290954d..3588498b11 100644 --- a/api-server/server/middleware.json +++ b/api-server/server/middleware.json @@ -29,7 +29,6 @@ "auth:before": { "express-flash": {}, "./middlewares/express-extensions": {}, - "./middlewares/add-return-to": {}, "./middlewares/cookie-parser": {}, "./middlewares/request-authorization": {} }, diff --git a/api-server/server/middlewares/add-return-to.js b/api-server/server/middlewares/add-return-to.js deleted file mode 100644 index 47d7bbfc03..0000000000 --- a/api-server/server/middlewares/add-return-to.js +++ /dev/null @@ -1,37 +0,0 @@ -const pathsOfNoReturn = [ - 'link', - 'auth', - 'login', - 'logout', - 'signin', - 'signup', - 'fonts', - 'favicon', - 'js', - 'css' -]; - -const pathsAllowedList = ['challenges', 'map', 'commit']; - -const pathsOfNoReturnRegex = new RegExp(pathsOfNoReturn.join('|'), 'i'); -const pathsAllowedRegex = new RegExp(pathsAllowedList.join('|'), 'i'); - -export default function addReturnToUrl() { - return function(req, res, next) { - // Remember original destination before login. - var path = req.path.split('/')[1]; - - if ( - req.method !== 'GET' || - pathsOfNoReturnRegex.test(path) || - !pathsAllowedRegex.test(path) || - /hot/i.test(req.path) - ) { - return next(); - } - req.session.returnTo = req.originalUrl.includes('/map') - ? '/' - : req.originalUrl; - return next(); - }; -} diff --git a/api-server/server/utils/get-return-to.js b/api-server/server/utils/get-return-to.js new file mode 100644 index 0000000000..ba90ea4610 --- /dev/null +++ b/api-server/server/utils/get-return-to.js @@ -0,0 +1,23 @@ +const jwt = require('jsonwebtoken'); +const { allowedOrigins } = require('../../../config/cors-settings'); +const { homeLocation } = require('../../../config/env.json'); + +function getReturnTo(encryptedReturnTo, secret) { + let returnTo; + let success = false; + try { + returnTo = jwt.verify(encryptedReturnTo, secret).returnTo; + // we add the '/' to prevent returns to + // www.freecodecamp.org.somewhere.else.com + if (!allowedOrigins.some(origin => returnTo.startsWith(origin + '/'))) { + throw Error(); + } + success = true; + } catch { + returnTo = `${homeLocation}/learn`; + } + + return { returnTo, success }; +} + +module.exports = getReturnTo; diff --git a/api-server/server/utils/get-return-to.test.js b/api-server/server/utils/get-return-to.test.js new file mode 100644 index 0000000000..c535e687f0 --- /dev/null +++ b/api-server/server/utils/get-return-to.test.js @@ -0,0 +1,54 @@ +/* global describe expect it */ + +const { homeLocation } = require('../../../config/env.json'); +const jwt = require('jsonwebtoken'); + +const getReturnTo = require('./get-return-to'); + +const validJWTSecret = 'this is a super secret string'; +const invalidJWTSecret = 'This is not correct secret'; +const validReturnTo = 'https://www.freecodecamp.org/settings'; +const invalidReturnTo = 'https://www.freecodecamp.org.fake/settings'; +const defaultReturnTo = `${homeLocation}/learn`; + +describe('get-return-to', () => { + describe('getReturnTo', () => { + it('should extract returnTo from a jwt', () => { + expect.assertions(1); + + const encryptedReturnTo = jwt.sign( + { returnTo: validReturnTo }, + validJWTSecret + ); + expect(getReturnTo(encryptedReturnTo, validJWTSecret)).toStrictEqual({ + returnTo: validReturnTo, + success: true + }); + }); + + it('should return a default url if the secrets do not match', () => { + expect.assertions(1); + + const encryptedReturnTo = jwt.sign( + { returnTo: validReturnTo }, + invalidJWTSecret + ); + expect(getReturnTo(encryptedReturnTo, validJWTSecret)).toStrictEqual({ + returnTo: defaultReturnTo, + success: false + }); + }); + + it('should return a default url for unknown origins', () => { + expect.assertions(1); + const encryptedReturnTo = jwt.sign( + { returnTo: invalidReturnTo }, + validJWTSecret + ); + expect(getReturnTo(encryptedReturnTo, validJWTSecret)).toStrictEqual({ + returnTo: defaultReturnTo, + success: false + }); + }); + }); +}); diff --git a/client/src/client-only-routes/ShowSettings.js b/client/src/client-only-routes/ShowSettings.js index e6ade34b0f..dc23eb839b 100644 --- a/client/src/client-only-routes/ShowSettings.js +++ b/client/src/client-only-routes/ShowSettings.js @@ -5,7 +5,7 @@ import { createSelector } from 'reselect'; import { Grid, Button } from '@freecodecamp/react-bootstrap'; import Helmet from 'react-helmet'; -import { apiLocation } from '../../config/env.json'; +import { apiLocation, homeLocation } from '../../config/env.json'; import { signInLoadingSelector, userSelector, @@ -167,7 +167,7 @@ export function ShowSettings(props) { } if (!isSignedIn) { - navigate(`${apiLocation}/signin?returnTo=settings`); + navigate(`${apiLocation}/signin?returnTo=${homeLocation}/settings`); return ; } diff --git a/client/src/client-only-routes/ShowSettings.test.js b/client/src/client-only-routes/ShowSettings.test.js index 91051a0c80..3e7cd4ec96 100644 --- a/client/src/client-only-routes/ShowSettings.test.js +++ b/client/src/client-only-routes/ShowSettings.test.js @@ -1,7 +1,7 @@ /* global jest, expect */ import React from 'react'; import ShallowRenderer from 'react-test-renderer/shallow'; -import { apiLocation } from '../../config/env.json'; +import { apiLocation, homeLocation } from '../../config/env.json'; import { ShowSettings } from './ShowSettings'; @@ -23,7 +23,7 @@ describe('', () => { shallow.render(); expect(navigate).toHaveBeenCalledTimes(1); expect(navigate).toHaveBeenCalledWith( - `${apiLocation}/signin?returnTo=settings` + `${apiLocation}/signin?returnTo=${homeLocation}/settings` ); const result = shallow.getRenderOutput(); // Renders Loader rather than ShowSettings