Skip to content

test(ksef): de-flake aes-cipher wrong-key spec#1539

Merged
norbert-kulus-blockydevs merged 3 commits into
add_cifrom
1538-fix-flaky-ksef-aes-cipher-wrong-key-test
Jul 14, 2026
Merged

test(ksef): de-flake aes-cipher wrong-key spec#1539
norbert-kulus-blockydevs merged 3 commits into
add_cifrom
1538-fix-flaky-ksef-aes-cipher-wrong-key-test

Conversation

@norbert-kulus-blockydevs

Copy link
Copy Markdown
Collaborator

What / Why

The KSeF unit test aes-cipher > should throw KsefSessionCryptoException when decrypting with the wrong key was inherently flaky (~0.4% of CI runs). AES-256-CBC has no built-in key verification: decrypting with a random wrong key produces pseudo-random bytes that end in accidentally valid PKCS#7 padding with probability ~1/256 (+ smaller contributions from longer paddings), in which case decryptAesCbc returns garbage instead of throwing and the toThrow assertion failed. Observed on PR #1515 CI run 29322744163, which touched only a workflow file.

Test-only fix, per the issue:

  • The wrong-key test now asserts the actual CBC contract: either decryptAesCbc throws KsefSessionCryptoException, or it returns a value different from the original plaintext ('secret'). Both are legal outcomes.
  • A new deterministic test covers the KsefSessionCryptoException wrapping path: a ciphertext truncated by 1 byte is not a multiple of the AES block size, so decipher.final() always throws.

No changes to aes-cipher.ts.

Verification

  • jest --testPathPattern aes-cipher in @openlinker/integrations-ksef: 6/6 tests pass.
  • Flake-proof loop driving the real encryptAesCbc/decryptAesCbc functions directly:
    • 100,000 wrong-key iterations: 99,626 threw KsefSessionCryptoException, 374 returned garbage (the ~0.4% case that used to fail CI - now asserted as legal), 0 failures.
    • 10,000 truncated-ciphertext iterations: 10,000/10,000 threw KsefSessionCryptoException (deterministic).
  • Scoped lint (eslint on the spec) and pnpm --filter @openlinker/integrations-ksef... build (tsc -b) pass.

Closes #1538

🤖 Generated with Claude Code

AES-256-CBC has no key verification: decrypting with a random wrong key
yields accidentally valid PKCS#7 padding ~0.4% of the time, so the old
toThrow assertion failed randomly in CI. The wrong-key test now accepts
both legal outcomes (KsefSessionCryptoException or garbage != plaintext),
and a new deterministic test covers the exception-wrapping path via a
truncated ciphertext, which always fails decipher.final().

No production code changes.

Closes #1538

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>
@norbert-kulus-blockydevs
norbert-kulus-blockydevs changed the base branch from main to add_ci July 14, 2026 10:47
@norbert-kulus-blockydevs
norbert-kulus-blockydevs changed the base branch from add_ci to main July 14, 2026 10:51
@norbert-kulus-blockydevs
norbert-kulus-blockydevs changed the base branch from main to add_ci July 14, 2026 10:57
@norbert-kulus-blockydevs

Copy link
Copy Markdown
Collaborator Author

PR Review - #1539 test(ksef): de-flake aes-cipher wrong-key spec

Scope: test-only, single file (libs/integrations/ksef/src/infrastructure/crypto/__tests__/aes-cipher.spec.ts). No production code touched, so hexagonal/port/DI criteria do not apply. Reviewed for correctness, naming, and coverage per docs/code-review-guide.md.

Summary

Correctly addresses the root cause from #1538: AES-CBC has no key verification, so a wrong key throws on PKCS#7 padding failure only ~99.6% of the time and returns garbage the rest. The rewritten test accepts both legal outcomes, and a new truncated-ciphertext test keeps the exception-wrapping path covered deterministically. Naming follows should [behaviour] when [condition]; comments explain why (not what), matching the standards. Good, minimal fix.

Findings

SUGGESTION - assertion inside try can be swallowed by the catch

try {
  const result = decryptAesCbc(ciphertext, wrongKey, iv);
  expect(result).not.toBe(plaintext);   // if this throws (JestAssertionError)...
} catch (err) {
  expect(err).toBeInstanceOf(KsefSessionCryptoException); // ...it lands here and mis-asserts
}

If decryptAesCbc ever returned exactly plaintext, the expect(...).not.toBe(...) failure would be caught by the catch and re-checked as instanceof KsefSessionCryptoException, producing a confusing failure message instead of the intended "recovered the plaintext" one. The triggering condition (a wrong key recovering the exact plaintext) is cryptographically negligible (~2^-128), so this is cosmetic, not a real flake risk. If you want to tighten it, move the success-path assertion out of the try, e.g.:

let result: string | undefined;
try {
  result = decryptAesCbc(ciphertext, wrongKey, iv);
} catch (err) {
  expect(err).toBeInstanceOf(KsefSessionCryptoException);
  return;
}
expect(result).not.toBe(plaintext);

Non-blocking.

Notes (no action)

  • Truncated-ciphertext test: 'secret' + PKCS#7 pads to one 16-byte block; slice(0, -1) yields 15 bytes, not a block multiple, so decipher.final() throws deterministically. Solid choice for covering the wrapping path.
  • The author reported a 100k-iteration loop (0.374% garbage, 0 failures) confirming both branches are exercised and the flake is gone.

Verdict

Approve. Merge-ready as-is. The single suggestion is optional polish, not a blocker.

Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>
@norbert-kulus-blockydevs
norbert-kulus-blockydevs merged commit 544c08c into add_ci Jul 14, 2026
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.

[BUG] Integration/KSeF - flaky aes-cipher wrong-key test fails ~0.4% of CI runs (accidental valid PKCS#7 padding)

1 participant