feat(http): add RFC 9111 cache w/ per-request disable#6746
feat(http): add RFC 9111 cache w/ per-request disable#6746dwisiswant0 wants to merge 10 commits intodevfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds RFC 9111–compliant HTTP response caching with LevelDB (fallback in-memory), global enable flag and per-request disable, integrates caching into HTTP transport/client pool, updates CLI/docs/templates/tests, refactors bindgen to use packages.Load, updates dependencies and Makefile build flags. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Nuclei Request
participant Pool as HTTP Client Pool
participant Transport as httpcache.Transport
participant Cache as LevelDB / In‑memory Cache
participant Server as Origin Server
Client->>Pool: Issue HTTP request
Pool->>Transport: Forward request (wrapped RoundTripper)
activate Transport
Transport->>Cache: Lookup cache key (URL + Vary + CacheKeyHeaders)
alt cache hit (fresh)
Cache-->>Transport: Cached response
Transport-->>Pool: Return cached response
else cache miss or stale
Transport->>Server: Send request (may add conditional headers)
Server-->>Transport: Response (200 / 304 / other)
alt 304 Not Modified
Transport->>Cache: Update metadata
Transport-->>Pool: Serve cached body + headers
else 2xx
Transport->>Cache: Store/refresh entry
Transport-->>Pool: Return response
else non-2xx
Transport-->>Pool: Return response (not cached)
end
end
deactivate Transport
Pool-->>Client: Deliver final response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
.github/workflows/compat-checks.yamlis excluded by!**/*.yaml.github/workflows/release.yamlis excluded by!**/*.yaml.github/workflows/tests.yamlis excluded by!**/*.yaml.goreleaser.ymlis excluded by!**/*.ymlgo.sumis excluded by!**/*.sumnuclei-jsonschema.jsonis excluded by!**/*.json
📒 Files selected for processing (10)
MakefileSYNTAX-REFERENCE.mdcmd/nuclei/main.gogo.modpkg/js/devtools/bindgen/generator.gopkg/protocols/http/http.gopkg/protocols/http/httpcache/httpcache.gopkg/protocols/http/httpclientpool/clientpool.gopkg/templates/templates_doc.gopkg/types/types.go
🧰 Additional context used
📓 Path-based instructions (6)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code usinggo fmt ./...
Run static analysis usinggo vet ./...on Go code
Files:
pkg/protocols/http/http.gocmd/nuclei/main.gopkg/types/types.gopkg/js/devtools/bindgen/generator.gopkg/protocols/http/httpcache/httpcache.gopkg/templates/templates_doc.gopkg/protocols/http/httpclientpool/clientpool.go
pkg/protocols/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/protocols/**/*.go: Each protocol implementation should implement the Request interface with Compile(), ExecuteWithResults(), Match(), and Extract() methods
Protocol implementations should embed Operators for matching/extraction functionality
Files:
pkg/protocols/http/http.gopkg/protocols/http/httpcache/httpcache.gopkg/protocols/http/httpclientpool/clientpool.go
cmd/nuclei/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Main CLI entry point with flag parsing and configuration should be located in cmd/nuclei
Files:
cmd/nuclei/main.go
pkg/js/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use custom JavaScript runtime for code protocol templates with auto-generated bindings from pkg/js/generated/
Files:
pkg/js/devtools/bindgen/generator.go
pkg/js/devtools/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Development tools for JavaScript binding generation should be located in pkg/js/devtools/
Files:
pkg/js/devtools/bindgen/generator.go
pkg/templates/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Template parsing, compilation, and management should be implemented in pkg/templates
Files:
pkg/templates/templates_doc.go
🧬 Code graph analysis (2)
pkg/protocols/http/httpcache/httpcache.go (2)
pkg/protocols/http/request_annotations.go (1)
Once(48-48)pkg/catalog/config/nucleiconfig.go (1)
DefaultConfig(23-23)
pkg/protocols/http/httpclientpool/clientpool.go (1)
pkg/protocols/http/httpcache/httpcache.go (1)
NewTransportWrapper(46-71)
⏰ 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 (15)
SYNTAX-REFERENCE.md (1)
1460-1469: LGTM!Documentation for the new
disable-http-cachefield is clear and follows the established format. The description correctly indicates that this is a per-request override of global cache settings.go.mod (3)
3-3: Go 1.25.3 toolchain upgrade looks good.The Go version bump is well-documented in the PR description, including notes about breaking changes (stricter x509 parsing, nil-check fixes, container-aware GOMAXPROCS, etc.). Ensure the CI/CD pipelines and development environments are updated to use Go 1.25.3.
63-67: Dependency updates look reasonable.The dependency updates include:
- Coordinated AWS SDK v2 updates (all modules aligned)
- testcontainers bump for testing (v0.38.0 → v0.39.0)
- mongo-driver patch update (v1.17.4 → v1.17.6)
- OpenTelemetry and protobuf patch updates
These are primarily patch and minor version updates that should maintain backward compatibility. The coordinated AWS SDK update is particularly good practice.
Also applies to: 122-128, 157-170, 380-381, 416-416
116-116: HTTP caching dependency is well-maintained and secure.The
github.com/sandrolain/httpcache v1.4.0dependency is actively maintained (last updated December 2025) with no known security vulnerabilities. The library correctly implements RFC 9111 caching semantics including Age calculation, Cache-Control handling, and Vary header support. This is a solid choice for the HTTP caching feature.pkg/protocols/http/http.go (2)
175-178: LGTM!The
DisableHTTPCachefield is well-implemented:
- Follows struct field naming conventions
- Properly tagged for YAML/JSON serialization with
omitempty- Clear documentation indicating it overrides global settings
- Appropriate jsonschema metadata for API documentation
312-316: Proper integration with HTTP client pool configuration.The
DisableHTTPCachefield is correctly propagated to thehttpclientpool.Configurationduring request compilation. This placement alongside other connection configuration settings (likeDisableCookie) is appropriate and will allow the HTTP client pool to respect per-request cache control settings.pkg/js/devtools/bindgen/generator.go (1)
105-130: Bindgen functionality verified after package loading refactor.The migration from
go/parsertogolang.org/x/tools/go/packagesis a sound modernization that has been successfully tested. The bindgen tool correctly processes all library modules (bytes, fs, goconsole, ikev2, kerberos, ldap, mssql, mysql, net, oracle, and others) with proper package discovery and error handling. Thejsupdate-bindgenMake target uses the refactored code and operates without issues.pkg/protocols/http/httpclientpool/clientpool.go (3)
23-23: LGTM! Clean integration of httpcache package.The import and usage of the httpcache package is properly integrated into the client pool configuration.
87-88: LGTM! Configuration field added correctly.The
DisableHTTPCachefield is properly added to the Configuration struct with clear documentation.
226-228: LGTM! Correct conditional caching logic.The caching is only enabled when both the global option (
options.DisableHTTPCache) and the per-configuration option (configuration.DisableHTTPCache) are false (not disabled). This provides the correct two-level control as described in the PR objectives.pkg/templates/templates_doc.go (1)
602-606: LGTM! Documentation correctly describes the new field.The
disable-http-cachefield documentation is clear and accurately describes that it disables HTTP caching for individual requests and overrides global cache settings.pkg/protocols/http/httpcache/httpcache.go (4)
27-40: LGTM! Robust cache initialization with fallback.The singleton pattern is correctly implemented with
sync.Once, and the fallback to in-memory cache when LevelDB initialization fails is a good defensive approach. The verbose logging on success is appropriate.
46-71: LGTM! Well-configured transport wrapper.The transport configuration is comprehensive and follows RFC 9111 best practices:
SkipServerErrorsFromCache: trueprevents serving stale errorsEnableVarySeparation: truerespects Vary headersCacheKeyHeadersincludes appropriate headers for cache segmentation (Authorization, Cookie, etc.)AsyncRevalidateTimeoutprovides reasonable async revalidation window
75-78: LGTM! Correct cache policy.Caching only 2xx responses is the correct approach per RFC 9111 and prevents caching error responses that might be transient.
29-29: No initialization order issue exists.The code is safe as-is. Go's
init()functions execute automatically during package import (beforemain()starts), soconfig.DefaultConfigis guaranteed to be initialized before any code callsGetCacheDir(). Thesync.Oncepattern at line 28-37 provides thread-safe lazy initialization for the cache. By the timeGetCache()is invoked during HTTP client pool operations,config.DefaultConfigis already available.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/tests/sdk_test.go (1)
22-28: Remove redundant function-level ignores.The package-level ignore on line 23 (
IgnoreAnyContainingPkg("github.com/syndtr/goleveldb")) already suppresses all goroutines from the LevelDB package, making the specific function ignores on lines 24-28 redundant.Recommendation: Choose one approach:
- Option 1 (recommended): Keep only the package-level ignore if all LevelDB background goroutines are expected and legitimate.
- Option 2: Remove the package-level ignore and keep only the specific function ignores if you want tighter control and the ability to detect unexpected goroutines from LevelDB.
Option 1 is simpler and likely sufficient since LevelDB's background goroutines are part of its normal operation.
🔎 Proposed fix to remove redundancy
// LevelDB goroutines (used by httpcache) goleak.IgnoreAnyContainingPkg("github.com/syndtr/goleveldb"), - goleak.IgnoreAnyFunction("github.com/syndtr/goleveldb/leveldb.(*DB).compactionError"), - goleak.IgnoreAnyFunction("github.com/syndtr/goleveldb/leveldb.(*DB).mCompaction"), - goleak.IgnoreAnyFunction("github.com/syndtr/goleveldb/leveldb.(*DB).mpoolDrain"), - goleak.IgnoreAnyFunction("github.com/syndtr/goleveldb/leveldb.(*DB).tCompaction"), - goleak.IgnoreAnyFunction("github.com/syndtr/goleveldb/leveldb/util.(*BufferPool).drain"), }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/tests/sdk_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code usinggo fmt ./...
Run static analysis usinggo vet ./...on Go code
Files:
lib/tests/sdk_test.go
lib/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
SDK for embedding nuclei as a library should be implemented in lib/
Files:
lib/tests/sdk_test.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). (3)
- GitHub Check: Tests (windows-latest)
- GitHub Check: Tests (ubuntu-latest)
- GitHub Check: Tests (macOS-latest)
|
Note that if this ships, it will affect some templates (particularly cache poisoning/deception & cache-related vulnerabilities). Scenario:
Here is what I've found (so far - tho still needs double-checking & deeper pass - I mainly found these via # Cache poisoning
http/vulnerabilities/generic/cache-poisoning.yaml
http/vulnerabilities/generic/cache-poisoning-xss.yaml
http/vulnerabilities/nextjs/next-js-cache-poisoning.yaml
http/misconfiguration/akamai/akamai-s3-cache-poisoning.yaml
http/misconfiguration/aws/cdn-cache-poisoning.yaml
http/fuzzing/cache-poisoning-fuzz.yaml
# Cache deception/leakage
http/vulnerabilities/magento/magento-cacheleak.yaml
http/vulnerabilities/wordpress/wpdm-cache-session.yaml
http/vulnerabilities/wordpress/clearfy-cache-xss.yaml
http/vulnerabilities/wordpress/wp-super-cache-fpd.yaml
http/vulnerabilities/wordpress/wp-fastest-cache-fpd.yaml
http/vulnerabilities/wordpress/wp-sg-cachepress-fpd.yaml
http/misconfiguration/aem/aem-cached-pages.yaml
# Cache purge
http/misconfiguration/unauthenticated-varnish-cache-purge.yamlThey will need to be adjusted by adding cc: @projectdiscovery/template |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
integration_tests/protocols/http/disable-http-cache.yamlis excluded by!**/*.yamlintegration_tests/protocols/http/http-cache.yamlis excluded by!**/*.yaml
📒 Files selected for processing (1)
cmd/integration-test/http.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code usinggo fmt ./...
Run static analysis usinggo vet ./...on Go code
Files:
cmd/integration-test/http.go
🧬 Code graph analysis (1)
cmd/integration-test/http.go (1)
pkg/testutils/integration.go (1)
RunNucleiTemplateAndGetResults(30-32)
⏰ 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)
cmd/integration-test/http.go (2)
14-14: LGTM!The
sync/atomicimport is necessary for the atomic counter operations used in the new cache tests.
92-92: LGTM!The test case registration follows the existing pattern and correctly associates the template path with the test implementation.
dwisiswant0
force-pushed
the
dwisiswant0/feat/http/httpcache
branch
from
ea668ec to
5cc562e
Compare
dwisiswant0
changed the title
The breaking changes are coming from the Go bump, not by the introduction of this feature. So making it opt-in would not really help either (the break would still be there), because the dependency requires that Go version. And it is not a hard break anyway, just a behavior change. |
|
I am noting & calling this out based on past experience, when we bumped the Go version before, we did not revisit or check the release notes and it resulted in breaking. Like:
"just in case" means I do not remember which PR it was anymore, but I still remember it happening, iirc. so I think it is worth being extra explicit this time, just want to make sure we do not repeat the same mistake. |
dwisiswant0
force-pushed
the
dwisiswant0/feat/http/httpcache
branch
from
5cc562e to
cc402af
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/templates/templates_doc.go (1)
463-686: Clarify per‑request cache disable wording to avoid implying it can re‑enable cacheThe
disable-http-cachefield description says it “overrides global cache settings.” Given the implementation only ever disables caching (and cannot re‑enable it if--no-http-cacheis set), consider rephrasing to something like:“Disables HTTP caching for this request, even when HTTP caching is globally enabled.”
and regenerate this file via
docgenso SYNTAX-REFERENCE and generated docs stay in sync.pkg/js/devtools/bindgen/generator.go (1)
15-16: go/packages-based loader looks good; optional diagnostics improvementThe switch to
golang.org/x/tools/go/packageswithNeedSyntax/NeedTypesand wiringpkgMain.TypesintoTemplateDatais a solid fit for this devtool and keeps the rest of the pipeline unchanged.If you want richer diagnostics when a package fails to load, you could aggregate
pkgMain.Errorsinto a single error message (instead of only the first) and includedirectory/pattern info, but that’s purely a tooling UX improvement.Also applies to: 105-122, 126-130
SYNTAX-REFERENCE.md (1)
1459-1467: Tighten wording fordisable-http-cacheto reflect one‑way disableTo match the actual behavior (per-request flag can only disable caching, not re-enable it if
--no-http-cacheis set), consider rephrasing this entry to something like:“Disables HTTP caching for this request, even when HTTP caching is globally enabled.”
That keeps intent clear and avoids suggesting it can override a global disable.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
.github/workflows/compat-checks.yamlis excluded by!**/*.yaml.github/workflows/release.yamlis excluded by!**/*.yaml.github/workflows/tests.yamlis excluded by!**/*.yaml.goreleaser.ymlis excluded by!**/*.ymlgo.sumis excluded by!**/*.sumintegration_tests/protocols/http/disable-http-cache.yamlis excluded by!**/*.yamlintegration_tests/protocols/http/http-cache.yamlis excluded by!**/*.yamlnuclei-jsonschema.jsonis excluded by!**/*.json
📒 Files selected for processing (12)
MakefileSYNTAX-REFERENCE.mdcmd/integration-test/http.gocmd/nuclei/main.gogo.modlib/tests/sdk_test.gopkg/js/devtools/bindgen/generator.gopkg/protocols/http/http.gopkg/protocols/http/httpcache/httpcache.gopkg/protocols/http/httpclientpool/clientpool.gopkg/templates/templates_doc.gopkg/types/types.go
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/tests/sdk_test.go
- cmd/nuclei/main.go
- pkg/protocols/http/httpcache/httpcache.go
- pkg/protocols/http/http.go
🧰 Additional context used
📓 Path-based instructions (5)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code usinggo fmt ./...
Run static analysis usinggo vet ./...on Go code
Files:
pkg/types/types.gopkg/templates/templates_doc.gopkg/protocols/http/httpclientpool/clientpool.gocmd/integration-test/http.gopkg/js/devtools/bindgen/generator.go
pkg/templates/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Template parsing, compilation, and management should be implemented in pkg/templates
Files:
pkg/templates/templates_doc.go
pkg/protocols/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/protocols/**/*.go: Each protocol implementation should implement the Request interface with Compile(), ExecuteWithResults(), Match(), and Extract() methods
Protocol implementations should embed Operators for matching/extraction functionality
Files:
pkg/protocols/http/httpclientpool/clientpool.go
pkg/js/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use custom JavaScript runtime for code protocol templates with auto-generated bindings from pkg/js/generated/
Files:
pkg/js/devtools/bindgen/generator.go
pkg/js/devtools/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Development tools for JavaScript binding generation should be located in pkg/js/devtools/
Files:
pkg/js/devtools/bindgen/generator.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 (10)
go.mod (1)
3-416: Validate module state and compatibility under Go 1.25.3The Go toolchain bump to
go 1.25.3and dependency upgrades (AWS SDK v2 family, sandrolain/httpcache, testcontainers, MongoDB driver, Docker, otel, protobuf, gopsutil, and others) are consistent with the new HTTP caching feature and modern runtime requirements.Ensure this branch passes module validation and build checks under Go 1.25.3:
go mod tidy go build ./... go vet ./...pkg/types/types.go (2)
204-205: LGTM! DisableHTTPCache field correctly added.The field is properly declared with a clear comment, and the default
falsevalue aligns with the PR objective to enable caching by default.
559-559: LGTM! Copy() now correctly propagates DisableHTTPCache.The field is properly copied, ensuring the setting persists across option clones.
cmd/integration-test/http.go (3)
94-95: LGTM! Test case registrations added correctly.The new test cases follow the established pattern and reference the appropriate template files.
1739-1769: LGTM! httpCache test correctly validates caching behavior.The test properly verifies that:
- Both template requests produce matching results (2 results)
- Only 1 actual HTTP request reaches the server due to caching
The use of
Cache-Control: max-age=2ensures the response is cacheable per RFC 9111.
1771-1801: LGTM! httpDisableCache test correctly validates cache-disable behavior.The test properly verifies that when
disable-http-cache: trueis set in the template:
- Both template requests produce matching results (2 results)
- Both requests actually hit the server (2 requests, confirming cache is bypassed)
pkg/protocols/http/httpclientpool/clientpool.go (4)
87-88: LGTM! DisableHTTPCache field added to Configuration.The field is properly placed with a clear comment explaining its purpose.
128-129: LGTM! Hash correctly includes DisableHTTPCache.Including the cache setting in the hash ensures proper client pooling - cached and non-cached clients will have different hashes and won't be mixed.
144-144: LGTM! HasStandardOptions correctly handles DisableHTTPCache.When
DisableHTTPCacheistrue,HasStandardOptions()returnsfalse, ensuring a custom client is created with caching disabled rather than using the default pooled client.
226-228: LGTM! Cache wrapper applied correctly with proper precedence.The logic
!options.DisableHTTPCache && !configuration.DisableHTTPCachecorrectly implements the PR objective: caching is enabled only when neither the global flag (--no-http-cache) nor the template-level option (disable-http-cache) disables it. Either mechanism can independently disable caching.
Here I just came across this accidentally: projectdiscovery/fastdialer#434. Another normative references: |
|
The reason I proposed to make it optional as a start and gradually turn into default, was about not introducing breaking changes on the actual templates. We can quantify on public templates, but many might be private. Hence I think either we introduce some heuristic for cache templates types and we disable this automatically or we can introduce it as optional and activate gradually for what over time we tag as safe template categories (for example single-request templates, which should exclude cache poisoning as they generally require multiple request to poison and check). What do you think? |
Mzack9999
left a comment
Mzack9999
left a comment
There was a problem hiding this comment.
marking as "request changes" as there is open discussion about go version bump and default behavior
dwisiswant0
force-pushed
the
dwisiswant0/feat/http/httpcache
branch
from
cc402af to
1c7d02b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cmd/nuclei/main.go`:
- Line 438: The short flag "-hc" for the http-cache flag (defined via
flagSet.BoolVarP and bound to options.EnableHTTPCache) collides with the
existing "--health-check" short alias; remove the short alias for http-cache or
assign a different unused short letter—e.g., drop the short alias by switching
from BoolVarP to BoolVar (so only --http-cache remains) or change the short rune
to a non-conflicting one in the http-cache flag registration; update the
corresponding second occurrence at the other location similarly to avoid the
duplicate "-hc" binding.
In `@pkg/protocols/http/httpclientpool/clientpool.go`:
- Around line 226-228: The cache enablement condition in clientpool.go currently
uses OR and makes caching on by default; update the condition that sets
retryableHttpOptions.WrapTransport so it only enables caching when both the
global opt-in and the per-request allow it — i.e., replace the expression using
options.EnableHTTPCache || !configuration.DisableHTTPCache with a conjunction
that checks the global flag (configuration.DisableHTTPCache or its inverse) and
that the request did not disable caching (options.EnableHTTPCache), ensuring
WrapTransport is set only when both criteria are true.
🧹 Nitpick comments (3)
pkg/js/devtools/bindgen/generator.go (2)
103-103: Consider removing or converting debug output.
fmt.Println(directory)appears to be debug output. Consider removing it or converting it to a proper log statement usinglog.Printffor consistency with Line 124.Suggested fix
func CreateTemplateData(directory string, packagePrefix string) (*TemplateData, error) { - fmt.Println(directory) + log.Printf("[create] Processing directory: %s\n", directory)
119-122: Consider reporting all package errors for better diagnostics.Currently only the first error is reported. For a dev tool, aggregating all errors could help with debugging when multiple issues exist.
Optional: Report all errors
pkgMain := pkgs[0] if len(pkgMain.Errors) > 0 { - return nil, errors.Wrap(pkgMain.Errors[0], "could not check package") + var errMsgs []string + for _, e := range pkgMain.Errors { + errMsgs = append(errMsgs, e.Error()) + } + return nil, fmt.Errorf("could not check package: %s", strings.Join(errMsgs, "; ")) }pkg/protocols/http/httpcache/httpcache.go (1)
29-36: Log when LevelDB init fails to avoid silent in‑memory fallback.Right now the fallback is silent, which makes it hard to diagnose why persistence isn’t used. Consider a warning log on the error path.
♻️ Suggested tweak
- cache, err = leveldbcache.New(cacheDir) - if err != nil { - cache = httpcache.NewMemoryCache() - } else { - gologger.Verbose().Msgf("HTTP cache initialized at %q", cacheDir) - } + cache, err = leveldbcache.New(cacheDir) + if err != nil { + gologger.Warning().Msgf("HTTP cache fallback to memory (leveldb init failed): %v", err) + cache = httpcache.NewMemoryCache() + } else { + gologger.Verbose().Msgf("HTTP cache initialized at %q", cacheDir) + }
dwisiswant0
force-pushed
the
dwisiswant0/feat/http/httpcache
branch
2 times, most recently
from
8b9d7e7 to
fd513ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/protocols/http/httpclientpool/clientpool.go`:
- Around line 226-228: DefaultHTTPClient is left nil and the cache wrapper is
only applied in wrappedGet(), so Get() can return a nil client and miss caching;
in initDialers() ensure Dialers.DefaultHTTPClient is constructed (not nil) and
that when options.EnableHTTPCache && !configuration.DisableHTTPCache you apply
the httpcache.NewTransportWrapper() to the DefaultHTTPClient's transport (or set
retryableHttpOptions.WrapTransport accordingly) so the cache is present on the
default client; update initDialers() to create the http.Client (or retryable
client used elsewhere), attach the wrapper, and ensure Get()/wrappedGet() return
that initialized client.
| if options.EnableHTTPCache && !configuration.DisableHTTPCache { | ||
| retryableHttpOptions.WrapTransport = httpcache.NewTransportWrapper() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where DefaultHTTPClient is created and how transport wrapping is configured
rg -nP 'DefaultHTTPClient|NewWithHTTPClient|WrapTransport|httpcache\.NewTransportWrapper' --type=go pkgRepository: projectdiscovery/nuclei
Length of output: 708
🏁 Script executed:
#!/bin/bash
# Get context around line 182 (DefaultHTTPClient return) and the full Get method
sed -n '170,200p' pkg/protocols/http/httpclientpool/clientpool.go
# Get context around line 227 (WrapTransport)
sed -n '210,240p' pkg/protocols/http/httpclientpool/clientpool.go
# Get context around line 364 (NewWithHTTPClient)
sed -n '350,370p' pkg/protocols/http/httpclientpool/clientpool.goRepository: projectdiscovery/nuclei
Length of output: 2993
🏁 Script executed:
#!/bin/bash
# Find HasStandardOptions definition
rg -nP 'func.*HasStandardOptions' --type=go pkg
# Find where DefaultHTTPClient is created/initialized
rg -nP 'DefaultHTTPClient\s*=' --type=go pkg
# Find the initialization of DefaultHTTPClient
rg -nB5 -A5 'DefaultHTTPClient\s*=' --type=go pkg/protocols/common/protocolstate/Repository: projectdiscovery/nuclei
Length of output: 170
🏁 Script executed:
#!/bin/bash
# Get HasStandardOptions implementation
sed -n '143,160p' pkg/protocols/http/httpclientpool/clientpool.go
# Search for DefaultHTTPClient initialization in dialers
cat -n pkg/protocols/common/protocolstate/dialers.go | head -100
# Also search for where DefaultHTTPClient might be initialized
rg -nP 'DefaultHTTPClient\s*=' --type=goRepository: projectdiscovery/nuclei
Length of output: 1617
🏁 Script executed:
#!/bin/bash
# Find where DefaultHTTPClient is assigned/created
rg -nB10 -A10 'DefaultHTTPClient\s*=' --type=go pkg/protocols/
# Also check in cmd or main files for initialization
git ls-files | grep -E '(main|init|setup)' | head -20Repository: projectdiscovery/nuclei
Length of output: 530
🏁 Script executed:
#!/bin/bash
# Check the protocol initialization file
cat -n pkg/protocols/common/protocolinit/init.go | head -150
# Search more thoroughly for where DefaultHTTPClient might be created
rg -nB5 -A15 'DefaultHTTPClient' --type=go pkg/ | grep -E '(DefaultHTTPClient|NewWithHTTPClient|WrapTransport)'Repository: projectdiscovery/nuclei
Length of output: 1940
🏁 Script executed:
#!/bin/bash
# Find protocolstate Init function
rg -nA30 'func Init' pkg/protocols/common/protocolstate/ --type=go
# Find httpclientpool Init function
rg -nA50 'func Init' pkg/protocols/http/httpclientpool/ --type=goRepository: projectdiscovery/nuclei
Length of output: 8975
🏁 Script executed:
#!/bin/bash
# Get the complete initDialers function
sed -n '66,200p' pkg/protocols/common/protocolstate/state.goRepository: projectdiscovery/nuclei
Length of output: 4192
🏁 Script executed:
#!/bin/bash
# Continue reading initDialers
sed -n '156,250p' pkg/protocols/common/protocolstate/state.goRepository: projectdiscovery/nuclei
Length of output: 3007
🏁 Script executed:
#!/bin/bash
# Search for actual assignments to DefaultHTTPClient
rg -nP '\.DefaultHTTPClient\s*=' --type=go pkg/
# Also search for where it might be assigned using Set or other methods
rg -nP 'DefaultHTTPClient' --type=go pkg/ -B2 -A2 | head -100Repository: projectdiscovery/nuclei
Length of output: 1316
🏁 Script executed:
#!/bin/bash
# Absolutely verify no assignment to DefaultHTTPClient anywhere
rg 'DefaultHTTPClient\s*=' --type=go
# Also search in all files, not just Go
git grep 'DefaultHTTPClient' | grep -E '(\s=\s|:=)' || echo "No assignments found"
# Check if there's any lazy initialization logic
rg -B5 -A5 'dialers\.DefaultHTTPClient' --type=go pkg/Repository: projectdiscovery/nuclei
Length of output: 1687
DefaultHTTPClient is uninitialized and cache wrapper is not applied.
The concern is valid. DefaultHTTPClient is never created or initialized in the codebase—it remains a nil field in the Dialers struct. Even if it were initialized, the cache wrapper is only applied in wrappedGet() at line 227, not during DefaultHTTPClient creation. When Get() returns dialers.DefaultHTTPClient for standard configurations (line 182), it returns nil, and requests using standard configs will not have caching enabled.
The httpcache.NewTransportWrapper() must be applied to DefaultHTTPClient during its initialization in protocolstate.initDialers() when EnableHTTPCache is true.
🤖 Prompt for AI Agents
In `@pkg/protocols/http/httpclientpool/clientpool.go` around lines 226 - 228,
DefaultHTTPClient is left nil and the cache wrapper is only applied in
wrappedGet(), so Get() can return a nil client and miss caching; in
initDialers() ensure Dialers.DefaultHTTPClient is constructed (not nil) and that
when options.EnableHTTPCache && !configuration.DisableHTTPCache you apply the
httpcache.NewTransportWrapper() to the DefaultHTTPClient's transport (or set
retryableHttpOptions.WrapTransport accordingly) so the cache is present on the
default client; update initDialers() to create the http.Client (or retryable
client used elsewhere), attach the wrapper, and ensure Get()/wrappedGet() return
that initialized client.
dwisiswant0
force-pushed
the
dwisiswant0/feat/http/httpcache
branch
from
fd513ff to
3cc45c8
Compare
Neo - PR Security ReviewMedium: 4 Current PR state: 4 medium active findings. Highlights
Medium (4)Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/js/devtools/bindgen/generator.go (1)
105-110: Optional: Document or pin build environment for bindgen determinismLine 110 loads packages with default environment settings. While no build-constrained files currently exist in the target packages, consider documenting or explicitly pinning the environment (e.g., BuildFlags in packages.Config) if future changes introduce platform-specific code. This ensures generated bindings remain consistent across machines.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/js/devtools/bindgen/generator.go` around lines 105 - 110, The packages.Load call uses a packages.Config (cfg) with default environment settings; to make bindgen deterministic, update cfg to explicitly pin the build environment (for example by setting cfg.BuildFlags and/or cfg.Env to a stable GOOS/GOARCH and any necessary tags) and document this choice near the creation of cfg, so future platform-specific files won’t change generated output; reference the packages.Config instance named cfg and the packages.Load call to apply this change and add a brief comment explaining the pinned values.pkg/protocols/http/httpcache/httpcache.go (1)
31-35: Log LevelDB init failures before in-memory fallback.Fallback is good, but the current path is silent on failure and makes cache persistence issues hard to diagnose in production.
🔎 Suggested improvement
cache, err = leveldbcache.New(cacheDir) if err != nil { + gologger.Warning().Msgf("HTTP cache LevelDB init failed at %q: %v; using in-memory cache", cacheDir, err) cache = httpcache.NewMemoryCache() } else { gologger.Verbose().Msgf("HTTP cache initialized at %q", cacheDir) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/protocols/http/httpcache/httpcache.go` around lines 31 - 35, When leveldbcache.New(cacheDir) fails the code silently falls back to httpcache.NewMemoryCache(); change this to log the failure before falling back by capturing the returned err and calling gologger.Error().Msgf (or Warn) with a descriptive message that includes cacheDir and err; keep the existing gologger.Verbose().Msgf("HTTP cache initialized at %q", cacheDir) for the success path and only perform the in-memory fallback after logging the error from leveldbcache.New.
🤖 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/templates/templates_doc.go`:
- Around line 605-606: The description and line comment for
HTTPRequestDoc.Fields[22] currently say "DisableHTTPCache disables HTTP caching
for this request, overriding global cache settings." — update both the
Description and the encoder.LineComment for HTTPRequestDoc.Fields[22] to clarify
that DisableHTTPCache is a one-way opt-out: it disables caching for this request
but cannot re-enable caching if caching is globally disabled (i.e., it prevents
caching locally but does not override a global disable). Ensure the revised text
explicitly states the precedence: local opt-out allowed, local opt-in not
permitted when global cache is disabled.
---
Nitpick comments:
In `@pkg/js/devtools/bindgen/generator.go`:
- Around line 105-110: The packages.Load call uses a packages.Config (cfg) with
default environment settings; to make bindgen deterministic, update cfg to
explicitly pin the build environment (for example by setting cfg.BuildFlags
and/or cfg.Env to a stable GOOS/GOARCH and any necessary tags) and document this
choice near the creation of cfg, so future platform-specific files won’t change
generated output; reference the packages.Config instance named cfg and the
packages.Load call to apply this change and add a brief comment explaining the
pinned values.
In `@pkg/protocols/http/httpcache/httpcache.go`:
- Around line 31-35: When leveldbcache.New(cacheDir) fails the code silently
falls back to httpcache.NewMemoryCache(); change this to log the failure before
falling back by capturing the returned err and calling gologger.Error().Msgf (or
Warn) with a descriptive message that includes cacheDir and err; keep the
existing gologger.Verbose().Msgf("HTTP cache initialized at %q", cacheDir) for
the success path and only perform the in-memory fallback after logging the error
from leveldbcache.New.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 839d3329-7632-44bc-ac65-5251e1589582
⛔ Files ignored due to path filters (8)
.github/workflows/compat-checks.yamlis excluded by!**/*.yaml.github/workflows/release.yamlis excluded by!**/*.yaml.github/workflows/tests.yamlis excluded by!**/*.yaml.goreleaser.ymlis excluded by!**/*.ymlgo.sumis excluded by!**/*.sumintegration_tests/protocols/http/disable-http-cache.yamlis excluded by!**/*.yamlintegration_tests/protocols/http/http-cache.yamlis excluded by!**/*.yamlnuclei-jsonschema.jsonis excluded by!**/*.json
📒 Files selected for processing (12)
MakefileSYNTAX-REFERENCE.mdcmd/integration-test/http.gocmd/nuclei/main.gogo.modlib/tests/sdk_test.gopkg/js/devtools/bindgen/generator.gopkg/protocols/http/http.gopkg/protocols/http/httpcache/httpcache.gopkg/protocols/http/httpclientpool/clientpool.gopkg/templates/templates_doc.gopkg/types/types.go
✅ Files skipped from review due to trivial changes (1)
- SYNTAX-REFERENCE.md
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/types/types.go
- pkg/protocols/http/httpclientpool/clientpool.go
- pkg/protocols/http/http.go
- Makefile
- cmd/integration-test/http.go
- cmd/nuclei/main.go
| HTTPRequestDoc.Fields[22].Description = "DisableHTTPCache disables HTTP caching for this request, overriding global cache settings." | ||
| HTTPRequestDoc.Fields[22].Comments[encoder.LineComment] = "DisableHTTPCache disables HTTP caching for this request, overriding global cache settings." |
There was a problem hiding this comment.
Clarify disable-http-cache precedence semantics.
The current text implies full override behavior. disable-http-cache should be documented as a one-way opt-out (cannot re-enable cache when globally disabled).
✏️ Suggested wording update
- HTTPRequestDoc.Fields[22].Description = "DisableHTTPCache disables HTTP caching for this request, overriding global cache settings."
- HTTPRequestDoc.Fields[22].Comments[encoder.LineComment] = "DisableHTTPCache disables HTTP caching for this request, overriding global cache settings."
+ HTTPRequestDoc.Fields[22].Description = "DisableHTTPCache disables HTTP caching for this request. If global HTTP cache is disabled, this field cannot enable it."
+ HTTPRequestDoc.Fields[22].Comments[encoder.LineComment] = "DisableHTTPCache disables HTTP caching for this request."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/templates/templates_doc.go` around lines 605 - 606, The description and
line comment for HTTPRequestDoc.Fields[22] currently say "DisableHTTPCache
disables HTTP caching for this request, overriding global cache settings." —
update both the Description and the encoder.LineComment for
HTTPRequestDoc.Fields[22] to clarify that DisableHTTPCache is a one-way opt-out:
it disables caching for this request but cannot re-enable caching if caching is
globally disabled (i.e., it prevents caching locally but does not override a
global disable). Ensure the revised text explicitly states the precedence: local
opt-out allowed, local opt-in not permitted when global cache is disabled.
|
@coderabbitai create an issue for this (CI error): |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 54
The issue covers two aspects of the problem:
|
Mzack9999
left a comment
Mzack9999
left a comment
There was a problem hiding this comment.
- Merge Conflicts
- Failing Tests
* wrap HTTP transports with sandrolain/httpcache and expose global `--no-http-cache` flag. * allow templates to set `disable-http-cache`. * bump Go to 1.25.7. Signed-off-by: Dwi Siswanto <git@dw1.io>
Signed-off-by: Dwi Siswanto <git@dw1.io>
Signed-off-by: Dwi Siswanto <git@dw1.io>
Signed-off-by: Dwi Siswanto <git@dw1.io>
Signed-off-by: Dwi Siswanto <git@dw1.io>
Signed-off-by: Dwi Siswanto <git@dw1.io>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Signed-off-by: Dwi Siswanto <git@dw1.io>
Signed-off-by: Dwi Siswanto <git@dw1.io>
Signed-off-by: Dwi Siswanto <git@dw1.io>
dwisiswant0
force-pushed
the
dwisiswant0/feat/http/httpcache
branch
from
3cc45c8 to
834fae0
Compare
2 participants
Proposed changes
feat(http): add RFC-9111 cache w/ per-request disable
and expose global
--no-http-cacheflag.disable-http-cache.Closes #6725.
by default, caching is on. but if either:
(
disable-http-cache), the cache is turned off.neither can force it "on" if the other wants it "off".
everything follows the spec outlined in #6725,
with LevelDB configured using default settings.
see:
nit changes:
Previously pinned to the "stable" version
because
GOEXPERIMENT=greenteagcrequires >= go1.25. now that constraint is no
longer needed, it follows
go.modagain.SHA-1 sig algos are now disallowed in TLS 1.2
handshakes by default (RFC 9155) in >= go1.25.
although we explicitly set
tls.VersionTLS10(in
httpclientpool), we also set globally,just in case.
parser.ParseDircallsas commit message.
GO*envs forgo-buildas commit message.
🚨 behavior/breaking changes & action items
few things to consider after bumping to >= go1.25,
beyond what's already addressed in this patch (as
outlined above):
crypto/x509.ParseCertificatenow rejectscerts with negative
pathLenConstraintand isstricter about ASN.1 T61String/BMPString
encoding.
Impact: Nuclei might report errors parsing
malformed certs that were previously accepted.
So be aware that "failed to parse certificate"
errors might increase for targets with
non-compliant certs.
nilpointer compiler bug fixed in 1.25 meanscode that incorrectly delayed
nilchecks(e.g., using a file handle before checking the
errfromos.Open) will now correctly panic.Impact: Low risk but worth noting if user see
new panics in error handling paths.
Container-aware
GOMAXPROCS1, means the runtimenow respects cgroup CPU limits and updates
GOMAXPROCSdynamically.Impact: Nuclei running in Docker/Kubernetes
might see different default concurrency levels.
This is basically an improvement but changes
perf characteristics. Can be disabled via
GODEBUG=containermaxprocs=0.Change to unhandled panic output2, means the
output format for recovered and re-panicked
panics has changed (it no longer repeats the
panic value).
Impact: If Nuclei has any internal log parsing
tests that match exact panic output strings,
they might fail.
Platform support. go1.25 now requires macOS 12
(Monterey) or later and windows/arm (32-bit)
port is broken and deprecated (support wil be
removed in >= go1.26).
Impact: No action needed. We already ignore that
deprecated
GOOS&GOARCHin the GoReleaserconfig.
Proof
Template:
As demonstrated above, enabling the HTTP cache
results in a ~4x speedup for repeated requests
to cacheable endpoints. This significantly reduces
network traffic and scan times when testing
against targets with proper cache control headers,
making re-scans and complex workflows much more
efficient.
Checklist
Summary by CodeRabbit
New Features
Documentation
Chores
Tests