Repository navigation
Phase 1: security, data integrity and Resolve EDL fixes; broader test battery - #6
Merged
Merged
Conversation
…nor updates - zod 4.6.7 fixes a high-severity memory exhaustion advisory (< 4.6.6) - @fastify/cors 11 only changes the default methods, which the server sets explicitly - Dependabot ignores @types/node majors until the runtime moves to that major
…and lookup limits - transaction(): a failed BEGIN no longer leaves every later transaction running without one; the rollback keeps the original error; afterCommit() for broadcasts that must not outlive a rollback - Stopping a session that already stopped neither moves its end nor orphans the running session - Markers never go into a stopped session (a dock that hasn't seen SESSION_STOPPED yet) - Undo only reaches the running session, as documented - Partial status updates are validated against the merged OBS framerate - Migration v5 repairs sessions v4 closed before they started; a newer schema is refused and the database file is closed when it can't be opened - Restart recovery: running sessions resume after a recent clean shutdown and close at the last heartbeat after a crash; members are no longer reported as recording until their dock rejoins - Unknown room codes count per IP across REST and WebSocket; custom codes need 8+ characters - Sockets that leave their room or whose room is deleted are closed after the unjoined timeout; simultaneous sockets per IP are capped - Exports of an unknown session answer 404; repeated query parameters answer 400 - Empty PORT / HOST / LOG_LEVEL / ROOM_CREATE_LIMIT_PER_MINUTE use their defaults - RFC 5987 file names percent-encode ' ( ) *; startup errors are logged before exiting
…mecode in FCP XML - The DaVinci Resolve export now writes the layout Resolve itself produces with Timeline Markers to EDL (one-frame event + |C:ResolveColor<Color> |M:<name> |D:1) instead of Avid-style * LOC: locators; '|' in names is replaced, White maps to Cream (Resolve has no white marker) - FCP 7 XML writes the base timecode with ';' for drop-frame sessions - Golden-layout and property tests for the marker EDL; import steps updated in both languages
…input limits and a11y - Tokens and the member id are kept per server: a ?server= link can no longer make the page send the credentials of one server to another; older shared credentials are only reused with that server after the user accepts it in a banner naming it - OBS: a retry left by an earlier failed attempt no longer closes a good connection (reconnect loop on double Connect / React StrictMode); attempts finishing after disconnect are closed - Server socket: the backoff resets on JOINED instead of on open; deleted or unknown rooms are not rejoined on every reconnect; late events of a replaced socket are ignored; the SNTP filter re-baselines after 3 consecutive RTT spikes instead of rejecting samples forever - ?room= follows the room in use; TIMEOUT no longer replaces the banner - Inputs limited to the server's lengths (name, room code, marker text, session name); the base timecode is validated before the start dialog closes; long stored names are trimmed - A custom marker's text is no longer saved as both label and comment - Dock shortcuts ignore key repeat, modifier keys and open dialogs - Dialogs trap and restore focus; missing labels added; category translated in the timeline - Clipboard failures are caught; export file names follow the server's rule; download links URL-encode their ids; API errors are shown with a translated prefix - The Director Panel no longer shows the previous room's session history after switching rooms
…resholds; fix contrast - Vitest projects: unit (node) and browser (Chromium via @vitest/browser-playwright with vitest-browser-react) for dialogs, focus handling, clipboard, file names and the start form - e2e/a11y.spec.ts runs axe (WCAG 2.1 A/AA rules) on the dock, HUD preview, Director Panel and both dialogs in English and Spanish; the grep-only dialog checks are replaced by these - Coverage counts every source file (66 % statements, measured) and now has thresholds - vitest.config.ts -> vitest.config.mts (no more ESM-as-CommonJS warning) - Contrast fixes found by axe: secondary text zinc-500/600 -> zinc-400, emerald/sky buttons one shade darker; labels for the filter, search and POV selects; inactive download links are aria-disabled and not focusable - CI runs the browser tests and coverage in the job that installs Chromium
…testing - ACCESSIBILITY.md / ACCESSIBILITY.es.md say only what is verified (axe-core WCAG 2.1 A/AA rules on the main screens in both languages, dialog focus tests) and what is not tested yet (screen readers, Firefox, Safari); the previous text named marker categories that don't exist and screen readers that were never tried - README: WCAG is a target, not a conformance claim; test counts recounted (289 at v1.6.2) - The database stress test really interleaves its 20 writers and takes the backup in the middle of them, and asserts it; the README no longer claims 2,000 writes/sec - Spanish community files sit next to the English ones; language selector in CODE_OF_CONDUCT.md - Stryker config (on demand, not a dependency) and a manual workflow; tests for the mutants it found: mutation score of the timecode engine and exporters 85.9 % -> 93.7 % - CHANGELOG [Unreleased] in English and Spanish
… keys, review findings Independent review of the phase 1 changes: - After ROOM_NOT_FOUND the same code could not be joined again without a reload (regression): reconnects no longer retry it by themselves, but entering the code again asks once more - Tokens of the page's own server are now kept under their own key, so the trust question for another server only offers credentials written before v1.6.2 - A HUD opened while the trust question is pending still connects (it sends no credentials) - The Director Panel no longer shows the previous room's sessions until the new room answers - A failing after-commit callback no longer hides a committed transaction or skips the others - A marker pressed just before the host's Stop stays in that session instead of opening an Auto Session; one pressed after it goes to the running session - MAX_SOCKETS_PER_IP (default 64), with a warning in the log when a connection is refused; README and .env.example explain the per-IP limits behind a proxy - axe scans run with reduced motion, so they never measure text halfway through a fade-in
…efore marking Second independent review: - Entering the same room code reconnects only when this client is not in that room (refused or deleted), tracked in React state: a joined dock showing an unrelated error keeps its socket, and the retry still works after the error banner was dismissed - MAX_SOCKETS_PER_IP default lives with the other limits (config no longer loads the WebSocket handler); comments on the shared pre-v1.6.2 token copy and on the HUD made accurate - e2e openView() waits for the server's JOINED (opt out for invalid codes and the offline-server test): tests pressed hotkeys before the join, which made the export test fail now and then
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f667cfe40b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+318
to
+319
| if (msg.payload.code === 'ROOM_NOT_FOUND') { | ||
| this.autoRejoin = false; |
There was a problem hiding this comment.
When a client receives RATE_LIMITED for the per-IP lookup limit or socket cap, the server closes the socket, but this branch disables auto-rejoin only for ROOM_NOT_FOUND. Because joinRoom() leaves autoRejoin enabled, each reconnect retries the same join and is refused again until the limit clears. Disable auto-rejoin for this refusal so the client waits for an explicit retry.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Not applied: RATE_LIMITED is temporary, and disabling auto-rejoin would make the Dock worse.
- Per-IP socket cap (
handler.ts, on connect): the refusal happens before any join.autoRejoindoesn't stop reconnecting, it only decides whether to sendJOIN_ROOMafterwards, so turning it off would leave the Dock reconnecting forever without ever entering the room, even once a slot frees up. - Unknown-code block: it uses a fixed window, and refused attempts don't count toward it (
recordFailureonly runs onROOM_NOT_FOUND, after theisBlockedcheck). Retrying doesn't extend the block, and the Dock gets back in by itself once the window ends. An unknown room already disables auto-rejoin throughROOM_NOT_FOUND. - Per-socket message limit: it doesn't close the socket and has nothing to do with joining.
The reconnect backoff spaces retries up to 15 s and only resets on JOINED. A test now pins this behaviour (rejoins by itself after a rate-limit refusal, once the server lets it in again, in e909d6d), and the reasoning is in a comment next to this branch.
The server, its healthcheck and the backup command only need node; the npm bundled with the base image brought five high-severity advisories in its own dependencies (brace-expansion, undici, http-cache-semantics) that Trivy reported. They are gone from the scan; Debian's own advisories (none with a fix yet) stay until the base image moves to trixie.
DaVinci Resolve keeps at most one timeline marker per frame (its scripting API returns markers as a dict keyed by frame), so two streamers marking the same frame lost one of the markers on import. Markers on the same frame are now one event whose name lists all of them. Found by reading the exports back with OpenTimelineIO's CMX 3600 adapter, which rejects overlapping events.
An oversized frame (or any frame breaking the protocol) made @fastify/websocket's default handler log an error-level entry per socket, so a client could flood the error log. Its documented errorHandler option now logs ws protocol errors (WS_ERR_*) and abrupt disconnects as warnings and keeps real errors at the error level; the socket is dropped either way. Found by fuzzing the running server.
With storage blocked (some locked-down browser or OBS profiles) the member token issued at join was not kept anywhere, so after any reconnect the server refused the member id and the Dock came back as a new member. Values now also live in memory for the page load. Found by a chaos test that kills and restarts the server under 20 real socket clients.
The server imports ws at runtime (WebSocket.OPEN in the room manager and the socket handler) and the client's mock OBS server uses it in tests, but neither declared it: it only resolved because @fastify/websocket and obs-websocket-js depend on it. Same version as already installed (8.22.0). Found by Knip.
The root package was private but @streamcollab/shared, server and client were not, so an npm publish across workspaces would have published them; they also had no license field (the project is MIT).
It started when the server created it, slightly after the calibrated key press, so markers pressed in the first few milliseconds were all clamped to relative time 0. Found in the in-browser walkthrough (four quick hotkeys all on 01:00:00:00).
…allback and known limits From a CodeQL (security-and-quality) run: - Room code characters were drawn with byte % 31, so 8 of the 31 characters came up more often; crypto.randomInt draws them uniformly (chi-square test added) - New member ids came from Math.random; they now use crypto.getRandomValues (64 bits), which also works on plain-http LAN pages, so another device can't guess and claim an id first - SECURITY.md: an email fallback while GitHub private vulnerability reporting is not enabled, and the by-design limits CodeQL pointed at (credentials in browser storage, ?server=)
… session timeline Found in a real browser walkthrough with a mock OBS: - The Dock sent color 'Blue' with every marker, which overrode the server's per-category colors (Kill red, Wipe fuchsia...): all Dock markers were blue in every export since v1.0.0. The color is now only sent when chosen; the e2e export test checks the exact colors per category. - While OBS recorded, the big SMPTE clock showed the recording file's position with the session's base timecode (minutes apart from the markers listed under it). It shows the session timeline, on the server-calibrated clock, whenever a session runs.
The Resolve import steps said a per-POV EDL starts at 00:00:00:00; it uses the session's base timecode (01:00:00:00 by default), with the recording file starting there.
…D, members and marker editing
…flow, focus and the new axe states
…n merged EDL marker colors
… the limit clears
…e equivalent ones
This was referenced Oct 25, 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
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.
Phase 1 of the improvement plan: fix, debug and test. Everything below is under
[Unreleased]in both changelogs; the version stays 1.6.1 until a release is decided.Security
?server=link could make the page send saved host and member tokens to another server. Tokens and the member id are now kept per server; credentials saved before this version are only reused with another server after the user accepts it in a banner naming it.MAX_SOCKETS_PER_IP, default 64).crypto.randomInt: the oldbyte % 31made 8 of the 31 characters more likely (a chi-square test now guards it).crypto.getRandomValues(64 bits) instead ofMath.random, so another device can't predict and claim them.?server=).Fixes
|C:ResolveColor<Color> |M:<name> |D:1), imported with Timelines → Import → Timeline Markers from EDL. Checked against an EDL exported by Resolve and three independent implementations. Markers that land on the same frame are merged into one (all names, the first one's color), because Resolve keeps one marker per frame. Still to be tried in Resolve itself.BEGINno longer disables later transactions;.envvalues mean the default; an oversized WebSocket frame is logged as a warning, not an error (a client could flood the error log).wsis declared where it is imported; the workspace packages are markedprivateand MIT.Tests (all green locally and in CI)
stryker.config.mjsnow sets a 60 s timeout, and the honest score was then 85.8 % (the same status for every mutant in two runs).packages/shared/test/mutants-v162.test.tskills the mutants that were real gaps: the timecode parser accepted junk around a timecode when its^/$anchors ortrim()were removed, the drop-frame validation of session framerates had no test inshared, chapters past one hour, the 10 s spacing after the intro chapter, EDL text cleanup and category colors, XML defaults and padding, CSV fields missing in old rows and drop-frame timecodes.framesPerTenMinutesfields of the framerate table, which nothing reads.JOINEDbefore pressing hotkeys (this removed an intermittent failure in the export test).Verified outside the unit tests
X-Forwarded-Fordoesn't bypass the rate limits.npm audit(0 vulnerabilities) and CodeQL (security-and-quality): the two real CodeQL findings are fixed; the rest are documented limits or a false positive (the public default display name).compare/links of the CHANGELOG answer 404 until the v1.0.0–v1.6.0 tags exist on GitHub (only v1.6.1 does).Documentation
Not verified here