Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
21 changes: 14 additions & 7 deletions lib/renderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -324,13 +324,20 @@ export class CanvasRenderer {
this.renderLine(line, cursor.y, dims.cols);
}
}
if (cursorMoved && this.lastCursorPosition.y !== cursor.y) {
// Also redraw old cursor line if cursor moved to different line
if (!forceAll && !buffer.isRowDirty(this.lastCursorPosition.y)) {
const line = buffer.getLine(this.lastCursorPosition.y);
if (line) {
this.renderLine(line, this.lastCursorPosition.y, dims.cols);
}
if (cursorMoved && !forceAll) {
// Always redraw the OLD cursor row to erase the previous cursor
// glyph, whether or not the row is dirty and whether or not it
// differs from the new cursor row (issue #122: ghost cursor
// persisted at the initial (0,0) position because the prior
// logic skipped the redraw when the row was already dirty —
// assuming the regular dirty pass would handle it — but the
// regular dirty pass only runs when buffer cells changed, not
// when the cursor moved across unchanged cells. A double redraw
// when the row is both dirty AND cursor-moved is a trivial perf
// cost compared to the visual correctness gain.).
const line = buffer.getLine(this.lastCursorPosition.y);
if (line) {
this.renderLine(line, this.lastCursorPosition.y, dims.cols);
}
}
Comment on lines +327 to 342
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The current logic results in redundant rendering when the cursor moves horizontally on the same row. In this case, cursor.y and this.lastCursorPosition.y are identical. The preceding block (lines 309-315) already ensures the row is rendered if it is not dirty. By removing the row inequality check, this row is now rendered twice in a single frame. Adding a check to ensure the rows are different would be more efficient.

      if (cursorMoved && !forceAll && this.lastCursorPosition.y !== cursor.y) {
        // Redraw the OLD cursor row to erase the previous cursor glyph
        // when the cursor has moved to a different row. If the cursor
        // moved horizontally on the same row, that row is already
        // handled by the redraw logic above or the regular dirty pass.
        const line = buffer.getLine(this.lastCursorPosition.y);
        if (line) {
          this.renderLine(line, this.lastCursorPosition.y, dims.cols);
        }
      }

}
Expand Down
98 changes: 98 additions & 0 deletions lib/terminal.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3090,3 +3090,101 @@
term.dispose();
});
});

