Skip to content

feat: record popup window interactions with correct statement ordering#308

Open
cx-pedro-nascimento wants to merge 2 commits intozaproxy:mainfrom
cx-pedro-nascimento:feat/popup-window-handle
Open

feat: record popup window interactions with correct statement ordering#308
cx-pedro-nascimento wants to merge 2 commits intozaproxy:mainfrom
cx-pedro-nascimento:feat/popup-window-handle

Conversation

@cx-pedro-nascimento
Copy link
Copy Markdown

Detect popup windows via window.opener in the content script and register them with the background service worker (ZAP_REGISTER_POPUP). The background assigns a new windowHandle, emits ZestActionSleep(10000) before ZestClientWindowHandle so ZAP waits for the popup to load before trying to locate it. All recorder statements in popup windows use the assigned handle.

Adds ZestStatementWindowHandle and ZestStatementActionSleep types, bumps version to 0.1.9, adds the tabs permission to the recorder manifest, and includes unit tests verifying the sleep-before-window-handle ordering.

@psiinon
Copy link
Copy Markdown
Member

psiinon commented Mar 23, 2026

Logo
Checkmarx One – Scan Summary & Details291b76bc-9bc7-4039-8397-f108a18d9c14


New Issues (2) Checkmarx found the following issues in this Pull Request
# Severity Issue Source File / Package Checkmarx Insight
1 HIGH CVE-2026-32141 Npm-flatted-3.3.2
detailsRecommended version: 3.4.2
Description: flatted is a circular JSON parser. Prior to 3.4.0, flatted's "parse()" function uses a recursive "revive()" phase to resolve circular references in...
Attack Vector: NETWORK
Attack Complexity: LOW
Vulnerable Package
2 HIGH CVE-2026-33228 Npm-flatted-3.3.2
detailsRecommended version: 3.4.2
Description: The "parse()" function in flatted can use attacker-controlled string values from the parsed JSON as direct array index keys, without validating t...
Attack Vector: NETWORK
Attack Complexity: LOW
Vulnerable Package

Use @Checkmarx to interact with Checkmarx PR Assistant.
Examples:
@Checkmarx how are you able to help me?
@Checkmarx rescan this PR

Detect popup windows via window.opener in the content script and register
them with the background service worker (ZAP_REGISTER_POPUP). The background
assigns a new windowHandle, emits ZestActionSleep(10000) **before**
ZestClientWindowHandle so ZAP waits for the popup to load before trying to
locate it. All recorder statements in popup windows use the assigned handle.

Adds ZestStatementWindowHandle and ZestStatementActionSleep types, bumps
version to 0.1.9, adds the tabs permission to the recorder manifest, and
includes unit tests verifying the sleep-before-window-handle ordering.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: CxPedroNascimento <174706762+cx-pedro-nascimento@users.noreply.github.com>
@psiinon
Copy link
Copy Markdown
Member

psiinon commented Mar 24, 2026

Review from Claude:


PR Review: feat: record popup window interactions with correct statement ordering

Summary: This PR detects window.opener !== null in the content script to identify popups, registers them with the background worker (ZAP_REGISTER_POPUP), assigns
a new windowHandle, and emits ZestActionSleep(10000) + ZestClientWindowHandle before any popup interactions so ZAP can locate the window first. Tab cleanup emits
ZestClientWindowClose.


Issues

  1. ZAP_GET_WINDOW_HANDLE is dead code

source/utils/constants.ts exports ZAP_GET_WINDOW_HANDLE and source/Background/index.ts handles it, but nothing in the codebase ever sends this message. The
handler at Background/index.ts:278 is unreachable code.

  1. tabs.onRemoved sends to ZAP unconditionally, ignoring IS_FULL_EXTENSION

Background/index.ts:369-383:
Browser.tabs.onRemoved.addListener(async (tabId) => {
...
sendZestScriptToZAP(data, items.zapkey as string, items.zapurl as string);
});
Every other call to sendZestScriptToZAP is guarded by if (IS_FULL_EXTENSION). This listener will fire HTTP requests to ZAP in recorder-only mode too.

  1. Hardcoded 10-second sleep is fragile

new ZestStatementActionSleep(10000) is a magic number. Fast popups will block replay unnecessarily; slow popups (or those over a slow network) could still fail.
Consider making this configurable or using a smarter wait strategy on the ZAP side.

  1. message.windowHandle in ZAP_START_RECORDING handler is dead code

