Semgrep: Semgrep 0.20.0 is matching things that were not matched before or missing matches

Created on 20 Aug 2020  ·  14Comments  ·  Source: returntocorp/semgrep

Describe the bug

EXAMPLE missing match
Semgrep matches changes across versions for the same rule.

      - pattern: |
          if (password == $X) {
              ...
          }
      - pattern: |
          if ($X == password) {
              ...
          }

The above pattern is not working anymore.

To Reproduce
Working on 0.18.0: https://semgrep.dev/editor/?version=0.18.0
Failing on 0.20.0 https://semgrep.dev/Wep2/?version=0.20.0

Expected behavior
https://semgrep.dev/editor/?version=0.18.0

Screenshots
N/A

Environment
semgrep.dev

bug external-user
Source
picture ajinabraham

Most helpful comment

I think you are right. This has nothing to do with the regression. This came up as a bug opened by a user. I got confused.
Somehow this gives the impression that the rule exec stops on the first match.
Screenshot 2020-08-21 at 12 49 27 AM

picture ajinabraham on 21 Aug 2020
👍2

All 14 comments

possibly related to #1540 ?

ievans picture ievans on 20 Aug 2020

EXAMPLE extra matches.

why are there two matches? https://semgrep.dev/KdX5/
IIUC, pattern-either should exit on the first match?

ajinabraham picture ajinabraham on 20 Aug 2020

Notes:
This actually broke in 0.19.0:
https://semgrep.dev/Wep2/?version=0.19.0

minusworld picture minusworld on 20 Aug 2020

Looks potentially related to semgrep-core

$ pytest tests/e2e/test_issue1548.py           
====================================================================================================== test session starts ======================================================================================================
platform darwin -- Python 3.7.3, pytest-6.0.1, py-1.9.0, pluggy-0.13.1
benchmark: 3.2.3 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
rootdir: /Users/grayson/Documents/r2c/semgrep/semgrep, configfile: tox.ini
plugins: xdist-2.0.0, snapshot-0.4.2, benchmark-3.2.3, forked-1.3.0
collected 1 item                                                                                                                                                                                                                

tests/e2e/test_real_code_samples.py F                                                                                                                                                                                     [100%]

=========================================================================================================== FAILURES ============================================================================================================
________________________________________________________________________________________________________ test_issue_1548 ________________________________________________________________________________________________________

run_semgrep_in_tmp = <function _run_semgrep at 0x10a13bd08>, snapshot = <pytest_snapshot.plugin.Snapshot object at 0x10a422908>

    def test_issue_1548(run_semgrep_in_tmp, snapshot):
        snapshot.assert_match(
            run_semgrep_in_tmp(
                "rules/code_samples/issue_1548_missing_match.yaml", target_name="code_samples/issue_1548_missing_match.js"
            ),
>           "results.json",
        )

/Users/grayson/Documents/r2c/semgrep/semgrep/tests/e2e/test_real_code_samples.py:17: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <pytest_snapshot.plugin.Snapshot object at 0x10a422908>
value = '{\n  "errors": [],\n  "results": [\n    {\n      "check_id": "rules.code_samples.node_timing_attack",\n      "end": {...samples/issue_1548_missing_match.js",\n      "start": {\n        "col": 5,\n        "line": 55\n      }\n    }\n  ]\n}'
snapshot_name = 'results.json'

    def assert_match(self, value, snapshot_name):
        """
        Asserts that ``value`` equals the current value of the snapshot with the given ``snapshot_name``.

        If pytest was run with the --snapshot-update flag, the snapshot will instead be updated to ``value``.
        The test will fail if the value changed.

        :type value: str
        :type snapshot_name: str or Path
        """
        if not isinstance(value, text_type):
            raise TypeError('value must be {}'.format(text_type.__name__))

        snapshot_path = self._snapshot_path(snapshot_name)

        if snapshot_path.is_file():
            expected_value = snapshot_path.read_text()
        elif snapshot_path.exists():
            raise AssertionError('snapshot exists but is not a file: {}'.format(shorten_path(snapshot_path)))
        else:
            expected_value = None

        if self._snapshot_update:
            snapshot_path.parent.mkdir(parents=True, exist_ok=True)
            if expected_value is not None:
                if expected_value != value:
                    snapshot_path.write_text(value)
                    self._updated_snapshots.append(snapshot_path)
            else:
                snapshot_path.write_text(value)
                self._created_snapshots.append(snapshot_path)
        else:
            if expected_value is not None:
                # pytest diffs before version 5.4.0 required expected to be on the left hand side.
                expected_on_right = version.parse(pytest.__version__) >= version.parse("5.4.0")
                try:
                    if expected_on_right:
                        assert value == expected_value
                    else:
                        assert expected_value == value
                except AssertionError as e:
                    snapshot_diff_msg = str(e)
                else:
                    snapshot_diff_msg = None

                if snapshot_diff_msg:
                    snapshot_diff_msg = 'value does not match the expected value in snapshot {}\n{}'.format(
                        shorten_path(snapshot_path), snapshot_diff_msg)
>                   raise AssertionError(snapshot_diff_msg)
E                   AssertionError: value does not match the expected value in snapshot /Users/grayson/Documents/r2c/semgrep/semgrep/tests/e2e/snapshots/test_real_code_samples/test_issue_1548/results.json
E                   assert '{\n  "errors...    }\n  ]\n}' == '{\n  "errors...    }\n  ]\n}'
E                     Skipping 118 identical leading characters in diff, use -v to show
E                     -    "col": 17,
E                     ?           ^^
E                     +    "col": 2,
E                     ?           ^
E                     -         "line": 7
E                     ?                 ^...
E                     
E                     ...Full output truncated (260 lines hidden), use '-vv' to show

/Users/grayson/.local/share/virtualenvs/semgrep-pzyhF5IL/lib/python3.7/site-packages/pytest_snapshot/plugin.py:167: AssertionError
==================================================================================================== short test summary info ====================================================================================================
FAILED tests/e2e/test_real_code_samples.py::test_issue_1548 - AssertionError: value does not match the expected value in snapshot /Users/grayson/Documents/r2c/semgrep/semgrep/tests/e2e/snapshots/test_real_code_samples/test...
======================================================================================================= 1 failed in 1.66s =======================================================================================================
(semgrep) ➜  semgrep git:(8c19366) ✗ python3 -m semgrep --version                          
0.18.0
(semgrep) ➜  semgrep git:(8c19366) ✗ which semgrep-core
/usr/local/bin/semgrep-core
(semgrep) ➜  semgrep git:(8c19366) ✗ semgrep-core -version                                                                             
semgrep-core version: v0.20.0-0-g1e89dcc-dirty, pfff: 0.38
(semgrep) ➜  semgrep git:(8c19366) ✗
minusworld picture minusworld on 21 Aug 2020 
(semgrep) ➜  semgrep git:(8c19366) ✗ pytest tests/e2e/test_issue1548.py
========================================================= test session starts ==========================================================
platform darwin -- Python 3.7.3, pytest-6.0.1, py-1.9.0, pluggy-0.13.1
benchmark: 3.2.3 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
rootdir: /Users/grayson/Documents/r2c/semgrep/semgrep, configfile: tox.ini
plugins: xdist-2.0.0, snapshot-0.4.2, benchmark-3.2.3, forked-1.3.0
collected 1 item                                                                                                                       

tests/e2e/test_real_code_samples.py .                                                                                            [100%]

========================================================== 1 passed in 1.65s ===========================================================
(semgrep) ➜  semgrep git:(8c19366) ✗ semgrep-core -version                     
semgrep-core version: v0.18.0-0-g959717c, pfff: 0.38
minusworld picture minusworld on 21 Aug 2020

Broken in semgrep-core 0.19.0:

(semgrep) ➜  semgrep git:(8c19366) ✗ pytest tests/e2e/test_real_code_samples.py
========================================================= test session starts ==========================================================
platform darwin -- Python 3.7.3, pytest-6.0.1, py-1.9.0, pluggy-0.13.1
benchmark: 3.2.3 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
rootdir: /Users/grayson/Documents/r2c/semgrep/semgrep, configfile: tox.ini
plugins: xdist-2.0.0, snapshot-0.4.2, benchmark-3.2.3, forked-1.3.0
collected 1 item                                                                                                                       

tests/e2e/test_real_code_samples.py F                                                                                            [100%]

=============================================================== FAILURES ===============================================================
___________________________________________________________ test_issue_1548 ____________________________________________________________

run_semgrep_in_tmp = <function _run_semgrep at 0x10a305d08>, snapshot = <pytest_snapshot.plugin.Snapshot object at 0x10a5eb9e8>

    def test_issue_1548(run_semgrep_in_tmp, snapshot):
        snapshot.assert_match(
            run_semgrep_in_tmp(
                "rules/code_samples/issue_1548_missing_match.yaml", target_name="code_samples/issue_1548_missing_match.js"
            ),
>           "results.json",
        )

