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|:[email protected]')`
// https://user%7C:[email protected]/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! |
- revert d1b5e9c patch fix - bump @react-native-community/cli - bump @react-native-community/cli-platform-android - bump @react-native-community/cli-platform-ios Resolve: #1994 Supersedes - #1980 - #2012 - #2013 Ref - react-native-community/template#231 - react-native-community/template#232 - react/react-native#57344 Includes URL sanitisation fix (v20.1.1 regression) - react-native-community/cli#2814 - react-native-community/cli#2812 - react-native-community/cli#2758 - react-native-community/cli#2735 - react-native-community/cli#2697
- revert d1b5e9c patch fix - bump @react-native-community/cli - bump @react-native-community/cli-platform-android - bump @react-native-community/cli-platform-ios Resolve: #1994 Supersedes - #1980 - #2012 - #2013 Ref - react-native-community/template#231 - react-native-community/template#232 - react/react-native#57344 Includes URL sanitisation fix (v20.1.1 regression) - react-native-community/cli#2814 - react-native-community/cli#2812 - react-native-community/cli#2758 - react-native-community/cli#2735 - react-native-community/cli#2697
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