Withdraw overstated security absolutes — including 'the Hub relay is end-to-end encrypted', which it is not - #99
Merged
Conversation
…sweep Follow-on from S217(b), which found the same overstated claim in three places rather than two. This sweep looked for the general class: a sentence asserting something *cannot* happen, is *always* true, or is *verified/immutable/guaranteed/enforced*, where the code has an exemption, a config flag, a default-off feature, or a fallback. Method: 856 candidate lines matched the literal set across every tracked file type; 496 were in the published surface (`docs/**`, including the two OpenAPI `.yaml` specs). 348 candidate sentences were then read in context and checked against phlix-server / phlix-hub / phlix-ui / phlix-contracts / phlix-shared. The great majority verified TRUE and are untouched — see the worklog for the full examined list with evidence, including the ones left alone deliberately. Changed here (each verified at the named file:line, not inferred): - privacy-security.md — the most serious. Three separate places claimed the Hub relay is "end-to-end encrypted" and that the Hub "cannot decrypt the WebSocket payload" / "does NOT see" media content, filenames, library metadata or watch history. `phlix-hub/src/Relay/` contains no encryption or decryption at all, and `RelayProxyManager.php:239-250` base64-decodes each proxied body to forward it. Everything relayed crosses the Hub in cleartext. Reframed: TLS on the wire, terminated at the Hub; the "does not see" list now states the condition under which it holds (clients reaching the server directly). Also: Argon2ID cost figures were wrong (the code passes no options array, so PHP defaults apply, not 12 MiB/3/4); the DLNA disable snippet named the wrong file and key (`config/dlna.php`, `cds_enabled`); and two "never leaves the server" / "no viewing habits" absolutes ignored the opt-in Trakt and Last.fm scrobblers. - advanced/parental-controls.md — access schedules were documented as allow windows with "if no schedule matches, access is denied by default". The implementation is the exact inverse: schedules are blackout windows and `AccessScheduleService::isAccessAllowed()` returns true when nothing matches, so a profile with no schedules is unrestricted. Documenting a fail-open gate as fail-closed is the dangerous direction, so this is corrected with the inversion called out explicitly. - plugins/install-from-url.md — "must be HTTPS (`http://` refused unless `PHLIX_PLUGINS_ALLOW_HTTP=1`)" is false: `isAllowedInstallUrl()` (PluginAdminController.php:844-852) permits only https and file, and never consults that variable. The page also never mentioned that an un-pinned remote source is refused outright unless `PHLIX_PLUGINS_ALLOW_UNVERIFIED=1` — a default-deny that governs the page's entire premise, and which the controller's own comment at :320-322 states. A cited error code `plugin.signature.mismatch` exists nowhere; every install failure emits `plugin.install.failed`. - plugins/plugin-catalog.md — "Plugins must be signed before they can be installed from the catalog" is false: `PluginsProvider.php:90` resolves `PHLIX_PLUGINS_REQUIRE_SIGNATURE` with a default of `false`, and the catalog's real gate is the `ref` + `artifactSha256` pin, not a signature. - plugins/*.md — "signature-verified" (3 sites) renamed to "digest-pinned", which is the control the catalog actually applies. - security/sso-oidc-ldap.md — "The consume is atomic (`SELECT … FOR UPDATE` + `DELETE`), so a code cannot be redeemed twice" holds for GitHub only. `DbOidcStateStore.php` takes no row lock; the sibling OAuth2 store documents that gap by name. - reference/api/admin-plugins.yaml — "gates every install on both channels" restated an absolute the prose page had already qualified. This is exactly the prose/spec drift S217(b) hit, so the spec is moved in lockstep. Deliberately NOT changed, to avoid over-generalising: - The ~30 files documenting the HTTP port as `32400`. The real default is `8096` (`config/server.php:7`) and `PHLIX_HTTP_PORT` has zero hits in the server source, so `reference/env-vars.md:193` documents a variable that does not exist. This is an estate-wide factual-default defect, not an absolute-claim overclaim; correcting one page would leave it contradicting the other thirty. Filed for its own step. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Up to standards ✅🟢 Issues
|
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
1 participant
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.
A sweep for the class of overclaim S217(b) found: doc sentences asserting something cannot happen or is always true, where the code has an exemption, a flag, a default-off feature, or a fallback.
The denominator
856 candidate lines across every tracked file type · 496 in the published surface · 348 sentences read in context and verdicted against the five code repos. The large majority verified TRUE and are untouched — full list in the worklog. Roughly 290 feature-description lines were deprioritised in favour of security and data-safety claims, and ~14 verified-open items are itemised and prioritised rather than silently dropped.
🚨 The most serious find: the relay is not end-to-end encrypted
privacy-security.mdclaimed the Hub relay was end-to-end encrypted and that "the Hub terminates the TLS connection and acts as a relay — it cannot decrypt the WebSocket payload". That is self-contradictory, and false.Independently verified before merging:
phlix-hub/src/Relay/contains no crypto at all — zeroopenssl_*,sodium_*,encrypt, ordecrypt— andRelayProxyManager.php:242base64-decodes each proxied body, which is encoding, not encryption. Everything relayed crosses the Hub process in cleartext: request paths, headers including the session token, JSON library listings, and the media bytes.The page now distinguishes the two cases explicitly — direct access (LAN/VPN/Cloudflare Tunnel/own proxy), where the "Hub does NOT see" list genuinely holds, versus relayed traffic, where it does not — and points at the direct remote-access options for anyone that trust boundary matters to.
Other corrections
0.ALLOW_HTTPgating that the endpoint never consults..yamlspec again restated an absolute the prose had retracted — the exact S217(b) pattern, and again caught only by the non-.mdpass. A markdown-only sweep would have left the spec contradicting the prose for a second time.⚠ One correct-looking fix that was reverted
Seven port references were changed to
8096, then32400turned up in ~30 files. The underlying truth is real —config/server.php:7hardcodes8096, andPHLIX_HTTP_PORThas zero hits in the server source, soenv-vars.md:193documents a variable that does not exist — but that is an estate-wide factual-default defect, not an absolute-claim overclaim. Fixing seven pages would have left them contradicting thirty. Reverted and filed separately.Also reported, not fixed
8 items belong to other repos — including phlix-server shipping the same retracted overstatement through its API strings.
Gates
npm test0 (84) ·npm run docs:build0 · 158 pages, 3474 unique fragment links, 0 dead — identical to the untouched control atbe6f417, with the only movement beingwithout a fragment+1 from one added link.🤖 Generated with Claude Code