fix(api): only use homeLocation as a fallback (#40517)

api-server/server/utils/commit-goals.json

@@ -1,16 +0,0 @@
-{
-  "frontEndCert": "Front End Development Certification",
-  "backEndCert": "Back End Development Certification",
-  "fullStackCert": "Full Stack Development Certification",
-  "respWebDesignCert": "Responsive Web Design Certification",
-  "frontEndLibsCert": "Front End Libraries Certification",
-  "jsAlgoDataStructCert": "JavaScript Algorithms and Data Structures Certification",
-  "dataVisCert": "Data Visualisation Certification",
-  "apisMicroservicesCert": "APIs and Microservices Certification",
-  "infosecQaCert": "Information Security and Quality Assurance Certification",
-  "qaCert": "Quality Assurance Certification",
-  "infosecCert": "Information Security Certification",
-  "sciCompPyCert": "Scientific Computing with Python Certification",
-  "dataAnalysisPyCert": "Data Analysis with Python Certification",
-  "machineLearningPyCert": "Machine Learning with Python Certification"
-}

api-server/server/utils/commit.js

@@ -1,63 +0,0 @@
-import dedent from 'dedent';
-import debugFactory from 'debug';
-import { Observable } from 'rx';
-import commitGoals from './commit-goals.json';
-
-const debug = debugFactory('fcc:utils/commit');
-
-export { commitGoals };
-
-export function completeCommitment$(user) {
-  const {
-    isFrontEndCert,
-    isBackEndCert,
-    isFullStackCert,
-    isRespWebDesignCert,
-    isFrontEndLibsCert,
-    isJsAlgoDataStructCert,
-    isDataVisCert,
-    isApisMicroservicesCert,
-    isInfosecQaCert,
-    isQaCertV7,
-    isInfosecCertV7,
-    isSciCompPyCertV7,
-    isDataAnalysisPyCertV7,
-    isMachineLearningPyCertV7
-  } = user;
-
-  return Observable.fromNodeCallback(user.pledge, user)().flatMap(pledge => {
-    if (!pledge) {
-      return Observable.just('No pledge found');
-    }
-
-    const { goal } = pledge;
-
-    if (
-      (isFrontEndCert && goal === commitGoals.frontEndCert) ||
-      (isBackEndCert && goal === commitGoals.backEndCert) ||
-      (isFullStackCert && goal === commitGoals.fullStackCert) ||
-      (isRespWebDesignCert && goal === commitGoals.respWebDesignCert) ||
-      (isFrontEndLibsCert && goal === commitGoals.frontEndLibsCert) ||
-      (isJsAlgoDataStructCert && goal === commitGoals.jsAlgoDataStructCert) ||
-      (isDataVisCert && goal === commitGoals.dataVisCert) ||
-      (isApisMicroservicesCert && goal === commitGoals.apisMicroservicesCert) ||
-      (isInfosecQaCert && goal === commitGoals.infosecQaCert) ||
-      (isQaCertV7 && goal === commitGoals.QaCert) ||
-      (isInfosecCertV7 && goal === commitGoals.infosecCert) ||
-      (isSciCompPyCertV7 && goal === commitGoals.sciCompPyCert) ||
-      (isDataAnalysisPyCertV7 && goal === commitGoals.dataAnalysisPyCert) ||
-      (isMachineLearningPyCertV7 && goal === commitGoals.machineLearningPyCert)
-    ) {
-      debug('marking goal complete');
-      pledge.isCompleted = true;
-      pledge.dateEnded = new Date();
-      pledge.formerUserId = pledge.userId;
-      pledge.userId = null;
-      return Observable.fromNodeCallback(pledge.save, pledge)();
-    }
-    return Observable.just(dedent`
-        You have not yet reached your goal of completing the ${goal}
-        Please retry when you have met the requirements.
-      `);
-  });
-}

api-server/server/utils/commit.json

@@ -1,50 +0,0 @@
-[
-  {
-    "name": "girl develop it",
-    "displayName": "Girl Develop It",
-    "donateUrl": "https://www.girldevelopit.com/donate",
-    "description": "Girl Develop It provides in-person classes for women to learn to code.",
-    "imgAlt": "Girl Develop It participants coding at tables.",
-    "imgUrl": "https://cdn-media-1.freecodecamp.org/imgr/U1CyEuA.jpg"
-  },
-  {
-    "name": "black girls code",
-    "displayName": "Black Girls CODE",
-    "donateUrl": "http://www.blackgirlscode.com/",
-    "description": "Black Girls CODE is devoted to showing the world that black girls can code, and do so much more.",
-    "imgAlt": "Girls developing code with instructor",
-    "imgUrl": "https://cdn-media-1.freecodecamp.org/imgr/HBVrdaj.jpg"
-  },
-  {
-    "name": "coderdojo",
-    "displayName": "CoderDojo",
-    "donateUrl": "https://www.globalgiving.org/projects/coderdojo-start-a-dojo-support/",
-    "description": "CoderDojo is the global network of free computer programming clubs for young people.",
-    "imgAlt": "Two adults help several kids program on their laptops.",
-    "imgUrl": "https://cdn-media-1.freecodecamp.org/imgr/701RLfV.jpg"
-  },
-  {
-    "name": "women who code",
-    "displayName": "Women Who Code",
-    "donateUrl": "https://www.womenwhocode.com/donate",
-    "description": "Women Who Code (WWCode) is a global leader in propelling women in the tech industry.",
-    "imgAlt": "Four women sitting in a classroom together learning to code.",
-    "imgUrl": "https://cdn-media-1.freecodecamp.org/imgr/tKUi6Rf.jpg"
-  },
-  {
-    "name": "girls who code",
-    "displayName": "Girls Who Code",
-    "donateUrl": "http://girlswhocode.com/",
-    "description": "Girls Who Code programs work to inspire, educate, and equip girls with the computing skills to pursue 21st century opportunities.",
-    "imgAlt": "Three women smiling while they code on a computer together.",
-    "imgUrl": "https://cdn-media-1.freecodecamp.org/imgr/op8BVph.jpg"
-  },
-  {
-    "name": "hack club",
-    "displayName": "Hack Club",
-    "donateUrl": "https://hackclub.com/donate",
-    "description": "Hack Club helps high schoolers start after-school coding clubs.",
-    "imgAlt": "A bunch of high school students posing for a photo in their Hack Club.",
-    "imgUrl": "https://cdn-media-1.freecodecamp.org/imgr/G2YvPHf.jpg"
-  }
-]

api-server/server/utils/get-return-to.test.js

@@ -1,60 +0,0 @@
-/* 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`;
-const defaultOrigin = homeLocation;
-const defaultPrefix = '';
-
-const defaultObject = {
-  returnTo: defaultReturnTo,
-  origin: defaultOrigin,
-  pathPrefix: defaultPrefix
-};
-
-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({
-        ...defaultObject,
-        returnTo: validReturnTo
-      });
-    });
-
-    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(
-        defaultObject
-      );
-    });
-
-    it('should return a default url for unknown origins', () => {
-      expect.assertions(1);
-      const encryptedReturnTo = jwt.sign(
-        { returnTo: invalidReturnTo },
-        validJWTSecret
-      );
-      expect(getReturnTo(encryptedReturnTo, validJWTSecret)).toStrictEqual(
-        defaultObject
-      );
-    });
-  });
-});

api-server/server/utils/middleware.js

@@ -2,22 +2,23 @@ import dedent from 'dedent';
 import { validationResult } from 'express-validator';
 
 import { createValidatorErrorFormatter } from './create-handled-error.js';
-import { homeLocation } from '../../../config/env';
 import {
   getAccessTokenFromRequest,
   removeCookies
 } from './getSetAccessToken.js';
+import { getRedirectParams } from './redirection';
 
-export function ifNoUserRedirectTo(url, message, type = 'errors') {
+export function ifNoUserRedirectHome(message, type = 'errors') {
   return function(req, res, next) {
     const { path } = req;
     if (req.user) {
       return next();
     }
 
+    const { origin } = getRedirectParams(req);
     req.flash(type, message || `You must be signed in to access ${path}`);
 
-    return res.redirect(url);
+    return res.redirect(origin);
   };
 }
 
@@ -55,15 +56,13 @@ export function ifNotVerifiedRedirectToUpdateEmail(req, res, next) {
   return next();
 }
 
-export function ifUserRedirectTo(path = `${homeLocation}/learn`, status) {
+export function ifUserRedirectTo(status) {
   status = status === 301 ? 301 : 302;
   return (req, res, next) => {
     const { accessToken } = getAccessTokenFromRequest(req);
+    const { returnTo } = getRedirectParams(req);
     if (req.user && accessToken) {
-      if (req.query && req.query.returnTo) {
-        return res.status(status).redirect(req.query.returnTo);
-      }
-      return res.status(status).redirect(path);
+      return res.status(status).redirect(returnTo);
     }
     if (req.user && !accessToken) {
       // This request has an active auth session

api-server/server/utils/redirection.js

@@ -1,30 +1,33 @@
 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
+// 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) {
+function getReturnTo(encryptedParams, secret, _homeLocation = homeLocation) {
   let params;
   try {
-    params = jwt.verify(encryptedReturnTo, secret);
+    params = jwt.verify(encryptedParams, 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,
+      returnTo: `${_homeLocation}/learn`,
+      origin: _homeLocation,
       pathPrefix: ''
     };
   }
 
-  return normalizeParams(params);
+  return normalizeParams(params, _homeLocation);
 }
 
 // TODO: tests!
-function normalizeParams({ returnTo, origin, pathPrefix }) {
+function normalizeParams(
+  { returnTo, origin, pathPrefix },
+  _homeLocation = homeLocation
+) {
   // coerce to strings, just in case something weird and nefarious is happening
   returnTo = '' + returnTo;
   origin = '' + origin;
@@ -35,20 +38,20 @@ function normalizeParams({ returnTo, origin, pathPrefix }) {
     !returnTo ||
     !allowedOrigins.some(allowed => returnTo.startsWith(allowed + '/'))
   ) {
-    returnTo = `${homeLocation}/learn`;
+    returnTo = `${_homeLocation}/learn`;
+    origin = _homeLocation;
+    pathPrefix = '';
   }
-  // this can be strict equality.
   if (!origin || !allowedOrigins.includes(origin)) {
-    origin = homeLocation;
+    returnTo = `${_homeLocation}/learn`;
+    origin = _homeLocation;
+    pathPrefix = '';
   }
-  // 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) {
@@ -59,7 +62,7 @@ function getRedirectBase(origin, pathPrefix) {
 // 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) {
+function getRedirectParams(req, _normalizeParams = normalizeParams) {
   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.
@@ -68,7 +71,7 @@ function getParamsFromReq(req) {
   // 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 };
+  return _normalizeParams({ returnTo: returnUrl.href, origin, pathPrefix });
 }
 
 function isRootPath(redirectBase, returnUrl) {
@@ -80,5 +83,5 @@ function isRootPath(redirectBase, returnUrl) {
 module.exports.getReturnTo = getReturnTo;
 module.exports.getRedirectBase = getRedirectBase;
 module.exports.normalizeParams = normalizeParams;
-module.exports.getParamsFromReq = getParamsFromReq;
+module.exports.getRedirectParams = getRedirectParams;
 module.exports.isRootPath = isRootPath;

api-server/server/utils/redirection.test.js

@@ -0,0 +1,120 @@
+/* global describe expect it */
+
+const jwt = require('jsonwebtoken');
+
+const { getReturnTo, normalizeParams } = require('./redirection');
+
+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 = 'https://www.freecodecamp.org/learn';
+const defaultOrigin = 'https://www.freecodecamp.org';
+const defaultPrefix = '';
+
+const defaultObject = {
+  returnTo: defaultReturnTo,
+  origin: defaultOrigin,
+  pathPrefix: defaultPrefix
+};
+
+describe('redirection', () => {
+  describe('getReturnTo', () => {
+    it('should extract returnTo from a jwt', () => {
+      expect.assertions(1);
+
+      const encryptedReturnTo = jwt.sign(
+        { returnTo: validReturnTo, origin: defaultOrigin },
+        validJWTSecret
+      );
+      expect(
+        getReturnTo(encryptedReturnTo, validJWTSecret, defaultOrigin)
+      ).toStrictEqual({
+        ...defaultObject,
+        returnTo: validReturnTo
+      });
+    });
+
+    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, defaultOrigin)
+      ).toStrictEqual(defaultObject);
+    });
+
+    it('should return a default url for unknown origins', () => {
+      expect.assertions(1);
+      const encryptedReturnTo = jwt.sign(
+        { returnTo: invalidReturnTo },
+        validJWTSecret
+      );
+      expect(
+        getReturnTo(encryptedReturnTo, validJWTSecret, defaultOrigin)
+      ).toStrictEqual(defaultObject);
+    });
+  });
+  describe('normalizeParams', () => {
+    it('should return a {returnTo, origin, pathPrefix} object', () => {
+      expect.assertions(2);
+      const keys = Object.keys(normalizeParams({}));
+      const expectedKeys = ['returnTo', 'origin', 'pathPrefix'];
+      expect(keys.length).toBe(3);
+      expect(keys).toEqual(expect.arrayContaining(expectedKeys));
+    });
+    it('should default to homeLocation', () => {
+      expect.assertions(1);
+      expect(normalizeParams({}, defaultOrigin)).toEqual(defaultObject);
+    });
+    it('should convert an unknown pathPrefix to ""', () => {
+      expect.assertions(1);
+      const brokenPrefix = {
+        ...defaultObject,
+        pathPrefix: 'not-really-a-name'
+      };
+      expect(normalizeParams(brokenPrefix, defaultOrigin)).toEqual(
+        defaultObject
+      );
+    });
+    it('should not change a known pathPrefix', () => {
+      expect.assertions(1);
+      const spanishPrefix = {
+        ...defaultObject,
+        pathPrefix: 'espanol'
+      };
+      expect(normalizeParams(spanishPrefix, defaultOrigin)).toEqual(
+        spanishPrefix
+      );
+    });
+    // we *could*, in principle, grab the path and send them to
+    // homeLocation/path, but if the origin is wrong something unexpected is
+    // going on. In that case it's probably best to just send them to
+    // homeLocation/learn.
+    it('should return default parameters if the origin is unknown', () => {
+      expect.assertions(1);
+      const exampleOrigin = {
+        ...defaultObject,
+        origin: 'http://example.com',
+        pathPrefix: 'espanol'
+      };
+      expect(normalizeParams(exampleOrigin, defaultOrigin)).toEqual(
+        defaultObject
+      );
+    });
+    it('should return default parameters if the returnTo is unknown', () => {
+      expect.assertions(1);
+      const exampleReturnTo = {
+        ...defaultObject,
+        returnTo: 'http://example.com/path',
+        pathPrefix: 'espanol'
+      };
+      expect(normalizeParams(exampleReturnTo, defaultOrigin)).toEqual(
+        defaultObject
+      );
+    });
+  });
+});