Reapply "Add rules for pytorch config best practices"#64
Reapply "Add rules for pytorch config best practices"#64GrosQuildu wants to merge 1 commit intomainfrom
Conversation
This reverts commit 8727fec.
GrosQuildu
left a comment
GrosQuildu
left a comment
There was a problem hiding this comment.
I would expand error message in all rules to describe what the problem is and what actions devs should take.
I would adjust categories, severities and impacts, as it seems we are overestimating/
And pls add tests: we can add artificial path to the included paths so tests can be detected. Also review CONTRIBUTING file for linting etc
| @@ -0,0 +1,18 @@ | |||
| rules: | |||
| - id: pytorch-allowed-urls | |||
| message: Allowing URLs via environment variables is enabled | |||
There was a problem hiding this comment.
Why is this bad? What is more secure alternative?
There was a problem hiding this comment.
We set impact to medium, but I don't see exploit scenario
There was a problem hiding this comment.
If it's not always exploitable maybe let's set category to best-practice?
There was a problem hiding this comment.
Agreed, category should be best practice here; that was my intention with this rule but I didn't realize it was an available option. I'll make this change — in fact I think it applies to some other rules in here as well.
| include: | ||
| - 'config.properties' | ||
| patterns: | ||
| - pattern-regex: | |
There was a problem hiding this comment.
There is a strange bug that this rule will find issue in the following:
inference_address=https://127.0.0.1:8443
job_queue_size=2
May be semgrep's issue we may report
2 participants
This reapplies commit 2d82231cd5caaabe4061db563135e15855dd4028.