fix: resolve & in url upon redirect, shift prefill to textfield c…

…omponent (#569) * chore: replace '&' in ejs template * chore: remove he decoding * chore: shift stealth prefil to textfield component * chore: add tests for frontend.server.controller * chore: update package-lock * chore: add error handling to frontend.server.controller * chore: reorganize tests, add tests for invalid request * chore: log errors on frontend server controller * chore: reorganise tests * fix: relative paths * chore: standardise logger action to calling function * chore: return res
opengovsg · Nov 16, 2020 · 13b5ebf · 13b5ebf
1 parent e0ba8f2
commit 13b5ebf
Show file tree

Hide file tree

Showing 7 changed files with 174 additions and 49 deletions.
diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -121,7 +121,6 @@
     "font-awesome": "4.7.0",
     "fp-ts": "^2.8.5",
     "has-ansi": "^4.0.0",
-    "he": "^1.2.0",
     "helmet": "^4.2.0",
     "http-status-codes": "^2.1.4",
     "intl-tel-input": "~12.1.6",

diff --git a/src/app/controllers/__tests__/frontend.server.controller.spec.ts b/src/app/controllers/__tests__/frontend.server.controller.spec.ts
@@ -0,0 +1,80 @@
+import { StatusCodes } from 'http-status-codes'
+
+import expressHandler from 'tests/unit/backend/helpers/jest-express'
+
+import frontendServerController from '../frontend.server.controller'
+
+describe('frontend.server.controller', () => {
+  afterEach(() => jest.clearAllMocks())
+  const mockRes = expressHandler.mockResponse()
+  const mockReq = {
+    app: {
+      locals: {
+        GATrackingID: 'abc',
+        appName: 'xyz',
+        environment: 'efg',
+      },
+    },
+    query: {
+      redirectPath: 'formId?fieldId1=abc&fieldId2=xyz',
+    },
+  }
+  const mockBadReq = {
+    get: jest.fn().mockImplementation(() => 'abc'),
+  }
+  describe('datalayer', () => {
+    it('should return the correct response when the request is valid', () => {
+      frontendServerController.datalayer(mockReq, mockRes)
+      expect(mockRes.send).toHaveBeenCalledWith(
+        expect.stringContaining("'app_name': 'xyz'"),
+      )
+      expect(mockRes.send).toHaveBeenCalledWith(
+        expect.stringContaining("'config', 'abc'"),
+      )
+      expect(mockRes.type).toHaveBeenCalledWith('text/javascript')
+      expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.OK)
+    })
+    it('should return BAD_REQUEST if the request is not valid', () => {
+      frontendServerController.datalayer(mockBadReq, mockRes)
+      expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST)
+    })
+  })
+
+  describe('environment', () => {
+    it('should return the correct response when the request is valid', () => {
+      frontendServerController.environment(mockReq, mockRes)
+      expect(mockRes.send).toHaveBeenCalledWith('efg')
+      expect(mockRes.type).toHaveBeenCalledWith('text/javascript')
+      expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.OK)
+    })
+    it('should return BAD_REQUEST if the request is not valid', () => {
+      frontendServerController.environment(mockBadReq, mockRes)
+      expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST)
+    })
+  })
+
+  describe('redirectLayer', () => {
+    it('should return the correct response when the request is valid', () => {
+      const mockReqModified = {
+        // Test other special characters
+        query: {
+          redirectPath: 'formId?fieldId1=abc&fieldId2=<>\'"',
+        },
+      }
+      const redirectString =
+        'window.location.hash = "#!/formId?fieldId1=abc&fieldId2=&lt;&gt;&#39;&#34;'
+      // Note this is different from mockReqModified.query.redirectPath as
+      // there are html-encoded characters
+      frontendServerController.redirectLayer(mockReqModified, mockRes)
+      expect(mockRes.send).toHaveBeenCalledWith(
+        expect.stringContaining(redirectString),
+      )
+      expect(mockRes.type).toHaveBeenCalledWith('text/javascript')
+      expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.OK)
+    })
+    it('should return BAD_REQUEST if the request is not valid', () => {
+      frontendServerController.redirectLayer(mockBadReq, mockRes)
+      expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST)
+    })
+  })
+})
diff --git a/src/app/controllers/frontend.server.controller.js b/src/app/controllers/frontend.server.controller.js
@@ -2,6 +2,8 @@
 
 const ejs = require('ejs')
 const { StatusCodes } = require('http-status-codes')
