Xss validator not allowing having < and > characters in request body

[dariasamo]

What is actually happening?

This issue is similar to Error when sending FormData #147. The same error is being thrown when there is a request to nuxt api with request body containing > and < characters. For example: Organization name>. It looks like parsing it through the validator causes this problem. Sending Organization <a>name is parsed correctly. Disabling xss validator middleware solves this problem but I was wondering if it can accept single < or > characters without disabling the whole xss validation functionality?

Version

nuxt-security: v0.14.4
nuxt: v3.5.3

Steps to reproduce

Send a request to nuxt api with request body containing < or > or both characters but not a valid HTML tag which are supported.

[Baroshem]

Hey, that is an interesting finding!

Have you tried using whitelist option? https://github.com/leizongmin/js-xss/blob/master/README.md

It is using some default whitelist so maybe you will be able to override it somehow.

We can also try to add some custom filters here that would change the behavior of the default filters by XSS package.

[dariasamo]

@Baroshem Thank you for your reply. I should be able to use whitelist to whitelist certain tags but in my case it's just < and > characters which cause the issue. Same happens if I submit unknown HTML tag. I would expect that xss validator would encode < and > characters as mentioned here but it doesn't happen 🤔 By default it should escape the tag. But in my case the error is being thrown and I'm not able to save the data.

[dariasamo]

Actually I just added the following config to test out whitelist functionality:

xssValidator: {
  whiteList: {
      a: ['href', 'title', 'target'],
  },
  stripIgnoreTag: true,
  throwError: false,
},

What happens now is that if I try to submit any HTML tag other than <a>, i.e. <b> tag, it doesn't strip it out but throws a 400 error instead. I would expect the sanitised input will not contain this tag and the error is not thrown at all (since it's disabled). What can be wrong?

[dariasamo]

I believe the problem is located here:
https://github.com/Baroshem/nuxt-security/blob/0d36fec295e9b8bb2d5749d3330ff88397fe8ac6/src/runtime/server/middleware/xssValidator.ts#L28

If the input string contains a tag, < or > character, and this tag is not whitelisted (or in default whitelist), it will be encoded by xss validator using default escapeHTML function. So the processed value will always be different that the initial value, and the error will be thrown.

[Baroshem]

Hey @dariasamo

Interesting finding. Would you be interested in creating a pull request with your proposition for change?

I could then test it out to see if it can be added to the next release :)

[dariasamo]

Sure, I just need to understand when and if the error has to be thrown. Right now it happens whenever the the values don't match, which I think shouldn't be the case since the sanitized input will differ from the initial one. So I guess I don't totally understand the purpose of the error 🤔

[Baroshem]

The general idea was to to throw an error once the passed value in either body or query contains some values that are sanitized by XSS Library.

To solve your isse and do not break the existing functionality, I would recommend to create some custom filters that will be triggered before the xss package as it is an external dependency that we cannot modify that easily.

Maybe we could create some additional logic that would alter the query and body so that once it contains one char of < or > it will do something with the input so that it is not sanitized by the XSS pckage?

Then it would fix your issue and wont stop how it works for other users.

[dariasamo]

Hi @Baroshem thank you for the explanation. My understanding was different before as I expected no error to be thrown if the input was sanitized by xss library. I think I have one more unclear point in this integration not related to my previous use case.

In case the error is thrown each time the value is sanitized, what is the point of stripIgnoreTag setting as it would cause modified input as well? As I understand from the documentation it should be possible to disable throwError but I can see only the setting on the requestSizeLimiter would disable it.
https://github.com/Baroshem/nuxt-security/blob/0d36fec295e9b8bb2d5749d3330ff88397fe8ac6/src/runtime/server/middleware/xssValidator.ts#L33
And what would happen if I manage to disable the error to be thrown, how should I proceed if I want to post the sanitized input to API? Let's say with the following config:

xssValidator: {
      stripIgnoreTag: true
      throwError: false, // optional
    }

and input:

<sdsds>Hello world<sdsdd>

I expect the tags to be filtered out. If I don't have error disabled, I still get an error and cannot proceed. If I can disable an error, I would still not be able to do anything with the sanitized input, or am I missing something? Thank you

[Baroshem]

Hey,

The case with requestSizeLimiter is a bug (typo when I was implementing throwError functionality for all middlewares). I will fix it in the upcoming 1.0.0-rc.1 version.

The throw error functionality is related to the fact that by default Nuxt has its own design for errors (custom style). The throwError allows you as a user to get the error object as a return value and just react to it with your custom error page.

