Repository navigation
Security audit fixes (combines #19–#24) - #25
Merged
Merged
Conversation
…issions The authenticated /api/v1 group only checked login and MFA. Admin handlers (users, API tokens, settings, sessions, audit logs) never checked roles, so any logged-in user or API token, including the CircleCI token, could create admins, mint convox:*:* tokens, or turn off deploy approvals and MFA. The checks were lost in the gin rewrite. - New Authorize middleware runs after Authenticated. It enforces the access policy declared for every route in rbac's route table and denies routes without a policy. Admin routes require their gateway permission. Self-service routes (own MFA, /info, settings reads) are open to any logged-in human. - API tokens are allowed only on the routes the CLI uses in CI: /info, /rack, creating a deploy approval request and reading one by ID. Tokens can no longer approve deploys or call any admin endpoint. - rbac.Manager.Authorize(principal, permission) replaces Enforce, EnforceUser and EnforceForAPIToken. Humans are checked against the roles on their current DB record, so demotion and deletion apply on the next request instead of after a restart (Casbin user groupings were only ever added, never removed). API tokens need the permission themselves AND their owner's current role must allow it. Suspended or locked users and their tokens are denied. - Handler and proxy checks (env reads/writes, secret masking, deploy approval view/create, Slack) now authorize the real caller. Before, an admin-owned token was treated as the admin. - Audit logs get their own gateway:audit_log:read permission instead of borrowing deploy_approval_request:read. Listing approval requests and their audit logs requires approver permission, matching the handlers. Admin role gains security:*:*, which the rack TLS cert refresh route requires. - Tokens of locked owners are rejected at authentication. Token creation honours expires_at, which the CLI already sends but the server ignored. Creating a token for an unknown user returns 404 instead of dereferencing nil. - Removed dead code: RequireRole, Authenticated's unused rbac parameter, and the header-based env permission check that nothing called (the proxy already strips Env headers). routes/authorization_test.go exercises the real router as viewer and deployer sessions, a viewer token, the production-style admin-owned cicd token and an admin-owned wildcard token, and asserts every admin endpoint refuses them.
A session that had only passed Google login could call the MFA enrollment endpoints (MFANone), enroll its own TOTP and be marked MFA-verified, then delete the user's real factors and mint tokens. TOTP enrollment also replaced the user's backup codes and returned fresh ones, so an attacker didn't even need to enroll. - New RequireVerifiedMFASession middleware: a session whose user must complete an MFA challenge can only reach mfa/status, mfa/verify, the WebAuthn assertion endpoints and /info until it verifies. It may also complete the challenge inline with an MFA header (used by the SPA step-up dialog). - The same rule applies on /rack-proxy, so a pending web session token can't be replayed as a Bearer credential. - Adding a factor when already enrolled requires a recent step-up; first-time enrollment is unchanged. - Enrollment no longer replaces an enrolled user's backup codes (regeneration stays a separate step-up action). - MFA factor enrollment (incl. YubiOTP), deletion and backup-code regeneration now write DB audit events.
PUT /api/v1/apps/:app/env only validated the submitted key, then joined values with newlines for the rack API. A value like "x\nDATABASE_URL=postgres://evil" injected a second line that Convox applied (last duplicate wins). That bypassed protected env vars and the secrets:set permission, and the audit log only recorded the harmless key. MergeEnv now requires keys to match [A-Za-z_][A-Za-z0-9_]* and rejects CR, LF and NUL in values. The handler returns 400 with an explanation.
The proxy copied every client header to the rack except a five-entry denylist. The rack trusts several of those headers because the gateway authenticates as admin with the rack password: - Image/Volumes/Privileged on a process run let ops or an approved CI token start an arbitrary image with host paths mounted - X-Convox-Actor forged the rack's own audit trail - X-Convox-TID switched tenant namespace resolution - Client headers now pass through an explicit allowlist derived from the Convox SDK (sdk.Client.Headers, stdsdk.Client.Request) and the header tags on structs.LogsOptions, ProcessExecOptions, ProcessRunOptions and ObjectOptions. Credentials, cookies, actor and tenant overrides, X-Forwarded-*, hop-by-hop headers, Accept-Encoding, env headers and gateway-internal headers are dropped, on both HTTP and WebSocket requests. - Run options that escape the release image or scheduling constraints (Image, Volumes, Privileged, Node-Labels, Node-Affinity, Run-Tolerations, System-Critical, Run-Annotations, Run-Labels) need the new convox:process:run_privileged permission, which only the admin wildcard covers. API tokens can never use them. Requests without that permission get 403 and never reach the rack. - The gateway sets X-Convox-Actor to the user's email, or token:<name> for API tokens. - New StripInternalHeaders middleware runs first and removes client-supplied X-Audit-Resource, X-Release-Created, X-Original-Path, X-User-*, X-API-Token-*, X-Auth-Source, X-RBAC-Decision and X-Rack-* headers, which the audit logger and request logger read. - buildTargetURL escapes the authorized path, so %3F/%23 can't truncate the route the rack sees. Paths with control characters, '?', '#' or dot segments are rejected before route matching. - The WebSocket dial follows redirects only on the rack's own host, so the rack credential is never sent anywhere else.
…oduction Sentry was initialised with SendDefaultPII=true, so every 5xx event carried the Authorization header (session or API token plus inline MFA code) and session cookies. PII collection is now off, and a BeforeSend hook strips everything except a small header allowlist, plus cookies, query strings, request bodies and env. The gateway now refuses to start when its database is marked production while any test-only switch is set: DEV_MODE, E2E_TEST_MODE (skips WebAuthn assertion checks), AWS_ENDPOINT_URL_S3 or POSTMARK_API_BASE. GOOGLE_OAUTH_BASE_URL must be https there. Development and E2E databases are unaffected.
…okens - Pin every third-party action to a full commit SHA (newest release within the current major), with the version in a comment. - Install Task, the Convox CLI and TruffleHog from pinned releases with SHA-256 verification instead of piping install scripts (including golangci-lint's install.sh from master) into sh. The golangci-lint action already installs the pinned version and verifies the config schema, so the separate install and config-verify steps are gone. govulncheck is pinned to v1.8.0. - ci.yml and e2e.yml run with contents: read. The docs workflow gives pages/id-token write only to the deploy job, pins Bun and installs with --frozen-lockfile. - New daily Security Scan workflow: govulncheck, bun audit (high+) for web, and a Trivy scan of the published image, so new advisories surface without a push (CI was red for a month unnoticed). - Dependabot for gomod, bun (web, docs, mock-oauth), GitHub Actions and Dockerfiles, weekly with grouped minor/patch updates.
…curl - Pin oven/bun, golang and alpine base images by digest (runtime moves from alpine:latest to alpine:3.24.2) in all Dockerfiles, including mock-oauth's floating oven/bun:1-alpine. - The production image runs as UID/GID 10001. Binaries, web assets and start-gateway.sh stay root-owned and read-only; the gateway only writes temp files under /tmp. - curl is no longer installed in the gateway images. The compose healthcheck uses busybox wget, which is already in alpine. - Fix the commit hash in the gateway binary: the build stage read an unset COMMIT_HASH arg, so every image reported CommitHash=unknown. It now uses the required COMMIT_SHA build arg that CI already passes.
- ID tokens must have email_verified=true. - When GOOGLE_ALLOWED_DOMAIN is set, the hosted-domain (hd) claim must match it as well as the email domain (case-insensitive). Before, a bare suffix compare on the email was the only check. - The ID token verifier accepts RS256 only. Google never uses HS256, and allowing it only matters if the issuer can be swapped. - GOOGLE_ALLOWED_DOMAIN is required outside DEV_MODE. An empty value used to let any Google account sign in. - The mock OAuth server now emits hd like Google does for Workspace accounts.
- Listing deploy approval requests and reading a request's audit trail were mapped to deploy_approval_request:approve, which carries MFAAlways, so every page load demanded a fresh MFA code. They now use the read-level :list permission (admin-only); the handlers still require approver permission. A test asserts no GET route can require MFAAlways. - A 403 from the Authorize middleware names the missing permission, e.g. "insufficient permissions: requires convox:env:read". - E2E login helper: when login lands on the MFA challenge, complete it with a TOTP code like a user would. Enrolling TOTP directly in the DB for an already logged-in user now also marks that session MFA-verified, matching a real UI enrollment (step-up stays unset). - cli-login-webui clears TOTP replay state before generating its approve code, so another test on the same shard can't have used the time step already. - CLI E2E expectations updated for the new permission error message.
When the CLI runs in the background (e.g. from an agent), osascript isn't the frontmost app, so the dialog appeared without keyboard focus and the PIN field had to be clicked before typing. The script now runs `activate` first, which brings the dialog to the front with the field focused. No extra macOS permissions are needed.
An approval used to authorize "a build" for the token and app, so a
compromised CI job could ship any tarball or image under it: git-sha was
optional (DocSpring CI never sends one), the build's archive wasn't
compared with the upload, and image patterns were optional and unanchored.
Token builds under an approval now:
- must use the archive uploaded under that same approval, chosen
deterministically (no "most recent approved" lookup);
- take the commit from the approval record, never from the client. A
git-sha, if sent, must equal it, and approvals need a full 40-hex SHA;
- may only reference pre-built images tagged <sha> or <sha>-<suffix>.
Services built from source are rejected;
- require service_image_patterns, so the image repository is pinned
too. Patterns are anchored and the substituted commit is regex-escaped.
Also:
- approved_deploy_commands fails closed: an empty list allows no
commands under an approval. The CLI E2E seed stored it as
{"commands": [...]}, which was silently read as empty and so allowed
anything; it now uses the plain array production uses.
- GitHub verification: "branch" mode requires the commit to be on the
feature branch (compare status behind/identical, not any 200),
"latest" mode compares full SHAs, and URL segments are path-escaped.
- CircleCI auto-approve checks the workflow's pipeline revision and
repository against the approval (and rejects fork pipelines) before
approving the hold job. workflow_id must be a UUID.
- CircleCI and GitHub API tokens are no longer stored in River job
args; workers read them from config. The Jobs API redacts token-,
secret-, password- and key-like arg keys.
The redirect check only compared hosts, so a wss -> ws redirect on the rack host would resend the rack Basic credential in cleartext. The redirect must now keep both the host and the scheme of the configured rack URL.
The CLI login used to be a device flow without a user code: whoever held the gateway-issued `state` got the 90-day session. /auth/cli/complete never checked the code_verifier, states never expired, `state` and the OAuth code were written to the request log, and nothing tied the browser that approved the login to the CLI that polled. A log reader or anyone who got a user to open their login link could take over the session. Now: - The CLI generates the PKCE verifier and its own state, listens on 127.0.0.1 with a random port, and sends only the S256 challenge, state and loopback redirect URI to /auth/cli/start. - The browser that completes Google OAuth is bound to the login with an HttpOnly cookie. The MFA form, MFA submit and return steps all require it, so a third party holding the state can't use it or burn the user's MFA attempts. - After MFA, the browser is redirected to the CLI's loopback listener with a single-use login code (stored hashed, 10-minute TTL, consumed atomically). /auth/cli/complete needs the code AND the verifier. The polling endpoint and the old state-as-credential path are gone. - The MFA page shows the initiating IP and device name. A completed CLI login sends a notification. - state, code and login_code query params are redacted from request and audit logs. - The CLI only opens https auth URLs, and on Windows uses rundll32 rather than `cmd /c start` (which split URLs on '&'). E2E harness: the simulated browser stays on the gateway's hostname (the binding cookie is host-scoped, as in a real browser), and commands run with stdin from /dev/null. `env set` reads extra input from a non-terminal stdin until EOF, so an inherited open pipe hung the suite.
WebAuthn (server): - Challenges used to round-trip through the client as session_data and were trusted on the way back. One captured assertion could be replayed as a step-up proof indefinitely, and a zeroed Expires skipped go-webauthn's expiry check. - Challenges now live in a webauthn_challenges table, bound to the user and the session that started the ceremony. They are single-use (DELETE ... RETURNING), expire after a few minutes, and the client only gets an opaque ID back in the same session_data field. This covers login MFA, step-up, inline WebAuthn on the proxy, and enrollment. - The credential sign counter is stored and enforced (clone detection). CLI: - Every proxied Convox command went through stdsdk's http.Client and websocket dialer, which set InsecureSkipVerify. A man-in-the-middle with any certificate got the session token and MFA proof from the Basic auth header. Both now verify certificates against the system roots, and plain http:// is refused except for loopback gateways. - The rack-proxy URL's credentials are URL-encoded, so inline WebAuthn data can't break parsing (the parse error used to echo the token). - Inline step-up WebAuthn derives the RP ID from the configured gateway host and rejects a different RP ID sent by the server.
- The Release workflow runs only for v* tags. workflow_dispatch is gone:
it built whatever branch it was dispatched from, then pushed :latest
and overwrote the newest release's assets. v* tags are admin-only via
a repository ruleset.
- Per-job least-privilege permissions; the tag regex is anchored; tag
values reach shell steps through env: instead of ${{ }} interpolation.
- Images are pushed as :vX.Y.Z, :<short-sha> and :latest, with SLSA
build provenance attestations for the image and the CLI binaries
(verify with `gh attestation verify`). setup-go caching is off in
release jobs so a poisoned main-branch cache can't reach a release.
- convox.yml deploys an immutable tag instead of :latest. It's pinned
to :e327add (v0.1.1, same digest as the current :latest), and
scripts/bump-version.sh now sets it to :vX.Y.Z on each bump.
- The CLI reports its real version: ldflags set main.version /
main.buildTime, not the non-existent main.Version / main.BuildTime.
- Go tooling installs are pinned rather than @latest.
- Docs (CLAUDE.md, README, deployment docs) describe the new process.
TruffleHog runs its auto-updater by default. In CI it tried to replace the
checksum-verified binary installed root-owned in /usr/local/bin and failed
("cannot move binary back"), failing the secret scan. A pinned binary
must not update itself anyway, so pass --no-update.
…role Review fixes for PR #19: - Refuse query parameters the Convox SDK never sends. The rack reads options from the query string as well as headers and the body, so `?command=`, `?env=` and `?manifest=` could override the exec command, release env and build manifest the gateway had checked. The web UI's scale request now sends `count` in the form body, like the SDK. - deploy_with_approval no longer lets a token exceed its owner's current role. - Env change refusals name the keys and the reason ("ADMIN_PASSWORD is a protected env var for docspring. Unprotect it in rack-gateway settings to change it.") instead of "You don't have permission to create releases." Removing a secret now needs secret:set, like changing one. - Token names may not contain control characters (they become the X-Convox-Actor header). - The production guard also refuses AWS_ENDPOINT_URL and the STS/KMS endpoint overrides. Adds tests for token callers, the websocket header path and the query allowlist.
Sessions created while a user had no factor are marked MFA-verified at login. Once the user enrolls their first factor, those sessions must prove it too, so first enrollment clears their MFA state; the session that enrolled is re-verified. YubiKey enrollment now re-verifies its session like TOTP and WebAuthn do. Also adds tests that ID tokens are only accepted when signed with RS256 and that a locked owner's API tokens are rejected.
Review fixes for PR #19 (A03-F01, A03-F03): - The MFA challenge page and step-up dialog have a "Use a backup code" mode. Codes are accepted in any case, with spaces or dashes. Now that a pending session can't enroll a new factor, this is the recovery path for a user who has lost their authenticator. - Approving a CLI login with MFA also verifies the browser's web session for the same user, so the web UI doesn't ask for MFA again afterwards. - /api/v1/info reports mfa_pending, and the web UI sends a pending session to the challenge page instead of letting every request fail. - When several requests need step-up at once, the dialog now settles all of them (retry after verifying, reject on cancel) instead of leaving all but the last one hanging.
Review fixes for PR #19 (A01-F01, A01-F02, A06-F01, A06-F04, A06-F05). Locking admin routes down removed access the viewer, ops and deployer roles need. This restores it with ownership rules instead of admin-only routes: - Every user can view their own profile, sessions and audit trail and sign out their own sessions (route-table self rule on /users/:email: exact email match, people only, never API tokens). Other users' records still need gateway:user:read / user:update / audit_log:read. - The team directory (GET /users) needs the new gateway:user:list, which every role has. Lock and MFA details are only returned with user:read. - API tokens: gateway:api_token:read/create/update/delete cover the caller's own tokens; the new gateway:api_token:manage (admin) covers everyone's. Deployers can issue CI tokens for themselves. A token's permissions must be within its owner's current role at create and update time, as well as at request time. - /api/v1/info returns the caller's permissions so the web UI shows only what they can use; navigation and pages follow them. - Fixes the role editor, which called an unregistered route. - CLI: deploy-approval approve and test-auth handle API tokens clearly. - A test walks the real router: every /api/v1 route is public by design or has a policy, and every policy has a route. - Docs updated for the new permissions, token rules, self-service, the production safety check and the proxy header and query allowlists.
…alls .claude/ holds local agent worktrees (several GB), which made the CLI E2E deploy tarball exceed the gateway's 500MB manifest-check limit and would bloat any Docker build context. CLI commands in the E2E harness now get </dev/null so a command that reads stdin (env set) can't hang the run.
- The OTP, MFA and backup code inputs take focusOnMount instead of an autoFocus prop: they focus their first field from an effect and never used the native attribute. - Small cleanups DeepSource flagged: no redundant undefined arguments, the per-user audit log call uses the generated client, and the scale mutation is no longer async without an await. - Naming yourself as a token's owner is matched case-insensitively. - The redirect-after-login E2E spec enforces MFA for its user, so it no longer depends on global settings left by earlier tests in the worker.
Findings from the PR #21 review: - Approval-bound builds need an image pattern for every service (its own or "*"). Per-service patterns used to leave unlisted services' image repository unpinned. - The settings API rejects wrongly shaped values: approved_deploy_commands, protected_env_vars and secret_env_vars must be JSON arrays of strings and service_image_patterns an object of compilable patterns. A wrong shape used to read back as an empty list, which now allows no commands. - The CircleCI auto-approve job re-checks the deploy approval on every attempt and is cancelled once it is rejected, expired or deployed, so a retry can't approve the hold afterwards. - The pipeline repository check fails closed when the repository can't be determined. - Docs and settings help text describe the new rules; an upgrade note covers the rollout order (set service_image_patterns right after deploying), approved command shapes and token rotation. - Tests: pattern coverage, prefix git-sha, DocSpring's real manifest shape, listed and unlisted approved commands, approval re-check, unknown repository, empty revision, job-arg wiring and redaction. - The E2E deploy approval seed builds its commit SHA instead of a 40-hex literal, and the CLI E2E image pattern pins the repository.
# Conflicts: # scripts/lib/cli-e2e/cli_helpers.sh # web/e2e/cli-login-webui.spec.ts
Findings from the PR #22 review: - A first factor enrolled during an enrollment-required CLI login only completes the login if the browser that approved it verified its own session after the enrollment. - The Google code is exchanged in the callback and the browser is bound only after it succeeds, so a junk-code callback can't claim the login, and the Google code is no longer stored. - Failures after binding, provider errors and exchange failures go back to the CLI's loopback listener, so the CLI exits instead of waiting out its timeout. "Cancel Login" now cancels on the server (POST /auth/cli/cancel) and tells the CLI. - The CLI always prints the login URL, accepts any https identity provider, and reports version skew clearly in both directions. - Login codes are issued once; the CLI-login email dedup uses the client IP instead of the client-supplied device name. - Docs describe the loopback flow, the same-machine requirement (ssh -L for remote hosts) and the CLI upgrade needed after a gateway upgrade.
Findings from the PR #23 review: - CLI connections to the gateway ignore HTTP(S)_PROXY, so websocket commands no longer break behind a proxy, and stay on HTTP/1.1. - Plain http to a non-loopback gateway is refused where the gateway URL is loaded, covering MFA calls and all native CLI requests, not only the Convox SDK. - The proxied gateway URL is parsed properly (query, fragment and user info are rejected) and errors never include the session token. - The WebAuthn sign counter is compared and stored in one guarded UPDATE; a counter that doesn't advance is treated as a cloned key. - Tests for WebAuthn registration (session binding, expiry, replay, user verification) and for the exact Authorization header the CLI sends. - Docs: PIN/biometric requirement, 5-minute single-use challenges, one key touch per command, CLI TLS. Removes the outdated WEBAUTHN.md.
Findings from the PR #24 review: - Release docs and the bump script point at scripts/deploy_all.sh, which runs database migrations before promoting; production never migrates on startup, so plain `convox deploy` skipped them. The public upgrade guide no longer claims migrations run automatically. - Docker docs use wget healthchecks; the image no longer ships curl. - The release workflow's check wait paginates check runs, so the daily security scan can't push the required checks off the first page. - Prerelease tags don't move :latest.
- The CLI-login MFA submit consumes the WebAuthn challenge with the bound browser's own web session, and challenges started by a session can no longer be redeemed by a caller that names no session. - `rack-gateway login` refuses plain http to a non-loopback gateway before sending anything to it. - One isLoopbackHost helper (both branches declared it).
|
Warning Review limit reached
This review includes 300 billable files and costs up to $75.00. View limit detailsReview configuration: ⚙️ Run configuration
⛔ Files ignored due to path filters (3)
📒 Files selected for processing (300)
Comment |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| JavaScript |  |
Oct 9, 2026 11:22p.m. | Review ↗ |
| Go | |
Oct 9, 2026 11:22p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
This was referenced Oct 10, 2026
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.
Combines the 2026-10-09 security audit fixes and their review rounds into one release. It supersedes #19, #20,
#21, #22, #23 and #24; each of those PRs describes its own change in detail, and every one went through a
multi-area review whose findings are fixed here.
What's in it
scripts/deploy_all.sh, which runs migrations;:latest.Deploy notes
20261009000000_cli_login_loopback: in-flight CLI logins are dropped.20261009120000_webauthn_challenges.scripts/deploy_all.sh(staging → eu → us), which migrates before promoting.rack-gatewayCLI after the gateway is upgraded. CI uses API tokens and is unaffected.service_image_patternsfor thedocspringapp ({"*": "docker\\.io/docspringcom/app:{{GIT_COMMIT}}-amd64"}). Approval-bound CI builds are refused until it's set. Don't set it before deploying: the old gateway requires agit-shaonce patterns exist, and CI sends none.circleci_token/github_tokenfromriver_job.argsand rotate both tokens.Verification
Each original PR's checks passed on its own branch. On this combined branch,
task ci(lint, duplication, file length, shellcheck, build, Go tests, govulncheck, web tests, mock-OAuth tests, CLI E2E and web E2E) passed locally. Results: Go 847 tests + 18 integration, web 109, mock-OAuth tests, CLI E2E green, web E2E 60/60.