+const logger = require('../../config/logger').createLoggerWithLabel(module)
+const { createReqMeta } = require('../utils/request')
 
 /**
  * Google Tag Manager initialisation Javascript code templated
@@ -18,21 +20,47 @@ module.exports.datalayer = function (req, res) {
         'app_name': '<%= appName%>'
       });
     `
-  res
-    .type('text/javascript')
-    .status(StatusCodes.OK)
-    .send(ejs.render(js, req.app.locals))
+  try {
+    const ejsRendered = ejs.render(js, req.app.locals)
+    return res.type('text/javascript').status(StatusCodes.OK).send(ejsRendered)
+  } catch (err) {
+    logger.error({
+      message: 'Error returning datalayer',
+      meta: {
+        action: 'datalayer',
+        ...createReqMeta(req),
+      },
+      error: err,
+    })
+    return res.status(StatusCodes.BAD_REQUEST).json({
+      message: 'There was an unexpected error. Please refresh and try again.',
+    })
+  }
 }
 
 /**
  * Custom Javascript code templated with environment variables.
  * @returns {String} Templated Javascript code for the frontend
  */
 module.exports.environment = function (req, res) {
-  res
-    .type('text/javascript')
-    .status(StatusCodes.OK)
-    .send(req.app.locals.environment)
+  try {
+    return res
+      .type('text/javascript')
+      .status(StatusCodes.OK)
+      .send(req.app.locals.environment)
+  } catch (err) {
+    logger.error({
+      message: 'Error returning environment',
+      meta: {
+        action: 'environment',
+        ...createReqMeta(req),
+      },
+      error: err,
+    })
+    return res.status(StatusCodes.BAD_REQUEST).json({
+      message: 'There was an unexpected error. Please refresh and try again.',
+    })
+  }
 }
 
 /**
@@ -46,8 +74,25 @@ module.exports.redirectLayer = function (req, res) {
     // Change url from form.gov.sg/123#!123 to form.gov.sg/#!/123
     window.history.replaceState("","", "/#!/<%= redirectPath%>")
   `
-  res
-    .type('text/javascript')
-    .status(StatusCodes.OK)
-    .send(ejs.render(js, req.query))
+  // If there are multiple query params, '&' is html-encoded as '&amp;', which is not valid URI
+  // Prefer to replace just '&' instead of using <%- to output unescaped values into the template
+  // As this could potentially introduce security vulnerability
+  // See https://ejs.co/#docs for tags
+
+  try {
+    const ejsRendered = ejs.render(js, req.query).replace(/&amp;/g, '&')
+    return res.type('text/javascript').status(StatusCodes.OK).send(ejsRendered)
+  } catch (err) {
+    logger.error({
+      message: 'Error returning redirectLayer',
+      meta: {
+        action: 'redirectlayer',
+        ...createReqMeta(req),
+      },
+      error: err,
+    })
+    return res.status(StatusCodes.BAD_REQUEST).json({
+      message: 'There was an unexpected error. Please refresh and try again.',
+    })
+  }
 }
diff --git a/src/public/modules/forms/base/components/field-textfield.client.component.js b/src/public/modules/forms/base/components/field-textfield.client.component.js
@@ -1,5 +1,7 @@
 'use strict'
 
+const querystring = require('querystring')
+
 angular.module('forms').component('textFieldComponent', {
   templateUrl:
     'modules/forms/base/componentViews/field-textfield.client.view.html',
@@ -8,5 +10,35 @@ angular.module('forms').component('textFieldComponent', {
     forms: '<',
     placeholder: '<',
   },
+  controller: ['$location', '$sanitize', textFieldComponentController],
   controllerAs: 'vm',
 })
+
+function textFieldComponentController($location, $sanitize) {
+  // Stealth prefill feature
+  // If a query parameter is provided to a form URL in the form ?<fieldId1>=<value1>&<fieldId2>=<value2>...
+  // And if the fieldIds are valid mongoose object IDs and refer to a short text field,
+  // Then prefill and disable editing the corresponding form field on the frontend
+
+  const vm = this
+  vm.$onInit = () => {
+    const query = $location.url().split('?')
+    const queryParams =
+      query.length > 1 ? querystring.parse(query[1]) : undefined
+
+    if (
+      !vm.field.myInfo && // disallow prefill for myinfo
+      vm.field.allowPrefill && // allow prefill only if flag enabled
+      queryParams &&
+      vm.field._id in queryParams
+    ) {
+      const prefillValue = queryParams[vm.field._id]
+      if (typeof prefillValue === 'string') {
+        // Only support unique query params. If query params are duplicated,
+        // none of the duplicated keys will be prefilled
+        vm.field.fieldValue = $sanitize(prefillValue) // $sanitize as a precaution to prevent xss
+        // note that there are currently no unit tests to ensure that value is sanitized correctly; manual testing required
+      }
+    }
+  }
+}
diff --git a/src/public/modules/forms/base/directives/field.client.directive.js b/src/public/modules/forms/base/directives/field.client.directive.js
@@ -1,19 +1,12 @@
 'use strict'
 
 const { get } = require('lodash')
-const he = require('he')
-const querystring = require('querystring')
 
 angular
   .module('forms')
-  .directive('fieldDirective', [
-    'FormFields',
-    '$location',
-    '$sanitize',
-    fieldDirective,
-  ])
+  .directive('fieldDirective', ['FormFields', fieldDirective])
 
-function fieldDirective(FormFields, $location, $sanitize) {
+function fieldDirective(FormFields) {
   return {
     restrict: 'E',
     templateUrl:
@@ -30,32 +23,6 @@ function fieldDirective(FormFields, $location, $sanitize) {
       isValidateDate: '<',
     },
     link: function (scope) {
-      // Stealth prefill feature
-      // If a query parameter is provided to a form URL in the form ?<fieldId1>=<value1>&<fieldId2>=<value2>...
-      // And if the fieldIds are valid mongoose object IDs and refer to a short text field,
-      // Then prefill and disable editing the corresponding form field on the frontend
-
-      const decodedUrl = he.decode($location.url()) // tech debt; after redirect, & is encoded as &amp; in the query string
-      const query = decodedUrl.split('?')
-      const queryParams =
-        query.length > 1 ? querystring.parse(query[1]) : undefined
-
-      if (
-        !scope.field.myInfo && // disallow prefill for myinfo
-        scope.field.allowPrefill && // allow prefill only if flag enabled
-        queryParams &&
-        scope.field._id in queryParams &&
-        scope.field.fieldType === 'textfield'
-      ) {
-        const prefillValue = queryParams[scope.field._id]
-        if (typeof prefillValue === 'string') {
-          // Only support unique query params. If query params are duplicated,
-          // none of the duplicated keys will be prefilled
-          scope.field.fieldValue = $sanitize(prefillValue) // $sanitize as a precaution to prevent xss
-          // note that there are currently no unit tests to ensure that value is sanitized correctly; manual testing required
-        }
-      }
-
       if ((scope.isadminpreview || scope.isTemplate) && scope.field.myInfo) {
         // Determine whether to disable field in preview
         if (

diff --git a/tests/unit/backend/helpers/jest-express.ts b/tests/unit/backend/helpers/jest-express.ts
@@ -38,6 +38,7 @@ const mockResponse = (
     status: jest.fn().mockReturnThis(),
     send: jest.fn().mockReturnThis(),
     sendStatus: jest.fn().mockReturnThis(),
+    type: jest.fn().mockReturnThis(),
     json: jest.fn(),
     render: jest.fn(),
     redirect: jest.fn(),