-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add a new linting rule to catch unsafe exec usage #11902
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 2e736b8 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
petebacondarwin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love a PREVENTS - This looks good so not blocking but are we missing possible other scenarios such as aliased imports?
| ], | ||
| }); | ||
|
|
||
| console.log("✅ All tests passed!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about testing cases where the imports have been aliased?
E.g.
import { execSync as run } from "node:child_process";There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And also namespaced imports?
import * as cp from "node:child_process";
vicb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
Thanks @emily-shen
NB this PR was written with LLM help, i've reviewed and tested manually but I'm also not the most familiar with eslint plugins.
Agreed with that. It is a specific syntax that we don't really need to take time to deeply understand. Skimming through the rule file AND adding a test as you did is great.
| }, | ||
| }, | ||
| { | ||
| files: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: what's the up of excluding those? execution time I guess? Does it make a huge difference? If not I would not exclude them for the reason I mentioned earlier offline: ideally there should be no vulnerable code anywhere to prevent avoid copy-pasting mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have quite a lot of 'vulnerable' code in the tests, and i figured this was okay since we couldn't copy paste vulnerable code around by mistake without the linter complaining, since any non-test code is covered by the rule
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our beloved AI models (and developers before when they were still a thing) learn from that so it would definitely be better to fix but maybe not worth the effort.
If there is a common pattern that is mechanical to update, ASTGrep would be the best tool, we can discuss about that offline.
4b83756 to
c1c0aa3
Compare
c1c0aa3 to
2e736b8
Compare
Fixes n/a
Add a custom eslint rule that checks for unsafe command execution.
NB this PR was written with LLM help, i've reviewed and tested manually but I'm also not the most familiar with eslint plugins.
Also, not really related to this PR but I've added AGENTS.local.md to gitignore, hopefully that's okay.
A picture of a cute animal (not mandatory, but encouraged)