Skip to content

Fix the flaky suite and triage the CodeQL baseline - #215

Merged
Bccorb merged 2 commits into
mainfrom
chore/release-prep-190-208
Aug 30, 2026
Merged

Fix the flaky suite and triage the CodeQL baseline#215
Bccorb merged 2 commits into
mainfrom
chore/release-prep-190-208

Conversation

@Bccorb

@Bccorb Bccorb commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Pre-release work on #190 and #208. Two commits, each green on its own (typecheck
plus full suite verified at each SHA).

Rebased. This PR originally carried a third commit adopting
@seamless-auth/types 0.15.0. main moved during the work and 40e17ca
already does that, better: it extracts an authenticatorPolicyService, warns
at startup when an allow list is set without attestation: 'direct', and
refuses an authenticator that declines to identify itself rather than admitting
it. My duplicate has been dropped and this is rebased onto it. See the note at
the bottom about the dependent survey, which still applies.

test: fix the flaky suite, closes #190

Three 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.

  1. clearAllMocks does not drain the Once queue. Only mockReset clears
    onceMockImplementations. Every spec's beforeEach called clearAllMocks,
    so a queued mockResolvedValueOnce the code path never consumed survived into
    a later test and shifted every subsequent queued value by a place.
    organizations.spec.ts queues 22. That accounts for the full symptom range,
    including the 20s timeout: a call expecting a queued value got undefined and
    awaited something that never settled.
  2. Fire-and-forget handlers bleeding across the test boundary. Handlers
    answer the request and then finish void AuthEventService.log(...), so
    supertest resolved with continuations still queued; one landing mid-next-test
    breaks a toHaveBeenCalledTimes. Drained in a global afterEach. The void
    in the handlers is deliberate and untouched.
  3. Stubs are process state. vi.stubEnv / vi.stubGlobal write to
    process.env and globalThis, which isolate does not roll back between
    files. Nothing in the tree ever called vi.unstubAllGlobals.

Verified: 20 consecutive runs in each mode, re-run after the rebase. main
before this failed 3 of 20 sequential runs.

Side effect: fileParallelism is back on and the suite went from 12.9s to
2.1s
. npm run coverage no 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 never
simulated 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 #208

The 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.ts built its output on a plain object, so a __proto__ key in
    untrusted 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 did
    check-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 at
    five call sites, since untrusted values reach log messages through template
    strings across the codebase and one missed interpolation reopens it.

Escalated to #213: passkeyAvailable is read straight from the POST /login
body, and sending false skips the passkey-only branch and gets the caller
offered 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-hash alerts are HMAC over OAuth state and PKCE material
rather than passwords; eight of the nine user-controlled-bypass alerts fail
closed or are an admin list filter; the polynomial-redos alert is unreachable
because 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-injection alerts may reappear on the next scan, because the
escaping 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 syncedPasskeys breaking change

Carried out for the duplicate commit, and it applies unchanged to 40e17ca now
on main, so recording it here rather than losing it.

403 synced_passkey_not_allowed and 403 authenticator_not_allowed are new
responses on POST /webAuthn/register/finish, and authenticator_policy gained
three fields.

Repo Impact
seamless-auth-server None. readPassthroughFailure forwards the body verbatim, so the code and status arrive intact.
seamless-auth-react Breaks the UX. extractMessage uses { error } as the message, so the user sees the literal synced_passkey_not_allowed. fells-code/seamless-auth-react#132
seamless-auth-admin-dashboard No authenticator_policy UI at all, so the new default cannot be changed from the console. fells-code/seamless-auth-admin-dashboard#238
seamless-auth-docs New fields and refusal codes undocumented, as is the existing prf_required. fells-code/seamless-auth-docs#41
seamless-cli, seamless-templates No coupling found.

Worth weighing before the release: with syncedPasskeys defaulting to block,
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:check and npm run build
all pass. Full suite green at 1054 tests.

Two changesets included.

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
Bccorb force-pushed the chore/release-prep-190-208 branch from 3837ea6 to 99c77c7 Compare August 30, 2026 16:37
@Bccorb Bccorb changed the title Fix the flaky suite, triage the CodeQL baseline, and adopt types 0.15.0 Fix the flaky suite and triage the CodeQL baseline Aug 30, 2026
Comment thread src/services/organizationService.ts Fixed
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
Bccorb force-pushed the chore/release-prep-190-208 branch from 99c77c7 to a7b8e77 Compare August 30, 2026 16:43
@Bccorb
Bccorb merged commit d0514db into main Aug 30, 2026
4 checks passed
@Bccorb
Bccorb deleted the chore/release-prep-190-208 branch August 30, 2026 16:57
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.

Test suite is intermittently flaky, including in the sequential coverage run

2 participants