fix: resolve every open bug-labelled issue (#257, #258, #259, #263, #267, #268) - #297
Merged
Conversation
`build_link` fell back to `request.base_url` whenever `PUBLIC_BASE_URL` was blank — i.e. to the client-supplied `Host` header. Because `POST /api/auth/password-reset/request` is unauthenticated, an attacker could request a reset for a victim with `Host: attacker.example` and the real deployment would mail the victim a genuine link carrying a valid single-use reset token pointed at the attacker. The raw token is the entire authorization, so clicking it is account takeover. Invite links shared the builder. The request is now never consulted: `build_link(path, token)` roots every link at the configured base and `link_base()` returns None when unset. Fail closed at three levels — startup refuses `SMTP_HOST` without `PUBLIC_BASE_URL`, the admin API refuses to enable email while it is blank, and the reset/invite endpoints return 503 rather than mailing a request-derived link. Closes #258 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every adapter computed `email_verified` — Entra even handles `xms_edov` and demotes the mutable `preferred_username` fallback — but nothing read it. `provision_oidc_user` JIT-created the local `User` with the asserted email regardless. On an IdP where a user can set an address before verifying it, an attacker signs in as `victim@corp.com` and takes the email column: the real person is locked out of both registration and their own later SSO login, and facilitators — who enrol from a picker showing only email and display name — hand the attacker that team's injects and communications. The collision checks above only stop an unverified address *taking* an existing row; nothing stopped it pre-claiming a future one. JIT creation now requires an IdP-asserted verified email and audits the refusal like the collision denial. `OIDC_ALLOW_UNVERIFIED_EMAIL` (env-only, default false) restores the previous behaviour for an IdP that genuinely never emits the claim. Closes #257 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`SIEM_HTTP_TOKEN`, `SMTP_PASSWORD`, and the proxy credentials are env-only precisely so admins cannot read them — the effective-config view shows set/unset booleans, never values. But three admin-editable *host* fields decide where each one is sent, and each pairs with a "test" endpoint: PUT `http_endpoint`, `smtp_host`, or `proxy_url` at a host you control, press test, and the app hands over the secret. The boundary the docstrings assert was one settings-PUT from being defeated. `sink_pinning.check_pinned` now refuses a runtime edit that would move the destination origin while the matching secret is set (422), following `llm_settings_service.validate_selection`, which pins provider base URLs for the same reason. Path edits on the pinned host and clearing the sink entirely stay allowed — an operator must keep the off switch, and with no destination the secret goes nowhere. Independently, any destination move now emits a `critical` audit event naming the old and new origin, so a re-point is visible in the trail even where nothing is pinned. Closes #259 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Same shape as #22/#210, in newer code paths — invisible on demo data, linear in exercise activity: - `GET /responses` resolved branch suggestions per response: a scenario-definition load, an inject fetch, and a scan of the *entire* pending-inject set for every row. `response_next_inject_suggestions_bulk` loads each once and resolves in memory; `select_next_injects` is the pure part both paths now share. - `GET /inject-comments` re-fetched the inject, re-ran the caller's `ExerciseMember` lookup, and fetched the author — per comment. The router now resolves the inject map and the caller's group once, and `comment_payloads` batches authors into one `in_()`. - `GET /injects` re-ran the same membership lookup for every inject in the list; it resolves the group once and filters with the new pure `access_control.inject_visible_to`. Each endpoint gets a query-count regression test asserting the statement count does not move when the row count six-times; all three fail against the previous code (39 statements vs 9). Closes #263 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`release.yml` triggered on any `v*` tag and went straight to building, pushing to GHCR, SBOM-attesting, and cosign-signing. Branch protection covers pushes to `main` but not tag pushes, so a tag on a branch commit, a commit that never went through a PR, or one whose CI failed published a signed, provenance-attested public image of unvalidated code — with the signing machinery from #73 actively lending it credibility. A `verify` job now gates `release` via `needs`: the tagged commit must be contained in `origin/main`, and the most recent completed `ci.yml` run for that exact SHA must have concluded `success`. It queries the CI workflow's own runs rather than the commit's check-runs, which would also match this workflow's own jobs. Both checks are conditioned on `SHOULD_PUSH`, so a `workflow_dispatch` dry run stays a build-only smoke test while a dispatch that would publish is held to the same bar as a tag. Closes #267 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The backup CronJob ran `pg_dump` only, while inject attachments live as files on the `app-uploads` PVC and are referenced from the DB by `inject.attachment_path`. Restoring after PVC loss (or moving to a new cluster) therefore produced a consistent-looking database whose injects point at files that do not exist — the record says the evidence is there and every download fails. Nothing in the manifests or the docs said the "backup" covered half the durable state, so an operator who set it up reasonably believed they were covered. The job now archives `/uploads` to the same backup PVC under the same timestamp as the dump, prunes both on the same retention, and mounts the claim read-only. `app-uploads` is ReadWriteOnce, so a `podAffinity` rule co-schedules the job with the app pod (droppable on ReadWriteMany). The docs point at `python -m app.reconcile_attachments` as the post-restore integrity check. Closes #268 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`secret_var="SIEM_HTTP_TOKEN"` reads as a hardcoded credential to bandit's hardcoded_password_funcarg check (`^secret_` matches its word pattern), which turned the blocking CI security scan red on three call sites. Renamed to `credential_var`/`credential_present`; behaviour unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…258) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Release verify job (#267): an annotated tag's GITHUB_SHA is the tag *object*, not the commit CI ran against, so the head_sha CI-run lookup found zero runs and refused a valid release. Peel to the commit first and use it for both the ancestry check and the API query. (docs/RELEASING.md shows a lightweight tag, which happened to work; `git tag -a` did not.) - OIDC unverified-email denial (#257): the service emitted an auth.oidc_login deny and the router's OIDCProvisionError handler then emitted a second one for the same attempt — the collision path emits exactly once, and double events skew SIEM denial counting. Drop the service-level emit; the callback test now pins exactly one deny event. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The merge kept #287's checkout bump on every pre-existing step but the new verify job (#267) still carried the pre-bump SHA with a stale tag comment — zizmor's online ref-version-mismatch audit (skipped in offline local runs) fails CI on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
richardmhope
added a commit
that referenced
this pull request
Aug 1, 2026
chore: untrack machine-local dotfiles swept into main by #297
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.
Resolves every open issue labelled
bug— one commit per issue, each self-contained.Security
Closes #258— reset-link poisoning.build_linkfell back torequest.base_urlwhenPUBLIC_BASE_URLwas blank, i.e. to the client-suppliedHost. Since the reset request is unauthenticated, an attacker could make the real deployment mail a victim a genuine link — carrying a valid single-use reset token — pointing at the attacker. The request is no longer consulted at all; failure is closed at three levels (startup guard, admin-API guard, 503 at the endpoint) so the feature can never silently degrade to a request-derived link.Closes #257— OIDC unverified-email squatting. Every adapter computedemail_verified; nothing read it. Collision handling stopped an unverified address taking an existing row but not pre-claiming a future one — locking the real person out and collecting the enrolments facilitators make by email. JIT creation now requires the assertion and audits the refusal;OIDC_ALLOW_UNVERIFIED_EMAILis the documented escape hatch.Closes #259— secret exfiltration via sink re-pointing. Three admin-editable host fields decided where an env-only secret went, each paired with a "test" button. Newsink_pinningrefuses a runtime edit that moves the destination origin while the matching credential is set (422), following thellm_settings_service.validate_selectionprecedent. Clearing a sink and editing the path on the pinned host stay allowed; every destination move now emits acriticalaudit event.Closes #267— unguarded release publish. Branch protection does not cover tag pushes, so av*tag on any commit published a signed, provenance-attested image. Averifyjob now requires the commit to be onmainwith a greenci.ymlrun for that exact SHA.Correctness / performance
Closes #268— half-backed-up state. The CronJob dumped Postgres only, so a restore yielded a database whose injects referenced attachment files that no longer existed. Database and theapp-uploadsPVC are now captured in one job under one timestamp, with apodAffinityrule for the RWO claim.Closes #263— N+1 in three list endpoints. Facilitator responses, inject comments, and the participant inject list each resolved per row. All three now cost a fixed number of queries; new regression tests assert the statement count does not move when the row count six-times, and all three fail against the previous code (39 statements vs 9).Verification
uv run pytest— 692 passed, 2 skipped (the Playwright suite, which skips without a server on:8765; CI runs it).ruff check,pyright app/(0 errors),bandit(clean — one commit renames thesink_pinningkwargs that tripped B106's heuristic),uv lock --check,zizmor .github/workflows/,actionlint.SMTP_HOSTempty, so the new startup guard does not trip existing deployments.🤖 Generated with Claude Code