Sitelet https://web.archive.org/web/20260220065820/https://github.com/github/codeql/pull/3848
Skip to content

JS: support simple callbacks for setting up Express route handlers#3848

Closed
erik-krogh wants to merge 4 commits intogithub:masterfrom
erik-krogh:handlerCallbacks
Closed

JS: support simple callbacks for setting up Express route handlers#3848
erik-krogh wants to merge 4 commits intogithub:masterfrom
erik-krogh:handlerCallbacks

Conversation

@erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Jun 30, 2020 •
edited
Loading

Inspired by this route handler. (Found using ATM)

I tried to see if I could find more route-handlers using this addition.
But so far I've not found anything other than the motivating-example.

TODO:

  • Add test
  • Evaluation

@erik-krogh erik-krogh added JS Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish labels Jun 30, 2020
@erik-krogh erik-krogh marked this pull request as ready for review July 1, 2020 12:09
@erik-krogh erik-krogh requested a review from a team as a code owner July 1, 2020 12:09
esbena
esbena reviewed Jul 1, 2020
View reviewed changes
Copy link
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.

Hmm. This seems a bit too ad hoc for my taste. And the wrapped candidate isn't really a route handler, is it? Its return/throw semantic has changed slightly (c.f. js/server-crash).

I wonder if type-tracking of req/res instead be convinced to go through such wrapper functions?

@erik-krogh erik-krogh marked this pull request as draft July 1, 2020 14:15
@erik-krogh
Copy link
Contributor Author

erik-krogh commented Jul 1, 2020

I wonder if type-tracking of req/res instead be convinced to go through such wrapper functions?

It can, and that is probably a better approach.

I've implemented it in RequestSource::ref() for now, but I feel it belongs somewhere else (although I'm unsure where).

Doing the same thing for all calls is a bad idea, as our callgraph might blow up in e.g. angular.forEach().

@erik-krogh
Copy link
Contributor Author

erik-krogh commented Jul 8, 2020

An evaluation looks fine.

I'll check out --metric TaintSources.ql, move the new type-tracking steps into a helper predicate, and then un-draft this PR.

@erik-krogh erik-krogh removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Jul 8, 2020
@erik-krogh
Copy link
Contributor Author

erik-krogh commented Aug 5, 2020

A rather large evaluation with --metric TaintSources.ql was unable to find new taint-sources.

I'll just close this PR for now because of the extremely limited impact.
I'll take it up again if someone thinks the PR is worth landing.

@erik-krogh erik-krogh closed this Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@esbena esbena esbena left review comments

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Assignees

No one assigned

Labels

JS

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@erik-krogh @esbena

Comments