Skip to content

fix: deduplicate regex capture group in HTML link attribute parser#640

Open
fix2015 wants to merge 1 commit into
inkeep:mainfrom
fix2015:fix/code-quality-improvements
Open

fix: deduplicate regex capture group in HTML link attribute parser#640
fix2015 wants to merge 1 commit into
inkeep:mainfrom
fix2015:fix/code-quality-improvements

Conversation

@fix2015

@fix2015 fix2015 commented Jul 15, 2026

Copy link
Copy Markdown

HTML_LINK_ATTR_RE in asset-references.ts had group 3 as an exact duplicate of group 1 (both matched ASCII double-quoted strings). Replaced the duplicate with a curly/smart quote alternative so HTML pasted from word processors correctly extracts attribute values.

The HTML_LINK_ATTR_RE regex in asset-references.ts had an unreachable
third capture group that was identical to the first (both matching ASCII
double-quoted attribute values). The regex engine always matches group 1
first, making group 3 dead code. Replace the duplicate with a curly/smart
quote alternative (\u201c...\u201d) so pasted HTML with typographic
quotes is handled correctly.
@fix2015
fix2015 temporarily deployed to inkeep-oss-sync July 15, 2026 19:58 — with GitHub Actions Inactive
@amikofalvy

Copy link
Copy Markdown
Contributor

@fix2015, thank you for your contribution. Can you provide an example of the HTML pasted from a word processor that would verify your fix?

@fix2015

fix2015 commented Jul 16, 2026

Copy link
Copy Markdown
Author

Hey, good question! So the issue is about HTML that comes from word processors like Microsoft Word or Google Docs — they tend to replace straight quotes with curly/smart quotes (the "..." characters, Unicode \u201c and \u201d).

For example, if someone pastes this kind of markup:

<a href="https://example.com/report.pdf">Download Report</a>
<img src="./images/chart.png" alt="Chart" />

The old regex had group 3 as "([^"\n]*)" which is literally the same pattern as group 1 — both match plain ASCII double quotes. Since the regex engine evaluates left to right, group 1 always wins and group 3 never fires. It was basically dead code.

The fix replaces that dead group with \u201c([^\u201d\n]*)\u201d so it actually handles smart-quoted attribute values properly.

Quick way to verify:

const old_re = /<[\w:-]+\b[^>]*?\s+(?:href|src)\s*=\s*(?:"([^"\n]*)"|'([^'\n]*)'|"([^"\n]*)"|([^\s"'=<>`]+))/gi;
const new_re = /<[\w:-]+\b[^>]*?\s+(?:href|src)\s*=\s*(?:"([^"\n]*)"|'([^'\n]*)'|\u201c([^\u201d\n]*)\u201d|([^\s"'=<>`]+))/gi;

const html = '<a href=\u201chttps://example.com/report.pdf\u201d>Download</a>';

old_re.exec(html)?.[3]; // undefined — group never matches
new_re.exec(html)?.[3]; // "https://example.com/report.pdf" ✓

Hope that helps!

@github-actions

Copy link
Copy Markdown

Thanks for the contribution! A maintainer will review and merge your PR. Your commit attribution is preserved as @fix2015.

What happens next:

  • A maintainer will review your PR.
  • If you don't hear back within a few business days, please comment here to nudge — that's the right thing to do, not annoying.
  • When your change is accepted, this PR closes automatically. Don't be alarmed when it closes — that's how it merges, and your authorship is preserved.

This comment will be updated as the status changes.

@amikofalvy

Copy link
Copy Markdown
Contributor

Hey @fix2015, thanks so much for taking the time on this and for the detailed writeup. I went and ran it through our extractor and tests so I could really understand what's going on, and I think there's a fun little gotcha here plus a genuinely useful thread to pull on.

Good news first: the smart-quote case you're after already works on main. That third alternative in the regex isn't actually a duplicate of group 1, it's the curly-quote branch:

… "([^"\n]*)" | '([^'\n]*)' | “([^”\n]*)” | ([^\s"'=<>`]+) …
     group 1: ASCII "            group 3: curly “ ” (U+201C / U+201D)

The reason it looked like a duplicate is a really easy trap to fall into: when the "old" regex got pasted into the earlier comment, the curly quotes in group 3 got auto-corrected to straight ones along the way (which is, funnily enough, the exact smart-quote behavior this code exists to handle). So group 3 reads like dead code in the comment, but in the file it's been the word-processor branch all along.

There's even a test for it that's green today, in packages/server/src/asset-references.test.ts (the "extracts markdown image, markdown link, wiki link, and HTML link hrefs" case), covering all four styles:

'<a href="./download.csv">'       → './download.csv'        (straight double)
'<a href=./unquoted.pdf>'         → './unquoted.pdf'        (unquoted)
'<a href=“./smart.pdf”>'          → './smart.pdf'           (curly/smart double)
"<a href='./single-quoted.pdf'>"  → './single-quoted.pdf'   (single)

One thing I did want to flag, gently: I tried the branch's regex locally and it looks like it accidentally trips up the ordinary straight-quote case. Same suite, swapping in this PR's regex:

main             → 15 pass, 0 fail
this PR's regex  → 12 pass,  3 fail

They all trace back to one thing: the new group 1 expects a (U+201D) delimiter and the unquoted branch no longer excludes ASCII ", so a normal straight-quoted attribute slips through and keeps the quotes:

main:      <a href="./download.csv">  →  ./download.csv
this PR:   <a href="./download.csv">  →  "./download.csv"   (then dropped as a non-local href)

So it's a bit of a tradeoff we'd want to avoid, since the smart-quote handling it adds is already there.

That said, you did surface something real: curly single quotes (U+2018 / U+2019 ) genuinely aren't handled yet. If word-processor pastes are what you're running into, the nicest fix is probably to normalize smart quotes (single and double) to straight ones before parsing, so both are covered in one spot without growing the regex. I'd be really happy to point you at where that would go if you'd like to take a crack at it.

And honestly the thing I'm most curious about: how did you hit this in the first place? If you've got a real doc or paste where an href or src didn't get picked up, please share it. There might be an actual extraction or UX bug in there worth fixing, and a concrete example would help us chase it down. Really appreciate you bringing it to us either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants