Javascript: Improve Restify support and add new Spife support #10663

pwntester · 2022-10-03T14:01:55Z

This PR improves Restify support and adds brand new support for Spife framework

javascript/ql/lib/semmle/javascript/frameworks/Restify.qll

erik-krogh

two quick comments from a quick read of the code.

javascript/ql/lib/semmle/javascript/frameworks/Restify.qll

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll

erik-krogh · 2022-10-13T12:10:21Z

The qldoc check is failing because you don't have a toplevel QLDoc in Spife.qll.

I think you should add a change-note to javascript/ql/lib/change-notes.

asgerf

Looks great, just a few things I noticed from a first read through. I haven't done a detailed review and I'm not very familiar with these libraries so will need more time to digest this.

javascript/ql/lib/semmle/javascript/frameworks/Restify.qll

javascript/ql/test/library-tests/frameworks/Restify2/tests.ql

javascript/ql/test/library-tests/frameworks/Spife/tests.ql

javascript/ql/test/library-tests/frameworks/Restify2/tests.ql

javascript/ql/test/library-tests/frameworks/Spife/tests.ql

erik-krogh · 2022-10-14T07:44:43Z

You have a few failing tests:
javascript/ql/test/library-tests/frameworks/restify/tests.ql, javascript/ql/test/library-tests/frameworks/HTTP/RemoteRequestInput.ql, javascript/ql/test/library-tests/frameworks/NodeJSLib/tests.ql.

erik-krogh

A lot of minor comments after a readthrough.

My suggestions will definitely cause the autoformatter to fail, so you need to run that.

javascript/ql/lib/semmle/javascript/frameworks/Restify.qll

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll

erik-krogh · 2022-10-18T09:57:35Z

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll

+        reply.ref().(DataFlow::CallNode).getCalleeName() =
+          ["reply", "cookie", "link", "header", "headers", "raw", "status", "toStream", "vary"] and
+        this = reply.ref().(DataFlow::CallNode).getArgument(0) and
+        rh = reply.getRouteHandler()


Calling reply.ref() two times gives you two different references, so you should only do that once here.

And I think you might have meant reply.ref().getAMethodCall(["reply", ...])? (Similar to what I pointed out in CookieDefinition).

Thanks, will fix it!

erik-krogh · 2022-10-18T09:59:05Z

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll

+    TemplateObjectInput() {
+      reply.ref().(DataFlow::MethodCallNode).getMethodName() = "template" and
+      this = reply.ref().(DataFlow::MethodCallNode).getArgument(1)
+    }


Don't call reply.ref() twice, it gives two different references.

And I think this is right, I think you meant reply.ref().getAMethodCall("template"). (Similar to what I pointed out in CookieDefinition).

And there are also some other classes where you do eply.ref().(DataFlow::MethodCallNode).
Could you add tests for those so we are sure they are correct.

Sure, thanks

erik-krogh · 2022-10-18T10:00:25Z

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll

+    RedirectInvocation() {
+      this = reply.ref().(DataFlow::MethodCallNode) and
+      this.getMethodName() = "redirect"
+    }


Same as above.

erik-krogh · 2022-10-18T10:00:33Z

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll

+      exists(ReplySource reply |
+        reply.ref().(DataFlow::MethodCallNode).getMethodName() = "template" and
+        this = reply.ref()
+      )


Same as above.

I checked your test for this.

You have a call to reply.template(..) in your tests, and on that you test for templateInstantiation.
But your tests.ql don't have a case for templateInstantiation, so that is never tested.

Nice, catch, thanks, will add it!

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll

javascript/ql/lib/semmle/javascript/frameworks/Restify.qll

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll

javascript/ql/lib/semmle/javascript/frameworks/Restify.qll

pwntester · 2022-10-18T11:31:32Z

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll

+        reply.ref().(DataFlow::CallNode).getCalleeName() =
+          ["reply", "cookie", "link", "header", "headers", "raw", "status", "toStream", "vary"] and
+        this = reply.ref().(DataFlow::CallNode).getArgument(0) and
+        rh = reply.getRouteHandler()


Thanks, will fix it!

pwntester · 2022-10-18T11:32:50Z

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll

+    TemplateObjectInput() {
+      reply.ref().(DataFlow::MethodCallNode).getMethodName() = "template" and
+      this = reply.ref().(DataFlow::MethodCallNode).getArgument(1)
+    }


Sure, thanks

pwntester · 2022-10-18T11:33:49Z

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll

+      exists(ReplySource reply |
+        reply.ref().(DataFlow::MethodCallNode).getMethodName() = "template" and
+        this = reply.ref()
+      )


Nice, catch, thanks, will add it!

javascript/ql/test/library-tests/frameworks/Spife/tests.ql

Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>

erik-krogh · 2022-10-19T20:07:09Z

