Skip to content

Triage the 96 CodeQL findings — path injection on the server surfaces first #23

Open

Description

@MSKazemi

Problem

The first CodeQL run on this codebase reported 96 open alerts: 27 high, 5 medium, 13 warning, 51 note. They are dominated by four rules:

Count Rule
27 py/empty-except
25 py/path-injection
12 py/implicit-string-concatenation-in-list
9 py/unused-import

Most of a first scan on an established codebase is noise. Some of it is not, and the job here is to separate the two rather than to close 96 alerts.

Where to start — and why

py/path-injection is the one that matters. AOBench builds filesystem paths from task and environment IDs. When that happens in a local CLI, the caller already has shell access and the finding is largely theoretical. But AOBench now also exposes:

  • a REST API (aobench serve rest)
  • an MCP server (aobench serve mcp)

On those surfaces the identifier arrives from a remote caller, and a path built from it without validation is a real traversal risk. Start by classifying each of the 25 by which surface can reach it.

py/empty-except is worth a look too — a swallowed exception in a scorer can silently turn a failure into a zero, which is a correctness problem in a benchmark, not just a style one.

Where to look

Path Why
Security tab → Code scanning The live alert list with line links
src/aobench/loaders/ Task and environment path construction
src/aobench/paths.py resolve_benchmark_root() — the corpus resolution logic
REST + MCP request handlers Where remote input enters

Implementation hints

  • Do not bulk-dismiss. Each alert gets one of three outcomes: fixed, dismissed with a written reason, or converted to its own issue. A dismissal with no reason is indistinguishable from not having looked.
  • The durable fix for path injection is to validate identifiers against the known corpus rather than sanitising strings — the set of valid task and environment IDs is finite and enumerable, so resolve against that set and reject anything not in it. That also gives better error messages (see Replace raw tracebacks with actionable errors when a task or environment ID doesn't exist #2).
  • Split into several PRs by rule. One PR per rule class is reviewable; one PR touching 96 sites is not.
  • For py/empty-except, prefer logging the exception over widening the catch, and never make a test pass by swallowing an error.

Acceptance criteria

  • All 25 py/path-injection alerts resolved or dismissed with a written justification
  • Path construction from remote-supplied identifiers validated against the known corpus
  • py/empty-except reviewed; any that hide a real failure fixed rather than silenced
  • Remaining note-level alerts either fixed or dismissed with reasons
  • No behaviour change to a valid run

Difficulty

Medium, and splittable. The path-injection subset alone is a good, self-contained contribution with real security value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area: infraBuild, CI, packaging, deploymenteffort: mediumRoughly a dayhelp wantedExtra attention is neededtype: tech-debtCleanup, typing, lint, flaky tests

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions