Skip to content

Conversation

esbena
Copy link
Contributor

@esbena esbena commented May 20, 2020

Yet another minimal security-related model of a HTTP server implementation. The implementation is mostly boiler-plate for this kind of model. It would be nice if the already extensive HTTP.qll could recude the boiler-plate even more, but that can be done in a separate PR.

I am evaluating this on a collection of fastify dependents.

Addresses #3435.

@esbena esbena added JS Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish labels May 20, 2020
@esbena esbena requested a review from a team as a code owner May 20, 2020 09:37
@esbena
Copy link
Contributor Author

esbena commented May 20, 2020

Evaluation: nothing of interest on a set of relatively small applications.

Copy link
Contributor

@asgerf asgerf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

I'd like it if we could move towards the SourceNode chainable API over the calls/accesses predicates, and certainly away from Expr-based API when there is a data flow alternative (even though it's slightly less urgent after the recent PR).

Feel free to introduce RouteHandler.getARequestRef() and RouteHandler.getAResponseRef() if you feel the .ref() calls are a bit much.

@esbena
Copy link
Contributor Author

esbena commented May 20, 2020

I'd like it if we could move towards the SourceNode chainable API over the calls/accesses predicates, and certainly away from Expr-based API when there is a data flow alternative (even though it's slightly less urgent after the recent PR).

I agree. I should have mentioned that this was a deliberate choice to match the existing code, I am sorry that I made you make the many suggestions.

I have addressed all of the comments, except the one about user-controlled objects. I'll dig into how that works for this library...

@esbena
Copy link
Contributor Author

esbena commented May 20, 2020

A simple isUserControlledObject has been implemented. Additional work items have been recorded in https://github.com/github/codeql-javascript-team/issues/128.

@@ -132,7 +132,9 @@ module Fastify {
string kind;

RequestInputAccess() {
exists(string name | this.(DataFlow::PropRead).accesses(rh.getARequestExpr().flow(), name) |
exists(DataFlow::PropRead read, string name |
this = read and read = rh.getARequestSource().ref().getAPropertyRead(name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a need for the read variable here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good spot.

@esbena
Copy link
Contributor Author

esbena commented May 20, 2020

The dataflow-based evaluation is equally boring.

@esbena
Copy link
Contributor Author

esbena commented May 20, 2020

All comments have been addressed.

asgerf
asgerf previously approved these changes May 20, 2020
@subesokun subesokun mentioned this pull request May 20, 2020
@Ethan-Arrowood
Copy link

This is very nice work. The Fastify team appreciates it @esbena . If you ever need our input please feel free to reach out.

@mcollina
Copy link

Thanks @esbena!

@esbena
Copy link
Contributor Author

esbena commented May 22, 2020

Rebased with updated expected output.

@esbena esbena linked an issue May 22, 2020 that may be closed by this pull request
@semmle-qlci semmle-qlci merged commit 4b56229 into github:master May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish JS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JS: Add support for Fastify
5 participants