fix(qr): accept fountain parts with seqnum above sequence count

[ben-kaufman]

Summary

URQR scanning is broken in v6.13.0 (current Latest release): scanning any animated UR fails immediately with "UR processing failed", typically on the first captured frame. Regression introduced by ef78955 (the security-audit hardening for #2608), first shipped in v6.13.0 — 6.12.x is unaffected.

Root cause

The #2608 hardening added a multipart frame validation that rejects frames whose sequence number exceeds the declared sequence count (sequenceNumber > sequenceCount).

Animated URs are fountain-coded (BCR-2020-005): after the pure fragments 1..N have played, the stream keeps emitting mixed parts N+1, N+2, … indefinitely — that unbounded sequence is exactly what allows a decoder that missed frames (or joined mid-animation) to recover. Since the animation has usually been looping before the camera points at it, nearly every captured frame has seqNum > seqCount, so the very first frame threw UrSequenceLimitExceeded, the reader reset in the catch path, and the scan could never complete.

The existing roundtrip test didn't catch this because it feeds parts 1..N in order, where seqNum ≤ seqCount always holds.

Fix

Drop the sequenceNumber > sequenceCount condition in UrQrReader._validateMultipartFrame. The guards that actually bound the decoder's resources stay in place:

• sequenceCount > maxMultipartParts (1000) — caps the part count
• data.length > maxMultipartMessageSize (1 MB) — caps frame size
• sequenceNumber < 1 — rejects malformed frames

So the #2608 DoS protection is preserved; only the spec-violating check is removed.

Tests

• New regression test test/core_test/urqr/ur_qr_reader_test.dart: builds a genuine fountain stream with the real UREncoder (including mixed parts past isComplete, which UrQrGenerator cannot produce) and decodes a stream the camera joins mid-animation — the exact reported scenario. Verified to fail against the pre-fix code and pass with the fix.
• All existing test/core_test/urqr and test/security_audit tests pass (106 tests), including the Security (Medium, QR F-2): unbounded UR sequence lengths can exhaust device memory #2608 sequence-bound test.
• make analyze clean, dart fix --dry-run clean.

[claude]

Claude finished @ben-kaufman's task in 5m 42s —— View job

---

Review: fix(qr): accept fountain parts with seqnum above sequence count

• Read the diff and surrounding code
• Verify fountain-coding claim & remaining DoS guards (vs Security (Medium, QR F-2): unbounded UR sequence lengths can exhaust device memory #2608)
• Check decoder internals for unbounded-resource risks with seqNum > seqCount
• Review the new regression test
• Post findings + summary

Verdict: the fix is correct and I found no blocking issues. One non-blocking security observation and one test-robustness issue worth addressing, below.

Note: I attempted to post these as inline review comments via the GitHub API, but that call isn't permitted for this workflow (gh api POST requires approval). Findings are listed here with file/line references instead. Similarly, I could not execute flutter test/make analyze in this sandbox, so I'm relying on static analysis plus CI.

Correctness of the fix — confirmed

The removed sequenceNumber > sequenceCount check was genuinely spec-violating. Per BCR-2020-005, a fountain-coded UR stream emits parts N+1, N+2, … indefinitely after the pure fragments, and the reference decoders (URKit, Hummingbird, foundation-ur, and the pinned bc-ur-dart fountain decoder itself) impose no seqNum ≤ seqCount bound — they validate only stream consistency (seqCount, message length, checksum, fragment length). So rejecting those frames broke every scan that joins a looping animation, exactly as described.

I also verified the fix doesn't undo #2608:

• test/security_audit/issue_2608_test.dart only asserts the sequenceCount > 1000 rejection and the pinned decoder ref — both untouched, so it still passes.
• The retained guards (sequenceCount > maxMultipartParts, per-frame 1 MB cap, sequenceNumber < 1) are intact at lib/core/urqr/urqr.dart:146-150.
• Edge case checked: an absurd seqNum with >19 digits makes int.parse throw FormatException, which the catch block at lib/core/urqr/urqr.dart:123-130 converts to UrQrError + reset — same handling as any malformed frame, no crash.
• Post-completion behavior in the app is safe: qr_scanner_widget.dart resets the reader immediately on completion before invoking onScanned, so the still-looping animation can't wedge or wipe a finished decode.

Finding 1 — non-blocking security note (lib/core/urqr/urqr.dart:146)

The old (wrong) check had one incidental benefit: it bounded the fountain decoder's mixed-part buffer to at most maxMultipartParts distinct sequence numbers. With it removed, a hostile looping stream that withholds pure fragments can grow the decoder's mixed-part buffer and _processedQrCodes by one entry per frame for as long as the camera points at it. Practical severity is low — ingestion is limited to camera frame rate, each frame is capped at 1 MB, and _processedQrCodes was already growable pre-fix by varying fragment bytes within 1..N — but it slightly widens the surface #2608 was about.

If you want a hard bound back without violating the spec, bound session resources rather than seqNum — e.g. reset/throw once _processedQrCodes.length exceeds a generous cap like 4 * maxMultipartParts. A cap on seqNum itself would be wrong: a scanner joining an animation that has been looping for a while legitimately sees arbitrarily large sequence numbers.

Finding 2 — test fragility (test/core_test/urqr/ur_qr_reader_test.dart:27-29)

The test feeds the mixed part, then all pure parts 1..N unconditionally. If the mixed part's fragment set ever allows the decoder to complete before the last pure part is fed (which fragment subset seqNum N+1 maps to is an opaque function of the fountain PRNG over this exact payload/fragment length), the next receive() gets URDecoder.receivePart(...) == false (the decoder refuses parts once complete), which throws FailedToReceiveUrPart and reset() wipes decoded — the test would fail. It passes today only because this particular mixed part happens to stay redundant until the final pure part; any change to the payload, fragment length, or encoder internals could flip it.

Also, since every pure part is fed, the test proves the fountain part is accepted (which is the regression, so it's adequate) but not that it contributes to recovery. Both are fixed cheaply by simulating the actual missed-frame scenario:

final reader = UrQrReader();
reader.receive(mixedPart);
for (final part in pureParts.skip(1)) { // camera missed frame 1
  if (reader.isComplete) break;
  reader.receive(part);
}
while (!reader.isComplete) {
  reader.receive(encoder.nextPart()); // animation keeps looping
}
expect(reader.isComplete, isTrue);

Now the mixed parts must supply the missing fragment, and the test is robust to PRNG/encoder changes. Fix this →

Summary

The root-cause analysis in the PR description is accurate, the one-line validation change is the right fix, the explanatory comment is good, and the remaining #2608 guards genuinely cover the DoS concern for realistic inputs. Recommend hardening the new test as in Finding 2 before merge; Finding 1 is optional follow-up.