[dariasamo]

Hey, yes the throw error functionality is clear but it is still not clear what I can do if I enable stripIgnoreTag. It seems to me that this setting doesn't do anything with the current design 🤔 Anyway I will think about the custom filters for the < and > characters, thanks for the suggestion.

[Baroshem]

If you need any assistance from my side, just let me know :)

[Baroshem]

Heyo, any progress here?

I think this could be useful for the upcoming 1.0.0 release :)

[dariasamo]

Hi @Baroshem

I had 2 ideas in mind when trying to tackle this issue:

1. Add a custom escape HTML function which would skip single > and < characters but escape the HTML tags which are not in the whitelist. Something like this:

escapeHtml: (html: string) => {
    return html.replace(/<[^>]*>/g, function (match) {
        return match.replace(/</g, '&lt;').replace(/>/g, '&gt;');
    });
},

This solution is not bullet-proof and I can imagine there will be some edge cases, when this might not work.

2. Switch off the xssValidator middleware and add the js-xss or other similar package as our direct dependency, so there would be better control over the sanitized input in our app.

In the end we decided to simply disallow using such characters, add some FE validation and keep the middleware functionality as is.

[dariasamo]

What do you think about adding a new property to the error object (i.e. code: DangerousInput) or changing statusMessage to something more descriptive, like Dangerous Input? It would be easier to distinguish where this error belongs to.

[Baroshem]

1. I think this could work. We could replace single < or > with this and just let the rest of the input be sanitized normally with xss package. Could you create a proof of concept of this solution? I would like to try it out for some more advanced cases :) (please target bracnch 1.0.0-rc.1)
2. But built in xssValidators already uses https://github.com/leizongmin/js-xss
3. I have used the default HTTP error messages for status codes and in this case it is a Bad Request and I think that it is a good approach as it follows general HTTP rules. Sharing too much information inside err.message can leak information how to override the security behavior.

[dariasamo]

1. I think it shouldn't be a part of nuxt-security. I imagined this function to be passed as an option to nuxt config under xssValidator to overwrite the default escapeHtml function of the js-xss validator.
2. Yes, of course. But it's not possible to do anything with the sanitized input right now, as the xssValidator middleware just throws an error when the input was sanitized. If one wants to save the sanitized input to the server or do something else about it, they should disable the middleware and implement their own sanitization.
3. What if I want to distinguish between the error thrown by xssValidator and other Bad Request in my app so I can add appropriate handling?

[Baroshem]

1. So you are not thinking about adding this as a part of the module but more as a way of customizing the. functionality of the default xss validator?
2. But why would you like to save sanitized input? If it was sanitized, it most probably means that it includes the characters or code that shouldnt be allowed to move further. The idea of this middleware is to not let that happen. From what I understand from your comment, you would expect to be able to return or store somewhere the sanitized input so that you could do something about it. But to be honest, to make it work while supporting current behavior, it would most probably mean that a custom implementation is needed anyway. I dont think having such case is a reason to add such custom override in the module. Maybe it would be better to wait for the js-xss to fix this kind of behavior globally?
3. I see your point. What would be your suggestion of an error message for such validation error?

[dariasamo]

1. Yes, exactly :) I can imagine adding a allowArrowCharacters option which would enable the above mentioned customized escapeHtml function but I'm not sure if it's really needed for most users. It is ambiguous whether these < and > are broken / malicious tags or just plaintext input. In my case it was not important to have such characters in the user input, so I decided to disallow them. I can imagine a person who really needs them will just customize the functionality of js-xss.

2. There can be use cases for that. Maybe the better example would be not with escaped HTML but filtered out tags (stripIgnoreTag option). Let's say you want to allow users to paste HTML fragments (or other markup) such as those created by rich text editors but you want to clean it up before you save it. I agree that it doesn't have to be the part of this module. That is why we were considering to switch off the xssValidator middleware as I mentioned in the earlier post and implement our own sanitization.

3. What about Validation Error? It is general enough to not leak extra information but more specific than Bad Request

[Baroshem]

Hey @dariasamo

Sorry for no contact from my side. I was quite busy with work stuff.

Could you prepare a Proof of Concept PR with the solution that would solve the issue you mentioned? I would like to try it out and see how we can make it part of the module :)

[Baroshem]

@dariasamo

Would you like to work on it? :)

[Baroshem]

Closing due to no further contact from the issue author.

If this will be needed again, please let me know and I will reopen the issue :)