Skip to content

Milestone 0: close the parser RCE, fix silent wrong answers, add a test net - #2

Merged
rudimk merged 9 commits into
mainfrom
claude/sisyphus-plans-review-p81sl3
Aug 3, 2026
Merged

rudimk merged 9 commits into
mainfrom
claude/sisyphus-plans-review-p81sl3

Conversation

@rudimk

@rudimk rudimk commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Reviews the Sisyphus plan against the code, then implements Milestone 0.

Why this is urgent

parse_expr eval()s its transformed input, and attribute access passes straight through. An ordinary-looking equation value reaches the Python object graph and runs arbitrary code. Verified end-to-end against the tool on main:

equation = 'sqrt.__globals__["__builtins__"]["__import__"]("os").getpid()'
→ {"solution": {"value": 7172.0}, ...}   # that's the server's PID

The SSE transport binds 0.0.0.0:4242 with no auth, so on an exposed instance this is pre-auth RCE, reachable from any prompt an LLM can be talked into emitting.

Every published v0.1.0 binary has this. They are still on the releases page. Cutting a replacement release is tracked in M6 and is not part of this PR.

The plan's prescribed remediation (size limit + restricted local_dict) was measured and does not work — local_dict alone and global_dict={} plus a curated local_dict both still execute the payload, because the escape is attribute traversal off whatever SymPy object is in scope, not name resolution. A size cap is moot; the payload is 60 characters.

What actually fixes it

guard.py parses the normalized code SymPy is about to evaluate and walks it against an AST allowlist — Attribute, Subscript, keyword args and non-whitelisted Call are rejected — then evaluates.

Ordering is load-bearing. An allowlist applied to the raw string rejects x^2 (a BitXor node) and 2x (a SyntaxError) — the very syntax LLMs emit and that normalization exists to accept. The pipeline is normalize → AST-guard → parse, and there is a test pinning that ordering.

Exponent towers and oversized exponents are rejected separately: 10**10**8 is pure arithmetic that any syntax-only filter passes, and it exhausts memory on eval.

Silent wrong answers

The a == 0 check only caught polynomials with no linear term, so anything with one was mis-solved without an error:

Input On main Here
x**2 + x = 4 1.5615… — one of two roots, with a fabricated linear derivation error: not_linear
x**3 - 2*x = 0 0.0, silently dropping ±sqrt(2) error: not_linear

Back-substitution cannot catch this — the returned root does satisfy the equation. Only a degree check does, so degree is now asserted before solving. This is the sharp edge of the guardrail the project is built on: verification proves soundness, not completeness.

Also fixed

  • Exact values. solution.value is the exact form as a string with float_value alongside, so 3*x = 1 gives "1/3" rather than 0.3333333333333333, and I*x = 1 returns "-I" instead of raising TypeError: Cannot convert complex to float.
  • Typed outcomes. solved | no_solution | infinite_solutions | error, discriminated by outcome. x = x + 1, x = x and an unknown variable were all IndexError: list index out of range before. Both envelope changes land together so clients break once, not twice.
  • Coherent steps. Generated from the operations actually performed, so a label can no longer contradict the equation printed beside it (2*x + 3 = 7 said "Add 4 to both sides"), and the symbolic-form line no longer renders SymPy's Eq(...) repr.
  • Parametric input. a*x + b = c raised TypeError: cannot determine truth value of Relational from a sign comparison in step generation. Sign now comes from SymPy's tri-state assumptions, and unknown sign gets neutral phrasing.
  • Wall-clock timeout. A thread cannot be cancelled, so the old executor could time out the caller but never the work. Solving runs in a reusable spawned worker that is killed and replaced on overrun. Verified inside the frozen PyInstaller binary — a 17s input with a 3s budget returned a timeout outcome in 4.7s with no fallback.
  • asyncio.to_thread replaces the deprecated get_event_loop pattern; logging is scoped to this package instead of setting the root logger to DEBUG; errors no longer forward raw Python exception text to clients.

Three things surfaced while implementing

  • pipenv install --dev silently upgraded fastmcp 2.3.3 → 3.4.5, a different API whose wheel resolves to a fastmcp_slim namespace package with no Client. Adding a test runner shouldn't ship a major runtime upgrade, so fastmcp is pinned to ==2.3.3 — what the committed lock already had. Upgrading is its own task.
  • The first timeout draft charged worker startup to the request budget, so a cold first request would time out at any sensible setting. Fixed with a readiness handshake; a test asserts a 2s budget survives a cold worker.
  • implicit_multiplication_application bundles split_symbols, which rewrites factorial(9) into 9*a**2*c*f*i*l*o*r*t and solves that. Quietly answering a different question is the exact failure this project exists to avoid, so it is replaced with plain implicit_multiplication; unknown function names are now parse errors. 2x and x^2 still normalize.

Tests

74 tests, green. Happy paths pin the full step list, not just the value — a suite that checks only value passes even when every label is wrong, which is how the mislabelling survived. tests/test_sandbox.py drives real worker processes and really kills them; a mocked worker would prove nothing about a mechanism that exists because the work can't be interrupted from inside.

One thing this PR does not deliver

The CI test job is written but not installed. Pushes touching .github/workflows/ need GitHub's workflow scope, which the authoring session's token lacked, and the contents API returns 404 for the same reason. It is parked at ci/test-workflow.yml with ci/README.md giving the two commands to move it into place, and the M0 checkbox is left unticked. Until someone runs that git mv, nothing runs the suite automatically — build.yml still triggers only on tag pushes.

Note on scope

README and docs/ example outputs were regenerated from the working code. That is nominally M7, but this PR changes the tool contract and leaving docs describing the removed schema would be worse than the scope creep. The false "non-linear input raises ValueError" claim in docs/ and CLAUDE.md was M0's to fix and is corrected.


