Conversation
| } | ||
|
|
||
| // Reconstruct URL with proper encoding to prevent command injection | ||
| // The URL constructor doesn't automatically encode special characters like | in query strings, |
There was a problem hiding this comment.
To be specific, it encodes special characters, but only sets of them in each URL part 1. For example, | is encoded in userinfo:
new URL('https://user|:pass@example.com')`
// https://user%7C:pass@example.com/Current implementation double-encodes several characters for that reason; for example, whitespaces:
const parsedUrl = new URL('https://example.com/?#some hash')
// https://example.com/?#some%20hash
const sanitizedUrl = new URL(parsedUrl.origin);
// ...
console.log(sanitizedUrl.href)
// https://example.com/#some%2520hashA simpler approach could be:
const sanitizedUrl = encodeURI(url);Footnotes
|
For posterity: this is likely still fragile, but better than it was. On a side note, this can (still) be exploited to exfiltrate some environment variables; possibilities are more limited, though. For example, |
|
@thymikee hi, |
|
Uh, I wanted to followup with a more robust fix, but then forgot about it. I'll try to prioritize this soon. Maintaining Community CLI is not my primary focus and anyone is free to contribute |
|
@thymikee Opened #2758 as an alternative. Uses strict-url-sanitise and continues to cover logic with our own unit tests. |
|
Thank you @huntie, let's move the discussion there! |
6 participants
Summary
Continuation of the fix that landed in 1508990, that prevents RCE using a spoofed URL with
|character, such as: https://evil.com?|calc.exe.cc @633kh4ck @mbaraniak-exodus