Skip to content

fix: #4749 handle malformed percent-encoded local links - #4773

Closed
jianongHe wants to merge 1 commit into
marktext:developfrom
jianongHe:fix-4749-format-link-path
Closed

jianongHe wants to merge 1 commit into
marktext:developfrom
jianongHe:fix-4749-format-link-path

Conversation

@jianongHe

Copy link
Copy Markdown

Closes #4749.

Summary

This PR moves local format-link path normalization into a small helper and handles URIError from decodeURIComponent(). Valid CommonMark percent-encoded local paths such as bad%20name.md still decode, while malformed percent escapes such as bad%zz.md, %.md, and invalid UTF-8 escape sequences are kept as literal path text instead of throwing from the main-process IPC handler.

Type of change

  • Bug fix (non-breaking, fixes an issue)
  • New feature (non-breaking, adds functionality)
  • Breaking change (causes existing functionality to change)
  • Documentation update

Test plan

  • New tests added (or explain why not needed)
  • Manually tested on: not run; this is covered by focused path-normalization unit tests

Verification:

  • corepack pnpm exec vitest run test/unit/specs/format-link-path.spec.ts
  • corepack pnpm --filter marktext typecheck
  • corepack pnpm exec vitest run --testTimeout=20000

Note: corepack pnpm test still fails locally in existing pdf.spec.ts tests under the default 5s timeout. The same full Vitest suite passes with --testTimeout=20000 (38 files, 684 tests).

Notes for reviewers [optional]

No UI screenshot is included because the visible behavior is avoiding a main-process exception for malformed local link paths. The regression tests cover valid percent-decoding and malformed percent escape inputs.


By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

niyo-oyin added a commit to niyo-oyin/marktext-ja that referenced this pull request Jul 30, 2026
Upstream PRs applied (all diff-reviewed, tests included and passing):
- marktext#4776: preserve ordered-list source markers — opening and saving no
  longer rewrites 1./1./1. to 1./2./3. (marktext#4772, silent doc mutation)
- marktext#4788: UNC/WSL paths become valid file:// authority URLs
  (file://server/share/…, not file:////…) so network images load
  (marktext#4577, marktext#4563)
- marktext#4952: table columns size to content (min-width 10em → 2em, marktext#4894)
- marktext#4773: malformed percent-escape in a clicked link no longer throws an
  unhandled URIError (marktext#4749)
- marktext#4873: Shift+digit/punctuation keybindings recordable and matchable
  again (patch-package fix for @hfelix/electron-localshortcut, marktext#4863)
- marktext#4910: sidebar icons match by extension first, so Dockerfile-Notes.md
  gets the markdown icon (marktext#4890)
- marktext#4317: restored windows are clamped to the target display's work area
  (multi-monitor / DPI, marktext#2928, marktext#1947)

Found while integrating marktext#4776: cloneStateTree shallow-copied meta, so
array-valued fields (order-list sourceMarkers, table aligns) stayed
SHARED between a getState() clone and the live document — mutating a
returned tree corrupted the document. Meta arrays are now copied, and
the clone walks an explicit work list instead of recursing, so
pathological nesting (600-level lists, marktext#4747) can no longer overflow
the call stack (10k-depth regression test).

Also: folder search debounces its per-keystroke ripgrep run (300ms,
Enter searches immediately, IME-composing keys ignored) (marktext#3556), and a
new undoFloor spec pins that undoing past the opened baseline never
empties the document (marktext#5028 — legacy-engine bug, does not reproduce on
@muyajs/core).

muya 1536/1536, desktop 783/783, lint/tsc/madge clean. PLANS.md gains a
prioritized backlog from the full 563-item issue/PR survey.
@Jocs

Jocs commented Sep 22, 2026

Copy link
Copy Markdown
Member

Thanks for digging into this, @jianongHe, and apologies for the slow reply on a PR that has been open for a while.

The semantics you picked are the right ones: % is a legal filename character, so keeping a malformed escape as literal path text — rather than rejecting the click — is what we want here, and the renderer already does exactly that for anchors in SCROLL_TO_ANCHOR.

Unfortunately the code this patch edits no longer exists. 7ab05fff (#5292 / #5298) extracted the join + decode out of file.ts into resolveLocalLinkTarget() in packages/desktop/src/main/filesystem/index.ts, which is why the PR now shows as conflicting. Resolving that conflict in this PR's favour would silently revert two shipped features — the anchor handling from #5292 and the unsafe-executable confirmation from #3575 — so this needs a rebase plus a move of the guard, not a merge.

While reviewing I also found that the same unguarded decodeURIComponent() has two more triggers this patch would not cover, and one case where the literal fallback introduces a silent regression:

  1. The decode runs on the joined dirname + target. window.DIRNAME is a raw filesystem path, not a URL, so a document saved in a folder whose name contains a bare % throws URIError for any ordinary link — resolveLocalLinkTarget('other.md', '/docs/50%off') throws on develop today. That is a much wider trigger than a malformed link target. And with an all-or-nothing try/catch around the joined path, bad%20name.md inside such a folder then stops decoding and silently fails to open, which is the very use case the decode exists for (CommonMark Mermaid support #503, Add swift syntax highlighting #57). The fix is to decode only the target and join the raw dirname — the same asymmetry encodeDirnameForUrl() already documents on the renderer side ([Bug] v 0.20.0-rc.1 - Images with relative path don't display, even when the preferences is set #5212 / [Bug] Relative images fail to load when the document's folder name contains # or ? #5302).

  2. A malformed escape in the fragment still throws: resolveLocalLinkTarget('other.md#100%', dirname), because toPathname() runs before the # split. Interestingly this also settles where the guard belongs: inside toPathname() the case recovers correctly (the first call returns a literal path, isFile() is false, so the # split runs and other.md opens with 100% handed to the renderer, which already tolerates it). Wrapped around the whole link, #100% gets baked into the path and the file never opens.

So the guard needs to live inside resolveLocalLinkTarget, and decode-before-join is what actually fixes the family of bugs. Since that is a different file and a different shape from this diff, I am closing this in favour of a separate PR that keeps your semantics — I will link it here in a moment.

Thank you for the write-up in #4749; the reproduction cases you listed there are in the new regression spec, and the analysis in this PR is what pointed at the right fallback behaviour.

@Jocs

Jocs commented Sep 22, 2026

Copy link
Copy Markdown
Member

Replacement PR: #5506. It keeps your literal-path fallback, moves the guard into resolveLocalLinkTarget's toPathname so the fragment case (other.md#100%) resolves instead of being baked into the path, and additionally stops the document folder from being percent-decoded. Your four reproduction cases from #4749 are in the regression spec. Thanks again.

Jocs added a commit that referenced this pull request Sep 22, 2026
…folder (#5506)

`resolveLocalLinkTarget` joined the document folder with the link target and
then percent-decoded the joined string, unguarded. The target is URL-encoded
text taken from the document, but `dirname` is a raw filesystem path that may
legally contain `%`, so decoding the join misread the document's own folder
name: a folder called `50%off` made every ordinary link inside it throw
`URIError` out of the `mt::format-link-click` handler, and one called
`my%20docs` resolved to `my docs`. Decode the target alone and join the raw
dirname — the mirror of `encodeDirnameForUrl` on the renderer side (#5212).
`isAbsolute` keeps testing the encoded target so a `%2F` cannot turn a relative
link into an absolute one.

A target that is not valid percent-encoding threw as well: `bad%zz.md`, `%.md`,
`%E0%A4%A.md`, and also `other.md#100%`, since `toPathname` runs before the `#`
split. `%` is a legal filename character, so fall back to the target as written,
the same guard the renderer already applies in `SCROLL_TO_ANCHOR`. Keeping it
inside `toPathname` is what lets the fragment case resolve: the literal path
does not exist, so the `#` split runs and `other.md` opens with `100%` handed to
the renderer, which tolerates an undecodable anchor.

Since #5298 made this handler `async` the throw surfaced as an unhandled
rejection rather than the error dialog reported in #4749, so the symptom on
develop was a click that did nothing at all.

A click on a target that does not exist stays a no-op, as it already is for any
other broken local link.

Closes #4749
Supersedes #4773

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
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.

[Bug] Malformed percent-encoded local link throws an unhandled URIError in the main process

2 participants