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(sanitiseUrl): fix limitations with library #1821

Merged

Conversation

kishore03109
Copy link
Contributor

@kishore03109 kishore03109 commented Mar 5, 2024

Problem

The sanitize-url library does filters HTML entities, but it does not do so recursively. By nesting HTML entities, it is possible to create a URL which specifies the JavaScript protocol handler.

Closes GTA-24-006

Solution

Handroll a quick url constructor and check that the protocols are adhered to.

Breaking Changes

  • Yes - this PR contains breaking changes
    • Details ...
  • No - this PR is backwards compatible with ALL of the following feature flags in this doc

Tests

Screenshot 2024-03-05 at 1 10 36 PM

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @kishore03109 and the rest of your teammates on Graphite Graphite

@kishore03109 kishore03109 marked this pull request as ready for review March 5, 2024 05:15
@kishore03109 kishore03109 requested a review from a team March 5, 2024 05:15
@kishore03109 kishore03109 force-pushed the 03-05-fix_sanitiseUrl_fix_limitations_with_library branch from 4b83b15 to da1888f Compare March 7, 2024 10:39
Copy link
Contributor

@alexanderleegs alexanderleegs left a comment

Choose a reason for hiding this comment

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

lgtm

@kishore03109 kishore03109 merged commit fe6ca6c into develop Mar 11, 2024
9 checks passed
@mergify mergify bot deleted the 03-05-fix_sanitiseUrl_fix_limitations_with_library branch March 11, 2024 06:16
This was referenced Mar 11, 2024
dcshzj added a commit that referenced this pull request Mar 11, 2024
* fix(media): should not have files that begin with underscore (#1819)

## Problem

currently files that start with _ are ignored. this means that in the final output, the site does not have the broken link.

this issue is quite existent in our sites (eg. https://www.cdc.gov.sg/our-programmes/gallery/2021/) 

when i did a string search, this lead to quite a number of sites with these types of images. this is not ideal, and as such this edge case will be coded out as part of the link checker for user to fix

Tests 
- [ ] enter into a repo and try to rename an image into something with an leading underscore.
- [ ] asset that you get the error message as shown below
![Screenshot 2024-03-05 at 2 03 20 PM](https://github.com/isomerpages/isomercms-frontend/assets/42832651/95fbccc9-aa7e-4fae-831c-2ccbb236c8c7)
- [ ] upload an image named '_name.png' into the images folder. assert that the leading underscore gets stripped off


https://github.com/isomerpages/isomercms-frontend/assets/42832651/1ce91503-04b0-442e-8fef-10ae99e3129c

* fix(sanitiseUrl): fix limitations with library (#1821)

## Problem

The sanitize-url library does filters HTML entities, but it does not do so recursively. By nesting HTML entities, it is possible to create a URL which specifies the JavaScript protocol handler.

Closes GTA-24-006

## Solution

Handroll a quick url constructor and check that the protocols are adhered to. 

**Breaking Changes**

<!-- Does this PR contain any backward incompatible changes? If so, what are they and should there be special considerations for release? -->

- [ ] Yes - this PR contains breaking changes
  - Details ...
- [X] No - this PR is backwards compatible with ALL of the following feature flags in this [doc](https://www.notion.so/opengov/Existing-feature-flags-518ad2cdc325420893a105e88c432be5)

## Tests

<!-- What tests should be run to confirm functionality? -->
- [ ] Login via github and visit "http://localhost:3000/sites/kishore-test-dev-gh/contact-us" 
- [ ] when hovering over `[+65 6123 4589](tel:+6561234589)` verify that it links to `tel:+6561234589`
- [ ] when hovering over `[[email protected]](mailto:[email protected])` verify that it links to `[email protected]`
- [ ] when hovering over `[online form](https://www.form.gov.sg/)` verify that it links to `https://www.form.gov.sg/`

<img width="533" alt="Screenshot 2024-03-05 at 1 10 36 PM" src="https://github.com/isomerpages/isomercms-frontend/assets/42832651/87c5edbb-8744-47d1-8b9d-01f38893dc15">

* 0.81.0

---------

Co-authored-by: Alexander Lee <[email protected]>
Co-authored-by: Kishore <[email protected]>
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