From 7823ff968c252f581a3234a4802624fedddb7a95 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Tue, 18 Jul 2023 15:55:11 +0100 Subject: [PATCH 1/6] JavaScript: Improve query help for `js/server-side-unvalidated-url-redirection`. --- .../Security/CWE-601/ServerSideUrlRedirect.qhelp | 11 +++++++++++ .../CWE-601/examples/ServerSideUrlRedirect.js | 4 ++-- .../CWE-601/examples/ServerSideUrlRedirectGood.js | 9 ++++++--- .../examples/ServerSideUrlRedirectGood2.js | 15 +++++++++++++++ .../ServerSideUrlRedirect.expected | 5 +++++ .../ServerSideUrlRedirect.js | 6 ++++++ .../ServerSideUrlRedirectGood.js | 13 +++++++++++++ .../ServerSideUrlRedirectGood2.js | 15 +++++++++++++++ 8 files changed, 73 insertions(+), 5 deletions(-) create mode 100644 javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood2.js create mode 100644 javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.js create mode 100644 javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood.js create mode 100644 javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood2.js diff --git a/javascript/ql/src/Security/CWE-601/ServerSideUrlRedirect.qhelp b/javascript/ql/src/Security/CWE-601/ServerSideUrlRedirect.qhelp index 2052f16146bc..2faa5f8f9d72 100644 --- a/javascript/ql/src/Security/CWE-601/ServerSideUrlRedirect.qhelp +++ b/javascript/ql/src/Security/CWE-601/ServerSideUrlRedirect.qhelp @@ -16,6 +16,10 @@ To guard against untrusted URL redirection, it is advisable to avoid putting use directly into a redirect URL. Instead, maintain a list of authorized redirects on the server; then choose from that list based on the user input provided.

+

+If this is not possible, then the user input should be validated in some other way, +for example by verifying that the target URL is on the same host as the current page. +

@@ -32,6 +36,13 @@ before doing the redirection:

+ +

+Alternatively, we can check that the target URL does not redirect to a different host: +

