fix(favicon): correct relative favicon resolution & .ico prioritization #2214
Conversation
WalkthroughThe changes refactor and enhance the favicon extraction and hashing logic. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Runner
participant HTTPX
participant TargetServer
User->>Runner: Initiate analysis with favicon flag
Runner->>HTTPX: Send HTTP request
HTTPX->>TargetServer: Fetch URL (handle redirects)
TargetServer-->>HTTPX: Return HTML response (possibly with redirects)
HTTPX-->>Runner: Provide response, finalURL
Runner->>Runner: extractPotentialFavIconsURLs(response)
Runner->>Runner: Determine base URL (finalURL, <base href>)
alt Favicon link is base64 data URL
Runner->>Runner: Decode and hash favicon
else Favicon link is URL
Runner->>TargetServer: Fetch favicon from resolved URL
TargetServer-->>Runner: Return favicon data
alt Fetch fails and candidate is relative
Runner->>TargetServer: Retry with root-relative path
TargetServer-->>Runner: Return favicon data
end
Runner->>Runner: Hash favicon data
end
Runner-->>User: Return favicon hash and info
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
runner/runner.go (1)
2339-2451: Excellent refactoring that addresses all PR objectives!The implementation correctly:
- Uses
finalURLfor base URL determination- Supports
<base href>tags- Handles data URLs for base64-encoded favicons
- Implements root fallback logic
- Limits network attempts to prevent excessive requests
- Includes debug logging for troubleshooting
Minor suggestions for consideration:
- Consider extracting the network attempt limit as a constant:
+const maxFaviconFetchAttempts = 2 + func (r *Runner) HandleFaviconHash(hp *httpx.HTTPX, req *retryablehttp.Request, currentResp []byte, finalURL string, defaultProbe bool) (string, string, string, []byte, string, error) {
- The debug log could include the base URL for better debugging:
-gologger.Debug().Msgf("favicon resolved url=%s raw_href=%s size=%d bytes", faviconURL, faviconPath, len(faviconData)) +gologger.Debug().Msgf("favicon resolved url=%s raw_href=%s base=%s size=%d bytes", faviconURL, faviconPath, baseNet.String(), len(faviconData))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
runner/runner.go(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
runner/runner.go (2)
common/httpx/httpx.go (2)
HTTPX(33-43)UnsafeOptions(346-348)common/stringz/stringz.go (2)
IsBase64Icon(161-171)DecodeBase64Icon(173-182)
🔇 Additional comments (4)
runner/runner.go (4)
16-16: LGTM!The
net/urlimport is correctly added and necessary for the new URL resolution logic.
2028-2028: Method signature correctly updated to support redirect-aware favicon resolution.The addition of the
finalURLparameter enables proper favicon resolution based on the final redirected URL, which addresses the core issue described in the PR objectives.Also applies to: 2337-2337
2461-2498: Well-implemented favicon extraction with proper .ico prioritization!The function correctly:
- Extracts
<base href>for proper URL resolution- Supports all common favicon
relattribute values- Uses
sort.SliceStableto prioritize.icofiles while preserving order- Handles case-insensitive matching and empty hrefs properly
The stable sort implementation is particularly elegant, ensuring
.icofiles are tried first as specified in the PR objectives.
2418-2434: Verify root fallback behavior for edge cases.The root fallback logic only triggers when the href doesn't start with "/". This handles cases like
href="portal/images/favicon.ico"but nothref="/portal/images/favicon.ico".While this might be intentional, please verify that this behavior is correct for all expected use cases.
Consider if you need to handle other edge cases where directory-relative resolution might fail, such as:
- Hrefs starting with "./" or "../"
- Protocol-relative URLs (//example.com/favicon.ico)
dogancanbakir
requested review from
ehsandeep
and removed request for
dwisiswant0
ehsandeep
left a comment
ehsandeep
left a comment
There was a problem hiding this comment.
$ ./httpx -favicon -u https://211.148.131.226:10443 -fr -silent
https://211.148.131.226:10443 [-631559155]
4 participants
Fixes #2203
Problem
-faviconmis-resolved directory-relative favicon paths when the final page is under a subdirectory.Example final URL: /global-protect/login.esp
HTML:
The code treated it as /portal/images/... (dropped /global-protect/), fetched 404, so no hash.
Additional issues:
Root Cause
Relative href joined against root instead of the final redirect URL directory; did not use net/url ResolveReference.
Changes
Result
Before (no favicon hash printed):
https://211.148.131.226:10443/ [302,200] []
After (favicon hash appended):
https://211.148.131.226:10443/ [302,200] [-631559155]
Debug log:
[DBG] favicon resolved url=https://211.148.131.226:10443/global-protect/portal/images/favicon.ico raw_href=portal/images/favicon.ico size=2518 bytes
Summary by CodeRabbit
New Features
.icofavicon files and improved fallback handling for default favicon paths.Bug Fixes