Skip to content

security: hardening pass 1 (token scoping, sandbox credential isolation, path-traversal guard)#38

Merged
pelazas merged 4 commits into
mainfrom
security/hardening-pass-1
Jun 14, 2026
Merged

security: hardening pass 1 (token scoping, sandbox credential isolation, path-traversal guard)#38
pelazas merged 4 commits into
mainfrom
security/hardening-pass-1

Conversation

@pelazas

@pelazas pelazas commented Jun 14, 2026

Copy link
Copy Markdown
Owner

First batch of fixes from the audit in docs/SECURITY_THREAT_MODEL.md. Scope was deliberately limited to self-contained application-code changes — no CDK/IAM/network redesign, no UX-affecting changes — so this can land safely and fast. The larger architectural items are listed under Deferred below for follow-up PRs.

What's in this PR

F-3 — Scope the GitHub installation token (main.py)

_get_installation_token sent no request body, so GitHub minted a token covering every repo in the installation with the App's full permission set. Now scoped to the single PR repository and the four permissions RenderPR uses: contents:write (clone + apply push), pull_requests:write (diff/metadata), issues:write (review comment), metadata:read. A leaked token can no longer reach other repos or capabilities.

F-1 (defense-in-depth) — Keep task-role credentials out of untrusted PR processes (main.py)

Dependency install (postinstall hooks) and the dev server run arbitrary PR code, yet inherited the full container environment, including the ECS task-role credential pointers (AWS_CONTAINER_CREDENTIALS_*). That let malicious PR code assume the task role and read every secret in SSM (GitHub App private key, OpenRouter key, all tenants' secrets). Both the install and dev-server subprocesses now launch with a credential-stripped environment (_untrusted_subprocess_env). The agent process keeps its own credentials for S3 upload / task registration.

Note: this is defense-in-depth, not a substitute for removing the role + egress from the runner (see Deferred). It raises the bar significantly because Fargate exposes task-role creds only via the relative-URI pointer we now strip.

F-4 — Stop leaking the command token into the dev server (main.py)

RENDERPR_COMMAND_TOKEN was written to os.environ, which the untrusted dev server inherited. It's now held in a local and passed straight to CommandServer(token=...).

F-11 — Reject path traversal in LLM-driven file access (code_edit.py, editor.py)

Edit file paths come from the LLM (attacker-influenced via the request/diff) and were joined onto REPO_DIR with no guard. New safe_repo_path() resolves under REPO_DIR and rejects ../absolute escapes; validate_edit, apply_edit, apply_edits, request_edit, and find_occurrence_in_file all route through it.

Testing

  • Full suite: 465 passed locally (Python 3.12 venv, playwright installed).
  • New tests: token scoping (repo + min perms; omits repositories when repo unknown), _untrusted_subprocess_env stripping + overlay precedence, path-traversal/absolute-path rejection for both validate_edit and apply_edit.
  • ruff clean on changed files; mypy clean on changed source (pre-existing routes.py errors untouched).

⚠️ Deploy validation needed before merge

contents/pull_requests/issues/metadata are the permissions the bot already relies on, so a correctly-installed App should mint the scoped token fine — but if the installed GitHub App is missing one of these grants, token minting will start returning 422 and reviews will break. Please confirm against a live PR review (waiting for any stale task to STOP first) before merging.

Deferred to follow-up PRs (need infra / product decisions)

  • F-1 (full): split untrusted execution into a role-less runner task; mint the scoped token in a trusted stage; hand the runner a pre-signed S3 upload URL so it needs no IAM. Removes the GitHub App private key and OpenRouter key from the sandbox entirely.
  • F-2: scope the SSM secrets policy per-repo instead of /renderpr/secrets/*.
  • F-5: restrict task egress (currently allowAllOutbound).
  • F-6: privatize the screenshot bucket + pre-signed URLs.
  • F-7/F-8/F-9/F-10: command channel off the public internet, trigger authorization + quotas, webhook replay protection, CI OIDC.

See docs/SECURITY_THREAT_MODEL.md for the full analysis and prioritized roadmap.

🤖 Generated with Claude Code

pelazas and others added 4 commits June 14, 2026 18:41
…imal permissions

The token request sent no body, so GitHub minted an installation token
covering every repository in the installation with the App's full
permission set. Scope it to the single PR repository and the four
permissions RenderPR uses (contents, pull_requests, issues, metadata) so a
leaked token can't reach other repos or capabilities.

Addresses finding F-3 in docs/SECURITY_THREAT_MODEL.md.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…trusted PR processes

Dependency install scripts and the dev server execute arbitrary code from
the pull request, yet inherited the full container environment — including
the ECS task-role credential pointers (AWS_CONTAINER_CREDENTIALS_*) and the
SSM parameter names. That let a malicious postinstall hook or dev script
assume the task role and read every secret in SSM.

- Launch both the install and the dev server with a credential-stripped
  environment (_untrusted_subprocess_env).
- Stop writing RENDERPR_COMMAND_TOKEN into os.environ (the dev server
  inherited it); pass it straight to CommandServer instead.

Defense-in-depth for finding F-1 and fix for F-4 in
docs/SECURITY_THREAT_MODEL.md. Does not replace removing the role/egress
from the runner (tracked as follow-up).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
File paths in code-change edits come from the LLM, whose input (the user
request and PR diff/source) is attacker-influenced. validate_edit,
apply_edit, apply_edits, request_edit and find_occurrence_in_file joined
those paths onto REPO_DIR with no guard, so a '..' or absolute path could
read or write outside the cloned repo.

Add safe_repo_path() which resolves under REPO_DIR and rejects anything that
escapes it, and route every file access through it.

Addresses finding F-11 in docs/SECURITY_THREAT_MODEL.md.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…_path

Self-review follow-up: _routes_for_edits joined edit["file"] onto REPO_DIR
directly to read source for action validation. It is only reached after
validate_edit gates the same edits, so it was not exploitable, but route it
through safe_repo_path too so every REPO_DIR/edit-path join is uniformly
guarded (finding F-11).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@pelazas pelazas merged commit 4979a97 into main Jun 14, 2026
@pelazas pelazas deleted the security/hardening-pass-1 branch June 14, 2026 16:48
pelazas added a commit that referenced this pull request Jun 14, 2026
PR #38's F-3 scoping requested issues:write, but the GitHub App is only
granted issues:read, so GitHub rejected EVERY installation-token mint with
422 -> no token -> every review failed at startup (prod outage on rev 101).
RenderPR only comments on PRs, which is covered by pull_requests:write, so
issues isn't needed. Verified: the minimal scope mints 201.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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