describe('ESC k title sequence (issue #153)', () => {
let container: HTMLElement | null = null;

beforeEach(() => {
if (typeof document !== 'undefined') {
container = document.createElement('div');
document.body.appendChild(container);
}
});

afterEach(() => {
if (container && container.parentNode) {

Check warning on line 3105 in lib/terminal.test.ts

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Prefer using an optional chain expression instead, as it's more concise and easier to read.

See more on https://sonarcloud.io/project/issues?id=diegosouzapw_ghostty-web&issues=AZ5Vxh0veDPdaD0YZJlz&open=AZ5Vxh0veDPdaD0YZJlz&pullRequest=16
container.parentNode.removeChild(container);

Check warning on line 3106 in lib/terminal.test.ts

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Prefer `childNode.remove()` over `parentNode.removeChild(childNode)`.

See more on https://sonarcloud.io/project/issues?id=diegosouzapw_ghostty-web&issues=AZ5Vxh0veDPdaD0YZJl0&open=AZ5Vxh0veDPdaD0YZJl0&pullRequest=16
container = null;
}
});

test('ESC k <text> ESC \\ does not leak the title payload onto the grid', async () => {

Check warning on line 3111 in lib/terminal.test.ts

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

`String.raw` should be used to avoid escaping `\`.

See more on https://sonarcloud.io/project/issues?id=diegosouzapw_ghostty-web&issues=AZ5Vxh0veDPdaD0YZJl1&open=AZ5Vxh0veDPdaD0YZJl1&pullRequest=16
if (!container) return;

const term = await createIsolatedTerminal({ cols: 80, rows: 24 });
term.open(container);

// GNU screen / tmux title-set: ESC k /tmp ESC \ then ESC k ls ESC \
// then the actual visible content. Before the strip pass landed,
// /tmp leaked onto row 0 and "ls" merged with the next line.
term.write('\x1bk/tmp\x1b\\\x1bkls\x1b\\demo.txt\r\n');

const line0 = term.wasmTerm!.getLine(0);
const text0 = line0
.map((c) => (c.codepoint ? String.fromCodePoint(c.codepoint) : ''))
.join('')
.trimEnd();
expect(text0).toBe('demo.txt');
expect(text0).not.toContain('/tmp');
expect(text0).not.toContain('ls');

term.dispose();
});

test('ESC k variant terminated by BEL is also stripped', async () => {
if (!container) return;

const term = await createIsolatedTerminal({ cols: 80, rows: 24 });
term.open(container);

term.write('\x1bktitle\x07after\r\n');

const line0 = term.wasmTerm!.getLine(0);
const text0 = line0
.map((c) => (c.codepoint ? String.fromCodePoint(c.codepoint) : ''))
.join('')
.trimEnd();
expect(text0).toBe('after');

term.dispose();
});

test('OSC 0 title-set continues to be consumed by the WASM parser', async () => {
if (!container) return;

const term = await createIsolatedTerminal({ cols: 80, rows: 24 });
term.open(container);

// OSC 0 ; <title> BEL — handled by WASM. The strip pass should not
// touch this sequence.
term.write('\x1b]0;mywindow\x07visible\r\n');

const line0 = term.wasmTerm!.getLine(0);
const text0 = line0
.map((c) => (c.codepoint ? String.fromCodePoint(c.codepoint) : ''))
.join('')
.trimEnd();
expect(text0).toBe('visible');

term.dispose();
});

test('Uint8Array input is stripped equivalently to string input', async () => {
if (!container) return;

const term = await createIsolatedTerminal({ cols: 80, rows: 24 });
term.open(container);

const bytes = new TextEncoder().encode('\x1bktitle\x1b\\done\r\n');
term.write(bytes);

const line0 = term.wasmTerm!.getLine(0);
const text0 = line0
.map((c) => (c.codepoint ? String.fromCodePoint(c.codepoint) : ''))
.join('')
.trimEnd();
expect(text0).toBe('done');

term.dispose();
});
});
61 changes: 60 additions & 1 deletion lib/terminal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -553,6 +553,61 @@
this.writeInternal(data, callback);
}

/**
* Strip unimplemented escape sequences that Ghostty WASM does not consume
* cleanly. The current parser (5714ed07) prints the inner text of
* `ESC k <text> ESC \` (screen/tmux title set) onto the grid instead of
* silently consuming it — the same is true for any 7-bit terminator
* (`BEL`). We pre-filter the input so those titles don't leak as visible
* text. Issue: coder/ghostty-web#153.
*
* Only `ESC k …` is stripped. OSC sequences (`ESC ] …`) already work in
* the WASM parser and are untouched.
*/
private stripUnimplementedTitleSequences(data: string | Uint8Array): string | Uint8Array {

Check failure on line 567 in lib/terminal.ts

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this function to reduce its Cognitive Complexity from 23 to the 15 allowed.

See more on https://sonarcloud.io/project/issues?id=diegosouzapw_ghostty-web&issues=AZ5VxhyceDPdaD0YZJlt&open=AZ5VxhyceDPdaD0YZJlt&pullRequest=16
if (typeof data === 'string') {
// ESC = \x1b, ST = \x1b\x5c (ESC followed by backslash), BEL = \x07
return data.replace(/\x1bk[^\x1b\x07]*(?:\x1b\\|\x07)/g, '');

Check warning on line 570 in lib/terminal.ts

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove this control character.

See more on https://sonarcloud.io/project/issues?id=diegosouzapw_ghostty-web&issues=AZ5VxhyceDPdaD0YZJlv&open=AZ5VxhyceDPdaD0YZJlv&pullRequest=16

Check warning on line 570 in lib/terminal.ts

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove this control character.

See more on https://sonarcloud.io/project/issues?id=diegosouzapw_ghostty-web&issues=AZ5VxhyceDPdaD0YZJlx&open=AZ5VxhyceDPdaD0YZJlx&pullRequest=16

Check warning on line 570 in lib/terminal.ts

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove this control character.

See more on https://sonarcloud.io/project/issues?id=diegosouzapw_ghostty-web&issues=AZ5VxhyceDPdaD0YZJlw&open=AZ5VxhyceDPdaD0YZJlw&pullRequest=16

Check warning on line 570 in lib/terminal.ts

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove this control character.

See more on https://sonarcloud.io/project/issues?id=diegosouzapw_ghostty-web&issues=AZ5VxhyceDPdaD0YZJlu&open=AZ5VxhyceDPdaD0YZJlu&pullRequest=16

Check warning on line 570 in lib/terminal.ts

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove this control character.

See more on https://sonarcloud.io/project/issues?id=diegosouzapw_ghostty-web&issues=AZ5VxhyceDPdaD0YZJly&open=AZ5VxhyceDPdaD0YZJly&pullRequest=16
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The regex used for string input ([^\x1b\x07]*) will stop at any ESC character, even if it is not followed by a backslash (the ST terminator). In contrast, the Uint8Array implementation (lines 577-589) correctly continues scanning until it finds a valid ST (ESC \) or BEL. This inconsistency means that certain payloads containing internal escape sequences will be handled differently depending on the input type.

Suggested change
return data.replace(/\x1bk[^\x1b\x07]*(?:\x1b\\|\x07)/g, '');
return data.replace(/\x1bk.*?(?:\x1b\\|\x07)/gs, '');

}
// Byte-level scan for Uint8Array. We only allocate a copy when we
// actually find a sequence to strip.
let i = 0;
let writeIdx = -1;
let out: Uint8Array | null = null;
while (i < data.length) {
if (data[i] === 0x1b && i + 1 < data.length && data[i + 1] === 0x6b) {
// Found ESC k — scan forward to ESC \ or BEL
let j = i + 2;
while (j < data.length) {
if (data[j] === 0x07) {
j++;
break;
}
if (data[j] === 0x1b && j + 1 < data.length && data[j + 1] === 0x5c) {
j += 2;
break;
}
Comment on lines +586 to +589
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

This check for the ST terminator (ESC \) will fail if the sequence is split across chunks such that the ESC is at the end of the current Uint8Array. In this case, the ESC will be incorrectly treated as part of the payload and stripped, and the subsequent \ in the next chunk will be passed to the terminal as literal text, potentially corrupting the output stream.

// No terminator yet — keep scanning (handles split writes if WASM
// ever assembles them; defensively bail if we hit another ESC k).
j++;
}
if (out === null) {
out = new Uint8Array(data.length);
out.set(data.subarray(0, i));
writeIdx = i;
}
i = j;
continue;
}
if (out !== null) {
out[writeIdx++] = data[i];
}
i++;
}
if (out === null) return data;
return out.subarray(0, writeIdx);
}
Comment on lines +567 to +609
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The stripUnimplementedTitleSequences method is stateless and does not handle escape sequences split across multiple write() calls. If a chunk ends mid-sequence, the remainder of the payload will be passed to the WASM terminal as plain text in the next call, leading to the "leak" this PR intends to fix. For a robust solution, this filtering should be integrated into a stateful parser or the WASM parser itself should be updated to handle these sequences.


/**
* Internal write implementation (extracted from write())
*/
Expand All @@ -561,6 +616,10 @@
// preserve selection when new data arrives. Selection is cleared by user actions
// like clicking or typing, not by incoming data.

// Strip unimplemented escape sequences (e.g. ESC k …) that would
// otherwise leak their payload onto the grid. See issue #153.
const sanitized = this.stripUnimplementedTitleSequences(data);

// Save scroll state before writing, ONLY when preserveScrollOnWrite is
// active. viewportY is relative to the bottom, so if new lines push
// content into scrollback we need to bump viewportY by the same amount
Expand All @@ -571,7 +630,7 @@
preserveScroll && savedViewportY > 0 ? this.wasmTerm!.getScrollbackLength() : 0;

// Write directly to WASM terminal (handles VT parsing internally)
this.wasmTerm!.write(data);
this.wasmTerm!.write(sanitized);

// Process any responses generated by the terminal (e.g., DSR cursor position)
// These need to be sent back to the PTY via onData
Expand Down