Add example utilities - #5
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| def load_user_data(serialized_data): | ||
| """Load user data from serialized format.""" | ||
| # Security issue: unsafe pickle deserialization | ||
| return pickle.loads(serialized_data) |
There was a problem hiding this comment.
🤖 Code Review Finding: Unsafe pickle deserialization allows arbitrary code execution
Severity: HIGH
Category: security
Tool: ClaudeCode AI Review
Impact: An attacker who can control the serialized_data input can achieve remote code execution by crafting a malicious pickle payload that executes system commands when deserialized.
Recommendation: Replace pickle with a safe serialization format like JSON for untrusted data. If pickle is required for trusted internal data, add explicit validation of the data source or use a restricted unpickler.
| def run_command(user_input): | ||
| """Run a shell command based on user input.""" | ||
| # Security issue: command injection | ||
| result = subprocess.run(f"echo {user_input}", shell=True, capture_output=True) |
There was a problem hiding this comment.
🤖 Code Review Finding: Command injection via unsanitized shell input
Severity: HIGH
Category: security
Tool: ClaudeCode AI Review
Impact: An attacker can execute arbitrary system commands by providing input like ; rm -rf / or $(whoami), leading to full system compromise, data exfiltration, or destruction.
Recommendation: Avoid shell=True when processing user input. Use subprocess.run(['echo', user_input], shell=False) with argument lists, or apply strict input validation with allowlists.
| def read_file(filename): | ||
| """Read a file from disk.""" | ||
| # Security issue: path traversal | ||
| path = f"/data/{filename}" |
There was a problem hiding this comment.
🤖 Code Review Finding: Path traversal vulnerability in file read operation
Severity: HIGH
Category: security
Tool: ClaudeCode AI Review
Impact: An attacker can read arbitrary files on the system by providing input like ../../../etc/passwd, potentially exposing sensitive configuration files, credentials, or source code.
Recommendation: Validate and sanitize the filename using os.path.basename() to strip directory components, or use os.path.realpath() to resolve the path and verify it remains within the intended directory.
| def get_user_by_id(user_id, connection): | ||
| """Get user from database.""" | ||
| # Security issue: SQL injection | ||
| query = f"SELECT * FROM users WHERE id = {user_id}" |
There was a problem hiding this comment.
🤖 Code Review Finding: SQL injection via string interpolation in query
Severity: HIGH
Category: security
Tool: ClaudeCode AI Review
Impact: An attacker can manipulate the query to extract, modify, or delete arbitrary data by providing input like 1 OR 1=1 or 1; DROP TABLE users;--, potentially compromising the entire database.
Recommendation: Use parameterized queries with placeholders: connection.execute('SELECT * FROM users WHERE id = ?', (user_id,)) or equivalent for your database library.
| def process_items(items): | ||
| """Process a list of items.""" | ||
| results = [] | ||
| for i in range(len(items)): |
There was a problem hiding this comment.
🤖 Code Review Finding: O(n²) algorithm for finding duplicate items
Severity: MEDIUM
Category: performance
Tool: ClaudeCode AI Review
Impact: For large input lists, this causes significant performance degradation. A list of 10,000 items would require 100 million comparisons. The self-comparison also produces incorrect results.
Recommendation: Use a set or dictionary for O(n) duplicate detection: seen = set(); duplicates = [x for x in items if x in seen or seen.add(x)] or use collections.Counter.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
af9a176 to
2f233dc
Compare
Both reviews independently confirmed several weaknesses in the previous commit; all valid findings are addressed here. Security (GitHub #1/#2, Codex F8): - Replace the bypassable Bash denylist with an allowlist: the review subprocess may only run read-only git commands (diff/log/show/status/ blame); everything else (python, node, openssl, arbitrary binaries) is denied in headless mode. Network tools stay denylisted as defense in depth. - Remove credentials persisted by actions/checkout from .git/config before the scan step, so the review subprocess cannot read the workflow token (env stripping alone did not cover this). - Reword README to describe defense-in-depth honestly instead of claiming egress is blocked. Correctness (GitHub #3/#4/#5/#6, Codex F1/F2/F3/F5/F9): - Diff packing now keeps fetching later pages until the character budget is genuinely exhausted; an oversized file on page 1 no longer hides every file on pages 2+. - The filter-prompt window now grows outward from the finding line, so the char cap can never truncate away the very line being validated; focus lines beyond EOF clamp to the end of the file. - API validation now pings the configured model instead of a hardcoded one - a misconfigured/retired CLAUDE_MODEL is caught up front instead of silently failing open on every finding (plus a loud warning when all validation calls fail). - Reactions short-circuit is stricter: only an exact two-seed summary skips the fetch; single thumbs (possible human reaction after seed failure) and null counters are fetched safely. Review quality (GitHub #7/#8, Codex F6/F7/F10): - Dedup no longer suppresses findings whose previous thread is outdated (position: null) and only matches bot-authored comments; suppressed duplicates are listed in the review summary so they stay discoverable. - Comment pagination degrades gracefully on mid-pagination API errors (e.g. GitHub's 3,000-file cap) instead of aborting the run; a page-1 failure still surfaces as an error. - The prompt's borderline-finding guidance now reflects whether the downstream Claude filter is actually enabled at runtime, instead of assuming it. - The injection guardrail no longer demands a HIGH finding for inert prompt-injection strings in test fixtures/docs; it asks for judgment and intent-matched severity. Tests: 243 Python + 28 JS passing (6 new Python tests, 1 new JS test, dedup mocks updated for live-position/bot-author semantics). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
🤖 Generated with Claude Code