Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test: Add support for RelatedLocation tag and use in a JS query #18810

Merged
merged 8 commits into from
Feb 25, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion csharp/ql/test/utils/inline-tests/InlineTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,14 @@ void Problems()

// correct expectation comment
x = "Alert"; // $ Alert[problem-query]

// correct expectation comments with a related location
var related = "Related"; // $ RelatedLocation[problem-query-with-related-loc]
x = "Alert:1"; // $ Alert[problem-query-with-related-loc]

// expectation comments missing the related location
related = "Related";
x = "Alert:1"; // $ Alert[problem-query-with-related-loc]
}

void PathProblems()
Expand Down Expand Up @@ -78,5 +86,17 @@ void PathProblems()
// incorrect expectation comments, using an identifier tag; the alert location coincides with the source location
sink = "Sink"; // $ Sink[path-problem-query]=sink2
x = "Alert:0:1"; // $ Alert[path-problem-query]=sink1

// correct expectation comments with a related location
source = "Source"; // $ Source[path-problem-query-with-related-loc]
sink = "Sink"; // $ Sink[path-problem-query-with-related-loc]
var related = "Related"; // $ RelatedLocation[path-problem-query-with-related-loc]
x = "Alert:3:2:1"; // $ Alert[path-problem-query-with-related-loc]

