fix: use html/template to resolve unsafe-quoting alert in bootstrap page renderer#47333
Conversation
…HubAppRegistrationPage (CodeQL alert #650) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Replaces manual HTML construction with context-aware html/template rendering to address unsafe quoting.
Changes:
- Adds a reusable bootstrap page template.
- Escapes form action and manifest data by context.
- Propagates template parsing and execution errors.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/bootstrap_profile_helpers.go |
Safely renders the GitHub App registration page. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Medium
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (20 additions detected, threshold is 100). |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. This change modifies only pkg/cli/bootstrap_profile_helpers.go (production code). Test Quality Sentinel analysis skipped. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: html/template migration
The change correctly resolves the CodeQL go/unsafe-quoting alert by switching from manual string concatenation to html/template, which applies context-aware escaping automatically. Implementation is clean and handles errors at both Parse and Execute steps.
Minor (non-blocking): The template is parsed on every call to renderBootstrapGitHubAppRegistrationPage. Since it's a package-level constant, consider pre-parsing once:
var bootstrapRegistrationPage = template.Must(template.New("bootstrap").Parse(bootstrapRegistrationPageTmpl))This avoids repeated parse overhead, though the call is infrequent so impact is negligible.
LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 15.5 AIC · ⌖ 5.69 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — no blocking issues, but two improvements are worth addressing before merge.
📋 Key Themes & Highlights
Issues Found
- Template re-parsed on every call (line 257) —
template.New(...).Parse(...)runs on every invocation. Move to a package-leveltemplate.Mustvar for correctness (panics at startup if invalid) and efficiency. - Missing URL edge-case test (line 262) —
html/templatesanitises theactionattribute as a URL context; a URL rejected by the sanitiser silently becomes#ZgotmplZ. The existing test doesn't exercise this path.
Positive Highlights
- ✅ Root cause properly addressed: context-aware
html/templateescaping replaces the fragile manual concatenation that triggered CodeQL alert #650. - ✅ Error from
t.Executeis now propagated correctly. - ✅ Inline anonymous struct keeps the data shape local and readable.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 25.7 AIC · ⌖ 7.58 AIC · ⊞ 6.7K
Comment /matt to run again
| return "https://github.com/settings/apps/new?state=" + state | ||
| } | ||
|
|
||
| const bootstrapRegistrationPageTmpl = `<!doctype html><html lang="en"><head><meta charset="utf-8"><title>Redirecting To GitHub App Creation</title></head><body><p>Redirecting to GitHub App creation...</p><form id="manifest-form" action="{{.Action}}" method="post"><input type="hidden" name="manifest" value="{{.Manifest}}"><noscript><button type="submit">Continue To GitHub App Creation</button></noscript></form><script>document.getElementById('manifest-form').submit();</script></body></html>` |
There was a problem hiding this comment.
[/diagnosing-bugs] Template is re-parsed on every call — this allocates a new *template.Template on every page render and silently swallows a parse error at runtime.
💡 Suggested fix: compile once at package level with template.Must
var bootstrapRegistrationPage = template.Must(
template.New("bootstrap").Parse(bootstrapRegistrationPageTmpl),
)Then in the function body replace the parse block with just:
var buf bytes.Buffer
if err = bootstrapRegistrationPage.Execute(&buf, data); err != nil {
return "", err
}template.Must panics at program startup if the template literal is malformed — a far better failure mode than a silent error returned from an HTTP handler on first use.
@copilot please address this.
| func renderBootstrapGitHubAppRegistrationPage(registrationURL string, manifest map[string]any) (string, error) { | ||
| encoded, err := json.Marshal(manifest) | ||
| if err != nil { | ||
| return "", err |
There was a problem hiding this comment.
[/tdd] The existing test checks for "name" (attribute-escaped JSON) but doesn't cover a URL with query characters (?, &, =) in registrationURL. html/template URL-normalises the action attribute — if registrationURL ever contains characters that the URL sanitiser rejects it will silently substitute #ZgotmplZ, breaking the redirect silently.
💡 Suggested test case
Add a sub-test (or extend the existing one) that:
- Passes a
registrationURLthat contains query-string characters, e.g.https://github.com/settings/apps/new?state=abc&foo=bar - Asserts the rendered
actionattribute equals the expected escaped URL - Also verifies the manifest JSON double-quotes appear as
"in the output
This guards against the #ZgotmplZ substitution and documents the expected escaping contract.
@copilot please address this.
There was a problem hiding this comment.
Three issues to fix before merging
The html/template direction is correct — replacing manual concatenation with a context-aware template engine properly resolves the CodeQL alert. However, the implementation has three problems:
🔴 Issues found (3)
-
Template re-parsed on every call (line 264):
template.New(...).Parse(...)inside the function body re-parses a constant string on each invocation. Use a package-levelvarinitialized withtemplate.Must(...). -
Silent
#ZgotmplZcorruption for unexpected URLs (line 264):html/templatesilently replaces URL-context values it deems unsafe with#ZgotmplZ, returning no error fromExecute. Add a pre-execution URL scheme check or a post-execution#ZgotmplZguard. -
htmlEscapefunction is now dead code (line 344): The only two call sites were removed by this change. The function and the"html"import should be deleted.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 43.4 AIC · ⌖ 4.85 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/bootstrap_profile_helpers.go:264
Template re-parsed on every call: template.New("bootstrap").Parse(...) executes inside the function body, re-parsing a compile-time constant string on every invocation.
<details>
<summary>💡 Suggested fix</summary>
Parse once at package initialization using template.Must:
var bootstrapRegistrationPageTmpl = template.Must(
template.New("bootstrap").Parse(`<!doctype html>...`),
)Then in the function, just call bootstrapRegistrationPageTmpl.Execute(&buf, data).
This e…
pkg/cli/bootstrap_profile_helpers.go:264
html/template silently replaces unsafe URLs with #ZgotmplZ: {{.Action}} is placed in the action attribute, which html/template treats as a URL context. If registrationURL is ever deemed unsafe (e.g. unexpected scheme), the engine silently substitutes #ZgotmplZ — producing a broken form with no error returned from t.Execute.
<details>
<summary>💡 Details and mitigation</summary>
The current registrationURL is https://github.com/settings/apps/new?state=<hex> which is saf…
pkg/cli/bootstrap_profile_helpers.go:344
htmlEscape is now dead code: the htmlEscape helper (and by extension the "html" import) is no longer called anywhere after this change removed the only two call sites.
<details>
<summary>💡 Suggested fix</summary>
Remove the dead function and the now-unused "html" import:
// Delete this:
func htmlEscape(value string) string {
return html.EscapeString(value)
}And remove "html" from the import block. The Go compiler will flag the unused import, but the dead functi…
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran pr-finisher workflow. Addressed the two unresolved bot review threads in commit |
|
🎉 This pull request is included in a new release. Release: |
CodeQL alert #650 (
go/unsafe-quoting, critical) flaggedrenderBootstrapGitHubAppRegistrationPagefor embedding JSON manifest data — which inherently contains double quotes — into a double-quoted HTML attribute via manual string concatenation.Changes
pkg/cli/bootstrap_profile_helpers.go: Replaces the one-liner manual string concatenation withhtml/template, using a package-level template constant andbytes.Bufferexecution.html/templateis context-aware: it applies HTML attribute escaping to{{.Manifest}}and URL normalization + HTML escaping to{{.Action}}, using a well-known safe API that static analysis tools can reason about correctly.