The qldoc check is complaining about missing documentation on Restify::Restify::FormatterSetup::getAFormatterHandler.

…odeql into restify_improvements

pwntester · 2022-12-05T21:44:16Z

@pwntester did you see my above suggestions?

Sorry I missed the notifications about the review comments. Will take a look ASAP

Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll

pwntester · 2022-12-07T09:42:18Z

Thanks @erik-krogh I applied all suggestions, but had to revert 3 of them since there raised some ambiguity errors between parent types

erik-krogh

Another round of comments.

Could you also do a merge with main? your base-commit is getting old.

javascript/ql/lib/semmle/javascript/frameworks/Restify.qll

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll

erik-krogh · 2022-12-11T22:23:50Z

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll

+      )
+    }
+
+    API::Node getHandlerByName(string name) { result = this.getParameter(0).getMember(name) }


You also have to remove the rest of the uses of API::Node in this class.

@erik-krogh how would you rewrite this predicate? I thought about this result = this.getACallee().getParameter(0).(DataFlow::ObjectLiteralNode).getAPropertyRead(name) but ObjectLiteralNode is not compatible with Parameter

Just remove .(DataFlow::ObjectLiteralNode) from your attempt and you should be good.
getAPropertyRead() is defined on SourceNode, and both ObjectLiteralNode and ParameterNode are SourceNodes.

Ok, I was not getting that option from the autocomplete menu, but I was missing a wrapping call to DataFlow::parameterNode()

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll

Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll

erik-krogh

One more comment, and then I think we're there.

I have done an evaluation, and that looks fine.

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll

Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>

erik-krogh · 2022-12-14T11:11:20Z

You fixed the implementation in 4ba3190, but you didn't add any tests to match.
I don't think you have any tests of Spife::RouteSetup (and the classes that depend on Spife::RouteSetup).

Those tests can probably be added as inline test comments in javascript/ql/test/library-tests/frameworks/Spife/lib/routes/index.js, and that file should probably also be expanded.

pwntester · 2022-12-14T11:38:59Z

Thought about adding some tests as you proposed but I thought it was already tested indirectly and we have plenty of tests for handlers and those can only be correctly identified if the RouteSetup was matched correctly. Will add some explicit tests though

…odeql into restify_improvements

erik-krogh · 2022-12-14T13:15:47Z

javascript/ql/test/library-tests/frameworks/Spife/tests.ql

+import semmle.javascript.heuristics.AdditionalRouteHandlers
+


Suggested change

import semmle.javascript.heuristics.AdditionalRouteHandlers

This was the reason that your tests "worked" before the fix.

Your Spife::RouteSetup and Spife::StandardRouteHandler were effectively unused in your tests before the fix, because your tests don't distinguish between heuristic and non-heuristic route-handlers.
You have to distinguish between heuristic and non-heuristic, otherwise you're only testing the former.

You can try to delete Spife::StandardRouteHandler, your tests still pass as they are right now.

You have a bunch of test failures when you delete this line, and I see three possible solution to that:

Accept the new test output (including failures), to document that some things can only be detected using the heuristic route-handlers.

Delete the tests lines are only detected by the heuristic route-handlers.
This is fine, our other heuristic route-handlers don't really have tests.

Add another test that tests the heuristic route-handlers.

(The above assumes that no other bug surfaces from this).

Sorry for figuring this out so late, but I didn't look that closely at your test implementations (or your tests).

erik-krogh · 2022-12-14T13:21:07Z

javascript/ql/test/library-tests/frameworks/Restify2/tests.ql

+import semmle.javascript.heuristics.AdditionalRouteHandlers
+


Suggested change

import semmle.javascript.heuristics.AdditionalRouteHandlers

Same here.

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll

erik-krogh

Lets get this merged.

I got a final confirmation evaluation running, but I'll do something if that doesn't turn out OK.

github-actions bot added the JS label Oct 3, 2022

github-advanced-security bot found potential problems Oct 3, 2022

View reviewed changes

javascript/ql/lib/semmle/javascript/frameworks/Restify.qll Fixed Show fixed Hide fixed

javascript/ql/lib/semmle/javascript/frameworks/Restify.qll Fixed Show fixed Hide fixed

erik-krogh reviewed Oct 3, 2022

View reviewed changes

javascript/ql/lib/semmle/javascript/frameworks/Restify.qll Outdated Show resolved Hide resolved

javascript/ql/lib/semmle/javascript/frameworks/Restify.qll Outdated Show resolved Hide resolved

pwntester commented Oct 4, 2022

View reviewed changes

javascript/ql/lib/semmle/javascript/frameworks/Restify.qll Outdated Show resolved Hide resolved

pwntester changed the title ~~Improve Restify support~~ Javascript: Improve Restify support Oct 4, 2022

github-advanced-security bot found potential problems Oct 11, 2022

View reviewed changes

