URI encode template replacement to avoid XSS #38
Conversation
authn/pkce.index.js
Outdated
| page = page.replace(/%error_description%/g, error_description); | ||
| page = page.replace(/%error_uri%/g, error_uri); | ||
| page = page.replace(/%error%/g, encodeURI(error).replace(/%20/g,' ')); | ||
| page = page.replace(/%error_description%/g, encodeURI(error_description).replace(/%20/g,' ')); |
There was a problem hiding this comment.
This may make the supplied string "safe" but I don't think URI encoding is what e are going to want. encodeURI is designed to encode urls not strings shown as HTML. I think we will need some form of HTML Encoding instead.
For example when encoding the string <script>alert('boo & hiss');</script> the encodeURI results in %3Cscript%3Ealert('boo%20&%20hiss');%3C/script%3E.
Where as I think we would want it to be displayed as <script>alert('boo & hiss');</script>
I don't think there is a native function for HTML encoding in JavaScript, but some searching will confirm or deny a soln there.
There was a problem hiding this comment.
I think you're right, HTML encoding will be better than URI encoding for this text.
I'll look at moving it over, and probably also update it in the openid.index.js file as well to align them.
There was a problem hiding this comment.
I've installed and used the entities npm package to do HTML encoding instead of URI for both files.
There was a problem hiding this comment.
the entites package looks good from a health pov but I notice it has a BSD-2-Clause licence. Is that licence compatible with this repo?
There was a problem hiding this comment.
Looks like the size of the package listed in the action results is under the required 1MB
There was a problem hiding this comment.
the
entitespackage looks good from a health pov but I notice it has a BSD-2-Clause licence. Is that licence compatible with this repo?
@kristens-work do you know, or know anyone who's able to advise on use of a BSD 2 Clause licenced dependencies within our open-source repositories?
There was a problem hiding this comment.
I tend to seek advice from Rebecca Kelly on opensource licencing
There was a problem hiding this comment.
the
entitespackage looks good from a health pov but I notice it has a BSD-2-Clause licence. Is that licence compatible with this repo?
I've replaced the dependency on the BSD-2-Clause entities package with a dependency on the MIT html-entities package.
There was a problem hiding this comment.
I'm happy that resolves the licensing question to no detriment to the code base.
SiCoe
changed the title
This is due to licensing consern. 'html-entities' is distributed with the MIT license .
A pentest has highlighted where we need to encode user input to avoid potential XXS attacks.
I think I've narrowed it down to this repo and this change should mitigate the risk.
I looks like this has already been done for the
openid.index.js, so I followed the same implementation inpkce.index.js