-
Notifications
You must be signed in to change notification settings - Fork 389
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
Rule: OpenAI isConsequential flag set to false for state changing operation in OpenAPI spec #3446
Conversation
Hi @aabashkin, thanks for your contribution, this rule looks fine! if CI passes, we can merge this PR. |
Thanks, one unit test is failing, let me fix that and then we are good to go |
@0xDC0DE please review and approve thanks |
This rule is great as-is, and you can merge it if you want. But allow me to also suggest an improvement if you still feel up for it and want to improve your rule-writing skills 💪 Right now your rule marks a large block of code. You could narrow the marking to the specific line. You can always test this in the playground. (I wanted to add links, but it seems like there's a bug in the playground that messes up the yaml formatting when I save the rule, I've reported it to our engineers.) Use
This means you'll need to update the test syntax, because we are now marking the exact line of the
Even better would be narrowing your marking to the
Now that I've introduced metavariable-regex to you, you might see how we can simplify the
If you'v read till here, you've mastered some advanced Semgrep rule-writing techniques! 🎈 |
@aabashkin Do you want to update the rule, or should I merge the PR as is? |
@0xDC0DE sure I could update it. Do you think it would generate a performance benefit? |
@0xDC0DE I'm on vacation for the next 2 weeks, I will revisit this once I return |
@aabashkin I hope you had a great vacation! Let me know if you're still up to improve this rule, or I should merge it as is! |
@0xDC0DE thanks for the bump. I still plan to improve it. Standby 🙂 |
@aabashkin I am merging this and closing the PR. If you still want to make changes, feel free to open a new PR in the future. |
New Rule
Language: YAML
Technology: OpenAPI Spec (OAS), OpenAI
Description: OpenAI isConsequential flag set to false for state changing operation in OpenAPI spec