Repository navigation
security: close the findings from the 2026-08-19 audit - #74
Merged
Merged
Conversation
The nativeGoogleLogin message handler ran without checking who called it, while toggleQRButton right next to it did check. WKWebView also had no navigation policy at all, so the WebView would load any page it was sent to. Together that was a full account takeover: register an OAuth client with an attacker redirect_uri via the MCP's open DCR, get the victim to open the resulting consent link in the app (a QR code is enough), and the post-consent redirect lands the attacker's page inside the wrapper. From there window.webkit.messageHandlers.nativeGoogleLogin triggers the real Google sign-in and the token is injected into the attacker's page — the same token the CueVote server accepts as a login. Three layers now: - decidePolicyFor navigationAction gates the main frame to cuevote.com / www.cuevote.com. Everything else goes to the system browser, where the origin is visible and no bridge exists. Sub-frames stay unrestricted so the YouTube player keeps working; the server's CSP frame-src governs those. Only a page of ours may trigger the hand-off, otherwise a foreign document could launch arbitrary app URLs. - Both message handlers check WKFrameInfo.securityOrigin. The old toggleQRButton check read message.webView?.url, which is the main frame's URL — a third-party iframe would have passed it. - The token injection re-checks the origin at delivery time. ASWebAuthenticationSession stays up as long as the user needs to type a password, and the page underneath can navigate meanwhile. Also folds the QR scanner's host validation onto the same allowlist, and builds a single WKUserContentController instead of assigning a second one over the first — the old code silently dropped the user script that way, and could as easily have dropped a message handler.
…side the app wrapper Companion to the iOS navigation policy, and the half that ships without waiting for App Review. ConnectAI was the one place in the client that navigates off-origin: window.location.href = payload.redirectTo, straight to whatever redirect_uri the AI client registered. Builds already on users' phones have no navigation policy of their own, so this is what actually protects them until the app update rolls out. Inside the native wrapper the flow now stops instead of redirecting: - Pre-approval, on the ?redirect= hint, so the one-shot handle is not spent and the user can genuinely redo it in a browser. - Post-approval, on the host of the URL the server returns. The hint is attacker-controllable (they craft the link and can omit it); the server's answer is not. This is the authoritative check. Browsers are unaffected — they have an address bar and no native bridge, and redirecting to the client's redirect_uri is the point of OAuth. New strings openInBrowserTitle/openInBrowserDesc in all 35 languages, check:i18n passes.
…s payload handleFetchSuggestions was the only room handler with no authorization check at all — every one of the eleven owner actions goes through isOwner(), and SUGGEST_SONG and VOTE both require a login. It was also the most expensive handler: a cache miss is a YouTube Search call at about 100 of the 10.000 daily quota units. Worse, the caller controlled both ends of the cache write. The schema validated lengths only, so videoId — the related-videos cache key — was a free-form string, and `artist` went into the search query unmodified. An unauthenticated client that joined any public room could therefore write an attacker-chosen result list under a popular track's id, platform-wide, for the 30 days the cache entry lives, and from there into real queues via autoRefill. Nothing about it was attributable or selectively revocable. Now: - ws.user is required, like every other quota-spending action. - videoId must match the YouTube id format, so the cache key cannot be chosen freely. - title/artist are resolved server-side from the room's own tracks (current, queue, history, pending), falling back to the videos table for a track that just rotated out. The payload no longer carries them; zod strips what older clients still send. The client sends videoId only.
…r own header getClientIp read the FIRST element of X-Forwarded-For. Our nginx builds that header with `$proxy_add_x_forwarded_for`, which APPENDS the peer address — so the leading elements are whatever the client sent and the last one is nginx's own observation. The per-IP socket cap was therefore keyed on a value the client picks: send a different X-Forwarded-For per connection and the cap counts every socket separately. The server also binds 0.0.0.0, so this did not even need nginx in the path — anything that reaches port 8080 directly could name itself. Forwarding headers are now only believed when the peer is a trusted proxy (loopback by default, TRUSTED_PROXY_IPS to override), and X-Real-IP is preferred because nginx replaces it outright rather than appending to it. For X-Forwarded-For the last element is used. A direct peer is identified by its socket address. Combined with the FETCH_SUGGESTIONS fix this closes the cheap path to the YouTube day quota: search calls are ~100 units of 10.000, and unlimited sockets against an unauthenticated handler emptied it in about a second. DEPLOYMENT.md documents TRUSTED_PROXY_IPS and MAX_SOCKETS_PER_IP, and the firewall section now says why 8080 must stay closed.
Room state went to clients verbatim, and every track carries a `voters`
map keyed by Google account id plus a `suggestedBy` holding one. JOIN_ROOM
requires no login, so anyone could join a public room and read the Google
account ids of everyone who had voted, alongside their display names —
and walk the public room list doing it.
That this is an oversight rather than a decision is visible in the
codebase itself: admin.js strips exactly these two fields before state
leaves the process over localhost ("they don't need to leave the server
even over localhost"), and scrubDeletedUser exists so a deleted user's id
"is not broadcast in voters or suggestedBy".
State and deltas are now projected per recipient: voters and suggestedBy
are removed, and the viewer's own vote is folded in as `myVote`.
suggestedByUsername stays — the display name is meant to be visible.
Serialization happens once per distinct viewer identity rather than once
per socket, so all guests share one payload and so do a user's tabs.
Server-side state is unchanged, so voting, moderation, the admin API and
the GDPR scrub paths all keep working on the full data.
Client side this replaces a lookup that never matched: it read
voters[clientId], but clientId is a browser-local per-tab UUID while the
server keys by Google sub, so the vote highlight never actually appeared.
It works now. Also re-sends state to a socket whose identity changes
(login, resume, logout) — the copy it holds was projected for whoever it
was before.
shouldOverrideUrlLoading returned false unconditionally, so the main WebView would load any origin it was sent to — with no address bar, and with window.CueVoteAndroid injected into whatever document ended up loaded. addJavascriptInterface is not origin-bound. The impact is smaller than on iOS, where the same gap reached a Google access token: the interface here exposes only isNative() and a host-gated toggleQRButton. But it is a phishing surface with no visible origin, and the popup path already solved exactly this (onCreateWindow routes foreign targets to the system browser). Main-frame navigation now follows the same rule. Sub-frames are left alone so the YouTube player keeps working; the server's CSP frame-src governs those. Also: - One openExternally() helper for both paths, restricted to http/https/mailto/tel. The popup path fired ACTION_VIEW for any scheme, which let a loaded page reach other apps by their custom scheme. - QR validation, the JS-interface host check and the navigation gate now share one allowlist instead of three inline comparisons.
revokeToken deleted request.token from both stores without checking who was asking, so any authenticated client could revoke any token it learned by some other route. RFC 7009 §2.1 requires the ownership check. Exploitation needs prior knowledge of a foreign token, so this is hardening rather than an open door — but the check is one comparison. A token that does not belong to the caller is left alone silently, so the endpoint does not double as an oracle for whether a token exists.
…he client's own name Client registration is open (Dynamic Client Registration, by design), so `client_name` is whatever the requester typed — "CueVote Official" is as available to an attacker as anything else. The consent box gave that self-chosen name and the redirect host equal weight in one sentence, which puts the unverifiable value on the same footing as the verifiable one. The host is the value CueVote can actually vouch for: the authorization code goes there and nowhere else. It now leads the box, set in mono at a larger size, with the self-chosen name underneath explicitly labelled as a name anyone can pick. Replaces connectAi.requestedBy with willSendTo + clientClaimsUnverified in all 35 languages; check:i18n passes.
mcp-audit.log is written directly with appendFileSync, so pm2-logrotate — which only handles PM2's own stdout and stderr — never touches it. It has been unbounded since it was introduced, and the OAuth events added next would make that worse. Rotates at CUEVOTE_AUDIT_MAX_BYTES (10 MB) keeping CUEVOTE_AUDIT_KEEP (5) files. This is operational telemetry, not the GDPR Art. 33(5) incident record — that is kept separately, outside the repo — so a bounded window is the right trade. DEPLOYMENT.md says so next to the pm2-logrotate section, where the natural assumption is that PM2 covers everything.
Client registration is open (Dynamic Client Registration, by design) and both the client registry and the token stores are in-memory. A restart therefore erased every trace of who had registered what — so the question the audit raised, "was a client with a hostile redirect_uri ever registered?", had no answer from logs at all. For that surface, absence of evidence was not evidence of absence. Recorded now: - oauth_client_registered — client_id, the self-chosen name, redirect_uris, grant types, scope. The one that closes the evidence gap. - oauth_consent_requested — the attempt, not just the outcome. A consent screen the user refuses otherwise leaves no trace, and a burst of these across many users is what a phishing run looks like. - oauth_consent_granted — who approved which client sending their authorization to which host. - oauth_consent_failed, oauth_finalize_rejected — invalid handles, and calls to the consent bridge with a wrong or missing secret. That route is loopback-only server-to-server, so a rejection there is a misconfigured deploy or someone probing; the peer address is recorded because anything but 127.0.0.1 means it is exposed too far. - oauth_token_issued / oauth_token_revoked, with the grant type. - oauth_dev_bypass — hard-disabled under NODE_ENV=production, but it is a login bypass, so every use is visible if a dev build ever reaches users. Tokens are never written to the log. Issue and revocation carry a 12-char SHA-256 prefix instead, which is enough to tie the two together without the log holding anything usable.
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.
Fixes every finding from the full-codebase security audit run on 2026-08-19, one commit per finding.
Findings
1 · HIGH — iOS native Google-login bridge reachable from any origin (
012552c,516ec05)nativeGoogleLoginhad no origin check (thetoggleQRButtonhandler next to it did), and the WebView had no navigation policy at all. Chain: register an OAuth client with an attackerredirect_urivia the MCP's open DCR → get the victim to open the consent link in the app (a QR code suffices) → the post-consent redirect loads the attacker's page inside the wrapper → it defineswindow.handleNativeGoogleLoginand triggers the bridge → it receives a Google access token the CueVote server accepts as a login. Full account takeover.Now gated three ways: a
decidePolicyFornavigation policy that keeps the main frame on cuevote.com and hands everything else to the system browser;WKFrameInfo.securityOriginchecks on both message handlers; and an origin re-check when the token is injected.The client-side half ships without waiting for App Review and is what protects builds already on phones: inside the wrapper the consent flow refuses to hand off to a foreign host, checked both on the
?redirect=hint (before the one-shot handle is spent) and on the URL the server actually returns.2 · MEDIUM — Google account ids broadcast to every room member (
5160e52)Room state went out verbatim, and every track carries a
votersmap keyed by Google account id plus asuggestedBy.JOIN_ROOMrequires no login, so anyone could walk the public room list and harvest account ids next to display names. State and deltas are now projected per recipient —voters/suggestedByremoved, the viewer's own vote folded in asmyVote, one serialization per distinct viewer identity rather than per socket.admin.jsalready did exactly this strip for localhost.3 · MEDIUM — unauthenticated cache poisoning via FETCH_SUGGESTIONS (
f27205c)The only room handler with no authorization check, and the most expensive one. The caller controlled both the cache key (
videoId, format unvalidated) and the search term (artist), so an unauthenticated client could write an attacker-chosen result list under a popular track's id, platform-wide, for 30 days. Now requires a login, pinsvideoIdto the YouTube id format, and resolves title/artist server-side from the room's own tracks.Also fixed
7fe7f32—getClientIpread the firstX-Forwarded-Forelement, but nginx appends with$proxy_add_x_forwarded_for, so that was the client's own claim and the per-IP socket cap counted nothing. Forwarding headers are now only trusted from a trusted proxy. Together with finding 3 this closes a path to emptying the YouTube day quota in about a second.9509ef7— AndroidshouldOverrideUrlLoadingreturnedfalseunconditionally. Same gate as iOS, plus oneopenExternallyhelper restricted to http/https/mailto/tel.3545ae3—revokeTokendid not check token ownership (RFC 7009 §2.1).e2c07c3— the consent screen gave the attacker-chosenclient_namethe same weight as the verifiable redirect host. The host now leads.Verification
cuevote-server: 87 tests pass (10 new — 6 for state projection, 4 for the payload schema)cuevote-client: eslint clean, build clean,check:i18nOK (232 keys × 35 languages; consent + hand-off strings added in all 35)cuevote-mcp:tsccleanswiftc -typecheckclean against the iOS SDKcompileDebugKotlinBUILD SUCCESSFULFollow-up: OAuth registration logging (
e121fc5,8523a67)The audit surfaced an evidence gap rather than a vulnerability: client registration is open and both the client registry and the token stores are in-memory, so a restart erased every trace of who had registered what. "Was a client with a hostile redirect_uri ever registered?" had no answer from logs.
The existing
mcp-audit.lognow records the full OAuth lifecycle —oauth_client_registered,oauth_consent_requested,oauth_consent_granted,oauth_consent_failed,oauth_token_issued,oauth_token_revoked,oauth_finalize_rejected,oauth_dev_bypass. Tokens are never written; issue and revocation carry a 12-char SHA-256 prefix so the two can be tied together without the log holding anything usable.That log was also unbounded — it is written with
appendFileSync, sopm2-logrotatenever touched it. It now rotates atCUEVOTE_AUDIT_MAX_BYTES(10 MB) keepingCUEVOTE_AUDIT_KEEP(5) files, andDEPLOYMENT.mdsays so next to the pm2-logrotate section.Verified by running a full register → consent → finalize → token → revoke flow against the built server and reading back the log, and by forcing rotation with a small size limit.
Deploy note
DEPLOYMENT.mdgainsTRUSTED_PROXY_IPSandMAX_SOCKETS_PER_IP. Defaults cover the current "nginx on the same host" layout, so no action is needed unless the proxy moves. The firewall section now states why port 8080 has to stay closed.The iOS fix needs an App Store release; everything else deploys with a normal server/client rollout.