fix/wss for all#148
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces local development TLS certificate generation infrastructure and updates the application to prioritize secure connections (wss and ircs) over plain WebSocket. Changes include Docker-based certificate generation, updated IRC server TLS configuration, npm scripts for certificate setup, and client-side protocol handling modifications to support and prefer secure schemes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
|
Automated deployment preview for the PR in the Cloudflare Pages. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/ui/AddServerModal.tsx (1)
92-103:⚠️ Potential issue | 🟠 Major
isLocalhostguard onisSSLPortcontradicts the PR's stated local-dev TLS goal.The
!isLocalhost &&condition forcesirc://localhost:6697instead ofircs://localhost:6697, butCONTRIBUTING.md(line 71 in this PR) explicitly instructs users to connect viaircs://localhost:6697with Tauri native TCP. The whole point of adding mkcert cert generation is to enable TLS on localhost — this guard defeats that.🐛 Proposed fix
- // Use ircs:// for SSL ports (typically 6697, 9999, etc.) or common SSL ports, but not for localhost + // Use ircs:// for well-known SSL ports (includes localhost now that mkcert TLS is supported) const isSSLPort = - !isLocalhost && (port === 6697 || port === 9999 || port === 443 || port === 993);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/AddServerModal.tsx` around lines 92 - 103, The isLocalhost check prevents localhost from using TLS by short-circuiting isSSLPort; update the logic in AddServerModal where isLocalhost and isSSLPort are computed so localhost can be treated as SSL when appropriate (remove the `!isLocalhost &&` guard from the isSSLPort calculation or otherwise allow port-based SSL for `cleanHost === "localhost" || cleanHost === "127.0.0.1" || cleanHost === "::1"`), then ensure finalHost still chooses the "ircs" scheme when isSSLPort is true so `finalHost` produces `ircs://localhost:6697` when connecting locally with TLS.src/store/index.ts (1)
49-55:⚠️ Potential issue | 🟠 MajorHandle
ws://innormalizeHostto avoid host corruption.With the current regex,
ws://example.com:6697normalizes to"ws", breaking host comparisons and potentially storing"ws"as the host. This can cause duplicate servers or failed lookups for legacy entries.✅ Suggested fix
- const withoutProtocol = host.replace(/^(irc|ircs|wss):\/\//, ""); + const withoutProtocol = host.replace(/^(irc|ircs|ws|wss):\/\//, "");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/index.ts` around lines 49 - 55, normalizeHost is currently stripping protocol with a regex that omits "ws" (so "ws://example.com:6697" becomes "ws"), causing corrupted host values; update the normalizeHost implementation to include "ws" (and "http"/"https" if desired) in the protocol-matching pattern or replace the regex with a robust URL parse (e.g., new URL(host).hostname fallback) so the function returns the actual hostname (without port) for inputs like "ws://...", "wss://...", "irc://", etc.; keep the function name normalizeHost and ensure it still returns host.split(":")[0] or equivalent hostname-only output for non-URL inputs.
🧹 Nitpick comments (2)
src/store/index.ts (1)
60-66: Upgrade legacyws://entries (or confirm migration handles it).
ensureUrlFormatleaves existing schemes untouched, so any savedws://hosts remain insecure and bypass the “wss everywhere” goal. If migrations already upgrade these, please confirm; otherwise consider upgrading here.🔧 Optional upgrade path
function ensureUrlFormat(host: string, port: number): string { - if (host.includes("://")) { - return host; // Already in URL format - } + if (host.startsWith("ws://")) { + return host.replace(/^ws:\/\//, "wss://"); + } + if (host.includes("://")) { + return host; // Already in URL format + } // Convert old hostname-only format to URL — always wss:// return `wss://${host}:${port}`; }To verify migrations, you can run:
#!/bin/bash # Look for migration logic that rewrites ws:// to wss:// rg -n "ws://|wss://" -g '*migrat*' rg -n "ensureUrlFormat|normalizeHost" -g '*migrat*'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/index.ts` around lines 60 - 66, ensureUrlFormat currently returns any host that already contains a scheme unchanged, which leaves legacy "ws://" entries insecure; update ensureUrlFormat to detect a "ws://" scheme and rewrite it to "wss://" (e.g., replace leading "ws://" with "wss://") while leaving other schemes untouched, and keep the existing hostname+port handling when no scheme is present; make this change in the ensureUrlFormat function and, if applicable, add a unit test or note to verify migration logic that would otherwise handle ws->wss upgrades.compose.yaml (1)
43-53: Consider keeping plaintext IRC off the host interface.If port 6667 is only for internal bots, binding to localhost (or removing the host mapping) avoids advertising a non‑TLS endpoint on the host network.
🔧 Example: bind plaintext IRC to localhost only
- - '6667:6667' # plain IRC (bots) + - '127.0.0.1:6667:6667' # plain IRC (bots, local-only)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compose.yaml` around lines 43 - 53, The docker-compose service exposes plaintext IRC on the host via the port mapping '6667:6667'; change this to bind only to localhost or remove the host mapping to avoid advertising a non‑TLS endpoint. Locate the service block that contains the ports list (the '6667:6667' entry) in compose.yaml and either replace '6667:6667' with '127.0.0.1:6667:6667' to bind to localhost only or delete that mapping entirely if the service only needs internal network access; keep the other TLS/WSS mappings (6697 and 8097) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@compose.yaml`:
- Around line 16-33: The current guard in the gen-certs service uses `openssl
x509` before OpenSSL is installed and only checks server.pem, causing
unnecessary regeneration; change the command start to first check for the
presence of both /certs/server.pem and /certs/server-key.pem (e.g., test that
both files exist) and exit 0 if they do, and only then proceed to apk add
openssl and the CSR/certificate generation; update the gen-certs command block
to perform the existence check for both files before attempting OpenSSL
operations.
In `@scripts/setup-dev-certs.sh`:
- Line 12: Update the completion echo in the setup-dev-certs.sh script so the
user instruction matches CONTRIBUTING.md: replace the current echo message that
reads "Done. Run: docker compose --profile testing up -d" with one that says
"Done. Run: npm run run-dev-stack" (or otherwise mirrors the exact phrasing in
CONTRIBUTING.md) to avoid confusing developers; locate and edit the echo
statement in scripts/setup-dev-certs.sh.
- Line 11: The current echo command in scripts/setup-dev-certs.sh overwrites the
entire .env; change it to an "upsert" that updates or adds only the
MKCERT_CAROOT variable instead of truncating the file. Replace the single-line
echo that writes MKCERT_CAROOT with logic that detects and replaces an existing
MKCERT_CAROOT entry (e.g., using sed -i to substitute the line for
MKCERT_CAROOT) and, if not present, appends the MKCERT_CAROOT=... line; ensure
the code references the same mkcert -CAROOT invocation and writes to "$(dirname
"$0")/../.env" so existing environment variables are preserved.
In `@src/lib/ircClient.ts`:
- Around line 490-503: The host parsing in the host.startsWith("wss://") and
host.startsWith("ws://") branches fails for IPv6 and leaves actualHost unchanged
when the regex doesn't match; replace the fragile regex logic with robust URL
parsing using the URL API (e.g. construct new URL(host) inside a try/catch),
then set actualHost = url.hostname and actualPort = url.port ? Number(url.port)
: port; for the "ws://" branch ensure you still upgrade to wss by using the
parsed URL and adjusting the protocol if needed; in the catch/fallback,
explicitly handle invalid/empty hosts (throw or return a clear error) instead of
leaving actualHost as the original string so you don't produce double-scheme
URLs—modify the code around the host.startsWith("wss://") and
host.startsWith("ws://") branches and the variables
urlMatch/actualHost/actualPort accordingly.
---
Outside diff comments:
In `@src/components/ui/AddServerModal.tsx`:
- Around line 92-103: The isLocalhost check prevents localhost from using TLS by
short-circuiting isSSLPort; update the logic in AddServerModal where isLocalhost
and isSSLPort are computed so localhost can be treated as SSL when appropriate
(remove the `!isLocalhost &&` guard from the isSSLPort calculation or otherwise
allow port-based SSL for `cleanHost === "localhost" || cleanHost === "127.0.0.1"
|| cleanHost === "::1"`), then ensure finalHost still chooses the "ircs" scheme
when isSSLPort is true so `finalHost` produces `ircs://localhost:6697` when
connecting locally with TLS.
In `@src/store/index.ts`:
- Around line 49-55: normalizeHost is currently stripping protocol with a regex
that omits "ws" (so "ws://example.com:6697" becomes "ws"), causing corrupted
host values; update the normalizeHost implementation to include "ws" (and
"http"/"https" if desired) in the protocol-matching pattern or replace the regex
with a robust URL parse (e.g., new URL(host).hostname fallback) so the function
returns the actual hostname (without port) for inputs like "ws://...",
"wss://...", "irc://", etc.; keep the function name normalizeHost and ensure it
still returns host.split(":")[0] or equivalent hostname-only output for non-URL
inputs.
---
Nitpick comments:
In `@compose.yaml`:
- Around line 43-53: The docker-compose service exposes plaintext IRC on the
host via the port mapping '6667:6667'; change this to bind only to localhost or
remove the host mapping to avoid advertising a non‑TLS endpoint. Locate the
service block that contains the ports list (the '6667:6667' entry) in
compose.yaml and either replace '6667:6667' with '127.0.0.1:6667:6667' to bind
to localhost only or delete that mapping entirely if the service only needs
internal network access; keep the other TLS/WSS mappings (6697 and 8097)
unchanged.
In `@src/store/index.ts`:
- Around line 60-66: ensureUrlFormat currently returns any host that already
contains a scheme unchanged, which leaves legacy "ws://" entries insecure;
update ensureUrlFormat to detect a "ws://" scheme and rewrite it to "wss://"
(e.g., replace leading "ws://" with "wss://") while leaving other schemes
untouched, and keep the existing hostname+port handling when no scheme is
present; make this change in the ensureUrlFormat function and, if applicable,
add a unit test or note to verify migration logic that would otherwise handle
ws->wss upgrades.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.gitignoreCONTRIBUTING.mdcompose.yamldocker/ergo.yamlpackage.jsonscripts/setup-dev-certs.shsrc/components/ui/AddServerModal.tsxsrc/components/ui/LinkSecurityWarningModal.tsxsrc/lib/ircClient.tssrc/lib/socket.tssrc/store/index.tstests/components/LinkSecurityWarningModal.test.tsx
💤 Files with no reviewable changes (1)
- tests/components/LinkSecurityWarningModal.test.tsx
Comment on lines
+16
to
+33
| gen-certs: | ||
| image: alpine:3.20 | ||
| volumes: | ||
| - ${MKCERT_CAROOT:?Run ./scripts/setup-dev-certs.sh first}:/caroot:ro | ||
| - ./certs:/certs | ||
| command: | ||
| - sh | ||
| - -c | ||
| - | | ||
| openssl x509 -noout -in /certs/server.pem 2>/dev/null && exit 0 | ||
| apk add --no-cache openssl -q | ||
| openssl genrsa -out /certs/server-key.pem 2048 | ||
| openssl req -new -key /certs/server-key.pem -out /tmp/csr.pem -subj "/CN=localhost" | ||
| printf '[SAN]\nsubjectAltName=DNS:localhost,IP:127.0.0.1,IP:::1\n' > /tmp/ext.cnf | ||
| openssl x509 -req -in /tmp/csr.pem \ | ||
| -CA /caroot/rootCA.pem -CAkey /caroot/rootCA-key.pem -set_serial 1 \ | ||
| -out /certs/server.pem -days 825 -sha256 -extfile /tmp/ext.cnf -extensions SAN | ||
| profiles: |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Alpine Linux 3.20 openssl binary included by default
💡 Result:
No—Alpine Linux 3.20 does not include the openssl binary by default in the minimal root filesystem used for containers (and thus the official alpine:3.20 Docker image). The default mini-rootfs package set is busybox alpine-baselayout alpine-keys alpine-release apk-tools musl-utils (no openssl) [1].
If you need the openssl command, install it explicitly:
apk add opensslThe openssl package is available in Alpine 3.20’s main repo (package name openssl) [2].
Sources: [1], [2]
Early-exit guard can miss existing keys and always regenerates on fresh images.
The guard tries to use openssl x509 before OpenSSL is installed, so it always fails silently on fresh Alpine containers and regenerates unnecessarily. Additionally, it only checks server.pem, so a missing server-key.pem on container reuse could go undetected. Check both files exist before regenerating instead.
🔧 Suggested guard that checks both files before regenerating
- openssl x509 -noout -in /certs/server.pem 2>/dev/null && exit 0
+ if [ -s /certs/server.pem ] && [ -s /certs/server-key.pem ]; then
+ exit 0
+ fi
apk add --no-cache openssl -q🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@compose.yaml` around lines 16 - 33, The current guard in the gen-certs
service uses `openssl x509` before OpenSSL is installed and only checks
server.pem, causing unnecessary regeneration; change the command start to first
check for the presence of both /certs/server.pem and /certs/server-key.pem
(e.g., test that both files exist) and exit 0 if they do, and only then proceed
to apk add openssl and the CSR/certificate generation; update the gen-certs
command block to perform the existence check for both files before attempting
OpenSSL operations.
| command -v mkcert >/dev/null || { echo "mkcert not found"; exit 1; } | ||
|
|
||
| mkcert -install | ||
| echo "MKCERT_CAROOT=$(mkcert -CAROOT)" > "$(dirname "$0")/../.env" |
Contributor
There was a problem hiding this comment.
.env overwrite destroys existing developer environment variables.
The > redirect replaces the entire .env file. Any pre-existing variables (API keys, feature flags, etc.) in .env are silently deleted. Use a targeted sed-in-place update or at minimum >> + a deduplication guard.
🛡️ Proposed fix — upsert only the MKCERT_CAROOT line
-echo "MKCERT_CAROOT=$(mkcert -CAROOT)" > "$(dirname "$0")/../.env"
+ENV_FILE="$(dirname "$0")/../.env"
+CAROOT_LINE="MKCERT_CAROOT=$(mkcert -CAROOT)"
+if [ -f "$ENV_FILE" ] && grep -q '^MKCERT_CAROOT=' "$ENV_FILE"; then
+ sed -i "s|^MKCERT_CAROOT=.*|${CAROOT_LINE}|" "$ENV_FILE"
+else
+ echo "$CAROOT_LINE" >> "$ENV_FILE"
+fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| echo "MKCERT_CAROOT=$(mkcert -CAROOT)" > "$(dirname "$0")/../.env" | |
| ENV_FILE="$(dirname "$0")/../.env" | |
| CAROOT_LINE="MKCERT_CAROOT=$(mkcert -CAROOT)" | |
| if [ -f "$ENV_FILE" ] && grep -q '^MKCERT_CAROOT=' "$ENV_FILE"; then | |
| sed -i "s|^MKCERT_CAROOT=.*|${CAROOT_LINE}|" "$ENV_FILE" | |
| else | |
| echo "$CAROOT_LINE" >> "$ENV_FILE" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/setup-dev-certs.sh` at line 11, The current echo command in
scripts/setup-dev-certs.sh overwrites the entire .env; change it to an "upsert"
that updates or adds only the MKCERT_CAROOT variable instead of truncating the
file. Replace the single-line echo that writes MKCERT_CAROOT with logic that
detects and replaces an existing MKCERT_CAROOT entry (e.g., using sed -i to
substitute the line for MKCERT_CAROOT) and, if not present, appends the
MKCERT_CAROOT=... line; ensure the code references the same mkcert -CAROOT
invocation and writes to "$(dirname "$0")/../.env" so existing environment
variables are preserved.
|
|
||
| mkcert -install | ||
| echo "MKCERT_CAROOT=$(mkcert -CAROOT)" > "$(dirname "$0")/../.env" | ||
| echo "Done. Run: docker compose --profile testing up -d" |
Contributor
There was a problem hiding this comment.
Completion message doesn't match CONTRIBUTING.md.
The script prints docker compose --profile testing up -d, but CONTRIBUTING.md tells developers to run npm run run-dev-stack. Align to avoid confusion.
📝 Proposed fix
-echo "Done. Run: docker compose --profile testing up -d"
+echo "Done. Run: npm run run-dev-stack"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| echo "Done. Run: docker compose --profile testing up -d" | |
| echo "Done. Run: npm run run-dev-stack" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/setup-dev-certs.sh` at line 12, Update the completion echo in the
setup-dev-certs.sh script so the user instruction matches CONTRIBUTING.md:
replace the current echo message that reads "Done. Run: docker compose --profile
testing up -d" with one that says "Done. Run: npm run run-dev-stack" (or
otherwise mirrors the exact phrasing in CONTRIBUTING.md) to avoid confusing
developers; locate and edit the echo statement in scripts/setup-dev-certs.sh.
Comment on lines
+490
to
503
| } else if (host.startsWith("wss://")) { | ||
| // Parse wss:// URLs | ||
| const urlMatch = host.match(/^wss:\/\/([^:]+)(?::(\d+))?/); | ||
| if (urlMatch) { | ||
| protocol = urlMatch[1] as "ws" | "wss"; | ||
| actualHost = urlMatch[2]; | ||
| actualPort = urlMatch[3] ? Number.parseInt(urlMatch[3], 10) : port; | ||
| actualHost = urlMatch[1]; | ||
| actualPort = urlMatch[2] ? Number.parseInt(urlMatch[2], 10) : port; | ||
| } | ||
| } else if (host.startsWith("ws://")) { | ||
| // Upgrade legacy ws:// to wss:// — unencrypted WebSockets are no longer supported | ||
| const urlMatch = host.match(/^ws:\/\/([^:]+)(?::(\d+))?/); | ||
| if (urlMatch) { | ||
| actualHost = urlMatch[1]; | ||
| actualPort = urlMatch[2] ? Number.parseInt(urlMatch[2], 10) : port; | ||
| } |
Contributor
There was a problem hiding this comment.
[^:]+ host regex breaks for IPv6 and produces a misleading error when urlMatch is null.
Two related issues in the new wss:// and ws:// branches:
-
IPv6:
[^:]+captures only up to the first colon, sowss://[::1]:8097yieldsactualHost = "[". The PR's own local-dev instructions mention::1as a valid localhost alias. -
Null-match fallback: when
urlMatchis null (e.g.host = "wss://"),actualHostis left as the full original string, producing a double-scheme URL (wss://wss://:port) that generates a confusing error message downstream.
🐛 Proposed fix — robust URL parsing for both branches
-} else if (host.startsWith("wss://")) {
- // Parse wss:// URLs
- const urlMatch = host.match(/^wss:\/\/([^:]+)(?::(\d+))?/);
- if (urlMatch) {
- actualHost = urlMatch[1];
- actualPort = urlMatch[2] ? Number.parseInt(urlMatch[2], 10) : port;
- }
-} else if (host.startsWith("ws://")) {
- // Upgrade legacy ws:// to wss:// — unencrypted WebSockets are no longer supported
- const urlMatch = host.match(/^ws:\/\/([^:]+)(?::(\d+))?/);
- if (urlMatch) {
- actualHost = urlMatch[1];
- actualPort = urlMatch[2] ? Number.parseInt(urlMatch[2], 10) : port;
- }
-}
+} else if (host.startsWith("wss://") || host.startsWith("ws://")) {
+ if (host.startsWith("ws://")) {
+ // Upgrade legacy ws:// to wss:// — unencrypted WebSockets are no longer supported
+ console.warn(`[ircClient] Upgrading ws:// to wss:// for: ${host}`);
+ }
+ try {
+ // Use the URL API for robust parsing (handles IPv6, paths, etc.)
+ const parsed = new URL(host.startsWith("ws://") ? host.replace(/^ws:/, "wss:") : host);
+ actualHost = parsed.hostname; // strips brackets from IPv6
+ actualPort = parsed.port ? Number.parseInt(parsed.port, 10) : port;
+ } catch {
+ throw new Error(`Invalid WebSocket URL: ${host}`);
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } else if (host.startsWith("wss://")) { | |
| // Parse wss:// URLs | |
| const urlMatch = host.match(/^wss:\/\/([^:]+)(?::(\d+))?/); | |
| if (urlMatch) { | |
| protocol = urlMatch[1] as "ws" | "wss"; | |
| actualHost = urlMatch[2]; | |
| actualPort = urlMatch[3] ? Number.parseInt(urlMatch[3], 10) : port; | |
| actualHost = urlMatch[1]; | |
| actualPort = urlMatch[2] ? Number.parseInt(urlMatch[2], 10) : port; | |
| } | |
| } else if (host.startsWith("ws://")) { | |
| // Upgrade legacy ws:// to wss:// — unencrypted WebSockets are no longer supported | |
| const urlMatch = host.match(/^ws:\/\/([^:]+)(?::(\d+))?/); | |
| if (urlMatch) { | |
| actualHost = urlMatch[1]; | |
| actualPort = urlMatch[2] ? Number.parseInt(urlMatch[2], 10) : port; | |
| } | |
| } else if (host.startsWith("wss://") || host.startsWith("ws://")) { | |
| if (host.startsWith("ws://")) { | |
| // Upgrade legacy ws:// to wss:// — unencrypted WebSockets are no longer supported | |
| console.warn(`[ircClient] Upgrading ws:// to wss:// for: ${host}`); | |
| } | |
| try { | |
| // Use the URL API for robust parsing (handles IPv6, paths, etc.) | |
| const parsed = new URL(host.startsWith("ws://") ? host.replace(/^ws:/, "wss:") : host); | |
| actualHost = parsed.hostname; // strips brackets from IPv6 | |
| actualPort = parsed.port ? Number.parseInt(parsed.port, 10) : port; | |
| } catch { | |
| throw new Error(`Invalid WebSocket URL: ${host}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/ircClient.ts` around lines 490 - 503, The host parsing in the
host.startsWith("wss://") and host.startsWith("ws://") branches fails for IPv6
and leaves actualHost unchanged when the regex doesn't match; replace the
fragile regex logic with robust URL parsing using the URL API (e.g. construct
new URL(host) inside a try/catch), then set actualHost = url.hostname and
actualPort = url.port ? Number(url.port) : port; for the "ws://" branch ensure
you still upgrade to wss by using the parsed URL and adjusting the protocol if
needed; in the catch/fallback, explicitly handle invalid/empty hosts (throw or
return a clear error) instead of leaving actualHost as the original string so
you don't produce double-scheme URLs—modify the code around the
host.startsWith("wss://") and host.startsWith("ws://") branches and the
variables urlMatch/actualHost/actualPort accordingly.
ValwareIRC
approved these changes
Mar 8, 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
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.
I have accidentally reverted #143 but then I realized more than that and could be also more