Don't splice a link into the middle of a URL - #117
Conversation
`convertToMarkdown` turns HTML into Markdown by splicing `[label](href)` over
the label at the offset where it finds that label in the plain-text flavor.
That assumes `text/plain` is a flattening of `text/html`, which holds for prose
containing a link but not for how many native apps and clipboard tools copy a
link: `text/plain` is the URL, and `text/html` is an anchor labelled with a
shortened rendering of it.
The label's only occurrence is then inside the URL, so the `[` lands there:
text/plain: https://github.com/owner/repo/blob/main/a.js#L7
text/html: <a href="…">repo/blob/main/a.js#L7</a>
pasted: https://github.com/owner/[repo/blob/main/a.js#L7](https://github.com/owner/repo/blob/main/a.js#L7)
Such a label describes the whole URL, so replace the whole whitespace-delimited
token when it is this link's own href — which yields the link the clipboard
meant, `[repo/blob/main/a.js#L7](https://…#L7)`.
When the token is a URL but *not* this href, leave the paste alone rather than
corrupt it; splicing inside a URL is never right. Every other case keeps the
label's own occurrence, so a label that merely abuts other text
(`<a href="…">foo</a>bar` pasted alongside `foobar`) is unaffected.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The equal-length guard and the fallback both returned the label's own occurrence, so two of four return paths produced the same value and the guard read as a distinct case. Nesting the URL checks under the length comparison leaves one such return, and computes the token's text only where it is used. Also renames `isURL` to `looksLikeURL`. paste-markdown-link.ts already has an `isURL` that requires the whole string to round-trip through `new URL()`; this one only tests for a scheme, because the question here is whether splicing into the text would corrupt a URL rather than whether it is a valid one. The shared name invited a future consolidation that would change behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Punctuation-wrapped URLs can still be corrupted because URL detection only checks the token’s first character.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Prevents shortened link labels from being inserted into the middle of pasted URLs.
Changes:
- Detects URL-containing plaintext tokens and selects a safer replacement span.
- Adds regression tests for matching and mismatched URLs.
File summaries
| File | Description |
|---|---|
src/paste-markdown-html.ts |
Adds link-span detection and URL safeguards. |
test/test.js |
Adds regression coverage for shortened labels. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
`looksLikeURL` tests for a scheme at offset 0, so `(https://example.com/a)` did not read as a URL: neither branch applied and the label was still spliced inside it, leaving `(https://github.com/owner/[repo](…))`. Try the whitespace-delimited token first and then the same token with wrapping punctuation trimmed off both ends. Order matters: a URL can end in a bracket of its own, as `…/wiki/Ruby_(programming_language)` does, and trimming first would strip that bracket, fail the href comparison, and decline a paste that has a perfectly good link in it. Both orders are covered by tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@keithamus , can you take a look at this when you get a chance? |
|
I don't work at GitHub any more (I work at Mozilla now). @primer will need to look at this instead or maybe @mattcosta7? |
There was a problem hiding this comment.
Review details
Suppressed comments (2)
src/paste-markdown-html.ts:153
- The URL check returns before the punctuation-trimmed candidate is examined whenever the raw token starts with a scheme. Consequently, a shortened link followed by normal sentence punctuation (for example, plaintext
https://github.com/owner/repo.with an href lacking the period) is declined even though lines 124–125 say this case is recognized. Check every candidate for an href match first, and only then decide that the token is some other URL.
for (const span of [token, withoutWrappingPunctuation(markdown, token)]) {
const text = markdown.slice(span.index, span.index + span.length)
// The label describes this whole URL, so the link replaces the whole URL.
if (areEqualLinks(href, text)) return span
// Splicing inside a URL is never right, so a label inside one that is not this link's own
// href leaves the paste alone rather than corrupting it.
if (looksLikeURL(text)) return null
src/paste-markdown-html.ts:109
- This recognizes only hierarchical schemes containing
://, despite the function's contract covering text that starts with a scheme. A label insidemailto:user@example.com,urn:..., or another valid non-hierarchical URL still falls through and is spliced into that URL when the anchor has a different href.
function looksLikeURL(text: string): boolean {
return /^[a-z][a-z\d+.-]*:\/\//i.test(text)
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| while (start < end && OPENING_PUNCTUATION.includes(markdown[start])) start++ | ||
| while (end > start && CLOSING_PUNCTUATION.includes(markdown[end - 1])) end-- |
Bumps the npm_and_yarn group with 1 update in the / directory: [ip-address](https://github.com/beaugunderson/ip-address). Updates `ip-address` from 9.0.5 to 10.4.0 - [Release notes](https://github.com/beaugunderson/ip-address/releases) - [Commits](beaugunderson/ip-address@v9.0.5...v10.4.0) --- updated-dependencies: - dependency-name: ip-address dependency-version: 10.4.0 dependency-type: indirect dependency-group: npm_and_yarn ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps the npm_and_yarn group with 1 update in the / directory: [js-yaml](https://github.com/nodeca/js-yaml). Updates `js-yaml` from 4.3.0 to 4.3.1 - [Changelog](https://github.com/nodeca/js-yaml/blob/4.3.1/CHANGELOG.md) - [Commits](nodeca/js-yaml@4.3.0...4.3.1) --- updated-dependencies: - dependency-name: js-yaml dependency-version: 4.3.1 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 29a3ad99-f79c-4dc7-8a8e-24530c3e3c6b
|
👋🏽 thanks for getting this going @pranayshirolkar, added a failing test for #117 (comment), can you take a look? |
Fixes #118.
Problem
Copy a link from a native app or a clipboard tool and paste it into a GitHub comment box, and the Markdown comes out with a
[planted in the middle of the URL:It renders as two links, one of them junk. The shape that triggers it —
text/plainis the URL,text/htmlis an anchor labelled with a shortened rendering of that URL — is how a lot of tooling writes a link to the pasteboard (macOS automation scripts, clipboard managers, "copy link" affordances that shorten the visible text).Cause
convertToMarkdownsplices[label](href)over the label at the offset whereindexOffinds it in the plain text:https://github.com/github/paste-markdown/blob/main/src/paste-markdown-html.ts#L83-L90
That's right for prose containing a link —
example link plus more text— because there the plain text is a flattening of the HTML. When the plain text is the URL and the label is a piece of it, the label is found inside the URL (offset 25 in the example above) and the splice covers only the tail.Fix
When the label's occurrence sits inside a longer whitespace-delimited token, decide by what that token is:
<a href="…">foo</a>barpasted alongsidefoobarstill yields[foo](…)bar.areEqualLinksalready handles the trailing-slash and casing differences between an anchor's normalizedhrefand the pasted text, so the comparison reuses it.Tests
Two added, both failing before this change and passing after (
38 passed, 2 failed→40 passed):links the whole url when the label is a shortened rendering of itdoesn't splice a link inside a url that is not its own hrefAll existing tests pass unchanged.