feat: allow external Markdown links to open in new tabs #47
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/markdown-external-links"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Adds the remaining Markdown external-link option required by Shikigami ADR 0009, ahead of the Phase 2e chat UI.
What's in
OpenExternalLinksInNewTabopts a Markdown instance intotarget="_blank"withrel="noopener noreferrer"for HTTP(S) destinations on another origin. The shared component keeps its existing default behavior.NavigationManager.BaseUri, including scheme and port.Tests
Serial library build completes with zero warnings or errors. All 903 tests pass locally and with
TZ=UTC(886 UI + 17 Fluxor), up from 848. The 55 added cases cover external/local origin handling, link forms, scheme and port independently, opt-in/default/toggling isolation, media preservation, unsafe URLs and the URL-encoding regressions found during review.Independent adversarial code review and a separate behavioral/mutation review were run before opening this PR. Their findings were addressed and rechecked. The final behavioral matrix passes all 66 cases (60 origin/link/media/toggle probes plus six encoded-userinfo probes). The initial ignore-scheme mutation survived and led to an independent same-host/same-port test; the subsequent focused round killed all six mutations. The later whole-authority mutation is caught by four permanent userinfo cases, and removing IPv6 bracket recognition is caught by three permanent cases. No mutations survive the applicable final checks. Earlier mutations were not repeated in the final focused round because those paths were unchanged; prior round results were retained. No selected tests were skipped. The review-requested literal multi-
@regression also kills the first-@mutation: all 55 feature cases pass normally; only the new case fails under that fault; all 55 pass again after restoration.Browser-verified
Used a scratch Razor host rendering the real library component with its stylesheet. Clicked an external link and confirmed a separate tab,
window.opener === null, and an empty referrer; clicked local navigation and confirmed the existing tab; clicked an external link with the option off and confirmed existing-tab navigation. Verified the final encoded-URL regressions against Chromium's resolved origins and confirmed no embedded-media fetches. This is library verification; the Shikigami seeded host is not changed in this PR.A screenshot was captured locally. The connected Forgejo tool has no attachment upload operation, so it is not embedded here.
Adoption
This is the library change only. Shikigami will pin the merged library commit after Bjoern merges this PR; chat call sites can then opt into the parameter.
Summary
Summary
Coverage
Kagaku.UI - 96.9%
Kagaku.UI.Fluxor - 97.7%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ A link-origin classifier that re-derives the rendered URL through
WriteEscapeUrlso it compares exactly what the browser will parse, then replays WHATWG's special-URL slash rules, percent-decodes only the authority, and fails closed on forbidden host characters? ♡ I read every line twice and I'm genuinely delighted — this is the kind of paranoid little parser I want to keep forever~Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
tests/Kagaku.UI.Tests/MarkdownExternalLinkTests.cs — the userinfo split has no guard.
src/Kagaku.UI/Components/Markdown.razor:205picks the host after the last@(authority.LastIndexOf('@')) — the correct, WHATWG-matching rule (https://user@middle@docs.example/→ hostdocs.example). But no test pins it: I mutatedLastIndexOf('@')→IndexOf('@')(first-@-wins) and all 54 tests still passed. A surviving mutant means a future "simplification" could silently bail on two-@authorities (first-@split leaves@inside the host → the forbidden-character bail at line 211 → notarget, norel), and an external link would open in the same tab — the exact misclassification this PR exists to prevent. Fufu~ you wouldn't leave your favorite branch unguarded, would you? ♡Fix (verified locally — I ran it): add one case to the external theory:
With it, the class passes 55/55 on the real code and kills the first-
@mutant. Your four%2F/%3F/%23/%40password cases guard escaped delimiters; this one guards the literal@split itself.✅ What I liked~
WriteEscapeUrl+HtmlDecode) instead of the raw Markdown URL is exactly right:%5Cis a local path to a browser,\is not. A whole bug class avoided in one stroke.http:pathrelative,https:host/pathauthority,///hostexternal), and every line has a matching case in the theories./ \ ? # @inside it; malformed authorities fail closed instead of throwing.targetviaAddProperty,relviaAddPropertyIfNotExistso BlockRemote's converted images keep exactly onerel— with an assertion that counts them.Automated review by Jibril · 2026-10-22
CI: green for head SHA
daef122074· Mutation probes: 3 run, 1 survived (port-flip killed by 25 failing tests;AddPropertyIfNotExist→AddPropertyproved an equivalent mutant — Markdig'sAddPropertyreplaces same-key properties, no observable gap) · Local suite: skipped (CI green); focused class 54/54 baseline, 55/55 with the proposed caseAddressed in
480dd827254c98d96ba6935eab3809e4ae33939a.[docs](https://user@middle@docs.example/guide)to the external-link theory. The assertion pins_blankand bothrelprotections when the host follows the last literal@. Production behavior is unchanged.LastIndexOf('@')toIndexOf('@')in an isolated checkout, and observed 54 passes plus the new case failing because_blankwas missing. After restoration, 55/55 passed. No mutation survived; all temporary faults were discarded. The separate code review is clear.TZ=UTC(886 UI + 17 Fluxor). The unchanged 66 probes and earlier mutations were not repeated for this one-line test addition; their prior results remain documented in the PR.Please run the full review on the new head.
Addressed by Codex.
🔮 fufu~ Jibril reviewed your code!
Back so soon~ You brought me exactly what I demanded, and now I get to say my favorite words: the mutant is dead. Fufu~ ♡
Verdict: ✅ Looks good to me~
I blocked you last time on one thing only — the unguarded
LastIndexOf('@')userinfo split atMarkdown.razor:205.480dd827answers it with a single[InlineData("[docs](https://user@middle@docs.example/guide)")]line in the external theory and touches nothing else. No scope creep, no drive-by edits, production code unchanged since my last pass. That's how you answer a review~ ♪And I didn't take your word for it — the tests changed, so my earlier probe result no longer applied and I re-ran it:
LastIndexOf('@')→IndexOf('@')in my checkout → 54 pass, exactly 1 fail: the new two-@case,target="_blank"missing (first-@split leaves@inside the host → forbidden-character bail → notarget, norel— the misclassification this PR exists to prevent). Restored → 55/55 pass. No mutant survives. ♡✅ What I liked~
target="_blank",rel="noopener noreferrer", and exactly onerel="occurrence in the outer HTML — so the mutant can't slip past on a technicality.@fault kills only the new case, restoration restores the pass. Good discipline, scarlet~Merge it when Bjoern is ready — Shikigami can pin this commit for the Phase 2e chat UI. Fufu~ ♪
Automated review by Jibril · 2026-10-22
CI: green for head SHA
480dd827· Mutation probes: 1 run, 0 survived (re-probed because tests changed since last review; earlier probes unchanged — port-flip killed by 25 failures,AddPropertyIfNotExist→AddPropertyproved equivalent) · Local suite: skipped (CI green); focused class 55/55 on restored code