fix(security): close RCE via PR title, stop leaking secrets, harden runner (#65)

The runner entrypoint built a sed script out of the PR title, so a title
could close the s command and chain GNU sed's `e` -- which shells out.
Reproduced in the real image: a title of

    benign-looking title|;e id > /tmp/PWNED #

executed as the runner user, which holds CLAUDE_CODE_OAUTH_TOKEN and
GITHUB_TOKEN, and the rendered prompt showed no trace of the payload.
The same line broke on any title containing a plain `|`, aborting before
emit_error could run and leaving the frontend a dead session with no
reason. Substitution now goes through awk with the value passed via the
environment and matched with index()/substr(), so no byte of PR-controlled
data is parsed as a pattern or as code.

Settings rendered every credential in cleartext through repr(), while
sentry_sdk.init defaults to include_local_variables=True and a live
`settings` sits in a dozen frames that can raise -- one unhandled 500 was
enough to ship the whole set to a third party. Credentials are SecretStr
now; SecretStr defines __len__, so existing truthiness checks still hold.

Also:
- PidsLimit, CapDrop ALL and no-new-privileges on the one container that
  runs `claude --dangerously-skip-permissions` over attacker-authored PRs
- rate limit the SQLAdmin login: slowapi resolves handlers via
  hasattr(route, "endpoint"), so the whole /admin Mount was exempt and
  guessing against the shared password was unbounded
- ENVIRONMENT is a Literal, so a typo can no longer skip the production
  secret validation silently
- CORS registered last (outermost) per the documented convention, plus
  expose_headers so the frontend can read X-Request-ID
- shellcheck in CI over the entrypoint, the file where this bug lived and
  the only code in the repo with no linter over it
- drop ANTHROPIC_API_KEY and the two LEMONSQUEEZY_* settings, unread since
  the pivot, and the unused PR_DIFF shell variable shellcheck flagged

Author: mariuspruvot

.env.example

@@ -76,6 +76,12 @@ CONTAINER_TTL_SECONDS=900
 # Number of uvicorn workers (default: 4)
 UVICORN_WORKERS=4
 
+# --- Database pool, PER WORKER ----------------------------------------------
+# UVICORN_WORKERS x (DB_POOL_SIZE + DB_MAX_OVERFLOW) must stay below the
+# Postgres server's max_connections (default 100). Defaults give 4 x 15 = 60.
+# DB_POOL_SIZE=10
+# DB_MAX_OVERFLOW=5
+
 # Absolute path to skills/ directory on the Docker host.
 # Required for Docker-in-Docker volume mounts (the API container mounts this
 # path into claude-runner containers). Must be an absolute path on the HOST,

.github/workflows/ci.yml

@@ -7,6 +7,15 @@ on:
     branches: [main]
 
 jobs:
+  # The runner entrypoint executes untrusted PR content and was the only code
+  # in the repo with no linter over it. A shell injection lived there
+  # undetected; shellcheck is what stops the next one.
+  lint-shell:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - run: shellcheck infra/docker/claude-runner/entrypoint.sh scripts/*.sh
+
   lint-backend:
     runs-on: ubuntu-latest
     defaults:

apps/api/pyproject.toml

@@ -18,6 +18,10 @@ dependencies = [
     "structlog>=24.4.0",
     "sentry-sdk[fastapi]>=2.19.0",
     "slowapi>=0.1.9",
+    # Pulled in by slowapi, declared because it is imported directly to rate
+    # limit the SQLAdmin login, which slowapi's middleware cannot see: it
+    # skips anything mounted rather than routed.
+    "limits>=3.0.0",
     "itsdangerous>=2.2.0",
     "aiodocker>=0.23.0",
 ]

apps/api/src/helprs/admin/views.py

@@ -3,8 +3,14 @@
 import secrets
 
 import structlog
+from limits import parse
+from limits.storage import MemoryStorage
+from limits.strategies import FixedWindowRateLimiter
+from slowapi.util import get_remote_address
 from sqladmin import Admin, ModelView
 from sqladmin.authentication import AuthenticationBackend
+from sqlalchemy.ext.asyncio import AsyncEngine
+from starlette.applications import Starlette
 from starlette.requests import Request
 
 from helprs.core.config import get_settings
@@ -94,6 +100,18 @@ class WebhookEventAdmin(ModelView, model=WebhookEvent):
     icon = "fa-solid fa-bolt"
 
 
+# SlowAPIMiddleware cannot protect this login. It resolves the handler through
+# `_find_route_handler`, which requires `hasattr(route, "endpoint")`; SQLAdmin
+# is a Mount, so the whole /admin subtree is treated as exempt and skipped.
+# Guessing was therefore unlimited against a single shared password that
+# guards user rows and BYOK ciphertext, so the limit is enforced here instead.
+#
+# Storage is per-process, like the app's other limiter: with several uvicorn
+# workers the effective budget is this figure times the worker count. That is
+# still a hard bound on guessing, which is the point.
+_LOGIN_ATTEMPT_LIMIT = parse("5/minute")
+
+
 class AdminAuth(AuthenticationBackend):
     """Password authentication for the admin panel.
 
@@ -104,15 +122,26 @@ class AdminAuth(AuthenticationBackend):
     opening it (``setup_admin`` refuses to mount in that case).
     """
 
+    def __init__(self, secret_key: str) -> None:
+        super().__init__(secret_key=secret_key)
+        # One counter per mounted panel rather than a module global, so the
+        # lifetime matches the app's.
+        self._login_limiter = FixedWindowRateLimiter(MemoryStorage())
+
     async def login(self, request: Request) -> bool:
+        client = get_remote_address(request)
+        if not self._login_limiter.hit(_LOGIN_ATTEMPT_LIMIT, client):
+            logger.warning("admin_login_rate_limited", client=client)
+            return False
+
         form = await request.form()
         password = form.get("password")
         if not isinstance(password, str):
             return False
 
         # compare_digest("", "") is True, so an unset password would otherwise
         # authenticate an empty form field.
-        expected = get_settings().ADMIN_PASSWORD
+        expected = get_settings().ADMIN_PASSWORD.get_secret_value()
         if not expected or not secrets.compare_digest(password, expected):
             return False
 
@@ -127,7 +156,7 @@ async def authenticate(self, request: Request) -> bool:
         return request.session.get("authenticated", False)
 
 
-def setup_admin(app, engine, secret_key: str) -> Admin | None:
+def setup_admin(app: Starlette, engine: AsyncEngine, secret_key: str) -> Admin | None:
     """Mount SQLAdmin at /admin, or nothing at all when no password is set."""
     if not get_settings().ADMIN_PASSWORD:
         logger.warning("admin_panel_disabled", reason="ADMIN_PASSWORD is not set")

apps/api/src/helprs/core/config.py

@@ -2,45 +2,51 @@
 
 import base64
 from functools import lru_cache
+from typing import Literal
 
-from pydantic import field_validator, model_validator
+from cryptography.fernet import Fernet, InvalidToken
+from pydantic import SecretStr, field_validator, model_validator
 from pydantic_settings import BaseSettings, SettingsConfigDict
 
 
 class Settings(BaseSettings):
+    """Runtime configuration.
+
+    Every credential is a ``SecretStr`` so it cannot be rendered by accident.
+    ``repr(Settings())`` used to print all of them in cleartext, and
+    ``sentry_sdk.init`` defaults to ``include_local_variables=True`` -- a live
+    ``settings`` local sits in a dozen frames that can raise, so one unhandled
+    500 was enough to ship the whole secret set to a third party. Read a value
+    with ``.get_secret_value()``; ``SecretStr`` defines ``__len__``, so plain
+    truthiness checks still work.
+    """
+
     model_config = SettingsConfigDict(env_file=".env", extra="ignore")
 
     # Database
-    DATABASE_URL: str
+    DATABASE_URL: SecretStr
 
     # Security
-    SECRET_KEY: str
-    FERNET_KEY: str
-    ADMIN_PASSWORD: str = ""
+    SECRET_KEY: SecretStr
+    FERNET_KEY: SecretStr
+    ADMIN_PASSWORD: SecretStr = SecretStr("")
 
     # GitHub App
     GITHUB_APP_ID: str
-    GITHUB_APP_PRIVATE_KEY: str = ""
+    GITHUB_APP_PRIVATE_KEY: SecretStr = SecretStr("")
     GITHUB_CLIENT_ID: str = ""
-    GITHUB_CLIENT_SECRET: str = ""
-    GITHUB_WEBHOOK_SECRET: str = ""
-
-    # Anthropic
-    ANTHROPIC_API_KEY: str = ""
-
-    # Lemon Squeezy
-    LEMONSQUEEZY_API_KEY: str = ""
-    LEMONSQUEEZY_WEBHOOK_SECRET: str = ""
+    GITHUB_CLIENT_SECRET: SecretStr = SecretStr("")
+    GITHUB_WEBHOOK_SECRET: SecretStr = SecretStr("")
 
     # Sentry
-    SENTRY_DSN: str = ""
+    SENTRY_DSN: SecretStr = SecretStr("")
 
     # CORS
     CORS_ORIGINS: list[str] = ["http://localhost:5173"]
 
     # Frontend base URL used to build user-facing links (e.g. PR-comment session links).
     # Kept separate from CORS_ORIGINS: that list is for browser security, this single
-    # string is for link composition. See deferred-work.md nice-to-have #11.
+    # string is for link composition.
     APP_BASE_URL: str = "http://localhost:5173"
 
     # Container orchestration
@@ -51,25 +57,27 @@ class Settings(BaseSettings):
     DB_POOL_SIZE: int = 10
     DB_MAX_OVERFLOW: int = 5
 
-    # Environment
-    ENVIRONMENT: str = "development"
+    # Environment. A Literal rather than a free-form str: validate_production_secrets
+    # below only fires on the exact string "production", so a typo like "prod" would
+    # silently disable the one guard meant to fail loud at startup.
+    ENVIRONMENT: Literal["development", "test", "production"] = "development"
 
     @field_validator("GITHUB_APP_PRIVATE_KEY")
     @classmethod
-    def normalize_private_key(cls, v: str) -> str:
+    def normalize_private_key(cls, v: SecretStr) -> SecretStr:
         """Accept the App private key as raw PEM or base64-encoded PEM.
 
         ``.env`` files cannot hold multi-line values, so operators base64 the
         PEM; platforms with multi-line env vars (Coolify) paste it raw. Both
         are normalized to raw PEM here so the rest of the code only ever sees
         what ``jwt.encode`` expects.
         """
-        v = v.strip()
-        if not v or v.startswith("-----BEGIN"):
-            return v
+        raw = v.get_secret_value().strip()
+        if not raw or raw.startswith("-----BEGIN"):
+            return SecretStr(raw)
 
         try:
-            decoded = base64.b64decode(v, validate=True).decode()
+            decoded = base64.b64decode(raw, validate=True).decode()
         except (ValueError, UnicodeDecodeError) as e:
             raise ValueError(
                 "GITHUB_APP_PRIVATE_KEY must be a PEM private key, either raw "
@@ -79,16 +87,14 @@ def normalize_private_key(cls, v: str) -> str:
 
         if not decoded.lstrip().startswith("-----BEGIN"):
             raise ValueError("GITHUB_APP_PRIVATE_KEY decoded from base64 but is not a PEM private key")
-        return decoded
+        return SecretStr(decoded)
 
     @field_validator("FERNET_KEY")
     @classmethod
-    def validate_fernet_key(cls, v: str) -> str:
+    def validate_fernet_key(cls, v: SecretStr) -> SecretStr:
         """Validate that FERNET_KEY is a valid Fernet key (32 url-safe base64 bytes)."""
-        from cryptography.fernet import Fernet, InvalidToken
-
         try:
-            Fernet(v.encode() if isinstance(v, str) else v)
+            Fernet(v.get_secret_value().encode())
         except (ValueError, InvalidToken) as e:
             raise ValueError(
                 "FERNET_KEY must be a valid Fernet key (32 url-safe base64-encoded bytes). "
@@ -105,7 +111,7 @@ def validate_production_secrets(self) -> "Settings":
         missing: list[str] = []
         if not self.ADMIN_PASSWORD:
             missing.append("ADMIN_PASSWORD")
-        if len(self.SECRET_KEY) < 32:
+        if len(self.SECRET_KEY.get_secret_value()) < 32:
             missing.append("SECRET_KEY (must be >= 32 characters)")
         if not self.GITHUB_WEBHOOK_SECRET:
             missing.append("GITHUB_WEBHOOK_SECRET")

apps/api/src/helprs/core/database.py

@@ -101,7 +101,7 @@ def create_engine() -> AsyncEngine:
     """
     settings = get_settings()
     return create_async_engine(
-        settings.DATABASE_URL,
+        settings.DATABASE_URL.get_secret_value(),
         echo=False,
         pool_pre_ping=True,
         pool_size=settings.DB_POOL_SIZE,

apps/api/src/helprs/core/dependencies.py

@@ -36,7 +36,7 @@ async def get_db(request: Request) -> AsyncGenerator[AsyncSession, None]:
 async def _authenticate(request: Request, session: AsyncSession, settings: Settings, token: str) -> GitHubUser:
     """Resolve a bearer token to the user it belongs to."""
     try:
-        payload = decode_access_token(token, settings.SECRET_KEY)
+        payload = decode_access_token(token, settings.SECRET_KEY.get_secret_value())
     except JWTError as e:
         raise UnauthorizedError("Invalid or expired token") from e
 

apps/api/src/helprs/core/middleware.py

@@ -47,7 +47,7 @@ def configure_sentry(settings: Settings) -> None:
     """Initialize Sentry SDK if SENTRY_DSN is configured."""
     if settings.SENTRY_DSN:
         sentry_sdk.init(
-            dsn=settings.SENTRY_DSN,
+            dsn=settings.SENTRY_DSN.get_secret_value(),
             traces_sample_rate=0.2,
             integrations=[
                 StarletteIntegration(transaction_style="endpoint"),
@@ -89,20 +89,40 @@ async def dispatch(self, request: Request, call_next) -> Response:
 
 
 def setup_middleware(app: FastAPI, settings: Settings) -> None:
-    """Register all middleware on the app in correct order."""
-    # CORS first
+    """Register all middleware on the app.
+
+    Registration order is the REVERSE of execution order: Starlette inserts
+    each new middleware at the front of the stack, so the last one added ends
+    up outermost. Rate limiting is therefore registered first and CORS last.
+
+    CORS outermost is the documented convention and the safe default: any
+    response produced below it then carries the headers. Note this is
+    belt-and-braces rather than a bug fix -- rate-limit rejections already
+    reach the browser correctly, because RateLimitExceeded subclasses
+    HTTPException and is rendered by ExceptionMiddleware, which sits inside
+    the user stack either way. The genuinely broken case, unhandled 500s, is
+    unreachable from here at all: ServerErrorMiddleware always sits outside
+    the user stack, which is why ``_handle_unhandled_exception`` has to set
+    the headers by hand.
+    """
+    # Innermost: rate limiting.
+    app.state.limiter = limiter
+    app.add_exception_handler(RateLimitExceeded, _rate_limit_exceeded_handler)
+    app.add_middleware(SlowAPIMiddleware)
+
+    # Then request logging, so rate-limited requests get logged too.
+    app.add_middleware(RequestLoggingMiddleware)
+
+    # Outermost: CORS, so every response carries the headers -- including the
+    # ones short-circuited by the middleware below it.
     app.add_middleware(
         CORSMiddleware,
         allow_origins=settings.CORS_ORIGINS,
         allow_credentials=True,
         allow_methods=["GET", "POST", "PUT", "DELETE", "OPTIONS"],
         allow_headers=["Authorization", "Content-Type", "X-Request-ID", "Last-Event-ID"],
+        # Set on the way out; allow_headers only governs the request
+        # direction, so without this the frontend cannot read the ID it would
+        # quote in a bug report.
+        expose_headers=["X-Request-ID"],
     )
-
-    # Request logging
-    app.add_middleware(RequestLoggingMiddleware)
-
-    # Rate limiting
-    app.state.limiter = limiter
-    app.add_exception_handler(RateLimitExceeded, _rate_limit_exceeded_handler)
-    app.add_middleware(SlowAPIMiddleware)

apps/api/src/helprs/main.py

@@ -183,7 +183,7 @@ async def _lifespan(app: FastAPI) -> AsyncIterator[None]:
         set_session_factory(session_factory)
         stack.callback(clear_session_factory)
 
-        setup_admin(app, engine, settings.SECRET_KEY)
+        setup_admin(app, engine, settings.SECRET_KEY.get_secret_value())
 
         await _replay_pending_webhook_events(app)
         await _reconcile_stale_sessions(session_factory)

apps/api/src/helprs/modules/container/docker_client.py

@@ -12,6 +12,9 @@
 
 CONTAINER_MEMORY_BYTES = 512 * 1024 * 1024
 CONTAINER_NANO_CPUS = 1_000_000_000
+# Generous for a git clone plus a node process, tight enough that a runaway
+# fork loop hits the cap instead of the host's process table.
+CONTAINER_PIDS_LIMIT = 512
 
 # The entrypoint reads newline-terminated commands from this FIFO.
 _INPUT_FIFO = "/tmp/claude-input"
@@ -75,11 +78,19 @@ async def create_container(
                 "Env": [f"{k}={v}" for k, v in environment.items()],
                 "Labels": labels,
                 "OpenStdin": True,
+                # This is the one container in the system that executes
+                # untrusted input: `claude --dangerously-skip-permissions`
+                # against PR content written by whoever opened the PR. Memory
+                # and CPU alone do not bound that -- a fork bomb exhausts PIDs
+                # long before it exhausts a 512 MB cap.
                 "HostConfig": {
                     "Binds": volumes,
                     "Memory": CONTAINER_MEMORY_BYTES,
                     "NanoCPUs": CONTAINER_NANO_CPUS,
                     "NetworkMode": "bridge",
+                    "PidsLimit": CONTAINER_PIDS_LIMIT,
+                    "CapDrop": ["ALL"],
+                    "SecurityOpt": ["no-new-privileges"],
                 },
             }
         )