fix(http): host var scope across multi-request URLs#7064
Open
dwisiswant0 wants to merge 2 commits intodevfrom
Open
fix(http): host var scope across multi-request URLs#7064dwisiswant0 wants to merge 2 commits intodevfrom
dwisiswant0 wants to merge 2 commits intodevfrom
Conversation
Signed-off-by: Dwi Siswanto <git@dw1.io>
In multi-request HTTP templates, request-scoped host variables could inherit state from the original input target, and absolute URL requests could inherit queryparams from input URL. Fixes #7062 Signed-off-by: Dwi Siswanto <git@dw1.io>
Contributor
WalkthroughThis pull request fixes a bug where multi-request HTTP templates incorrectly inherited query parameters and host variables across requests targeting different domains. The fix conditionally merges query parameters only when hosts match and ensures DSL variables use actual formed URLs instead of input URLs. A new integration test validates the behavior. Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
1 task
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/protocols/http/build_request.go`:
- Around line 255-259: The host comparison using reqURL.Host == parsed.Host can
mismatch equivalent hosts (e.g., example.com vs example.com:80); update the
check to normalize hostnames and ports before merging params: extract hostname
via Hostname() and port via Port() from both reqURL and parsed, treat empty
ports as the scheme default (http->80, https->443), and compare
hostname+effectivePort equality; only when they match call
parsed.Params.Merge(reqURL.Params.Encode()) and assign to reqURL.Params
(referencing reqURL, parsed, parsed.Host, reqURL.Host, finalparams,
Params.Merge, Encode).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
integration_tests/protocols/http/multi-request-host-variable-scope.yamlis excluded by!**/*.yaml
📒 Files selected for processing (3)
cmd/integration-test/http.gopkg/protocols/http/build_request.gopkg/protocols/http/request.go
Comment on lines
+255
to
+259
| if reqURL.Host == parsed.Host { | ||
| finalparams := parsed.Params | ||
| finalparams.Merge(reqURL.Params.Encode()) | ||
| reqURL.Params = finalparams | ||
| } |
Contributor
There was a problem hiding this comment.
Normalize host comparison before deciding param merge.
Line 255 compares raw host strings, so equivalent hosts like example.com and example.com:80 can fail the check and skip intended same-host query merging.
💡 Suggested adjustment
- if reqURL.Host == parsed.Host {
+ normalizeHost := func(scheme, host string) string {
+ h := strings.ToLower(strings.TrimSpace(host))
+ switch scheme {
+ case "http":
+ return strings.TrimSuffix(h, ":80")
+ case "https":
+ return strings.TrimSuffix(h, ":443")
+ default:
+ return h
+ }
+ }
+ if normalizeHost(reqURL.Scheme, reqURL.Host) == normalizeHost(parsed.Scheme, parsed.Host) {
finalparams := parsed.Params
finalparams.Merge(reqURL.Params.Encode())
reqURL.Params = finalparams
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if reqURL.Host == parsed.Host { | |
| finalparams := parsed.Params | |
| finalparams.Merge(reqURL.Params.Encode()) | |
| reqURL.Params = finalparams | |
| } | |
| if reqURL.Host == parsed.Host { | |
| normalizeHost := func(scheme, host string) string { | |
| h := strings.ToLower(strings.TrimSpace(host)) | |
| switch scheme { | |
| case "http": | |
| return strings.TrimSuffix(h, ":80") | |
| case "https": | |
| return strings.TrimSuffix(h, ":443") | |
| default: | |
| return h | |
| } | |
| } | |
| if normalizeHost(reqURL.Scheme, reqURL.Host) == normalizeHost(parsed.Scheme, parsed.Host) { | |
| finalparams := parsed.Params | |
| finalparams.Merge(reqURL.Params.Encode()) | |
| reqURL.Params = finalparams | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/protocols/http/build_request.go` around lines 255 - 259, The host
comparison using reqURL.Host == parsed.Host can mismatch equivalent hosts (e.g.,
example.com vs example.com:80); update the check to normalize hostnames and
ports before merging params: extract hostname via Hostname() and port via Port()
from both reqURL and parsed, treat empty ports as the scheme default (http->80,
https->443), and compare hostname+effectivePort equality; only when they match
call parsed.Params.Merge(reqURL.Params.Encode()) and assign to reqURL.Params
(referencing reqURL, parsed, parsed.Host, reqURL.Host, finalparams,
Params.Merge, Encode).
Member
Author
There was a problem hiding this comment.
The previous implementation didn't consider this. So be it.
Contributor
There was a problem hiding this comment.
@dwisiswant0, understood! The current fix addresses the main scoping issue effectively. The host normalization edge case can be addressed separately if it becomes a practical concern.
🧠 Learnings used
Learnt from: ayuxsec
Repo: projectdiscovery/nuclei PR: 6769
File: pkg/installer/template.go:217-217
Timestamp: 2026-01-15T10:44:22.186Z
Learning: In projectdiscovery/gologger, the Print() function writes to stderr and Silent() writes to stdout. During reviews, ensure code uses Print() for error/output going to stderr and Silent() for stdout; verify the chosen function matches the intended destination and that logs are directed to the correct stream for proper piping/redirects.
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
1 participant
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.
Proposed changes
In multi-request HTTP templates, request-scoped
host variables could inherit state from the
original input target, and absolute URL requests
could inherit queryparams from input URL.
Fixes #7062
Proof
before:
after (this patch):
Checklist
Summary by CodeRabbit
Bug Fixes
Tests