Skip to content

Conversation

max-schaefer
Copy link
Contributor

@max-schaefer max-schaefer added the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Feb 25, 2021
@max-schaefer max-schaefer requested a review from a team as a code owner February 25, 2021 09:14
@github-actions github-actions bot added the JS label Feb 25, 2021
@max-schaefer
Copy link
Contributor Author

This was found by TSM 🎉 (/cc @garbervetsky)

@@ -0,0 +1,3 @@
document.getElementById('my-id').onclick = function() {
this.parentNode.innerHTML = '<h2><a href="' + location.href + '">A link</a></h2>'; // NOT OK
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how exploitable this code pattern would be, considering that location.href is URL-encoded, but of course that's nothing to do with this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I recall there was an issue with IE not encoding the URL when redirected via a Location header, so I'm fine with flagging it.

Copy link
Contributor

Choose a reason for hiding this comment

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

And maybe add a comment about not using getABoundFunctionValue exactly because bound functions will tend to have a different receiver anyway.

or
// A receiver node of an event handler on a DOM node
exists(string handler | handler.matches("on%") |
this = domValueRef().getAPropertySource(handler).(DataFlow::FunctionNode).getReceiver()
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we support addEventListener here?

@@ -0,0 +1,3 @@
document.getElementById('my-id').onclick = function() {
this.parentNode.innerHTML = '<h2><a href="' + location.href + '">A link</a></h2>'; // NOT OK
Copy link
Contributor

Choose a reason for hiding this comment

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

I recall there was an issue with IE not encoding the URL when redirected via a Location header, so I'm fine with flagging it.

@max-schaefer
Copy link
Contributor Author

Thanks, @asgerf! Comments addressed and change note added.

Copy link
Contributor

@asgerf asgerf left a comment

Choose a reason for hiding this comment

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

LGTM. Hope the evaluation works out

@max-schaefer
Copy link
Contributor Author

It did (internal link). Sorry for the delay, and many thanks to @erik-krogh for helping with finally getting the evaluation done!

@asgerf asgerf removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Mar 5, 2021
@codeql-ci codeql-ci merged commit d7b9251 into github:main Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants