Skip to content

feat(security): warn when SMB_ADDRESS uses plaintext http to a remote host - #303

Open
birme wants to merge 1 commit into
mainfrom
security/276-smb-plaintext-warning
Open

feat(security): warn when SMB_ADDRESS uses plaintext http to a remote host#303
birme wants to merge 1 commit into
mainfrom
security/276-smb-plaintext-warning

Conversation

@birme

@birme birme commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • At startup, src/server.ts now parses SMB_ADDRESS and, if the scheme is http: and the hostname is not localhost/127.0.0.1/::1, emits a Log().warn(...) explaining that SDP/ICE data would be sent unencrypted and to use https:// in production.
  • Parsing is wrapped in a try/catch so a malformed SMB_ADDRESS logs a warning instead of crashing startup.
  • Warning-only behavior: never a hard failure.

Test plan

  • npm run typecheck clean
  • npm test all pass (243 tests; the "worker process failed to exit gracefully" warning is a pre-existing timer-leak notice, not a failure)
  • npm run lint clean for src/server.ts
  • Manual: start with SMB_ADDRESS=http://remote-host:8080 and confirm the warning appears; confirm no warning for http://localhost:8080 or https://...

Closes #276

… host

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@birme

birme commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

code-reviewer verdict: Needs Changes

Code Review

Summary: PR #303 (#276) adds a startup check parsing SMB_ADDRESS and warning when scheme is http: to a non-localhost host, wrapped in try/catch. Intent and warning-only philosophy are correct, TS clean, npm hygiene fine. Two issues hold it back: an IPv6-loopback correctness bug and the absence of the required unit test.

Blocking:

  • src/server.ts:17-19 — IPv6 loopback is not detected. new URL('http://[::1]:8080').hostname returns '[::1]' (with brackets) but localHosts contains '::1' (no brackets), so http://[::1]:... is wrongly classified as remote and emits a false plaintext warning. Add '[::1]' to the list (and/or normalize brackets).
  • src/server.ts (no test) — The issue/criteria require a unit test covering plaintext-remote vs https vs localhost. Extract the warning logic into a small pure exported helper and cover it with a test mocking ./log, asserting warn for http://remote:8080, no-warn for https://..., http://localhost, http://127.0.0.1, http://[::1].

Warnings:

  • The plaintext block runs even when SMB_ADDRESS is unset (re-parses the hardcoded default); consider guarding on the env var being set.

Suggestions:

  • Lines 26-34 already parse DB_CONNECTION_STRING; a shared parseServiceUrl/warn helper would reduce duplication and is where the testable extraction should live.

Moving back to Ready to fix the IPv6 bug + add the test.

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.

Security: Warn at startup when SMB_ADDRESS uses plaintext http:// to a non-localhost host

2 participants