Conversation
WalkthroughAdds an HTTPS asset server and Engine, shifts injector APIs from in-place HTML injection to per-host asset retrieval (GetAsset), centralizes document injection via a new documentInjector, converts CSP patching to a batch API with per-resource nonces, adjusts certificate generation for IP vs DNS, updates PAC, fixes an import path, and removes several injector tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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 unit tests (beta)
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)asset/server.go (1)
⏰ 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)
🔇 Additional comments (5)
✏️ Tip: You can disable this entire section by setting 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.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@certgen/get.go`:
- Around line 56-60: Normalize the host inside GetCertificate before IP
detection: strip IPv6 brackets and any port (use net.SplitHostPort with fallback
to trim surrounding '[' ']' for IPv6) then run net.ParseIP on the cleaned host
so SANs are valid; assign template.IPAddresses for parsed IPs and
template.DNSNames for the cleaned hostname. Also fix the undefined variable in
the TLS callback in asset/server.go by extracting the hostname from the
*tls.ClientHelloInfo parameter (use its ServerName field) instead of referencing
a non-existent host variable.
In `@sysproxy/pac.go`:
- Around line 11-14: The PAC template created in template.New("pac").Parse (the
FindProxyForURL function) only bypasses proxy for "127.0.0.1" and "localhost"
and should also treat all 127/8 IPv4 loopback addresses and the IPv6 loopback as
DIRECT; update the PAC logic to (1) normalize the host (strip brackets for IPv6
literals and lower-case), (2) check IPv6 loopback (e.g., host === "::1" or host
=== "[::1]"), and (3) use isInNet(host, "127.0.0.0", "255.0.0.0") to cover the
whole 127/8 range before falling back to other proxy rules in FindProxyForURL so
local services on 127.x.x.x or ::1 are not proxied.
🧹 Nitpick comments (5)
asset/server.go (3)
76-80: Useerrors.Isfor error comparison.The comparison
err != http.ErrServerClosedshould useerrors.Isto properly handle wrapped errors.♻️ Proposed fix
go func() { - if err := s.httpServer.Serve(ln); err != nil && err != http.ErrServerClosed { + if err := s.httpServer.Serve(ln); err != nil && !errors.Is(err, http.ErrServerClosed) { log.Printf("assetserver: error serving: %v", err) } }()
132-133: Consider handlingw.Writeerror.The error returned by
w.Writeis ignored. While failures here are typically unrecoverable (client disconnected), logging at debug level could aid troubleshooting.♻️ Proposed fix
w.WriteHeader(http.StatusOK) -w.Write(body) +if _, err := w.Write(body); err != nil { + log.Printf("assetserver: write error for %q: %v", r.URL.Path, err) +}
62-66: TheGetCertificatecallback ignoresClientHelloInfo.The callback always generates a certificate for the fixed
hostconstant, ignoring the actualServerNamefrom the TLS handshake. This is likely intentional since the asset server only serves on one host, but consider adding a comment to clarify this design choice.asset/engine.go (2)
117-122: Consider usinghtml/templatefor safer HTML construction.While the current implementation is likely safe (nonces are internally generated, URLs are from constants), using
html/templatewould provide automatic escaping and be more resilient to future changes.♻️ Suggested approach
// Alternative using html/template for automatic escaping import "html/template" var injectionTmpl = template.Must(template.New("injection").Parse( `<script nonce="{{.ScriptNonce}}" src="{{.ScriptletsURL}}"></script>` + `<script nonce="{{.ScriptNonce}}" src="{{.JsRuleURL}}"></script>` + // ... etc ))
139-156: Simplify error-returning branches inResolve.The
elsebranches explicitly returnerrwhich is alwaysnilat that point. Consider restructuring for clarity.♻️ Proposed fix
case scriptletsPath: - if body, err = e.scriptlets.GetAsset(hostname); err != nil { + body, err = e.scriptlets.GetAsset(hostname) + if err != nil { return "", nil, fmt.Errorf("scriptlets asset: %w", err) - } else { - return "application/javascript; charset=utf-8", body, err } + return "application/javascript; charset=utf-8", body, nil case extendedCSSPath: - if body, err = e.extendedCSS.GetAsset(hostname); err != nil { + body, err = e.extendedCSS.GetAsset(hostname) + if err != nil { return "", nil, fmt.Errorf("extended CSS asset: %w", err) - } else { - return "application/javascript; charset=utf-8", body, err } + return "application/javascript; charset=utf-8", body, nil case jsRulePath: - if body, err = e.jsRules.GetAsset(hostname); err != nil { + body, err = e.jsRules.GetAsset(hostname) + if err != nil { return "", nil, fmt.Errorf("js rules: %w", err) - } else { - return "application/javascript; charset=utf-8", body, err } + return "application/javascript; charset=utf-8", body, nil
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
asset/asset.goasset/engine.goasset/server.gocertgen/get.gocosmetic/injector_test.gocssrule/injector_test.gofilter/filter.gointernal/asset/cosmetic/cosmetic.gointernal/asset/cosmetic/injector.gointernal/asset/cosmetic/sanitizer.gointernal/asset/cosmetic/sanitizer_test.gointernal/asset/cssrule/injector.gointernal/asset/extendedcss/bundle.jsinternal/asset/extendedcss/extendedcss.gointernal/asset/extendedcss/injector.gointernal/asset/jsrule/injector.gointernal/asset/scriptlet/addrule.gointernal/asset/scriptlet/arglist.gointernal/asset/scriptlet/arglist_test.gointernal/asset/scriptlet/argsplit.gointernal/asset/scriptlet/argsplit_test.gointernal/asset/scriptlet/bundle.jsinternal/asset/scriptlet/injector.gointernal/asset/scriptlet/injector_internal_test.goscriptlet/injector_test.gosysproxy/pac.go
💤 Files with no reviewable changes (3)
- scriptlet/injector_test.go
- cssrule/injector_test.go
- cosmetic/injector_test.go
🧰 Additional context used
🧬 Code graph analysis (3)
internal/asset/cssrule/injector.go (4)
internal/asset/cosmetic/injector.go (1)
Injector(25-27)internal/asset/extendedcss/injector.go (1)
Injector(32-37)internal/asset/jsrule/injector.go (1)
Injector(19-21)internal/asset/scriptlet/injector.go (1)
Injector(26-31)
internal/asset/scriptlet/injector.go (4)
internal/asset/cosmetic/injector.go (1)
Injector(25-27)internal/asset/cssrule/injector.go (1)
Injector(26-28)internal/asset/extendedcss/injector.go (1)
Injector(32-37)internal/asset/jsrule/injector.go (1)
Injector(19-21)
internal/asset/jsrule/injector.go (4)
internal/asset/cosmetic/injector.go (1)
Injector(25-27)internal/asset/cssrule/injector.go (1)
Injector(26-28)internal/asset/extendedcss/injector.go (1)
Injector(32-37)internal/asset/scriptlet/injector.go (1)
Injector(26-31)
🔇 Additional comments (13)
asset/asset.go (1)
1-3: LGTM: clear internal host constant.internal/asset/cosmetic/cosmetic.go (1)
3-7: LGTM: internal import path aligns with refactor.internal/asset/cosmetic/injector.go (1)
57-67: LGTM: clean asset retrieval API.internal/asset/cssrule/injector.go (1)
54-64: LGTM: asset retrieval looks good.internal/asset/scriptlet/injector.go (1)
53-72: LGTM: scriptlet asset builder is clear and consistent.internal/asset/jsrule/injector.go (1)
28-30: LGTM: IIFE wrapping and asset return are consistent.Also applies to: 56-75
internal/asset/extendedcss/injector.go (1)
78-98: LGTM!The
GetAssetmethod correctly handles the case when no rules exist by returning(nil, nil), and properly builds the JavaScript asset by concatenating the bundle with a self-executing function that callswindow.extendedCSSwith JSON-encoded rules. The use ofjson.Marshalensures proper escaping of the rules string.filter/filter.go (3)
35-39: Clean interface abstraction.The consolidation into a single
documentInjectorinterface simplifies the Filter's dependencies and provides a clear contract for rule handling and injection.
114-124: LGTM!The delegation pattern is correct: try the injector first, and if it doesn't handle the rule (
handled == false), fall back to parsing as a network rule. Error propagation is appropriate.
179-182: LGTM!Treating injection errors as recoverable and logging them while continuing response processing is the right approach—asset injection failures shouldn't block the user from receiving the response.
asset/engine.go (2)
44-71: LGTM - Clean constructor with proper error handling.The constructor properly initializes all injectors and constructs asset URLs. The use of
url.Parsefollowed byJoinPathis a safe way to build URLs.
74-104: LGTM - Well-structured rule dispatch.The switch-based rule dispatch is clear and correctly gates
cssruleandjsrulebehindfilterListTrustedfor security. Each case returns(true, nil)on success or(true, err)on failure, properly indicating the rule was handled.asset/server.go (1)
43-43: Remove the concern about undefinedhostvariable—it is properly defined.The
hostconstant is defined atasset/asset.go:3as a package-level constant (const host = "127.0.0.1"), making it accessible throughout theassetpackage. Line 43 correctly references this constant.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
This is done to avoid the performance overhead of wrapping the response body in multiple tokenizers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@asset/server.go`:
- Around line 56-78: The ListenAndServe docstring says it blocks but the
implementation spawns a goroutine and returns immediately; change it so the
method actually blocks by removing the goroutine and invoking
s.httpServer.Serve(ln) directly and returning its error (handle
http.ErrServerClosed the same way as in the goroutine before returning), or
alternatively update the comment to reflect non-blocking behavior—make the
change in Server.ListenAndServe where tlsConfig, ln, and the call to
s.httpServer.Serve(ln) are used.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
asset/engine.goasset/server.gocsp/meta.gocsp/patch.gocsp/patch_test.gofilter/filter.gosysproxy/pac.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-29T18:08:27.308Z
Learnt from: anfragment
Repo: ZenPrivacy/zen-core PR: 40
File: filter/filter.go:209-212
Timestamp: 2026-01-29T18:08:27.308Z
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)
filter/filter.go (3)
networkrules/networkrules.go (1)
New(31-36)filter/whitelistserver/server.go (1)
New(22-24)internal/redacted/redacted_prod.go (1)
Redacted(8-10)
asset/server.go (1)
asset/engine.go (1)
Engine(39-51)
csp/meta.go (1)
csp/patch.go (1)
PatchOperation(24-28)
🔇 Additional comments (9)
sysproxy/pac.go (1)
11-18: Local loopback bypass coverage looks complete.
Covers localhost variants, IPv6 loopback, and 127/8 as intended.csp/meta.go (1)
14-75: Batch meta CSP patching reads clean.
Sequential operations with a “changed” guard avoid unnecessary meta rewrites.csp/patch_test.go (1)
11-75: Tests align with batch CSP patching.
Nonce insertion, unsafe-inline URL inclusion, and meta/header scenarios are all exercised.Also applies to: 131-133, 174-177, 208-212, 240-243
filter/filter.go (2)
41-90: Injector consolidation and nil-guards look good.
The unified interface and constructor checks make the injection path clearer and safer.
223-234: Injection flow delegation is clear.
Rules are routed through the injector first, and injection errors are treated as recoverable.Also applies to: 287-291
csp/patch.go (2)
30-48: Batch CSP patching flow andsafeNoncelogic look consistent.
The per-operation patching and directive prioritization read well.Also applies to: 61-126, 137-193
50-58: No changes needed –crypto/rand.Read()never returns errors.Per Go's design,
rand.Read()is specified to always fill the buffer and never return an error. If the underlying OS RNG fails,rand.Read()crashes the program irrecoverably rather than returning an error. Error handling at the call site is unnecessary and not meaningful. The current implementation is correct.Likely an incorrect or invalid review comment.
asset/engine.go (1)
53-147: Engine wiring and CSP patch ops look cohesive.
Centralized injector routing plus batched CSP patching is clean.asset/server.go (1)
90-162: ServeHTTP flow and headers look good.
Method checks, referer parsing, andCache-Control: no-storealign with the cache-busting design.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
spellcascade
left a comment
spellcascade
left a comment
There was a problem hiding this comment.
LGTM. I've left a minor comment, it's up to you
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@asset/server.go`:
- Around line 90-100: The ServeHTTP method on Server currently sets overly
permissive CORS headers (Access-Control-Allow-Origin: "*",
Access-Control-Allow-Headers: "*", Access-Control-Allow-Methods: "GET, OPTIONS")
which allows any site to read asset contents; update ServeHTTP to remove or
restrict these headers (remove the wildcard headers or replace with a
configurable trusted-origin check) and preserve only necessary behavior for
OPTIONS preflight handling in ServeHTTP so normal <script>/<link> asset loading
still works without exposing asset contents to all origins.
In `@csp/patch.go`:
- Around line 30-47: The doc comment for PatchHeadersBatch is incorrect: it
claims the function "Returns the generated patch operations with nonces" whereas
PatchHeadersBatch(res *http.Response, operations []PatchOperation) only consumes
the provided operations and returns an error. Update the comment above
PatchHeadersBatch to accurately describe that it mutates CSP headers and meta
tags for multiple resources using the supplied operations (type PatchOperation)
and returns an error if patchMetaCSPsBatch fails, removing any wording that
implies it returns operations or nonces.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
asset/server.gocsp/patch.go
🧰 Additional context used
🧬 Code graph analysis (1)
asset/server.go (1)
asset/engine.go (1)
Engine(39-51)
🔇 Additional comments (9)
csp/patch.go (7)
16-28: Clear Script/Style tagging and PatchOperation shape.
The exported constants and structured operation payload make call sites explicit.
61-74: LGTM: header patching keeps multi-value headers intact.
The delete-and-readd flow preserves multiple header values cleanly.
76-125: LGTM: directive selection and token append logic are clear.
The specificity selection plus appendToken usage reads well.
136-150: LGTM: safeNonce preserves semantics when unsafe-inline is present.
This avoids inadvertently changing CSP behavior.
163-169: LGTM: appendToken handles empty/'none' lists correctly.
Simple and predictable behavior.
171-192: LGTM: directive priority matches script/style specificity.
The ordering logic looks correct for elem vs fallback directives.
50-58: The review comment is based on an incorrect premise aboutcrypto/rand.Readbehavior.According to Go's
crypto/randdocumentation,Read()does not return an error in normal operation. The docs explicitly state: "Read fills b … It never returns an error, and always fills b entirely." If an underlying read failure occurs on legacy systems, the process will panic rather than return an error. Therefore, ignoring the error return value is not a security concern—there is no error to handle in practical scenarios.The current code is correct as-is. The proposed change to return
(string, error)would add unnecessary complexity and require updating call sites inasset/engine.goand test files without providing any actual safety benefit.Likely an incorrect or invalid review comment.
asset/server.go (2)
62-79: TLS listener setup looks good.Min TLS version and dedicated listener with
http.Server.Serveis solid.
81-88: Graceful shutdown guard is fine.Nil checks and
Shutdown(ctx)are appropriate.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@csp/patch.go`:
- Around line 49-57: Change NewNonce to return (string, error) and handle the
error from rand.Read: in func NewNonce() call rand.Read(b[:]) and if it returns
a non-nil error return "", err, otherwise return the base64 nonce and nil; then
update all call sites to propagate the error—specifically update the Inject
method in asset/engine.go to accept and forward the nonce error (Inject already
returns error) and modify the five places it calls NewNonce to handle the error,
and update the nine test call sites in csp/patch_test.go to handle the new
(string, error) result (fail the test or use t.Fatal on error).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
csp/patch.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 Go (windows-latest)
🔇 Additional comments (6)
csp/patch.go (6)
16-28: LGTM:tagKindandPatchOperationare clear and aligned with batch patching.
30-47: LGTM: batch header patching flow is straightforward.
60-124: LGTM: per-operation header policy patching is consistent.
135-149: LGTM:safeNoncelogic is aligned with CSP3 precedence.
162-168: LGTM:appendTokenhandles'none'correctly.
170-191: LGTM: directive priority ordering is sensible.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
CORS is only triggered by a specific subset of cross-origin requests. <style> and <link> tags do not trigger CORS: https://developer.mozilla.org/en-US/docs/Web/HTTP/Guides/CORS#what_requests_use_cors
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@asset/server.go`:
- Around line 27-40: The NewServer function references an undefined host
variable when building addr and later certificate selection; update NewServer to
accept a host parameter (e.g., add host string to the signature of NewServer)
and use that host when calling net.JoinHostPort and when passing values to
certGenerator-related calls (keep checks for port, engine, certGenerator
intact); update all callers of NewServer to provide the host argument so
compilation and correct cert selection succeed.
- Around line 98-107: The handler currently rejects requests when r.Referer() is
empty; instead, fall back to using the Origin request header before returning
400: if r.Referer() is empty, read raw = r.Header.Get("Origin") and if that is
non-empty parse it into refererURL (same variable) and handle parse errors
similarly (log with raw and return invalid referer); only return missing referer
error if both Referer and Origin are absent or Origin parse fails. Update the
code paths that reference refererURL so they work unchanged when Origin was
used, and add/adjust tests to verify behavior when Referer is stripped but
Origin is present.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
asset/server.go
🧰 Additional context used
🧬 Code graph analysis (1)
asset/server.go (2)
asset/engine.go (1)
Engine(39-51)internal/ruletree/byteset/byteset.go (1)
Set(4-4)
⏰ 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 (windows-latest)
🔇 Additional comments (2)
asset/server.go (2)
15-25: Clean interface + struct definition.
Looks good and keeps the server state tight.
81-88: Stop implementation is safe and minimal.
Nil checks + graceful shutdown are solid.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
2 participants
What does this PR do?
<script>and<link>tags instead of inline elements, enabling instant cache invalidation.asset.Engine, which manages downstream asset stores.asset.Server, which hosts the injected assets on a local HTTPS server.How did you verify your code works?
Manual testing in Zen.
What are the relevant issues?
Resolves #39.