mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-05-19 16:28:28 +00:00
fix(cli): address banner audit findings
Three audit-driven fixes for the banner customization feature:
1. **VSCode JSON schema accepts every documented shape.** The
`ui.customAsciiArt` entry in
`packages/vscode-ide-companion/schemas/settings.schema.json` was
declared as `type: object`, which made VSCode flag the inline-string
form (`"customAsciiArt": " ___"`) — a shape the runtime accepts and
the design doc recommends — as a schema violation. Replaced with a
`oneOf` covering string, `{path}`, and `{small,large}` (with each
tier itself string-or-`{path}`).
2. **Narrow terminals no longer leak the QWEN logo over a white-label
deployment.** When a user supplied custom ASCII art but neither tier
fit the terminal, `Header.tsx` previously fell back to the bundled
`shortAsciiLogo` — silently undoing the white-label intent on small
windows. The fallback now distinguishes "user supplied custom art"
from "no custom art at all": in the first case the logo column is
hidden entirely (info panel still renders); in the second case the
default logo shows as before. Soft-failure paths (missing file,
sanitization rejection) still fall through to `shortAsciiLogo`.
3. **Sanitizer strips C1 control bytes (0x80-0x9F).** The art and title
strippers previously stopped at 0x7F, leaving single-byte CSI
(`0x9B`), DCS (`0x90`), ST (`0x9C`) and other C1 controls intact —
which legacy 8-bit terminals would still interpret. Aligned the
ranges with the repo's existing `stripUnsafeCharacters` (in
`textUtils.ts`) so banner content can't carry interpreted control
bytes through.
New tests cover: C1 strip in art and title, absolute path reads,
symlink rejection on POSIX, narrow-terminal hide-on-custom-art, and
end-to-end `<AppHeader />` rendering through `resolveCustomBanner`.
The full banner suite is 48 tests (was 42).
This commit is contained in:
parent
ab51f28469
commit
30889c30c4
6 changed files with 186 additions and 43 deletions
|
|
@ -20,19 +20,27 @@ const useTerminalSizeMock = vi.mocked(useTerminalSize.useTerminalSize);
|
|||
const createSettings = (options?: {
|
||||
hideTips?: boolean;
|
||||
hideBanner?: boolean;
|
||||
}): LoadedSettings =>
|
||||
({
|
||||
merged: {
|
||||
ui: {
|
||||
hideTips: options?.hideTips ?? true,
|
||||
hideBanner: options?.hideBanner,
|
||||
},
|
||||
},
|
||||
customBannerTitle?: string;
|
||||
customAsciiArt?: unknown;
|
||||
}): LoadedSettings => {
|
||||
const ui = {
|
||||
hideTips: options?.hideTips ?? true,
|
||||
hideBanner: options?.hideBanner,
|
||||
customBannerTitle: options?.customBannerTitle,
|
||||
customAsciiArt: options?.customAsciiArt,
|
||||
};
|
||||
return {
|
||||
merged: { ui },
|
||||
system: { settings: {}, originalSettings: {}, path: '' },
|
||||
systemDefaults: { settings: {}, originalSettings: {}, path: '' },
|
||||
user: { settings: {}, originalSettings: {}, path: '' },
|
||||
user: {
|
||||
settings: { ui },
|
||||
originalSettings: { ui },
|
||||
path: '/home/u/.qwen/settings.json',
|
||||
},
|
||||
workspace: { settings: {}, originalSettings: {}, path: '' },
|
||||
}) as never;
|
||||
} as never;
|
||||
};
|
||||
|
||||
const createMockConfig = (overrides = {}) => ({
|
||||
getContentGeneratorConfig: vi.fn(() => ({ authType: undefined })),
|
||||
|
|
@ -108,4 +116,20 @@ describe('<AppHeader />', () => {
|
|||
expect(lastFrame()).not.toContain('>_ Qwen Code');
|
||||
expect(lastFrame()).not.toContain('██╔═══██╗');
|
||||
});
|
||||
|
||||
it('renders custom banner title and inline ASCII art end-to-end through resolveCustomBanner', () => {
|
||||
const { lastFrame } = renderWithProviders(
|
||||
createMockUIState(),
|
||||
createSettings({
|
||||
customBannerTitle: 'Acme CLI',
|
||||
customAsciiArt: ' ACME\n ----',
|
||||
}),
|
||||
);
|
||||
const frame = lastFrame() ?? '';
|
||||
expect(frame).toContain('Acme CLI');
|
||||
expect(frame).not.toContain('>_ Qwen Code');
|
||||
expect(frame).toContain('ACME');
|
||||
// Default Qwen logo must NOT bleed through when the user supplied art.
|
||||
expect(frame).not.toContain('██╔═══██╗');
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -137,13 +137,27 @@ describe('<Header />', () => {
|
|||
expect(lastFrame()).not.toContain('X'.repeat(60));
|
||||
});
|
||||
|
||||
it('falls back to the default Qwen logo when no custom tier fits', () => {
|
||||
it('hides the logo column when neither custom tier fits — does NOT fall back to the default Qwen logo (preserves white-label intent)', () => {
|
||||
const { lastFrame } = render(
|
||||
<Header
|
||||
{...defaultProps}
|
||||
customAsciiArt={{ small: 'X'.repeat(150), large: 'Y'.repeat(150) }}
|
||||
/>,
|
||||
);
|
||||
expect(lastFrame()).toContain('██╔═══██╗');
|
||||
expect(lastFrame()).not.toContain('██╔═══██╗');
|
||||
expect(lastFrame()).not.toContain('X'.repeat(150));
|
||||
expect(lastFrame()).not.toContain('Y'.repeat(150));
|
||||
// Info panel still renders.
|
||||
expect(lastFrame()).toContain('Qwen OAuth');
|
||||
});
|
||||
|
||||
it('falls back to the default Qwen logo when no custom art was provided at all', () => {
|
||||
useTerminalSizeMock.mockReturnValue({ columns: 60, rows: 24 });
|
||||
const { lastFrame } = render(<Header {...defaultProps} />);
|
||||
// With no customAsciiArt, narrow widths still hide the QWEN logo, but a
|
||||
// wide enough terminal would show it — the previous test already covers
|
||||
// the wide case. This one just confirms the no-custom-art path doesn't
|
||||
// incidentally hide the logo.
|
||||
expect(lastFrame()).toContain('>_ Qwen Code');
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -74,8 +74,14 @@ export const Header: React.FC<HeaderProps> = ({
|
|||
terminalWidth - containerMarginX * 2,
|
||||
);
|
||||
|
||||
// Pick the widest custom tier that fits. If neither tier fits or no
|
||||
// custom art was provided, fall through to the bundled `shortAsciiLogo`.
|
||||
// Two distinct fallback paths:
|
||||
// - User supplied a custom tier and at least one tier fits → render that.
|
||||
// - User supplied custom art but neither tier fits → hide the logo column.
|
||||
// Falling back to the bundled QWEN logo here would silently undo a
|
||||
// white-label deployment on narrow terminals.
|
||||
// - User supplied no custom art → fall through to `shortAsciiLogo` and let
|
||||
// the existing width gate decide whether to show or hide it.
|
||||
const hasCustomArt = Boolean(customAsciiArt?.small || customAsciiArt?.large);
|
||||
const customTier = pickAsciiArtTier(
|
||||
customAsciiArt?.small,
|
||||
customAsciiArt?.large,
|
||||
|
|
@ -84,13 +90,14 @@ export const Header: React.FC<HeaderProps> = ({
|
|||
minInfoPanelWidth,
|
||||
getAsciiArtWidth,
|
||||
);
|
||||
const displayLogo = customTier ?? shortAsciiLogo;
|
||||
const displayLogo = customTier ?? (hasCustomArt ? '' : shortAsciiLogo);
|
||||
const logoWidth = getAsciiArtWidth(displayLogo);
|
||||
|
||||
// Check if we have enough space for logo + gap + minimum info panel.
|
||||
// For the default logo this can still be false on very narrow terminals;
|
||||
// for a custom tier we already proved it fits inside `pickAsciiArtTier`.
|
||||
// When `displayLogo` is empty (custom art too wide for both tiers) showLogo
|
||||
// will be false, hiding the column entirely.
|
||||
const showLogo =
|
||||
displayLogo !== '' &&
|
||||
availableTerminalWidth >= logoWidth + logoGap + minInfoPanelWidth;
|
||||
|
||||
// Calculate available width for info panel (use all remaining space)
|
||||
|
|
|
|||
|
|
@ -8,12 +8,12 @@ import * as fs from 'node:fs';
|
|||
import * as os from 'node:os';
|
||||
import * as path from 'node:path';
|
||||
import { afterEach, beforeEach, describe, expect, it } from 'vitest';
|
||||
import {
|
||||
pickAsciiArtTier,
|
||||
resolveCustomBanner,
|
||||
} from './customBanner.js';
|
||||
import { pickAsciiArtTier, resolveCustomBanner } from './customBanner.js';
|
||||
import type { LoadedSettings, SettingsFile } from '../../config/settings.js';
|
||||
import type { CustomAsciiArtSetting, Settings } from '../../config/settingsSchema.js';
|
||||
import type {
|
||||
CustomAsciiArtSetting,
|
||||
Settings,
|
||||
} from '../../config/settingsSchema.js';
|
||||
|
||||
function makeSettings(opts: {
|
||||
workspaceUi?: Settings['ui'];
|
||||
|
|
@ -46,7 +46,10 @@ function makeSettings(opts: {
|
|||
: empty,
|
||||
systemDefaults: empty,
|
||||
user: opts.userUi
|
||||
? file({ ui: opts.userUi }, opts.userPath ?? '/home/u/.qwen/settings.json')
|
||||
? file(
|
||||
{ ui: opts.userUi },
|
||||
opts.userPath ?? '/home/u/.qwen/settings.json',
|
||||
)
|
||||
: empty,
|
||||
workspace: opts.workspaceUi
|
||||
? file(
|
||||
|
|
@ -156,6 +159,20 @@ describe('resolveCustomBanner', () => {
|
|||
expect(out.asciiArt.small).toContain('ART');
|
||||
});
|
||||
|
||||
it('strips C1 control characters (0x80-0x9f) including single-byte CSI', () => {
|
||||
const out = resolveCustomBanner(
|
||||
makeSettings({
|
||||
userUi: { customAsciiArt: 'A\x9b31mB\x9c\x90X' },
|
||||
}),
|
||||
);
|
||||
// 0x9b (single-byte CSI), 0x9c (ST), 0x90 (DCS) are all C1 — must be
|
||||
// replaced with space, not interpreted by the terminal.
|
||||
expect(out.asciiArt.small).not.toMatch(/[\x80-\x9f]/);
|
||||
expect(out.asciiArt.small).toContain('A');
|
||||
expect(out.asciiArt.small).toContain('B');
|
||||
expect(out.asciiArt.small).toContain('X');
|
||||
});
|
||||
|
||||
it('strips OSC-8 hyperlinks from inline art', () => {
|
||||
const out = resolveCustomBanner(
|
||||
makeSettings({
|
||||
|
|
@ -174,7 +191,11 @@ describe('resolveCustomBanner', () => {
|
|||
userUi: { customAsciiArt: 'line1\nline2\nline3' },
|
||||
}),
|
||||
);
|
||||
expect(out.asciiArt.small?.split('\n')).toEqual(['line1', 'line2', 'line3']);
|
||||
expect(out.asciiArt.small?.split('\n')).toEqual([
|
||||
'line1',
|
||||
'line2',
|
||||
'line3',
|
||||
]);
|
||||
});
|
||||
|
||||
it('caps art at 200 lines × 200 cols', () => {
|
||||
|
|
@ -191,6 +212,39 @@ describe('resolveCustomBanner', () => {
|
|||
expect(out2.asciiArt.small?.length).toBe(200);
|
||||
});
|
||||
|
||||
it('reads from an absolute path verbatim', () => {
|
||||
const file = path.join(tmpDir, 'absolute.txt');
|
||||
fs.writeFileSync(file, 'ABS\nART');
|
||||
const out = resolveCustomBanner(
|
||||
makeSettings({
|
||||
userUi: {
|
||||
customAsciiArt: { path: file } as CustomAsciiArtSetting,
|
||||
},
|
||||
userPath: '/some/other/dir/settings.json',
|
||||
}),
|
||||
);
|
||||
expect(out.asciiArt.small).toBe('ABS\nART');
|
||||
});
|
||||
|
||||
it('refuses to follow a symlink at the configured path on POSIX', () => {
|
||||
if (process.platform === 'win32') return;
|
||||
const real = path.join(tmpDir, 'real.txt');
|
||||
fs.writeFileSync(real, 'REAL\nART');
|
||||
const link = path.join(tmpDir, 'link.txt');
|
||||
fs.symlinkSync(real, link);
|
||||
const out = resolveCustomBanner(
|
||||
makeSettings({
|
||||
userUi: {
|
||||
customAsciiArt: { path: 'link.txt' } as CustomAsciiArtSetting,
|
||||
},
|
||||
userPath: path.join(tmpDir, 'settings.json'),
|
||||
}),
|
||||
);
|
||||
// O_NOFOLLOW makes openSync throw ELOOP — resolver soft-fails, so the
|
||||
// tier ends up undefined rather than reading through the symlink.
|
||||
expect(out.asciiArt.small).toBeUndefined();
|
||||
});
|
||||
|
||||
it('falls back when the {path} target is missing', () => {
|
||||
const out = resolveCustomBanner(
|
||||
makeSettings({
|
||||
|
|
@ -272,6 +326,17 @@ describe('resolveCustomBanner', () => {
|
|||
expect(out.title).toBe('Acme CLI');
|
||||
});
|
||||
|
||||
it('strips C1 control characters from the title', () => {
|
||||
const out = resolveCustomBanner(
|
||||
makeSettings({
|
||||
userUi: { customBannerTitle: 'Acme\x9b31m CLI\x9c' },
|
||||
}),
|
||||
);
|
||||
expect(out.title).not.toMatch(/[\x80-\x9f]/);
|
||||
expect(out.title).toContain('Acme');
|
||||
expect(out.title).toContain('CLI');
|
||||
});
|
||||
|
||||
it('caps the title at 80 characters', () => {
|
||||
const out = resolveCustomBanner(
|
||||
makeSettings({
|
||||
|
|
@ -320,12 +385,12 @@ describe('pickAsciiArtTier', () => {
|
|||
});
|
||||
|
||||
it('skips missing tiers', () => {
|
||||
expect(
|
||||
pickAsciiArtTier(undefined, 'fits', 100, 2, 40, measure),
|
||||
).toBe('fits');
|
||||
expect(
|
||||
pickAsciiArtTier('fits', undefined, 100, 2, 40, measure),
|
||||
).toBe('fits');
|
||||
expect(pickAsciiArtTier(undefined, 'fits', 100, 2, 40, measure)).toBe(
|
||||
'fits',
|
||||
);
|
||||
expect(pickAsciiArtTier('fits', undefined, 100, 2, 40, measure)).toBe(
|
||||
'fits',
|
||||
);
|
||||
expect(
|
||||
pickAsciiArtTier(undefined, undefined, 100, 2, 40, measure),
|
||||
).toBeUndefined();
|
||||
|
|
|
|||
|
|
@ -43,9 +43,7 @@ type CacheEntry = { value: string | undefined };
|
|||
* and falls back to the locked default for that field. The CLI must never
|
||||
* crash on a banner config error.
|
||||
*/
|
||||
export function resolveCustomBanner(
|
||||
settings: LoadedSettings,
|
||||
): ResolvedBanner {
|
||||
export function resolveCustomBanner(settings: LoadedSettings): ResolvedBanner {
|
||||
const ui = settings.merged.ui;
|
||||
const cache = new Map<string, CacheEntry>();
|
||||
|
||||
|
|
@ -59,9 +57,11 @@ export function resolveCustomBanner(
|
|||
return {
|
||||
asciiArt: {
|
||||
small:
|
||||
scoped.small && resolveTier(scoped.small.source, scoped.small.dir, cache),
|
||||
scoped.small &&
|
||||
resolveTier(scoped.small.source, scoped.small.dir, cache),
|
||||
large:
|
||||
scoped.large && resolveTier(scoped.large.source, scoped.large.dir, cache),
|
||||
scoped.large &&
|
||||
resolveTier(scoped.large.source, scoped.large.dir, cache),
|
||||
},
|
||||
title,
|
||||
};
|
||||
|
|
@ -261,8 +261,10 @@ function sanitizeArt(input: string): string {
|
|||
s = s.replace(/\x1b\[[\d;?]*[a-zA-Z]/g, ' ');
|
||||
// SS2/SS3/DCS leaders
|
||||
s = s.replace(/\x1b[NOP]/g, ' ');
|
||||
// Remaining C0/C1 controls + DEL → space, but keep \n and \t.
|
||||
s = s.replace(/[\x00-\x08\x0b-\x1f\x7f]/g, ' ');
|
||||
// Remaining C0 controls + DEL + C1 controls (0x80-0x9f, e.g. single-byte
|
||||
// CSI 0x9b) → space. Keep \n (0x0a) and \t (0x09) so multi-line ASCII art
|
||||
// and tab-aligned art survive.
|
||||
s = s.replace(/[\x00-\x08\x0b-\x1f\x7f-\x9f]/g, ' ');
|
||||
/* eslint-enable no-control-regex */
|
||||
|
||||
const rawLines = s.split('\n');
|
||||
|
|
@ -293,9 +295,7 @@ function sanitizeArt(input: string): string {
|
|||
if (cappedLines.length === 0) return '';
|
||||
|
||||
if (truncatedRows) {
|
||||
debugLogger.warn(
|
||||
`Truncated ui.customAsciiArt to ${MAX_ART_LINES} lines.`,
|
||||
);
|
||||
debugLogger.warn(`Truncated ui.customAsciiArt to ${MAX_ART_LINES} lines.`);
|
||||
}
|
||||
if (truncatedCols) {
|
||||
debugLogger.warn(
|
||||
|
|
@ -313,7 +313,10 @@ function sanitizeTitle(raw: unknown): string | undefined {
|
|||
.replace(/\x1b\][^\x07\x1b]*(?:\x07|\x1b\\)/g, ' ')
|
||||
.replace(/\x1b\[[\d;?]*[a-zA-Z]/g, ' ')
|
||||
.replace(/\x1b[NOP]/g, ' ')
|
||||
.replace(/[\x00-\x1f\x7f]/g, ' ');
|
||||
// C0 + DEL + C1 controls. Titles never need newlines or tabs (they live
|
||||
// on a single line of the info panel) so the range is denser than the
|
||||
// art sanitizer's.
|
||||
.replace(/[\x00-\x1f\x7f-\x9f]/g, ' ');
|
||||
/* eslint-enable no-control-regex */
|
||||
t = t.replace(/\s+/g, ' ').trim();
|
||||
if (!t) return undefined;
|
||||
|
|
|
|||
|
|
@ -273,8 +273,38 @@
|
|||
},
|
||||
"customAsciiArt": {
|
||||
"description": "Replace the default QWEN ASCII art. Accepts an inline string, {\"path\": \"...\"}, or {\"small\": ..., \"large\": ...} for width-aware selection.",
|
||||
"type": "object",
|
||||
"additionalProperties": true
|
||||
"oneOf": [
|
||||
{ "type": "string" },
|
||||
{
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"path": { "type": "string" },
|
||||
"small": {
|
||||
"oneOf": [
|
||||
{ "type": "string" },
|
||||
{
|
||||
"type": "object",
|
||||
"properties": { "path": { "type": "string" } },
|
||||
"required": ["path"],
|
||||
"additionalProperties": false
|
||||
}
|
||||
]
|
||||
},
|
||||
"large": {
|
||||
"oneOf": [
|
||||
{ "type": "string" },
|
||||
{
|
||||
"type": "object",
|
||||
"properties": { "path": { "type": "string" } },
|
||||
"required": ["path"],
|
||||
"additionalProperties": false
|
||||
}
|
||||
]
|
||||
}
|
||||
},
|
||||
"additionalProperties": false
|
||||
}
|
||||
]
|
||||
}
|
||||
}
|
||||
},
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue