Use Content-Security-Policy: script nonce

[wxiaoguang]

Fix #305

⚠️ BREAKING ⚠️

For custom theme users only: if you used <script> tags in custom templates, you need to add the nonce attribute to them:

<script nonce="{{ctx.CspScriptNonce}}">
...
</script>

[Copilot]

Pull request overview

Adds per-request CSP script nonces and updates templates/JS to apply nonces consistently, aiming to improve compatibility with stricter Content-Security-Policy configurations (Fix #305).

Changes:

• Introduces ctx.CspScriptNonce, ctx.ScriptImport, and ctx.HeadMetaContentSecurityPolicy in TemplateContext and updates templates to use them.
• Adds nonce="{{ctx.CspScriptNonce}}" to various inline/external <script> tags and propagates the nonce when dynamically executing scripts in the PR merge box.
• Updates external markup rendering outputs/tests to include a nonce attribute on injected helper scripts.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
web_src/js/features/repo-issue-pull.ts	Propagates CSP nonce to dynamically executed scripts in the merge box reload path.
tests/integration/markup_external_test.go	Updates expected HTML to include nonce="not-needed" in injected script tags.
templates/user/dashboard/repolist.tmpl	Adds CSP nonce to inline module script.
templates/user/auth/captcha.tmpl	Adds CSP nonce to captcha provider scripts.
templates/swagger/openapi-viewer.tmpl	Adds CSP meta and switches to ctx.ScriptImport for nonce-aware script tags.
templates/status/500.tmpl	Adds CSP meta and nonce to inline script on the error page.
templates/shared/combomarkdowneditor.tmpl	Adds nonce to inline script used for editor styling toggle.
templates/repo/issue/view_content/pull_merge_box.tmpl	Adds nonce to inline module script in merge UI.
templates/repo/diff/box.tmpl	Adds nonces to inline scripts controlling diff file tree UI.
templates/base/head_script.tmpl	Adds nonce to the global inline config script and switches to ctx.ScriptImport.
templates/base/head.tmpl	Injects CSP meta into the base page head.
templates/base/footer.tmpl	Switches to ctx.ScriptImport for the main JS entry.
services/context/context_template.go	Adds CSP nonce generation, CSP meta emission, and nonce-aware script import helper.
services/context/context.go	Moves TemplateContext type definition out (now in context_template.go).
modules/util/util.go	Adds “fast” random helpers and changes CryptoRandomBytes error handling behavior.
modules/templates/helper.go	Removes legacy global ScriptImport template func implementation.
modules/markup/render.go	Adds nonce="not-needed" to external-render helper script injection.
modules/markup/external/openapi.go	Adds nonce="not-needed" to swagger script tag in generated HTML.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[silverwind]

Why these nonce="not-needed"?

[wxiaoguang]

Why these nonce="not-needed"?

Dummy attribute.

Then you grep the whole code base (or AI greps), find a lot of <script, you can know no tag nonce is missing. So it is pretty good for code maintenance.

[silverwind]

Dummy attribute.

Will browsers ignore such invalid values or could it cause issues?

[wxiaoguang]

Dummy attribute.

Will browsers ignore such invalid values or could it cause issues?

When no CSP, the nonce attribute does nothing.

[silverwind]

Fine if it's not causing issues, but I'd feel more comfortable if this would be a data-nonce="not-needed" indicator, or even better, just a comment.

[silverwind]

One must-fix from a review pass:

FastCryptoRandomBytes shares an unsynchronized rand/v2.ChaCha8 across goroutines (modules/util/util.go)

rand/v2.ChaCha8 is not documented as concurrency-safe, and its Read/Uint64 methods mutate internal state. The single instance returned by sync.OnceValue will be called concurrently by every HTTP handler rendering a page — sync.OnceValue only protects initialization, not subsequent Read calls. The race detector should flag this; in the worst case state corruption could lead to nonce collisions across requests.

Options:

• Wrap access in a sync.Mutex
• Use a sync.Pool of *ChaCha8 instances
• Just call crypto/rand.Read directly — generating a 16-byte nonce once per page render is not a hot path that needs a 20x speedup

Also worth considering while touching this area:

• _, _ = rand.Read(buf[:]) at seed time silently discards the error. If entropy is unavailable, the seed is all-zero and the generator is deterministic. panic on error would match the new style of CryptoRandomBytes right above it.
• CryptoRandomBytes was changed to panic on error but still returns (..., error) where the error is always nil. Until the signature change lands, callers designed to handle the error now crash the process instead — better to keep the old behavior until the real refactor.

---

This comment was written with the help of Claude Opus 4.6.

[wxiaoguang]

FastCryptoRandomBytes shares an unsynchronized rand/v2.ChaCha8 across goroutines (modules/util/util.go)

Done in 1dbf6f9

Fine if it's not causing issues, but I'd feel more comfortable if this would be a data-nonce="not-needed" indicator, or even better, just a comment.

Changed it to <script nonce src="...">, I think it reads better.

[wxiaoguang]

Also worth considering while touching this area:

* `_, _ = rand.Read(buf[:])` at seed time silently discards the error. If entropy is unavailable, the seed is all-zero

Hallucination, ChaCha8 never depends on entropy, it only does math calculation, never fails

And rand.Read also guarantees that it never fails.

[silverwind]

Aggregated review pass with two models (Claude Opus 4.6 + GPT-5.4). Findings where they converged or diverged:

1. CSP script-src * 'nonce-X' is too permissive (both) — services/context/context_template.go:HeadMetaContentSecurityPolicy. The * wildcard still allows any external script via src=. The nonce only blocks inline XSS; it does NOT prevent injected <script src="evil.com/x.js">. Comment "allow all by default (the same as old releases)" is honest about the trade-off, but users may expect strict CSP. Worth documenting what this protects (inline XSS, javascript: URIs) vs doesn't, or a follow-up with strict-dynamic/allowlist.

2. CryptoRandomBytes now panics but keeps (…, error) signature (both) — modules/util/util.go:88. Callers in models/auth/access_token.go, models/auth/oauth2.go (2x), models/user/user.go, models/auth/twofactor.go, services/auth/auth_token.go, models/actions/utils.go have recoverable error paths that are now unreachable — an unexpected rand.Read failure crashes the process. Either keep return buf, err, or remove the error in this PR and fix all callers.

3. math/rand/v2.ChaCha8 for CSP nonces (GPT-5.4: Critical; Opus 4.6: Low) — ChaCha8 is documented CSPRNG, seeded with 32 bytes from crypto/rand, so technically fine. But the math/rand/v2 package doc warns against security-sensitive use, which is a confusing signal. A comment on chaCha8RandPool clarifying "ChaCha8 is CSPRNG-safe, used for security-sensitive nonces" would prevent a future refactor from swapping it out.

4. <script nonce> with no value in modules/markup/render.go and modules/markup/external/openapi.go (GPT-5.4) — Creates an empty nonce="" attribute. Works as a grep marker by design per earlier comments, but if these iframe responses ever inherit a script-src 'nonce-X' CSP, scripts silently break.

5. executeScripts nonce propagation is fragile (both) — web_src/js/features/repo-issue-pull.ts:71 uses document.querySelector('script[nonce]')!.getAttribute('nonce')!. Per WHATWG spec, header-delivered CSP scrubs the attribute value to empty string (only .nonce IDL property retains it). Meta-CSP as used here doesn't trigger scrubbing, but adding a CSP header later silently breaks this. Prefer element.nonce. Also: double ! crashes if no nonce-bearing script exists, and if querySelector matches an empty-nonce script from (4), cloned scripts get empty nonce.

6. TemplateContext mutation is unsynchronized (GPT-5.4) — CspScriptNonce writes to a map[string]any without a lock. Today rendering is single-goroutine per request so no real race, but a dedicated struct field would be safer than a map entry if html/template ever parallelizes.

[wxiaoguang]

Aggregated review pass with two models (Claude Opus 4.6 + GPT-5.4). Findings where they converged or diverged:

I have no interest to read.

If you think anything is a must, please point out directly

[silverwind]

Those are high-quality reviews, you should at minimum comment them. Point 3 is likely a dupe as per discussion above. Why do you even request Copilot review when you don't want to read AI reviews?

[wxiaoguang]

Those are high-quality reviews, you should at minimum comment them. Point 3 is likely a dupe as per discussion above. Why do you even request Copilot review when you don't want to read AI reviews?

That's just bullshit. AI slop

[wxiaoguang]

Why do you even request Copilot review when you don't want to read AI reviews?

I ask AI to help to write code and review, I address my problems. I always hide the unrelated review, never waste reviewers' or author's time.

As the document said: you should post your thoughts, but not copy paste AI slop.

You are just keeping pasting AI slop, why I don't just ask my AI to review and waste time on answering your AI's bullshit?

[silverwind]

Ok with me to dismiss all except the * defaults.

[wxiaoguang]

Ok with me to dismiss all except the * defaults.

If you don't like it, show your plan, show feasible and actionable "TODOs".

Don't make nonsense comment with your guess/imagination.

[silverwind]

If you don't like it, do you really know a plan to improve it without breaking existing users?

Plan is to set self in this PR, call out the breaking change in the release notes for v1.27 so that users who run external scripts must adapt the CSP setting. As I understand you want to defer the self change to later. That may also be fine but why not do it now?

[wxiaoguang]

If you don't like it, do you really know a plan to improve it without breaking existing users?

Plan is to set self in this PR, call out the breaking change in the release notes for v1.27 so that users who run external scripts must adapt the CSP setting. As I understand you want to defer the self change to later. That may also be fine but why not do it now?

When you write and review other PRs, haven't you seen URLs like STATIC_URL_PREFIX?

You also ever said that "AssetURI" is a good name because it can be a full URL, not related to this origin.

I don't see any easy plan to make it completely right for end users.

[wxiaoguang]

Plan is to set self in this PR,

It is never the plan. It will also break a lot of users who use custom templates or external renders.

I believe you should remember this:

• PlantUML integration #18272
• Load jQuery as early as possible to support custom scripts Load jQuery as early as possible to support custom scripts #35926
• And when discussing about IIFE (Vite), I also told you jQuery is a must for user's plugins.

[wxiaoguang]

It's not all bullshit. Just filter it. The insecure * is definitely a concern I also have, it defeats the whole CSP.

I think I have answered everything in #37232 (comment).

These reviews are all bullshit.

[silverwind]

We should at least give users a opt-in ini setting so they can configure self. I'd definitely do that on my instances because I don't host 3rd-party resources and I don't want XSS. I'm already setting a self CSP via nginx but that setting would benefit users without nginx.

Do you not see the huge security benefit self would provide?

[wxiaoguang]

Do you not see the huge security benefit self would provide?

Why you assume that I don't see?

I have explained everything in a0c5b94

Does this PR make anything worse? If no, I don't think src self is in this PR's scope.

[wxiaoguang]

And, isn't this PR's purpose quite clear?

[silverwind]

It's an improvement and it won't break stuff, but we definitely should make * configurable later.

[silverwind]

Some followup ideas in #37238.

[silverwind]

Regression: #37257