fix: address ecosystem audit findings (security, compose, docs, validation)#46
Merged
Conversation
Security:
- Fleet key now grants limited 'fleet' role instead of full 'writer'
- Deploy endpoint requires owner role (not writer/fleet)
- First-run registration restricted to private/loopback networks
- XSS fix: fleet page uses data attributes instead of inline onclick
- Fleet bootstrap: entrypoint ensures /fleet dir is writable
Compose generator:
- Named volumes now declared at top-level volumes section
- ${VAR} placeholders escaped as $${VAR} to prevent host interpolation
Validation:
- Deploy API rejects requests with blank required env vars
- Android workers: containers array skipped (only apps processed)
Docs:
- Fleet docs rewritten to match actual REST heartbeat implementation
- Presearch/IPRoyal/Earn.fm/Repocket guide env vars aligned with YAML
- Earn.fm/Repocket service YAMLs aligned with collector auth models
Testing:
- Added requirements-dev.txt for local test reproducibility
Closes #cp-bj8p #cp-jbkg #cp-370w #cp-3ddy #cp-zvhs #cp-xt5k #cp-zrlr
Closes #cp-pkio #cp-v65p #cp-p2sb #cp-hvay #cp-6hr9 #cp-6ho7
📝 WalkthroughWalkthroughThis PR transitions the fleet from multi-method authentication (master key, join tokens, WebSocket) to a unified Bearer-token model with REST/HTTP communication, adds security restrictions to registration and deployment, updates Docker Compose generation for proper environment variable escaping, refactors worker removal UI with delegated handlers, and aligns all service documentation to the new architecture. ChangesFleet Architecture Redesign: REST-Based Auth, Security Hardening, and Docs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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: 7
🤖 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 `@app/main.py`:
- Around line 809-816: The validation currently treats whitespace-only values as
present; update the missing list comprehension in app/main.py to treat strings
with only whitespace as blank by checking the trimmed value, e.g. replace the
condition `if var.get("required") and not env.get(var["key"])` with something
like `if var.get("required") and not ( (env.get(var["key"]) or "").strip() )` so
fields like " " are considered missing; modify the list comprehension that
builds missing to use this trimmed check (referencing the variables missing,
docker_conf.get("env", []), and env.get(var["key"])).
- Around line 377-381: The current first-run check calls
ipaddress.ip_address(request.client.host) without guarding for request.client or
request.client.host being None, which will raise ValueError; update the is_first
block to first fetch host = getattr(request.client, "host", None) (or check
request.client and request.client.host) and only call ipaddress.ip_address(host)
if host is not None, wrapping the conversion in a try/except ValueError to treat
invalid/absent hosts as non-private and raise the same 403 HTTPException;
reference the is_first check and request.client.host when making this change.
- Around line 413-417: The first-run IP check in the shown block assumes
request.client.host exists and duplicates logic used in page_register; extract a
module-level helper (e.g., _validate_first_run_ip(request: Request)) that reads
request.client.host safely, raises HTTPException(403, "Client IP unknown") if
missing, parses the host with ipaddress.ip_address, and raises
HTTPException(403, "First-run setup only allowed from private networks") if not
loopback/private; then replace the inline logic in the first-run setup and in
page_register to call _validate_first_run_ip instead.
In `@docs/guides/earnfm.md`:
- Line 8: The doc currently contradicts the credential model by specifying
"email and password" in the intro while still referencing an "API token" later;
update the guide so all auth wording consistently reflects the new credential
model: replace any mentions of "API token" with the correct credential
terminology (e.g., "email and password" or whatever the new credential name is),
remove token-related setup steps or examples, and ensure the initial sentence
("Earn.fm pays you for sharing your internet bandwidth. It authenticates via
Supabase using email and password.") and the later section that mentions "API
token" use the same, accurate phrasing so users aren’t confused.
In `@docs/guides/repocket.md`:
- Line 8: The guide text for "Repocket" contains conflicting guidance and
outdated auth instructions; update the document so it consistently states that
only residential IPs are allowed (remove or reconcile any claim that
VPS/datacenter IPs are permitted) and replace references to API-token retrieval
with the current Firebase email/password authentication flow (update any steps
that mention getting an API token to instead show signing in via email/password
and how to supply credentials to the Docker image using environment variables).
Ensure the "Max 5 devices/5 active sessions" and "Docker image uses environment
variables directly" statements remain accurate and align with the revised auth
instructions.
In `@requirements-dev.txt`:
- Around line 1-4: Update the pytest constraint in requirements-dev.txt from
"pytest>=8.0" to "pytest>=9.0.3" to eliminate the tmpdir handling vulnerability
(CVE affecting pytest < 9.0.3); edit the line containing pytest in the file and
leave other entries (pytest-asyncio, pytest-cov, tzdata) unchanged unless you
choose to optionally bump them separately.
In `@services/bandwidth/repocket.yml`:
- Around line 9-11: The service metadata contains contradictory policy
statements about VPS/datacenter IPs (one line prohibits VPS/proxy/VPN while
another accepts VPS/datacenter IPs at lower rates); pick a single consistent
policy and update the YAML accordingly: either remove or change the line that
"accepts VPS/datacenter IPs at lower rates" to match the prohibition, or change
the prohibition to allow datacenter IPs with rate limits, and make the
short_description key reflect that chosen policy (e.g., update short_description
to state "no VPS/proxy/VPN" or "datacenter IPs allowed at reduced rates") so
both the policy body and short_description are consistent.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8acf747c-8145-48cd-990f-b2c1f7ed30a3
📒 Files selected for processing (14)
app/auth.pyapp/compose_generator.pyapp/fleet_key.pyapp/main.pyapp/templates/fleet.htmldocs/fleet.mddocs/guides/earnfm.mddocs/guides/iproyal.mddocs/guides/presearch.mddocs/guides/repocket.mdentrypoint.shrequirements-dev.txtservices/bandwidth/earnfm.ymlservices/bandwidth/repocket.yml
Comment on lines
+1
to
+4
| pytest>=8.0 | ||
| pytest-asyncio>=0.23 | ||
| pytest-cov>=5.0 | ||
| tzdata>=2024.1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check PyPI for latest versions and GitHub advisories for security issues
echo "=== Latest versions on PyPI ==="
for pkg in pytest pytest-asyncio pytest-cov tzdata; do
echo -n "$pkg: "
curl -s "https://pypi.org/pypi/$pkg/json" | jq -r '.info.version'
done
echo -e "\n=== Security advisories ==="
for pkg in pytest pytest-asyncio pytest-cov tzdata; do
echo "--- $pkg ---"
gh api graphql -f query="
{
securityVulnerabilities(first: 3, ecosystem: PIP, package: \"$pkg\") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}" | jq '.data.securityVulnerabilities.nodes'
doneRepository: GeiserX/CashPilot
Length of output: 545
Update pytest to >=9.0.3 to resolve a MODERATE severity tmpdir handling vulnerability.
Latest versions show:
pytest>=8.0allows versions vulnerable to tmpdir handling (CVE details: pytest < 9.0.3). Update topytest>=9.0.3.pytest-asyncio>=0.23andpytest-cov>=5.0have no known vulnerabilities, though newer versions exist (1.3.0 and 7.1.0 respectively).tzdata>=2024.1is fine—the constraint allows the current 2026.2 version with no advisories.
🤖 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 `@requirements-dev.txt` around lines 1 - 4, Update the pytest constraint in
requirements-dev.txt from "pytest>=8.0" to "pytest>=9.0.3" to eliminate the
tmpdir handling vulnerability (CVE affecting pytest < 9.0.3); edit the line
containing pytest in the file and leave other entries (pytest-asyncio,
pytest-cov, tzdata) unchanged unless you choose to optionally bump them
separately.
…ix SSRF, align docs - Extract _require_private_network() helper (removes duplication, handles None/ValueError) - Strip whitespace in required env var validation - Fix SSRF in api_worker_command by using _get_verified_worker_url() - Align earnfm/repocket docs with email/password auth model - Fix repocket policy contradiction (VPS accepted at lower rates)
…verage
- Fix _escape_interpolation to use regex (skips already-escaped $${})
- Add isNaN guard in fleet.html event delegation
- Add TestRequireRole covering fleet escalation boundaries
- Add TestEscapeInterpolation, TestIsNamedVolume, TestNamedVolumeDeclaration
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/test_auth.py`:
- Around line 129-131: The test named test_fleet_does_not_satisfy_fleet_directly
contradicts its assertion; update the test name and/or comment to reflect the
actual behavior (that require_role({"r": "fleet"}, "fleet") returns True).
Concretely, rename test_fleet_does_not_satisfy_fleet_directly to something like
test_fleet_satisfies_fleet_directly and change the comment to state that the
fleet role has the implicit writer grant and therefore satisfies the "fleet"
check when calling require_role; ensure references to require_role and the role
payload {"r": "fleet"} remain unchanged.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4f58f6c0-0836-4f93-8d92-42a79f5178a1
📒 Files selected for processing (4)
app/compose_generator.pyapp/templates/fleet.htmltests/test_auth.pytests/test_compose.py
🚧 Files skipped from review as they are similar to previous changes (2)
- app/templates/fleet.html
- app/compose_generator.py
Comment on lines
+129
to
+131
| def test_fleet_does_not_satisfy_fleet_directly(self): | ||
| # fleet role only has the implicit writer grant, not self-match unless listed | ||
| assert require_role({"r": "fleet"}, "fleet") is True |
There was a problem hiding this comment.
Test intent conflicts with its assertion in fleet self-case.
test_fleet_does_not_satisfy_fleet_directly and its comment imply a deny case, but the assertion expects True. This makes the authorization boundary ambiguous in tests.
Suggested fix (align test name/comment with current assertion)
- def test_fleet_does_not_satisfy_fleet_directly(self):
- # fleet role only has the implicit writer grant, not self-match unless listed
+ def test_fleet_satisfies_fleet_when_explicitly_listed(self):
+ # fleet has implicit writer grant and should self-match when explicitly listed
assert require_role({"r": "fleet"}, "fleet") is True📝 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.
Suggested change
| def test_fleet_does_not_satisfy_fleet_directly(self): | |
| # fleet role only has the implicit writer grant, not self-match unless listed | |
| assert require_role({"r": "fleet"}, "fleet") is True | |
| def test_fleet_satisfies_fleet_when_explicitly_listed(self): | |
| # fleet has implicit writer grant and should self-match when explicitly listed | |
| assert require_role({"r": "fleet"}, "fleet") is True |
🤖 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_auth.py` around lines 129 - 131, The test named
test_fleet_does_not_satisfy_fleet_directly contradicts its assertion; update the
test name and/or comment to reflect the actual behavior (that require_role({"r":
"fleet"}, "fleet") returns True). Concretely, rename
test_fleet_does_not_satisfy_fleet_directly to something like
test_fleet_satisfies_fleet_directly and change the comment to state that the
fleet role has the implicit writer grant and therefore satisfies the "fleet"
check when calling require_role; ensure references to require_role and the role
payload {"r": "fleet"} remain unchanged.
…urns fleet role - Fix _require_private_network to allow non-IP hosts (test clients, unix sockets) - Update deploy tests to use owner auth (deploy endpoint now requires owner) - Update auth test to expect fleet role instead of writer - Fix ruff format on compose_generator.py and test_compose.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #46 +/- ##
==========================================
- Coverage 96.71% 96.58% -0.14%
==========================================
Files 24 24
Lines 2346 2373 +27
==========================================
+ Hits 2269 2292 +23
- Misses 77 81 +4
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/test_coverage_gaps.py (1)
378-414: ⚡ Quick winMissing coverage for the new required-env-var 400 path.
The PR adds blank-required-field validation in
api_deploy(returns 400 with field list), but no test exercises that branch. Consider a small case with arequired: trueenv entry, emptydefault, and either no override or a whitespace-only override, assertingresp.status_code == 400and the missing field name in the detail.🤖 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_coverage_gaps.py` around lines 378 - 414, Add a new test that exercises the api_deploy branch which returns 400 for blank required env vars: copy the pattern from test_deploy_with_command_substitution but set the service's docker.env list to include an entry like {"key": "TOKEN", "required": True, "default": ""} and call the POST to /api/deploy/<slug> with either no env override or an override of "TOKEN" set to whitespace; assert resp.status_code == 400 and that the response detail mentions "TOKEN" (this will cover the new blank-required-field validation path in api_deploy).app/main.py (1)
65-111: 💤 Low valueTwo mutually exclusive
ifblocks could be oneif/elif.Lines 66 (
if not is_android) and 89 (if is_android) are perfectly disjoint — collapse intoif not is_android: ... else: ...for clarity and a free constant-factor speedup. Functional behavior is correct.🤖 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 `@app/main.py` around lines 65 - 111, The two disjoint conditional branches using is_android should be merged into a single if/else to clarify intent and avoid checking the same flag twice: replace the separate `if not is_android:` (which iterates `containers = _safe_json(w.get("containers", "[]"))` and appends container dicts to `result`) and `if is_android:` (which iterates `apps = _safe_json(w.get("apps", "[]"))` and appends app dicts) with `if not is_android: ... else: ...` so the container-handling code remains under the first branch and the app-handling code becomes the `else` branch; keep all existing keys and values, including references to `worker_name`, `w.get("id")`, `_has_docker`, and `_is_android`.
🤖 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 `@app/main.py`:
- Around line 313-323: The _require_private_network check currently fails open
by returning when request.client/request.client.host is missing or unparseable;
change it to fail closed: treat missing client or unparsable host as forbidden
(raise HTTPException) unless an explicit allowlist or feature flag is set.
Update _require_private_network to (1) check for an explicit allowlist value
(e.g., the intended test-client string) or a controlled env flag (e.g.,
FIRST_RUN_ALLOW_INSECURE) before permitting non-IP hosts, (2) otherwise raise
HTTPException for missing or invalid host instead of returning, and (3) emit a
clear log entry via the module/application logger whenever a bypass is used
(allowed test-client or env flag) so insecure bypasses are observable. Ensure
you reference and use the existing function name _require_private_network and
the same HTTPException behavior for consistent handling.
- Line 794: The /api/workers/{worker_id}/command handler must enforce owner-only
access for deploys: inside the api_worker_command function, detect when
payload.get("command") == "deploy" and call _require_owner(request) before
accepting or processing the provided spec (reject or abort if it raises). Add
this check at the start of the deploy branch/path (the same place where other
command-specific authorization occurs) so fleet-role callers cannot submit
arbitrary deploy specs via api_worker_command; keep existing require_role checks
for other commands unchanged.
---
Nitpick comments:
In `@app/main.py`:
- Around line 65-111: The two disjoint conditional branches using is_android
should be merged into a single if/else to clarify intent and avoid checking the
same flag twice: replace the separate `if not is_android:` (which iterates
`containers = _safe_json(w.get("containers", "[]"))` and appends container dicts
to `result`) and `if is_android:` (which iterates `apps =
_safe_json(w.get("apps", "[]"))` and appends app dicts) with `if not is_android:
... else: ...` so the container-handling code remains under the first branch and
the app-handling code becomes the `else` branch; keep all existing keys and
values, including references to `worker_name`, `w.get("id")`, `_has_docker`, and
`_is_android`.
In `@tests/test_coverage_gaps.py`:
- Around line 378-414: Add a new test that exercises the api_deploy branch which
returns 400 for blank required env vars: copy the pattern from
test_deploy_with_command_substitution but set the service's docker.env list to
include an entry like {"key": "TOKEN", "required": True, "default": ""} and call
the POST to /api/deploy/<slug> with either no env override or an override of
"TOKEN" set to whitespace; assert resp.status_code == 400 and that the response
detail mentions "TOKEN" (this will cover the new blank-required-field validation
path in api_deploy).
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ab33dfff-d5ae-4b51-a8d4-f9330fac22fe
📒 Files selected for processing (6)
app/compose_generator.pyapp/main.pytests/test_auth_extended.pytests/test_compose.pytests/test_coverage_gaps.pytests/test_main_deploy_routes.py
🚧 Files skipped from review as they are similar to previous changes (2)
- app/compose_generator.py
- tests/test_compose.py
Comment on lines
+313
to
+323
| def _require_private_network(request: Request) -> None: | ||
| """Block requests from public IPs (for first-run setup).""" | ||
| if not request.client or not request.client.host: | ||
| return | ||
| try: | ||
| client_ip = ipaddress.ip_address(request.client.host) | ||
| except ValueError: | ||
| return | ||
| if not (client_ip.is_loopback or client_ip.is_private): | ||
| raise HTTPException(status_code=403, detail="First-run setup only allowed from private networks") | ||
|
|
There was a problem hiding this comment.
Fail-open security check with misleading docstring.
_require_private_network silently returns (allowing the request) in two cases:
request.clientorrequest.client.hostmissing.request.client.hostdoesn't parse as an IP (e.g.,"testclient", Unix socket peer names).
The docstring says "Block requests from public IPs", but the function is fail-open: when the IP is unknown, it lets the request through. For a first-run admin-account-creation gate, fail-open is the wrong default. The test-client carve-out is intentional per the PR, but it should be either explicit (allowlist "testclient"/loopback names) or rely on an env flag, not a generic ValueError swallow. At minimum, log when the bypass triggers so misconfigured deployments are observable.
🛡️ Suggested hardening
def _require_private_network(request: Request) -> None:
- """Block requests from public IPs (for first-run setup)."""
- if not request.client or not request.client.host:
- return
- try:
- client_ip = ipaddress.ip_address(request.client.host)
- except ValueError:
- return
- if not (client_ip.is_loopback or client_ip.is_private):
- raise HTTPException(status_code=403, detail="First-run setup only allowed from private networks")
+ """Block first-run setup from non-private/non-loopback origins.
+
+ Fails open only for known test transports (ASGI TestClient / unix sockets)
+ so production HTTP requests with absent client info are rejected.
+ """
+ _TEST_HOSTS = {"testclient", "unix"}
+ host = request.client.host if request.client else None
+ if not host:
+ logger.warning("First-run setup request has no client host; denying")
+ raise HTTPException(status_code=403, detail="Client IP unknown")
+ if host in _TEST_HOSTS:
+ return
+ try:
+ client_ip = ipaddress.ip_address(host)
+ except ValueError:
+ logger.warning("First-run setup from non-IP host %r; denying", host)
+ raise HTTPException(status_code=403, detail="Unrecognized client host")
+ if not (client_ip.is_loopback or client_ip.is_private):
+ raise HTTPException(status_code=403, detail="First-run setup only allowed from private networks")🤖 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 `@app/main.py` around lines 313 - 323, The _require_private_network check
currently fails open by returning when request.client/request.client.host is
missing or unparseable; change it to fail closed: treat missing client or
unparsable host as forbidden (raise HTTPException) unless an explicit allowlist
or feature flag is set. Update _require_private_network to (1) check for an
explicit allowlist value (e.g., the intended test-client string) or a controlled
env flag (e.g., FIRST_RUN_ALLOW_INSECURE) before permitting non-IP hosts, (2)
otherwise raise HTTPException for missing or invalid host instead of returning,
and (3) emit a clear log entry via the module/application logger whenever a
bypass is used (allowed test-client or env flag) so insecure bypasses are
observable. Ensure you reference and use the existing function name
_require_private_network and the same HTTPException behavior for consistent
handling.
| @app.post("/api/deploy/{slug}") | ||
| async def api_deploy(request: Request, slug: str, body: DeployRequest, worker_id: int | None = None) -> dict[str, str]: | ||
| _require_writer(request) | ||
| _require_owner(request) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C3 'def\s+require_role\s*\(' --type=py
rg -nP -C2 '"fleet"' app/auth.py 2>/dev/null || rg -nP -C2 'fleet' --type=py -g 'app/auth.py'Repository: GeiserX/CashPilot
Length of output: 838
🏁 Script executed:
# First, get the api_worker_command function around line 1675
sed -n '1670,1710p' app/main.pyRepository: GeiserX/CashPilot
Length of output: 1653
🏁 Script executed:
# Find the _require_writer and _require_owner implementations
rg -n '_require_(writer|owner)' app/main.py | head -20Repository: GeiserX/CashPilot
Length of output: 801
🏁 Script executed:
# Find the /api/deploy/{slug} endpoint to compare authorization gates
rg -n 'api/deploy' app/main.pyRepository: GeiserX/CashPilot
Length of output: 97
🏁 Script executed:
# Get the /api/deploy/{slug} endpoint implementation
sed -n '792,850p' app/main.pyRepository: GeiserX/CashPilot
Length of output: 2444
🏁 Script executed:
# Get the rest of the /api/deploy implementation to see what it does
sed -n '850,920p' app/main.pyRepository: GeiserX/CashPilot
Length of output: 3102
🏁 Script executed:
# Find _proxy_worker_deploy implementation
rg -n '_proxy_worker_deploy' app/main.pyRepository: GeiserX/CashPilot
Length of output: 231
🏁 Script executed:
# Also check what WorkerCommand class allows for spec
rg -n 'class WorkerCommand' app/main.py -A 5Repository: GeiserX/CashPilot
Length of output: 231
🏁 Script executed:
# Get _proxy_worker_deploy implementation
sed -n '958,1000p' app/main.pyRepository: GeiserX/CashPilot
Length of output: 2058
Gate the deploy command in /api/workers/{worker_id}/command on _require_owner.
The owner-only authorization on /api/deploy/{slug} is bypassed. Fleet API key holders (role="fleet") satisfy writer checks per require_role() (auth.py line 153–154), allowing them to call /api/workers/{worker_id}/command with command: "deploy" and arbitrary spec, sidestepping the owner gate. Both endpoints POST to the same worker deploy endpoint; the difference is /api/deploy validates the spec against the service catalog, while /api/workers/{worker_id}/command accepts raw caller-supplied specs. This defeats the audit requirement that fleet cannot deploy containers.
Fix in `api_worker_command` (lines 1675–1702)
`@app.post`("/api/workers/{worker_id}/command")
async def api_worker_command(request: Request, worker_id: int, body: WorkerCommand) -> dict[str, Any]:
"""Send a command to a worker by proxying to its REST API."""
- _require_writer(request)
+ # deploy mirrors /api/deploy/{slug} and must follow the same authz
+ if body.command == "deploy":
+ _require_owner(request)
+ else:
+ _require_writer(request)🤖 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 `@app/main.py` at line 794, The /api/workers/{worker_id}/command handler must
enforce owner-only access for deploys: inside the api_worker_command function,
detect when payload.get("command") == "deploy" and call _require_owner(request)
before accepting or processing the provided spec (reject or abort if it raises).
Add this check at the start of the deploy branch/path (the same place where
other command-specific authorization occurs) so fleet-role callers cannot submit
arbitrary deploy specs via api_worker_command; keep existing require_role checks
for other commands unchanged.
6 tasks
codeshard
pushed a commit
to codeshard/CashPilot
that referenced
this pull request
May 23, 2026
…, validation (GeiserX#46) * fix: address ecosystem audit — security, compose, docs, validation Security: - Fleet key now grants limited 'fleet' role instead of full 'writer' - Deploy endpoint requires owner role (not writer/fleet) - First-run registration restricted to private/loopback networks - XSS fix: fleet page uses data attributes instead of inline onclick - Fleet bootstrap: entrypoint ensures /fleet dir is writable Compose generator: - Named volumes now declared at top-level volumes section - ${VAR} placeholders escaped as $${VAR} to prevent host interpolation Validation: - Deploy API rejects requests with blank required env vars - Android workers: containers array skipped (only apps processed) Docs: - Fleet docs rewritten to match actual REST heartbeat implementation - Presearch/IPRoyal/Earn.fm/Repocket guide env vars aligned with YAML - Earn.fm/Repocket service YAMLs aligned with collector auth models Testing: - Added requirements-dev.txt for local test reproducibility Closes #cp-bj8p #cp-jbkg #cp-370w #cp-3ddy #cp-zvhs #cp-xt5k #cp-zrlr Closes #cp-pkio #cp-v65p #cp-p2sb #cp-hvay #cp-6hr9 #cp-6ho7 * fix: address review findings — extract IP helper, strip whitespace, fix SSRF, align docs - Extract _require_private_network() helper (removes duplication, handles None/ValueError) - Strip whitespace in required env var validation - Fix SSRF in api_worker_command by using _get_verified_worker_url() - Align earnfm/repocket docs with email/password auth model - Fix repocket policy contradiction (VPS accepted at lower rates) * fix: address all review nits — escape idempotency, NaN guard, test coverage - Fix _escape_interpolation to use regex (skips already-escaped $${}) - Add isNaN guard in fleet.html event delegation - Add TestRequireRole covering fleet escalation boundaries - Add TestEscapeInterpolation, TestIsNamedVolume, TestNamedVolumeDeclaration * fix: update tests for auth changes — deploy requires owner, fleet returns fleet role - Fix _require_private_network to allow non-IP hosts (test clients, unix sockets) - Update deploy tests to use owner auth (deploy endpoint now requires owner) - Update auth test to expect fleet role instead of writer - Fix ruff format on compose_generator.py and test_compose.py * style: split long comprehension lines for format compatibility
GeiserX
added a commit
that referenced
this pull request
Jun 11, 2026
* fix: restore Repocket container env to RP_EMAIL + RP_API_KEY The repocket/repocket image authenticates with RP_EMAIL + RP_API_KEY (an API key from the dashboard), but PR #46 renamed these to REPOCKET_EMAIL/REPOCKET_PASSWORD to match the earnings collector's Firebase auth model. The container ignores the unknown vars and logs "User credentials are missing!" regardless of what the user enters — which is why even pasting the API key into the password field failed. The container and the earnings collector need different secrets: the container uses the dashboard API key; the collector logs into the Repocket API via Firebase with the account email/password. They flow through independent paths, so this fix only touches the container env (YAML) and leaves the collector unchanged. - Rename docker.env keys to RP_EMAIL + RP_API_KEY (label "API Key", secret), with help text pointing to the dashboard bandwidth-earnings page where the key is found. - Update the guide and service description (API key, not password). - Add a regression test asserting the container env contract, so a future "align with collector" edit can't silently break deploys. Fixes #82 * test: add deploy-layer guard for Repocket RP_* env keys Complements the catalog-level regression test by asserting the real repocket catalog entry produces RP_EMAIL/RP_API_KEY in the container spec sent to the worker — catching both a YAML rename and any future api_deploy refactor that mangles env names. Reloads the catalog from disk to stay independent of cache state left by earlier tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses all 18 findings from the CashPilot ecosystem security/quality audit.
Security (Critical/High)
fleetrole instead of fullwriter— cannot deploy containers or access owner endpointsPOST /api/deployrequires owner role, not writer/fleet/registerrestricted to private/loopback IPs when no users existonclickhandlersentrypoint.shensures/fleetdirectory is writable before dropping to unprivileged userCompose Generator (High)
volumes:section${VAR}placeholders in commands/volumes escaped as$${VAR}to prevent Docker Compose host-shell interpolationValidation (Medium)
containersarray skipped during ingestion (onlyappsprocessed), fixing double-countingDocumentation (High/Medium)
PRESEARCH_REGISTRATION_CODE→REGISTRATION_CODEIPROYAL_*→IPROYALPAWNS_*Developer Experience (Low)
requirements-dev.txtwith pytest/pytest-asyncio/pytest-cov for local test reproducibilityBeads addressed
cp-bj8p, cp-jbkg, cp-370w, cp-3ddy, cp-zvhs, cp-xt5k, cp-zrlr, cp-pkio, cp-v65p, cp-p2sb, cp-hvay, cp-6hr9, cp-6ho7
Remaining (separate repos)
Test plan
$${EMAIL}not${EMAIL}Summary by CodeRabbit
New Features
Bug Fixes
Security
Documentation
Chore / Tests