Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📜 Recent review details⏰ 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). (2)
🔇 Additional comments (1)
WalkthroughThis pull request adds security linting annotations across multiple files to suppress false positives from gosec, refactors a cosmetic CSS injector using fmt.Fprintf, clarifies Windows API variable types as uintptr, and updates the golangci.yml configuration to exclude the G706 lint rule. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
filter/filter.go (1)
165-165: Slightly inaccurate justification comment on line 165.The
errhere comes fromresolveInclude(base, after), whereafteris parsed from filter list body content — which is downloaded from external URLs, making it technically external input (not just "URL parsing"). Theerr.Error()string may embed the rawaftervalue.The suppression itself is still correct (log injection via error messages from URL parsing is a very low-severity concern in this context), but the comment could be more accurate:
-log.Printf("filter: error resolving include: %v", err) // `#nosec` G706 -- err is from URL parsing, not arbitrary user input +log.Printf("filter: error resolving include: %v", err) // `#nosec` G706 -- err is from URL resolution of a filter list include directive; not from web request input🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@filter/filter.go` at line 165, Update the justification comment on the log line that logs errors from resolveInclude(base, after) to accurately state that `after` is derived from filter list body content downloaded from external URLs (i.e., external input) and that `err.Error()` may embed that raw input; keep the // `#nosec` G706 suppression but change the text to note the source and why the log is still acceptable (low-severity risk), referencing the call to resolveInclude and the log.Printf("filter: error resolving include: %v", err) site so reviewers can verify the reasoning.asset/server.go (1)
157-157: G705 suppression is correct, but the comment could be more precise.
bodyis the content of an internal asset (CSS/JS), which is correct. However, the selection key (refererURL.Hostname()) does originate from theReferer/Originrequest header, which is user-controlled. The XSS risk is still absent because the asset content is authored internally—the hostname only acts as a lookup key—but the comment "body is from internal asset storage, not user input" doesn't acknowledge this nuance.Consider refining to something like:
-w.Write(body) // `#nosec` G705 -- body is from internal asset storage, not user input +w.Write(body) // `#nosec` G705 -- body content is from internal asset storage; hostname is user-supplied but only used as a lookup key, not reflected in the response🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@asset/server.go` at line 157, Update the G705 suppression comment next to the w.Write(body) call to clearly state that the written `body` is internal asset content (CSS/JS) authored by the application and not user input, and also note that the lookup key (`refererURL.Hostname()` derived from the Referer/Origin header) is user-controlled but only used to select the asset and never incorporated into the response body or rendered without validation; place this clarified comment adjacent to the `w.Write(body)` line and, if present, near the code that computes `refererURL.Hostname()` so reviewers can see the distinction between the internal asset content and the external lookup key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@asset/server.go`:
- Line 157: Update the G705 suppression comment next to the w.Write(body) call
to clearly state that the written `body` is internal asset content (CSS/JS)
authored by the application and not user input, and also note that the lookup
key (`refererURL.Hostname()` derived from the Referer/Origin header) is
user-controlled but only used to select the asset and never incorporated into
the response body or rendered without validation; place this clarified comment
adjacent to the `w.Write(body)` line and, if present, near the code that
computes `refererURL.Hostname()` so reviewers can see the distinction between
the internal asset content and the external lookup key.
In `@filter/filter.go`:
- Line 165: Update the justification comment on the log line that logs errors
from resolveInclude(base, after) to accurately state that `after` is derived
from filter list body content downloaded from external URLs (i.e., external
input) and that `err.Error()` may embed that raw input; keep the // `#nosec` G706
suppression but change the text to note the source and why the log is still
acceptable (low-severity risk), referencing the call to resolveInclude and the
log.Printf("filter: error resolving include: %v", err) site so reviewers can
verify the reasoning.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
asset/server.gocertstore/truststore_darwin.gofilter/filter.gofilterliststore/filterliststore.gointernal/asset/cosmetic/injector.gonetworkrules/rulemodifiers/removejsconstant/removejsconstant.goproxy/proxy.goproxy/websocket.go
📜 Review details
⏰ 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 Go (macos-latest)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-29T18:08:36.912Z
Learnt from: anfragment
Repo: ZenPrivacy/zen-core PR: 40
File: filter/filter.go:209-212
Timestamp: 2026-01-29T18:08:36.912Z
Learning: Filter list rules in ad-blocking filter lists (e.g., EasyList, uBlock Origin lists) are typically short and don't exceed 300 characters in length. The default bufio.Scanner buffer size (64KB) is sufficient for parsing these rules without needing to be increased.
Applied to files:
filter/filter.go
🧬 Code graph analysis (3)
proxy/proxy.go (2)
internal/redacted/redacted_nonprod.go (1)
Redacted(10-12)internal/redacted/redacted_prod.go (1)
Redacted(8-10)
filter/filter.go (2)
internal/redacted/redacted_nonprod.go (1)
Redacted(10-12)internal/redacted/redacted_prod.go (1)
Redacted(8-10)
proxy/websocket.go (2)
internal/redacted/redacted_nonprod.go (1)
Redacted(10-12)internal/redacted/redacted_prod.go (1)
Redacted(8-10)
🔇 Additional comments (15)
proxy/websocket.go (1)
18-77: LGTM — all G706 suppressions are correctly applied.Each annotated
log.Printfpasses user-controlled host data exclusively throughredacted.Redacted(), which in production always returns"[REDACTED]", eliminating the log-injection risk that G706 flags.networkrules/rulemodifiers/removejsconstant/removejsconstant.go (2)
117-117: LGTM — G706 suppression is accurate.Using
%qforres.Request.URLcorrectly escapes all special characters (newlines, ANSI codes) that would otherwise enable log injection.
134-134: LGTM — same rationale as line 117.internal/asset/cosmetic/injector.go (1)
80-82: LGTM —fmt.Fprintfrefactor is correct.
strings.Builder.Writenever returns an error, so discardingfmt.Fprintf's return values is safe. The change is semantically identical to the previousbuilder.WriteString(fmt.Sprintf(...))and correctly addresses QF1012.filterliststore/filterliststore.go (2)
55-55: LGTM — G704 (SSRF) suppression is well-justified.The URL originates from configured filter list entries, not from arbitrary incoming request parameters, making SSRF a non-concern here.
107-107: LGTM — G706 suppression is accurate.
%qapplied to the[]bytelineproduces a double-quoted, escape-encoded Go string, neutralising any embedded newlines or control characters.asset/server.go (2)
110-110: LGTM — G706 suppression is correct.
142-142: LGTM.proxy/proxy.go (5)
142-142: LGTM — G706 suppression correct.
160-165: LGTM — G704 (SSRF) suppression is appropriate for the proxy use-case.The rationale — forwarding arbitrary requests is the proxy's core purpose — correctly distinguishes this from an inadvertent SSRF.
196-329: LGTM — all remaining G706 annotations inproxyConnectare correctly applied.Every annotated
log.Printfpasses user-controlled values exclusively throughredacted.Redacted(), eliminating the log-injection surface. The repeated use of the same suppression pattern is consistent with the rest of the codebase.
363-372: LGTM — G704 suppression atnet.Dialis appropriate.
295-295: Add G704 suppression for consistency with line 160.
p.requestTransport.RoundTrip(req)at line 295 forwards the same category of proxy traffic asp.requestClient.Do(r)at line 160, which carries//#nosecG704 -- this is a proxy; forwarding requests is its purpose. For consistency and to guard against future gosec taint analysis changes, add the same suppression to line 295.certstore/truststore_darwin.go (1)
97-140: LGTM — all four suppressions are correctly justified.
G204/G702on bothexec.Commandcalls: all arguments are either hardcoded orplistFile.Name()fromos.CreateTemp— not user-controlled input.G703onos.ReadFile/os.WriteFile: path comes fromos.CreateTemp, not from any request or user input.filter/filter.go (1)
133-177: LGTM — all G706 suppressions inparseURLare correctly applied.
%qformat oncurrentURLvalues escapes all special characters. Theredacted.Redacted()calls at lines 243 and 285 eliminate user-controlled data from logs in production.
1 participant
What does this PR do?
Fixes
golangci-linterrors:noseccomments for false positive gosec warningsQF1012by replacingWriteString(fmt.Sprintf(...))withfmt.FprintfHow did you verify your code works?
golangci-lint runWhat are the relevant issues?