From 77c46dba2cbb09d51fea8c7e86ed104222ec27a1 Mon Sep 17 00:00:00 2001 From: Mrugesh Mohapatra <1884376+raisedadead@users.noreply.github.com> Date: Mon, 21 Dec 2020 22:23:06 +0530 Subject: [PATCH] fix(i18n): redirect based on Referer header (#40512) Co-authored-by: Oliver Eyton-Williams --- api-server/server/boot/authentication.js | 4 +- api-server/server/boot/challenge.js | 25 ++++-- .../server/boot_tests/challenge.test.js | 15 +++- api-server/server/component-passport.js | 51 ++++++----- api-server/server/utils/get-return-to.js | 87 ++++++++++++++++--- api-server/server/utils/get-return-to.test.js | 28 +++--- client/src/client-only-routes/ShowSettings.js | 4 +- .../client-only-routes/ShowSettings.test.js | 6 +- .../src/components/Header/components/Login.js | 4 +- config/env.js | 16 ++-- 10 files changed, 162 insertions(+), 78 deletions(-) diff --git a/api-server/server/boot/authentication.js b/api-server/server/boot/authentication.js index 4f0ae01c7b..969fc97df2 100644 --- a/api-server/server/boot/authentication.js +++ b/api-server/server/boot/authentication.js @@ -16,6 +16,7 @@ import { ifUserRedirectTo, ifNoUserRedirectTo } from '../utils/middleware'; import { wrapHandledError } from '../utils/create-handled-error.js'; import { removeCookies } from '../utils/getSetAccessToken'; import { decodeEmail } from '../../common/utils'; +import { getParamsFromReq } from '../utils/get-return-to'; const isSignUpDisabled = !!process.env.DISABLE_SIGNUP; if (isSignUpDisabled) { @@ -56,7 +57,8 @@ module.exports = function enableAuthentication(app) { ); } else { api.get('/signin', ifUserRedirect, (req, res, next) => { - const state = jwt.sign({ returnTo: req.query.returnTo }, jwtSecret); + const { returnTo, origin, pathPrefix } = getParamsFromReq(req); + const state = jwt.sign({ returnTo, origin, pathPrefix }, jwtSecret); return passport.authenticate('auth0-login', { state })(req, res, next); }); diff --git a/api-server/server/boot/challenge.js b/api-server/server/boot/challenge.js index f6a9da6600..ce4dc7ac00 100644 --- a/api-server/server/boot/challenge.js +++ b/api-server/server/boot/challenge.js @@ -12,12 +12,15 @@ import { ObjectID } from 'mongodb'; import isNumeric from 'validator/lib/isNumeric'; import isURL from 'validator/lib/isURL'; -import { homeLocation } from '../../../config/env'; - import { ifNoUserSend } from '../utils/middleware'; import { dasherize } from '../../../utils/slugs'; import { fixCompletedChallengeItem } from '../../common/utils'; import { getChallenges } from '../utils/get-curriculum'; +import { + getParamsFromReq, + getRedirectBase, + normalizeParams +} from '../utils/get-return-to'; const log = debug('fcc:boot:challenges'); @@ -29,7 +32,9 @@ export default async function bootChallenge(app, done) { await getChallenges() ); const redirectToCurrentChallenge = createRedirectToCurrentChallenge( - challengeUrlResolver + challengeUrlResolver, + normalizeParams, + getParamsFromReq ); api.post( @@ -59,7 +64,6 @@ export default async function bootChallenge(app, done) { app.use(router); done(); } -const learnURL = `${homeLocation}/learn`; const jsProjects = [ 'aaa48de84e1ecc7c742e1124', @@ -327,15 +331,22 @@ function backendChallengeCompleted(req, res, next) { .subscribe(() => {}, next); } +// TODO: extend tests to cover www.freecodecamp.org/language and +// chinese.freecodecamp.org export function createRedirectToCurrentChallenge( challengeUrlResolver, - { _homeLocation = homeLocation, _learnUrl = learnURL } = {} + normalizeParams, + getParamsFromReq ) { return async function redirectToCurrentChallenge(req, res, next) { const { user } = req; + const { origin, pathPrefix } = normalizeParams(getParamsFromReq(req)); + + const redirectBase = getRedirectBase(origin, pathPrefix); if (!user) { - return res.redirect(_learnUrl); + return res.redirect(redirectBase + '/learn'); } + const challengeId = user && user.currentChallengeId; const challengeUrl = await challengeUrlResolver(challengeId).catch(next); if (challengeUrl === '/learn') { @@ -346,6 +357,6 @@ export function createRedirectToCurrentChallenge( db may not be properly seeded. `); } - return res.redirect(`${_homeLocation}${challengeUrl}`); + return res.redirect(`${redirectBase}${challengeUrl}`); }; } diff --git a/api-server/server/boot_tests/challenge.test.js b/api-server/server/boot_tests/challenge.test.js index 285930140d..e1a4f9b069 100644 --- a/api-server/server/boot_tests/challenge.test.js +++ b/api-server/server/boot_tests/challenge.test.js @@ -330,11 +330,18 @@ describe('boot/challenge', () => { describe('redirectToCurrentChallenge', () => { const mockHomeLocation = 'https://www.example.com'; const mockLearnUrl = `${mockHomeLocation}/learn`; + const mockgetParamsFromReq = () => ({ + returnTo: mockLearnUrl, + origin: mockHomeLocation, + pathPrefix: '' + }); + const mockNormalizeParams = params => params; it('redirects to the learn base url for non-users', async done => { const redirectToCurrentChallenge = createRedirectToCurrentChallenge( () => {}, - { _homeLocation: mockHomeLocation, _learnUrl: mockLearnUrl } + mockNormalizeParams, + mockgetParamsFromReq ); const req = mockReq(); const res = mockRes(); @@ -356,7 +363,8 @@ describe('boot/challenge', () => { const expectedUrl = `${mockHomeLocation}${requestedChallengeUrl}`; const redirectToCurrentChallenge = createRedirectToCurrentChallenge( challengeUrlResolver, - { _homeLocation: mockHomeLocation, _learnUrl: mockLearnUrl } + mockNormalizeParams, + mockgetParamsFromReq ); const req = mockReq({ user: mockUser @@ -379,7 +387,8 @@ describe('boot/challenge', () => { ); const redirectToCurrentChallenge = createRedirectToCurrentChallenge( challengeUrlResolver, - { _homeLocation: mockHomeLocation, _learnUrl: mockLearnUrl } + mockNormalizeParams, + mockgetParamsFromReq ); const req = mockReq({ user: { ...mockUser, currentChallengeId: '' } diff --git a/api-server/server/component-passport.js b/api-server/server/component-passport.js index 1b7ad1ffb7..7407e6266b 100644 --- a/api-server/server/component-passport.js +++ b/api-server/server/component-passport.js @@ -4,15 +4,18 @@ import { // prettier ignore PassportConfigurator } from '@freecodecamp/loopback-component-passport'; -import url from 'url'; import dedent from 'dedent'; 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'; +import { + getReturnTo, + getRedirectBase, + getParamsFromReq, + isRootPath +} from './utils/get-return-to'; const passportOptions = { emailOptional: true, @@ -82,18 +85,12 @@ export const devSaveResponseAuthCookies = () => { export const devLoginRedirect = () => { return (req, res) => { - const successRedirect = req => { - if (req && req.query && req.query.returnTo) { - return req.query.returnTo; - } - return `${homeLocation}/learn`; - }; - - let redirect = url.parse(successRedirect(req), true); - delete redirect.search; - - redirect = url.format(redirect); - return res.redirect(redirect); + // this mirrors the production approach, but without any validation + let { returnTo, origin, pathPrefix } = getParamsFromReq(req); + returnTo += isRootPath(getRedirectBase(origin, pathPrefix), returnTo) + ? '/learn' + : ''; + return res.redirect(returnTo); }; }; @@ -102,14 +99,6 @@ export const createPassportCallbackAuthenticator = (strategy, config) => ( res, next ) => { - 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 }, @@ -144,12 +133,22 @@ we recommend using your email address: ${user.email} to sign in instead. setAccessTokenToResponse({ accessToken }, req, res); req.login(user); } - // TODO: handle returning to /email-sign-up without relying on - // homeLocation + + const state = req && req.query && req.query.state; + // returnTo, origin and pathPrefix are audited by getReturnTo + let { returnTo, origin, pathPrefix } = getReturnTo(state, jwtSecret); + const redirectBase = getRedirectBase(origin, pathPrefix); + + // TODO: getReturnTo could return a success flag to show a flash message, + // but currently it immediately gets overwritten by a second message. We + // should either change the message if the flag is present or allow + // multiple messages to appear at once. + if (user.acceptedPrivacyTerms) { + returnTo += isRootPath(redirectBase, returnTo) ? '/learn' : ''; return res.redirectWithFlash(returnTo); } else { - return res.redirectWithFlash(`${homeLocation}/email-sign-up`); + return res.redirectWithFlash(`${redirectBase}/email-sign-up`); } } )(req, res, next); diff --git a/api-server/server/utils/get-return-to.js b/api-server/server/utils/get-return-to.js index ba90ea4610..89be3792fc 100644 --- a/api-server/server/utils/get-return-to.js +++ b/api-server/server/utils/get-return-to.js @@ -1,23 +1,84 @@ const jwt = require('jsonwebtoken'); +const { availableLangs } = require('../../../client/i18n/allLangs'); const { allowedOrigins } = require('../../../config/cors-settings'); +// homeLocation is being used as a fallback, here. If the one provided by the +// client is invalid we default to this. const { homeLocation } = require('../../../config/env.json'); function getReturnTo(encryptedReturnTo, secret) { - let returnTo; - let success = false; + let params; 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`; + params = jwt.verify(encryptedReturnTo, secret); + } catch (e) { + // TODO: report to Sentry? Probably not. Remove entirely? + console.log(e); + // something went wrong, use default params + params = { + returnTo: `${homeLocation}/learn`, + origin: homeLocation, + pathPrefix: '' + }; } - return { returnTo, success }; + return normalizeParams(params); } -module.exports = getReturnTo; +// TODO: tests! +function normalizeParams({ returnTo, origin, pathPrefix }) { + // coerce to strings, just in case something weird and nefarious is happening + returnTo = '' + returnTo; + origin = '' + origin; + pathPrefix = '' + pathPrefix; + // we add the '/' to prevent returns to + // www.freecodecamp.org.somewhere.else.com + if ( + !returnTo || + !allowedOrigins.some(allowed => returnTo.startsWith(allowed + '/')) + ) { + returnTo = `${homeLocation}/learn`; + } + // this can be strict equality. + if (!origin || !allowedOrigins.includes(origin)) { + origin = homeLocation; + } + // default to '' if the locale isn't recognised + pathPrefix = availableLangs.client.includes(pathPrefix) ? pathPrefix : ''; + return { returnTo, origin, pathPrefix }; +} + +// TODO: use this to redirect to current challenge +// TODO: tests! + +// TODO: ensure origin and pathPrefix validation happens first +// (it needs a dedicated function that can be called from here and getReturnTo) +function getRedirectBase(origin, pathPrefix) { + const redirectPathSegment = pathPrefix ? `/${pathPrefix}` : ''; + return `${origin}${redirectPathSegment}`; +} + +// TODO: this might be cleaner if we just use a URL for returnTo (call it +// returnURL for clarity) rather than pulling out origin and returning it +// separately +function getParamsFromReq(req) { + const url = req.header('Referer'); + // since we do not always redirect the user back to the page they were on + // we need client locale and origin to construct the redirect url. + const returnUrl = new URL(url); + const origin = returnUrl.origin; + // if this is not one of the client languages, validation will convert + // this to '' before it is used. + const pathPrefix = returnUrl.pathname.split('/')[0]; + return { returnTo: returnUrl.href, origin, pathPrefix }; +} + +function isRootPath(redirectBase, returnUrl) { + const base = new URL(redirectBase); + const url = new URL(returnUrl); + return base.pathname === url.pathname; +} + +module.exports.getReturnTo = getReturnTo; +module.exports.getRedirectBase = getRedirectBase; +module.exports.normalizeParams = normalizeParams; +module.exports.getParamsFromReq = getParamsFromReq; +module.exports.isRootPath = isRootPath; diff --git a/api-server/server/utils/get-return-to.test.js b/api-server/server/utils/get-return-to.test.js index c535e687f0..237a91fb37 100644 --- a/api-server/server/utils/get-return-to.test.js +++ b/api-server/server/utils/get-return-to.test.js @@ -3,13 +3,21 @@ const { homeLocation } = require('../../../config/env.json'); const jwt = require('jsonwebtoken'); -const getReturnTo = require('./get-return-to'); +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`; +const defaultOrigin = homeLocation; +const defaultPrefix = ''; + +const defaultObject = { + returnTo: defaultReturnTo, + origin: defaultOrigin, + pathPrefix: defaultPrefix +}; describe('get-return-to', () => { describe('getReturnTo', () => { @@ -21,8 +29,8 @@ describe('get-return-to', () => { validJWTSecret ); expect(getReturnTo(encryptedReturnTo, validJWTSecret)).toStrictEqual({ - returnTo: validReturnTo, - success: true + ...defaultObject, + returnTo: validReturnTo }); }); @@ -33,10 +41,9 @@ describe('get-return-to', () => { { returnTo: validReturnTo }, invalidJWTSecret ); - expect(getReturnTo(encryptedReturnTo, validJWTSecret)).toStrictEqual({ - returnTo: defaultReturnTo, - success: false - }); + expect(getReturnTo(encryptedReturnTo, validJWTSecret)).toStrictEqual( + defaultObject + ); }); it('should return a default url for unknown origins', () => { @@ -45,10 +52,9 @@ describe('get-return-to', () => { { returnTo: invalidReturnTo }, validJWTSecret ); - expect(getReturnTo(encryptedReturnTo, validJWTSecret)).toStrictEqual({ - returnTo: defaultReturnTo, - success: false - }); + expect(getReturnTo(encryptedReturnTo, validJWTSecret)).toStrictEqual( + defaultObject + ); }); }); }); diff --git a/client/src/client-only-routes/ShowSettings.js b/client/src/client-only-routes/ShowSettings.js index f4240c7ce5..8d0018113e 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, homeLocation } from '../../config/env.json'; +import { apiLocation } from '../../config/env.json'; import { signInLoadingSelector, userSelector, @@ -169,7 +169,7 @@ export function ShowSettings(props) { } if (!isSignedIn) { - navigate(`${apiLocation}/signin?returnTo=${homeLocation}/settings`); + navigate(`${apiLocation}/signin`); return ; } diff --git a/client/src/client-only-routes/ShowSettings.test.js b/client/src/client-only-routes/ShowSettings.test.js index 934193c1de..7c5825f628 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, homeLocation } from '../../config/env.json'; +import { apiLocation } from '../../config/env.json'; import { ShowSettings } from './ShowSettings'; @@ -22,9 +22,7 @@ describe('', () => { const shallow = new ShallowRenderer(); shallow.render(); expect(navigate).toHaveBeenCalledTimes(1); - expect(navigate).toHaveBeenCalledWith( - `${apiLocation}/signin?returnTo=${homeLocation}/settings` - ); + expect(navigate).toHaveBeenCalledWith(`${apiLocation}/signin`); const result = shallow.getRenderOutput(); // Renders Loader rather than ShowSettings expect(result.type.displayName).toBe('Loader'); diff --git a/client/src/components/Header/components/Login.js b/client/src/components/Header/components/Login.js index cbf2d08593..af3df1ef04 100644 --- a/client/src/components/Header/components/Login.js +++ b/client/src/components/Header/components/Login.js @@ -27,9 +27,7 @@ function Login(props) { children, isSignedIn } = props; - const href = isSignedIn - ? `${homeLocation}/learn` - : `${apiLocation}/signin?returnTo=${homeLocation}/learn`; + const href = isSignedIn ? `${homeLocation}/learn` : `${apiLocation}/signin`; return (