Skip to content

Commit f80651e

Browse files
NapalysNapalys
authored
Merge pull request #19750 from Napalys/js/remove_encodeURI
JS: remove `encodeURI` from sanitizer list of request forgery
2 parents 3f3a920 + 0d5f510 commit f80651e

File tree

5 files changed

+42
-7
lines changed

5 files changed

+42
-7
lines changed

‎javascript/ql/lib/change-notes/2025-06-13-remove-encodeuri.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Removed `encodeURI` and `escape` functions from the sanitizer list for request forgery.

‎javascript/ql/lib/semmle/javascript/security/dataflow/RequestForgeryCustomizations.qll

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -106,10 +106,12 @@ module RequestForgery {
106106
private import Xss as Xss
107107

108108
/**
109-
* A call to `encodeURI` or `encodeURIComponent`, viewed as a sanitizer for request forgery.
109+
* A call to `encodeURIComponent`, viewed as a sanitizer for request forgery.
110110
* These calls will escape "/" to "%2F", which is not a problem for request forgery.
111-
* The result from calling `encodeURI` or `encodeURIComponent` is not a valid URL, and only makes sense
111+
* The result from calling `encodeURIComponent` is not a valid URL, and only makes sense
112112
* as a part of a URL.
113113
*/
114-
class UriEncodingSanitizer extends Sanitizer instanceof Xss::Shared::UriEncodingSanitizer { }
114+
class UriEncodingSanitizer extends Sanitizer instanceof Xss::Shared::UriEncodingSanitizer {
115+
UriEncodingSanitizer() { this.encodesPathSeparators() }
116+
}
115117
}

‎javascript/ql/lib/semmle/javascript/security/dataflow/Xss.qll

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -47,15 +47,22 @@ module Shared {
4747
}
4848

4949
/**
50-
* A call to `encodeURI` or `encodeURIComponent`, viewed as a sanitizer for
50+
* A call to `encodeURI`, `encodeURIComponent` or `escape`, viewed as a sanitizer for
5151
* XSS vulnerabilities.
5252
*/
5353
class UriEncodingSanitizer extends Sanitizer, DataFlow::CallNode {
54+
string name;
55+
5456
UriEncodingSanitizer() {
55-
exists(string name | this = DataFlow::globalVarRef(name).getACall() |
56-
name in ["encodeURI", "encodeURIComponent", "escape"]
57-
)
57+
this = DataFlow::globalVarRef(name).getACall() and
58+
name in ["encodeURI", "encodeURIComponent", "escape"]
5859
}
60+
61+
/**
62+
* Holds if this URI encoding function properly encodes path separators,
63+
* making it safe for request forgery prevention.
64+
*/
65+
predicate encodesPathSeparators() { name = "encodeURIComponent" }
5966
}
6067

