Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: resolve & in url upon redirect, shift prefill to textfield component #569

Merged
merged 12 commits into from
Nov 16, 2020

Conversation

tshuli
Copy link
Contributor

@tshuli tshuli commented Nov 2, 2020

Problem

Solution

  • Replace & with & in the ejs template. This was preferred to allowing all characters to be escaped, which may possibly introduce a security vulnerability

Before & After

The following non-hashbang URL will be redirected as follows:
https://form.gov.sg/<formId>?<fieldId1>=abc&<fieldId2>=xyz

BEFORE:
https://form.gov.sg/#!/<formId>?<fieldId1>=abc&amp;<fieldId2>=xyz

AFTER:
https://form.gov.sg/#!/<formId>?<fieldId1>=abc&<fieldId2>=xyz

Tests

  • Create a form with short text field. Open in public view with the query params =, where comprises alphanumeric/hyphen/underscore characters. Check that the short text field is not prefilled with value1
  • In the db, check that allowPrefill is set to false for the short text field. set allowPrefill = true. Check that prefill now works.
  • Create myinfo field. Set allowPrefill = true for the myinfo field. Access form on public view and login to singpass. Check that prefill does not work on the myinfo field.
  • Check that prefilling works even without the hashbang in the url
  • Check that for non-hashbang url with two query params, upon redirection, the redirect path uses & to separate the query params instead of &amp;
  • Add radio button to the form. Set form logic to hide the short text field conditional on the radio button. Check that the short text field is prefilled when it is unhidden
  • Prefilled field value is cleared when field is hidden by logic at the point of submission

Deploy Notes

dependencies

  • he module has been removed

@tshuli tshuli marked this pull request as ready for review November 3, 2020 02:48
Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some comments on the tests!

Copy link
Contributor

@liangyuanruo liangyuanruo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to address @mantariksh 's comments before approval

@tshuli
Copy link
Contributor Author

tshuli commented Nov 12, 2020

@mantariksh thanks for the comments, edits for your re-review :)

@tshuli
Copy link
Contributor Author

tshuli commented Nov 14, 2020

@liangyuanruo @mantariksh please review before next release :)

Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a couple of nits:

  1. logger meta.action has been standardised to the calling function so it's easier to understand logs
  2. we always return res.status(...).json(...) to prevent mistakes in the future where code is written after res.send.

otherwise lgtm!

src/app/controllers/frontend.server.controller.js Outdated Show resolved Hide resolved
src/app/controllers/frontend.server.controller.js Outdated Show resolved Hide resolved
src/app/controllers/frontend.server.controller.js Outdated Show resolved Hide resolved
src/app/controllers/frontend.server.controller.js Outdated Show resolved Hide resolved
src/app/controllers/frontend.server.controller.js Outdated Show resolved Hide resolved
src/app/controllers/frontend.server.controller.js Outdated Show resolved Hide resolved
src/app/controllers/frontend.server.controller.js Outdated Show resolved Hide resolved
src/app/controllers/frontend.server.controller.js Outdated Show resolved Hide resolved
src/app/controllers/frontend.server.controller.js Outdated Show resolved Hide resolved
@tshuli tshuli merged commit 13b5ebf into develop Nov 16, 2020
@tshuli tshuli deleted the prefill-techdebt branch November 16, 2020 04:41
@karrui karrui mentioned this pull request Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate why & character in non-hashbang url query params is encoded to &amp; upon redirect
3 participants