fix(sanitize): preserve visible Markdown content - #3177
Open
SamMorrowDrums wants to merge 1 commit into
Open
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 |
Comment on lines
+716
to
+718
| if bytes.EqualFold(scheme, []byte("mailto")) { | ||
| return isMailtoAddress(destination[schemeEnd+1:]) | ||
| } |
Comment on lines
+750
to
+760
| if !isASCIIAlphaNumeric(domain[0]) { | ||
| return false | ||
| } | ||
| for _, character := range domain { | ||
| switch { | ||
| case isASCIIAlphaNumeric(character), character == '.', character == '-': | ||
| default: | ||
| return false | ||
| } | ||
| } | ||
| return domain[len(domain)-1] != '.' && domain[len(domain)-1] != '-' |
Route Markdown and code-bearing response fields through a fidelity-preserving content path while retaining metadata sanitization for titles. Add exact converter and sanitizer coverage for useful Markdown and invisible-character filtering. Refs #2202 Refs #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 2, 2026 12:58
c37624f to
1c4c824
Compare
baggiiiie
reviewed
Sep 2, 2026
|
|
||
| // Content preserves Markdown and code content while removing invisible characters. | ||
| func Content(input string) string { | ||
| return FilterInvisibleCharacters(input) |
There was a problem hiding this comment.
hey @SamMorrowDrums
i have an issue titled [bug] can't add a connection to toolkits in desktop app and mcp returned [bug] can't add a connection to toolkits in desktop app.
i guess a separate func Title(input string) string for title sanitization would do.
searched github mcp and landed on this pr, haven't opened an issue yet. do you think this could be included in this PR? or a separate issue/PR would be preferred?
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
Fix the sanitizer boundary so reading Markdown/code-bearing GitHub content no longer silently deletes or truncates source that may later be written back.
Security boundary
Valid non-empty HTTP/HTTPS/mailto/relative link destinations remain functional because GitHub exposes them on hover/click. Non-URL-like, titled, image, empty-label, full-reference, unused, and duplicate forms are made visible.
Rich content removes variation selectors and zero-width joiners rather than trying to validate the full Unicode variation/grapheme registries. Visible base characters remain, but presentation may change. GitHub-rendered diagram/math fence types are returned as visible source rather than opaque rendered output.
Validation
script/lintscript/testscript/licenses-checkFuzzContentIsIdempotentruns covering idempotence, rendered hidden-rune safety, hidden Markdown constructs, and adversarial nestingFixes #2202
Fixes #3165