1885098 - All `&` in URLs from markdown content are encoded as `&amp%3B` leading to bad links

Status: NEW

(Developer Infrastructure :: Firefox Source Docs: Content, defect)

Description

[Julian Descottes [:jdescottes]]

All links containing an & become encoded as &, which breaks links with more than 1 query parameter.
This affects several links in our documentation.

We attempted to fix this in Bug 1828390 by updating myst_parser to version 2.0.0, however the issue still occurs with the latest myst_parser version.
This is tracked on the myst parser repository at https://github.com/executablebooks/MyST-Parser/issues/760

Comment 1

[BugBot [:suhaib / :marco]]

The severity field is not set for this bug.
:Sylvestre, could you have a look please?

For more information, please visit BugBot documentation.

Comment 2

[Nick Alexander :nalexander [he/him] (PTO August 22-September 12)]

On Slack, from @flod:

(also commented on Matrix) Not the prettiest, but using HTML syntax instead of Markdown seems to work around the issue for me.
diff --git a/toolkit/components/messaging-system/schemas/SpecialMessageActionSchemas/index.md b/toolkit/components/messaging-system/schemas/SpecialMessageActionSchemas/index.md
index aa1c5c21878e..ccd1f120de51 100644
--- a/toolkit/components/messaging-system/schemas/SpecialMessageActionSchemas/index.md
+++ b/toolkit/components/messaging-system/schemas/SpecialMessageActionSchemas/index.md
@@ -281,7 +281,7 @@ Action for setting various browser prefs
 
 Prefs that can be changed with this action can be found in the `allowList`
 definition for Special Message Actions in
