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

chore: use deepok to mark lines as ok when run with the pro engine #2985

Merged
merged 2 commits into from
Jun 28, 2023

Conversation

emjin
Copy link
Contributor

@emjin emjin commented Jun 26, 2023

We have deepruleid to mark lines as matching only with the pro engine, but we didn't have anything to mark lines as ok only with the pro engine. deepok is intended for that.

This PR updates annotations to use deepok.

We have deepruleid to mark lines as matching only with the pro engine,
but we didn't have anything to mark lines as ok only with the pro
engine. deepok is intended for that.

This PR updates annotations to use deepok.
@emjin emjin requested a review from colleend June 26, 2023 21:00
@emjin
Copy link
Contributor Author

emjin commented Jun 26, 2023

Failing tests are validation errors, appear unrelated (they're probably due to Brandon's changes)

@colleend
Copy link
Contributor

yep, seems like the validation errors are unrelated and historical isn't passing bc of some type inference checks. Lgtm -- thanks emma!

@p4p3r
Copy link
Contributor

p4p3r commented Jun 27, 2023

I merged all the latest changes in develop (including Brandon's changes to regex metavars capture), but tests are still reported as failing. Is that expected?

@colleend
Copy link
Contributor

colleend commented Jun 27, 2023

nope, that's not expected -- I suspect that this is because ruleid: deepok: isn't recognized by the semgrep --test test script.

I can look into it for a bit 👍


edit: yep:

Could not parse         // ruleid: deepok: tainted-sql-string as a test annotation in file /Users/chai/Desktop/semgrep_repos/semgrep-rules/java/spring/security/injection/tainted-sql-string.java. Skipping this line
Could not parse         // ruleid: deepok: tainted-sql-string as a test annotation in file /Users/chai/Desktop/semgrep_repos/semgrep-rules/java/spring/security/injection/tainted-sql-string.java. Skipping this line

@emjin
Copy link
Contributor Author

emjin commented Jun 27, 2023

Oh no, --test must check it differently. I could change the format of this annotation if that would be the simplest way

emjin pushed a commit to semgrep/semgrep that referenced this pull request Jun 27, 2023
This will help solve:
semgrep/semgrep-rules#2985.

Adds parsing of notations like `ruleid: deepok: foobar` and `ruleid:
deepruleid: foobar`.

_To Test:_
Run `python -m semgrep --test
semgrep-rules/java/spring/security/injection`

PR checklist:

- [X] Purpose of the code is [evident to future
readers](https://semgrep.dev/docs/contributing/contributing-code/#explaining-code)
- [x] Tests included or PR comment includes a reproducible test plan
- [x] Documentation is up-to-date
- [x] A changelog entry was [added to
changelog.d](https://semgrep.dev/docs/contributing/contributing-code/#adding-a-changelog-entry)
for any user-facing change
- [x] Change has no security implications (otherwise, ping security
team)

If you're unsure about any of this, please see:

- [Contribution
guidelines](https://semgrep.dev/docs/contributing/contributing-code)!
- [One of the more specific guides located
here](https://semgrep.dev/docs/contributing/contributing/)
@colleend
Copy link
Contributor

passes develop now! I will merge :D

@colleend colleend merged commit b2b56f5 into develop Jun 28, 2023
@colleend colleend deleted the emma/introduce-deepok branch June 28, 2023 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants