fix(tokenizer): contain malformed input — clamp children to parent bounds, treat both delimiters as terminators (review #1-#7)

[MichaelLeeHobbs]

Second batch from the deep adversarial review: a cluster of silent data-loss/corruption bugs on malformed input — precisely the attack surface SECURITY.md flags. Each was independently reproduced (repros in the review report). All share one root cause: child constructs were bounded by the whole stream rather than their enclosing item/sequence, and the delimiter-terminator set was incomplete.

#	Before (silent)	After
#1/#3/#7	a stray/mis-set FFFE,E00D/E0DD was read as an element xfffee00d; the item then swallowed every following root element to EOF	delimitation items are structural terminators at element boundaries in any item frame — E00D ends+consumes, E0DD ends without consuming (belongs to the sequence). A real element never has group FFFE, so this can't misfire on genuine values.
#4	an undefined-length non-sequence element with a missing delimiter ate its siblings to end-of-stream	scanUnknown is bounded by frame.bound
#5	an item length overrunning its sequence pulled the sequence's siblings into the item (asymmetric with readValue/pushSequence which threw)	items are bounded by their enclosing sequence; an overlong length is clamped with a warning
#2	a defined-length encapsulated BOT overrunning the value read the next element's bytes as offset entries	the BOT is bounded by the value end → falls back to opaque via the speculative frame
#6	trailing padding after an early scan return surfaced as phantom elements	defined-length encapsulated always resumes exactly at the value end

6 regression tests added. Full suite (620) green including the byte-identical corpus round-trip — no offset-model regressions.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DSSLWZvjSRByQP3KsghQcJ

[Copilot]

Pull request overview

This PR hardens the tokenizer against malformed DICOM input by enforcing parent-frame bounds during scanning/iteration and by treating both item/sequence delimitation items as structural terminators in non-root item frames, preventing silent data loss/corruption on adversarial structures.

Changes:

• Add structural termination for item frames when encountering delimitation items at element boundaries.
• Bound undefined-length “unknown” scans and sequence item extents to their enclosing frame to prevent sibling swallowing.
• Tighten encapsulated pixel data scanning by bounding BOT reads to the value end and resuming exactly at the defined-length value end; add regression tests for review cases #1–#7.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
src/tokenizer.ts	Adds delimiter-based termination for item frames; clamps item bounds to sequence; bounds scanUnknown to frame bounds.
src/tokenizer.test.ts	Adds regression tests covering multiple malformed-input containment scenarios (#1–#7).
src/encapsulated.ts	Bounds BOT reads to the value end for defined-length encapsulation; forces resume at value end to avoid phantom trailing elements.

Comments suppressed due to low confidence (3)

src/tokenizer.ts:237

• For undefined-length item frames, this branch uses stream.remaining and seeks to end-of-stream when the delimiter is missing. When the item is nested inside a defined-length sequence, that can run past frame.bound and either (a) swallow sibling elements or (b) throw malformed once an element overruns the bound, instead of containing the scan to the enclosing sequence/item bound.

This issue also appears in the following locations of the same file:

• line 252
• line 427

        // Delimitation items are structural terminators at element boundaries in
        // any non-root (item) frame — a real data element never has group FFFE,
        // so this cannot misfire on genuine values (review #1/#3/#7).
        if (!frame.root && this.terminateItemAtDelimiter(frame)) {
            return;
        }
        if (frame.undefinedLength) {
            if (this.stream.remaining < 8) {
                this.warn('missing-item-delimiter', 'eof encountered before finding item delimiter (FFFE,E00D) in item of undefined length');
                this.stream.seek(this.stream.remaining);
                this.finalizeDataSet(frame, this.stream.position, this.stream.position);
                return;
            }
            this.readElement(frame);
            return;

src/tokenizer.ts:444

• scanUnknown is intended to be bounded by frame.bound, but the loop currently checks this.stream.remaining < 8 (end-of-stream) rather than bytes remaining within frame.bound. If frame.bound < stream.length, this can peek/seek past the enclosing frame and potentially consume the next sibling's delimiter/tag bytes while scanning.

    private scanUnknown(frame: DataSetFrame, header: ElementHeader): UnknownElement {
        const maxEnd = frame.bound;
        let contentEnd: number;
        for (;;) {
            if (this.stream.position >= maxEnd || this.stream.remaining < 8) {
                this.warn('missing-item-delimiter', `element ${tagToString(header.tag)} of undefined length has no delimitation item; using end of data`);
                this.stream.seek(maxEnd - this.stream.position);
                contentEnd = this.stream.position;
                break;
            }
            const peeked = this.stream.peekTag();
            if (peeked === TAG_ITEM_DELIMITATION || peeked === TAG_SEQUENCE_DELIMITATION) {
                contentEnd = this.stream.position;
                this.consumeDelimiter(`delimiter of ${tagToString(header.tag)}`);
                break;
            }
            this.stream.seek(2);
        }

src/tokenizer.ts:259

• terminateItemAtDelimiter consumes an 8-byte delimiter based on stream.remaining >= 8, but does not ensure those 8 bytes are within frame.bound. If frame.bound is smaller than stream.length (nested items), this can consume past the enclosing frame boundary while trying to terminate the item.

    private terminateItemAtDelimiter(frame: DataSetFrame): boolean {
        const peeked = this.stream.peekTag();
        if (peeked === TAG_ITEM_DELIMITATION && this.stream.remaining >= 8) {
            const delimiterStart = this.stream.position;
            this.consumeDelimiter('item delimiter');
            this.finalizeDataSet(frame, delimiterStart, this.stream.position);
            return true;
        }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.