Fixed Chrome Tab Leak, Wrong Error Fields & Task Mutation #307
Open
0xreacher wants to merge 2 commits intoedoardottt:mainfrom
Open
Fixed Chrome Tab Leak, Wrong Error Fields & Task Mutation #3070xreacher wants to merge 2 commits intoedoardottt:mainfrom
0xreacher wants to merge 2 commits intoedoardottt:mainfrom
Conversation
Refactor Scan function and related methods for improved readability and structure. Introduce buildHeaders function to handle HTTP headers.
edoardottt
requested changes
Apr 30, 2026
Owner
edoardottt
left a comment
There was a problem hiding this comment.
thanks @0xreacher for the PR!
Requesting some changes
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.