Fix the flaky suite and triage the CodeQL baseline - #215
Merged
Conversation
Closes #190 The suite failed intermittently on assertions unrelated to whatever was being changed, in both the parallel run and the sequential coverage run. Three leaks, none of them about parallelism, which is why running one file at a time never helped. vi.clearAllMocks() empties call history but leaves the mockResolvedValueOnce queue alone. Only mockReset() clears onceMockImplementations. Every spec's beforeEach called clearAllMocks, so a value queued by one test and not consumed by the code path survived into a later one and shifted every subsequent queued value by a place. organizations.spec.ts queues 22 of them. That covers the whole symptom range: a wrong status, a wrong body, and a request that never settles, which is what happens when a call that expected a queued value gets undefined. mockReset restores an implementation passed to vi.fn(impl) but drops one attached with a chained .mockResolvedValue(), so the two mocks written the second way were converted. Handlers answer the request before their fire-and-forget audit logging settles, so supertest resolved with continuations still queued and a stray call could land in the middle of the next test, breaking a toHaveBeenCalledTimes or shifting a toHaveBeenNthCalledWith on a shared mock. Those are drained in a global afterEach. The void in the handlers is deliberate and was left alone. vi.stubEnv and vi.stubGlobal write to process.env and globalThis, which isolate does not roll back between files because it resets the module registry and not the worker. Nothing in the tree called vi.unstubAllGlobals. Both are now restored automatically, which meant moving APP_ORIGINS from a stub in mocks.ts to a plain assignment in env.ts, since restoring stubs before every test would otherwise drop it after the first test of each file. With the leaks closed the workaround is unnecessary: fileParallelism is back on and coverage no longer forces sequential execution. The suite went from 12.9s to 2.1s. Two tests were passing only because of the leak and are fixed rather than deleted. user.spec.ts 'returns 404 when no user' never simulated a missing user, carried an unused import and asserted that the status was one of two values; it now sends the x-omit-user header its own file mock already supported and asserts 404. Verified with 20 consecutive runs in each mode, plus 8 more in each mode under coverage. Before this, main failed 3 of 20 sequential runs.
Bccorb
force-pushed
the
chore/release-prep-190-208
branch
from
August 30, 2026 16:37
3837ea6 to
99c77c7
Compare
Refs #208 Three of the alerts were real. The rest are triaged and dismissed with written reasons on the issue, and one is a genuine downgrade that needs a product decision, so it is filed as #213 rather than patched here. redactSensitiveValue built its output on a plain object literal, so a __proto__ key in untrusted audit metadata reached the prototype setter instead of creating a property. The key silently disappeared from the redacted output unredacted, and replaced that object's prototype with caller-supplied content. It is not the global prototype pollution the rule name suggests, but it is a redaction gap and an attacker-influenced prototype on an object headed for the audit trail. The output is built on a null prototype now, so the key is recorded as ordinary data. Log messages interpolate request paths, provider ids and similar caller-supplied values through template strings across the codebase, so a newline let a caller forge a second log entry. Control characters are escaped in the winston format, the one place every line already passes through for redaction, rather than at each of the five call sites where one missed interpolation reopens it. Dev signing key generation checked for a key file and then wrote one. Two processes starting together could both pass the check, both generate, and both write, leaving one of them signing with a key that was neither the one on disk nor the one JWKS publishes. Both paths create the file exclusively now and adopt the winner's key on losing the race. keyManager keeps a cheap existence check in front of the generation so the common case does not pay for an RSA keypair it is about to throw away. slugify is not vulnerable to the flagged polynomial backtracking, because the preceding collapse of non-alphanumeric runs leaves no two dashes adjacent. Measured flat and sub-millisecond at 10k, 40k and 80k dashes. That safety depends on the order of the two replaces, so it is recorded as a comment.
Bccorb
force-pushed
the
chore/release-prep-190-208
branch
from
August 30, 2026 16:43
99c77c7 to
a7b8e77
Compare
This was referenced Aug 30, 2026
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
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.
Pre-release work on #190 and #208. Two commits, each green on its own (typecheck
plus full suite verified at each SHA).
test: fix the flaky suite, closes #190Three leaks, none of them about parallelism, which is why running one file at a
time never helped and why the sequential coverage run flaked too.
clearAllMocksdoes not drain theOncequeue. OnlymockResetclearsonceMockImplementations. Every spec'sbeforeEachcalledclearAllMocks,so a queued
mockResolvedValueOncethe code path never consumed survived intoa later test and shifted every subsequent queued value by a place.
organizations.spec.tsqueues 22. That accounts for the full symptom range,including the 20s timeout: a call expecting a queued value got
undefinedandawaited something that never settled.
answer the request and then finish
void AuthEventService.log(...), sosupertest resolved with continuations still queued; one landing mid-next-test
breaks a
toHaveBeenCalledTimes. Drained in a globalafterEach. Thevoidin the handlers is deliberate and untouched.
vi.stubEnv/vi.stubGlobalwrite toprocess.envandglobalThis, whichisolatedoes not roll back betweenfiles. Nothing in the tree ever called
vi.unstubAllGlobals.Verified: 20 consecutive runs in each mode, re-run after the rebase.
mainbefore this failed 3 of 20 sequential runs.
Side effect:
fileParallelismis back on and the suite went from 12.9s to2.1s.
npm run coverageno longer forces sequential either.Two tests were passing only because of the leak and are fixed rather than
deleted, most notably
user.spec.ts > returns 404 when no user, which neversimulated a missing user and asserted
expect([200, 404]).toContain(res.status).One residual is tracked separately in #214: a rare 20s timeout under coverage,
one occurrence in ~57 runs, with a different signature (a handler that cannot
hang on its own logic, so it looks like worker starvation rather than ordering).
fix(security): CodeQL baseline, refs #208The baseline had grown to 25 alerts, not the 10 in the issue. All are now
resolved: 8 fixed here, 16 dismissed with written reasons on the issue, 1
escalated.
Fixed:
redaction.tsbuilt its output on a plain object, so a__proto__key inuntrusted audit metadata hit the prototype setter: the key vanished from the
redacted output unredacted and replaced that object's prototype with
caller-supplied content. Now built on a null prototype.
file-system-race×2, worse than "dev only" suggested. Both didcheck-then-write, so two processes starting together could both pass the check,
both generate and both write, leaving one signing with a key that was neither
on disk nor published in JWKS. Both now create exclusively and adopt the
winner's key on losing the race.
log-injection×5, fixed centrally in the winston format rather than atfive call sites, since untrusted values reach log messages through template
strings across the codebase and one missed interpolation reopens it.
Escalated to #213:
passkeyAvailableis read straight from thePOST /loginbody, and sending
falseskips the passkey-only branch and gets the calleroffered email OTP instead. That defeats
passkey_login_fallback_enabled: false,which is the one setting whose job is to prevent exactly that. It overlaps #177
and needs a product decision, so it is not folded in here.
Notable dismissals, with the reasoning on the issue: the four
insufficient-password-hashalerts are HMAC over OAuth state and PKCE materialrather than passwords; eight of the nine
user-controlled-bypassalerts failclosed or are an admin list filter; the
polynomial-redosalert is unreachablebecause the preceding collapse leaves no two dashes adjacent (measured flat at
10k/40k/80k dashes, and the invariant is now a comment since it depends on the
order of the two replaces).
The five
log-injectionalerts may reappear on the next scan, because theescaping sits in the winston format rather than on the taint path CodeQL follows.
If so they should be dismissed on the same footing as
clear-text-logging, not"fixed" by scattering sanitiser calls across every call site.
Dependent survey for the
syncedPasskeysbreaking changeCarried out for the duplicate commit, and it applies unchanged to
40e17canowon
main, so recording it here rather than losing it.403 synced_passkey_not_allowedand403 authenticator_not_allowedare newresponses on
POST /webAuthn/register/finish, andauthenticator_policygainedthree fields.
seamless-auth-serverreadPassthroughFailureforwards the body verbatim, so the code and status arrive intact.seamless-auth-reactextractMessageuses{ error }as the message, so the user sees the literalsynced_passkey_not_allowed. fells-code/seamless-auth-react#132seamless-auth-admin-dashboardauthenticator_policyUI at all, so the new default cannot be changed from the console. fells-code/seamless-auth-admin-dashboard#238seamless-auth-docsprf_required. fells-code/seamless-auth-docs#41seamless-cli,seamless-templatesWorth weighing before the release: with
syncedPasskeysdefaulting toblock,a default deployment refuses every iCloud Keychain and Google Password Manager
passkey, and the React SDK currently renders the raw code as the user-facing
message. The two SDK issues above are what make that recoverable.
Checks
npm run typecheck,npm run lint,npm run format:checkandnpm run buildall pass. Full suite green at 1054 tests.
Two changesets included.