Implement cache busting #43
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
WalkthroughThis PR introduces a new asset server system for managing injected content over HTTPS, refactors injectors to expose asset retrieval APIs, consolidates filter rule handling through a unified injector interface, updates certificate generation for IP addresses, and removes legacy integration tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ 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.
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.
| if ip := net.ParseIP(host); ip != nil { | ||
| template.IPAddresses = []net.IP{ip} | ||
| } else { | ||
| template.DNSNames = []string{host} | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the GetCertificate function in certgen/get.go
fd "get.go" --path "certgen/" | head -20Repository: ZenPrivacy/zen-core
Length of output: 278
🏁 Script executed:
# Look at the file structure around lines 56-60
cat -n certgen/get.go | sed -n '1,80p'Repository: ZenPrivacy/zen-core
Length of output: 2798
🏁 Script executed:
# Find all GetCertificate call sites in the codebase
rg -n "GetCertificate" --type go -B2 -A2Repository: ZenPrivacy/zen-core
Length of output: 2127
🏁 Script executed:
# Look at the context around the GetCertificate call in proxy/proxy.go
cat -n proxy/proxy.go | sed -n '200,230p'Repository: ZenPrivacy/zen-core
Length of output: 1205
🏁 Script executed:
# Look at the context around the GetCertificate call in asset/server.go
cat -n asset/server.go | sed -n '50,75p'Repository: ZenPrivacy/zen-core
Length of output: 814
🏁 Script executed:
# Search for where the 'host' variable is assigned in proxy.go around line 214
cat -n proxy/proxy.go | sed -n '180,220p'Repository: ZenPrivacy/zen-core
Length of output: 1537
🏁 Script executed:
# Look at more context in asset/server.go to understand where 'host' comes from
cat -n asset/server.go | sed -n '1,80p'Repository: ZenPrivacy/zen-core
Length of output: 2402
🏁 Script executed:
# Also verify how net.SplitHostPort handles IPv6 addresses
python3 << 'EOF'
import subprocess
import json
# Test net.SplitHostPort behavior with IPv6
test_code = '''
package main
import (
"fmt"
"net"
)
func main() {
testCases := []string{
"[::1]:443",
"example.com:443",
"192.168.1.1:443",
"::1",
"[::1]",
}
for _, tc := range testCases {
host, port, err := net.SplitHostPort(tc)
fmt.Printf("Input: %q -> host: %q, port: %q, err: %v\n", tc, host, port, err)
ip := net.ParseIP(host)
fmt.Printf(" ParseIP(%q): %v\n", host, ip)
}
}
'''
with open('/tmp/test_split.go', 'w') as f:
f.write(test_code)
# We can't execute Go code, but we can reason about it
# Let me search for test files or documentation
EOF
# Search for any tests that might reveal the behavior
rg -n "SplitHostPort" --type go -A2 -B2Repository: ZenPrivacy/zen-core
Length of output: 336
🏁 Script executed:
# Look for where 'host' variable should come from in asset/server.go
cat -n asset/server.go | sed -n '60,100p'Repository: ZenPrivacy/zen-core
Length of output: 1463
🏁 Script executed:
# Check if ClientHelloInfo has ServerName field
rg -n "ServerName" asset/server.goRepository: ZenPrivacy/zen-core
Length of output: 45
🏁 Script executed:
# Check the git history or any recent changes
git log --oneline asset/server.go | head -5Repository: ZenPrivacy/zen-core
Length of output: 133
🏁 Script executed:
# Search for how ClientHelloInfo is typically used
rg -n "ClientHelloInfo" --type go -A2 -B2Repository: ZenPrivacy/zen-core
Length of output: 351
🏁 Script executed:
# Check if asset/server.go has a build issue
rg -n "undefined" asset/server.goRepository: ZenPrivacy/zen-core
Length of output: 45
Normalize host before IP detection to avoid invalid SANs.
The primary call site in proxy/proxy.go line 201 already extracts the host via net.SplitHostPort, so ports are stripped before passing to GetCertificate. However, for defensive programming and to protect against future callers or direct invocations, normalizing within GetCertificate is prudent. IPv6 addresses like [::1] (with brackets) or hostnames with ports could be passed directly.
Additionally, there is a critical bug in asset/server.go line 64: the variable host is undefined. The callback should extract the hostname from the *tls.ClientHelloInfo parameter using its ServerName field.
✅ Proposed fix for certgen/get.go
+ "strings"
@@
- if ip := net.ParseIP(host); ip != nil {
+ hostForSAN := host
+ if h, _, err := net.SplitHostPort(host); err == nil {
+ hostForSAN = h
+ }
+ hostForSAN = strings.Trim(hostForSAN, "[]")
+ if ip := net.ParseIP(hostForSAN); ip != nil {
template.IPAddresses = []net.IP{ip}
} else {
- template.DNSNames = []string{host}
+ template.DNSNames = []string{hostForSAN}
}🤖 Prompt for AI Agents
In `@certgen/get.go` around lines 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.
| template.New("pac").Parse(`function FindProxyForURL(url, host) { | ||
| if (host === "127.0.0.1" || host === "localhost") { | ||
| return "DIRECT"; | ||
| } |
There was a problem hiding this comment.
Cover IPv6 and 127/8 loopback hosts to avoid proxying local services.
Right now only 127.0.0.1 and exact "localhost" bypass the proxy. If a client uses 127.0.1.1, another 127/8 address, or IPv6 loopback (::1), it will still be proxied. Consider normalizing the host and using isInNet + IPv6 checks.
Suggested update
- if (host === "127.0.0.1" || host === "localhost") {
+ var hostLower = host.toLowerCase();
+ if (hostLower === "localhost" || hostLower === "localhost.") {
+ return "DIRECT";
+ }
+ if (isInNet(host, "127.0.0.0", "255.0.0.0") || host === "::1" || host === "[::1]") {
return "DIRECT";
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| template.New("pac").Parse(`function FindProxyForURL(url, host) { | |
| if (host === "127.0.0.1" || host === "localhost") { | |
| return "DIRECT"; | |
| } | |
| template.New("pac").Parse(`function FindProxyForURL(url, host) { | |
| var hostLower = host.toLowerCase(); | |
| if (hostLower === "localhost" || hostLower === "localhost.") { | |
| return "DIRECT"; | |
| } | |
| if (isInNet(host, "127.0.0.0", "255.0.0.0") || host === "::1" || host === "[::1]") { | |
| return "DIRECT"; | |
| } |
🤖 Prompt for AI Agents
In `@sysproxy/pac.go` around lines 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.
This is done to avoid the performance overhead of wrapping the response body in multiple tokenizers.
What does this PR do?
How did you verify your code works?
What are the relevant issues?