Skip to content

JS: Recognize Express param value callback as RemoteFlowSource#3424

Merged
semmle-qlci merged 12 commits into
github:masterfrom
asger-semmle:js/express-param-handler
May 18, 2020
Merged

JS: Recognize Express param value callback as RemoteFlowSource#3424
semmle-qlci merged 12 commits into
github:masterfrom
asger-semmle:js/express-param-handler

Conversation

@asgerf
Copy link
Copy Markdown
Contributor

@asgerf asgerf commented May 6, 2020

A param route handler in Express takes the value of the parameter in the 4th argument as seen here. This PR adds it as a RequestInputSource.

@asgerf asgerf added JS Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish labels May 6, 2020
@asgerf asgerf requested a review from a team as a code owner May 6, 2020 21:04
esbena
esbena reviewed May 7, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

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

Nice spot. I am curious about how common this is.

Have you considered adjusting

codeql/javascript/ql/src/semmle/javascript/frameworks/ConnectExpressShared.qll

Lines 15 to 28 in b2f1008

SimpleParameter getRouteHandlerParameter(Function routeHandler, string kind) {
exists(int index, int offset |
result = routeHandler.getParameter(index + offset) and
(if routeHandler.getNumParameter() = 4 then offset = 0 else offset = -1)
|
kind = "error" and index = 0
or
kind = "request" and index = 1
or
kind = "response" and index = 2
or
kind = "next" and index = 3
)
}
to account for this?

(also: change-note)

@asgerf
Copy link
Copy Markdown
Contributor Author

asgerf commented May 11, 2020

Thanks for pointing that out. Turns out we even misclassified the request and response parameters of these route handlers, due to them taking 4 parameters.

Updating this was more tricky than it had any right to be. PTAL.

Evaluation looks a bit swingy, will re-run the slowest.

esbena
esbena reviewed May 11, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

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

Updating this was more tricky than it had any right to be

You don't say! :(
The implementation looks good though.

JS: Go back to disjunction 8fcd024

Is this due to an optimizer limitation?

I suppose we also need a change-note for - [express](https://www.npmjs.com/package/express)

Comment thread javascript/ql/src/semmle/javascript/frameworks/ConnectExpressShared.qll Outdated
@asgerf
Copy link
Copy Markdown
Contributor Author

asgerf commented May 11, 2020

Is this due to an optimizer limitation?

No, just formatting.

@asgerf
Copy link
Copy Markdown
Contributor Author

asgerf commented May 13, 2020

Re-run of slowest is nowhere near as bad as in the original evaluation, but still a bit biased. I'll look at some tuple counts to see what's going.

@asgerf asgerf force-pushed the js/express-param-handler branch from 9f68e3f to d84f1b4 Compare May 15, 2020 12:00
@asgerf
Copy link
Copy Markdown
Contributor Author

asgerf commented May 15, 2020

The use of accesses and calls had dubious join orderings, so I opted to switch to the SourceNode-based API instead. After rebasing on the moduleImport-caching PR this now seems to perform well but I'll run more evaluations over the weekend.

As a bonus, we now recognize req.query[x] as a RemoteInputAccess. 😅

Hopefully that's the last feature-creep in this PR.

esbena
esbena previously approved these changes May 15, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

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

Excellent. Thanks for creeping so fast :)

@asgerf
Copy link
Copy Markdown
Contributor Author

asgerf commented May 16, 2020

Security/nightly looks good.

@asgerf asgerf removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label May 16, 2020
@semmle-qlci semmle-qlci merged commit 6041d52 into github:master May 18, 2020
@esbena esbena mentioned this pull request May 18, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@esbena esbena esbena approved these changes

Assignees

No one assigned

Labels

JS

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@asgerf @esbena @semmle-qlci