Skip to content
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

matching keys and values in JavaScript objects #3279

Closed
ievans opened this issue Jun 4, 2021 · 9 comments · Fixed by #3310
Closed

matching keys and values in JavaScript objects #3279

ievans opened this issue Jun 4, 2021 · 9 comments · Fixed by #3310
Assignees
Labels
bug Something isn't working lang:javascript priority:medium user:external requested by someone outside of r2c

Comments

@ievans
Copy link
Member

ievans commented Jun 4, 2021

Reporting for an external user. When trying to match a structure like those found in package.json, such as:

{
  "name": "frontend",
  "version": "0.1.0",
  "private": true,
  "dependencies": {
    "@fortawesome/fontawesome-svg-core": "^1.2.32",
    "@fortawesome/free-regular-svg-icons": "^5.15.1",
  },
}

(1) The following matches only the first or last entry in the object:

{ ...,
  "dependencies": { 
    ...,
    $DEP_NAME: $DEP_VERSION,
    ...
  }
}

Desired behavior is that it returns a match for every key under "dependencies".

Playground url: https://semgrep.dev/s/2b25

Bonus bug, it finds nothing when in JSON mode. Playground url in JSON mode: https://semgrep.dev/s/XLXA

@ievans ievans added bug Something isn't working user:external requested by someone outside of r2c lang:javascript labels Jun 4, 2021
@aryx aryx self-assigned this Jun 7, 2021
@aryx
Copy link
Collaborator

aryx commented Jun 8, 2021

semgrep-core returns 22 matches, but the python wrapper reduces it to just one.

@aryx
Copy link
Collaborator

aryx commented Jun 8, 2021

It's because the Python wrapper removes findings that have the same range, even if they have different metavariable bindings. Not sure
why we do that, but we have been doing that for a long time.

aryx added a commit that referenced this issue Jun 8, 2021
This closes #3279

test plan:
test file included
and
semgrep --config ~/yy/tests/OTHER/rules/misc_field.yaml
~/yy/tests/OTHER/rules/misc_field.js

returns now 22 findings (same range though).
@aryx
Copy link
Collaborator

aryx commented Jun 8, 2021

This might be a big change for the user @ievans because we may report multiple times the same findings, with the exact same range.

@emjin
Copy link
Contributor

emjin commented Jun 8, 2021

aryx added a commit to semgrep/pfff that referenced this issue Jun 8, 2021
This may help semgrep/semgrep#3279

test plan:
see related PR in semgrep
aryx added a commit that referenced this issue Jun 8, 2021
This may help #3279

test plan:
test file included
aryx added a commit that referenced this issue Jun 8, 2021
This may help #3279

test plan:
test file included
@aryx
Copy link
Collaborator

aryx commented Jun 8, 2021

Note that the user should write the rule in a different way, where we match individual fields separately, with
a pattern-inside and pattern, but I just realized you could not write partial single field definition in a pattern
in JS (we could in JSON mode but not JS). This is partly because x: { stuff } is ambiguous and could also
be a statement (a label x followed by a block). But I think we can assume people prefer the partial field def so I've fixed that.

aryx added a commit that referenced this issue Jun 9, 2021
* [JS] support partial single field for semgrep

This may help #3279

test plan:
test file included

* changelog

* misc
@aryx
Copy link
Collaborator

aryx commented Jun 9, 2021

Ok the JSON part is fixed too: https://semgrep.dev/s/XLXA?version=develop

@aryx
Copy link
Collaborator

aryx commented Jun 9, 2021

Also here is an alternative solution to the issue: use a pattern-inside and another pattern to just match the deps.
https://semgrep.dev/s/0npB/?version=develop

@aryx
Copy link
Collaborator

aryx commented Jun 9, 2021

Works also in JS mode: https://semgrep.dev/s/KWGL/?version=develop

@aryx
Copy link
Collaborator

aryx commented Jun 9, 2021

@ievans do you have a slack link to the request from the external user? That way I can tell him workarounds.

aryx added a commit that referenced this issue Jun 29, 2021
…3310)

* Do not remove findings with same range but different metavariables

This closes #3279

test plan:
test file included
and
semgrep --config ~/yy/tests/OTHER/rules/misc_field.yaml
~/yy/tests/OTHER/rules/misc_field.js

returns now 22 findings (same range though).

* changelog

* use latest pfff
@IagoAbal IagoAbal mentioned this issue Jul 29, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working lang:javascript priority:medium user:external requested by someone outside of r2c
Development

Successfully merging a pull request may close this issue.

4 participants