tests: rewrite the change-kdf test against the model server - #1451
Closed
quexten wants to merge 2 commits into
Closed
tests: rewrite the change-kdf test against the model server#1451quexten wants to merge 2 commits into
quexten wants to merge 2 commits into
Conversation
Integration tests currently hand-roll their backend per test: a fetch mock returning an empty 200 for the one route under test, plus account constants copied out of tests/utils.ts. Nothing is stored, so a test can only assert on the request body — which shows what the client intended, not what a user is left with afterwards. This adds tests/model-server/: a stateful model of the API that tests seed accounts into and then assert against. Two rules live on the server rather than in every test, so a client that gets them wrong fails the operation itself — a write carrying a superseded revision is rejected, and every request body is checked for plaintext no client may send. Handlers and serializers are typed against DTOs declared in dto.ts rather than taking `any`. A model server that accepts `any` can drop a field on the way out and still pass every test, which is the bug class this suite exists to catch. model-server.test.ts covers the harness's own mechanisms; each of its assertions was confirmed to fail when the thing it covers is broken. Also turns on `tsc --noEmit` in `npm test` and widens tsconfig's include to tests/**, which was excluding every test file from type checking. That surfaced seven pre-existing brand-type errors, fixed here.
The old test asserted key ordering in the posted JSON and never proved the account still opens afterwards. A KDF change that posts a correct body and then persists the wrong thing locally passed it. This drives change_kdf against a seeded account and asserts on the state the change leaves behind: the state bridge, the stored account and the wire must agree, and the account must still open from local state alone and from a client holding nothing but what the server serves. The two round trips are separate on purpose and run in that order. A sync overwrites the local state the first one checks, and only the first one notices a write that reached the server but corrupted what it persisted — confirmed by corrupting the persisted wrapped user key and watching lock/unlock fail while logout/login still passed. Two findings from making those assertions fail on demand: - validateAfterLockUnlock took a server-derived unlock method, so it never read local state at all. It now unlocks from the state bridge, which is what makes the split meaningful. - expectPlaintextEqual now refuses a comparison whose ignore list covers every field the value carries. An assertion that excuses everything it was meant to check is worse than none, because it looks like coverage. Accounts are seeded from tests/fixtures/accounts.ts, shaped as SeedAccount so swapping in JSON test vectors is a change of source and nothing else.
This was referenced Sep 4, 2026
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
Comment on lines
+33
to
+34
| export const PASSWORD_ACCOUNT_USER_KEY = | ||
| "w2LO+nwV4oxwswVYCxlOfRUseXfvU03VzvKQHrqeklPgiMZrspUe6sOBToCnDn9Ay0tuCBn8ykVVRb7PWhub2Q=="; |
There was a problem hiding this comment.
Exposed secret in crates/bitwarden-wasm-internal/integration-tests/tests/fixtures/accounts.ts - low severity
Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More Info
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| import { isChangeKdfError, type ChangeKdfError } from "@bitwarden/sdk-internal"; | ||
|
|
||
| import { NO_MASTER_PASSWORD_ACCOUNT } from "../fixtures/accounts"; | ||
| import { unlockMethodFor, validateAfterLockUnlock } from "../model-server/validate"; |
Contributor
🔍 SDK Breaking Change DetectionSDK Version:
Breaking change detection uses the build of the SDK from this branch, including any incompatibities pre-existing on or merged into this branch. Check the workflow logs to confirm. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## km/integration-model-server #1451 +/- ##
===============================================================
- Coverage 86.14% 86.01% -0.14%
===============================================================
Files 533 525 -8
Lines 79394 77769 -1625
===============================================================
- Hits 68397 66891 -1506
+ Misses 10997 10878 -119 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
quexten
force-pushed
the
km/integration-model-server
branch
from
September 8, 2026 13:05
2a3a596 to
6178c03
Compare
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.
Objective
The old
change-kdf.test.tsasserted key ordering in the posted JSON and never proved the account still opens — a change that posts correctly then persists the wrong thing locally passed it.Now
change_kdfruns against a seeded account and the assertions are on what it leaves behind: bridge, stored account and wire must agree, and the account must still open from local state alone and from a client holding only what the server serves. Order matters — a sync overwrites the local state the first check reads.Split into
conformance.test.ts(wire shape, persisted state) andedge-cases.test.ts(three refusals, each asserting neither side moved).Findings from making these fail on demand
validateAfterLockUnlockwas vacuous — it took a server-derived unlock method, so it never read local state, the one thing it claims to check. Now unlocks from the bridge. tests: add model api server and test support funcitons and claud rules #1368 has the same bug; its rules doc asserts the opposite.containedKeyIdis really sent onunlockData; tests: migrate integration tests #1369's key-list assertion omits it and would fail.expectPlaintextEqualnow refuses a comparison whose ignore list covers every field present.Testing
10 tests. Deletes the superseded
user-crypto-management/change-kdf.test.ts.Falsifying
change_kdf's own bridge write-back needs a Rust rebuild, not done; proven at the harness level instead.Breaking changes
None. Test-only.