fix(sanitize): preserve Markdown body fidelity on read surfaces - #3177
Merged
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) |
social4hyq
pushed a commit
to social4hyq/homebrew-core
that referenced
this pull request
Sep 20, 2026
github-mcp-server 1.12.0 Created-by: HarmonybrewBot Commit-by: HarmonybrewBot Merged-by: HarmonybrewBot Description: Created by `brew bump` --- Created with `brew bump-formula-pr`.<details> <summary>release notes</summary> <pre>## Highlights - **New governance tools for agents.** Read and manage repository rulesets and custom properties across repository, organization, and enterprise levels. - **Faster, safer feature flags.** Functional availability rules evaluate lazily after static filtering, deduplicate checks per request, and preserve precise availability errors. - **Safer write operations.** Pin merge HEADs, recover file SHAs, use least-privilege public-repository access, and detect silently dropped labels. - **Richer review and discovery support.** Adds review range coordinates, thread resolution reasons, projected-header preflights, Server Card support, and Agent Plugins. - **Better content fidelity.** Markdown bodies, titles, release notes, comments, and commit messages preserve visible content while filtering unsafe invisible characters. ## What's Changed * Remove documentation for unavailable tool-search command by @tommaso-moro in github/github-mcp-server#3162 * Expose Copilot review thread resolution reasons by @cagesellchen in github/github-mcp-server#3123 * fix(repos): give create_or_update_file callers a SHA they can actually get by @dylanpulver in github/github-mcp-server#3131 * fix: allow public_repo for public contribution tools by @paulcakeface in github/github-mcp-server#3140 * Enable feature flags via URL query parameter (?features=) for headerless hosted connections by @CAOShurong in github/github-mcp-server#3146 * Add expectedHeadSha pinning to merge_pull_request by @jcosta1970 in github/github-mcp-server#3182 * Fix e2e harness compilation against go-github v89 and go-sdk v1.7 by @ppoffice in github/github-mcp-server#3187 * Return range coordinates for review comments by @yiheng-kkk in github/github-mcp-server#3193 * Fix issue_write silently dropped label errors by @SamMorrowDrums in github/github-mcp-server#3195 * fix(http): allow projected MCP headers in preflights by @SamMorrowDrums in github/github-mcp-server#3167 * Spell perPage the same way in every paginated tool by @karpovantonme in github/github-mcp-server#3142 * Add MCP Server Card (SEP-2127) types + handler by @SamMorrowDrums in github/github-mcp-server#2768 * Refactor feature flags to use functional availability rules by @SamMorrowDrums in github/github-mcp-server#3166 * feat(governance): add repository ruleset tools with multi-level scope challenge by @SamMorrowDrums in github/github-mcp-server#2991 * feat(governance): add custom properties tools by @SamMorrowDrums in github/github-mcp-server#2992 * Fix HTML entities in sanitized titles by @SamMorrowDrums in github/github-mcp-server#3216 * build(deps): bump fast-uri from 3.1.5 to 3.1.7 in /ui in the npm_and_yarn group across 1 directory by @dependabot[bot] in github/github-mcp-server#3208 * build(deps): bump the npm_and_yarn group across 1 directory with 2 updates by @dependabot[bot] in github/github-mcp-server#3217 * build(deps): bump github/codeql-action from 4.37.4 to 4.37.9 by @dependabot[bot] in github/github-mcp-server#3192 * feat: add Agent Plugins 1.0 package by @777genius in github/github-mcp-server#3169 * fix(sanitize): preserve Markdown body fidelity on read surfaces by @SamMorrowDrums in github/github-mcp-server#3177 * build(deps): bump actions/stale from 10 to 11 by @dependabot[bot] in github/github-mcp-server#3003 * Fix main CI sanitizer integration by @SamMorrowDrums in github/github-mcp-server#3219 ## New Contributors * @cagesellchen made their first contribution in github/github-mcp-server#3123 * @paulcakeface made their first contribution in github/github-mcp-server#3140 * @CAOShurong made their first contribution in github/github-mcp-server#3146 * @jcosta1970 made their first contribution in github/github-mcp-server#3182 * @ppoffice made their first contribution in github/github-mcp-server#3187 * @yiheng-kkk made their first contribution in github/github-mcp-server#3193 * @karpovantonme made their first contribution in github/github-mcp-server#3142 * @777genius made their first contribution in github/github-mcp-server#3169 **Full Changelog**: https://github.com/github/github-mcp-server/compare/v1.11.0...v1.12.0</pre> <p>View the full release notes at <a href="https://github.com/github/github-mcp-server/releases/tag/v1.12.0">https://github.com/github/github-mcp-server/releases/tag/v1.12.0</a>.</p> </details> <hr> See merge request: Harmonybrew/homebrew-core!18304
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