+ + + diff --git a/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirect.js b/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirect.js index 0aa4eff9a8f8..d2ae7511c932 100644 --- a/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirect.js +++ b/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirect.js @@ -1,6 +1,6 @@ const app = require("express")(); -app.get('/some/path', function(req, res) { +app.get('/redirect', function(req, res) { // BAD: a request parameter is incorporated without validation into a URL redirect - res.redirect(req.param("target")); + res.redirect(req.params["target"]); }); diff --git a/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood.js b/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood.js index d2f2a95ef680..8903ddf09fae 100644 --- a/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood.js +++ b/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood.js @@ -2,9 +2,12 @@ const app = require("express")(); const VALID_REDIRECT = "http://cwe.mitre.org/data/definitions/601.html"; -app.get('/some/path', function(req, res) { +app.get('/redirect', function(req, res) { // GOOD: the request parameter is validated against a known fixed string - let target = req.param("target"); - if (VALID_REDIRECT === target) + let target = req.params["target"] + if (VALID_REDIRECT === target) { res.redirect(target); + } else { + res.redirect("/"); + } }); diff --git a/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood2.js b/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood2.js new file mode 100644 index 000000000000..5eaedd0aac58 --- /dev/null +++ b/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood2.js @@ -0,0 +1,15 @@ +const app = require("express")(); + +function isLocalUrl(url) { + return url.startsWith("/") && !url.startsWith("//") && !url.startsWith("/\\"); +} + +app.get('/redirect', function(req, res) { + // GOOD: check that we don't redirect to a different host + let target = req.params["target"]; + if (isLocalUrl(target)) { + res.redirect(target); + } else { + res.redirect("/"); + } +}); \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.expected b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.expected index e0ef0bfaae84..2883cd3fdac0 100644 --- a/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.expected +++ b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.expected @@ -1,4 +1,7 @@ nodes +| ServerSideUrlRedirect.js:5:16:5:35 | req.params["target"] | +| ServerSideUrlRedirect.js:5:16:5:35 | req.params["target"] | +| ServerSideUrlRedirect.js:5:16:5:35 | req.params["target"] | | express.js:7:16:7:34 | req.param("target") | | express.js:7:16:7:34 | req.param("target") | | express.js:7:16:7:34 | req.param("target") | @@ -114,6 +117,7 @@ nodes | react-native.js:9:26:9:32 | tainted | | react-native.js:9:26:9:32 | tainted | edges +| ServerSideUrlRedirect.js:5:16:5:35 | req.params["target"] | ServerSideUrlRedirect.js:5:16:5:35 | req.params["target"] | | express.js:7:16:7:34 | req.param("target") | express.js:7:16:7:34 | req.param("target") | | express.js:12:26:12:44 | req.param("target") | express.js:12:26:12:44 | req.param("target") | | express.js:27:7:27:34 | target | express.js:33:18:33:23 | target | @@ -211,6 +215,7 @@ edges | react-native.js:7:17:7:33 | req.param("code") | react-native.js:7:7:7:33 | tainted | | react-native.js:7:17:7:33 | req.param("code") | react-native.js:7:7:7:33 | tainted | #select +| ServerSideUrlRedirect.js:5:16:5:35 | req.params["target"] | ServerSideUrlRedirect.js:5:16:5:35 | req.params["target"] | ServerSideUrlRedirect.js:5:16:5:35 | req.params["target"] | Untrusted URL redirection depends on a $@. | ServerSideUrlRedirect.js:5:16:5:35 | req.params["target"] | user-provided value | | express.js:7:16:7:34 | req.param("target") | express.js:7:16:7:34 | req.param("target") | express.js:7:16:7:34 | req.param("target") | Untrusted URL redirection depends on a $@. | express.js:7:16:7:34 | req.param("target") | user-provided value | | express.js:12:26:12:44 | req.param("target") | express.js:12:26:12:44 | req.param("target") | express.js:12:26:12:44 | req.param("target") | Untrusted URL redirection depends on a $@. | express.js:12:26:12:44 | req.param("target") | user-provided value | | express.js:33:18:33:23 | target | express.js:27:16:27:34 | req.param("target") | express.js:33:18:33:23 | target | Untrusted URL redirection depends on a $@. | express.js:27:16:27:34 | req.param("target") | user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.js b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.js new file mode 100644 index 000000000000..d2ae7511c932 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.js @@ -0,0 +1,6 @@ +const app = require("express")(); + +app.get('/redirect', function(req, res) { + // BAD: a request parameter is incorporated without validation into a URL redirect + res.redirect(req.params["target"]); +}); diff --git a/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood.js b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood.js new file mode 100644 index 000000000000..8903ddf09fae --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood.js @@ -0,0 +1,13 @@ +const app = require("express")(); + +const VALID_REDIRECT = "http://cwe.mitre.org/data/definitions/601.html"; + +app.get('/redirect', function(req, res) { + // GOOD: the request parameter is validated against a known fixed string + let target = req.params["target"] + if (VALID_REDIRECT === target) { + res.redirect(target); + } else { + res.redirect("/"); + } +}); diff --git a/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood2.js b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood2.js new file mode 100644 index 000000000000..5eaedd0aac58 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood2.js @@ -0,0 +1,15 @@ +const app = require("express")(); + +function isLocalUrl(url) { + return url.startsWith("/") && !url.startsWith("//") && !url.startsWith("/\\"); +} + +app.get('/redirect', function(req, res) { + // GOOD: check that we don't redirect to a different host + let target = req.params["target"]; + if (isLocalUrl(target)) { + res.redirect(target); + } else { + res.redirect("/"); + } +}); \ No newline at end of file From 87364137df810ab89c2c0dc2173c1d8f99918de5 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Fri, 18 Aug 2023 14:43:57 +0100 Subject: [PATCH 2/6] Use more sensible validator in example. --- .../CWE-601/examples/ServerSideUrlRedirect.js | 4 ++-- .../CWE-601/examples/ServerSideUrlRedirectGood.js | 4 ++-- .../CWE-601/examples/ServerSideUrlRedirectGood2.js | 12 ++++++------ .../ServerSideUrlRedirect.expected | 10 +++++----- .../ServerSideUrlRedirect/ServerSideUrlRedirect.js | 4 ++-- .../ServerSideUrlRedirectGood.js | 4 ++-- .../ServerSideUrlRedirectGood2.js | 12 ++++++------ 7 files changed, 25 insertions(+), 25 deletions(-) diff --git a/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirect.js b/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirect.js index d2ae7511c932..38538fc0f964 100644 --- a/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirect.js +++ b/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirect.js @@ -1,6 +1,6 @@ const app = require("express")(); -app.get('/redirect', function(req, res) { +app.get("/redirect", function (req, res) { // BAD: a request parameter is incorporated without validation into a URL redirect - res.redirect(req.params["target"]); + res.redirect(req.query["target"]); }); diff --git a/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood.js b/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood.js index 8903ddf09fae..179f28ccb0bd 100644 --- a/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood.js +++ b/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood.js @@ -2,9 +2,9 @@ const app = require("express")(); const VALID_REDIRECT = "http://cwe.mitre.org/data/definitions/601.html"; -app.get('/redirect', function(req, res) { +app.get("/redirect", function (req, res) { // GOOD: the request parameter is validated against a known fixed string - let target = req.params["target"] + let target = req.query["target"]; if (VALID_REDIRECT === target) { res.redirect(target); } else { diff --git a/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood2.js b/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood2.js index 5eaedd0aac58..78b3be6c260f 100644 --- a/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood2.js +++ b/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood2.js @@ -1,15 +1,15 @@ const app = require("express")(); -function isLocalUrl(url) { - return url.startsWith("/") && !url.startsWith("//") && !url.startsWith("/\\"); +function isRelativePath(path) { + return !/^(\w+:)?[/\\]{2}/.test(path); } -app.get('/redirect', function(req, res) { +app.get("/redirect", function (req, res) { // GOOD: check that we don't redirect to a different host - let target = req.params["target"]; - if (isLocalUrl(target)) { + let target = req.query["target"]; + if (isRelativePath(target)) { res.redirect(target); } else { res.redirect("/"); } -}); \ No newline at end of file +}); diff --git a/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.expected b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.expected index 2883cd3fdac0..c03f57e7dd5c 100644 --- a/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.expected +++ b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.expected @@ -1,7 +1,7 @@ nodes -| ServerSideUrlRedirect.js:5:16:5:35 | req.params["target"] | -| ServerSideUrlRedirect.js:5:16:5:35 | req.params["target"] | -| ServerSideUrlRedirect.js:5:16:5:35 | req.params["target"] | +| ServerSideUrlRedirect.js:5:16:5:34 | req.query["target"] | +| ServerSideUrlRedirect.js:5:16:5:34 | req.query["target"] | +| ServerSideUrlRedirect.js:5:16:5:34 | req.query["target"] | | express.js:7:16:7:34 | req.param("target") | | express.js:7:16:7:34 | req.param("target") | | express.js:7:16:7:34 | req.param("target") | @@ -117,7 +117,7 @@ nodes | react-native.js:9:26:9:32 | tainted | | react-native.js:9:26:9:32 | tainted | edges -| ServerSideUrlRedirect.js:5:16:5:35 | req.params["target"] | ServerSideUrlRedirect.js:5:16:5:35 | req.params["target"] | +| ServerSideUrlRedirect.js:5:16:5:34 | req.query["target"] | ServerSideUrlRedirect.js:5:16:5:34 | req.query["target"] | | express.js:7:16:7:34 | req.param("target") | express.js:7:16:7:34 | req.param("target") | | express.js:12:26:12:44 | req.param("target") | express.js:12:26:12:44 | req.param("target") | | express.js:27:7:27:34 | target | express.js:33:18:33:23 | target | @@ -215,7 +215,7 @@ edges | react-native.js:7:17:7:33 | req.param("code") | react-native.js:7:7:7:33 | tainted | | react-native.js:7:17:7:33 | req.param("code") | react-native.js:7:7:7:33 | tainted | #select -| ServerSideUrlRedirect.js:5:16:5:35 | req.params["target"] | ServerSideUrlRedirect.js:5:16:5:35 | req.params["target"] | ServerSideUrlRedirect.js:5:16:5:35 | req.params["target"] | Untrusted URL redirection depends on a $@. | ServerSideUrlRedirect.js:5:16:5:35 | req.params["target"] | user-provided value | +| ServerSideUrlRedirect.js:5:16:5:34 | req.query["target"] | ServerSideUrlRedirect.js:5:16:5:34 | req.query["target"] | ServerSideUrlRedirect.js:5:16:5:34 | req.query["target"] | Untrusted URL redirection depends on a $@. | ServerSideUrlRedirect.js:5:16:5:34 | req.query["target"] | user-provided value | | express.js:7:16:7:34 | req.param("target") | express.js:7:16:7:34 | req.param("target") | express.js:7:16:7:34 | req.param("target") | Untrusted URL redirection depends on a $@. | express.js:7:16:7:34 | req.param("target") | user-provided value | | express.js:12:26:12:44 | req.param("target") | express.js:12:26:12:44 | req.param("target") | express.js:12:26:12:44 | req.param("target") | Untrusted URL redirection depends on a $@. | express.js:12:26:12:44 | req.param("target") | user-provided value | | express.js:33:18:33:23 | target | express.js:27:16:27:34 | req.param("target") | express.js:33:18:33:23 | target | Untrusted URL redirection depends on a $@. | express.js:27:16:27:34 | req.param("target") | user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.js b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.js index d2ae7511c932..38538fc0f964 100644 --- a/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.js +++ b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.js @@ -1,6 +1,6 @@ const app = require("express")(); -app.get('/redirect', function(req, res) { +app.get("/redirect", function (req, res) { // BAD: a request parameter is incorporated without validation into a URL redirect - res.redirect(req.params["target"]); + res.redirect(req.query["target"]); }); diff --git a/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood.js b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood.js index 8903ddf09fae..179f28ccb0bd 100644 --- a/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood.js +++ b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood.js @@ -2,9 +2,9 @@ const app = require("express")(); const VALID_REDIRECT = "http://cwe.mitre.org/data/definitions/601.html"; -app.get('/redirect', function(req, res) { +app.get("/redirect", function (req, res) { // GOOD: the request parameter is validated against a known fixed string - let target = req.params["target"] + let target = req.query["target"]; if (VALID_REDIRECT === target) { res.redirect(target); } else { diff --git a/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood2.js b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood2.js index 5eaedd0aac58..78b3be6c260f 100644 --- a/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood2.js +++ b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood2.js @@ -1,15 +1,15 @@ const app = require("express")(); -function isLocalUrl(url) { - return url.startsWith("/") && !url.startsWith("//") && !url.startsWith("/\\"); +function isRelativePath(path) { + return !/^(\w+:)?[/\\]{2}/.test(path); } -app.get('/redirect', function(req, res) { +app.get("/redirect", function (req, res) { // GOOD: check that we don't redirect to a different host - let target = req.params["target"]; - if (isLocalUrl(target)) { + let target = req.query["target"]; + if (isRelativePath(target)) { res.redirect(target); } else { res.redirect("/"); } -}); \ No newline at end of file +}); From a02f373e79fb3ff3d9be7d6865266c28b1855c3c Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Wed, 6 Sep 2023 14:06:16 +0100 Subject: [PATCH 3/6] Use better sanitiser. --- .../CWE-601/examples/ServerSideUrlRedirectGood2.js | 12 +++++++++--- .../ServerSideUrlRedirectGood2.js | 12 +++++++++--- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood2.js b/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood2.js index 78b3be6c260f..a36721a3717c 100644 --- a/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood2.js +++ b/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood2.js @@ -1,13 +1,19 @@ const app = require("express")(); -function isRelativePath(path) { - return !/^(\w+:)?[/\\]{2}/.test(path); +function isLocalUrl(path) { + try { + return ( + new URL(path, "https://example.com").origin === "https://example.com" + ); + } catch (e) { + return false; + } } app.get("/redirect", function (req, res) { // GOOD: check that we don't redirect to a different host let target = req.query["target"]; - if (isRelativePath(target)) { + if (isLocalUrl(target)) { res.redirect(target); } else { res.redirect("/"); diff --git a/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood2.js b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood2.js index 78b3be6c260f..a36721a3717c 100644 --- a/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood2.js +++ b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood2.js @@ -1,13 +1,19 @@ const app = require("express")(); -function isRelativePath(path) { - return !/^(\w+:)?[/\\]{2}/.test(path); +function isLocalUrl(path) { + try { + return ( + new URL(path, "https://example.com").origin === "https://example.com" + ); + } catch (e) { + return false; + } } app.get("/redirect", function (req, res) { // GOOD: check that we don't redirect to a different host let target = req.query["target"]; - if (isRelativePath(target)) { + if (isLocalUrl(target)) { res.redirect(target); } else { res.redirect("/"); From 46d7165885da3b80c7d4b45dc29d7069be3e0a38 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Thu, 7 Sep 2023 09:12:07 +0100 Subject: [PATCH 4/6] Explain about redirects to example.com. --- .../src/Security/CWE-601/ServerSideUrlRedirect.qhelp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/javascript/ql/src/Security/CWE-601/ServerSideUrlRedirect.qhelp b/javascript/ql/src/Security/CWE-601/ServerSideUrlRedirect.qhelp index 2faa5f8f9d72..b2217d357d12 100644 --- a/javascript/ql/src/Security/CWE-601/ServerSideUrlRedirect.qhelp +++ b/javascript/ql/src/Security/CWE-601/ServerSideUrlRedirect.qhelp @@ -38,11 +38,19 @@ before doing the redirection:

-Alternatively, we can check that the target URL does not redirect to a different host: +Alternatively, we can check that the target URL does not redirect to a different host +by parsing it relative to a base URL with a known host and verifying that the host +stays the same:

+

+Note that as written, the above code will allow redirects to URLs on example.com, +which is harmless but perhaps not intended. Substitute your own domain name for +example.com to prevent this. +

+ From 7ddb7da65efdbc938064788c6a70f75df5995f28 Mon Sep 17 00:00:00 2001 From: Max Schaefer <54907921+max-schaefer@users.noreply.github.com> Date: Tue, 12 Sep 2023 16:47:23 +0100 Subject: [PATCH 5/6] Apply suggestions from code review Co-authored-by: Felicity Chapman --- javascript/ql/src/Security/CWE-601/ServerSideUrlRedirect.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/Security/CWE-601/ServerSideUrlRedirect.qhelp b/javascript/ql/src/Security/CWE-601/ServerSideUrlRedirect.qhelp index b2217d357d12..6c74b7b1a5da 100644 --- a/javascript/ql/src/Security/CWE-601/ServerSideUrlRedirect.qhelp +++ b/javascript/ql/src/Security/CWE-601/ServerSideUrlRedirect.qhelp @@ -18,7 +18,7 @@ redirects on the server; then choose from that list based on the user input prov

If this is not possible, then the user input should be validated in some other way, -for example by verifying that the target URL is on the same host as the current page. +for example, by verifying that the target URL is on the same host as the current page.

From a9e81672f039aead086bce412220d98727fb1adc Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Tue, 12 Sep 2023 16:53:36 +0100 Subject: [PATCH 6/6] Make suggestion to replace example.com more explicit. --- javascript/ql/src/Security/CWE-601/ServerSideUrlRedirect.qhelp | 2 +- .../src/Security/CWE-601/examples/ServerSideUrlRedirectGood2.js | 1 + .../CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood2.js | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/javascript/ql/src/Security/CWE-601/ServerSideUrlRedirect.qhelp b/javascript/ql/src/Security/CWE-601/ServerSideUrlRedirect.qhelp index 6c74b7b1a5da..9a4d7d1c0b7c 100644 --- a/javascript/ql/src/Security/CWE-601/ServerSideUrlRedirect.qhelp +++ b/javascript/ql/src/Security/CWE-601/ServerSideUrlRedirect.qhelp @@ -47,7 +47,7 @@ stays the same:

Note that as written, the above code will allow redirects to URLs on example.com, -which is harmless but perhaps not intended. Substitute your own domain name for +which is harmless but perhaps not intended. You can substitute your own domain (if known) for example.com to prevent this.

diff --git a/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood2.js b/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood2.js index a36721a3717c..54ac26811b31 100644 --- a/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood2.js +++ b/javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood2.js @@ -3,6 +3,7 @@ const app = require("express")(); function isLocalUrl(path) { try { return ( + // TODO: consider substituting your own domain for example.com new URL(path, "https://example.com").origin === "https://example.com" ); } catch (e) { diff --git a/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood2.js b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood2.js index a36721a3717c..54ac26811b31 100644 --- a/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood2.js +++ b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood2.js @@ -3,6 +3,7 @@ const app = require("express")(); function isLocalUrl(path) { try { return ( + // TODO: consider substituting your own domain for example.com new URL(path, "https://example.com").origin === "https://example.com" ); } catch (e) {