-[SearchFox](https://searchfox.org/mozilla-central/search?q=allowedPrefs&path=toolkit%2Fcomponents%2Fmessaging-system&case=false&regexp=false).
+<a href="https://searchfox.org/mozilla-central/search?q=allowedPrefs&path=toolkit%2Fcomponents%2Fmessaging-system&case=false&regexp=false">SearchFox</a>.
 
 Any pref that begins with `messaging-system-action.` is also allowed.
 If the pref is not present in the list above and does not begin

Comment 3

[Rob Wu [:robwu]]

Another example at https://searchfox.org/firefox-main/rev/d4ae522db3b933e502d1febec899e7955c1fb633/browser/extensions/newtab/docs/v2-system-addon/train_hopping.md#260

[This link](https://treeherder.mozilla.org/jobs?repo=mozilla-central&searchStr=trainhop) should show the most recent `trainhop` jobs occurring on `main`.

renders with & as https://firefox-source-docs.mozilla.org/browser/extensions/newtab/docs/v2-system-addon/train_hopping.html as seen in this excerpt from the generated HTML:

<a class="reference external" href="https://treeherder.mozilla.org/jobs?repo=mozilla-central&amp;amp;searchStr=trainhop">This link</a> should show the most recent <code class="docutils literal notranslate"><span class="pre">trainhop</span></code> jobs occurring on <code class="docutils literal notranslate"><span class="pre">main</span></code>.</p>

Comment 4

[Rob Wu [:robwu]]

Allegedly https://github.com/executablebooks/MyST-Parser/issues/760 was fixed for real by https://github.com/executablebooks/MyST-Parser/pull/1126
which is part of the v5.1.0 release: https://github.com/executablebooks/MyST-Parser/releases/tag/v5.1.0

Looks like we can now try bumping https://searchfox.org/firefox-main/rev/d4ae522db3b933e502d1febec899e7955c1fb633/python/sites/docs.txt#14

except I see that exactly that (bumping to 5.1.0) was attempted before by bug 2023603 , and failed.

Comment 5

[Manuel Bucher [:manuel]]

I wasn't aware of this bug when trying to fix it in bug 2023603. Probably better to do the update here. I won't be able to do it (as I tried to explain why in Bug 2023603 comment 13). Perhaps we could also consider cherry-picking the fix (https://github.com/executablebooks/MyST-Parser/commit/ca5ac8665106c16128ae838b4c2151415f8e30ca). This is the only relevant part of the commit:

diff --git a/myst_parser/mdit_to_docutils/base.py b/myst_parser/mdit_to_docutils/base.py
index ba50b387..7e293f73 100644
--- a/myst_parser/mdit_to_docutils/base.py
+++ b/myst_parser/mdit_to_docutils/base.py
@@ -1011,7 +1010,7 @@ def render_link_url(
             if "classes" in conversion:
                 ref_node["classes"].extend(conversion["classes"])
 
-        ref_node["refuri"] = escapeHtml(uri)
+        ref_node["refuri"] = uri
         if implicit_text is not None:
             with self.current_node_context(ref_node, append=True):
                 self.current_node.append(nodes.Text(implicit_text))

Comment 6

[Dan Mehic]

Claude is digging into why bug 2023603's 5.1.0 bump got backed out, because the blocker turns out to be circular and I think that changes how hard this is.

The pin blocking the upgrade exists because of the version being upgraded away from

taskcluster/requirements.in:

# Temporarily resolve conflict with myst-parser 2.0.0 in python/sites/docs.txt
markdown-it-py < 4

myst-parser 5.1.0 needs markdown-it-py >= 4. The line forbidding that was added to unblock myst-parser 2.0.0 — the exact version we want to remove — and its own comment calls it temporary.

It reaches the docs venv because python/sites/docs.txt pulls in requirements-txt:taskcluster/requirements.txt wholesale, so docs inherits every taskcluster pin, markdown-it-py==3.0.0 included.

Why regenerating the lockfile wasn't enough

In bug 2023603 comment 13 manuel described "some transitive dependency towards the old markdown-it-py that goes back to the mach-build command" after following ahal's uv pip compile advice. That is two concrete things:

1. taskcluster/requirements.in line 1 is -c ../third_party/python/requirements.txt, so the vendored set is a hard constraints file on the resolution, and it pins markdown-it-py==3.0.0
2. third_party/python/markdown_it_py/ is vendored at 3.0.0

Regenerating the lockfile was necessary but could never move past the -c constraint.

Nothing actually requires < 4

• rich==15.0.0 declares Requires-Dist: markdown-it-py (>=2.2.0) — no upper bound
• markdown-it-py is not requested explicitly in third_party/python/requirements.in; it appears only in the lockfile, pulled in transitively by rich

The explicit line in taskcluster/requirements.in is the only real constraint, and it is self-referential.

The backout was the right signal, not an obstacle

The 5.1.0 attempt was backed out on test_site_compatibility.py, whose test_sites_compatible validates that all mach site .txt files resolve to a mutually consistent set. Bumping docs.txt on its own made it inconsistent with taskcluster's pinned set. The upgrade has to be atomic across vendored + taskcluster + docs, which is what that patch didn't do.

Suggested order

1. ./mach vendor python to move vendored markdown-it-py 3.0.0 -> 4.x (mdurl comes along)
2. Delete the markdown-it-py < 4 line from taskcluster/requirements.in
3. uv pip compile requirements.in --generate-hashes -o requirements.txt --universal
4. docs.txt: myst-parser -> 5.1.0, Sphinx bump, requires-python -> >=3.11 (per bug 2023603 comment 6, only mach.txt and build.txt are hard-3.9, and CI has a 3.11 toolchain)
5. Re-land the backed-out patch on top — it already did the Sphinx bump and the config.yml warning-pattern work

Step 1 is the one to be careful with, since it touches every markdown-it-py consumer across all venvs. source-test-python-mach is the job to watch.

One correction on the cherry-pick idea

Comment 5 here suggests cherry-picking the upstream one-liner onto the current version. Worth noting that myst-parser is not vendored — docs.txt has pypi:myst-parser==2.0, and there is no third_party/python/myst_parser. So cherry-picking would mean vendoring myst-parser first, which is the "last resort" ahal referred to. Given the pin above appears removable, the straight upgrade looks like less work than the patch-carry.

I have not attempted any of this, so the risk in step 1 is unmeasured.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

https://phabricator.services.mozilla.com/D277906 might help