Skip to content

fix(favicon): don't retry absolute favicon hrefs as a path on the target - #2681

Merged
Mzack9999 merged 1 commit into
projectdiscovery:devfrom
Yeagerist0:fix/favicon-no-root-fallback-for-absolute-href
Oct 23, 2026
Merged

fix(favicon): don't retry absolute favicon hrefs as a path on the target#2681
Mzack9999 merged 1 commit into
projectdiscovery:devfrom
Yeagerist0:fix/favicon-no-root-fallback-for-absolute-href

Conversation

@Yeagerist0

@Yeagerist0 Yeagerist0 commented Oct 23, 2026 •
edited by Mzack9999
Loading

Copy link
Copy Markdown
Contributor

Fixes #2682

What

When a favicon fetch fails and the href has no leading slash, HandleFaviconHash retries it as "/" + raw from the root. That fallback is meant for directory-relative hrefs like icon.png, but it also runs for absolute hrefs. With

<link rel="icon" href="http://127.0.0.1:1/icon.png">

an unreachable icon host makes httpx send this request to the target:

GET /http://127.0.0.1:1/icon.png

It can never return the icon. It costs one extra request per target and shows up as a strange 404 in the target's logs. This PR skips the fallback when the parsed href is absolute (url.URL.IsAbs()). Relative hrefs behave as before.

Test

TestFaviconAbsoluteHrefIsNotRetriedAsRootPath runs a full enumeration against an httptest server and records the request paths.

  • On dev it fails: paths=["/" "/http://127.0.0.1:1/icon.png"].
  • With the fix, only / is requested.
  • go test ./runner/ passes.

Independent of #2680 (same function, different lines).

AI disclosure

I used Claude Code to help read the code and write the test. I checked that the test fails on dev and passes with the fix myself.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed favicon handling so failed requests to absolute favicon URLs no longer trigger an incorrect retry against a path on the target site.
    • Added coverage to verify that favicon requests do not use malformed paths.

@Yeagerist0
fix(favicon): don't retry absolute favicon hrefs as a path on the target
a8a2daa 
When a favicon href without a leading slash failed to fetch, the root
fallback requested "/" + href on the base URL. For an absolute href
such as http://cdn.example.com/icon.png that meant a request for
/http://cdn.example.com/icon.png on the target, which never returns the
icon and spends a request per target. Only fall back for relative hrefs.
@coderabbitai

coderabbitai Bot commented Oct 23, 2026 •
edited
Loading

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 76830b32-cd32-4e19-aa41-3f98581afef5
📥 Commits

Reviewing files that changed from the base of the PR and between bf028a2 and a8a2daa.

📒 Files selected for processing: 2
  • runner/favicon_test.go
  • runner/runner.go

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

Walkthrough

The favicon root-relative fallback now applies only to hrefs that are not absolute and do not begin with /. A test checks that a failed absolute favicon URL is not retried as a root path.

Changes

Favicon fallback

Layer / File(s) Summary
Exclude absolute URLs from fallback
runner/runner.go, runner/favicon_test.go		 The fallback condition now excludes absolute hrefs. The test checks that requests do not use a path beginning with /http.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Suggested reviewers: mzack9999

Merge Risk: ⚪ Minimal · up to a8a2d

The favicon change appears ready to merge after normal checks.

Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: preventing absolute favicon hrefs from being retried as target paths.
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

A rabbit checks the favicon trail,
An absolute link no longer turns,
No /http path appears in the tale,
The test records each request it learns,
Then hops away beneath the moon.

Comment @coderabbitai help to get the list of available commands.

Mzack9999
Mzack9999 reviewed Oct 23, 2026
View reviewed changes

@Mzack9999 Mzack9999 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The extra request is real. An absolute favicon href that fails is retried as a path on the target. The new test fails on dev with GET /http://127.0.0.1:1/icon.png and passes once the fallback checks IsAbs. Relative hrefs still fall back. Linked #2682.

CI had not run because the workflows were waiting on a first-time contributor approval.

@Mzack9999
Mzack9999 merged commit eb53226 into projectdiscovery:dev Oct 23, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Mzack9999 Mzack9999 Mzack9999 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Absolute favicon hrefs are retried as a path on the target

2 participants

@Yeagerist0 @Mzack9999