From 5e02a76dfd23f97491c6e3a62ae71d223824a7ef Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Tue, 10 May 2022 14:37:47 +0200 Subject: [PATCH] add support for typed NextJS route-handlers --- .../lib/semmle/javascript/frameworks/Next.qll | 39 ++++++++++++++++--- .../javascript/frameworks/NodeJSLib.qll | 6 +-- .../ServerSideUrlRedirect.expected | 9 +++++ .../CWE-601/ServerSideUrlRedirect/next.ts | 12 ++++++ .../ServerSideUrlRedirect/tsconfig.json | 3 ++ 5 files changed, 60 insertions(+), 9 deletions(-) create mode 100644 javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/next.ts create mode 100644 javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/tsconfig.json diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Next.qll b/javascript/ql/lib/semmle/javascript/frameworks/Next.qll index 254901c1d7b..644754ce75e 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/Next.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/Next.qll @@ -158,13 +158,37 @@ module NextJS { NextHttpRouteHandler() { this = getServerSidePropsFunction(_) or this = getInitialProps(_) } } + /** + * A function that handles both a request and response from Next.js, seen as a routehandler. + */ + class NextReqResHandler extends HTTP::Servers::StandardRouteHandler, DataFlow::FunctionNode { + DataFlow::ParameterNode req; + DataFlow::ParameterNode res; + + NextReqResHandler() { + res = this.getAParameter() and + req = this.getAParameter() and + req.hasUnderlyingType("next", "NextApiRequest") and + res.hasUnderlyingType("next", "NextApiResponse") + } + + /** Gets the request parameter */ + DataFlow::ParameterNode getRequest() { result = req } + + /** Gets the response parameter */ + DataFlow::ParameterNode getResponse() { result = res } + } + /** * A NodeJS HTTP request object in a Next.js page. */ class NextHttpRequestSource extends NodeJSLib::RequestSource { - NextHttpRouteHandler rh; + HTTP::RouteHandler rh; - NextHttpRequestSource() { this = rh.getParameter(0).getAPropertyRead("req") } + NextHttpRequestSource() { + this = rh.(NextHttpRouteHandler).getParameter(0).getAPropertyRead("req") or + this = rh.(NextReqResHandler).getRequest() + } override HTTP::RouteHandler getRouteHandler() { result = rh } } @@ -173,9 +197,12 @@ module NextJS { * A NodeJS HTTP response object in a Next.js page. */ class NextHttpResponseSource extends NodeJSLib::ResponseSource { - NextHttpRouteHandler rh; + HTTP::RouteHandler rh; - NextHttpResponseSource() { this = rh.getParameter(0).getAPropertyRead("res") } + NextHttpResponseSource() { + this = rh.(NextHttpRouteHandler).getParameter(0).getAPropertyRead("res") or + this = rh.(NextReqResHandler).getResponse() + } override HTTP::RouteHandler getRouteHandler() { result = rh } } @@ -204,9 +231,9 @@ module NextJS { } override Parameter getRouteHandlerParameter(string kind) { - kind = "request" and result = getFunction().getParameter(0) + kind = "request" and result = this.getFunction().getParameter(0) or - kind = "response" and result = getFunction().getParameter(1) + kind = "response" and result = this.getFunction().getParameter(1) } } diff --git a/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll b/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll index 8f2576d58d4..2adc68f907a 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll @@ -168,9 +168,9 @@ module NodeJSLib { string kind; RequestInputAccess() { - // `req.url` - kind = "url" and - this.asExpr().(PropAccess).accesses(request, "url") + // `req.url` / `req.body` + kind = ["url", "body"] and + this.asExpr().(PropAccess).accesses(request, kind) or exists(PropAccess headers | // `req.headers.cookie` 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 5a8ed33b530..a4654b29c27 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 @@ -62,6 +62,10 @@ nodes | koa.js:14:16:14:18 | url | | koa.js:20:16:20:18 | url | | koa.js:20:16:20:18 | url | +| next.ts:11:31:11:38 | req.body | +| next.ts:11:31:11:38 | req.body | +| next.ts:11:31:11:50 | req.body.callbackUrl | +| next.ts:11:31:11:50 | req.body.callbackUrl | | node.js:5:7:5:52 | target | | node.js:5:16:5:39 | url.par ... , true) | | node.js:5:16:5:45 | url.par ... ).query | @@ -147,6 +151,10 @@ edges | koa.js:6:12:6:27 | ctx.query.target | koa.js:6:6:6:27 | url | | koa.js:8:18:8:20 | url | koa.js:8:15:8:26 | `${url}${x}` | | koa.js:8:18:8:20 | url | koa.js:8:15:8:26 | `${url}${x}` | +| next.ts:11:31:11:38 | req.body | next.ts:11:31:11:50 | req.body.callbackUrl | +| next.ts:11:31:11:38 | req.body | next.ts:11:31:11:50 | req.body.callbackUrl | +| next.ts:11:31:11:38 | req.body | next.ts:11:31:11:50 | req.body.callbackUrl | +| next.ts:11:31:11:38 | req.body | next.ts:11:31:11:50 | req.body.callbackUrl | | node.js:5:7:5:52 | target | node.js:6:34:6:39 | target | | node.js:5:7:5:52 | target | node.js:6:34:6:39 | target | | node.js:5:16:5:39 | url.par ... , true) | node.js:5:16:5:45 | url.par ... ).query | @@ -195,6 +203,7 @@ edges | koa.js:8:15:8:26 | `${url}${x}` | koa.js:6:12:6:27 | ctx.query.target | koa.js:8:15:8:26 | `${url}${x}` | Untrusted URL redirection due to $@. | koa.js:6:12:6:27 | ctx.query.target | user-provided value | | koa.js:14:16:14:18 | url | koa.js:6:12:6:27 | ctx.query.target | koa.js:14:16:14:18 | url | Untrusted URL redirection due to $@. | koa.js:6:12:6:27 | ctx.query.target | user-provided value | | koa.js:20:16:20:18 | url | koa.js:6:12:6:27 | ctx.query.target | koa.js:20:16:20:18 | url | Untrusted URL redirection due to $@. | koa.js:6:12:6:27 | ctx.query.target | user-provided value | +| next.ts:11:31:11:50 | req.body.callbackUrl | next.ts:11:31:11:38 | req.body | next.ts:11:31:11:50 | req.body.callbackUrl | Untrusted URL redirection due to $@. | next.ts:11:31:11:38 | req.body | user-provided value | | node.js:6:34:6:39 | target | node.js:5:26:5:32 | req.url | node.js:6:34:6:39 | target | Untrusted URL redirection due to $@. | node.js:5:26:5:32 | req.url | user-provided value | | node.js:14:34:14:45 | '/' + target | node.js:10:26:10:32 | req.url | node.js:14:34:14:45 | '/' + target | Untrusted URL redirection due to $@. | node.js:10:26:10:32 | req.url | user-provided value | | node.js:31:34:31:55 | target ... =" + me | node.js:28:26:28:32 | req.url | node.js:31:34:31:55 | target ... =" + me | Untrusted URL redirection due to $@. | node.js:28:26:28:32 | req.url | user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/next.ts b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/next.ts new file mode 100644 index 00000000000..14da7c4f5ba --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/next.ts @@ -0,0 +1,12 @@ + +import type { + NextApiRequest, + NextApiResponse, +} from "next" + +export async function handler( + req: NextApiRequest, + res: NextApiResponse +) { + res.setHeader("Location", req.body.callbackUrl); // NOT OK +} \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/tsconfig.json b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/tsconfig.json new file mode 100644 index 00000000000..82194fc7ab0 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/tsconfig.json @@ -0,0 +1,3 @@ +{ + "include": ["."] +}