Conversation
…lsrv over streamable-http, proxy as persistent shared gateway, Docker Compose orchestration
…cross-task anyio cancel-scope violation
… + full Docker stack with cascade verification, wipe volume between runs
Feature/policy engine
add: README file
📝 WalkthroughWalkthroughThe change moves MCP services and the proxy from stdio to streamable HTTP. It adds Docker Compose deployment, supervised upstream reconnection, HTTP-based integration tests, and expanded CI and attack-simulation validation. ChangesHTTP and Docker runtime
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant proxy
participant UpstreamConnection
participant MCPServer
MCPClient->>proxy: request tools or call a tool
proxy->>UpstreamConnection: route managed request
UpstreamConnection->>MCPServer: send MCP HTTP request
MCPServer-->>UpstreamConnection: return MCP result
UpstreamConnection-->>proxy: return result
proxy-->>MCPClient: return response
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (5)
proxy/upstream_connection.py (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the supported Streamable HTTP client API.
streamablehttp_clientis deprecated in MCP 1.28.1 and delegates tostreamable_http_client. Replace it with the supported API before the compatibility shim is removed.Proposed fix
-from mcp.client.streamable_http import streamablehttp_client +from mcp.client.streamable_http import streamable_http_client ... - streamablehttp_client(self.url) as (read, write, _get_session_id), + streamable_http_client(self.url) as (read, write, _get_session_id),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@proxy/upstream_connection.py` at line 13, Replace the deprecated streamablehttp_client import with the supported streamable_http_client API in the upstream connection setup, and update any references to the imported symbol accordingly.tools/run_attack_simulation.py (1)
98-109: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueClose the connection when a query raises.
verify_countsruns each query betweensqlite3.connectandconn.close(). If a table is missing,conn.executeraisesOperationalErrorand the connection leaks, and the caller loses the per-check context. Use awith contextlib.closing(...)block, or wrap each query and record the error as a failure message.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/run_attack_simulation.py` around lines 98 - 109, Update verify_counts to guarantee the SQLite connection is closed when any check query raises, using a contextlib.closing block or equivalent cleanup. Preserve per-check reporting by catching query errors and appending a failure message that includes the corresponding check message and exception details.tests/test_proxy_e2e.py (1)
21-38: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse an MCP handshake for the proxy readiness check.
wait_for_urlonly completes a TCP connect. uvicorn accepts connections before the Starlette lifespan finishes startingStreamableHTTPSessionManager, and the proxy upstream supervisors connect asynchronously. The subsequentsession.initialize()at line 82 can therefore fail intermittently in CI.tests/smoke_test.pypolls with a real MCPinitializecall; use that approach forPROXY_URLtoo, or retry the session setup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_proxy_e2e.py` around lines 21 - 38, Update wait_for_url to verify proxy readiness with a real MCP initialize handshake rather than only opening a TCP connection. Follow the established polling approach in smoke_test.py, retrying the session setup until timeout while preserving the existing timeout and final error behavior; ensure PROXY_URL is not considered ready until StreamableHTTPSessionManager and upstream supervisors are usable.tests/smoke_test.py (1)
75-80: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDrain and print the server output during cleanup.
The subprocess is started with
stdout=PIPEandstderr=STDOUT, but the pipe is never read. Two consequences follow. First, the server blocks if it writes more than the pipe buffer holds. Second, the test discards all server diagnostics when a call fails.tests/test_proxy_e2e.pyalready prints proxy output in itsfinallyblock; apply the same pattern here.♻️ Proposed cleanup
finally: proc.terminate() try: await asyncio.wait_for(proc.wait(), timeout=5) except TimeoutError: proc.kill() + + if proc.stdout: + output = (await proc.stdout.read()).decode(errors="replace") + print("\n=== server output ===") + print(output)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/smoke_test.py` around lines 75 - 80, Update the subprocess cleanup in the smoke test’s finally block to drain and print proc’s combined stdout/stderr, following the established pattern in test_proxy_e2e.py. Ensure output is consumed during cleanup while preserving the existing terminate, timeout wait, and kill fallback behavior.tests/test_rugpull_schema.py (1)
132-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDo not rely on
assertfor test failure in a standalone script.This module runs as
python tests/test_rugpull_schema.py, not under pytest. Python removesassertstatements when it runs with-O. Raise an explicit error, or callsys.exit(1), so the failure always propagates to CI.♻️ Proposed change
- assert desc is not None and "<system>" in desc, ( - f"proxy never picked up the poisoned description (last connection error: {last_error})" - ) + if desc is None or "<system>" not in desc: + raise RuntimeError( + f"proxy never picked up the poisoned description (last connection error: {last_error})" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_rugpull_schema.py` around lines 132 - 134, Replace the assert guarding the poisoned-description check with explicit failure handling that raises an error or exits with status 1 when desc is missing or lacks "<system>". Preserve the existing diagnostic message including last_error, and ensure the check remains effective when running tests/test_rugpull_schema.py directly or with Python optimization enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/schedule-attack-simulation.yml:
- Line 23: Move the schedule-attack-simulation workflow into the
.github/workflows directory, preserving its filename and existing configuration
so GitHub Actions loads and executes the scheduled attack simulation.
In @.github/workflows/ci.yml:
- Around line 9-14: Add a least-privilege permissions block to the test job
granting only read access to repository contents, and update the
actions/checkout@v4 step to disable credential persistence while preserving the
existing build and test flow.
- Around line 54-63: Update the “Wait for proxy to be reachable” step so that
exhausting all 30 attempts exits with a non-zero status and prints the proxy
logs before failing. Preserve the successful break path and existing retry
behavior.
In `@docker-compose.yml`:
- Around line 36-40: Update the ports mapping in the Docker Compose service to
bind port 8000 only to the host loopback interface instead of all interfaces,
preserving the existing container port and local development access.
In `@Dockerfile`:
- Around line 7-16: Update the Dockerfile to create a dedicated unprivileged
user, ensure /app/data exists and is writable by that user, then add a USER
instruction before the runtime command so vulnerable-server, lab-server-b, and
proxy run without root privileges.
In `@lab-server-b/mailserver.py`:
- Line 20: Correct the misspelled “subejct” label in the tool response returned
by the email-sending function, changing it to “subject” while preserving the
existing response format.
In `@proxy/proxy.py`:
- Around line 62-63: Update the upstream iteration in handle_list_tools to catch
timeout or connection failures from each conn.call invocation, emit an alert for
the affected server_name, and continue iterating so tools from connected
upstreams are still returned.
In `@README.md`:
- Around line 5-6: Update the scheduled workflow references in README.md,
including the badge link near the top and the repository-structure note, to use
the committed .github/schedule-attack-simulation.yml path instead of
scheduled-attack-simulation.yml.
- Around line 55-58: Update the Docker quickstart smoke-test instructions near
the `python tests/test_docker_stack.py` command to first install host test
dependencies with `python -m pip install -r requirements.txt`, then run the
existing test command.
In `@tests/test_proxy_e2e.py`:
- Around line 95-107: Prevent subprocess pipe backpressure in all three tests by
starting background drain tasks immediately after each relevant
create_subprocess_exec call: tests/test_proxy_e2e.py lines 95-107 for
filesrv_proc and proxy_proc, tests/smoke_test.py lines 75-80 for its server
process, and tests/test_rugpull_schema.py lines 136-144 for both processes,
including the filesrv restart at line 110. Collect drained output and print it
during each test’s finally cleanup instead of reading pipes only after
termination.
In `@tests/test_rugpull_schema.py`:
- Around line 26-43: Create a shared tests/harness.py containing wait_for_url
with the MCP-handshake readiness behavior from tests/smoke_test.py, plus
stop_proc, start_filesrv, the LOCAL_SERVERS_YAML template, and shared
REPO_ROOT/URL constants; update tests/test_rugpull_schema.py#L26-L43 and
tests/test_proxy_e2e.py#L21-L38 to import and use the shared helpers, removing
their local duplicates, and update tests/smoke_test.py#L14-L29 to use the shared
readiness helper.
- Around line 79-83: Set WATCHTOWER_CI_AUTO_APPROVE to true in the proxy_env
setup for the rug-pull proxy test, alongside the existing HOST, PORT, and
Watchtower configuration variables, so session.list_tools() cannot block waiting
for interactive approval.
In `@tools/run_attack_simulation.py`:
- Around line 59-76: Update the readiness loop around the curl invocation in the
simulation entry point so `up` is set only when the proxy responds successfully,
rather than merely when `result.stdout` is non-empty. Check curl’s return status
or require the HTTP status output to represent a successful response, while
preserving the existing retry, logging, and cleanup behavior.
- Around line 78-93: Ensure the test_result subprocess call in the main
simulation flow is covered by the existing teardown finally path so
TimeoutExpired also stops the Compose stack. Update the final docker compose
down command to include volume removal, matching the earlier teardown behavior
and ensuring watchtower-db is deleted after successful runs.
---
Nitpick comments:
In `@proxy/upstream_connection.py`:
- Line 13: Replace the deprecated streamablehttp_client import with the
supported streamable_http_client API in the upstream connection setup, and
update any references to the imported symbol accordingly.
In `@tests/smoke_test.py`:
- Around line 75-80: Update the subprocess cleanup in the smoke test’s finally
block to drain and print proc’s combined stdout/stderr, following the
established pattern in test_proxy_e2e.py. Ensure output is consumed during
cleanup while preserving the existing terminate, timeout wait, and kill fallback
behavior.
In `@tests/test_proxy_e2e.py`:
- Around line 21-38: Update wait_for_url to verify proxy readiness with a real
MCP initialize handshake rather than only opening a TCP connection. Follow the
established polling approach in smoke_test.py, retrying the session setup until
timeout while preserving the existing timeout and final error behavior; ensure
PROXY_URL is not considered ready until StreamableHTTPSessionManager and
upstream supervisors are usable.
In `@tests/test_rugpull_schema.py`:
- Around line 132-134: Replace the assert guarding the poisoned-description
check with explicit failure handling that raises an error or exits with status 1
when desc is missing or lacks "<system>". Preserve the existing diagnostic
message including last_error, and ensure the check remains effective when
running tests/test_rugpull_schema.py directly or with Python optimization
enabled.
In `@tools/run_attack_simulation.py`:
- Around line 98-109: Update verify_counts to guarantee the SQLite connection is
closed when any check query raises, using a contextlib.closing block or
equivalent cleanup. Preserve per-check reporting by catching query errors and
appending a failure message that includes the corresponding check message and
exception details.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b12f26d-6ec2-4ce7-a7fe-c12ca0b7ac9b
📒 Files selected for processing (20)
.dockerignore.github/schedule-attack-simulation.yml.github/workflows/ci.ymlDockerfileREADME.mddocker-compose.ymllab-server-b/mailserver.pyproxy/proxy.pyproxy/servers.yamlproxy/storage.pyproxy/upstream_connection.pyrequirements.txttests/smoke_test.pytests/test_cascade.pytests/test_docker_stack.pytests/test_multi_server_routing.pytests/test_proxy_e2e.pytests/test_rugpull_schema.pytools/run_attack_simulation.pyvulnerable-server/server.py
💤 Files with no reviewable changes (2)
- tests/test_cascade.py
- tests/test_multi_server_routing.py
| run: pip install -r requirements.txt | ||
|
|
||
| - name: Run full attack simulation | ||
| - name: Run full attack simulation (self-contained scenarios + Docker stack) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List all workflow-shaped YAML files and show which live in .github/workflows.
fd -e yml -e yaml . .githubRepository: Jeanm2005/MCP-security-proxy
Length of output: 230
Move this workflow into .github/workflows.
GitHub only loads workflow files from .github/workflows/, so .github/schedule-attack-simulation.yml will not run the scheduled attack simulation. Rename or move the file to .github/workflows/schedule-attack-simulation.yml if the schedule is intended to execute.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/schedule-attack-simulation.yml at line 23, Move the
schedule-attack-simulation workflow into the .github/workflows directory,
preserving its filename and existing configuration so GitHub Actions loads and
executes the scheduled attack simulation.
| jobs: | ||
| test: | ||
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Set up Python | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: "3.12" | ||
|
|
||
| - name: Install dependencies | ||
| run: | | ||
| pip install -r requirements.txt | ||
|
|
||
| - name: Lint | ||
| run: ruff check . --output-format=github | ||
|
|
||
| - name: Run smoke test (direct to vulnerable server) | ||
| run: python tests/smoke_test.py | ||
|
|
||
| - name: Run end-to-end proxy test | ||
| env: | ||
| WATCHTOWER_CI_AUTO_APPROVE: "true" | ||
| run: python tests/test_proxy_e2e.py | ||
|
|
||
| - name: Run rug-pull schema-change test | ||
| env: | ||
| WATCHTOWER_CI_AUTO_APPROVE: "true" | ||
| run: python tests/test_rugpull_schema.py | ||
|
|
||
| - name: Run cascade detection test | ||
| env: | ||
| WATCHTOWER_CI_AUTO_APPROVE: "true" | ||
| run: python tests/test_cascade.py | ||
| - name: Verify detection actually fired (fail build if not) | ||
| run: | | ||
| python -c " | ||
| import sqlite3 | ||
| conn = sqlite3.connect('proxy/watchtower.db') | ||
| flagged_calls = conn.execute('SELECT COUNT(*) FROM calls WHERE flags IS NOT NULL').fetchone()[0] | ||
| desc_findings = conn.execute('SELECT COUNT(*) FROM description_findings').fetchone()[0] | ||
| rug_pulls = conn.execute(\"SELECT COUNT(*) FROM tool_fingerprints WHERE last_flag = 'rug_pull'\").fetchone()[0] | ||
| cascade_findings = conn.execute('SELECT COUNT(*) FROM cascade_findings').fetchone()[0] | ||
| assert flagged_calls > 0, 'expected at least one flagged call, found none' | ||
| assert desc_findings > 0, 'expected at least one description finding, found none' | ||
| assert rug_pulls > 0, 'expected at least one rug-pull detection, found none' | ||
| assert cascade_findings > 0, 'expected at least one cascade finding, found none' | ||
| print(f'OK: {flagged_calls} flagged calls, {desc_findings} description findings, {rug_pulls} rug pulls, {cascade_findings} cascade findings') | ||
| " No newline at end of file | ||
| test: | ||
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Set an explicit least-privilege permissions block.
The job inherits the repository default GITHUB_TOKEN permissions. This workflow only builds and tests, so it needs read access to contents. Also disable credential persistence in the checkout step, because later steps run project code and Docker builds in the same workspace.
🔒 Proposed change
jobs:
test:
runs-on: ubuntu-latest
+ permissions:
+ contents: read
steps:
- uses: actions/checkout@v4
+ with:
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| jobs: | |
| test: | |
| runs-on: ubuntu-latest | |
| steps: | |
| - uses: actions/checkout@v4 | |
| - name: Set up Python | |
| uses: actions/setup-python@v5 | |
| with: | |
| python-version: "3.12" | |
| - name: Install dependencies | |
| run: | | |
| pip install -r requirements.txt | |
| - name: Lint | |
| run: ruff check . --output-format=github | |
| - name: Run smoke test (direct to vulnerable server) | |
| run: python tests/smoke_test.py | |
| - name: Run end-to-end proxy test | |
| env: | |
| WATCHTOWER_CI_AUTO_APPROVE: "true" | |
| run: python tests/test_proxy_e2e.py | |
| - name: Run rug-pull schema-change test | |
| env: | |
| WATCHTOWER_CI_AUTO_APPROVE: "true" | |
| run: python tests/test_rugpull_schema.py | |
| - name: Run cascade detection test | |
| env: | |
| WATCHTOWER_CI_AUTO_APPROVE: "true" | |
| run: python tests/test_cascade.py | |
| - name: Verify detection actually fired (fail build if not) | |
| run: | | |
| python -c " | |
| import sqlite3 | |
| conn = sqlite3.connect('proxy/watchtower.db') | |
| flagged_calls = conn.execute('SELECT COUNT(*) FROM calls WHERE flags IS NOT NULL').fetchone()[0] | |
| desc_findings = conn.execute('SELECT COUNT(*) FROM description_findings').fetchone()[0] | |
| rug_pulls = conn.execute(\"SELECT COUNT(*) FROM tool_fingerprints WHERE last_flag = 'rug_pull'\").fetchone()[0] | |
| cascade_findings = conn.execute('SELECT COUNT(*) FROM cascade_findings').fetchone()[0] | |
| assert flagged_calls > 0, 'expected at least one flagged call, found none' | |
| assert desc_findings > 0, 'expected at least one description finding, found none' | |
| assert rug_pulls > 0, 'expected at least one rug-pull detection, found none' | |
| assert cascade_findings > 0, 'expected at least one cascade finding, found none' | |
| print(f'OK: {flagged_calls} flagged calls, {desc_findings} description findings, {rug_pulls} rug pulls, {cascade_findings} cascade findings') | |
| " | |
| \ No newline at end of file | |
| test: | |
| runs-on: ubuntu-latest | |
| steps: | |
| - uses: actions/checkout@v4 | |
| jobs: | |
| test: | |
| runs-on: ubuntu-latest | |
| permissions: | |
| contents: read | |
| steps: | |
| - uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.29.0)
[warning] 14-14: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 9 - 14, Add a least-privilege
permissions block to the test job granting only read access to repository
contents, and update the actions/checkout@v4 step to disable credential
persistence while preserving the existing build and test flow.
Source: Linters/SAST tools
| - name: Wait for proxy to be reachable | ||
| run: | | ||
| for i in $(seq 1 30); do | ||
| if curl -s -o /dev/null http://localhost:8000/mcp; then | ||
| echo "proxy is up" | ||
| break | ||
| fi | ||
| echo "waiting for proxy... ($i/30)" | ||
| sleep 1 | ||
| done |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Fail the step when the proxy never becomes reachable.
The loop breaks on success, but it exits normally after 30 attempts when the proxy stays down. The step then reports success, and the next step fails with an unclear error. Exit non-zero after the loop, and print the proxy logs.
🐛 Proposed fix
- name: Wait for proxy to be reachable
run: |
for i in $(seq 1 30); do
if curl -s -o /dev/null http://localhost:8000/mcp; then
echo "proxy is up"
- break
+ exit 0
fi
echo "waiting for proxy... ($i/30)"
sleep 1
done
+ echo "proxy never became reachable"
+ exit 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 54 - 63, Update the “Wait for proxy to
be reachable” step so that exhausting all 30 attempts exits with a non-zero
status and prints the proxy logs before failing. Preserve the successful break
path and existing retry behavior.
| HOST: "0.0.0.0" | ||
| PORT: "8000" | ||
| WATCHTOWER_DB_PATH: "/app/data/watchtower.db" | ||
| ports: | ||
| - "8000:8000" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Do not publish an unauthenticated MCP endpoint on all interfaces.
Line 40 publishes the proxy on every host interface. The supplied proxy has no authentication middleware, and tests/test_docker_stack.py shows that clients can invoke filesrv__read_secret_file. An untrusted network client can therefore invoke every policy-permitted tool.
Bind the development endpoint to loopback by default. If remote access is required, add authentication, TLS, and network access controls before publishing it.
Proposed fix for local deployment
ports:
- - "8000:8000"
+ - "127.0.0.1:8000:8000"Docker publishes a port without a host IP on all interfaces. (docs.docker.com)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| HOST: "0.0.0.0" | |
| PORT: "8000" | |
| WATCHTOWER_DB_PATH: "/app/data/watchtower.db" | |
| ports: | |
| - "8000:8000" | |
| HOST: "0.0.0.0" | |
| PORT: "8000" | |
| WATCHTOWER_DB_PATH: "/app/data/watchtower.db" | |
| ports: | |
| - "127.0.0.1:8000:8000" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docker-compose.yml` around lines 36 - 40, Update the ports mapping in the
Docker Compose service to bind port 8000 only to the host loopback interface
instead of all interfaces, preserving the existing container port and local
development access.
| FROM python:3.12-slim | ||
|
|
||
| WORKDIR /app | ||
|
|
||
| COPY requirements.txt . | ||
| RUN pip install --no-cache-dir -r requirements.txt | ||
|
|
||
| COPY vulnerable-server/ ./vulnerable-server/ | ||
| COPY lab-server-b/ ./lab-server-b/ | ||
| COPY proxy/ ./proxy/ No newline at end of file |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Run the services as a non-root user.
The image has no USER instruction. All three service processes run as root. A compromise in any service then has root permissions inside its container. Create an unprivileged user, make /app/data writable by that user, and switch users before the runtime command.
Proposed fix
COPY vulnerable-server/ ./vulnerable-server/
COPY lab-server-b/ ./lab-server-b/
COPY proxy/ ./proxy/
+
+RUN adduser --system watchtower \
+ && mkdir -p /app/data \
+ && chown -R watchtower:watchtower /app
+
+USER watchtower📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| FROM python:3.12-slim | |
| WORKDIR /app | |
| COPY requirements.txt . | |
| RUN pip install --no-cache-dir -r requirements.txt | |
| COPY vulnerable-server/ ./vulnerable-server/ | |
| COPY lab-server-b/ ./lab-server-b/ | |
| COPY proxy/ ./proxy/ | |
| FROM python:3.12-slim | |
| WORKDIR /app | |
| COPY requirements.txt . | |
| RUN pip install --no-cache-dir -r requirements.txt | |
| COPY vulnerable-server/ ./vulnerable-server/ | |
| COPY lab-server-b/ ./lab-server-b/ | |
| COPY proxy/ ./proxy/ | |
| RUN adduser --system watchtower \ | |
| && mkdir -p /app/data \ | |
| && chown -R watchtower:watchtower /app | |
| USER watchtower |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Dockerfile` around lines 7 - 16, Update the Dockerfile to create a dedicated
unprivileged user, ensure /app/data exists and is writable by that user, then
add a USER instruction before the runtime command so vulnerable-server,
lab-server-b, and proxy run without root privileges.
Source: Linters/SAST tools
| finally: | ||
| for proc in (proxy_proc, filesrv_proc): | ||
| proc.terminate() | ||
| try: | ||
| await asyncio.wait_for(proc.wait(), timeout=5) | ||
| except TimeoutError: | ||
| proc.kill() | ||
|
|
||
| print("=== lookup_user x5 (expect alerts to start at call #4) ===") | ||
| for i in range(5): | ||
| r = await session.call_tool("filesrv__lookup_user", {"username": "jdoe"}) | ||
| print(f" call #{i+1}: {r.content[0].text}") | ||
| proxy_output = (await proxy_proc.stdout.read()).decode(errors="replace") if proxy_proc.stdout else "" | ||
| print("\n=== proxy output ===") | ||
| print(proxy_output) | ||
|
|
||
| Path(servers_config_path).unlink(missing_ok=True) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unread subprocess pipes can block the servers in all three migrated tests. Each test starts servers with stdout=asyncio.subprocess.PIPE and stderr=STDOUT, and reads the pipe only after termination. If a server writes more than the pipe buffer holds, it blocks and the test hangs.
tests/test_proxy_e2e.py#L95-L107: start background drain tasks forfilesrv_procandproxy_procright aftercreate_subprocess_exec, then print the collected output in thefinallyblock.tests/smoke_test.py#L75-L80: drain the server pipe during the run and print the collected output in thefinallyblock.tests/test_rugpull_schema.py#L136-L144: drain both the filesrv and proxy pipes during the run, including across the filesrv restart at line 110.
📍 Affects 3 files
tests/test_proxy_e2e.py#L95-L107(this comment)tests/smoke_test.py#L75-L80tests/test_rugpull_schema.py#L136-L144
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_proxy_e2e.py` around lines 95 - 107, Prevent subprocess pipe
backpressure in all three tests by starting background drain tasks immediately
after each relevant create_subprocess_exec call: tests/test_proxy_e2e.py lines
95-107 for filesrv_proc and proxy_proc, tests/smoke_test.py lines 75-80 for its
server process, and tests/test_rugpull_schema.py lines 136-144 for both
processes, including the filesrv restart at line 110. Collect drained output and
print it during each test’s finally cleanup instead of reading pipes only after
termination.
| async def wait_for_url(url: str, timeout: float = 15.0) -> None: | ||
| from urllib.parse import urlparse | ||
|
|
||
| parsed = urlparse(url) | ||
| host, port = parsed.hostname, parsed.port | ||
|
|
||
| deadline = time.time() + timeout | ||
| last_error = None | ||
| while time.time() < deadline: | ||
| try: | ||
| _reader, writer = await asyncio.open_connection(host, port) | ||
| writer.close() | ||
| await writer.wait_closed() | ||
| return | ||
| except OSError as e: | ||
| last_error = e | ||
| await asyncio.sleep(0.3) | ||
| raise RuntimeError(f"{url} never came up: {last_error}") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
The HTTP test harness is duplicated across three test files. wait_for_url, stop_proc, the subprocess launch blocks, the LOCAL_SERVERS_YAML template, and the REPO_ROOT/URL constants are copied. The readiness logic has already diverged: tests/smoke_test.py polls with an MCP initialize call, while the other two tests only complete a TCP connect. Extract one shared helper module, for example tests/harness.py.
tests/test_rugpull_schema.py#L26-L43: importwait_for_url,stop_proc, andstart_filesrvfrom the shared helper and delete the local copies.tests/test_proxy_e2e.py#L21-L38: import the samewait_for_urland delete the local copy.tests/smoke_test.py#L14-L29: move the MCP-handshake readiness poll into the shared helper and use it as the single readiness implementation.
📍 Affects 3 files
tests/test_rugpull_schema.py#L26-L43(this comment)tests/test_proxy_e2e.py#L21-L38tests/smoke_test.py#L14-L29
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_rugpull_schema.py` around lines 26 - 43, Create a shared
tests/harness.py containing wait_for_url with the MCP-handshake readiness
behavior from tests/smoke_test.py, plus stop_proc, start_filesrv, the
LOCAL_SERVERS_YAML template, and shared REPO_ROOT/URL constants; update
tests/test_rugpull_schema.py#L26-L43 and tests/test_proxy_e2e.py#L21-L38 to
import and use the shared helpers, removing their local duplicates, and update
tests/smoke_test.py#L14-L29 to use the shared readiness helper.
| proxy_env = os.environ.copy() | ||
| proxy_env["HOST"] = "127.0.0.1" | ||
| proxy_env["PORT"] = "8000" | ||
| proxy_env["WATCHTOWER_SERVERS_CONFIG"] = servers_config_path | ||
| proxy_env["WATCHTOWER_DB_PATH"] = str(REPO_ROOT / "proxy" / "watchtower.db") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where the proxy consults WATCHTOWER_CI_AUTO_APPROVE and which operations gate on approval.
rg -n -C 6 'WATCHTOWER_CI_AUTO_APPROVE|auto_approve|approval' --glob '*.py'Repository: Jeanm2005/MCP-security-proxy
Length of output: 166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tracked Python files (first 200):"
git ls-files '*.py' | sed -n '1,200p'
echo
echo "Relevant test files:"
fd -a 'test_.*\.py$' . | sed -n '1,100p'
echo
echo "Search approval/auto words in tracked files:"
rg -n -i -C 4 'approv|auto|interactive|requires|requires approval|list_tools|tool description|description' . --glob '!*.md' --glob '!*.txt' --glob '!package-lock.json' --glob '!pnpm-lock.yaml' --glob '!yarn.lock' | sed -n '1,240p'
echo
echo "Files test_rugpull_schema.py and test_proxy_e2e.py exist:"
for f in tests/test_rugpull_schema.py tests/test_proxy_e2e.py; do
echo "--- $f"
if [ -f "$f" ]; then
wc -l "$f"
sed -n '1,140p' "$f" | cat -n
else
echo "MISSING"
fi
doneRepository: Jeanm2005/MCP-security-proxy
Length of output: 25024
Add auto-approval for the rug-pull proxy test.
This test does not run a tool requiring approval, but session.list_tools() can trigger the approved auto-path if the proxy reports a suspicious tool description. Since tests/test_proxy_e2e.py uses WATCHTOWER_CI_AUTO_APPROVE=true for its proxy, set the same env var in tests/test_rugpull_schema.py so this e2e test does not wait for interactive approval.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_rugpull_schema.py` around lines 79 - 83, Set
WATCHTOWER_CI_AUTO_APPROVE to true in the proxy_env setup for the rug-pull proxy
test, alongside the existing HOST, PORT, and Watchtower configuration variables,
so session.list_tools() cannot block waiting for interactive approval.
| print(" waiting for proxy to be reachable...") | ||
| up = False | ||
| for _ in range(30): | ||
| result = run( | ||
| ["curl", "-s", "-o", "/dev/null", "-w", "%{http_code}", "http://localhost:8000/mcp"], | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| if result.stdout.strip(): | ||
| up = True | ||
| break | ||
| time.sleep(1) | ||
|
|
||
| if not up: | ||
| print(" FAILED: proxy never became reachable") | ||
| run(["docker", "compose", "logs", "proxy"]) | ||
| run(["docker", "compose", "down", "-v"]) | ||
| return False |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The readiness poll always succeeds on the first attempt.
curl -w "%{http_code}" writes a status code to stdout even when the connection fails; it writes 000. The condition at line 67 tests only that stdout is non-empty, so up becomes True immediately and the loop never waits. The Docker test then runs against a proxy that is not ready.
Check the curl exit status, or compare the printed code against a success value.
🐛 Proposed fix
for _ in range(30):
result = run(
["curl", "-s", "-o", "/dev/null", "-w", "%{http_code}", "http://localhost:8000/mcp"],
capture_output=True,
text=True,
)
- if result.stdout.strip():
+ if result.returncode == 0 and result.stdout.strip() not in ("", "000"):
up = True
break
time.sleep(1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| print(" waiting for proxy to be reachable...") | |
| up = False | |
| for _ in range(30): | |
| result = run( | |
| ["curl", "-s", "-o", "/dev/null", "-w", "%{http_code}", "http://localhost:8000/mcp"], | |
| capture_output=True, | |
| text=True, | |
| ) | |
| if result.stdout.strip(): | |
| up = True | |
| break | |
| time.sleep(1) | |
| if not up: | |
| print(" FAILED: proxy never became reachable") | |
| run(["docker", "compose", "logs", "proxy"]) | |
| run(["docker", "compose", "down", "-v"]) | |
| return False | |
| print(" waiting for proxy to be reachable...") | |
| up = False | |
| for _ in range(30): | |
| result = run( | |
| ["curl", "-s", "-o", "/dev/null", "-w", "%{http_code}", "http://localhost:8000/mcp"], | |
| capture_output=True, | |
| text=True, | |
| ) | |
| if result.returncode == 0 and result.stdout.strip() not in ("", "000"): | |
| up = True | |
| break | |
| time.sleep(1) | |
| if not up: | |
| print(" FAILED: proxy never became reachable") | |
| run(["docker", "compose", "logs", "proxy"]) | |
| run(["docker", "compose", "down", "-v"]) | |
| return False |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/run_attack_simulation.py` around lines 59 - 76, Update the readiness
loop around the curl invocation in the simulation entry point so `up` is set
only when the proxy responds successfully, rather than merely when
`result.stdout` is non-empty. Check curl’s return status or require the HTTP
status output to represent a successful response, while preserving the existing
retry, logging, and cleanup behavior.
| test_result = run( | ||
| [sys.executable, "tests/test_docker_stack.py"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=60, | ||
| ) | ||
| ok = test_result.returncode == 0 | ||
| print(f" {'OK' if ok else 'FAILED (nonzero exit)'}") | ||
| if not ok: | ||
| print(test_result.stdout[-2000:]) | ||
| print(test_result.stderr[-2000:]) | ||
|
|
||
| DOCKER_DB_COPY_PATH.unlink(missing_ok=True) | ||
| run(["docker", "compose", "cp", "proxy:/app/data/watchtower.db", str(DOCKER_DB_COPY_PATH)]) | ||
| run(["docker", "compose", "logs", "proxy"]) | ||
| run(["docker", "compose", "down"]) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Two teardown defects: an uncaught timeout and a retained volume.
First, subprocess.run with timeout=60 raises TimeoutExpired. The failure path at lines 72-76 tears the stack down, but this call site has no try/finally. If tests/test_docker_stack.py hangs, the exception propagates and the Compose stack stays running on the CI runner.
Second, line 93 runs docker compose down without -v, while line 75 runs down -v. The named volume watchtower-db therefore survives a successful run. On the next run, verify_counts can read cascade_findings rows that a previous run wrote and report a pass even if detection is broken.
🐛 Proposed fix
- test_result = run(
- [sys.executable, "tests/test_docker_stack.py"],
- capture_output=True,
- text=True,
- timeout=60,
- )
- ok = test_result.returncode == 0
- print(f" {'OK' if ok else 'FAILED (nonzero exit)'}")
- if not ok:
- print(test_result.stdout[-2000:])
- print(test_result.stderr[-2000:])
-
- DOCKER_DB_COPY_PATH.unlink(missing_ok=True)
- run(["docker", "compose", "cp", "proxy:/app/data/watchtower.db", str(DOCKER_DB_COPY_PATH)])
- run(["docker", "compose", "logs", "proxy"])
- run(["docker", "compose", "down"])
-
- print()
- return ok
+ try:
+ test_result = run(
+ [sys.executable, "tests/test_docker_stack.py"],
+ capture_output=True,
+ text=True,
+ timeout=60,
+ )
+ ok = test_result.returncode == 0
+ print(f" {'OK' if ok else 'FAILED (nonzero exit)'}")
+ if not ok:
+ print(test_result.stdout[-2000:])
+ print(test_result.stderr[-2000:])
+ except subprocess.TimeoutExpired:
+ print(" FAILED: docker stack test timed out")
+ ok = False
+
+ DOCKER_DB_COPY_PATH.unlink(missing_ok=True)
+ run(["docker", "compose", "cp", "proxy:/app/data/watchtower.db", str(DOCKER_DB_COPY_PATH)])
+ run(["docker", "compose", "logs", "proxy"])
+ run(["docker", "compose", "down", "-v"])
+
+ print()
+ return ok📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test_result = run( | |
| [sys.executable, "tests/test_docker_stack.py"], | |
| capture_output=True, | |
| text=True, | |
| timeout=60, | |
| ) | |
| ok = test_result.returncode == 0 | |
| print(f" {'OK' if ok else 'FAILED (nonzero exit)'}") | |
| if not ok: | |
| print(test_result.stdout[-2000:]) | |
| print(test_result.stderr[-2000:]) | |
| DOCKER_DB_COPY_PATH.unlink(missing_ok=True) | |
| run(["docker", "compose", "cp", "proxy:/app/data/watchtower.db", str(DOCKER_DB_COPY_PATH)]) | |
| run(["docker", "compose", "logs", "proxy"]) | |
| run(["docker", "compose", "down"]) | |
| try: | |
| test_result = run( | |
| [sys.executable, "tests/test_docker_stack.py"], | |
| capture_output=True, | |
| text=True, | |
| timeout=60, | |
| ) | |
| ok = test_result.returncode == 0 | |
| print(f" {'OK' if ok else 'FAILED (nonzero exit)'}") | |
| if not ok: | |
| print(test_result.stdout[-2000:]) | |
| print(test_result.stderr[-2000:]) | |
| except subprocess.TimeoutExpired: | |
| print(" FAILED: docker stack test timed out") | |
| ok = False | |
| DOCKER_DB_COPY_PATH.unlink(missing_ok=True) | |
| run(["docker", "compose", "cp", "proxy:/app/data/watchtower.db", str(DOCKER_DB_COPY_PATH)]) | |
| run(["docker", "compose", "logs", "proxy"]) | |
| run(["docker", "compose", "down", "-v"]) | |
| print() | |
| return ok |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/run_attack_simulation.py` around lines 78 - 93, Ensure the test_result
subprocess call in the main simulation flow is covered by the existing teardown
finally path so TimeoutExpired also stops the Compose stack. Update the final
docker compose down command to include volume removal, matching the earlier
teardown behavior and ensuring watchtower-db is deleted after successful runs.
1 participant
Summary by CodeRabbit