feat: prevent special characters from url #1266

lauramargar · 2023-08-28T12:19:27Z

Pull request template

To prevent a Cross-site Scripting attack, we must be careful with the characters we allow to type a query. We already have a function to avoid that when a user types in the search box. But we also need to check if there are any special characters when a query is typed through the URL.

Motivation and context

Dependencies. If any, specify:
Open issue. If applicable, link: EMP-1745

Type of change

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that causes existing functionality to not work as expected)
Change requires a documentation update

What is the destination branch of this PR?

Main
Other. Specify:

How has this been tested?

Try to type a special character (<>) in the URL query.

Checklist:

My code follows the style guidelines of this project.
I have performed a self-review on my own code.
I have commented my code, particularly in hard-to-understand areas.
I have made corresponding changes to the documentation.
My changes generate no new warnings.
I have added tests that prove my fix is effective or that my feature works.
New and existing unit tests pass locally with my changes.

feat: prevent special characters from url

annacv · 2023-08-28T14:01:15Z

Hi! I have 2 doubts:

Should we remove the code between <> from the URL?
Should this prevent special keys be applied also for extraParams?? I tried with http://localhost:8080/?q=sunglasses&store=%3Cdetails%20%253e%20open%3D1%20ontoggle%3D%22n%3D%2Fal%2F.source.concat%28%2Fert%2F.source%29%2Cx%3Ddocument%2Ca%3Dwindow%5Bn%5D%2Ca%28x.domain%29%22%3E, and when I deleted the query and typed again (this or another query), it kept those characters in the new search.

herrardo · 2023-08-28T20:30:21Z

packages/x-components/src/x-modules/url/components/url-handler.vue

@@ -156,7 +163,10 @@
    protected emitEvents(): void {
      const { all, extra } = this.parseUrlParams();
      const metadata = this.createWireMetadata();
-      this.$x.emit('ParamsLoadedFromUrl', all, metadata);
+      this.preventSpecialKey(all.query);
+      if (!this.hasSpecialKeys) {


Just curious, I think I like the approach, but why are we preventing to emit the event if we have special keys instead of removing them, and send send it without them?

Do you prefer to remove the special keys instead of the whole query? I mean, if you have this query round <hola> shirt, it would be transformed into round shirt and sent into the ParamsLoadedFromUrlevent.

I use this approach because, in the task description, I understood that we have to clean/remove the whole query. But for me makes more sense to remove only the special characters and the string inside them.

herrardo · 2023-08-28T20:33:07Z

packages/x-components/src/x-modules/url/components/url-handler.vue

@@ -165,6 +175,18 @@
      this.urlLoaded = true;
    }

+    /**
+     * Prevents the user from either typing or pasting special characters in the url query.


I would say:

Suggested change

* Prevents the user from either typing or pasting special characters in the url query.

* Detects if the user is typing or pasting special/forbidden characters in the URL query

herrardo · 2023-08-28T20:36:33Z

packages/x-components/src/x-modules/url/components/url-handler.vue

+     */
+    protected preventSpecialKey(query: string): void {
+      if (/[<>]/.test(query ?? '')) {
+        this.hasSpecialKeys = true;


Why storing this value in hasSpecialKeys protected property instead of returning true/false in this function and then test it on line 167. I mean, I would change this function name to hasSpecialKeys and then test:

if (!this.hasSpecialKeys(this query)) {

herrardo · 2023-08-28T21:12:01Z

packages/x-components/src/x-modules/url/components/url-handler.vue

+      this.preventSpecialKey(all.query);
+      if (!this.hasSpecialKeys) {


Suggested change

this.preventSpecialKey(all.query);

if (!this.hasSpecialKeys) {

if (this.hasSpecialKeys(all.query)) {

herrardo · 2023-08-28T21:14:24Z

packages/x-components/src/x-modules/url/components/url-handler.vue

+    protected preventSpecialKey(query: string): void {
+      if (/[<>]/.test(query ?? '')) {
+        this.hasSpecialKeys = true;
+      }


Suggested change

protected preventSpecialKey(query: string): void {

if (/[<>]/.test(query ?? '')) {

this.hasSpecialKeys = true;

}

protected hasSpecialKeys(query: string): void {

return (/[<>]/.test(query ?? ''))

packages/x-components/src/x-modules/url/components/url-handler.vue

…P-1745-prevent-special-keys-that-come-from-url

packages/x-components/src/x-modules/url/components/url-handler.vue

annacv · 2023-09-05T08:02:21Z

Hi! I will add this reference here, maybe it would be worth to give it a chance: https://github.com/braintree/sanitize-url/blob/main/src/index.ts

lauramargar · 2023-09-05T08:09:30Z

Hi! I will add this reference here, maybe it would be worth to give it a chance: https://github.com/braintree/sanitize-url/blob/main/src/index.ts

We have an util in X https://github.com/empathyco/x/blob/main/packages/x-components/src/utils/sanitize.ts . I have already made the new approach.

diegopf · 2023-09-06T13:14:22Z

packages/x-components/src/x-modules/url/components/url-handler.vue

@@ -255,10 +256,13 @@
          const urlKey = this.getUrlKey(name);
          if (urlSearchParams.has(urlKey)) {
            if (name in initialUrlState) {
-              const urlValue = urlSearchParams.getAll(urlKey);
+              let urlValue = urlSearchParams.getAll(urlKey);
+              urlValue[0] = sanitize(urlValue[0]);


To be safe we should sanitize all values and not just the first one. The query only has one occurrence but potentially any parameter could be malicious.

urlValue is not only query param, all URL params are passing one by one. [0] is necessary because urlValue is an array of only one element, the param that is being checked.

For example, try to search for a query and select a filter, then write the special characters (<>) in the filter and in the query.

Sorry, I think I did not explain properly 😅 What I meant is that a url param can have more than one occurrence. Think about filters, you can select more than one filter at the same time. In that scenario you get an array with multiple entries like:

['categoryIds:12fad53d7', 'price:1600.0-2000.0']

so if you only pick the first element, you will fail to sanitize the other values.

herrardo · 2023-09-12T07:31:47Z

This PR is being closed as we decided that we will solve this problem in the Archetype, not letting certain information to be applied to the website through v-html directives

[EMP-1745]

79ce508

feat: prevent special characters from url

lauramargar requested a review from a team as a code owner August 28, 2023 12:19

herrardo suggested changes Aug 28, 2023

View reviewed changes

chore: another approach related to the pr comments

4a42549

github-advanced-security bot found potential problems Aug 29, 2023

View reviewed changes

packages/x-components/src/x-modules/url/components/url-handler.vue Fixed Show fixed Hide fixed

lauramargar added 2 commits August 29, 2023 09:42

Merge branch 'main' of https://github.com/empathyco/x into feature/EM…

67a3d0a

…P-1745-prevent-special-keys-that-come-from-url

chore: remove special keys from all params in URL

db1cd5f

github-advanced-security bot found potential problems Aug 30, 2023

View reviewed changes

packages/x-components/src/x-modules/url/components/url-handler.vue Fixed Show resolved Hide resolved

chore: add more cases to prevent html injection

f2c20b5

github-advanced-security bot found potential problems Aug 31, 2023

View reviewed changes

packages/x-components/src/x-modules/url/components/url-handler.vue Fixed Show fixed Hide fixed

packages/x-components/src/x-modules/url/components/url-handler.vue Fixed Show fixed Hide fixed

chore: fix replace options

8e9b26d

github-advanced-security bot found potential problems Aug 31, 2023

View reviewed changes

packages/x-components/src/x-modules/url/components/url-handler.vue Fixed Show fixed Hide fixed

packages/x-components/src/x-modules/url/components/url-handler.vue Fixed Show fixed Hide fixed

herrardo suggested changes Aug 31, 2023

View reviewed changes

packages/x-components/src/x-modules/url/components/url-handler.vue Outdated Show resolved Hide resolved

chore: pr comments

461dd69

chore: new approach using sanitize util

eb64d7f

diegopf self-requested a review September 5, 2023 12:33

diegopf requested changes Sep 6, 2023

View reviewed changes

herrardo closed this Sep 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: prevent special characters from url #1266

feat: prevent special characters from url #1266

lauramargar commented Aug 28, 2023 •

edited by jira bot

Loading

annacv commented Aug 28, 2023 •

edited

Loading

herrardo Aug 28, 2023

lauramargar Aug 29, 2023

herrardo Aug 28, 2023

herrardo Aug 28, 2023

herrardo Aug 28, 2023

herrardo Aug 28, 2023

annacv commented Sep 5, 2023

lauramargar commented Sep 5, 2023

diegopf Sep 6, 2023

lauramargar Sep 6, 2023

diegopf Sep 7, 2023

herrardo commented Sep 12, 2023

	* Prevents the user from either typing or pasting special characters in the url query.
	* Detects if the user is typing or pasting special/forbidden characters in the URL query

		this.preventSpecialKey(all.query);
		if (!this.hasSpecialKeys) {

	this.preventSpecialKey(all.query);
	if (!this.hasSpecialKeys) {
	if (this.hasSpecialKeys(all.query)) {

feat: prevent special characters from url #1266

feat: prevent special characters from url #1266

Conversation

lauramargar commented Aug 28, 2023 • edited by jira bot Loading

Pull request template

Motivation and context

Type of change

What is the destination branch of this PR?

How has this been tested?

Checklist:

annacv commented Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annacv commented Sep 5, 2023

lauramargar commented Sep 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

herrardo commented Sep 12, 2023

lauramargar commented Aug 28, 2023 •

edited by jira bot

Loading

annacv commented Aug 28, 2023 •

edited

Loading