fix: code review (WS race, CSWSH, PBKDF2, base64 chunking, timeouts, Docker bumps) #4

Merged

skr merged 2 commits from fix/code-review into main 2026-05-08 12:49:03 +00:00

Conversation 0 Commits 2 Files changed 7 +129 -53

skr commented 2026-05-08 12:42:52 +00:00

Owner

Copy link

Addresses findings from code review against Context7-verified docs:

• gorilla/websocket concurrent-write race -> per-conn write mutex
• CSWSH via permissive CheckOrigin -> Origin host must match Host
• OWASP PBKDF2-HMAC-SHA256 minimum -> 600,000 iterations
• crypto.ts base64 -> chunked conversion (spread fails on >65k bytes)
• net/http slowloris -> ReadHeaderTimeout, IdleTimeout, MaxHeaderBytes
• progress flood -> 250 ms throttle in progressReader
• Dockerfile/go.mod -> golang 1.24, alpine 3.21, HEALTHCHECK

Addresses findings from code review against Context7-verified docs: - gorilla/websocket concurrent-write race -> per-conn write mutex - CSWSH via permissive CheckOrigin -> Origin host must match Host - OWASP PBKDF2-HMAC-SHA256 minimum -> 600,000 iterations - crypto.ts base64 -> chunked conversion (spread fails on >65k bytes) - net/http slowloris -> ReadHeaderTimeout, IdleTimeout, MaxHeaderBytes - progress flood -> 250 ms throttle in progressReader - Dockerfile/go.mod -> golang 1.24, alpine 3.21, HEALTHCHECK

skr added 1 commit 2026-05-08 12:42:52 +00:00

fix: address code review findings (security, correctness, hygiene) ... 127986d2e8

- WS write race: wrap each subscriber connection in subscriberConn{conn,mu}
  so notifySubscribers and the WS handler serialize WriteMessage calls.
  gorilla/websocket allows only one concurrent writer per conn.

- CSWSH: CheckOrigin now requires the request Origin host to match the
  server Host header (browser case); empty Origin (non-browser clients)
  is still allowed.

- HTTP slowloris: set ReadHeaderTimeout=15s, IdleTimeout=120s,
  MaxHeaderBytes=1 MiB on http.Server. ReadTimeout/WriteTimeout left
  off intentionally so they don't kill long uploads or live WebSockets.

- Progress flood: progressReader notifies callback at most every 250 ms
  (and on EOF/err) instead of every Read(), preventing the WS broadcast
  loop from being saturated by chunky reads.

- crypto.ts PBKDF2: 100k -> 600k iterations per OWASP Password Storage
  Cheat Sheet (PBKDF2-HMAC-SHA256, 2024+).

- crypto.ts base64: replace String.fromCharCode(...arr) and Uint8Array.from
  paths with chunked bytesToBase64/base64ToBytes helpers. Spread/apply
  fail on arrays past the engine argument-count limit (~16k-65k); large
  ciphertexts crashed silently. Regression test added (200 KiB payload).

- Dockerfile / go.mod / CI Go install: Go 1.21 -> 1.24, alpine 3.19 -> 3.21
  (both EOL); add HEALTHCHECK using wget against the embedded server.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

skr added 1 commit 2026-05-08 12:44:43 +00:00

fix: adapt to new throttled progressReader; add type annotations ... 40e6b97645

- TestProgressReader: assert that first Read fires the callback and that
  io.ReadAll triggers a final fire on EOF reaching len(data); drop the
  per-Read count assertion (now throttled to 250 ms intervals).
- crypto.ts: add explicit number type annotations on PBKDF2_ITERATIONS
  and BASE64_CHUNK; drop unnecessary 'as number[]' on Array.from(slice).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

skr merged commit b601156524 into main 2026-05-08 12:49:03 +00:00

skr deleted branch fix/code-review 2026-05-08 12:49:04 +00:00

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

bug

Something isn't working

  

documentation

Improvements or additions to documentation

  

duplicate

This issue or pull request already exists

  

enhancement

New feature or request

  

good first issue

Good for newcomers

  

help wanted

Extra attention is needed

  

invalid

This doesn't seem right

  

question

Further information is requested

  

wontfix

This will not be worked on

No labels

bug

documentation

duplicate

enhancement

good first issue

help wanted

invalid

question

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

nm/magic-wormhole-web!4

No description provided.