Skip to content

fix(render): harden drawBox width coupling and fragile newline assertion (#1345) #1355

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
griffinwork40 merged 1 commit into from
Aug 28, 2026
Merged

fix(render): harden drawBox width coupling and fragile newline assertion (#1345) #1355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/cli/render/compact-diff-view.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,13 @@ describe('compactDiffView', () => {
// Raw control chars must not appear in the output
expect(out).not.toContain('\x07');
expect(out).not.toContain('\r');
expect(out).not.toContain('\n\n'); // only the stat/box separator newline is expected
// Assert directly that sanitization stripped the raw LF from the
// adversarial path: the stat header (first output line) must not contain
// an embedded newline. Asserting '\n\n' here would couple the test to
// drawBox's trailing-newline behavior rather than to path sanitization.
const statLine = strip(out).split('\n')[0];
expect(statLine).not.toContain('\x0A');
Comment on lines +357 to +358

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Test LF sanitization before splitting the output

If sanitizeForDisplay regresses and preserves the adversarial path's LF, this assertion still passes because split('\n')[0] can never contain a newline. The test therefore no longer verifies the claimed security boundary; render a collapsed/header-only view and check the complete output for LF, or otherwise assert the expected output-line structure before selecting the first line.

Useful? React with 👍 / 👎.

expect(statLine).not.toContain('https://evil.example'); // OSC payload stripped
// The benign text fragments must survive
expect(strip(out)).toContain('src/');
expect(strip(out)).toContain('evil');
Expand Down
9 changes: 6 additions & 3 deletions src/cli/render/compact-diff-view.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,10 +176,13 @@ export function compactDiffView(spec: CompactDiffSpec): string {
bodyLines.push(palette.dim(`... and ${hidden} more ${noun}`));
}

// Wrap body in a box. Width accounts for border + padding overhead (box adds
// 4 cols for borders and 2 cols for default padding = 6 total).
// Wrap body in a box. BOX_OVERHEAD matches the constant used in utils.ts
// (maxInnerBoxWidth): 2 border cols + 2×1 default padding col on each side = 6.
// Named here so a change to drawBox's padding default causes a single update
// rather than a silent numeric mismatch.
const BOX_OVERHEAD = 6; // 2 border + 2×padding(1) on each side — mirrors utils.ts
const termWidth = spec.width ?? 80;
const innerWidth = Math.max(20, termWidth - 6);
const innerWidth = Math.max(20, termWidth - BOX_OVERHEAD);
const boxed = drawBox(bodyLines, { width: innerWidth });

return [statHeader, boxed].join('\n');
Expand Down
Loading