Accuracy fixed - #163
Conversation
Neo - PR Security ReviewCaution Neo couldn't finish analyzing this pull request during this run. Please run the review again. Comment |
Mzack9999
left a comment
Mzack9999
left a comment
There was a problem hiding this comment.
Thanks for the PR, the direction is good and the EnrichWithInfo / EnrichWithCats split is a nice cleanup. Two main asks before this can land.
Switch the headless backend from chromedp to go-rod. Built in browser pooling, WaitStable for SPA hydration, and HijackRequests for full response capture solve most of the rough edges of the current implementation, and go-rod is already used elsewhere in our stack. With one shared rod.Browser on the client and a fresh page per call we also avoid spawning a Chromium process on every invocation.
Rename FingerprintURL to FingerprintHeadless (and the WithInfo / WithCats variants accordingly). The current name reads like a static URL fetch and breaks the WithX naming pattern used everywhere else in the package, where WithX means the return value carries extra X.
Also please revert the go.mod toolchain bump from 1.25.0 to 1.26.2, looks accidental from a local toolchain. Once the backend is go-rod, hide the headless file behind a build tag or move it to a headless/ sub package so static only consumers don't inherit the browser dep tree.
A few smaller things left inline.
| } | ||
|
|
||
| // FingerprintURL checks the provided URL using headless browser (chromedp) for deep JS & DOM fingerprinting | ||
| func (s *Wappalyze) FingerprintURL(ctx context.Context, url string) (map[string]struct{}, error) { |
There was a problem hiding this comment.
Please rename to FingerprintHeadless (with FingerprintHeadlessWithInfo and FingerprintHeadlessWithCats following). FingerprintURL reads like a static fetch helper and the WithX suffix in this package is reserved for return shape, not mechanism.
While renaming, also worth moving this surface onto a small Headless sub client returned by Wappalyze.Headless(). It gives a natural home for the shared browser, options, and a Close method, none of which fit on Wappalyze itself.
| jsGlobals = make(map[string]string) | ||
| domMatches = make(map[string]map[string]string) | ||
|
|
||
| c, cancel := chromedp.NewContext(ctx) |
There was a problem hiding this comment.
Switch this whole flow to go-rod. One rod.Browser held on the client, one rod.Page per call, page.WaitStable instead of a fixed sleep, and page.HijackRequests to capture all response headers and cookies cleanly. Most of the current race and lifecycle concerns disappear with that rewrite.
| var domMatched bool | ||
| for attr, pattern := range attrPatterns { | ||
| var attrValue string | ||
| if attr == "main" { |
There was a problem hiding this comment.
text and exists are collapsed to the same compiled key main upstream, which loses the info this branch needs to know whether to look at el.textContent or just existence. End result is every text rule on the headless path matches against the literal string true. Keep the kind on the compiled struct, either as separate maps or a small enum, so the headless side can dispatch correctly.
| headlessFingerprintsWithCats := wappalyzerClient.EnrichWithCats(headlessFingerprints) | ||
| fmt.Printf("Categories: %v\n", headlessFingerprintsWithCats) | ||
|
|
||
| // Output: map[Acquia Cloud Platform:{} Amazon EC2:{} Apache:{} Cloudflare:{} Drupal:{} PHP:{} Percona:{} React:{} Varnish:{}] |
There was a problem hiding this comment.
This Output comment is copy pasted from the static cetus.zone example and has nothing to do with hackerone.com or with what the headless API would return. Please drop it or replace with a real sample.
| @@ -0,0 +1,29 @@ | |||
| package main | |||
There was a problem hiding this comment.
This duplicates examples/main.go. Please either remove examples/main.go in favor of the new structure or drop this file. Three near identical static examples is confusing.
2 participants
check this issue: #162