ContentScript/index.ts:358-360:
if (message.windowHandle) {
recorder.setWindowHandle(message.windowHandle);
}
Nothing currently sends ZAP_START_RECORDING with a windowHandle field. This should either be wired up or removed.

  1. Type unsafe double-cast

Background/index.ts:296:
const popupUrl = (msg as unknown as {url?: string}).url ?? '';
The ZAP_REGISTER_POPUP message has a known shape — define a typed interface instead:
interface RegisterPopupMessage { type: typeof ZAP_REGISTER_POPUP; url?: string; }

  1. Tests don't cover the background handler logic

The new tests validate ZestStatementWindowHandle/ZestStatementActionSleep serialization and that ZestScript.addStatement preserves order — but there are no tests
for the ZAP_REGISTER_POPUP message handler itself (the part that actually assigns handles and emits the sleep+windowHandle pair). The most important behavior —
that a second ZAP_REGISTER_POPUP from the same tab returns the cached handle — is untested.


Minor notes

  • windowHandleCounter and popupTabHandles are reset in both RESET_ZEST_SCRIPT and STOP_RECORDING — correct, but it's worth noting that if multiple concurrent
    recordings are ever supported, this shared state will need revisiting.
  • The regex escaping of the popup origin (replace(/[.+?^${}()|[]\]/g, '\$&')) is correct for the characters it covers, but the URL passed to
    ZestStatementWindowHandle uses raw .
    at the end, which means the regex flag on the statement must be true. The code does set regex: true, so this is fine — just
    coupled.

Summary

The core idea is sound and the ordering fix (sleep before windowHandle) is correct. The main actionable issues are: remove ZAP_GET_WINDOW_HANDLE dead code, guard
tabs.onRemoved with IS_FULL_EXTENSION, fix or remove the dead message.windowHandle path in the start-recording handler, replace the double-cast with a typed
interface, and add a test for the ZAP_REGISTER_POPUP handler.

@psiinon
Copy link
Copy Markdown
Member

psiinon commented Mar 24, 2026

Review from me 😁
I think the Claude review looks reasonable.
The code is failing the lint check, so that def needs to be addressed.
I would like to see tests to make sure new windows are handled correctly, so a test HTML page which launches a popup.
I also dont like the hard coded 10 second delay.
To prevent it having to wait so long we could add minWaitFor support in ZestClientWindowHandle which should be a relatively small Zest change.

- Remove dead ZAP_GET_WINDOW_HANDLE constant and its unreachable handler
- Add IS_FULL_EXTENSION guard to tabs.onRemoved so recorder variant
  does not make ZAP API calls when a popup tab closes
- Extract magic 10000 into POPUP_WINDOW_SLEEP_MS named constant
- Replace type-unsafe double cast with typed RegisterPopupMessage interface
- Remove non-null assertion; use explicit undefined check instead
- Remove unreachable message.windowHandle branch in ZAP_START_RECORDING handler
- Fix long lines in recorder.ts to satisfy prettier line-length rule
- Fix prefer-destructuring lint error in content script unit test
- Add unit tests for ZAP_REGISTER_POPUP handler covering: new tab
  assignment, same-tab caching, multi-tab handles, missing tab id,
  and invalid URL fallback
@cx-pedro-nascimento
Copy link
Copy Markdown
Author

Hi Simon, thanks for the review! Pushed a fix addressing the feedback:

  • Removed dead ZAP_GET_WINDOW_HANDLE constant and its unreachable handler
  • Added IS_FULL_EXTENSION guard to tabs.onRemoved — the recorder variant was incorrectly calling the ZAP API on popup close
  • Extracted the hardcoded 10000 into a POPUP_WINDOW_SLEEP_MS constant — agree the right long-term fix is minWaitFor support in ZestClientWindowHandle, happy to look at that as a follow-up Zest change
  • Replaced the type-unsafe double cast with a typed RegisterPopupMessage interface
  • Removed non-null assertion and the unreachable message.windowHandle branch in the ZAP_START_RECORDING handler
  • Fixed the lint failures
  • Added unit tests for the ZAP_REGISTER_POPUP handler covering handle assignment, same-tab caching, multi-tab handles, missing tab id, and invalid URL fallback

The one thing still outstanding is the integration test HTML page that launches a popup — I will work on that next.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants