SoulSync

Commit Graph

Author	SHA1	Message	Date
Broque Thomas	dd4cf130d7	Socket.IO CORS: handle self-review nits Six items from a Cin-style line-by-line pass on PR #383: - resolve_cors_origins: list of non-string entries (`[None, 123]`) now drops them instead of coercing to junk strings like `'None'`/`'123'`. - will_reject: backwards-compat shim removed. Production callers always pass `request.scheme` (Flask-guaranteed); the shim only existed for tests/non-Flask callers and made the production code path branchier than necessary. Tests now pass scheme explicitly. - maybe_log: redundant `if not origin` early-return dropped. will_reject handles missing origin (engineio's own behavior — server.py:207). - RejectionLogger.__init__: `int(dedup_cap)` wrapped in try/except so bad-type input falls back to DEFAULT_DEDUP_CAP instead of raising. - web_server.py: docstring on the before_request hook explains why the hook fires on every request (Flask doesn't scope before_request to a path prefix; the early-return string compare is the cheapest option). - settings.js: cors-origins URL regex tightened from `[^\s/]+` to `[^\s/?#]+` so query/fragment chars don't pass validation. Engineio would silently fail to match those anyway; better to flag at save. Test changes: - parametrize gained an explicit `scheme` column (12 cases updated). - New explicit case: scheme-mismatch rejects (engineio compares full `{scheme}://{host}` strings). - `test_will_reject_falls_back_to_host_only_when_no_scheme_info` deleted — the shim it tested is gone. - `test_will_reject_honors_x_forwarded_host` now passes scheme info. Net: -9 production lines, -3 test lines. Production code path is straight-line. 603 tests pass.	1 month ago
Broque Thomas	0f24739e27	Socket.IO CORS: polish — match engineio exactly, bound dedup, validate URLs Self-review pass on the security fix uncovered five issues, all fixed here: 1. will_reject scheme handling. Engineio compares full {scheme}://{host} strings, not just hostnames. A TLS-terminating proxy can leave the backend seeing http while the browser's Origin is https — engineio rejects, but the original predictor said "allow" → no helpful log line. Added request_scheme + forwarded_proto params, build full candidate strings to match engineio. 2. EITHER-forwarded-header rule. Engineio adds the forwarded candidate when EITHER X-Forwarded-Proto OR X-Forwarded-Host is present (it falls back to HTTP_HOST for the missing one). The original predictor only added it when forwarded_host was set — false negative for misconfigs sending only X-Forwarded-Proto. Now mirrors engineio. 3. will_reject incorrectly rejected missing-Origin requests. Engineio (server.py:207: `if origin: validate`) skips CORS validation when no Origin header is sent — non-browser clients (curl etc.) are intentionally permitted. The original code rejected them. Test was asserting the wrong behavior. Both fixed. 4. RejectionLogger had unbounded dedup set growth. A hostile actor opening connections from many distinct fake origins would fill memory unboundedly. Capped at 100 unique origins (configurable); when cap hit, one overflow notice is emitted and further rejections are silently dropped until restart. 5. Lock pattern: the overflow log path called logger.warning() while holding the dedup lock, inconsistent with the normal path. Fixed to pick the message under the lock and log after release. Critical section is now minimal and uniform. Plus polish: - Stale module docstring fixed (said "empty list" instead of "None"). - settings.js validates each cors_origins line against a URL regex on save; toasts a one-shot warning if entries are malformed (resolver silently filters them, but user gets feedback now). - web_server.py wiring passes request.scheme + X-Forwarded-Proto so the predictor has full proxy info. Tests: - 51 unit tests in tests/test_socketio_cors.py (was 45). New cases: * scheme comparison (5 cases including TLS-terminating proxies) * forwarded_proto-alone misconfig * missing-origin matches engineio (was asserting wrong behavior) * dedup cap with overflow + reset * default cap is reasonable (uses public DEFAULT_DEDUP_CAP constant) Engineio behavior independently verified by reading engineio/server.py and engineio/base_server.py source. Predictor mirrors both files. 604 tests pass.	1 month ago
Broque Thomas	013eebf350	Lock down Socket.IO CORS — same-origin default + opt-in allow-list Closes #366 (reported by JohnBaumb). Socket.IO was initialized with `cors_allowed_origins=''`, accepting WebSocket connections from any origin. A malicious site could open a WS to a user's local SoulSync instance and exfiltrate live progress / toast / activity events. This commit: - Defaults to engineio's same-origin behavior (`cors_allowed_origins=None`), which automatically honors X-Forwarded-Host so reverse proxies that send that header (Caddy / Traefik by default, properly-configured Nginx) work transparently. - Adds a `security.cors_origins` config setting + Settings → Security textarea where users behind unusual proxies / Electron wrappers / cross-origin integrations can whitelist their origin. Accepts comma or newline separated values; `` on its own line opts back into the legacy wildcard with a startup-warning log. - Logs a clear warning the first time engineio rejects each unique origin, naming the rejected Origin and request Host and pointing users to the settings field. Without this, engineio silently 403s the upgrade and the user just sees a half-broken UI with no clue why. Threadsafe dedup so a hostile origin can't spam logs. Logic lives in `core/socketio_cors.py` (resolver, rejection predictor, dedup logger class, startup-status emitter) — pure functions, no Flask dependency. `web_server.py` adds 23 lines of wiring and imports. Important catch during review: my first pass used `cors_allowed_origins=[]` as the "secure default." Reading engineio's source revealed `[]` actually means "DISABLE CORS HANDLING" (engineio/server.py:202: `if cors_allowed_origins != []:`) — identical security to `''`. Fixed to use `None` (engineio's actual same-origin sentinel) and pinned with a regression test that asserts the resolver never returns `[]` for any input shape. Tests: - tests/test_socketio_cors.py — 45 unit tests covering 19 resolver shape cases (None, empty, whitespace, comma, newline, garbage types, lists), the `[]`-must-never-be-returned security regression, 12 rejection prediction cases, X-Forwarded-Host handling, dedup logger behavior, threadsafe race (8 threads × 50 hammers → exactly 1 warning), and startup-status emitter outputs. Frontend: - Settings → Security gains an "Allowed WebSocket Origins" textarea with help text explaining same-origin default + when to add a domain + the `` opt-out. - helper.js — new '2.4.1' WHATS_NEW block (hidden until version bump) with a chill-voice entry describing the change. Conftest.py left at `'*'` — test environment, no security concern. 598 tests pass.	1 month ago

Author

SHA1

Message

Date

Broque Thomas

dd4cf130d7

Socket.IO CORS: handle self-review nits

Six items from a Cin-style line-by-line pass on PR #383:

- resolve_cors_origins: list of non-string entries (`[None, 123]`) now
  drops them instead of coercing to junk strings like `'None'`/`'123'`.
- will_reject: backwards-compat shim removed. Production callers always
  pass `request.scheme` (Flask-guaranteed); the shim only existed for
  tests/non-Flask callers and made the production code path branchier
  than necessary. Tests now pass scheme explicitly.
- maybe_log: redundant `if not origin` early-return dropped. will_reject
  handles missing origin (engineio's own behavior — server.py:207).
- RejectionLogger.__init__: `int(dedup_cap)` wrapped in try/except so
  bad-type input falls back to DEFAULT_DEDUP_CAP instead of raising.
- web_server.py: docstring on the before_request hook explains why the
  hook fires on every request (Flask doesn't scope before_request to a
  path prefix; the early-return string compare is the cheapest option).
- settings.js: cors-origins URL regex tightened from `[^\s/]+` to
  `[^\s/?#]+` so query/fragment chars don't pass validation. Engineio
  would silently fail to match those anyway; better to flag at save.

Test changes:
- parametrize gained an explicit `scheme` column (12 cases updated).
- New explicit case: scheme-mismatch rejects (engineio compares full
  `{scheme}://{host}` strings).
- `test_will_reject_falls_back_to_host_only_when_no_scheme_info`
  deleted — the shim it tested is gone.
- `test_will_reject_honors_x_forwarded_host` now passes scheme info.

Net: -9 production lines, -3 test lines. Production code path is
straight-line. 603 tests pass.

Broque Thomas

0f24739e27

Socket.IO CORS: polish — match engineio exactly, bound dedup, validate URLs

Self-review pass on the security fix uncovered five issues, all fixed
here:

1. will_reject scheme handling. Engineio compares full {scheme}://{host}
   strings, not just hostnames. A TLS-terminating proxy can leave the
   backend seeing http while the browser's Origin is https — engineio
   rejects, but the original predictor said "allow" → no helpful log
   line. Added request_scheme + forwarded_proto params, build full
   candidate strings to match engineio.

2. EITHER-forwarded-header rule. Engineio adds the forwarded candidate
   when EITHER X-Forwarded-Proto OR X-Forwarded-Host is present (it
   falls back to HTTP_HOST for the missing one). The original predictor
   only added it when forwarded_host was set — false negative for
   misconfigs sending only X-Forwarded-Proto. Now mirrors engineio.

3. will_reject incorrectly rejected missing-Origin requests. Engineio
   (server.py:207: `if origin: validate`) skips CORS validation when
   no Origin header is sent — non-browser clients (curl etc.) are
   intentionally permitted. The original code rejected them. Test was
   asserting the wrong behavior. Both fixed.

4. RejectionLogger had unbounded dedup set growth. A hostile actor
   opening connections from many distinct fake origins would fill
   memory unboundedly. Capped at 100 unique origins (configurable);
   when cap hit, one overflow notice is emitted and further rejections
   are silently dropped until restart.

5. Lock pattern: the overflow log path called logger.warning() while
   holding the dedup lock, inconsistent with the normal path. Fixed
   to pick the message under the lock and log after release. Critical
   section is now minimal and uniform.

Plus polish:
- Stale module docstring fixed (said "empty list" instead of "None").
- settings.js validates each cors_origins line against a URL regex on
  save; toasts a one-shot warning if entries are malformed (resolver
  silently filters them, but user gets feedback now).
- web_server.py wiring passes request.scheme + X-Forwarded-Proto so
  the predictor has full proxy info.

Tests:
- 51 unit tests in tests/test_socketio_cors.py (was 45). New cases:
  * scheme comparison (5 cases including TLS-terminating proxies)
  * forwarded_proto-alone misconfig
  * missing-origin matches engineio (was asserting wrong behavior)
  * dedup cap with overflow + reset
  * default cap is reasonable (uses public DEFAULT_DEDUP_CAP constant)

Engineio behavior independently verified by reading engineio/server.py
and engineio/base_server.py source. Predictor mirrors both files.

604 tests pass.

Broque Thomas

013eebf350

Lock down Socket.IO CORS — same-origin default + opt-in allow-list

Closes #366 (reported by JohnBaumb).

Socket.IO was initialized with `cors_allowed_origins='*'`, accepting
WebSocket connections from any origin. A malicious site could open a
WS to a user's local SoulSync instance and exfiltrate live progress /
toast / activity events.

This commit:

- Defaults to engineio's same-origin behavior (`cors_allowed_origins=None`),
  which automatically honors X-Forwarded-Host so reverse proxies that
  send that header (Caddy / Traefik by default, properly-configured
  Nginx) work transparently.
- Adds a `security.cors_origins` config setting + Settings → Security
  textarea where users behind unusual proxies / Electron wrappers /
  cross-origin integrations can whitelist their origin. Accepts comma
  or newline separated values; `*` on its own line opts back into the
  legacy wildcard with a startup-warning log.
- Logs a clear warning the first time engineio rejects each unique
  origin, naming the rejected Origin and request Host and pointing
  users to the settings field. Without this, engineio silently 403s
  the upgrade and the user just sees a half-broken UI with no clue
  why. Threadsafe dedup so a hostile origin can't spam logs.

Logic lives in `core/socketio_cors.py` (resolver, rejection
predictor, dedup logger class, startup-status emitter) — pure
functions, no Flask dependency. `web_server.py` adds 23 lines of
wiring and imports.

Important catch during review: my first pass used `cors_allowed_origins=[]`
as the "secure default." Reading engineio's source revealed `[]` actually
means "DISABLE CORS HANDLING" (engineio/server.py:202: `if cors_allowed_origins != []:`)
— identical security to `'*'`. Fixed to use `None` (engineio's actual
same-origin sentinel) and pinned with a regression test that asserts
the resolver never returns `[]` for any input shape.

Tests:
- tests/test_socketio_cors.py — 45 unit tests covering 19 resolver shape
  cases (None, empty, whitespace, comma, newline, garbage types, lists),
  the `[]`-must-never-be-returned security regression, 12 rejection
  prediction cases, X-Forwarded-Host handling, dedup logger behavior,
  threadsafe race (8 threads × 50 hammers → exactly 1 warning), and
  startup-status emitter outputs.

Frontend:
- Settings → Security gains an "Allowed WebSocket Origins" textarea
  with help text explaining same-origin default + when to add a domain
  + the `*` opt-out.
- helper.js — new '2.4.1' WHATS_NEW block (hidden until version bump)
  with a chill-voice entry describing the change.

Conftest.py left at `'*'` — test environment, no security concern.

598 tests pass.

3 Commits (e0e31079e6606e4e4dbdbe9b2da75e0ab14dbf21)