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

Improve URL validation and centralise client and server validators #164

Merged
merged 4 commits into from
Jun 10, 2020

Conversation

kylerwsm
Copy link
Contributor

@kylerwsm kylerwsm commented Jun 8, 2020

Problem

This PR is a follow-up to #149. Previously, the check for https was disabled in development, as localstack endpoints are not valid urls.

And as suggested by @liangyuanruo, I have centralised the common validators used by our client and server sides.

Solution

  • Shift from the built-in Sequelize isUrl check to our customised checks using the validator library. After which, I whitelist the endpoint used by localstack (http://localhost:4572).
  • Added a new optional parameter to isValidUrl and isHttps, to indicate whether to take into account whitelisted hostnames when running the validation.
  • Created tests for all url validators.

Improvements:

  • Validation methods used by the client and server are now centralised at src/shared/util/validation.ts.
  • Migrate the above-mentioned validation file to TypeScript.

@kylerwsm kylerwsm requested review from JasonChong96 and yong-jie June 8, 2020 11:58
// Disable long url checks for localstack in development.
...(!DEV_ENV ? { isUrl: true } : {}),
urlCheck(url: string) {
if (!isValidUrl(url, true)) {
Copy link
Contributor

@liangyuanruo liangyuanruo Jun 8, 2020

Choose a reason for hiding this comment

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

does this mean someone will be able to save the whitelisted localhost URL in production?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically, users will not be able to using as we are enforcing https during link creation and edits. But I get the gist here. I will convert the whitelist to a dev-only whitelist. Subsequently, if there is a need for a global whitelist in the future, it should be quite easy to add in.

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.

looks like a good first step towards standardising URL validation. can we guarantee there is no change in behavior?

one possible improvement in the future would be to replace the useWhitelist flag with dependency injection, because the setting is specific to the application environment, and the onus is on the caller to know whether to use a whitelist or not.

@kylerwsm
Copy link
Contributor Author

kylerwsm commented Jun 9, 2020

I have updated my code such that whitelist is only used when DEV_ENV is true. I was considering the use of dependency injection, which I have decided to drop for now, as it somewhat decentralises the validators again (as the frontend does not have access to environment variables, and have no dependency injection).

@kylerwsm kylerwsm requested a review from liangyuanruo June 9, 2020 14:53
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.

lgtm

@liangyuanruo liangyuanruo merged commit dbc22d2 into develop Jun 10, 2020
@liangyuanruo liangyuanruo deleted the url-val branch June 10, 2020 04:41
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.

2 participants