Skip to content

fix(security): match cookie domains by suffix, not substring (CodeQL #860/#861) - #11429

Merged
diegosouzapw merged 1 commit into
release/v3.8.50from
fix/volcengine-cookie-domain-suffix
Aug 24, 2026
Merged

diegosouzapw merged 1 commit into
release/v3.8.50from
fix/volcengine-cookie-domain-suffix

Conversation

@diegosouzapw

Copy link
Copy Markdown
Owner

Fixes CodeQL #860 and #861 (js/incomplete-url-substring-sanitization, High) in open-sse/services/volcengineConsoleAutoLogin.ts.

Not a false positive

Unlike the js/insufficient-password-hash alerts this repo routinely dismisses, CodeQL is describing exactly what the code does:

cookie.domain.includes("volcengine.com")

That check is an authorization decision, not a string test. The console auto-login harvests digest, AccountID, csrfToken and userInfo out of the Playwright context and persists them as the operator's Volcengine credentials. A cookie set by volcengine.com.attacker.tld — or notvolcengine.com, or myvolcengine.com — passed that filter and was stored as a provider connection.

Both flagged lines (621 and 766) are the same predicate: the capture loop and the timeout diagnostics.

Fix

New matchesCookieDomain() in open-sse/utils/cookieDomain.ts: exact host or dot-boundary suffix, leading dots (the RFC 6265 spelling) and case normalized on both sides, failing closed on an empty expected domain.

This is not a new idea in the codebase — isAdobeCookieDomain() in adobeFireflyBrowserLogin.ts already does exactly this for the Adobe login flow. The helper generalizes that shape so the next browser-login service does not have to rediscover it.

One finding CodeQL did not report

Sweeping the class turned up the identical weakness in inAppLoginService.ts:

c.name === source.name && (!domain || c.domain.includes(domain.replace(/^\./, "")))

Same code path (Playwright cookie capture → stored operator credential), same consequence. CodeQL missed it because the expected domain comes from TOKEN_EXTRACTION_CONFIGS instead of a literal, so the query's constant-string heuristic never fired. Both callsites now share the helper.

Flagging it explicitly because it widens the diff past the two alerts you asked about — it is one file and one line, and leaving the same hole open next door after fixing this one seemed worse than the extra scope.

Validation

tests/unit/volcengine-cookie-domain-suffix.test.ts — 5 tests, red before the fix:

  • the real console domains (volcengine.com, .volcengine.com, console.volcengine.com, uppercase, whitespace-padded)
  • seven look-alikes rejected (volcengine.com.attacker.tld, notvolcengine.com, volcengine.company, volcengine.com.br, …)
  • empty / undefined domain rejected instead of throwing
  • the config-supplied path (matchesCookieDomain directly), including failing closed when the expected domain is missing or just "."

Sibling sweep and gates, hermetic (env -u OMNIROUTE_API_KEY):

  • tests/unit/services/volcengine-console-auto-login.test.ts + tests/unit/tokenExtractionConfig.test.ts — 46/46
  • tests/unit/in-app-login-service.test.ts — 2/2
  • typecheck:core — exit 0, no output
  • prettier --check — clean; eslint — the only two errors are the pre-existing no-explicit-any pair already frozen in eslint-suppressions.json (count: 2, unchanged — the diff adds and removes no any)
  • check:cycles, check:tracked-artifacts — PASS

⚠️ base-red inherited: #9985

CodeQL js/incomplete-url-substring-sanitization, alerts #860 and #861:
volcengineConsoleAutoLogin accepted any cookie whose `domain` merely *contained*
"volcengine.com".

That check is an authorization decision, not a string test. The console
auto-login harvests `digest`, `AccountID`, `csrfToken` and `userInfo` out of the
Playwright context and persists them as the operator's Volcengine credentials,
so a cookie set by `volcengine.com.attacker.tld` — or `notvolcengine.com` — was
captured and stored as a provider connection.

Add `matchesCookieDomain()` (open-sse/utils/cookieDomain.ts): exact host or
dot-boundary suffix, leading dots and case normalized on both sides, failing
closed on an empty expected domain. Same shape as the existing
`isAdobeCookieDomain` in adobeFireflyBrowserLogin.ts, which already got this
right.

While sweeping the class, inAppLoginService's cookie capture had the identical
weakness — `c.domain.includes(domain.replace(/^\./, ""))` — with the identical
consequence: a look-alike host's cookie stored as the operator's credential.
CodeQL did not flag it because the expected domain comes from
TOKEN_EXTRACTION_CONFIGS rather than a literal. Both callsites now share the
helper.

tests/unit/volcengine-cookie-domain-suffix.test.ts — 5 tests, red before the
fix, covering the real domains, seven look-alikes, empty/missing input, and the
config-supplied path.
@diegosouzapw
diegosouzapw merged commit 1d0c5a3 into release/v3.8.50 Aug 24, 2026
15 of 22 checks passed
@diegosouzapw
diegosouzapw deleted the fix/volcengine-cookie-domain-suffix branch August 25, 2026 02:37
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…uzapw#11429)

CodeQL js/incomplete-url-substring-sanitization, alerts diegosouzapw#860 and diegosouzapw#861:
volcengineConsoleAutoLogin accepted any cookie whose `domain` merely *contained*
"volcengine.com".

That check is an authorization decision, not a string test. The console
auto-login harvests `digest`, `AccountID`, `csrfToken` and `userInfo` out of the
Playwright context and persists them as the operator's Volcengine credentials,
so a cookie set by `volcengine.com.attacker.tld` — or `notvolcengine.com` — was
captured and stored as a provider connection.

Add `matchesCookieDomain()` (open-sse/utils/cookieDomain.ts): exact host or
dot-boundary suffix, leading dots and case normalized on both sides, failing
closed on an empty expected domain. Same shape as the existing
`isAdobeCookieDomain` in adobeFireflyBrowserLogin.ts, which already got this
right.

While sweeping the class, inAppLoginService's cookie capture had the identical
weakness — `c.domain.includes(domain.replace(/^\./, ""))` — with the identical
consequence: a look-alike host's cookie stored as the operator's credential.
CodeQL did not flag it because the expected domain comes from
TOKEN_EXTRACTION_CONFIGS rather than a literal. Both callsites now share the
helper.

tests/unit/volcengine-cookie-domain-suffix.test.ts — 5 tests, red before the
fix, covering the real domains, seven look-alikes, empty/missing input, and the
config-supplied path.

Co-authored-by: Xiangzhe <bakryun0718@proton.me>
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.

2 participants