Skip to content

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Mar 28, 2022

Adds the following API::Node members:

  • getADecoratedClass
  • getADecoratedMember
  • getADecoratedParameter

And corresponding MaD tokens:

  • DecoratedClass
  • DecoratedMember
  • DecoratedParameter

For example:

import { D } from "foo";

@D
class C // Matched by: foo;;Member[D].DecoratedClass

DecoratedClass could alternatively be expressed in terms of Argument[0] and ReturnValue. The others are not so simple, however, as they involve passing a base object and member name as an argument to the decorator, which typically then performs a reflective lookup. We don't have API edges for reflective lookups as they're tricky to model as unary operators.

So for the time being, no argument/return edges are generated for any of the decorator patterns although it's possible we should do so for class decorators in the future.

Awaiting evaluation when DCA comes up again.

@asgerf asgerf added JS Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish labels Mar 28, 2022
@asgerf asgerf force-pushed the js/decorated-method-or-class branch from 0f18f64 to cf596a1 Compare March 28, 2022 13:34
@asgerf asgerf marked this pull request as ready for review March 29, 2022 08:55
@asgerf asgerf requested a review from a team as a code owner March 29, 2022 08:55
@asgerf asgerf added the no-change-note-required This PR does not need a change note label Mar 29, 2022
exists(Parameter param |
useNodeFlowsToDecorator(base, param.getADecorator()) and
lbl = Label::decoratedParameter() and
ref = DataFlow::parameterNode(param)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm unsure if there should be a def-node for this parameter-node.
And the tests pass just fine if I move the decoratedParameter edge to the decoratorUseEdge predicate.

Copy link
Contributor Author

@asgerf asgerf Mar 29, 2022

Choose a reason for hiding this comment

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

It's technically possible for the parameter decorator to replace the whole method with one that treats that parameter as a sink.

I have never seen any decorators that would have such behaviour, though. A made-up example might be a decorator that adds some expensive runtime checks on a value, which could then be a DoS sink.

In any case, it turns out using a parameter node as a sink is actually a terrible idea, at least with the current implementation (see test case in ca145f2).

So I've changed it to only generate use-nodes 👍

erik-krogh
erik-krogh previously approved these changes Mar 29, 2022
Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

👍

You just need the autoformatter to be happy.

@asgerf
Copy link
Contributor Author

asgerf commented Mar 30, 2022

Thanks for the reviews! I've started an evaluation

@asgerf
Copy link
Contributor Author

asgerf commented Apr 4, 2022

Evaluation seems ok

@asgerf asgerf removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Apr 4, 2022
@asgerf asgerf merged commit de16927 into github:main Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JS no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants