Conversation
|
Caution Review failedThe pull request is closed. WalkthroughUpdated a dependency and replaced buffer-size constants; migrated HTTP response accessors to new Bytes/String variants across storage, logging, event generation, and fuzzing paths. No public API signatures changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
Comment |
Signed-off-by: Dwi Siswanto <git@dw1.io>
Signed-off-by: Dwi Siswanto <git@dw1.io>
a75b7c0 to
6d72217
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/protocols/http/request.go (1)
951-955: Consistent use of new ResponseChain String/Bytes accessors; minor reuse opportunityThe refactor to
BodyBytes(),FullResponseString(),BodyString(),HeadersString(), andFullResponseBytes()is coherent with the updated utils API and preserves the prior semantics for:
- project file storage (
BodyBytes()),- request stats logging (
FullResponseString()),- DSL map construction (full/body/headers strings),
- debug dumping (
FullResponseBytes()), and- fuzz stats (
RawResponseas full response string).One small optimization you might consider: inside the
for respChain.Has()loop, cachefullResp := respChain.FullResponseString()(and, if needed,fullRespBytes := respChain.FullResponseBytes()) once per iteration and reuse it forRequestStatsLog,responseToDSLMap,dumpResponse, andRawResponse, assuming these helpers don’t already cache internally. That would avoid repeated conversions on large responses. Not suggesting any extra documentation for this internal accessor swap. Based on learnings, …Also applies to: 965-966, 1006-1006, 1069-1070, 1083-1084
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.mod(1 hunks)pkg/protocols/common/protocolstate/memguardian.go(2 hunks)pkg/protocols/http/request.go(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-30T16:34:42.125Z
Learnt from: dwisiswant0
Repo: projectdiscovery/nuclei PR: 6290
File: pkg/protocols/http/build_request.go:457-464
Timestamp: 2025-06-30T16:34:42.125Z
Learning: In the projectdiscovery/retryablehttp-go package, the Request struct embeds URL fields directly, making req.Scheme, req.Host, and other URL fields accessible directly on the Request object instead of requiring req.URL.Scheme, req.URL.Host, etc.
Applied to files:
pkg/protocols/http/request.go
📚 Learning: 2025-06-30T16:33:26.746Z
Learnt from: dwisiswant0
Repo: projectdiscovery/nuclei PR: 6290
File: pkg/fuzz/component/path_test.go:43-44
Timestamp: 2025-06-30T16:33:26.746Z
Learning: The user dwisiswant0 declined adding documentation for internal API changes in pkg/fuzz/component/path when the change was from nested URL field access (rebuilt.URL.Path, rebuilt.URL.String()) to direct field access (rebuilt.Path, rebuilt.String()), indicating they don't consider additional documentation necessary for such internal API modifications.
Applied to files:
pkg/protocols/http/request.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint
🔇 Additional comments (2)
go.mod (1)
112-112: Utils dependency bump looks consistent; confirm downstream compatibilityUpdating
github.com/projectdiscovery/utilstov0.7.1aligns with the new HTTP helpers used elsewhere in this PR. No issues from the mod file alone; just ensurego.sumis updated and the tree still builds and tests pass against the new utils API.pkg/protocols/common/protocolstate/memguardian.go (1)
99-103: Switch toDefaultBufferSizekeeps guard/restore behavior coherentUsing
httputil.DefaultBufferSizefor both the low‑memory guard check and the restore delta keeps the pool‑size adjustments symmetric and preserves the “only shrink from the default, then restore back to the default” behavior. Locking and delta math look correct.Please just confirm that
DefaultBufferSizein the updated utils version is the canonical default forGetPoolSize()so the equality checks remain valid.Also applies to: 115-118
|
Superseded by #6629. |
Proposed changes
Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.