CSP patching for inline styles #439
Conversation
anfragment
left a comment
anfragment
left a comment
There was a problem hiding this comment.
Thoughts on moving this into a separate package?
|
@anfragment Good call. I was thinking about this but hadn't come up with a proper name/location. Do you think a top-level |
anfragment
left a comment
anfragment
left a comment
There was a problem hiding this comment.
Overall looks good to me, mostly minor comments.
anfragment
added
in progress
|
LGTM, just wondering what coderabbit has to say |
🧩 Analysis chainI'll review the PR changes for you. Let me examine the code to provide a thorough analysis. 🏁 Script executed: Length of output: 16559 @anfragment I'll provide a thorough review of this CSP patching enhancement. Code ReviewThis is an excellent refactoring that addresses both architectural concerns and functional requirements. Here are my findings: ✅ Strengths
🔍 Technical Observations
💡 Minor Suggestions
📋 VerificationThe refactoring successfully:
This PR represents a solid improvement to the codebase architecture while extending functionality. The code is well-tested, secure, and follows Go best practices. 🏁 Script executed: Length of output: 117 Perfect! The additional verification confirms that the refactoring is complete and clean:
This validates that the migration was done thoroughly with proper attention to detail. The PR is ready and represents a high-quality improvement to the codebase architecture. ✅ Actions performedReview triggered.
|
WalkthroughGeneralizes CSP patching from scripts to both scripts and styles by introducing an inlineKind API and exported PatchHeaders. Updates injector to use csp.PatchHeaders for script injection. Adds comprehensive CSP patch tests; trims injector tests to focus on script presence, removing nonce-specific assertions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Injector
participant CSP as csp.PatchHeaders
Client->>Injector: HTTP Response (headers, body)
activate Injector
Injector->>CSP: PatchHeaders(headers, InlineScript)
activate CSP
CSP-->>Injector: nonce (maybe empty), patched headers
deactivate CSP
alt nonce returned
Injector->>Injector: Inject <script nonce="..."> into <head>
else no nonce
Injector->>Injector: Inject <script> or skip nonce
end
Injector-->>Client: Modified response
deactivate Injector
note over CSP: Also patches Report-Only if present
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/csp/patch.go (1)
117-120: Handle crypto/rand failures; ignoring the error can yield predictable nonces
rand.Read’s error is ignored. On failure/short read, nonce quality is undefined. Fail closed (panic) or propagate an error.Apply this diff:
func newCSPNonce() string { @@ - var b [18]byte // 144 bits - rand.Read(b[:]) + var b [18]byte // 144 bits + if _, err := rand.Read(b[:]); err != nil { + // Extremely unlikely; fail closed rather than emitting an invalid/empty nonce. + panic("csp: crypto/rand failed: " + err.Error()) + } return base64.StdEncoding.EncodeToString(b[:]) }internal/csp/patch_test.go (1)
1-191: Replace non-existent strings.SplitSeq usage
internal/csp/patch_test.go:176: changerawDirs := strings.SplitSeq(line, ";")to
rawDirs := strings.Split(line, ";")
♻️ Duplicate comments (2)
internal/csp/patch_test.go (2)
157-167: Avoid false positives when asserting nonce placement; parse directivesUse the same parser (
dirHasNonce) instead ofstrings.Containson the whole line.Apply this diff:
- token := "'nonce-" + nonce + "'" - found := false - for _, line := range h.Values("Content-Security-Policy") { - if strings.Contains(strings.ToLower(line), tc.wantDirective) && strings.Contains(line, token) { - found = true - break - } - } - if !found { + if !dirHasNonce(h, tc.wantDirective, nonce) { t.Errorf("%s: nonce not placed in %s; header: %s", tc.name, tc.wantDirective, strings.Join(h.Values("Content-Security-Policy"), " | ")) }
1-8: Optional: add package-level doc for csp package and PatchHeaders behaviorA brief package doc and note on returning empty nonce when no patch needed will help readers.
🧹 Nitpick comments (3)
internal/csp/patch.go (3)
81-85: Treat 'none' case-insensitivelyReal-world CSP often varies casing; use a case-insensitive check.
Apply this diff:
- if bestValue == "'none'" { + if strings.EqualFold(strings.TrimSpace(bestValue), "'none'") { newValue = nonceToken } else { newValue = bestValue + " " + nonceToken }
126-149: Make keyword matching case-insensitive in allowsInlineNormalize tokens before comparing CSP keywords to avoid casing pitfalls.
Apply this diff:
- for _, t := range tokens { - switch t { - case "'unsafe-inline'": + for _, t := range tokens { + tl := strings.ToLower(t) + switch tl { + case "'unsafe-inline'": unsafeInline = true - case "'strict-dynamic'": + case "'strict-dynamic'": if kind == InlineScript { return false } default: if isNonceOrHashSource(t) { return false } } }
25-39: Micro-opt: avoid generating a nonce if nothing will be patchedYou can pre-scan directives (using existing helpers) to decide if patching is needed before allocating a nonce. Not critical, but trims work on permissive CSPs.
Also applies to: 42-100
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
internal/csp/patch.go(8 hunks)internal/csp/patch_test.go(1 hunks)internal/scriptlet/injector.go(2 hunks)internal/scriptlet/injector_test.go(0 hunks)
💤 Files with no reviewable changes (1)
- internal/scriptlet/injector_test.go
🔇 Additional comments (3)
internal/scriptlet/injector.go (1)
11-11: LGTM: centralized CSP patching usageImporting
internal/cspand callingcsp.PatchHeaders(res.Header, csp.InlineScript)is correct; nonce handling is sound.Also applies to: 69-76
internal/csp/patch_test.go (2)
10-45: Nice coverage on “no CSP header” and “‘none’ replacement”Good behavioral checks and strict equality on the single-header case.
47-103: Solid priority tests for script/style directivesThe directive precedence logic is well exercised for both kinds.
Also applies to: 105-139
| rawDirs := strings.SplitSeq(line, ";") | ||
|
|
||
| for raw := range rawDirs { | ||
| d := strings.TrimSpace(raw) | ||
| if d == "" { | ||
| continue | ||
| } | ||
| name, value := cutDirective(d) | ||
| if name == dir && strings.Contains(value, token) { | ||
| return true |
There was a problem hiding this comment.
Tests don’t compile: use strings.Split and iterate values, not indexes
strings.SplitSeq doesn’t exist; and for raw := range rawDirs iterates indexes (and won’t type-check with TrimSpace).
Apply this diff:
- rawDirs := strings.SplitSeq(line, ";")
-
- for raw := range rawDirs {
+ rawDirs := strings.Split(line, ";")
+
+ for _, raw := range rawDirs {
d := strings.TrimSpace(raw)
if d == "" {
continue
}
name, value := cutDirective(d)
if name == dir && strings.Contains(value, token) {
return true
}
}📝 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.
| rawDirs := strings.SplitSeq(line, ";") | |
| for raw := range rawDirs { | |
| d := strings.TrimSpace(raw) | |
| if d == "" { | |
| continue | |
| } | |
| name, value := cutDirective(d) | |
| if name == dir && strings.Contains(value, token) { | |
| return true | |
| rawDirs := strings.Split(line, ";") | |
| for _, raw := range rawDirs { | |
| d := strings.TrimSpace(raw) | |
| if d == "" { | |
| continue | |
| } | |
| name, value := cutDirective(d) | |
| if name == dir && strings.Contains(value, token) { | |
| return true | |
| } | |
| } |
🤖 Prompt for AI Agents
In internal/csp/patch_test.go around lines 176 to 185, the test fails to compile
because it uses a non-existent strings.SplitSeq and iterates indexes with "for
raw := range rawDirs" (which yields ints), causing a type mismatch with
strings.TrimSpace; replace strings.SplitSeq(line, ";") with strings.Split(line,
";") and iterate values using "for _, raw := range rawDirs" so raw is a string
that can be TrimSpace'd, keeping the subsequent logic unchanged.
What does this PR do?
This PR makes
PatchCSPHeadersfunction support akindwhich is either style or script.How did you verify your code works?
Manual testing on inline scripts.
Summary by CodeRabbit
New Features
Refactor
Tests