fix: address CodeRabbit review findings for PR #419#421
Merged
fredcamaral merged 9 commits intodevelopfrom Mar 30, 2026
Merged
Conversation
Address 21 unresolved CodeRabbit review findings across certificate, systemplane, webhook, DLQ, and idempotency packages: - certificate: defensive copies in GetCertificate/TLSCertificate, nil-receiver safety for TLSCertificate, expanded DaysUntilExpiry godoc - systemplane: nil-safety guards for SnapSettingInt/SnapSettingBool, consolidate redundant state loads in supervisor reload, robust factory assertion in backend test, duplicate env var detection in catalog validation - webhook: redact URL userinfo in logs, versioned HMAC signature format (v1) with backward-compatible migration path, DNS-free scheme validation tests, preserve original authority for HTTP Host header, defensive filter comment, consistent metric reporting with result.Attempts - dlq: clone sources slice in WithSources, reject negative MaxRetries, validate tenant key segment before Redis routing - idempotency: binary-safe cached response with headers, fail-open error handling in handleDuplicate, accurate package documentation
- Extract validateEnqueueMessage, stampInitialEnqueue, and resolveAndValidateTenant helpers from Handler.Enqueue to bring cyclomatic complexity from 18 down to 7 - Add missing "errors" import in idempotency package for errors.Is calls
|
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:
WalkthroughThis change set touches many commons packages. Notable factual changes: certificate Manager returns deep-cloned x509.Certificate objects; DLQ consumer clones source slices; DLQ Handler splits enqueue into validation, stamping, and tenant-resolution helpers; idempotency middleware now stores response bodies as bytes, persists selected headers, adds Redis fail-open reads and header replay; a new commons/security/ssrf package implements SSRF validation and DNS pinning and callers delegate to it; webhook deliverer adds versioned HMAC (v0/v1) and verification helpers plus hardened URL sanitization; catalog validation now returns ValidationResult including EnvVarConflicts; assorted tests and docs updated. Sequence Diagram(s)sequenceDiagram
participant Client
participant IdemMiddleware as Idempotency Middleware
participant Redis
participant Handler as Downstream Handler
Client->>IdemMiddleware: Request (may include X-Idempotency-Key)
activate IdemMiddleware
IdemMiddleware->>Redis: GET lock/key
activate Redis
Redis-->>IdemMiddleware: lock status or error
deactivate Redis
alt Redis read error (fail-open)
IdemMiddleware->>Handler: Invoke handler (bypass replay)
activate Handler
Handler-->>IdemMiddleware: Response (status, body, headers)
deactivate Handler
IdemMiddleware-->>Client: Response
else Lock indicates in-flight
IdemMiddleware-->>Client: 409 (IDEMPOTENCY_CONFLICT)
else Lock free / no cached response
IdemMiddleware->>Handler: Invoke handler
activate Handler
Handler-->>IdemMiddleware: Response (status, body, headers)
deactivate Handler
alt Success and body size within max cache
IdemMiddleware->>IdemMiddleware: Capture body ([]byte) and selected headers
IdemMiddleware->>Redis: PIPELINE SET key:response, SET lock expiry
activate Redis
Redis-->>IdemMiddleware: OK
deactivate Redis
IdemMiddleware-->>Client: Response (replayable)
else Failure
IdemMiddleware->>Redis: DELETE key & lock
activate Redis
Redis-->>IdemMiddleware: OK
deactivate Redis
IdemMiddleware-->>Client: Error
end
end
deactivate IdemMiddleware
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
staticcheck SA1019: fasthttp Header.VisitAll is deprecated in favor of the range-compatible All() iterator.
Member
Author
|
@CodeRabbit -- review again, please |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@commons/net/http/idempotency/idempotency.go`:
- Around line 254-260: The replay loop currently uses c.Set(name, v) which
overwrites previous values and drops multi-value headers; update the loop that
iterates over resp.Headers to append each value instead of setting—replace the
c.Set(name, v) call with a call that adds the header (e.g.,
c.Response().Header.Add(name, v)) so headers like Set-Cookie, Location, ETag,
etc. are preserved across multiple values when replaying responses.
- Around line 252-267: The json.Unmarshal failure for variable resp
(cachedResponse) is currently ignored; update the idempotency replay path in
idempotency.go to log a warning including the unmarshal error and the cached
payload key/identifier before falling through so we preserve fail-open
semantics—locate the block that unmarshals into resp and add a processLogger or
package logger call (including the error and relevant cache key/ctx info)
immediately when unmarshalErr != nil, then continue to the existing fallback
that returns the generic "already processed" response.
- Around line 308-319: Replace the deprecated VisitAll call with iterating over
c.Response().Header.All() and append values exactly like before: call pairs :=
c.Response().Header.All() and loop over the slice in steps of two (key :=
pairs[i], value := pairs[i+1]), convert key to string to switch on header names
("Content-Type", "Content-Length", "Transfer-Encoding",
chttp.IdempotencyReplayed") and append string(value) into headers[name] when not
skipped; update the code around c.Response().Header.VisitAll to use this All()
iteration pattern so types match and behavior remains identical.
In `@commons/systemplane/catalog/validate.go`:
- Around line 170-177: The current logic keeps the first mapping in envIndex
when a duplicate env var is found, causing resolveSharedKey to incorrectly match
ambiguous env vars; update the duplicate handling in the duplicate-detection
block (where envIndex, EnvVarConflict and sk.Key are used) to remove the env var
from envIndex (or record it in a separate ambiguous set) immediately when you
append the EnvVarConflict so that future resolveSharedKey lookups will not
return the original mapping for that env var and will treat it as ambiguous.
- Around line 123-130: The loop that appends envConflicts is misusing the
Mismatch type (in the for _, conflict := range envConflicts block), inserting
catalog-vs-catalog keys into ProductKey/ProductValue and bypassing vc.shouldSkip
filtering; change this by either creating a dedicated result type (e.g.,
EnvVarDuplicate or CatalogConflict) and appending that instead of Mismatch, or
by passing each conflict through the existing mismatch-generation path that
calls vc.shouldSkip/WithIgnoreFields("EnvVar") before appending so filtering
behavior and Mismatch.String() remain correct; update the code that consumes
these results to handle the new type if you add one, or reuse the same Mismatch
creation function so existing callers/filters continue to work.
In `@commons/webhook/deliverer_test.go`:
- Around line 958-976: The test TestComputeHMACv1 currently only checks prefix
and determinism so it can pass even if the internal wire format is wrong; update
it to assert against a known, independently computed v1 signature string for the
chosen payload/timestamp/secret (use computeHMACv1 inputs: payload :=
[]byte(`{"id":"123"}`), timestamp := int64(1700000000), secret :=
"test-secret")—replace or add an assertion that sig ==
"<externally-computed-expected-signature>" to lock the exact "v1,sha256=<hex>" /
format behavior of computeHMACv1 and fail if the timestamp/payload concatenation
or encoding differs.
In `@commons/webhook/deliverer.go`:
- Around line 147-155: The godoc for WithSignatureVersion incorrectly references
VerifySignatureV1; update the comment to point to the actual exported verifier
helpers VerifySignature and VerifySignatureWithFreshness and clarify that
SignatureV1 produces the versioned "v1,sha256=..." signature string enabling
replay protection which receivers can enforce using VerifySignatureWithFreshness
(or VerifySignature for basic verification). Edit the paragraph mentioning
SignatureV1 to reference those two functions (VerifySignature,
VerifySignatureWithFreshness) and adjust phrasing to indicate which verifier
provides freshness/replay protection.
- Around line 741-756: The sanitizeURL function currently clears query and
userinfo but leaves URL fragments, so sensitive data in fragments (e.g., access
tokens) can leak via u.String(); update sanitizeURL to also clear the fragment
by setting u.Fragment = "" before returning, and add/adjust the TestSanitizeURL
test case to assert that fragments are removed (e.g., include a URL with
`#fragment` and expect the placeholder-free sanitized result without the
fragment).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4f2c383b-9b08-4b5b-b5bd-c2c90c8b6dc5
📒 Files selected for processing (16)
commons/certificate/certificate.gocommons/certificate/certificate_test.gocommons/dlq/consumer.gocommons/dlq/handler.gocommons/net/http/idempotency/doc.gocommons/net/http/idempotency/idempotency.gocommons/systemplane/bootstrap/backend_test.gocommons/systemplane/catalog/validate.gocommons/systemplane/domain/setting_helpers.gocommons/systemplane/service/supervisor.gocommons/systemplane/service/supervisor_helpers.gocommons/webhook/deliverer.gocommons/webhook/deliverer_test.gocommons/webhook/doc.gocommons/webhook/ssrf.gocommons/webhook/ssrf_test.go
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
commons/net/http/idempotency/idempotency.go (2)
254-260:⚠️ Potential issue | 🟠 MajorMulti-value headers are silently overwritten during replay.
c.Set(name, v)overwrites any previous value for the same header name. For headers that legitimately have multiple values (e.g.,Set-Cookie), only the last value will be replayed.Use
c.Response().Header.Add()to append values:🐛 Proposed fix
for name, values := range resp.Headers { for _, v := range values { - c.Set(name, v) + c.Response().Header.Add(name, v) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commons/net/http/idempotency/idempotency.go` around lines 254 - 260, The loop that replays persisted headers (for name, values := range resp.Headers { ... }) uses c.Set(name, v), which overwrites previous values and drops multi-value headers like Set-Cookie; change the body to append each value instead by calling the response header Add method (e.g., c.Response().Header.Add(name, v)) so all values are preserved when replaying headers in idempotency.go.
252-253:⚠️ Potential issue | 🟡 MinorSilent unmarshal failure masks cache corruption.
When
json.Unmarshalfails (line 253), the error is silently ignored and the request falls through to the generic "already processed" response. This could mask data corruption or schema evolution issues without any observability.Consider logging a warning to aid debugging while preserving fail-open semantics:
📝 Proposed fix
var resp cachedResponse - if unmarshalErr := json.Unmarshal([]byte(cached), &resp); unmarshalErr == nil { + if unmarshalErr := json.Unmarshal([]byte(cached), &resp); unmarshalErr != nil { + m.logger.Log(ctx, log.LevelWarn, + "idempotency: failed to unmarshal cached response, treating as unavailable", + log.String("key", responseKey), log.Err(unmarshalErr), + ) + } else { // Replay persisted headers first...Based on learnings: "Do not swallow errors; return or handle errors with context"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commons/net/http/idempotency/idempotency.go` around lines 252 - 253, When json.Unmarshal into cachedResponse (the variable resp) fails for the cached string (variable cached), don’t silently ignore it: add a warning log that includes the unmarshal error and relevant context (e.g., the cached payload or its key) before preserving the current fail-open behavior; update the error branch of the json.Unmarshal check (the else path where unmarshalErr != nil) to call the package logger (or log.Printf/log.Warnf) with the error and cached content so cache corruption or schema drift is observable while still returning the existing "already processed" behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@commons/net/http/idempotency/idempotency.go`:
- Around line 308-319: The loop variable named `key` inside the response header
iteration shadows the function parameter `key`; rename the loop variable (e.g.,
to `hdrKey` or `headerName`) wherever it appears in the for-range over
c.Response().Header.All() — update the usages string(key) and switch on name
accordingly — so the function parameter `key` is not shadowed and readability is
improved (references: the for-range over c.Response().Header.All(), the switch
on name, and the headers[name] = append(...) line).
---
Duplicate comments:
In `@commons/net/http/idempotency/idempotency.go`:
- Around line 254-260: The loop that replays persisted headers (for name, values
:= range resp.Headers { ... }) uses c.Set(name, v), which overwrites previous
values and drops multi-value headers like Set-Cookie; change the body to append
each value instead by calling the response header Add method (e.g.,
c.Response().Header.Add(name, v)) so all values are preserved when replaying
headers in idempotency.go.
- Around line 252-253: When json.Unmarshal into cachedResponse (the variable
resp) fails for the cached string (variable cached), don’t silently ignore it:
add a warning log that includes the unmarshal error and relevant context (e.g.,
the cached payload or its key) before preserving the current fail-open behavior;
update the error branch of the json.Unmarshal check (the else path where
unmarshalErr != nil) to call the package logger (or log.Printf/log.Warnf) with
the error and cached content so cache corruption or schema drift is observable
while still returning the existing "already processed" behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: af52c639-968e-46fc-8e18-a63a2a9e7297
📒 Files selected for processing (1)
commons/net/http/idempotency/idempotency.go
- idempotency: use Header.Add for multi-value replay, log unmarshal failures, rename shadowed loop variable - catalog: introduce ValidationResult to separate env var conflicts from Mismatch, mark duplicate env vars as ambiguous in index - webhook: strip URL fragments in sanitizeURL, fix godoc references to VerifySignature/VerifySignatureWithFreshness, pin TestComputeHMACv1 to independently computed expected value
Member
Author
Round 2 — Fixes Applied SuccessfullyFixed 5 file(s) based on 8 unresolved review comment(s). Files modified:
Commit: All 6,373 unit tests pass. Lint clean (0 issues). |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
commons/systemplane/catalog/validate_options_test.go (1)
101-136: 🧹 Nitpick | 🔵 TrivialAdd tests for
EnvVarConflictsdetection and handling.The
EnvVarConflictfeature inbuildCatalogIndexesdetects and tracks duplicate env var mappings but lacks dedicated test coverage. This includes verification of:
- Detection of the first duplicate env var mapping (when a duplicate is found in
envIndex)- Handling of subsequent conflicts for already-ambiguous env vars (when
ambiguousEnvVarsis set)- Confirmation that ambiguous env vars are excluded from
envIndexso they don't resolve during validationCurrent tests in
validate_test.gomanually check for duplicates viaTestAllSharedKeys_NoDuplicateEnvVarsbut do not verify the returnedresult.EnvVarConflictsfield itself. Tests invalidate_options_test.gofocus only onMismatches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commons/systemplane/catalog/validate_options_test.go` around lines 101 - 136, Tests are missing coverage for EnvVarConflicts produced by buildCatalogIndexes: add unit tests that call buildCatalogIndexes (or indirectly via ValidateKeyDefsWithOptions) with shared key sets that create duplicate EnvVar entries and assert the returned result.EnvVarConflicts contains the first detected conflict, that further duplicates are tracked as ambiguousEnvVars (i.e., subsequent conflicts mark the same env var as ambiguous), and that ambiguous env vars are not present in the returned envIndex so they cannot resolve during validation; reference functions/types: buildCatalogIndexes, EnvVarConflicts, envIndex, ambiguousEnvVars, ValidateKeyDefsWithOptions, and add equivalents to the tests in validate_options_test.go to assert result.EnvVarConflicts and absence from envIndex.
♻️ Duplicate comments (1)
commons/net/http/idempotency/idempotency.go (1)
315-329:⚠️ Potential issue | 🟠 MajorThe multi-value header fix is still lossy on the write side.
This loop appends one serialized
valueper header name, so repeated headers can already be flattened before they reachresp.Headers.Header.Addon replay won't recover separateSet-Cookielines once they're collapsed here; capture the response with an API that yields every emitted header occurrence. A regression test with twoSet-Cookievalues would make this path explicit.In the Fiber/fasthttp version used by this repository, what API should be used to enumerate every response header occurrence—especially repeated Set-Cookie headers—without flattening them into a single value per header name?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commons/net/http/idempotency/idempotency.go` around lines 315 - 329, The current loop uses c.Response().Header.All() which flattens repeated headers; instead iterate every emitted header occurrence via c.Response().Header.VisitAll (or Header.VisitAll for fasthttp) to capture each key/value pair separately (so duplicate Set-Cookie entries are preserved), skip the same middleware-managed names (Content-Type, Content-Length, Transfer-Encoding, chthtp.IdempotencyReplayed), and append each visited value to the headers map; add a regression test that returns two Set-Cookie headers to assert both are recorded and replayed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@commons/net/http/idempotency/idempotency.go`:
- Around line 228-250: The code treats a missing Redis key (errors.Is(...,
redis.Nil)) as if the operation is complete and later may return a cached 200
even when no completion marker exists; update the logic around client.Get(ctx,
key).Result() and client.Get(ctx, responseKey).Result() so that when Get returns
redis.Nil after a lost SetNX race you do NOT consider the request "complete" —
instead re-enter the acquisition/retry path or fail open (call c.Next()) rather
than mapping to a successful cached response; adjust handling in the blocks
around keyValue/keyErr and cached/cacheErr (and the analogous block at 280-296)
to explicitly detect redis.Nil and either retry acquiring the idempotency marker
or return c.Next() rather than treating it as a completed response.
- Around line 231-234: The WARN logs currently pass the raw idempotency key into
m.logger.Log (e.g., the calls that include log.String("key", key) around
m.logger.Log), which leaks high-cardinality client/tenant data; replace those
raw key fields with a deterministic surrogate (e.g., compute a fast hash like
SHA-256 or HMAC of the key and log only a truncated hex of that hash or a
fixed-length prefix) and use that surrogate in the log.String("key", ...)
argument; apply the same change to the other occurrences mentioned (the
m.logger.Log calls around the ranges corresponding to lines 245-248 and 257-260)
so no raw idempotency key is emitted.
---
Outside diff comments:
In `@commons/systemplane/catalog/validate_options_test.go`:
- Around line 101-136: Tests are missing coverage for EnvVarConflicts produced
by buildCatalogIndexes: add unit tests that call buildCatalogIndexes (or
indirectly via ValidateKeyDefsWithOptions) with shared key sets that create
duplicate EnvVar entries and assert the returned result.EnvVarConflicts contains
the first detected conflict, that further duplicates are tracked as
ambiguousEnvVars (i.e., subsequent conflicts mark the same env var as
ambiguous), and that ambiguous env vars are not present in the returned envIndex
so they cannot resolve during validation; reference functions/types:
buildCatalogIndexes, EnvVarConflicts, envIndex, ambiguousEnvVars,
ValidateKeyDefsWithOptions, and add equivalents to the tests in
validate_options_test.go to assert result.EnvVarConflicts and absence from
envIndex.
---
Duplicate comments:
In `@commons/net/http/idempotency/idempotency.go`:
- Around line 315-329: The current loop uses c.Response().Header.All() which
flattens repeated headers; instead iterate every emitted header occurrence via
c.Response().Header.VisitAll (or Header.VisitAll for fasthttp) to capture each
key/value pair separately (so duplicate Set-Cookie entries are preserved), skip
the same middleware-managed names (Content-Type, Content-Length,
Transfer-Encoding, chthtp.IdempotencyReplayed), and append each visited value to
the headers map; add a regression test that returns two Set-Cookie headers to
assert both are recorded and replayed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 816a5af3-8604-4e34-bb14-f5b68c9560cf
📒 Files selected for processing (5)
commons/net/http/idempotency/idempotency.gocommons/systemplane/catalog/validate.gocommons/systemplane/catalog/validate_options_test.gocommons/webhook/deliverer.gocommons/webhook/deliverer_test.go
Introduce commons/security/ssrf as the single source of truth for SSRF
protection across all Lerian services. Consolidates two internal,
duplicated implementations into one exported package.
New exported API:
- IsBlockedIP(net.IP) / IsBlockedAddr(netip.Addr): IP-level blocking
with canonical CIDR blocklist (8 ranges + stdlib predicates)
- IsBlockedHostname(hostname): hostname-level blocking for localhost,
cloud metadata endpoints, .local/.internal/.cluster.local suffixes
- BlockedPrefixes(): returns copy of CIDR blocklist for auditing
- ValidateURL(ctx, url, opts...): scheme + hostname + IP validation
without DNS resolution
- ResolveAndValidate(ctx, url, opts...): DNS-pinned validation with
TOCTOU elimination, returns ResolveResult{PinnedURL, Authority,
SNIHostname}
- Functional options: WithHTTPSOnly, WithAllowPrivateNetwork,
WithLookupFunc, WithAllowHostname
- Sentinel errors: ErrBlocked, ErrInvalidURL, ErrDNSFailed
Refactored consumers:
- commons/webhook/ssrf.go: resolveAndValidateIP delegates to
ssrf.ResolveAndValidate, removed duplicated isPrivateIP/CIDR blocklist
- commons/net/http/proxy_validation.go: isUnsafeIP delegates to
ssrf.IsBlockedIP, removed duplicated blockedProxyPrefixes
Canonicalized on netip.Prefix (modern Go) with net.IP bridge for
legacy callers. All tests hermetic via WithLookupFunc injection.
Add repository shape entry, API invariants section, and other-packages bullet for the new canonical SSRF validation package.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
commons/webhook/ssrf_test.go (1)
39-62: 🧹 Nitpick | 🔵 TrivialDuplicate test coverage for scheme validation.
TestResolveAndValidateIP_UnsupportedSchemes(lines 39-62) andTestResolveAndValidateIP_InvalidScheme(lines 68-92) test overlapping schemes (gopher,file,ftp). Consider consolidating these into a single table-driven test to reduce redundancy.Also applies to: 68-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commons/webhook/ssrf_test.go` around lines 39 - 62, Consolidate the overlapping scheme tests by merging the cases from TestResolveAndValidateIP_UnsupportedSchemes and TestResolveAndValidateIP_InvalidScheme into a single table-driven test (e.g., TestResolveAndValidateIP_InvalidOrUnsupportedSchemes) that runs t.Parallel and iterates over all scheme cases, calls resolveAndValidateIP(context.Background(), url), and asserts error is ErrSSRFBlocked; remove the duplicate test function to avoid redundant coverage and keep references to the existing helper resolveAndValidateIP and the ErrSSRFBlocked check so test behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@commons/security/ssrf/validate.go`:
- Around line 41-42: The ValidateURL function currently silences the unused ctx
via "_ = ctx" with a "reserved for future use" comment; replace that with a TODO
pointing to a tracking reference (e.g. TODO: integrate tracing/metrics -
ISSUE-1234) and keep ctx to preserve the API surface, or if you prefer to
acknowledge intent more explicitly, add a comment like "// TODO(OWNERS): use ctx
for tracing/metrics - ISSUE-1234" next to ctx usage; ensure the symbol
ValidateURL and parameter name ctx remain unchanged so callers/ABI are stable.
- Around line 126-147: The implementation currently fails fast on the first
blocked IP (see IsBlockedAddr check in the loop) which contradicts the godoc
that says "Validate ALL resolved IPs"; update the godoc (the comment near the
function in validate.go) to state that validation rejects the request if any
resolved IP is in a blocked range (fail-on-any-blocked-IP), referencing the
behavior of cfg.allowPrivate and IsBlockedAddr so the documentation matches the
actual logic.
---
Outside diff comments:
In `@commons/webhook/ssrf_test.go`:
- Around line 39-62: Consolidate the overlapping scheme tests by merging the
cases from TestResolveAndValidateIP_UnsupportedSchemes and
TestResolveAndValidateIP_InvalidScheme into a single table-driven test (e.g.,
TestResolveAndValidateIP_InvalidOrUnsupportedSchemes) that runs t.Parallel and
iterates over all scheme cases, calls resolveAndValidateIP(context.Background(),
url), and asserts error is ErrSSRFBlocked; remove the duplicate test function to
avoid redundant coverage and keep references to the existing helper
resolveAndValidateIP and the ErrSSRFBlocked check so test behavior remains
identical.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 50a8ea93-75b5-4d81-91af-80b60aa7b674
📒 Files selected for processing (10)
AGENTS.mdcommons/net/http/proxy_validation.gocommons/security/ssrf/doc.gocommons/security/ssrf/hostname.gocommons/security/ssrf/options.gocommons/security/ssrf/ssrf.gocommons/security/ssrf/ssrf_test.gocommons/security/ssrf/validate.gocommons/webhook/ssrf.gocommons/webhook/ssrf_test.go
X-Lerian-Ref: 0x1
Remove duplicate test functions (InvalidScheme was duplicate of BlockedSchemes, PrivateIP replaced by simpler BlockedHostname). Add dedicated TestMapSSRFError covering all four sentinel translation branches. Upgrade assert.Error to require.Error for fail-fast on nil errors. X-Lerian-Ref: 0x1
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 `@commons/security/ssrf/ssrf.go`:
- Around line 61-84: IsBlockedAddr currently lets the zero-value netip.Addr{}
through; update IsBlockedAddr (and the similar block at lines 92-103) to
immediately treat invalid addresses as blocked by returning true when
addr.IsValid() is false (i.e., if !addr.IsValid() { return true }). Keep the
existing Unmap(), predicate checks, and blockedPrefixes loop unchanged so all
other logic still runs for valid addresses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 62796042-05d9-4b6e-9abe-75985fa3e9df
📒 Files selected for processing (2)
commons/security/ssrf/ssrf.gocommons/webhook/ssrf_test.go
Member
Author
Fixes Applied SuccessfullyFixed 4 file(s) based on 6 unresolved review comment(s). Issues addressed:
Files modified:
Commit: The latest autofix changes are on the |
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.
Summary
Key changes:
GetCertificate/TLSCertificate, nil-receiver safety, expanded godocsTest plan
make ci— full local CI pipeline passes