6168
/**

‎javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@
3333
| serverSide.js:141:3:141:30 | axios.g ... ring()) | serverSide.js:139:17:139:29 | req.query.url | serverSide.js:141:13:141:29 | target.toString() | The $@ of this request depends on a $@. | serverSide.js:141:13:141:29 | target.toString() | URL | serverSide.js:139:17:139:29 | req.query.url | user-provided value |
3434
| serverSide.js:142:3:142:19 | axios.get(target) | serverSide.js:139:17:139:29 | req.query.url | serverSide.js:142:13:142:18 | target | The $@ of this request depends on a $@. | serverSide.js:142:13:142:18 | target | URL | serverSide.js:139:17:139:29 | req.query.url | user-provided value |
3535
| serverSide.js:143:3:143:24 | axios.g ... t.href) | serverSide.js:139:17:139:29 | req.query.url | serverSide.js:143:13:143:23 | target.href | The $@ of this request depends on a $@. | serverSide.js:143:13:143:23 | target.href | URL | serverSide.js:139:17:139:29 | req.query.url | user-provided value |
36+
| serverSide.js:145:3:145:23 | axios.g ... dedUrl) | serverSide.js:139:17:139:29 | req.query.url | serverSide.js:145:13:145:22 | encodedUrl | The $@ of this request depends on a $@. | serverSide.js:145:13:145:22 | encodedUrl | URL | serverSide.js:139:17:139:29 | req.query.url | user-provided value |
37+
| serverSide.js:147:3:147:23 | axios.g ... pedUrl) | serverSide.js:139:17:139:29 | req.query.url | serverSide.js:147:13:147:22 | escapedUrl | The $@ of this request depends on a $@. | serverSide.js:147:13:147:22 | escapedUrl | URL | serverSide.js:139:17:139:29 | req.query.url | user-provided value |
3638
edges
3739
| Request/app/api/proxy/route2.serverSide.ts:4:9:4:15 | { url } | Request/app/api/proxy/route2.serverSide.ts:4:9:4:34 | url | provenance | |
3840
| Request/app/api/proxy/route2.serverSide.ts:4:9:4:34 | url | Request/app/api/proxy/route2.serverSide.ts:5:27:5:29 | url | provenance | |
@@ -110,6 +112,8 @@ edges
110112
| serverSide.js:130:9:130:45 | myUrl | serverSide.js:131:15:131:19 | myUrl | provenance | |
111113
| serverSide.js:130:37:130:43 | tainted | serverSide.js:130:9:130:45 | myUrl | provenance | |
112114
| serverSide.js:139:9:139:29 | input | serverSide.js:140:26:140:30 | input | provenance | |
115+
| serverSide.js:139:9:139:29 | input | serverSide.js:144:32:144:36 | input | provenance | |
116+
| serverSide.js:139:9:139:29 | input | serverSide.js:146:29:146:33 | input | provenance | |
113117
| serverSide.js:139:17:139:29 | req.query.url | serverSide.js:139:9:139:29 | input | provenance | |
114118
| serverSide.js:140:9:140:31 | target | serverSide.js:141:13:141:18 | target | provenance | |
115119
| serverSide.js:140:9:140:31 | target | serverSide.js:142:13:142:18 | target | provenance | |
@@ -118,6 +122,12 @@ edges
118122
| serverSide.js:140:26:140:30 | input | serverSide.js:140:18:140:31 | new URL(input) | provenance | Config |
119123
| serverSide.js:141:13:141:18 | target | serverSide.js:141:13:141:29 | target.toString() | provenance | |
120124
| serverSide.js:143:13:143:18 | target | serverSide.js:143:13:143:23 | target.href | provenance | |
125+
| serverSide.js:144:9:144:37 | encodedUrl | serverSide.js:145:13:145:22 | encodedUrl | provenance | |
126+
| serverSide.js:144:22:144:37 | encodeURI(input) | serverSide.js:144:9:144:37 | encodedUrl | provenance | |
127+
| serverSide.js:144:32:144:36 | input | serverSide.js:144:22:144:37 | encodeURI(input) | provenance | |
128+
| serverSide.js:146:9:146:34 | escapedUrl | serverSide.js:147:13:147:22 | escapedUrl | provenance | |
129+
| serverSide.js:146:22:146:34 | escape(input) | serverSide.js:146:9:146:34 | escapedUrl | provenance | |
130+
| serverSide.js:146:29:146:33 | input | serverSide.js:146:22:146:34 | escape(input) | provenance | |
121131
nodes
122132
| Request/app/api/proxy/route2.serverSide.ts:4:9:4:15 | { url } | semmle.label | { url } |
123133
| Request/app/api/proxy/route2.serverSide.ts:4:9:4:34 | url | semmle.label | url |
@@ -221,4 +231,12 @@ nodes
221231
| serverSide.js:142:13:142:18 | target | semmle.label | target |
222232
| serverSide.js:143:13:143:18 | target | semmle.label | target |
223233
| serverSide.js:143:13:143:23 | target.href | semmle.label | target.href |
234+
| serverSide.js:144:9:144:37 | encodedUrl | semmle.label | encodedUrl |
235+
| serverSide.js:144:22:144:37 | encodeURI(input) | semmle.label | encodeURI(input) |
236+
| serverSide.js:144:32:144:36 | input | semmle.label | input |
237+
| serverSide.js:145:13:145:22 | encodedUrl | semmle.label | encodedUrl |
238+
| serverSide.js:146:9:146:34 | escapedUrl | semmle.label | escapedUrl |
239+
| serverSide.js:146:22:146:34 | escape(input) | semmle.label | escape(input) |
240+
| serverSide.js:146:29:146:33 | input | semmle.label | input |
241+
| serverSide.js:147:13:147:22 | escapedUrl | semmle.label | escapedUrl |
224242
subpaths

‎javascript/ql/test/query-tests/Security/CWE-918/serverSide.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,4 +141,8 @@ var server2 = http.createServer(function(req, res) {
141141
axios.get(target.toString()); // $Alert[js/request-forgery]
142142
axios.get(target); // $Alert[js/request-forgery]
143143
axios.get(target.href); // $Alert[js/request-forgery]
144+
const encodedUrl = encodeURI(input);
145+
axios.get(encodedUrl); // $Alert[js/request-forgery]
146+
const escapedUrl = escape(input);
147+
axios.get(escapedUrl); // $Alert[js/request-forgery]
144148
});

0 commit comments

Comments
 (0)