Skip to content

JS: Add dedicated API graph label for the receiver, instead of parameter -1 #8523

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 23, 2022

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Mar 22, 2022

This changes the API graph label representing the receiver of call from parameter -1 to receiver.

The member predicates of API::Node are affected as follows:

  • getParameter(i) no longer has a result for i = -1, previously it returned the receiver.
  • getAParameter() no longer includes the receiver.
  • getLastParameter() no longer returns the receiver when the call has zero arguments. (🤦)

In theory this is not a behaviour-preserving change, but in practice it doesn't change much:

  • There are 90 uses of getParameter(i) outside ApiGraphs.qll, all of them unaffected due to how they restrict i.
  • There are 11 uses of getAParameter, some of which are affected, but as far as I can tell, none of them actually intended to get the receiver.
  • There are 3 uses of getLastParameter, which are affected in theory but unlikely to matter in practice (since the modeled function expects >=1 arguments), and none of them intended to get the receiver.

This was motivated by an upcoming change in MaD, where we want to use Argument[this] to represent the receiver (Argument[self] in Python/Ruby).

Evaluation looks good. A few spurious taint sources have been removed, which referred to the receiver argument to a callback.

@asgerf asgerf added JS Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish and removed Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish labels Mar 22, 2022
@asgerf asgerf marked this pull request as ready for review March 23, 2022 08:14
@asgerf asgerf requested a review from a team as a code owner March 23, 2022 08:14
@asgerf
Copy link
Contributor Author

asgerf commented Mar 23, 2022

@max-schaefer let me know if this is breaking anything

@max-schaefer
Copy link
Contributor

@max-schaefer let me know if this is breaking anything

I don't think I have any code that relies on this behaviour, so no objection from me!

@asgerf asgerf force-pushed the js/api-graph-receiver-label branch from aac4070 to f228570 Compare March 23, 2022 09:42
@asgerf
Copy link
Contributor Author

asgerf commented Mar 23, 2022

Rebased in an attempt to fix the seemingly unrelated PR check failure.

@codeql-ci codeql-ci merged commit ac29d5f into github:main Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants