fix: quality hardening wave 3 — /video token guard, STT contract CI, secrets-scan fail-closed#7
Merged
Merged
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eline per-file) site/demo/ was blanket-excluded from all three secrets-scan paths, so a real secret pushed into the generated demo report would slip through undetected. Narrow the exclude to the vendored JSZip bundle only; site/demo is now scanned and its benign high-entropy assets stay pinned per-file in .secrets.baseline (already present, 0 new entries). Fail-closed proof added: a planted AWS-key shape in a site/demo file is caught end-to-end. - .pre-commit-config.yaml: exclude regex drops site/demo/, keeps vendor/ - Makefile secrets-check: drop ':(exclude)site/demo' pathspec - .screenscribe-verify.yml: drop site/demo/ from secrets_exclude (the third path, consumed by ss_verify.check_no_secrets) - tests/test_ss_verify.py: narrowing seam + planted-secret detection tests Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The source-video endpoints (/video on both servers + the review by-filename twin) served a FileResponse behind the Host guard only. Bring them under the session token like the rest of the API, reusing the signed ?st= query idiom the frame <img> URLs already use (a <video src> cannot carry the token header). - server_security: video_access_token() (HMAC of a fixed 'video' label keyed by the session token) + install_security(video_paths=...) guards those exact GET paths, accepting the signed st query or the header. - analyze: renders src="/video?st=..." via the shell render context. - review: the report is pre-generated (token-free) and stays shareable, so the video src is signed at serve time; the on-disk report is untouched (data: embeds are left as-is). Generated-report contract (html_report/html_pro) is not touched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the 'site/demo/' directory from the secrets scan exclusion list to ensure generated demo reports are scanned for secrets. It also implements a signed query parameter ('st') authentication mechanism for video endpoints, allowing secure video playback since '
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Cut F (f9daef8) accidentally removed the @app.get(f"/{video_path.name}") route when adding /{report_filename}. StaticFiles is mounted on output_dir, so a source video living OUTSIDE output_dir 404s without the explicit route even with a valid signed ?st token. Restore the route before /{report_filename} and add a regression test that exercises the out-of-tree case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The analyze theme test asserted --bg/--background/prefers-color-scheme, none of which exist in this tree. The shared-shell refactor delivers theming as inline CSS custom properties (design tokens): renderer._head inlines report-pro.css + screenscribe-theme.css into one <style> block, :root defines --surface-primary (the page background surface), and the base stylesheet consumes it via var(--surface-primary). Rewrite the assertion to check that real mechanism — token declared (:root, --surface-primary) AND consumed (var(--surface-primary)) — so it still fails if the theme layer is dropped or stops being inlined, not a loosened status check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
What this is
Third hardening wave executing three operator-confirmed backlog decisions (2026-07-09). Three focused commits, disjoint domains, each independently gated.
Changes
/videoserved with no session token (both servers + the by-filename twin)?st=query token (video_access_tokeninserver_security, same pattern as marker frames —<video src>cannot carry headers). Analyze signs the src in the render context; review signs at serve time so the on-disk report stays shareable/token-free. Unsigned GET → 403.stt-contract.yml: nightly cron + manual dispatch,pytest -m integration --run-integration(the flag matters — without it conftest skips everything and the run is a green no-op). Fails with a clear::errorwhen theLIBRAXIS_API_KEYsecret is missing. Not a required check;ci.ymluntouched. ~5 live API calls per run.site/demo/exclude in secrets scans.screenscribe-verify.ymlcontract); existing per-file baseline already covers the generated demo (0 new baseline entries). Fail-closed proven by tests planting an AWS-key-shaped secret insite/demo/and asserting the scan catches it.Test plan
make check,make test(1178 passed, 28 skipped),make verify(ss_verify 11/11 → READY).src="/video?st=…"→ 200; bare/video→ 403 on both servers.actionlintclean on the new workflow.Post-merge operator action
Add the
LIBRAXIS_API_KEYsecret (Settings → Secrets and variables → Actions) before the first nightlystt-contractrun; until then the workflow fails with an explicit configuration error message.🤖 Generated with Claude Code