From ac3a17ebf36a66f5f950446405b9e23ff7ab68f9 Mon Sep 17 00:00:00 2001
From: fernando_jacob <f.jacob1996@gmail.com>
Date: Thu, 21 May 2026 17:47:08 +0800
Subject: [PATCH] fix(browser): scope evaluateWithArgs declarations in an IIFE
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`browser upload` would throw `SyntaxError: Identifier 'markerAttr' has
already been declared` on the second invocation against the same page —
even after a navigation.

Root cause: `BasePage.evaluateWithArgs` prepended raw `const <key> = ...`
statements to the caller-supplied expression and sent the whole script
to `Runtime.evaluate`. Because the upload command always passes a fixed
key set (`markerAttr`, `markerValue`), and the bridge reuses the same
execution context, the second call hit a duplicate top-level `const`.

Wrap the declarations and the caller expression in an IIFE so the
bindings are block-scoped. `wrapForEval` already detects this shape as
already-IIFE and forwards it unchanged. The body is parenthesized and
returned, preserving Promise return values (CDP awaits via
`awaitPromise: true`).

Added a regression test that asserts the emitted script is IIFE-wrapped
with the declarations inside.
---
 src/browser/base-page.test.ts | 37 +++++++++++++++++++++++++++++++++++
 src/browser/base-page.ts      |  8 +++++++-
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/src/browser/base-page.test.ts b/src/browser/base-page.test.ts
index 10ae9cb96..78a949621 100644
--- a/src/browser/base-page.test.ts
+++ b/src/browser/base-page.test.ts
@@ -122,6 +122,43 @@ describe('BasePage.fetchJson', () => {
   });
 });
 
+describe('BasePage.evaluateWithArgs', () => {
+  class RealEvalPage extends BasePage {
+    scripts: string[] = [];
+    async goto(): Promise<void> {}
+    async evaluate<T = unknown>(_js: string): Promise<T>;
+    async evaluate<Args extends unknown[], T>(_fn: BrowserEvaluateFunction<Args, T>, ..._args: Args): Promise<Awaited<T>>;
+    async evaluate(input: string | BrowserEvaluateFunction<unknown[], unknown>): Promise<unknown> {
+      this.scripts.push(typeof input === 'string' ? input : input.toString());
+      return null;
+    }
+    async getCookies(): Promise<[]> { return []; }
+    async screenshot(): Promise<string> { return ''; }
+    async tabs(): Promise<unknown[]> { return []; }
+    async selectTab(): Promise<void> {}
+  }
+
+  it('wraps declarations and body in an IIFE so const bindings are block-scoped', async () => {
+    const page = new RealEvalPage();
+    await page.evaluateWithArgs('(() => ({ ok: true }))()', { markerAttr: 'data-x', markerValue: 'v1' });
+    expect(page.scripts).toHaveLength(1);
+    const script = page.scripts[0];
+    // Declarations must live inside an IIFE so a second call into the same
+    // Runtime.evaluate context does not throw "Identifier 'markerAttr' has
+    // already been declared".
+    expect(script.startsWith('(() => {')).toBe(true);
+    expect(script.endsWith('})()')).toBe(true);
+    expect(script).toContain('const markerAttr = "data-x";');
+    expect(script).toContain('const markerValue = "v1";');
+    expect(script).toContain('return ((() => ({ ok: true }))());');
+  });
+
+  it('rejects keys that are not valid JS identifiers', async () => {
+    const page = new RealEvalPage();
+    await expect(page.evaluateWithArgs('(() => 1)()', { 'bad-key': 1 })).rejects.toThrow(/invalid key/);
+  });
+});
+
 describe('BasePage annotatedScreenshot', () => {
   it('refreshes DOM refs, captures with a temporary visual overlay, and cleans up', async () => {
     const page = new ActionPage();
diff --git a/src/browser/base-page.ts b/src/browser/base-page.ts
index 22a326bd2..d21b81cda 100644
--- a/src/browser/base-page.ts
+++ b/src/browser/base-page.ts
@@ -154,6 +154,12 @@ export abstract class BasePage implements IPage {
    * Each key in `args` becomes a `const` declaration with JSON-serialized value,
    * prepended to the JS code. Prevents injection by design.
    *
+   * The declarations and the caller-provided expression are wrapped in an IIFE
+   * so the `const` bindings are block-scoped. Without the wrapper, the bindings
+   * land in the script's top-level realm and the second call into the same
+   * Runtime.evaluate context throws `SyntaxError: Identifier '<key>' has already
+   * been declared` (the `markerAttr` regression hit by `browser upload`).
+   *
    * Usage:
    *   page.evaluateWithArgs(`(async () => { return sym; })()`, { sym: userInput })
    */
@@ -166,7 +172,7 @@ export abstract class BasePage implements IPage {
         return `const ${key} = ${JSON.stringify(value)};`;
       })
       .join('\n');
-    return this.evaluate(`${declarations}\n${js}`);
+    return this.evaluate(`(() => { ${declarations}\nreturn (${js}); })()`);
   }
 
   async fetchJson(url: string, opts: FetchJsonOptions = {}): Promise<unknown> {