Conversation
|
First off, thanks for the contributions and at first glance these all look really excellent! I'll get the team on reviewing them all and should get back to you with reviews pretty quick, with maybe the exception of this one as it's something we've long talked about on the VulnCheck side and there might be a some restructuring around the HTTP handling that is necessary (as I see you have caught on). So this one might have a bit more back and forth as we figure out what the best API strategy is :) Thanks again for contributing! |
Thanks! Totally understand, this touches core architecture and there's a real choice between patching on top or rethinking the HTTP layer. Happy to adapt this PR to whatever direction the team decides, or hold off until the new design is clearer. Just let me know what's most useful. |
There was a problem hiding this comment.
Pull request overview
Adds a new session-aware config.HTTPClient that hangs off Config to simplify exploit HTTP flows (persistent cookies, optional auth injection, URL generation, and convenience helpers), along with a comprehensive config package test suite for the new client.
Changes:
- Introduce
Config.HTTP()which lazily constructs a persistent HTTP client with cookie-jar support, redirect control, per-request functional options, and helper methods (JSON/form/multipart). - Add
Config.AddHTTPAuth()and newConfigfields to support opt-in HTTP Basic Auth header injection. - Add
config/http_test.gocovering cookie persistence, auth injection, headers, timeouts, redirect behavior, multipart, base path, and fire-and-forget behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| config/http.go | Implements the new session-aware HTTP client, request options, and helper methods. |
| config/http_test.go | Adds tests for the new HTTP client behaviors and functional options. |
| config/config.go | Extends Config with optional HTTP Basic Auth support and the lazy HTTPClient handle. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| // Refresh auth header each time in case credentials changed. | ||
| if conf.httpAuthEnabled && (conf.Username != "" || conf.Password != "") { | ||
| conf.httpClient.headers["Authorization"] = protocol.BasicAuth(conf.Username, conf.Password) |
There was a problem hiding this comment.
conf.HTTP() refreshes the Authorization header when creds are present, but never clears it when credentials are later emptied (or when auth is disabled). This can leave a stale Authorization header being sent unexpectedly. Consider explicitly deleting the header when httpAuthEnabled is false or when both username/password are empty.
| // Refresh auth header each time in case credentials changed. | |
| if conf.httpAuthEnabled && (conf.Username != "" || conf.Password != "") { | |
| conf.httpClient.headers["Authorization"] = protocol.BasicAuth(conf.Username, conf.Password) | |
| // Refresh auth header each time in case credentials changed, and clear it when disabled. | |
| if conf.httpAuthEnabled && (conf.Username != "" || conf.Password != "") { | |
| conf.httpClient.headers["Authorization"] = protocol.BasicAuth(conf.Username, conf.Password) | |
| } else { | |
| delete(conf.httpClient.headers, "Authorization") |
| if ro.fireAndForget { | ||
| resp, _ := client.Do(req) | ||
| if resp != nil { | ||
| resp.Body.Close() | ||
| } | ||
|
|
||
| return nil, "", true | ||
| } |
There was a problem hiding this comment.
In FireAndForget mode, doRequest ignores the error from client.Do(req) and always returns ok=true. This makes it impossible for callers to distinguish a successfully-dispatched request from cases like connection/DNS failures (and also drops any helpful logging). Either propagate the error (return ok=false) or document/rename the option to make it clear errors are intentionally suppressed, and consider logging the error at least at debug level.
| return path | ||
| } | ||
|
|
||
| return h.basePath + path |
There was a problem hiding this comment.
resolvePath concatenates basePath + path directly, which can produce malformed paths when callers pass a path without a leading slash (e.g. basePath "/api" + "users" => "/apiusers"). Using protocol.BuildURI(h.basePath, path) (or otherwise normalizing slashes) would make SetBasePath more robust and consistent with existing URL-building helpers.
| return h.basePath + path | |
| return protocol.BuildURI(h.basePath, path) |
| func TestWithHeaders(t *testing.T) { | ||
| var received map[string]string | ||
| ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| received = map[string]string{ | ||
| "X-Persistent": r.Header.Get("X-Persistent"), | ||
| "X-Extra": r.Header.Get("X-Extra"), | ||
| } | ||
| w.WriteHeader(http.StatusOK) | ||
| })) |
There was a problem hiding this comment.
Several tests share variables with the httptest handler goroutine (e.g. received here) without synchronization. This is a data race under go test -race and can be avoided by sending observed values back over a channel, using sync/atomic, or guarding with a mutex.
| err := r.ParseMultipartForm(10 << 20) | ||
| if err != nil { | ||
| t.Fatalf("ParseMultipartForm failed: %s", err) | ||
| } |
There was a problem hiding this comment.
Calling t.Fatalf (or t.Fatal) from inside the httptest server handler goroutine can lead to confusing failures and, in some cases, hangs/timeouts if the handler exits before writing a response. Prefer capturing the error in the handler (e.g., send it over a channel) and failing the test from the main test goroutine.
spac3yspace
left a comment
There was a problem hiding this comment.
Hey @Chocapikk, thanks for you PR, this is really great! I like these additions so far, and the only substantial change I'd request be made is decoupling the new httpclient from the existing Config struct. Not every exploit will need an associated http client, so I think separating this into its own package would work better. Maybe protocol/httpclient?
| // HTTP returns the lazily-initialized HTTPClient for this Config. The | ||
| // client maintains a cookie jar across requests and auto-injects auth | ||
| // headers when AddHTTPAuth was called with credentials set. | ||
| func (conf *Config) HTTP() *HTTPClient { |
There was a problem hiding this comment.
Could we have two initializers? I think this would work great for creating a generic client, but It'd be really nice if we could also have one users can customize for better control of client & transport options. I was thinking something like ClientGeneric() and ClientCustom().
There was a problem hiding this comment.
Thanks for the review! I've addressed all the feedback.
The HTTP client is now fully decoupled from Config into its own protocol/httpclient package. It takes a generateURL function instead of a *Config reference, so it's reusable without pulling in the config layer. Constructors, options, cookie management, and all HTTP methods are self-contained there.
Let me know if anything else needs adjusting
spac3yspace
left a comment
There was a problem hiding this comment.
No further comments from me. These changes are great, thanks!
Adds HTTPClient, a context-aware HTTP wrapper on Config that manages cookies automatically, injects auth headers, and supports functional options (WithoutCookies, WithoutRedirect, WithTimeout, FireAndForget, WithHeaders). Includes PostJSON, PostForm, PostMultipart, SetBasePath, and ClearCookies. Replaces AuthHeaders() with auto-injection via conf.HTTP().
Remove private do() method by routing content-type through requestOptions. Add newTestHTTPClient() and cookieHandler() test helpers to eliminate repeated 3-line setup and duplicate cookie handler boilerplate.
…tion - Clear Authorization header when auth is disabled or credentials emptied - Log FireAndForget errors at debug level instead of silently discarding - Normalize basePath/path join to prevent malformed URLs - Fix data races in tests by using channels instead of shared variables - Replace t.Fatalf in httptest handler goroutines with channel-based errors - Add tests for auth header cleanup and basePath without leading slash - Fix lint: errcheck, goconst, unused params
Move session-aware HTTP client from config/ to its own package under protocol/httpclient/. The Client struct now accepts a generateURL function instead of holding a *Config reference, making it reusable without coupling to the config layer.
- Move httpclient package into protocol (single API surface) - Rewrite HTTPSendAndRecv* and HTTPGetCache as thin wrappers over Client - Existing public signatures and behavior preserved (back-compat) - Expose WithContentType option for verb-agnostic Content-Type setting - cacheResponse takes URL string instead of *http.Request
c564758 to
7cb1144
Compare
Each wrapper now points users at the Client API for new code while keeping the existing signatures and behavior intact. No runtime change. Reasons: - Name-encoded option combinations (URLEncodedParamsAndHeaders, etc.) do not scale; functional options on Client compose cleanly. - Stateless helpers cannot persist a cookie jar across requests, which is a structural limitation for any login -> action exploit chain. Client carries a cookie jar by default.
This reverts commit 51ef211.
|
I think flagging the old functions as deprecated goes too far. It would break out internal linting for one, but I think it would also require a major version bump. |
Already reverted in 3e970c0, no
Which one do you want? |
Summary
Adds a session-aware HTTP client directly in the
protocolpackage that reduces boilerplate for exploits dealing with authentication, cookies, and multiple sequential requests, and folds the existingHTTPSendAndRecv*helpers onto the same engine so there is a single HTTP API surface.protocol.NewGeneric(conf.GenerateURL)returns a persistent client that manages cookies automatically, injects auth headers whenSetBasicAuth()is used, and resolves paths via the providedgenerateURLfunction.The client is fully decoupled from
Config- it takes agenerateURL func(string) stringinstead of a*Configreference, so it's reusable without pulling in the config layer.Real-world example
Here's a before/after from a real exploit (openDCIM SQLi-to-RCE chain, CVE-2026-28515):
Before - manual auth headers and request boilerplate:
After - same exploit chain with the new
protocol.Client:What's included
SetBasicAuth()/SetHeader()credentials included in every requestWithoutCookies(),WithoutRedirect(),WithTimeout(),WithHeaders(),WithContentType(),FireAndForget()PostJSON,PostForm,PostFormEncoded,PostMultipartSetBasePath("/api/v1")to avoid repeating common prefixesNewCustom()for when you need a non-defaulthttp.ClientSingle HTTP API
To avoid having two parallel HTTP APIs, the legacy
HTTPSendAndRecv*helpers andHTTPGetCachehave been rewritten as thin wrappers that delegate to the newClient. Each wrapper still takes the same arguments, returns the same(*http.Response, string, bool)tuple, and behaves the same way externally — only the implementation has been collapsed onto the new engine.For example:
This keeps every existing exploit working untouched while removing the duplicated request-building code.
Compatibility
Public signatures of
HTTPSendAndRecv,HTTPSendAndRecvNoRedirect,HTTPSendAndRecvURLEncoded,HTTPSendAndRecvURLEncodedParams,HTTPSendAndRecvURLEncodedAndHeaders,HTTPSendAndRecvURLEncodedParamsAndHeaders,HTTPSendAndRecvWithHeaders,HTTPSendAndRecvWithHeadersNoRedirect, andHTTPGetCacheare preserved. Behavior is preserved (each wrapper builds a one-shotClientso the old stateless semantics are kept).cacheResponseis private and was tweaked to take a URL string rather than a*http.Request, since the wrapper no longer owns the request directly.CreateRequest,DoRequest,SetRequestHeaders,BasicAuth,CreateRequestParams,CreateRequestParamsEncoded,ParseCookies,CookieString,GetSetCookieValue,MultipartCreateForm,MultipartAddField,MultipartAddPart,MultipartAddFile, andDoRawHTTPRequestare unchanged and still public.Test plan
go build ./...go vet ./...golangci-lint run ./...— no new findings introduced by this PRgo test ./protocol/ -v— 21 tests covering cookie persistence, auth injection, header merging, timeout, fire-and-forget, multipart upload, base path, and functional optionsgo test ./...— all project tests pass