Release 0.10.0: close injection vector, confine writes, cut tools/list 66% - #49
Conversation
Security:
- `packages` was an injection vector. execute_r_analysis interpolated
caller strings straight into library(...) lines and validate_r_code
never saw them, so "stats); system('...'); library(utils" executed
arbitrary R past the allowlist, the dangerous-pattern block and every
approval category. Entries must now be bare R identifiers and pass the
allowlist or session approval.
- File writes are confined to the VFS roots via a new
VFS.validate_write_path(), called by the three fileops writers and the
six visualization tools before the path reaches R. VFS.write_file had
no production callers, so the VFS had never enforced anything.
Fails open with no VFS configured, so embedders are unaffected.
- approve_operation grants write access to the requested directory
instead of setting vfs.read_only = False for the whole process.
Correctness:
- `rmcp serve` never configured structlog, whose default factory writes
to stdout, corrupting the stdio JSON-RPC stream.
- R timeouts left the subprocess running: asyncio.wait_for sat inside a
TaskGroup, so the timeout arrived as a BaseExceptionGroup that the
sibling handler could not catch and proc.kill() never ran.
- The module-level asyncio.Semaphore bound to whichever event loop first
made a caller wait, then raised "bound to a different event loop"
everywhere else. One limiter per running loop now.
- load_example returned a fallback dict missing required output fields,
so every real error surfaced as "'statistics' is a required property".
- execute_r_analysis's timeout_seconds was declared and ignored.
- CORS was always "*": configured origins were never forwarded, and the
localhost wildcards are unmatchable by allow_origins regardless. They
compile to allow_origin_regex now.
- Approvals live on LifespanState, not the per-request Context, so they
survive to the next request as documented. approve_operation and
approve_r_package are now registered and reachable at all.
- Oversized results are capped at 32 retained entries, and an expired
rmcp://data/{id} link explains itself instead of raising KeyError.
Context:
- tools/list is down ~66%, 103,939 -> ~35,500 chars, while gaining two
tools. outputSchema is no longer advertised (54% of the payload;
results are still validated against it) and the 46 templated
descriptions are one-liners. The initialize instructions blob went
2,789 -> 922 chars and stopped duplicating the catalogue.
Removed:
- write_csv's `append`: write.csv refuses it and truncates, so the
schema promised behaviour that silently lost data.
- ~1,700 lines never wired into any execution path: package_tiers.py,
package_security.py (the documented "4-tier security system"),
discovery.py, r_session.py.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Several tests wrapped their body in `except Exception` and returned False. AssertionError is an Exception and pytest discards return values, so those tests passed no matter what they computed. Injecting `assert False` into test_business_analyst_scenario confirmed it passed. - test_realistic_scenarios.py: drop the swallowing wrapper from all four tests, and delete the dead run_all_scenarios() manual runner, which depended on the return values and printed "ALL SCENARIOS PASSED!". - test_r_error_handling.py: narrow nine handlers from Exception to RExecutionError so assertions inside the try propagate. This surfaced a real wrong assertion: test_logistic_regression_separation_warning expected a `model_summary` key the tool has never returned. Replaced with a check of the inflated standard errors separation actually produces. - test_excel_plotting_scenarios.py: test_other_problematic_tools had no assert at all; it now collects failures and asserts on them. - test_all_tool_schemas.py: additionalProperties test asserted True in both branches. Its fixture was also invalid (passed `variables` when t_test requires `variable`), so it was failing on a missing required property and calling that a pass. Promote PytestReturnNotNoneWarning to an error. Note this only guards sync tests -- pytest-asyncio unwraps coroutines, so it would not have caught the four above. New cover: packages injection and allowlist bypass, per-loop semaphore behaviour across successive event loops, and large-data store eviction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Minor rather than patch: outputSchema left the wire, file writes are confined, write_csv's `append` is gone and two tools were added. The tool count was stated four ways -- 44 in `rmcp --help`, 52 in the PyPI description, 53 in the install docs, 54 in the READMEs. Counts rot and change no reader's decision, so they are dropped from the prose rather than corrected. Removed claims that were simply false: - RMCP_DISABLE_OPERATION_APPROVAL and RMCP_AUTO_APPROVE_FILE_OPERATIONS appear nowhere in the code. - context.approval_state.approve_category() does not exist; approvals live on LifespanState. - The validate_r_code example had the arguments reversed, awaited a sync function, and destructured a dict from a function returning a tuple. - `rmcp http` is not a command. - "Zero Skipped Tests" -- there are 30+ dependency-gated skips. - The "Secure" feature bullet advertised the permission tiers deleted in 166f33f. It now names the guardrails that exist and says plainly they guard against mistakes, not adversaries, since RMCP runs R as the invoking user. Also: broken references (CONTRIBUTING.md, tests/e2e/, black), Python 3.10+ against a >=3.11 floor, README package counts contradicting get_package_categories(), a stray committed "# Test comment", and an `echo ... | rmcp start` diagnostic that CHANGELOG 0.9.1 documents as a broken pattern. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
docker/metadata-action only populates {{branch}} for branch pushes. On a
pull_request it is empty, so `type=sha,prefix={{branch}}-` produced the
tag "-<sha>", which docker rejects:
ERROR: failed to build: invalid tag
"ghcr.io/finite-sample/rmcp/rmcp-ci:-798eb64": invalid reference format
Latent since the tag template was written; it only fires when a PR
touches a Docker-relevant path, so most PRs skip the build and never hit
it. The consequence is worse than a red X: r-testing, docker-scenarios
and production-container-tests all depend on this job, so an R-affecting
PR silently skips every R integration test.
Emit a valid pr-<number> tag on PRs and gate the {{branch}} sha tag to
non-PR events.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![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: 59f85f50ae
ℹ️ 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".
| vfs = getattr(self.lifespan, "vfs", None) | ||
| if vfs is None: | ||
| return None | ||
| return vfs.validate_write_path(path) |
There was a problem hiding this comment.
When a write target contains ~, validate_write_path() validates Path(path).resolve() but the writer tools discard that returned path and pass the original string to R. For example, with the repository as an allowed root, Python treats ~/output.csv as <repo>/~/output.csv, while R expands it to the process user's home directory; in read-write mode, or after granting the corresponding apparent directory, a named writer can therefore escape every allowed root. Replace the tool parameter with the validated absolute path (or otherwise normalize using R-compatible expansion) before starting R.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| context._approved_operations[operation_type][specific_operation] = approval_data | ||
| approvals[operation_type][specific_operation] = approval_data |
There was a problem hiding this comment.
Once a file_operations approval is stored here, validate_r_code() authorizes the operation solely by function name, while execute_r_analysis() never calls require_write_path() for paths embedded in the R source. Thus approving write.csv or ggsave—even with a directory grant—allows a later script to write to any filesystem path accessible to the server process, bypassing the VFS confinement added to the named tools. File-operation approval needs to enforce the approved destination or run the subprocess in an actual filesystem sandbox.
Useful? React with 👍 / 👎.
| # Operation approvals granted by the user. Lives here rather than on Context | ||
| # because a fresh Context is built per request -- approvals must outlive it. | ||
| approved_operations: dict[str, dict[str, Any]] = field(default_factory=dict) | ||
| approved_packages: set[str] = field(default_factory=set) |
There was a problem hiding this comment.
On Streamable HTTP deployments with multiple clients or MCP sessions, LifespanState is shared by the entire server, so an approval granted by one client immediately authorizes package loads, file operations, and even system calls for every other client until restart. This contradicts the advertised session-only scope and creates a cross-client privilege leak; approvals need to be keyed by transport/session identity rather than stored in a single lifespan-wide map and set.
Useful? React with 👍 / 👎.
| "action": "denied", | ||
| "scope": "none", | ||
| "message": message, | ||
| "approved_operations": { | ||
| k: list(v.keys()) | ||
| for k, v in getattr(context, "_approved_operations", {}).items() | ||
| }, | ||
| "approved_operations": {k: list(v.keys()) for k, v in approvals.items()}, |
There was a problem hiding this comment.
If an operation was approved earlier in the server lifetime and is later called with action="deny", this branch reports success and action="denied" but never removes specific_operation from approvals; the returned approved_operations even continues to list it, and subsequent R code remains authorized. The analogous package-denial branch also leaves the package in approved_packages, so both denial paths should revoke any existing persistent approval.
Useful? React with 👍 / 👎.
1 participant
Three commits. The first is the one worth reading closely.
Security
packageswas an injection vector.execute_r_analysisinterpolated caller-supplied strings straight intolibrary(...)lines, andvalidate_r_codenever received them. Verified executing before the fix:That routed around the package allowlist,
DANGEROUS_PATTERNS, and everyOPERATION_CATEGORIESapproval gate. It mattered specifically because it holed the consent mechanism: a model refusedOPERATION_APPROVAL_NEEDEDforsystem(...)inr_codecould put it inpackagesand get no prompt. Entries must now be bare R identifiers and pass the allowlist or session approval.File writes are now confined to the VFS roots.
VFS.write_filehad zero production callers, so the VFS had never enforced anything against R — including forexecute_r_analysis, wheregrant_writewas bookkeeping only. The three fileops writers and six visualization tools now validatefile_pathbefore it reaches R. Fails open when no VFS is configured, so embedders are unaffected.approve_operationno longer disables read-only globally, granting write access to the requested directory instead.Correctness
rmcp servenever configured structlog, whose default factory writes to stdout — corrupting the stdio JSON-RPC stream.asyncio.wait_forsat inside aTaskGroup, so the timeout arrived as aBaseExceptionGroupthe sibling handler couldn't catch, andproc.kill()never ran.asyncio.Semaphorebound to whichever event loop first made a caller wait, then raised "bound to a different event loop" everywhere else. This was the cause of the long-standingtest_concurrent_requestsfailure — 10 requests minus 4 permits was exactly the 6 failures observed.load_examplereturned a fallback dict missing required output fields, so every real error surfaced as'statistics' is a required property. That was masking the bug above.timeout_secondswas declared and ignored; CORS was pinned to*; approvals didn't survive to the next request;approve_operation/approve_r_packagewere never registered at all.Context
tools/listis down ~66%, 103,939 → ~35,500 chars (~26k → ~9k tokens), while gaining two tools.outputSchemais no longer advertised — it was 54% of the payload and results are still validated against it server-side.Tests
Several tests wrapped their body in
except Exceptionand returnedFalse.AssertionErroris anExceptionand pytest discards return values, so they passed regardless of outcome — confirmed by injectingassert Falseintotest_business_analyst_scenarioand watching it pass. Fixing them surfaced a real wrong assertion:test_logistic_regression_separation_warningexpected amodel_summarykey the tool has never returned.279 passing, 0 failing (previously 255 passing with 1 failure).
Removed
write_csv'sappend— R'swrite.csvrefuses it and truncates, so the schema promised behaviour that silently lost data. Plus ~1,700 lines never wired into any execution path:package_tiers.py,package_security.py(the documented "4-tier security system"),discovery.py,r_session.py.Note on CI
python-publish.ymlhas noneeds:and no test gate, andci.ymldoesn't trigger on tags — so this PR is the only place CI validates these changes before they ship. Worth gating separately.🤖 Generated with Claude Code