fix(sanitize): preserve Markdown body fidelity on read surfaces - #3177
Merged
Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The sanitizer still has source-corruption and URL-masking edge cases that weaken its fidelity and security guarantees.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 4
New issues introduced by this change (4)
| Severity | Finding |
|---|---|
 |
pkg/sanitize/sanitize.go — This fallback escapes every ampersand, including those inside inline, fenced, and indented code.… |
|
pkg/sanitize/sanitize.go — The bare-URL detector treats every alphabetic scheme:// token as safely visible, although the… |
|
pkg/sanitize/sanitize.go — These loops cast individual UTF-8 bytes to runes, so Unicode whitespace is never recognized. With… |
|
pkg/sanitize/sanitize.go — This lookup is case-sensitive even though URI schemes are case-insensitive. A valid CommonMark… |
What changed in this PR
Introduces Markdown-aware sanitization to preserve code-bearing GitHub content while neutralizing hidden constructs.
Changes:
- Adds Goldmark-based
sanitize.Content. - Applies it across bodies, comments, releases, commits, and sub-issues.
- Adds extensive tests, benchmarks, and license metadata.
| File | Description |
|---|---|
pkg/sanitize/sanitize.go |
Implements Markdown-aware sanitization. |
pkg/sanitize/sanitize_test.go |
Tests fidelity, safety, and performance. |
pkg/github/minimal_types.go |
Applies content sanitization to converters. |
pkg/github/issues.go |
Sanitizes issue and sub-issue responses. |
pkg/github/issues_test.go |
Tests sub-issue sanitization. |
pkg/github/repositories.go |
Sanitizes releases and blame messages. |
pkg/github/repositories_test.go |
Tests release and blame behavior. |
pkg/github/discussions.go |
Preserves discussion body content. |
pkg/github/discussions_test.go |
Updates discussion expectations. |
pkg/github/projects.go |
Uses content policy for status updates. |
pkg/github/sanitize_coverage_test.go |
Expands policy coverage tests. |
go.mod |
Adds Goldmark dependency. |
go.sum |
Records Goldmark checksums. |
third-party/github.com/yuin/goldmark/LICENSE |
Adds Goldmark’s license. |
third-party-licenses.linux.md |
Updates Linux licenses. |
third-party-licenses.darwin.md |
Updates macOS licenses. |
third-party-licenses.windows.md |
Updates Windows licenses. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
URL masking can leave adjacent hidden Markdown active and has quadratic behavior on adversarial input.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 2
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
|
pkg/sanitize/sanitize.go — This URL mask extends from any http(s):// occurrence to the end of the whitespace-delimited… |
|
pkg/sanitize/sanitize.go — This backward scan makes markdownURLMask quadratic on an untrusted single-line body containing… |
Issues resolved since last review (4)
| Severity | Finding |
|---|---|
|
pkg/sanitize/sanitize.go — This lookup is case-sensitive even though URI schemes are case-insensitive. A valid CommonMark… View resolved comment |
|
pkg/sanitize/sanitize.go — These loops cast individual UTF-8 bytes to runes, so Unicode whitespace is never recognized. With… View resolved comment |
|
pkg/sanitize/sanitize.go — The bare-URL detector treats every alphabetic scheme:// token as safely visible, although the… View resolved comment |
|
pkg/sanitize/sanitize.go — This fallback escapes every ampersand, including those inside inline, fenced, and indented code.… View resolved comment |
This was referenced Sep 1, 2026
|
|
SamMorrowDrums
force-pushed
the
sammorrowdrums-sanitization-robustness
branch
from
September 1, 2026 14:27
c433667 to
0069d2d
Compare
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
URL masking currently rewrites valid relative reference destinations and can exempt malformed HTTP-like text from math neutralization.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 2
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
|
pkg/sanitize/sanitize.go — Valid relative reference-link destinations are not actually masked here. inlineDestinationSpan… |
|
pkg/sanitize/sanitize.go — This accepts any non-whitespace suffix after http:// or https:// without checking that it is a… |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
pkg/sanitize/sanitize.go — This backward scan makes markdownURLMask quadratic on an untrusted single-line body containing… View resolved comment |
|
pkg/sanitize/sanitize.go — This URL mask extends from any http(s):// occurrence to the end of the whitespace-delimited… View resolved comment |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
URL masking retains security bypasses and quadratic parsing paths for malformed untrusted Markdown.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 3
New issues introduced by this change (3)
| Severity | Finding |
|---|---|
|
pkg/sanitize/sanitize.go — Malformed inline-link candidates make this path quadratic. For `"$hidden$" + strings.Repeat("[x](",… |
|
pkg/sanitize/sanitize.go — An unbalanced opening parenthesis still returns a valid reference destination. Thus `[x]:… |
|
pkg/sanitize/sanitize.go — The same URL-mask pass is quadratic for unmatched angle brackets: each < invokes IndexByte… |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
pkg/sanitize/sanitize.go — This accepts any non-whitespace suffix after http:// or https:// without checking that it is a… View resolved comment |
|
pkg/sanitize/sanitize.go — Valid relative reference-link destinations are not actually masked here. inlineDestinationSpan… View resolved comment |
Suppressed comments (2)
pkg/sanitize/sanitize.go:507
- This unbounded search restarts at every line beginning with
[. An input containing$plus many unterminated definition-like lines (for example, repeated[label\n) causes each call to search the whole remaining document for]:, yielding quadratic work on untrusted bodies. Limit the search to the reference-label grammar/current line or derive destination spans from the already parsed Markdown AST.
closing := bytes.Index(source[offset:], []byte("]:"))
pkg/sanitize/sanitize.go:584
- Scheme-less text is accepted here as an autolink even when it is not one. For example,
<$ignore$>is plain CommonMark text, buturl.Parsereturns an empty scheme andlinkDestinationIsSafeaccepts it as a relative path, so the URL mask suppresses neutralization of the GitHub math delimiters and leavesignorerender-hidden. Restrict this mask to syntax that actually matches a URI or email autolink, ideally using the parsed AST.
parsed, err := url.Parse(string(destination))
if err != nil ||
(parsed.Scheme != "" &&
!strings.EqualFold(parsed.Scheme, "http") &&
!strings.EqualFold(parsed.Scheme, "https") &&
!strings.EqualFold(parsed.Scheme, "mailto")) {
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
URL masking can leave hidden Markdown unneutralized, and malformed reference labels can cause quadratic processing.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 2
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
|
pkg/sanitize/sanitize.go — This accepts every destination with an empty parsed scheme, so non-autolink text such as… |
|
pkg/sanitize/sanitize.go — This skips an arbitrary amount of whitespace, including blank lines, while Goldmark inline links… |
Issues resolved since last review (3)
| Severity | Finding |
|---|---|
|
pkg/sanitize/sanitize.go — The same URL-mask pass is quadratic for unmatched angle brackets: each < invokes IndexByte… View resolved comment |
|
pkg/sanitize/sanitize.go — An unbalanced opening parenthesis still returns a valid reference destination. Thus `[x]:… View resolved comment |
|
pkg/sanitize/sanitize.go — Malformed inline-link candidates make this path quadratic. For `"$hidden$" + strings.Repeat("[x](",… View resolved comment |
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
pkg/sanitize/sanitize.go:514
bytes.Indexsearches the entire remaining document for every line that starts with[. An input containing many unmatched reference-label openings (for example, thousands of"[x\n"lines and no"]:") therefore rescans nearly the same suffix once per line and makesContentquadratic, contrary to the bounded adversarial-work goal. Bound label scanning to Goldmark/CommonMark's label grammar and maximum length rather than searching to EOF.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Autolink classification still mishandles valid mailto URIs and invalid email domains, affecting fidelity and hidden-content neutralization.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 2
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
|
pkg/sanitize/sanitize.go — Valid mailto: autolinks are URI autolinks and may contain query parameters with additional… |
|
pkg/sanitize/sanitize.go — This domain check is looser than the goldmark/CommonMark email-autolink grammar: it accepts empty… |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
pkg/sanitize/sanitize.go — This skips an arbitrary amount of whitespace, including blank lines, while Goldmark inline links… View resolved comment |
|
pkg/sanitize/sanitize.go — This accepts every destination with an empty parsed scheme, so non-autolink text such as… View resolved comment |
SamMorrowDrums
force-pushed
the
sammorrowdrums-sanitization-robustness
branch
from
September 2, 2026 12:58
c37624f to
1c4c824
Compare
baggiiiie
reviewed
Sep 2, 2026
SamMorrowDrums
changed the title
SamMorrowDrums
force-pushed
the
sammorrowdrums-sanitization-robustness
branch
from
September 3, 2026 07:55
1c4c824 to
89b787e
Compare
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Direct release and sub-issue response paths lack fidelity regression coverage, and one blame assertion is now misleading.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 4
New issues introduced by this change (3)
| Severity | Finding |
|---|---|
|
pkg/github/issues.go — None of the sub-issue endpoint tests uses content that distinguishes this helper from a no-op, so… |
|
pkg/github/repositories.go — The new direct release-response path has no regression coverage: neither existing handler test… |
 |
pkg/github/repositories_test.go — The following raw-JSON NotContains("<script>") assertion now passes only because encoding/json… |
Pre-existing issues (2)
| Severity | Finding |
|---|---|
|
pkg/sanitize/sanitize.go — This domain check is looser than the goldmark/CommonMark email-autolink grammar: it accepts empty… View comment |
|
pkg/sanitize/sanitize.go — Valid mailto: autolinks are URI autolinks and may contain query parameters with additional… View comment |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The policy split is consistently applied across the targeted response paths with focused regression coverage.
Review tier: Balanced
Findings: 4
Pre-existing issues (5)
| Severity | Finding |
|---|---|
|
pkg/github/repositories.go — The new direct release-response path has no regression coverage: neither existing handler test… View comment |
|
pkg/github/issues.go — None of the sub-issue endpoint tests uses content that distinguishes this helper from a no-op, so… View comment |
|
pkg/sanitize/sanitize.go — This domain check is looser than the goldmark/CommonMark email-autolink grammar: it accepts empty… View comment |
|
pkg/sanitize/sanitize.go — Valid mailto: autolinks are URI autolinks and may contain query parameters with additional… View comment |
|
pkg/github/repositories_test.go — The following raw-JSON NotContains("<script>") assertion now passes only because encoding/json… View comment |
Route body-bearing response fields through the fidelity-preserving content path while retaining strict title handling and remove only unconditional invisible characters. Cover direct and converter read-modify-write surfaces for issues, releases, comments, discussions, projects, and commits. Refs #2202, #3165 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 69c5ab30-9815-4c07-8385-11a206e68f66
SamMorrowDrums
force-pushed
the
sammorrowdrums-sanitization-robustness
branch
from
September 3, 2026 13:20
808697e to
71ba0e8
Compare
Auto-generated by license-check workflow
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Several updated tests currently fail because corresponding converters and direct release handlers still use lossy or no sanitization.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 3
New issues introduced by this change (3)
| Severity | Finding |
|---|---|
 |
pkg/github/repositories_test.go — This new expectation fails: GetLatestRelease still marshals the raw RepositoryRelease at… |
|
pkg/github/sanitize_coverage_test.go — This expected-content branch makes the converter regression test fail for every case whose… |
|
pkg/github/sanitize_coverage_test.go — This assertion currently fails because newMinimalDiscussionComment still applies… |
Issues resolved since last review (5)
| Severity | Finding |
|---|---|
|
pkg/github/repositories_test.go — The following raw-JSON NotContains("<script>") assertion now passes only because encoding/json… View resolved comment |
|
pkg/github/repositories.go — The new direct release-response path has no regression coverage: neither existing handler test… View resolved comment |
|
pkg/github/issues.go — None of the sub-issue endpoint tests uses content that distinguishes this helper from a no-op, so… View resolved comment |
|
pkg/sanitize/sanitize.go — This domain check is looser than the goldmark/CommonMark email-autolink grammar: it accepts empty… View resolved comment |
|
pkg/sanitize/sanitize.go — Valid mailto: autolinks are URI autolinks and may contain query parameters with additional… View resolved comment |
Suppressed comments (2)
pkg/github/repositories_test.go:5093
- This new expectation fails because
GetReleaseByTagstill directly marshals the untouched release atrepositories.go:2322; the decoded body therefore includes\u200B. Sanitize the direct response's body withsanitize.Contentbefore marshaling it.
assert.Equal(t, "<details>Notes</details>", *returnedRelease.Body)
pkg/github/issues.go:2019
- The added endpoint tests do not protect this fidelity boundary: every body fixture is
This is **Markdown**\u200B, which produces the same result under bothsanitize.Contentand the lossysanitize.Sanitize. Include angle-bracket source such as<int>in the body fixtures and assert it survives across list/add/remove/reprioritize, so a regression to the sanitizer that caused #2202 is detected.
issue.Body = github.Ptr(sanitize.Content(*issue.Body))
| require.NoError(t, err) | ||
| assert.Equal(t, tc.expectedResult.TagName, returnedRelease.TagName) | ||
| assert.Equal(t, "First Release", *returnedRelease.Name) | ||
| assert.Equal(t, "<details>Notes</details>", *returnedRelease.Body) |
Comment on lines
+237
to
+241
| expected := sanitizedText | ||
| if tt.content { | ||
| expected = sanitizedContentText | ||
| } | ||
| assert.Equal(t, expected, tt.got()) |
| t.Run("discussion comment body (newMinimalDiscussionComment, used by get_discussion_comments)", func(t *testing.T) { | ||
| comment := newMinimalDiscussionComment("id", maliciousText, false) | ||
| assert.Equal(t, sanitizedText, comment.Body) | ||
| assert.Equal(t, sanitizedContentText, comment.Body) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Narrow release fix for Markdown fidelity on body-bearing GitHub response fields.
sanitize.Sanitizebehavior for short metadata fields such as titlessanitize.Content, preserving source text while removing only unconditional invisible charactersScope
This PR is limited to the Markdown/body fidelity boundary. Title handling remains strict.
Validation
script/lintscript/testpkg/sanitizeandpkg/githubFixes #2202
Fixes #3165