pwntester changed the title ~~Javascript: Improve Restify support~~ Javascript: Improve Restify support and add new Spife support Oct 13, 2022

pwntester marked this pull request as ready for review October 13, 2022 12:00

pwntester requested a review from a team as a code owner October 13, 2022 12:00

pwntester requested a review from erik-krogh October 13, 2022 12:01

pwntester force-pushed the restify_improvements branch from 2ebba03 to 573cad4 Compare October 13, 2022 13:09

asgerf reviewed Oct 13, 2022

View reviewed changes

javascript/ql/lib/semmle/javascript/frameworks/Restify.qll Outdated Show resolved Hide resolved

javascript/ql/lib/semmle/javascript/frameworks/Restify.qll Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems Oct 13, 2022

View reviewed changes

github-actions bot added the documentation label Oct 13, 2022

erik-krogh reviewed Oct 18, 2022

View reviewed changes

github-advanced-security bot found potential problems Oct 18, 2022

View reviewed changes

pwntester force-pushed the restify_improvements branch from 9d9efa6 to e080d8f Compare October 18, 2022 14:58

pwntester commented Oct 18, 2022

View reviewed changes

Alvaro Muñoz and others added 2 commits October 18, 2022 21:41

Add Restify/Spife support

6ab62da

Address code review comments

b79f7f3

Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>

pwntester force-pushed the restify_improvements branch from e080d8f to b79f7f3 Compare October 18, 2022 19:42

Alvaro Muñoz added 3 commits October 19, 2022 17:32

Fix format errors

31d271b

Fix format errors

976dd7f

Merge branch 'main' into restify_improvements

2ad5a70

Alvaro Muñoz added 3 commits October 19, 2022 22:18

Add QLDoc for FormatterSetup.getAFormatterHandler

009403b

Merge branch 'restify_improvements' of https://github.com/pwntester/c…

c10087b

…odeql into restify_improvements

Update test results after merging new XSS improvements

c7ac237

Alvaro Muñoz and others added 3 commits December 7, 2022 10:29

Apply suggestions from code review

3e92b4c

Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>

Add feedback from Code review

407df37

restoring previous casts to avoid super type ambiguity

af015d3

pwntester commented Dec 7, 2022

View reviewed changes

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll Outdated Show resolved Hide resolved

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll Outdated Show resolved Hide resolved

Use ReplyCall.super syntax instead of this.(ReplyCall)

38b2f53

erik-krogh reviewed Dec 11, 2022

View reviewed changes

Update javascript/ql/lib/semmle/javascript/frameworks/Spife.qll

1410d28

Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>

pwntester commented Dec 12, 2022

View reviewed changes

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll Outdated Show resolved Hide resolved

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll Outdated Show resolved Hide resolved

Alvaro Muñoz added 3 commits December 12, 2022 10:46

Use fluent API instead of hasPropertyWrite

469d7f5

Replace API::Node with DataFlow::Node for Spife's RouteSetup

4ba3190

format Restify.qll

270a435

erik-krogh reviewed Dec 13, 2022

View reviewed changes

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll Outdated Show resolved Hide resolved

Update javascript/ql/lib/semmle/javascript/frameworks/Spife.qll

701676e

Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>

Alvaro Muñoz added 2 commits December 14, 2022 13:11

add tests

a71fc93

Merge branch 'restify_improvements' of https://github.com/pwntester/c…

e1f05e9

…odeql into restify_improvements

erik-krogh reviewed Dec 14, 2022

View reviewed changes

Alvaro Muñoz added 4 commits December 14, 2022 15:38

fix restify tests

14faff4

restore Spife.qll to working status

4cf7299

fix Spife tests (without heuristics)

818c2da

port RouteSetup API-based implementation to DataFlow one

f46a8fa

github-advanced-security bot found potential problems Dec 14, 2022

View reviewed changes

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll Fixed Show fixed Hide fixed

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll Fixed Show fixed Hide fixed

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll Fixed Show fixed Hide fixed

erik-krogh reviewed Dec 14, 2022

View reviewed changes

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll Outdated Show resolved Hide resolved

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll Outdated Show resolved Hide resolved

javascript/ql/lib/semmle/javascript/frameworks/Spife.qll Outdated Show resolved Hide resolved

add explicit this

5555812

erik-krogh approved these changes Dec 15, 2022

View reviewed changes

erik-krogh merged commit 1500fa5 into github:main Dec 15, 2022

Javascript: Improve Restify support and add new Spife support #10663

Javascript: Improve Restify support and add new Spife support #10663

Uh oh!

Conversation

pwntester commented Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erik-krogh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erik-krogh commented Oct 13, 2022

Uh oh!

asgerf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erik-krogh commented Oct 14, 2022

Uh oh!

erik-krogh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pwntester commented Oct 3, 2022 •

edited

Loading

erik-krogh Dec 14, 2022 •

edited

Loading