// expectation comments missing the related location
source = "Source"; // $ Source[path-problem-query-with-related-loc]
sink = "Sink"; // $ Sink[path-problem-query-with-related-loc]
related = "Related";
x = "Alert:3:2:1"; // $ Alert[path-problem-query-with-related-loc]
}
}
}
52 changes: 26 additions & 26 deletions csharp/ql/test/utils/inline-tests/PathProblemQuery.expected
Original file line number Diff line number Diff line change
@@ -1,31 +1,31 @@
#select
| InlineTests.cs:26:17:26:27 | "Alert:2:1" | InlineTests.cs:24:22:24:29 | "Source" | InlineTests.cs:25:20:25:25 | "Sink" | This is a problem |
| InlineTests.cs:36:13:36:23 | "Alert:2:1" | InlineTests.cs:34:18:34:25 | "Source" | InlineTests.cs:35:16:35:21 | "Sink" | This is a problem |
| InlineTests.cs:41:13:41:23 | "Alert:2:1" | InlineTests.cs:39:18:39:25 | "Source" | InlineTests.cs:40:16:40:21 | "Sink" | This is a problem |
| InlineTests.cs:45:13:45:23 | "Alert:1:0" | InlineTests.cs:44:18:44:25 | "Source" | InlineTests.cs:45:13:45:23 | "Alert:1:0" | This is a problem |
| InlineTests.cs:49:13:49:23 | "Alert:0:1" | InlineTests.cs:49:13:49:23 | "Alert:0:1" | InlineTests.cs:48:16:48:21 | "Sink" | This is a problem |
| InlineTests.cs:54:13:54:23 | "Alert:2:1" | InlineTests.cs:52:18:52:25 | "Source" | InlineTests.cs:53:16:53:21 | "Sink" | This is a problem |
| InlineTests.cs:59:13:59:23 | "Alert:2:1" | InlineTests.cs:57:18:57:25 | "Source" | InlineTests.cs:58:16:58:21 | "Sink" | This is a problem |
| InlineTests.cs:64:13:64:23 | "Alert:2:1" | InlineTests.cs:62:18:62:25 | "Source" | InlineTests.cs:63:16:63:21 | "Sink" | This is a problem |
| InlineTests.cs:68:13:68:23 | "Alert:1:0" | InlineTests.cs:67:18:67:25 | "Source" | InlineTests.cs:68:13:68:23 | "Alert:1:0" | This is a problem |
| InlineTests.cs:72:13:72:23 | "Alert:1:0" | InlineTests.cs:71:18:71:25 | "Source" | InlineTests.cs:72:13:72:23 | "Alert:1:0" | This is a problem |
| InlineTests.cs:76:13:76:23 | "Alert:0:1" | InlineTests.cs:76:13:76:23 | "Alert:0:1" | InlineTests.cs:75:16:75:21 | "Sink" | This is a problem |
| InlineTests.cs:80:13:80:23 | "Alert:0:1" | InlineTests.cs:80:13:80:23 | "Alert:0:1" | InlineTests.cs:79:16:79:21 | "Sink" | This is a problem |
| InlineTests.cs:34:17:34:27 | "Alert:2:1" | InlineTests.cs:32:22:32:29 | "Source" | InlineTests.cs:33:20:33:25 | "Sink" | This is a problem |
| InlineTests.cs:44:13:44:23 | "Alert:2:1" | InlineTests.cs:42:18:42:25 | "Source" | InlineTests.cs:43:16:43:21 | "Sink" | This is a problem |
| InlineTests.cs:49:13:49:23 | "Alert:2:1" | InlineTests.cs:47:18:47:25 | "Source" | InlineTests.cs:48:16:48:21 | "Sink" | This is a problem |
| InlineTests.cs:53:13:53:23 | "Alert:1:0" | InlineTests.cs:52:18:52:25 | "Source" | InlineTests.cs:53:13:53:23 | "Alert:1:0" | This is a problem |
| InlineTests.cs:57:13:57:23 | "Alert:0:1" | InlineTests.cs:57:13:57:23 | "Alert:0:1" | InlineTests.cs:56:16:56:21 | "Sink" | This is a problem |
| InlineTests.cs:62:13:62:23 | "Alert:2:1" | InlineTests.cs:60:18:60:25 | "Source" | InlineTests.cs:61:16:61:21 | "Sink" | This is a problem |
| InlineTests.cs:67:13:67:23 | "Alert:2:1" | InlineTests.cs:65:18:65:25 | "Source" | InlineTests.cs:66:16:66:21 | "Sink" | This is a problem |
| InlineTests.cs:72:13:72:23 | "Alert:2:1" | InlineTests.cs:70:18:70:25 | "Source" | InlineTests.cs:71:16:71:21 | "Sink" | This is a problem |
| InlineTests.cs:76:13:76:23 | "Alert:1:0" | InlineTests.cs:75:18:75:25 | "Source" | InlineTests.cs:76:13:76:23 | "Alert:1:0" | This is a problem |
| InlineTests.cs:80:13:80:23 | "Alert:1:0" | InlineTests.cs:79:18:79:25 | "Source" | InlineTests.cs:80:13:80:23 | "Alert:1:0" | This is a problem |
| InlineTests.cs:84:13:84:23 | "Alert:0:1" | InlineTests.cs:84:13:84:23 | "Alert:0:1" | InlineTests.cs:83:16:83:21 | "Sink" | This is a problem |
| InlineTests.cs:88:13:88:23 | "Alert:0:1" | InlineTests.cs:88:13:88:23 | "Alert:0:1" | InlineTests.cs:87:16:87:21 | "Sink" | This is a problem |
edges
testFailures
| InlineTests.cs:6:26:6:35 | // ... | Missing result: Alert |
| InlineTests.cs:12:34:12:43 | // ... | Missing result: Alert |
| InlineTests.cs:29:28:29:38 | // ... | Missing result: Source |
| InlineTests.cs:30:24:30:32 | // ... | Missing result: Sink |
| InlineTests.cs:31:33:31:42 | // ... | Missing result: Alert |
| InlineTests.cs:34:18:34:25 | "Source" | Unexpected result: Source |
| InlineTests.cs:35:16:35:21 | "Sink" | Unexpected result: Sink |
| InlineTests.cs:36:13:36:23 | InlineTests.cs:34:18:34:25 | Unexpected result: Alert |
| InlineTests.cs:58:16:58:21 | "Sink" | Unexpected result: Sink=source2 |
| InlineTests.cs:58:24:58:60 | // ... | Missing result: Sink[path-problem-query]=source1 |
| InlineTests.cs:64:13:64:23 | InlineTests.cs:62:18:62:25 | Unexpected result: Alert=source3 |
| InlineTests.cs:64:26:64:63 | // ... | Missing result: Alert[path-problem-query]=source2 |
| InlineTests.cs:72:13:72:23 | "Alert:1:0" | Unexpected result: Alert=source5 |
| InlineTests.cs:72:26:72:63 | // ... | Missing result: Alert[path-problem-query]=source4 |
| InlineTests.cs:79:16:79:21 | "Sink" | Unexpected result: Sink=sink1 |
| InlineTests.cs:79:24:79:58 | // ... | Missing result: Sink[path-problem-query]=sink2 |
| InlineTests.cs:37:28:37:38 | // ... | Missing result: Source |
| InlineTests.cs:38:24:38:32 | // ... | Missing result: Sink |
| InlineTests.cs:39:33:39:42 | // ... | Missing result: Alert |
| InlineTests.cs:42:18:42:25 | "Source" | Unexpected result: Source |
| InlineTests.cs:43:16:43:21 | "Sink" | Unexpected result: Sink |
| InlineTests.cs:44:13:44:23 | InlineTests.cs:42:18:42:25 | Unexpected result: Alert |
| InlineTests.cs:66:16:66:21 | "Sink" | Unexpected result: Sink=source2 |
| InlineTests.cs:66:24:66:60 | // ... | Missing result: Sink[path-problem-query]=source1 |
| InlineTests.cs:72:13:72:23 | InlineTests.cs:70:18:70:25 | Unexpected result: Alert=source3 |
| InlineTests.cs:72:26:72:63 | // ... | Missing result: Alert[path-problem-query]=source2 |
| InlineTests.cs:80:13:80:23 | "Alert:1:0" | Unexpected result: Alert=source5 |
| InlineTests.cs:80:26:80:63 | // ... | Missing result: Alert[path-problem-query]=source4 |
| InlineTests.cs:87:16:87:21 | "Sink" | Unexpected result: Sink=sink1 |
| InlineTests.cs:87:24:87:58 | // ... | Missing result: Sink[path-problem-query]=sink2 |
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#select
| InlineTests.cs:94:13:94:25 | "Alert:3:2:1" | InlineTests.cs:91:18:91:25 | "Source" | InlineTests.cs:92:16:92:21 | "Sink" | This is a problem with $@ | InlineTests.cs:93:23:93:31 | "Related" | a related location |
| InlineTests.cs:100:13:100:25 | "Alert:3:2:1" | InlineTests.cs:97:18:97:25 | "Source" | InlineTests.cs:98:16:98:21 | "Sink" | This is a problem with $@ | InlineTests.cs:99:19:99:27 | "Related" | a related location |
edges
testFailures
| InlineTests.cs:6:26:6:35 | // ... | Missing result: Alert |
| InlineTests.cs:12:34:12:43 | // ... | Missing result: Alert |
| InlineTests.cs:32:32:32:42 | // ... | Missing result: Source |
| InlineTests.cs:33:28:33:36 | // ... | Missing result: Sink |
| InlineTests.cs:34:30:34:39 | // ... | Missing result: Alert |
| InlineTests.cs:37:28:37:38 | // ... | Missing result: Source |
| InlineTests.cs:38:24:38:32 | // ... | Missing result: Sink |
| InlineTests.cs:39:33:39:42 | // ... | Missing result: Alert |
| InlineTests.cs:99:19:99:27 | "Related" | Unexpected result: RelatedLocation |
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
query: utils/inline-tests/queries/PathProblemQueryRelatedLocs.ql
postprocess: utils/test/InlineExpectationsTestQuery.ql
4 changes: 2 additions & 2 deletions csharp/ql/test/utils/inline-tests/ProblemQuery.expected
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@
testFailures
| InlineTests.cs:12:34:12:43 | // ... | Missing result: Alert |
| InlineTests.cs:15:13:15:19 | This is a problem | Unexpected result: Alert |
| InlineTests.cs:26:30:26:39 | // ... | Missing result: Alert |
| InlineTests.cs:31:33:31:42 | // ... | Missing result: Alert |
| InlineTests.cs:34:30:34:39 | // ... | Missing result: Alert |
| InlineTests.cs:39:33:39:42 | // ... | Missing result: Alert |
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#select
| InlineTests.cs:22:13:22:21 | "Alert:1" | This is a problem with $@ | InlineTests.cs:21:23:21:31 | "Related" | a related location |
| InlineTests.cs:26:13:26:21 | "Alert:1" | This is a problem with $@ | InlineTests.cs:25:19:25:27 | "Related" | a related location |
testFailures
| InlineTests.cs:6:26:6:35 | // ... | Missing result: Alert |
| InlineTests.cs:12:34:12:43 | // ... | Missing result: Alert |
| InlineTests.cs:25:19:25:27 | "Related" | Unexpected result: RelatedLocation |
| InlineTests.cs:34:30:34:39 | // ... | Missing result: Alert |
| InlineTests.cs:39:33:39:42 | // ... | Missing result: Alert |
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
query: utils/inline-tests/queries/ProblemQueryRelatedLocs.ql
postprocess: utils/test/InlineExpectationsTestQuery.ql
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
edges
#select
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/**
* @kind path-problem
* @id path-problem-query-with-related-loc
*/

