Sanitize CI #385
Sanitize CI #385
Conversation
|
@dan-mm all tests were passing, but there was an issue: |
Aye, its a bug I introduced in v2 of the eco-ci, looking at it now |
|
frontend/js/ci.js
Outdated
| } | ||
| else if(badges_data.data[0][8] == 'gitlab') { | ||
| repo_link = `https://gitlab.com/${url_params.get('repo')}`; | ||
| repo_link = `https://gitlab.com/${sanitize(url_params.get('repo'))}`; | ||
| } | ||
| //${repo_link} | ||
| const repo_link_node = `<a href="${repo_link}" target="_blank">${url_params.get('repo')}</a>` |
There was a problem hiding this comment.
why is url_params.get('repo') not sanitzed here?
There was a problem hiding this comment.
Missed. Fixed in last commit
| @@ -5,7 +5,7 @@ const compareButton = () => { | |||
| checkedBoxes.forEach(checkbox => { | |||
| link = `${link}${checkbox.value},`; | |||
| }); | |||
| window.location = link.substr(0,link.length-1); | |||
| window.location = encodeURIComponent(link.substr(0, link.length - 1)); | |||
There was a problem hiding this comment.
What is this supposed to do? Which part of the checkboxes is supposed to be malicious?
There was a problem hiding this comment.
I don't honestly understand the vulnerability here, but this was the recommendation on how to resolve it.
There was a problem hiding this comment.
It was a ChatGPT suggestion:
To fix the warning about DOM text being reinterpreted as HTML without escaping meta-characters, you need to properly escape the text before assigning it to window.location. You can use the encodeURIComponent function to achieve this. Here's an updated version of the code:
window.location = encodeURIComponent(link.substr(0, link.length - 1));By using encodeURIComponent, you ensure that any special characters in the link string are properly encoded, preventing any unintended interpretation as HTML meta-characters.
However, please note that directly manipulating window.location like this can have security implications. It's important to ensure that the link variable contains a trusted URL and to validate it before using it in this context.
|
escape sounds better to me. fwiw, I already encode some of these strings when the eco-ci sends them to our API to begin with - but its good to check them on this end anyways in case that ever changes / they ever come in from a different source. |
|
Work has been ported to #411 |
Code scanning alerts do have a # but they can't be referenced if they are not issues, so here's what this fixes:
encodeURIComponent()sanitize()js function inhelpers/main.jsthat escapes < > & ' "