fix(web-shell): sanitise goal approval text and edit paths (#16080)

* fix(web-shell): sanitise goal approval text and edit paths

Co-authored-by: Qwen-Coder <[email protected]>

* style(web-shell): let Prettier place the diff-path chain

The Lint & Static gate runs prettier --check and this test file failed it on a
member chain that wraps differently. Formatting only.

Not fixed here: neutralises controls in the edit diff path still fails on this
branch (expected '+1-1' to be the sanitised path), on the unformatted original as
well, so it predates this commit.

* test(web-shell): reach the diff path past DiffView's own wrapper

The case asserts that a crafted edit path has its zero-width joiner, ANSI escape
and BEL rendered as visible escapes. It walked from the File diff region up one
level and took the first child, which is DiffView's stats element - hence the
'+1-1' the assertion reported. The path sits one level higher, as the sibling of
DiffView's root, so the traversal needs one more parent.

Selector only; the assertion and the sanitisation it exercises are unchanged.

* fix(web-shell): tolerate a diff block without a path

DiffBlock.path is optional, but the new sanitise call passed it straight to
sanitizeControlChars, which takes a string. tsc -p tsconfig.lib.json rejected
it as TS2345 at ToolApproval.tsx:727, which is what took down Lint & Static,
Test (ubuntu), Integration Tests, web-shell E2E and Capture visuals on the same
head. Use the file's existing optional-string shape ("?? ''") so a path-less
block still renders an empty label, as before.

Co-authored-by: Qwen-Coder <[email protected]>
Patrol-Run: qwen-pr-closeout/jmvy7yxvq7f

---------

Co-authored-by: Qwen-Coder <[email protected]>
Co-authored-by: yiliang114 <[email protected]>

packages/web-shell/client/components/messages/GoalApprovalContent.tsx

@@ -1,6 +1,7 @@
 import { useState } from 'react';
 import { useI18n } from '../../i18n';
 import { Tabs, TabsContent, TabsList, TabsTrigger } from '../ui/tabs';
+import { sanitizeControlChars } from './toolFormatting';
 import styles from './ToolApproval.module.css';
 
 const sectionLabels: Record<string, string> = {
@@ -49,16 +50,23 @@ export function GoalApprovalContent({
   const [view, setView] = useState('overview');
   const sections = splitSections(objective);
   const fullText = content || objective;
+  const objectiveDisplay = sanitizeControlChars(objective);
+  const fullTextDisplay = sanitizeControlChars(fullText);
+  const sectionDisplays = sections.map((section) => ({
+    ...section,
+    text: sanitizeControlChars(section.text),
+  }));
   // Confirmation text can warn that an existing goal will be replaced. Keep
   // that notice in the overview too, rather than hiding it behind a tab.
   const trimmedObjective = objective.trim();
   const prompt = fullText.trimEnd();
-  const notice =
+  const noticeText =
     trimmedObjective && prompt.endsWith(trimmedObjective)
       ? prompt.slice(0, -trimmedObjective.length).trim()
       : fullText !== objective
         ? fullText
         : '';
+  const notice = sanitizeControlChars(noticeText);
 
   return (
     <Tabs
@@ -76,19 +84,19 @@ export function GoalApprovalContent({
       <div className={styles.goalBody} id={id}>
         <TabsContent value="overview" className={styles.goalPanel}>
           {objective && notice && <p className={styles.goalNotice}>{notice}</p>}
-          {sections.length ? (
-            sections.map((section, index) => (
+          {sectionDisplays.length ? (
+            sectionDisplays.map((section, index) => (
               <section key={index} className={styles.goalSection}>
                 <h3>{t(section.label)}</h3>
                 <p>{section.text}</p>
               </section>
             ))
           ) : (
-            <p>{objective || fullText}</p>
+            <p>{objective ? objectiveDisplay : fullTextDisplay}</p>
           )}
         </TabsContent>
         <TabsContent value="full" className={styles.goalPanel}>
-          <p>{fullText}</p>
+          <p>{fullTextDisplay}</p>
         </TabsContent>
       </div>
     </Tabs>

packages/web-shell/client/components/messages/ToolApproval.test.tsx

@@ -751,6 +751,80 @@ describe('ToolApproval accessibility', () => {
     expect(contentPre.getAttribute('title')).toBe(rendered);
   });
 
+  it('neutralises bidi and C0 controls in the goal approval body', () => {
+    const craftedObjective = [
+      'Outcome: Audit \u202eopen PRs.',
+      'Done when: Every PR has \u001b[31mevidence\u001b[0m\u0007.',
+      'Must not: Push or comment.',
+    ].join('\n');
+    render(false, {
+      ...goalRequest,
+      id: 'crafted-goal',
+      rawInput: { objective: craftedObjective },
+      content: [{ type: 'text', text: craftedObjective }],
+    });
+
+    const panel = container!.querySelector('[role="alertdialog"]')!;
+    const describedText = panel
+      .getAttribute('aria-describedby')!
+      .split(' ')
+      .map((id) => document.getElementById(id)?.textContent ?? '')
+      .join(' ');
+    expect(describedText).not.toContain('\u202e');
+    expect(describedText).not.toContain('\u001b');
+    expect(describedText).not.toContain('\u0007');
+    expect(describedText).toContain('Audit \\u202eopen PRs.');
+
+    switchGoalTab('full');
+    const fullText = container!.querySelector(
+      '[role="tabpanel"][data-state="active"]',
+    )!.textContent!;
+    expect(fullText).not.toContain('\u202e');
+    expect(fullText).not.toContain('\u001b');
+    expect(fullText).not.toContain('\u0007');
+    expect(fullText).toContain('Audit \\u202eopen PRs.');
+  });
+
+  it('neutralises controls in the goal replacement notice', () => {
+    const craftedNotice = 'Replace paused goal\u202e with this one?\u001b[31m';
+    render(false, {
+      ...goalRequest,
+      id: 'crafted-goal-notice',
+      rawInput: { objective: goalObjective },
+      content: [{ type: 'text', text: `${craftedNotice}\n\n${goalObjective}` }],
+    });
+
+    const notice = container!.querySelector('[class*="goalNotice"]')!;
+    expect(notice.textContent).toBe(
+      'Replace paused goal\\u202e with this one?\\u001b[31m',
+    );
+  });
+
+  it('neutralises controls in the edit diff path', () => {
+    const craftedPath = 'src/\u202eevil\u001b[31m\u0007.ts';
+    render(false, {
+      id: 'req-crafted-diff',
+      toolName: 'edit',
+      title: 'Edit',
+      content: [
+        {
+          type: 'diff',
+          path: craftedPath,
+          oldText: 'before',
+          newText: 'after',
+        },
+      ],
+      options: [
+        { id: 'proceed', label: 'Proceed', kind: 'allow_once' },
+        { id: 'reject', label: 'Reject', kind: 'reject_once' },
+      ],
+    });
+
+    const path = container!.querySelector('[aria-label="File diff"]')!
+      .parentElement!.parentElement!.firstElementChild!;
+    expect(path.textContent).toBe('src/\\u202eevil\\u001b[31m\\u0007.ts');
+  });
+
   it('renders exec warnings alongside the command block', () => {
     const adapted = extractPendingPermission([
       {

packages/web-shell/client/components/messages/ToolApproval.tsx

@@ -724,7 +724,7 @@ export function ToolApproval({
         <div className={styles.content} data-plan-interactive>
           {diffs.map((block, index) => (
             <div key={index}>
-              <div>{block.path}</div>
+              <div>{sanitizeControlChars(block.path ?? '')}</div>
               <DiffView
                 diff={block.diff}
                 fillScrollRegion={variant === 'floating'}