Generated by Claude Code

rudimk added 4 commits August 3, 2026 06:56
Re-ran every factual claim in PROJECT_PLAN.md against the code and
revised the plan accordingly.

Critical:
- M0's input-security item understated the defect. parse_expr eval()s
  transformed input and attribute access passes through, so an equation
  string reaches the Python object graph and executes arbitrary code.
  With http-server bound to 0.0.0.0:4242 unauthenticated, that is
  pre-auth RCE. The remediation the plan prescribed (size limit +
  restricted local_dict) was verified not to block it; an AST-level
  allowlist was verified to block it while leaving normal math intact.
- The security tasks and the LLM-syntax task conflict: an AST allowlist
  applied to the raw string rejects x^2 and 2x. Recorded the required
  ordering (normalize -> AST-guard -> parse) rather than listing them as
  independent checkboxes.
- The plan, CLAUDE.md and docs/ all claim non-linear input raises. False
  whenever a linear term is present: x**2 + x = 4 returns one of two
  roots with a fabricated linear narrative. Back-substitution cannot
  catch this, so added a completeness guardrail and a degree check.

Corrected two claims the plan made that the code contradicts, both of
which had acceptance criteria built on them:
- sqrt(2) does not raise ValidationError; complex and parametric input
  are the real failures, and the parametric one is a step-generation bug
  that a schema change will not fix.
- The stale-example description was wrong about which values differ.

Also folded in: the discriminated union moves into M0 so the tool schema
breaks once rather than twice; M6 gains the test-gate task its acceptance
criterion already assumed; step-text assertions become an M0 acceptance
criterion; plus root-logger, error-leak and stale-action-version items.
Closes the code-execution vulnerability and the silent wrong-answer class,
replaces float coercion with exact values, and puts a test suite behind
all of it.

Security (guard.py, sandbox.py)
- parse_expr eval()s its transformed input and attribute access passes
  straight through, so an equation string reached the Python object graph
  and ran arbitrary code. Restricting the parser namespace does not stop
  this; the escape is attribute traversal off any SymPy object in scope.
  Input is now normalised with SymPy's own tokenizer, walked against an
  AST allowlist, and only then evaluated. Ordering matters: guarding the
  raw string would reject x^2 (BitXor) and 2x (SyntaxError), the syntax
  normalisation exists to accept.
- Exponent towers and oversized exponents are rejected, since 10**10**8
  is pure arithmetic that any syntax-only filter passes and that exhausts
  memory on eval. Expression size and complexity are bounded too.
- Solving runs in a reusable spawned worker process under a wall-clock
  timeout. A thread cannot be cancelled, so the previous executor could
  time out the caller but never the work. On overrun the worker is killed
  and the next request starts a fresh one. Verified in the frozen
  PyInstaller binary, which is where multiprocessing is most likely to
  misbehave.

Correctness (solver.py, schemas.py)
- The old a==0 check only caught polynomials with no linear term, so
  x**2 + x = 4 returned one of two roots and x**3 - 2*x = 0 dropped
  ±sqrt(2), each with a fabricated linear derivation. Back-substitution
  cannot catch this — the returned root satisfies the equation — so
  degree is now checked before solving and non-linear input is refused.
- Solutions are exact: value carries the SymPy form as a string with
  float_value alongside, so 1/3 stays 1/3 and complex results no longer
  raise TypeError.
- Output is a discriminated union tagged by outcome. No solution,
  infinitely many, and an unknown variable were all IndexError before.
- Steps are generated from the operations actually performed, so a label
  can no longer contradict the equation printed beside it, and the
  symbolic-form line no longer renders SymPy's Eq() repr.
- Step generation no longer branches on the sign of a possibly-symbolic
  quantity, which raised TypeError on a*x + b = c.
- asyncio.to_thread replaces the deprecated get_event_loop pattern;
  logging is scoped to this package instead of reconfiguring root; errors
  no longer forward raw Python exception text to clients.

Dependencies
- fastmcp pinned to ==2.3.3. Adding dev-packages forces a re-resolve and
  an unpinned spec jumps to 3.x, whose wheel resolves to a fastmcp_slim
  namespace package with no Client. The committed lock had always pinned
  this version; upgrading is separate work.

Also adds a CI job running the suite on push and PR with --dev deps, and
brings README, docs/ and CLAUDE.md in line with the new contract.
The job itself is ready: it runs `pipenv run test` on every push and PR
with `--dev` deps, which the build workflow's bare `pipenv install` would
skip. It cannot be committed under .github/workflows/ from this session —
GitHub rejects pushes that touch that path without the `workflow` scope,
and the contents API returns 404 for the same reason.

ci/README.md has the two commands to move it into place.
@rudimk
rudimk force-pushed the claude/sisyphus-plans-review-p81sl3 branch from c377d2b to 45a47ff Compare August 3, 2026 06:56
rudimk added 5 commits August 3, 2026 07:59
The job is live at .github/workflows/test-workflow.yml and reported its
first green run on PR #2 (both check runs succeeded). Ticks the M0
checkbox and drops the wording describing it as parked pending
workflow-scope access.

Records two follow-ups in M6, neither blocking:
- it triggers on both push and pull_request, so a branch in this repo
  runs the suite twice per push
- .github/workflows/README.md came across with the move and still says
  the job is awaiting installation
Narrowing the push trigger to main leaves pull_request as the only event
that fires for branch work, so a push now schedules one run instead of
two. Confirmed against the workflow run history: the two prior commits
each produced a push run and a pull_request run, and the first commit
under the new triggers produced a single pull_request run, green.
@rudimk
rudimk merged commit 070b62f into main Aug 3, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant