JavaScript: Add new query InvalidEntityTranscoding.
#556
Conversation
|
No particular rush to review this, it's not going into 1.19. |
| * A call to `String.prototype.replace` that replaces all instances of a pattern. | ||
| */ | ||
| class Replacement extends DataFlow::Node { | ||
| RegExpLiteral pattern; |
There was a problem hiding this comment.
I take it string patterns here will always be reported by IncompleteSanitization, and that's why we don't include them here? Maybe worth leaving a comment about this.
|
LGTM too, but I see two features that could go in a later PR. How about escapes with backslashes and the escaping of the backslash it self?
|
That's a very interesting idea; let me play with that a little bit. |
There was a problem hiding this comment.
@xiemaisi - this LGTM. I only made a very minor suggestion, which you can reject if you think it's OTT. Just let me know and I'll approve instead of requesting changes. Thanks!
| </p> | ||
|
|
||
| <p> | ||
| Instead, the decoding function should decode ampersand last: |
There was a problem hiding this comment.
Tiny suggestion:
Instead, the decoding function should decode THE ampersand last:
xiemaisi
force-pushed
the
js/invalid-entity-transcoding
branch
2 times, most recently
from
713a27d to
9fab6a9
Compare
|
I have significantly rewritten the query based on @esben-semmle's suggestion, generalising it beyond HTML transcoding to a few other kinds of (un-)escaping. I like the new results a lot, for instance this one from an old version of underscore. Full comparison is running. |
|
Results from full run look good as well: internal link. |
xiemaisi
force-pushed
the
js/invalid-entity-transcoding
branch
from
cf0020f to
10166be
Compare
|
Rebased to resolve conflict on change notes. Perhaps @mc-semmle wants to take another look at the query help, which has been rewritten to reflect the expanded scope of the query (but no rush; it's not relevant for 1.19). |
|
Documentation LGTM, I've approved the changes :) |
|
Thanks, @mc-semmle! I've resolved the conflict on the change notes, so this should be good to go in once it's green again. |
ghost
merged commit 
A simple, lightweight query that spots a common mistake people make when writing HTML entity encoders/decoders: when encoding,
&has to be encoded first to avoid double-encoding ampersands introduced by the encoding of other characters; conversely, when decoding it has to be decoded last to avoid the decoded ampersand being interpreted as part of an entity reference later on.Finds good results on LGTM.com, here is the full report (internal link). One of the results is in a moderately popular (but not very actively maintained) utility-belt library.
@esben-semmle suggested looking for a similar problem with URL encoding and
%, but a quick exploratory query seems to indicate that people don't often implement URL transcoding by hand.