Skip to content

Fixed Chrome Tab Leak, Wrong Error Fields & Task Mutation #307

Open
0xreacher wants to merge 2 commits intoedoardottt:mainfrom
0xreacher:patch-1
Open

Fixed Chrome Tab Leak, Wrong Error Fields & Task Mutation #307
0xreacher wants to merge 2 commits intoedoardottt:mainfrom
0xreacher:patch-1

Conversation

@0xreacher
Copy link
Copy Markdown

Bug Fix - Dropped tab cancel
The original ctx, _ = chromedp.NewContext(ctx) was silently dropping the cancel function for the new Chrome tab context. That tab would never be explicitly closed, leaking a browser tab on every scan call. Fixed by capturing it as tabCancel and deferring it immediately.

Bug Fix - Wrong error field for ExploitError
resultData.ExploitError = errDetection.Error() was copying the detection phase error into the exploit error field. So if detection succeeded but exploit failed, you'd get zero error surfaced, or worse, a stale detection error. Fixed to errExploit.Error() where it belongs.

Bug Fix - ecancel not called on fatal browser startup failure
In GetChromeBrowser, if chromedp.Run errored, the code called gologger.Fatal() without first calling ecancel(). On non-fatal builds or deferred cleanup paths this leaks the exec allocator. Added ecancel() before the fatal log.

Bug Fix - chromedpTasksScan mutation causing double navigation
The original code appended the fingerprint eval directly onto chromedpTasksScan, which already contained Navigate and the initial JS eval. Running it again would re-navigate the page and re-run the pollution check before fingerprinting, wasteful and potentially incorrect if the page has state. Fingerprinting now runs as its own isolated fingerprintTasks.

Upgrade - buildHeaders helper extracted
The if headers != nil { append... } pattern was duplicated for both the scan phase and the exploit phase. Pulled into a small buildHeaders(headers) function that returns nil-safe tasks. Cleaner and means both phases are guaranteed to handle headers the same way.

Upgrade - Early return instead of deeply nested if
The original exploit block was nested inside if r.Options.Exploit && errScan == nil and then again inside if resTrimmed != "". Restructured to an early return guard at the top of the exploit block - same logic, much flatter and easier to follow when reading the control flow.

Upgrade - Consistent error logging
Before, detection errors only logged under Verbose, but exploit errors logged unconditionally only when !Verbose the exact opposite of each other with no good reason. Now both errors always log via gologger.Error() regardless of verbose mode, since errors are not verbosity-dependent information.

Upgrade - resTrimmed eliminated, reuses JSEvaluation
strings.TrimSpace(resScan) was called twice once to set resultData.JSEvaluation, and again into a local resTrimmed just to check emptiness. The guard now reads directly from resultData.JSEvaluation which already holds the trimmed value.

Refactor Scan function and related methods for improved readability and structure. Introduce buildHeaders function to handle HTTP headers.
@auto-assign auto-assign Bot requested a review from edoardottt April 30, 2026 05:46
@edoardottt edoardottt self-assigned this Apr 30, 2026
Copy link
Copy Markdown
Owner

@edoardottt edoardottt left a comment

Choose a reason for hiding this comment

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

thanks @0xreacher for the PR!
Requesting some changes

Comment thread pkg/scan/chrome.go
Comment thread pkg/scan/chrome.go
Comment thread pkg/scan/chrome.go
Comment thread pkg/scan/chrome.go
Comment thread pkg/scan/chrome.go
Copy link
Copy Markdown
Author

@0xreacher 0xreacher left a comment

Choose a reason for hiding this comment

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

yep comments added now

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