import csharp

query predicate edges(StringLiteral sl1, StringLiteral sl2) { none() }

from StringLiteral alert, StringLiteral source, StringLiteral sink, StringLiteral related
where
exists(string regexp, int sourceOffset, int sinkOffset, int relatedOffset |
regexp = "Alert:([0-9]+):([0-9]+):([0-9]+)"
|
sourceOffset = alert.getValue().regexpCapture(regexp, 1).toInt() and
sinkOffset = alert.getValue().regexpCapture(regexp, 2).toInt() and
relatedOffset = alert.getValue().regexpCapture(regexp, 3).toInt() and
source.getLocation().getStartLine() = alert.getLocation().getStartLine() - sourceOffset and
sink.getLocation().getStartLine() = alert.getLocation().getStartLine() - sinkOffset and
related.getLocation().getStartLine() = alert.getLocation().getStartLine() - relatedOffset
)
select alert, source, sink, "This is a problem with $@", related, "a related location"
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/**
* @kind problem
* @id problem-query-with-related-loc
*/

import csharp

from StringLiteral sl, StringLiteral related, int offset
where
sl.getValue().regexpCapture("Alert:([0-9]+)", 1).toInt() = offset and
related.getLocation().getStartLine() = sl.getLocation().getStartLine() - offset
select sl, "This is a problem with $@", related, "a related location"
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
Security/CWE-352/MissingCsrfMiddleware.ql
query: Security/CWE-352/MissingCsrfMiddleware.ql
postprocess: utils/test/InlineExpectationsTestQuery.ql
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,17 @@ var passport = require('passport');

var app = express();

app.use(cookieParser());
app.use(cookieParser()); // $ Alert
app.use(passport.authorize({ session: true }));

app.post('/changeEmail', function (req, res) {
let newEmail = req.cookies["newEmail"];
});
}); // $ RelatedLocation

(function () {
var app = express();

app.use(cookieParser());
app.use(cookieParser()); // $ Alert
app.use(passport.authorize({ session: true }));

const errorCatch = (fn) =>
Expand All @@ -24,13 +24,13 @@ app.post('/changeEmail', function (req, res) {

app.post('/changeEmail', errorCatch(async function (req, res) {
let newEmail = req.cookies["newEmail"];
}));
})); // $ RelatedLocation
})

(function () {
var app = express();

app.use(cookieParser());
app.use(cookieParser()); // $ Alert
app.use(passport.authorize({ session: true }));

const errorCatch = (fn) =>
Expand All @@ -40,9 +40,9 @@ app.post('/changeEmail', function (req, res) {

app.post('/changeEmail', errorCatch(async function (req, res) {
let newEmail = req.cookies["newEmail"];
}));
})); // $ RelatedLocation

app.post('/doLoginStuff', errorCatch(async function (req, res) {
req.session.user = loginStuff(req);
}));
})); // $ RelatedLocation
})
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,10 @@ function createApiRouter () {
res.send('no csrf to get here')
})

router.post('/getProfile_unsafe', cookieParser(), function (req, res) { // NOT OK - may use cookies
router.post('/getProfile_unsafe', cookieParser(), function (req, res) { // $ Alert - may use cookies
let newEmail = req.cookies["newEmail"];
res.send('no csrf to get here')
})
}) // $ RelatedLocation

return router
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ var app = express()

// parse cookies
// we need this because "cookie" is true in csrfProtection
app.use(cookieParser())
app.use(cookieParser()) // $ Alert

