Audit fixes: security, accessibility, test isolation, and CI/infra#37
Open
kmatzen wants to merge 6 commits into
Open
Audit fixes: security, accessibility, test isolation, and CI/infra#37kmatzen wants to merge 6 commits into
kmatzen wants to merge 6 commits into
Conversation
Address several issues from the project audit: - Fix stored XSS in the admin dashboard by HTML-escaping all participant-supplied values before interpolating into innerHTML (#25). - Harden admin auth: constant-time password comparison via hmac, and refuse to start in production with the default password (#26). - Fix test DB isolation so get_db() honors app.config['DATABASE']; tests no longer read/write the checked-in survey.db (#27). - Configure SQLite for concurrency (busy timeout + WAL) (#29). - Add GitHub Actions CI running pytest and jest (#30). - Add Dockerfile, .env.example, and CONTRIBUTING; fix .dockerignore that was stripping the README from the build context (#31). - Remove dead legacy code: sample_questions.py and the unused build_column_map helper/import (#35). - Add tests for the DB migration path, security headers, DB isolation, and admin auth (#34), raising migration coverage from 36% to 84%. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gJuKPCoiDH8Dq96hMBQAG
- Radio and likert inputs: replace display:none (which removed them from tab order and the accessibility tree) with a visually-hidden-but-focusable pattern, plus a :focus-within outline so keyboard users see focus. - Image lightbox: add a reusable attachLightboxTrigger() that makes zoom images focusable (role=button, tabindex, Enter/Space); give the lightbox dialog semantics (role=dialog, aria-modal), move focus in on open, trap Tab, and restore focus to the trigger on close. - Identity images: make focusable (tabindex) with Enter/Space toggling the mask overlay and aria-pressed reflecting state. - Retake modal: add dialog semantics, focus management, Escape-to-cancel, and a Tab focus trap; also fixes a listener leak where the backdrop-click handler was re-added on every open. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gJuKPCoiDH8Dq96hMBQAG
The committed lock file was out of sync with the resolved dependency versions, causing the new frontend CI job (which uses `npm ci`, requiring an exact lock match) to fail. Regenerated the lock file; `npm ci` now installs cleanly and all 99 jest tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gJuKPCoiDH8Dq96hMBQAG
Error handling (#36): - checkDevMode now checks response.ok and shows a persistent, role=alert error if the survey config can't load, instead of silently rendering an empty survey. - showError guards against a missing active section and supports a persistent (non-auto-hiding) mode. - Mask image now has an onerror handler. De-duplication (#33): - IDENTITY_COLORS in script.js is now the single source of truth for the identity/mask palette: thumbnail border colors are set inline from it and the 10 duplicated CSS border-color rules are removed. Admin flatten de-duplication (#32, safe part): - Extracted the copy-pasted JSON->row flattening from admin_results and admin_export into a single _flatten_result_row() helper plus a shared SELECT constant, preserving each endpoint's exact output (results keeps old-style preferred_method_* fields; export unchanged). Verified: 68 Python tests and 99 jest tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gJuKPCoiDH8Dq96hMBQAG
Adds a "UI screenshots" CI job that boots the app (DEV_MODE, gunicorn), drives Playwright/Chromium through the main surfaces -- demographics page, survey trial view, admin login, admin dashboard -- and uploads the PNGs as a `ui-screenshots` artifact. This gives every PR a downloadable visual preview from the Checks tab, since the Flask app can't use GitHub Pages. - scripts/screenshots.py captures each page independently (best-effort, so one failure never aborts the rest) and honors PW_EXECUTABLE_PATH for local runs against a preinstalled Chromium. - CI installs its own Chromium via `playwright install --with-deps`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gJuKPCoiDH8Dq96hMBQAG
The screenshot job now captures the demographics and survey-trial pages at both desktop (1280) and mobile (390) widths, so PR previews show the responsive layout (stacked tiles, compact A/B buttons, Likert labels). Admin pages remain desktop-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gJuKPCoiDH8Dq96hMBQAG
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
2 participants
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.
Implements fixes for issues found in the project audit (#25–#36).
Security
esc()helper before being interpolated intoinnerHTML.hmac.compare_digest(constant-time), and production refuses to start with the defaultadmin123password (previously only logged a warning).Correctness / reliability
get_db()now honorsapp.config['DATABASE'], so the suite uses the fixture's temp DB instead of reading/writing the checked-insurvey.db. Added a regression-guard test.timeout+PRAGMA journal_mode=WALto avoiddatabase is lockedunder multiple gunicorn workers.Accessibility (#28)
display:none(which removed them from tab order and the accessibility tree) with a visually-hidden-but-focusable pattern, plus a:focus-withinoutline.attachLightboxTrigger()(focusable, Enter/Space) and gave the lightbox dialog semantics with focus move-in, Tab trap, and focus restore on close.aria-pressedstate.Infrastructure / tooling
.github/workflows/ci.ymlruns pytest and jest on push/PR.Dockerfile(non-root, gunicorn,/datavolume),.env.example, andCONTRIBUTING.md; fixed.dockerignore(it excluded*.md, which would have stripped the README from the build context); documented Docker in the README.sample_questions.pyand the unusedbuild_column_maphelper/import.Tests (#34)
tests/test_migration.py(old→JSON schema conversion + idempotency) andtests/test_security.py(security headers, DB isolation, admin auth). Migration coverage rose from 36% → 84%.Verification
script.jspassesnode --check; app imports cleanly.Not included (tracked as follow-ups)
🤖 Generated with Claude Code
Generated by Claude Code