Skip to content

fix: allow cross-origin WebSocket and fix Redis stream MINID format - #23

Merged
johnnyluo merged 1 commit into
mainfrom
fix/websocket-origin-and-stream
Apr 5, 2026
Merged

fix: allow cross-origin WebSocket and fix Redis stream MINID format#23
johnnyluo merged 1 commit into
mainfrom
fix/websocket-origin-and-stream

Conversation

@rcoderdev

@rcoderdev rcoderdev commented Apr 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

Two bugs preventing browser extension WebSocket connections to the notification server:

  1. WebSocket Origin rejectionwebsocket.Accept(w, r, nil) uses the coder/websocket library's default origin check, which requires the Origin header to match the Host header. Browser extensions send Origin: chrome-extension://<id>, causing 403 rejection. Fix: set OriginPatterns: ["*"] — authentication is already handled via the token in query params. Also re-applies the Cloudflare HTTP/1.0 workaround that was lost in a recent merge.

  2. Redis Streams MINID formatXAdd was called with MinID: "~" + minID, concatenating the approximate-trimming prefix into the ID string itself. go-redis v9 expects Approx: true as a separate field. The ~-prefixed string is an invalid stream ID, causing ERR Invalid stream ID specified as stream command argument.

Testing

  • Ran the notification server locally with both fixes
  • Browser extension successfully established WebSocket connection (status 101)
  • Sent POST /notify → notification delivered via Redis Streams → WebSocket → extension showed the keysign notification banner
  • Verified full E2E: registration → WebSocket → notify → banner display

Summary by CodeRabbit

  • Bug Fixes
    • WebSocket connections now properly support HTTP/1.0 protocol requests by upgrading to HTTP/1.1
    • Updated cross-origin request handling to accept all origin patterns during WebSocket upgrade
  • Performance
    • Optimized stream data trimming operations

Two issues preventing browser extension WebSocket connections:

1. websocket.Accept rejected connections from chrome-extension:// origins
   because the coder/websocket library's default origin check requires
   Origin to match Host. Browser extensions send their extension ID as
   Origin. Fix: set OriginPatterns: ["*"] since auth is via token in
   query params. Also re-apply the Cloudflare HTTP/1.0 workaround that
   was lost in a recent merge.

2. Redis XADD failed with "Invalid stream ID" because the approximate
   trimming prefix "~" was concatenated into the MinID string instead
   of using the go-redis Approx field. Fix: use Approx: true with a
   plain MinID value.
@coderabbitai

coderabbitai Bot commented Apr 5, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 448e23fd-69e0-4707-99fa-c97152e26f90

📥 Commits

Reviewing files that changed from the base of the PR and between 3fe92d5 and 0d0c7d2.

📒 Files selected for processing (2)
  • stream/stream.go
  • ws/handler.go

Walkthrough

The PR modifies Redis stream trimming logic in the stream publisher and updates WebSocket upgrade handling to detect HTTP/1.0 requests and force HTTP/1.1, while also changing origin validation options during the WebSocket accept step.

Changes

Cohort / File(s) Summary
Stream Publishing
stream/stream.go
Modified Redis stream entry trimming in (*Store).Publish: removed "~" prefix from MinID and set Approx: true on redis.XAddArgs.
WebSocket Handler
ws/handler.go
Updated (*Handler).ServeHTTP to detect HTTP/1.0 requests and force protocol to HTTP/1.1 before WebSocket accept. Changed websocket.Accept options from nil to &websocket.AcceptOptions{ OriginPatterns: []string{"*"} }.
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly summarizes the two main changes: cross-origin WebSocket fix and Redis stream MINID format fix, matching the primary objectives of the PR.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/websocket-origin-and-stream

Comment @coderabbitai help to get the list of available commands and usage tips.

@rcoderdev
rcoderdev marked this pull request as draft April 5, 2026 04:58
@rcoderdev
rcoderdev marked this pull request as ready for review April 5, 2026 09:23

@johnnyluo johnnyluo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@johnnyluo
johnnyluo merged commit 9e31c80 into main Apr 5, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants