From deb715a5177976063033534966d48fee0475597b Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Fri, 13 Jun 2025 11:15:16 +0200 Subject: [PATCH 1/4] JS: Add test case with `encodeURI` for request forgery --- javascript/ql/test/query-tests/Security/CWE-918/serverSide.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/javascript/ql/test/query-tests/Security/CWE-918/serverSide.js b/javascript/ql/test/query-tests/Security/CWE-918/serverSide.js index aec8c4195c83..d204580d1ebd 100644 --- a/javascript/ql/test/query-tests/Security/CWE-918/serverSide.js +++ b/javascript/ql/test/query-tests/Security/CWE-918/serverSide.js @@ -141,4 +141,8 @@ var server2 = http.createServer(function(req, res) { axios.get(target.toString()); // $Alert[js/request-forgery] axios.get(target); // $Alert[js/request-forgery] axios.get(target.href); // $Alert[js/request-forgery] + const encodedUrl = encodeURI(input); + axios.get(encodedUrl); // $MISSING:Alert[js/request-forgery] + const escapedUrl = escape(input); + axios.get(escapedUrl); // $MISSING:Alert[js/request-forgery] }); From bdbc49c63f39c9a92a32ca3782f74e8017d5618e Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Fri, 13 Jun 2025 11:16:39 +0200 Subject: [PATCH 2/4] JS: Removed `encodeURI` from request forgery sanitizer list --- .../dataflow/RequestForgeryCustomizations.qll | 4 +++- .../javascript/security/dataflow/Xss.qll | 15 +++++++++++---- .../Security/CWE-918/RequestForgery.expected | 18 ++++++++++++++++++ .../query-tests/Security/CWE-918/serverSide.js | 4 ++-- 4 files changed, 34 insertions(+), 7 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/RequestForgeryCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/RequestForgeryCustomizations.qll index 8d182d116c6d..a9b0592696f9 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/RequestForgeryCustomizations.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/RequestForgeryCustomizations.qll @@ -111,5 +111,7 @@ module RequestForgery { * The result from calling `encodeURI` or `encodeURIComponent` is not a valid URL, and only makes sense * as a part of a URL. */ - class UriEncodingSanitizer extends Sanitizer instanceof Xss::Shared::UriEncodingSanitizer { } + class UriEncodingSanitizer extends Sanitizer instanceof Xss::Shared::UriEncodingSanitizer { + UriEncodingSanitizer() { this.encodesPathSeparators() } + } } diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/Xss.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/Xss.qll index 41487b8c3b64..f139ccc48dbb 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/Xss.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/Xss.qll @@ -47,15 +47,22 @@ module Shared { } /** - * A call to `encodeURI` or `encodeURIComponent`, viewed as a sanitizer for + * A call to `encodeURI`, `encodeURIComponent` or `escape`, viewed as a sanitizer for * XSS vulnerabilities. */ class UriEncodingSanitizer extends Sanitizer, DataFlow::CallNode { + string name; + UriEncodingSanitizer() { - exists(string name | this = DataFlow::globalVarRef(name).getACall() | - name in ["encodeURI", "encodeURIComponent", "escape"] - ) + this = DataFlow::globalVarRef(name).getACall() and + name in ["encodeURI", "encodeURIComponent", "escape"] } + + /** + * Holds if this URI encoding function properly encodes path separators, + * making it safe for request forgery prevention. + */ + predicate encodesPathSeparators() { name = "encodeURIComponent" } } /** diff --git a/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected b/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected index dde72095df16..58762ae23d70 100644 --- a/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected +++ b/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected @@ -33,6 +33,8 @@ | 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 | | 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 | | 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 | +| 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 | +| 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 | edges | 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 | | | 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 | serverSide.js:130:9:130:45 | myUrl | serverSide.js:131:15:131:19 | myUrl | provenance | | | serverSide.js:130:37:130:43 | tainted | serverSide.js:130:9:130:45 | myUrl | provenance | | | serverSide.js:139:9:139:29 | input | serverSide.js:140:26:140:30 | input | provenance | | +| serverSide.js:139:9:139:29 | input | serverSide.js:144:32:144:36 | input | provenance | | +| serverSide.js:139:9:139:29 | input | serverSide.js:146:29:146:33 | input | provenance | | | serverSide.js:139:17:139:29 | req.query.url | serverSide.js:139:9:139:29 | input | provenance | | | serverSide.js:140:9:140:31 | target | serverSide.js:141:13:141:18 | target | provenance | | | serverSide.js:140:9:140:31 | target | serverSide.js:142:13:142:18 | target | provenance | | @@ -118,6 +122,12 @@ edges | serverSide.js:140:26:140:30 | input | serverSide.js:140:18:140:31 | new URL(input) | provenance | Config | | serverSide.js:141:13:141:18 | target | serverSide.js:141:13:141:29 | target.toString() | provenance | | | serverSide.js:143:13:143:18 | target | serverSide.js:143:13:143:23 | target.href | provenance | | +| serverSide.js:144:9:144:37 | encodedUrl | serverSide.js:145:13:145:22 | encodedUrl | provenance | | +| serverSide.js:144:22:144:37 | encodeURI(input) | serverSide.js:144:9:144:37 | encodedUrl | provenance | | +| serverSide.js:144:32:144:36 | input | serverSide.js:144:22:144:37 | encodeURI(input) | provenance | | +| serverSide.js:146:9:146:34 | escapedUrl | serverSide.js:147:13:147:22 | escapedUrl | provenance | | +| serverSide.js:146:22:146:34 | escape(input) | serverSide.js:146:9:146:34 | escapedUrl | provenance | | +| serverSide.js:146:29:146:33 | input | serverSide.js:146:22:146:34 | escape(input) | provenance | | nodes | Request/app/api/proxy/route2.serverSide.ts:4:9:4:15 | { url } | semmle.label | { url } | | Request/app/api/proxy/route2.serverSide.ts:4:9:4:34 | url | semmle.label | url | @@ -221,4 +231,12 @@ nodes | serverSide.js:142:13:142:18 | target | semmle.label | target | | serverSide.js:143:13:143:18 | target | semmle.label | target | | serverSide.js:143:13:143:23 | target.href | semmle.label | target.href | +| serverSide.js:144:9:144:37 | encodedUrl | semmle.label | encodedUrl | +| serverSide.js:144:22:144:37 | encodeURI(input) | semmle.label | encodeURI(input) | +| serverSide.js:144:32:144:36 | input | semmle.label | input | +| serverSide.js:145:13:145:22 | encodedUrl | semmle.label | encodedUrl | +| serverSide.js:146:9:146:34 | escapedUrl | semmle.label | escapedUrl | +| serverSide.js:146:22:146:34 | escape(input) | semmle.label | escape(input) | +| serverSide.js:146:29:146:33 | input | semmle.label | input | +| serverSide.js:147:13:147:22 | escapedUrl | semmle.label | escapedUrl | subpaths diff --git a/javascript/ql/test/query-tests/Security/CWE-918/serverSide.js b/javascript/ql/test/query-tests/Security/CWE-918/serverSide.js index d204580d1ebd..c578b268e400 100644 --- a/javascript/ql/test/query-tests/Security/CWE-918/serverSide.js +++ b/javascript/ql/test/query-tests/Security/CWE-918/serverSide.js @@ -142,7 +142,7 @@ var server2 = http.createServer(function(req, res) { axios.get(target); // $Alert[js/request-forgery] axios.get(target.href); // $Alert[js/request-forgery] const encodedUrl = encodeURI(input); - axios.get(encodedUrl); // $MISSING:Alert[js/request-forgery] + axios.get(encodedUrl); // $Alert[js/request-forgery] const escapedUrl = escape(input); - axios.get(escapedUrl); // $MISSING:Alert[js/request-forgery] + axios.get(escapedUrl); // $Alert[js/request-forgery] }); From 798721bd714981d0f5df3b4549dfc77dea43f706 Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Fri, 13 Jun 2025 10:39:18 +0200 Subject: [PATCH 3/4] JS: add change note --- javascript/ql/lib/change-notes/2025-06-13-remove-encodeuri.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 javascript/ql/lib/change-notes/2025-06-13-remove-encodeuri.md diff --git a/javascript/ql/lib/change-notes/2025-06-13-remove-encodeuri.md b/javascript/ql/lib/change-notes/2025-06-13-remove-encodeuri.md new file mode 100644 index 000000000000..ab91e9905af6 --- /dev/null +++ b/javascript/ql/lib/change-notes/2025-06-13-remove-encodeuri.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Removed `encodeURI` and `escape` functions from the sanitizer list for request forgery. From 0d5f5104d1ffab850c01df40989c220515c927df Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Mon, 16 Jun 2025 10:54:12 +0200 Subject: [PATCH 4/4] Updated `UriEncodingSanitizer` comment --- .../security/dataflow/RequestForgeryCustomizations.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/RequestForgeryCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/RequestForgeryCustomizations.qll index a9b0592696f9..ec46ef91c3c0 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/RequestForgeryCustomizations.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/RequestForgeryCustomizations.qll @@ -106,9 +106,9 @@ module RequestForgery { private import Xss as Xss /** - * A call to `encodeURI` or `encodeURIComponent`, viewed as a sanitizer for request forgery. + * A call to `encodeURIComponent`, viewed as a sanitizer for request forgery. * These calls will escape "/" to "%2F", which is not a problem for request forgery. - * The result from calling `encodeURI` or `encodeURIComponent` is not a valid URL, and only makes sense + * The result from calling `encodeURIComponent` is not a valid URL, and only makes sense * as a part of a URL. */ class UriEncodingSanitizer extends Sanitizer instanceof Xss::Shared::UriEncodingSanitizer {