app.get('/form', csrfProtection, function (req, res) { // OK
let newEmail = req.cookies["newEmail"];
Expand All @@ -28,7 +28,7 @@ app.post('/process', parseForm, csrfProtection, function (req, res) { // OK
res.send('data is being processed')
})

app.post('/process_unsafe', parseForm, function (req, res) { // NOT OK
app.post('/process_unsafe', parseForm, function (req, res) {
let newEmail = req.cookies["newEmail"];
res.send('data is being processed')
})
}) // $ RelatedLocation
6 changes: 3 additions & 3 deletions javascript/ql/test/query-tests/Security/CWE-352/fastify.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ const fastify = require('fastify')

const app = fastify();

app.register(require('fastify-cookie'));
app.register(require('fastify-cookie')); // $ Alert
app.register(require('fastify-csrf'));

app.route({
Expand All @@ -17,10 +17,10 @@ app.route({
app.route({
method: 'POST',
path: '/',
handler: async (req, reply) => { // NOT OK - lacks CSRF protection
handler: async (req, reply) => { // lacks CSRF protection
req.session.blah;
return req.body
}
} // $ RelatedLocation
})


Expand Down
6 changes: 3 additions & 3 deletions javascript/ql/test/query-tests/Security/CWE-352/fastify2.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const fp = require('fastify-plugin');
const app = fastify();

function plugin(app) {
app.register(require('fastify-cookie'));
app.register(require('fastify-cookie')); // $ Alert
app.register(require('fastify-csrf'));
}
app.register(fp(plugin));
Expand All @@ -21,10 +21,10 @@ app.route({
app.route({
method: 'POST',
path: '/',
handler: async (req, reply) => { // NOT OK - lacks CSRF protection
handler: async (req, reply) => { // lacks CSRF protection
req.session.blah;
return req.body
}
} // $ RelatedLocation
})


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ var parseForm = bodyParser.urlencoded({ extended: false })
var lusca = require('lusca');

var app = express()
app.use(cookieParser())
app.use(cookieParser()) // $ Alert

app.post('/process', parseForm, lusca.csrf(), function (req, res) { // OK
let newEmail = req.cookies["newEmail"];
Expand All @@ -23,12 +23,12 @@ app.post('/process', parseForm, lusca({csrf:{}}), function (req, res) { // OK
res.send('data is being processed')
})

app.post('/process', parseForm, lusca(), function (req, res) { // NOT OK - missing csrf option
app.post('/process', parseForm, lusca(), function (req, res) { // missing csrf option
let newEmail = req.cookies["newEmail"];
res.send('data is being processed')
})
}) // $ RelatedLocation

app.post('/process_unsafe', parseForm, function (req, res) { // NOT OK
app.post('/process_unsafe', parseForm, function (req, res) {
let newEmail = req.cookies["newEmail"];
res.send('data is being processed')
})
}) // $ RelatedLocation
6 changes: 3 additions & 3 deletions javascript/ql/test/query-tests/Security/CWE-352/tst.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@ const cookieParser = require('cookie-parser')
const csrf = require('csurf')

const app = express()
app.use(cookieParser())
app.use(cookieParser()) // $ Alert

app.post('/unsafe', (req, res) => { // NOT OK
app.post('/unsafe', (req, res) => {
req.cookies.x;
});
}); // $ RelatedLocation

function middlewares() {
return express.Router()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@ let cookieParser = require('cookie-parser');

let app = express();

app.use(cookieParser());
app.use(cookieParser()); // $ Alert

app.post('/doSomethingTerrible', (req, res) => { // NOT OK - uses cookies
app.post('/doSomethingTerrible', (req, res) => { // uses cookies
if (req.cookies['secret'] === app.secret) {
somethingTerrible();
}
res.end('Ok');
});
}); // $ RelatedLocation

app.post('/doSomethingElse', (req, res) => { // OK - doesn't actually use cookies
somethingElse(req.query['data']);
Expand All @@ -26,14 +26,14 @@ app.post('/doWithCaptcha', (req, res) => { // OK - attacker can't guess the capt
res.end('Ok');
});

app.post('/user', (req, res) => { // NOT OK - access to req.user is unprotected
app.post('/user', (req, res) => { // access to req.user is unprotected
somethingElse(req.user.name);
res.end('Ok');
});
}); // $ RelatedLocation

app.post('/session', (req, res) => { // NOT OK - access to req.session is unprotected
app.post('/session', (req, res) => { // access to req.session is unprotected
somethingElse(req.session.name);
res.end('Ok');
});
}); // $ RelatedLocation

app.listen();
Loading