/Users/grayson/Documents/r2c/semgrep/semgrep/tests/e2e/test_real_code_samples.py:17: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <pytest_snapshot.plugin.Snapshot object at 0x10a5eb9e8>
value = '{\n  "errors": [],\n  "results": [\n    {\n      "check_id": "rules.code_samples.node_timing_attack",\n      "end": {...samples/issue_1548_missing_match.js",\n      "start": {\n        "col": 5,\n        "line": 55\n      }\n    }\n  ]\n}'
snapshot_name = 'results.json'

    def assert_match(self, value, snapshot_name):
        """
        Asserts that ``value`` equals the current value of the snapshot with the given ``snapshot_name``.

        If pytest was run with the --snapshot-update flag, the snapshot will instead be updated to ``value``.
        The test will fail if the value changed.

        :type value: str
        :type snapshot_name: str or Path
        """
        if not isinstance(value, text_type):
            raise TypeError('value must be {}'.format(text_type.__name__))

        snapshot_path = self._snapshot_path(snapshot_name)

        if snapshot_path.is_file():
            expected_value = snapshot_path.read_text()
        elif snapshot_path.exists():
            raise AssertionError('snapshot exists but is not a file: {}'.format(shorten_path(snapshot_path)))
        else:
            expected_value = None

        if self._snapshot_update:
            snapshot_path.parent.mkdir(parents=True, exist_ok=True)
            if expected_value is not None:
                if expected_value != value:
                    snapshot_path.write_text(value)
                    self._updated_snapshots.append(snapshot_path)
            else:
                snapshot_path.write_text(value)
                self._created_snapshots.append(snapshot_path)
        else:
            if expected_value is not None:
                # pytest diffs before version 5.4.0 required expected to be on the left hand side.
                expected_on_right = version.parse(pytest.__version__) >= version.parse("5.4.0")
                try:
                    if expected_on_right:
                        assert value == expected_value
                    else:
                        assert expected_value == value
                except AssertionError as e:
                    snapshot_diff_msg = str(e)
                else:
                    snapshot_diff_msg = None

                if snapshot_diff_msg:
                    snapshot_diff_msg = 'value does not match the expected value in snapshot {}\n{}'.format(
                        shorten_path(snapshot_path), snapshot_diff_msg)
>                   raise AssertionError(snapshot_diff_msg)
E                   AssertionError: value does not match the expected value in snapshot /Users/grayson/Documents/r2c/semgrep/semgrep/tests/e2e/snapshots/test_real_code_samples/test_issue_1548/results.json
E                   assert '{\n  "errors...    }\n  ]\n}' == '{\n  "errors...    }\n  ]\n}'
E                     Skipping 118 identical leading characters in diff, use -v to show
E                     -    "col": 17,
E                     ?           ^^
E                     +    "col": 2,
E                     ?           ^
E                     -         "line": 7
E                     ?                 ^...
E                     
E                     ...Full output truncated (260 lines hidden), use '-vv' to show

/Users/grayson/.local/share/virtualenvs/semgrep-pzyhF5IL/lib/python3.7/site-packages/pytest_snapshot/plugin.py:167: AssertionError
======================================================= short test summary info ========================================================
FAILED tests/e2e/test_real_code_samples.py::test_issue_1548 - AssertionError: value does not match the expected value in snapshot /Us...
========================================================== 1 failed in 1.81s ===========================================================
(semgrep) ➜  semgrep git:(8c19366) ✗ semgrep-core -version                     
semgrep-core version: v0.19.0-0-geefc727, pfff: 0.38
(semgrep) ➜  semgrep git:(8c19366) ✗ semgrep --version    
0.18.0
minusworld picture minusworld on 21 Aug 2020

Working in this Docker image: https://semgrep.dev/Wep2/?version=64695a87331c56d0b95cc6032a5ef46fb442873d
Broken in this Docker image: https://semgrep.dev/Wep2/?version=3c82fe06833217620275220a0a633836485fb993

minusworld picture minusworld on 21 Aug 2020

Here's the commit: https://github.com/returntocorp/semgrep/commit/3c82fe06833217620275220a0a633836485fb993

minusworld picture minusworld on 21 Aug 2020

@aryx Would you mind taking a look at what might have caused this?

minusworld picture minusworld on 21 Aug 2020

It looks like we used to treat the following as equivalent:

if (cond)
  doSomething;

if (cond) {
  doSomething;
}

and we don't any longer

nbrahms picture nbrahms on 21 Aug 2020

I think this diff may be the culprit:

https://github.com/returntocorp/pfff/compare/dcea8f46d4f47c60f4f9dae553f733e57f5bc79e...53808c569065a62d09b7338de8e1b24661336385 at lang_js/analyze/ast_js_build.ml#337

cc. @aryx

nbrahms picture nbrahms on 21 Aug 2020

I am not sure if this got buried in the discussion. Is https://github.com/returntocorp/semgrep/issues/1548#issuecomment-677778551 related to the diff in pfff?

ajinabraham picture ajinabraham on 21 Aug 2020

I am not sure if this got buried in the discussion. Is #1548 (comment) related to the diff in pfff?

I think this is working as intended. I think pattern-either reports back all findings. At least, that's how I've understood it up until now.

minusworld picture minusworld on 21 Aug 2020

I think you are right. This has nothing to do with the regression. This came up as a bug opened by a user. I got confused.
Somehow this gives the impression that the rule exec stops on the first match.
Screenshot 2020-08-21 at 12 49 27 AM

ajinabraham picture ajinabraham on 21 Aug 2020
👍2
Was this page helpful?
0 / 5 - 0 ratings

Related issues

msorens picture msorens  ·  4Comments
ajinabraham picture ajinabraham  ·  6Comments
brendongo picture brendongo  ·  5Comments
DrewDennison picture DrewDennison  ·  4Comments
DrewDennison picture DrewDennison  ·  3Comments