Repository navigation
Security hardening, CI, and show-ops features - #5
Merged
Merged
Conversation
Security hardening: - Gate POST /api/_test_reset behind CUELIGHT_TEST_MODE (never live in prod) - Require the join password on mutating endpoints + show log/report when set - Enforce label uniqueness on position reconnect, not just first join CI: GitHub Actions running ruff + pytest on Python 3.10 and 3.12. Show-ops features: - Operator attention signal (raise a flag + short message to the caller, works while locked; caller banner sits above the lock overlay) - Opt-in cue alert sounds/vibration on the position console - Show clock/timer (START SHOW, elapsed + since-last-GO, configurable) - On-demand show report computed from the log (nothing stored) - One-command launcher: run.sh / run.bat / python -m server
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR replaces problem signaling with attention signaling, renames show timing to a show clock using epoch fields, adds password-gated HTTP endpoints, rewrites show report generation, updates launcher startup, and refreshes frontend UI, tests, CI, and documentation. ChangesCueLight attention/clock/password/report rework
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Extracts server/timeutil.py verbatim from origin/main (exposes now_iso(), the canonical local-timezone ISO timestamp with millisecond precision). Replaces the inline datetime.now().astimezone().isoformat() call in ShowLog.record() with now_iso() — the only site that duplicated this job.
report_to_csv(): renders this branch's report dict as a 4-column CSV (section/label/metric/value) with formula-injection sanitisation via _DANGEROUS/_safe (ported from PR #4's approach, applied to all cells). report_to_html(): self-contained dark-theme HTML with inline CSS, no external dependencies, rendered from the same build_report() dict. GET /api/showreport now accepts ?format=txt|csv|html (existing default and password gating unchanged); csv is served as text/csv with Content-Disposition: attachment. Tests added to TestShowReport: csv format returns text/csv with attachment header and sane content; html format returns text/html with DOCTYPE; CSV injection — position label "=SUM(A1)" is sanitised to "'=SUM(A1)" in the CSV output.
…audio race
run.bat: add 'if errorlevel 1' failure guards after venv creation and
pip install so Windows users see a clear error message instead of a
silent launch failure.
server/ws.py raise_attention: change str(msg.get("message", "")) to
str(msg.get("message") or "") so a JSON null value cannot become the
string "None" in the attention message.
static/js/position.js cueAlert: guard beep() calls with
audioCtx.state === "running" check so audio does not attempt to
schedule into a suspended context before audioCtx.resume() resolves.
CLAUDE.md Show report section: document report_to_csv/report_to_html, _DANGEROUS/_safe sanitisation, and the ?format=txt|csv|html endpoint behaviour. Update HTTP API line to reflect the new format options. FEATURES.md Show report entry: describe the three download formats. API table row updated to show txt|csv|html.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
server/showreport.py (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the future annotations import.
This server Python file now starts directly with imports; add
from __future__ import annotationsbefore other imports. As per coding guidelines,server/**/*.py: “Usefrom __future__ import annotationsin every Python file.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/showreport.py` at line 1, The Python module is missing the required future annotations import at the top of the file. Add `from __future__ import annotations` before any other imports in this module, keeping the rest of the import order intact, so it matches the `server/**/*.py` convention and the existing pattern used in files like `showreport.py`.Source: Coding guidelines
static/js/caller.js (1)
750-774: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftPassword exposed via URL query string for log/report links.
pwQuery()puts the plaintext join password into the URL forwindow.opennavigation on download/view/report buttons. Query-string credentials are commonly logged in browser history, server access logs, and proxy logs, and can leak viaRefererheaders.This matches the server's documented support for
?password=(needed since link navigation can't set headers), but it's worth weighing a fetch+blob-download approach for these three buttons to avoid putting the password in the URL at all.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/js/caller.js` around lines 750 - 774, The download/view/report button handlers in caller.js are leaking the join password through pwQuery() by appending it to the URL passed to window.open. Replace these link navigations with a flow that keeps the password out of the URL, such as using fetch with pwAuth() to send the X-CueLight-Password header and then handling the response as a blob/text for the existing downloadLogBtn, viewLogBtn, and showReportBtn actions.run.sh (1)
13-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd friendly failure messages to match
run.bat's guards.
run.batexplicitly reports "Failed to create the virtual environment"/"Failed to install dependencies" on error;run.shrelies solely onset -eto exit silently with the raw tool output, which is less helpful for non-technical users running this launcher.🛠️ Proposed fix
if [ ! -d .venv ]; then echo "First run: setting up (this takes a minute)…" - python3 -m venv .venv + python3 -m venv .venv || { echo "Failed to create the virtual environment."; exit 1; } fi -./.venv/bin/pip install --quiet -r requirements.txt +./.venv/bin/pip install --quiet -r requirements.txt || { echo "Failed to install dependencies. Check your internet connection and try again."; exit 1; } exec ./.venv/bin/python -m server "$@"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@run.sh` around lines 13 - 19, Add explicit failure handling in run.sh to mirror the user-friendly guards in run.bat: after the virtualenv creation step and after the pip install step, check for failure and print clear messages like “Failed to create the virtual environment” and “Failed to install dependencies” before exiting. Keep the existing launcher flow in run.sh (the venv setup, pip install, and exec call), but wrap the failing commands so non-technical users get a helpful message instead of only raw tool output.static/position.html (1)
26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueIcon-only button lacks an accessible label.
#attentionBtnonly renders "⚠" with noaria-label/title, unlikelockAttentionBtnwhich has a text label.- <button class="btn-attention" id="attentionBtn">⚠</button> + <button class="btn-attention" id="attentionBtn" aria-label="Report a problem">⚠</button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/position.html` at line 26, The icon-only attention button in the position page lacks an accessible name, unlike the labeled lock attention control. Update the attentionBtn element in the relevant markup to include an accessible label such as aria-label and, if appropriate, a title so assistive technologies can identify the button’s purpose.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 17-21: The CI job’s actions/checkout step is leaving persisted git
credentials enabled even though the workflow is read-only. Update the checkout
configuration in the workflow to set persist-credentials to false on
actions/checkout v4 so the token is not stored in git config; keep the existing
actions/setup-python and matrix.python-version setup unchanged.
In `@server/persistence.py`:
- Around line 65-66: The snapshot restore path in persistence handling does not
preserve legacy show-clock data, so older snapshots with show_start_time end up
resetting state.show_started_at to 0.0. Update the restore logic in the
persistence code to detect the legacy field and migrate it into the current
epoch-based show_started_at value, or explicitly version and invalidate outdated
snapshots; keep the fix localized around the snapshot loading/restoration flow
that sets state.show_started_at.
In `@server/showreport.py`:
- Around line 160-168: The report formatting in showreport’s latency rendering
is appending the seconds unit even when standby/go ack latency values are
missing, producing “—s” in both text and HTML output. Update the formatting
logic in the report builder that handles s['standby_ack_avg_s'],
s['standby_ack_max_s'], s['go_ack_avg_s'], and s['go_ack_max_s'] so the
placeholder stays unitless when the value is None, while still showing “s” for
real numeric latencies.
- Around line 9-18: The CSV sanitization helper _safe currently misses values
starting with LF, so a user-controlled string can bypass the formula-prefix
guard. Update the _DANGEROUS tuple in server/showreport.py to include newline as
an additional trigger, and keep the existing _safe logic in sync so any value
starting with a spreadsheet formula prefix, including LF, is prefixed with a
single quote before export.
In `@server/state.py`:
- Around line 378-388: `raise_attention` currently stringifies the incoming
message directly, which can persist the literal "None" when callers pass a null
value. Update `raise_attention` in `state.py` so the `attention_message`
assignment coerces `None` to an empty string before applying the max-length
trim, and keep the existing `attention_raised` logging behavior unchanged.
In `@static/js/caller.js`:
- Around line 374-385: Replace the inline visibility toggles in the clock modal
click handler by using UI state classes instead of setting `style.display` on
`startShowBtn` and `clearClockBtn`. Update the
`clockStrip.addEventListener("click", ...)` logic in `caller.js` to add/remove
or toggle a shared hidden class based on `state.show_started_at`, and ensure the
matching CSS utility class (such as `hidden`) exists or is reused so the modal
button visibility follows the project’s `classList` convention.
In `@static/js/position.js`:
- Around line 219-224: The UI state toggles in applyDim() and
openAttentionPanel() should use classList instead of rebuilding className or
setting style.display directly. Refactor the dim overlay and attention panel
visibility logic to add/remove/toggle state classes (for example, hidden plus
dim/red variants) on the existing elements, and keep the text updates and
localStorage persistence unchanged.
- Around line 251-289: The first alert cue can be skipped because cueAlert()
checks audioCtx.state immediately after ensureAudio() calls resume()
asynchronously. Update ensureAudio() to return/await the resume promise, and
have cueAlert() wait for audioCtx to reach running before calling beep() for
standby/GO cues. Use the ensureAudio() and cueAlert() helpers in
static/js/position.js to keep the first sound from being dropped after alerts
are enabled.
---
Nitpick comments:
In `@run.sh`:
- Around line 13-19: Add explicit failure handling in run.sh to mirror the
user-friendly guards in run.bat: after the virtualenv creation step and after
the pip install step, check for failure and print clear messages like “Failed to
create the virtual environment” and “Failed to install dependencies” before
exiting. Keep the existing launcher flow in run.sh (the venv setup, pip install,
and exec call), but wrap the failing commands so non-technical users get a
helpful message instead of only raw tool output.
In `@server/showreport.py`:
- Line 1: The Python module is missing the required future annotations import at
the top of the file. Add `from __future__ import annotations` before any other
imports in this module, keeping the rest of the import order intact, so it
matches the `server/**/*.py` convention and the existing pattern used in files
like `showreport.py`.
In `@static/js/caller.js`:
- Around line 750-774: The download/view/report button handlers in caller.js are
leaking the join password through pwQuery() by appending it to the URL passed to
window.open. Replace these link navigations with a flow that keeps the password
out of the URL, such as using fetch with pwAuth() to send the
X-CueLight-Password header and then handling the response as a blob/text for the
existing downloadLogBtn, viewLogBtn, and showReportBtn actions.
In `@static/position.html`:
- Line 26: The icon-only attention button in the position page lacks an
accessible name, unlike the labeled lock attention control. Update the
attentionBtn element in the relevant markup to include an accessible label such
as aria-label and, if appropriate, a title so assistive technologies can
identify the button’s purpose.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26be9af8-d3dc-493d-9368-b8912ca9cf10
📒 Files selected for processing (24)
.github/workflows/ci.ymlCLAUDE.mdFEATURES.mdREADME.mdrun.batrun.shserver/__main__.pyserver/main.pyserver/models.pyserver/persistence.pyserver/showreport.pyserver/state.pyserver/ws.pystatic/caller.htmlstatic/css/caller.cssstatic/css/common.cssstatic/css/position.cssstatic/js/caller.jsstatic/js/editor.jsstatic/js/observer.jsstatic/js/position.jsstatic/observer.htmlstatic/position.htmltests/test_cuelight.py
…t refactors - ci.yml: set persist-credentials: false on checkout (read-only workflow) - persistence.py: migrate legacy show_start_time ISO snapshots to epoch show_started_at so a mid-show upgrade keeps the show clock - showreport.py: add LF to formula-injection prefixes; render missing ack latencies as bare em-dash instead of "—s" (text + HTML) - state.py: coerce None attention messages to "" in raise_attention - caller.js/position.js: replace style.display and className rebuilds with classList toggles (.hidden utility added to common.css) - position.js: ensureAudio() returns the resume() promise; cueAlert() beeps after it settles so the first alert isn't dropped while suspended - run.sh: friendly venv/pip failure messages to match run.bat guards - position.html: aria-label/title on the icon-only attention button
- position.js: the pointerdown listener that resumes the AudioContext was
{ once: true }, so after the first tap nothing could re-resume a context
suspended by an iOS audio interruption (call, Siri) — cueAlert's resume()
runs outside a user gesture and can be rejected, silently killing beeps
for the rest of the show. Restore main's behavior: any tap re-arms audio
while alerts are on.
- showreport.py: remove unused _PAIRS dict; build_report() does the
called/acked pairing inline and the constant's comment implied otherwise.
Replace pwQuery() query-param navigation on the three showlog/showreport buttons with header-authenticated fetches: view buttons open a tab synchronously (Safari popup blocker) and point it at a blob URL; the CSV export downloads via a temporary anchor. Keeps the join password out of uvicorn access logs and browser history. Server-side ?password= support is unchanged for curl/direct-URL use.
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.
Summary
Security hardening, CI, and the five show-ops features (attention signal, show clock, cue alerts, show report, one-command launcher) — now reconciled with the parallel implementation that was merged via #4.
How this relates to #4
#4 was an independent implementation of the same five features by a scheduled agent (different terminology:
raise_problem,report_csv/report_html). This branch resolves the collision as a hybrid:fb50315,-s ours): attention system with inline caller banner readable/clearable while locked, show clock with server-offset drift correction and configurable segments, WebAudio cue alerts, on-demand show report, launcher. This side was interactively refined and visually verified end-to-end.server/timeutil.pyshared timestamp helper, CSV + HTML show-report formats (?format=txt|csv|html) with formula-injection sanitization,run.batfailure guards, null-message coercion inraise_attention, and a suspended-AudioContext guard on cue beeps.Also in this PR (additive, not in #4)
POST /api/_test_resetgated behindCUELIGHT_TEST_MODE=1(404 otherwise)X-CueLight-Password/?password=)register_position()Verification
ruff check .clean; 125 tests pass (3 new: report csv/html formats + CSV-injection sanitization)🤖 Generated with Claude Code
Summary by CodeRabbit
python -m serverusage.