FIX: Prevent backslash accumulation in upload markdown labels (#39461)

#39133 backslash-escaped markdown characters in upload filenames at
generation time. That worked for freshly uploaded files but exposed a
latent bug in the rich-editor round-trip: markdown-it kept `\_` in the
parsed image token's content, ProseMirror stored it verbatim in the
`alt` attribute, and on save the serializer re-escaped each `\` to `\\`.
Every edit doubled the backslashes (1 → 2 → 4 → … → 2^N) until the post
exceeded `max_post_length` and became uneditable.

Escaping the raw is fundamentally fragile — nothing treats the stored
raw as canonical, so any parse/serialize cycle either drops the escape
or re-applies it. Fix it at parse time instead:

- Revert the generation-side escaping from #39133 in `UploadMarkdown`,
  `uploads.js`, `inline_uploads.rb`, `to-markdown.js` and `sanitizeAlt`.
- Add a `literalize_upload_labels` core ruler in the markdown-it engine.
  After inline parsing runs, it walks `image` / `link_open` tokens whose
  URL starts with `upload://` and collapses their children into a single
  literal text token, rebuilt from the children `content` plus the
  `markup` of emphasis/strong/strikethrough delimiters. So `_foo_`,
  `**foo**`, `~~foo~~`, `` `foo` ``, `\_foo`, linkified URLs, hashtags
and mentions inside upload labels all render literally. Reference-style
  links (`[label][ref]` with `[ref]: upload://…`) get the same treatment
  for free since they go through the same tokens.

The raw now stays canonical: the filename goes in verbatim, cooks the
same way on every pass, and the textarea and rich editor round-trip
identically.

Because escaping is gone, the structural characters `[`, `]` and `|`
(which would break the link/image syntax and can't be escaped without
reintroducing the doubling) are stripped from labels at every generation
point — `UploadMarkdown`, the HTML-anchor and hotlinked-image
conversions in `inline_uploads.rb`, `uploads.js` and `to-markdown.js`.

The multi-token scan-forward in `renderAttachment` (engine.js) and in
ProseMirror's `link.js` parser is kept: it still matters for non-upload
attachment links like
`[**bold**|attachment](https://example.com/x.pdf)`,
where the label legitimately contains inline formatting the new ruler
doesn't touch.

`StripUploadLabelEscapes` (post-deploy migration) heals posts already
damaged by the regression, batching a scoped `regexp_replace` across the
`posts` table. The lookahead `(?=[^\]\[]*\]\(upload://)` bounds each
match to an upload label — forbidding `[`/`]` between the escape and the
closing `](upload://` keeps user-written `\_` escapes elsewhere in the
raw intact. Scoping on the lookahead alone, rather than anchoring on the
opening `[`, lets a single pass strip every escape in a label (e.g.
`foo\_bar\_baz`), not just the first.

https://meta.discourse.org/t/401231
This commit is contained in:
Régis Hanol
2026-06-03 18:10:38 +02:00
committed by GitHub
parent 80fd7e426f
commit 49474b0188
15 changed files with 293 additions and 47 deletions
+4 -4
View File
@@ -198,8 +198,8 @@ class InlineUploads
if href && (external_href || matched_uploads(href).present?)
has_attachment = node.attributes["class"]&.value
index = $~.offset(0)[0]
text = match[2].strip.gsub("\n", "").gsub(/ +/, " ")
text = "#{UploadMarkdown.escape_markdown(text)}|attachment" if has_attachment
text = match[2].strip.gsub("\n", "").gsub(/ +/, " ").gsub(/[\[\]\|]/, "")
text = "#{text}|attachment" if has_attachment
yield(match[0], href, +"[#{text}](#{PLACEHOLDER})", index) if block_given?
end
@@ -255,9 +255,9 @@ class InlineUploads
raw =
raw.gsub(%r{^(https?://\S+)(\s?)$}) do |match|
if upload = blk.call(match)
filename_modified = upload.original_filename.to_s
filename_modified = upload.original_filename&.gsub(/[\[\]\|]/, "").to_s
filename_modified = File.basename(filename_modified, File.extname(filename_modified))
"![#{UploadMarkdown.escape_markdown(filename_modified)}](#{upload.short_url})"
"![#{filename_modified}](#{upload.short_url})"
else
match
end
@@ -0,0 +1,49 @@
# frozen_string_literal: true
# One-off cleanup for posts whose upload markdown labels accumulated backslash
# escapes through repeated rich-editor edits (regression from
# https://meta.discourse.org/t/401231). The backslashes doubled on each save
# (1 → 2 → 4 → … → 2^N). The engine-side fix prevents further damage; this
# heals existing posts.
#
# Batched + non-transactional so we never hold update locks across the whole
# posts table at once on large sites.
#
# Pattern: strip runs of `\` that escape `_ * ~ | `` inside a label closing
# with `](upload://…`. `\\+` swallows the whole run; the `(?=…)` lookahead
# scopes the match to upload labels — forbidding `[`/`]` in the gap means an
# escape only matches while it sits inside the label that ends in
# `](upload://`, so user-written escapes elsewhere stay intact. Scoping on the
# lookahead alone (rather than consuming the opening `[`) lets a single pass
# strip every escape in a label, e.g. `foo\_bar\_baz`, not just the first.
class StripUploadLabelEscapes < ActiveRecord::Migration[8.0]
disable_ddl_transaction!
BATCH_SIZE = 10_000
def up
min_id, max_id = DB.query_single("SELECT MIN(id), MAX(id) FROM posts")
return if max_id.nil?
current_id = min_id
while current_id <= max_id
DB.exec(<<~SQL, start_id: current_id, end_id: current_id + BATCH_SIZE)
UPDATE posts
SET raw = regexp_replace(
raw,
'\\\\+([_*~|`])(?=[^\\]\\[]*\\]\\(upload://)',
'\\1',
'g'
)
WHERE id >= :start_id
AND id < :end_id
AND raw ~ '\\\\+[_*~|`][^\\]\\[]*\\]\\(upload://'
SQL
current_id += BATCH_SIZE
end
end
def down
raise ActiveRecord::IrreversibleMigration
end
end
+1
View File
@@ -21824,6 +21824,7 @@ INSERT INTO "schema_migrations" (version) VALUES
('20260428072232'),
('20260424004343'),
('20260422144944'),
('20260422135650'),
('20260422130653'),
('20260422102523'),
('20260422062938'),
@@ -27,6 +27,7 @@ export default function makeEngine(
setupHoister(engine);
setupImageAndPlayableMediaRenderer(engine);
setupAttachments(engine);
setupLiteralizeUploadLabels(engine);
setupBlockBBCode(engine);
setupInlineBBCode(engine);
setupTextPostProcessRuler(engine);
@@ -329,6 +330,88 @@ function setupAttachments(engine) {
engine.renderer.rules.link_open = renderAttachment;
}
// Rebuilds a label's source text from the parsed inline tokens, treating
// emphasis/strikethrough delimiters as literal characters (using each token's
// `markup` field). Used to reverse inline parsing on labels we want to keep
// verbatim.
const EMPHASIS_MARKUP = /^[_*~]+$/;
function reconstructLiteralLabel(tokens) {
let result = "";
for (const token of tokens) {
if (token.type === "text" || token.type === "text_special") {
result += token.content;
} else if (token.type === "code_inline") {
const markup = token.markup || "`";
result += `${markup}${token.content}${markup}`;
} else if (token.type.endsWith("_open") || token.type.endsWith("_close")) {
// `markup` is only reliable as source text for emphasis/strong/strike
// delimiters. Other open/close tokens (linkify, autolink) store a
// descriptor like "linkify" — skip those.
const markup = token.markup || "";
if (EMPHASIS_MARKUP.test(markup)) {
result += markup;
}
} else if (token.type === "softbreak" || token.type === "hardbreak") {
result += " ";
} else if (token.children) {
result += reconstructLiteralLabel(token.children);
}
}
return result;
}
// Discourse generates upload markdown like `![name|100x100](upload://hash)`
// and `[name|attachment](upload://hash)`. By default markdown-it runs inline
// parsing on the label, which italicizes filenames containing `_`, strips
// them from the alt attribute, and can hide the `|attachment` marker behind
// formatting tokens.
//
// This core ruler walks the parsed tokens, finds links/images whose URL is
// an `upload://` short URL, and collapses their children back to a single
// literal text token. Emphasis and other inline formatting are preserved as
// literal characters so the rendered output matches the original filename.
function literalizeUploadLabels(state) {
for (const block of state.tokens) {
const children = block.children;
if (!children) {
continue;
}
for (let i = 0; i < children.length; i++) {
const token = children[i];
if (token.type === "image") {
if (token.attrGet("src")?.startsWith("upload://")) {
const literal = reconstructLiteralLabel(token.children);
const text = new state.Token("text", "", 0);
text.content = literal;
token.children = [text];
token.content = literal;
}
} else if (
token.type === "link_open" &&
token.attrGet("href")?.startsWith("upload://")
) {
// Markdown can't nest links, so the first `link_close` is ours.
let j = i + 1;
while (j < children.length && children[j].type !== "link_close") {
j++;
}
if (j > i + 1) {
const literal = reconstructLiteralLabel(children.slice(i + 1, j));
const text = new state.Token("text", "", 0);
text.content = literal;
children.splice(i + 1, j - i - 1, text);
}
}
}
}
}
function setupLiteralizeUploadLabels(engine) {
engine.core.ruler.push("literalize_upload_labels", literalizeUploadLabels);
}
// TODO we may just use a proper ruler from markdown it... this is a basic proxy
class Ruler {
constructor() {
@@ -1,7 +1,3 @@
export function escapeMarkdownCharacters(text) {
return text.replace(/[\\*_~`\[\]|]/g, "\\$&");
}
export function sanitizeAlt(text, options = {}) {
const fallback = options.fallback ?? "";
@@ -14,7 +10,7 @@ export function sanitizeAlt(text, options = {}) {
return fallback;
}
return escapeMarkdownCharacters(trimmed);
return trimmed.replace(/\|/g, "&#124;").replace(/([\\\[\]])/g, "\\$1");
}
/**
+2 -3
View File
@@ -1,6 +1,5 @@
import deprecated from "discourse/lib/deprecated";
import { getOwnerWithFallback } from "discourse/lib/get-owner";
import { escapeMarkdownCharacters } from "discourse/lib/markdown-image-builder";
import { humanizeList } from "discourse/lib/text";
import { capabilities } from "discourse/services/capabilities";
import I18n, { i18n } from "discourse-i18n";
@@ -285,7 +284,7 @@ function markdownNameFromFileName(fileName) {
name = i18n("upload_selector.default_image_alt_text");
}
return escapeMarkdownCharacters(name);
return name.replace(/\[|\]|\|/g, "");
}
function imageMarkdown(upload) {
@@ -301,7 +300,7 @@ function playableMediaMarkdown(upload, type) {
}
function attachmentMarkdown(upload) {
return `[${escapeMarkdownCharacters(upload.original_filename)}|attachment](${
return `[${upload.original_filename.replace(/\[|\]|\|/g, "")}|attachment](${
upload.short_url
}) (${I18n.toHumanSize(upload.filesize)})`;
}
@@ -1,5 +1,6 @@
import { module, test } from "qunit";
import { setupRenderingTest } from "discourse/tests/helpers/component-test";
import pretender, { response } from "discourse/tests/helpers/create-pretender";
import { testRenderedMarkdown } from "discourse/tests/helpers/rich-editor-helper";
module(
@@ -144,5 +145,15 @@ module(
.hasAttribute("data-orig-src", "upload://hash");
})
);
test("upload:// image alt text is preserved verbatim across round-trips", async function (assert) {
pretender.post("/uploads/lookup-urls", () => response([]));
await testRenderedMarkdown(
"![_test_file_|100x100](upload://hash)",
(a) => {
a.dom("img").hasAttribute("alt", "_test_file_");
}
).call(this, assert);
});
}
);
@@ -23,10 +23,10 @@ module("Unit | Lib | markdown-image-builder", function () {
assert.strictEqual(sanitizeAlt(" ", { fallback: "image" }), "image");
});
test("escapes markdown special characters", function (assert) {
test("escapes characters that would break markdown parsing", function (assert) {
assert.strictEqual(
sanitizeAlt("alt|text|with|pipes"),
"alt\\|text\\|with\\|pipes"
"alt&#124;text&#124;with&#124;pipes"
);
assert.strictEqual(
sanitizeAlt("text\\with\\slashes"),
@@ -36,14 +36,6 @@ module("Unit | Lib | markdown-image-builder", function () {
sanitizeAlt("text[with]brackets"),
"text\\[with\\]brackets"
);
assert.strictEqual(
sanitizeAlt("_underscores_ and *stars*"),
"\\_underscores\\_ and \\*stars\\*"
);
assert.strictEqual(
sanitizeAlt("~~strike~~ and `code`"),
"\\~\\~strike\\~\\~ and \\`code\\`"
);
});
test("trims whitespace", function (assert) {
@@ -142,7 +134,7 @@ module("Unit | Lib | markdown-image-builder", function () {
src: "/uploads/image.png",
alt: "text|with|pipes",
}),
"![text\\|with\\|pipes](/uploads/image.png)"
"![text&#124;with&#124;pipes](/uploads/image.png)"
);
});
@@ -321,7 +321,7 @@ module("Unit | Utility | uploads", function (hooks) {
);
assert.strictEqual(
testUploadMarkdown("[foo|bar].png"),
"![\\[foo\\|bar\\]|100x200](/uploads/123/abcdef.ext)"
"![foobar|100x200](/uploads/123/abcdef.ext)"
);
assert.strictEqual(
testUploadMarkdown("file name with space.png"),
@@ -342,7 +342,12 @@ module("Unit | Utility | uploads", function (hooks) {
assert.strictEqual(
testUploadMarkdown("_test_file_.txt", { short_url }),
`[\\_test\\_file\\_.txt|attachment](${short_url}) (42 Bytes)`
`[_test_file_.txt|attachment](${short_url}) (42 Bytes)`
);
assert.strictEqual(
testUploadMarkdown("[foo|bar].txt", { short_url }),
`[foobar.txt|attachment](${short_url}) (42 Bytes)`
);
});
+10 -9
View File
@@ -16,15 +16,12 @@ class UploadMarkdown
end
def image_markdown(display_name: nil)
display_name ||= @upload.original_filename
"![#{self.class.escape_markdown(display_name)}|#{@upload.width}x#{@upload.height}](#{@upload.short_url})"
"![#{display_label(display_name)}|#{@upload.width}x#{@upload.height}](#{@upload.short_url})"
end
def attachment_markdown(display_name: nil, with_filesize: true)
human_filesize = with_filesize ? " (#{@upload.human_filesize})" : ""
display_name ||= @upload.original_filename
"[#{self.class.escape_markdown(display_name)}|attachment](#{@upload.short_url})#{human_filesize}"
"[#{display_label(display_name)}|attachment](#{@upload.short_url})#{human_filesize}"
end
def playable_media_markdown(display_name: nil)
@@ -35,11 +32,15 @@ class UploadMarkdown
"video"
end
return attachment_markdown if !type
display_name ||= @upload.original_filename
"![#{self.class.escape_markdown(display_name)}|#{type}](#{@upload.short_url})"
"![#{display_label(display_name)}|#{type}](#{@upload.short_url})"
end
def self.escape_markdown(text)
text.gsub(/[\\*_~`\[\]|]/) { |c| "\\#{c}" }
private
# `[`, `]` and `|` would break the link/image syntax, and escaping them would
# reintroduce the rich-editor backslash doubling this whole change removes, so
# strip them from the label instead.
def display_label(display_name)
(display_name || @upload.original_filename).to_s.gsub(/[\[\]\|]/, "")
end
end
@@ -661,7 +661,7 @@ describe Chat::Message do
expect(message.to_markdown).to eq(<<~MSG.chomp)
hey friend, what's up?!
![test\\_image.jpg|400x300](#{image.short_url})
![test_image.jpg|400x300](#{image.short_url})
![meme.jpg|10x10](#{image2.short_url})
MSG
end
@@ -0,0 +1,33 @@
# frozen_string_literal: true
require Rails.root.join("db/post_migrate/20260422135650_strip_upload_label_escapes.rb")
describe StripUploadLabelEscapes do
around { |example| ActiveRecord::Migration.suppress_messages { example.run } }
def raw_for(id)
DB.query_single("SELECT raw FROM posts WHERE id = ?", id)[0]
end
it "strips accumulated backslashes from upload markdown labels" do
damaged = Fabricate(:post, raw: "![foo\\\\\\\\_bar|100x100](upload://abc.jpg)")
multi = Fabricate(:post, raw: "![My\\\\_Awesome\\\\_Photo|100x100](upload://abc.jpg)")
clean = Fabricate(:post, raw: "![foo_bar|100x100](upload://abc.jpg)")
outside =
Fabricate(
:post,
raw: "keep \\_these\\_ — fix ![name\\\\\\\\_x|100x100](upload://abc.jpg) here",
)
unrelated = Fabricate(:post, raw: "plain \\_escape\\_ in prose")
described_class.new.up
expect(raw_for(damaged.id)).to eq("![foo_bar|100x100](upload://abc.jpg)")
expect(raw_for(multi.id)).to eq("![My_Awesome_Photo|100x100](upload://abc.jpg)")
expect(raw_for(clean.id)).to eq("![foo_bar|100x100](upload://abc.jpg)")
expect(raw_for(outside.id)).to eq(
"keep \\_these\\_ — fix ![name_x|100x100](upload://abc.jpg) here",
)
expect(raw_for(unrelated.id)).to eq("plain \\_escape\\_ in prose")
end
end
+54
View File
@@ -2569,6 +2569,60 @@ HTML
end
end
describe "upload:// links" do
it "treats the label as literal so formatting characters are preserved" do
cooked = PrettyText.cook <<~MD
![_test_file_|100x100](upload://abc.jpg)
[_test_file_.txt|attachment](upload://abc.txt)
MD
expect(cooked).to include('alt="_test_file_"')
expect(cooked).to include('class="attachment"')
expect(cooked).to include(">_test_file_.txt<")
expect(cooked).not_to include("<em>")
end
it "unescapes backslash escapes left over from legacy posts" do
cooked = PrettyText.cook("![20260421\\_140231|100x100](upload://abc.jpg)")
expect(cooked).to include('alt="20260421_140231"')
end
it "leaves non-upload links alone" do
cooked = PrettyText.cook("[_foo_](http://example.com)")
expect(cooked).to include("<em>foo</em>")
end
it "keeps plain URLs in the label intact when they would otherwise linkify" do
cooked = PrettyText.cook("![foo https://example.com bar|100x100](upload://abc.jpg)")
expect(cooked).to include('alt="foo https://example.com bar"')
end
it "keeps hashtags and mentions in the label literal" do
cooked = PrettyText.cook("[#cat @sam|attachment](upload://abc.txt)")
expect(cooked).to include(">#cat @sam<")
expect(cooked).not_to include("hashtag")
expect(cooked).not_to include("mention")
end
it "treats reference-style upload labels as literal too" do
cooked = PrettyText.cook("[_foo_][1]\n\n[1]: upload://abc.jpg")
expect(cooked).to include(">_foo_<")
expect(cooked).not_to include("<em>")
end
it "still renders inline formatting in non-upload attachment labels" do
cooked = PrettyText.cook("[**bold**|attachment](https://example.com/file.pdf)")
expect(cooked).to include('class="attachment"')
expect(cooked).to include("<strong>bold</strong>")
end
end
describe "upload decoding" do
it "can decode upload:// for default setup" do
set_cdn_url("https://cdn.com")
+22 -10
View File
@@ -16,25 +16,25 @@ RSpec.describe UploadMarkdown do
)
expect(UploadMarkdown.new(video).to_markdown).to eq(<<~MD.chomp)
![test\\_video.mp4|video](#{video.short_url})
![test_video.mp4|video](#{video.short_url})
MD
expect(UploadMarkdown.new(audio).to_markdown).to eq(<<~MD.chomp)
![test\\_audio.mp3|audio](#{audio.short_url})
![test_audio.mp3|audio](#{audio.short_url})
MD
expect(UploadMarkdown.new(attachment).to_markdown).to eq(<<~MD.chomp)
[test\\_file.pdf|attachment](#{attachment.short_url}) (#{attachment.human_filesize})
[test_file.pdf|attachment](#{attachment.short_url}) (#{attachment.human_filesize})
MD
expect(UploadMarkdown.new(image).to_markdown).to eq(<<~MD.chomp)
![test\\_img.jpg|100x200](#{image.short_url})
![test_img.jpg|100x200](#{image.short_url})
MD
unknown = Fabricate(:upload, original_filename: "test_video.mmmppp444", extension: "mmmppp444")
expect(UploadMarkdown.new(unknown).playable_media_markdown).to eq(<<~MD.chomp)
[test\\_video.mmmppp444|attachment](#{unknown.short_url}) (#{unknown.human_filesize})
[test_video.mmmppp444|attachment](#{unknown.short_url}) (#{unknown.human_filesize})
MD
end
it "escapes markdown characters in attachment filenames" do
it "renders filenames with markdown formatting characters literally" do
SiteSetting.authorized_extensions = "txt"
{
@@ -45,15 +45,27 @@ RSpec.describe UploadMarkdown do
"`code`.txt" => "<code>",
}.each do |filename, bad_tag|
upload = Fabricate(:upload, original_filename: filename, extension: "txt")
markdown = UploadMarkdown.new(upload).attachment_markdown
cooked = PrettyText.cook(markdown)
cooked = PrettyText.cook(UploadMarkdown.new(upload).attachment_markdown)
expect(cooked).to include('class="attachment"'),
"expected attachment class for filename: #{filename}\ncooked: #{cooked}"
expect(cooked).not_to include(bad_tag),
"unexpected #{bad_tag} in cooked output for filename: #{filename}\ncooked: #{cooked}"
expect(cooked).to include(filename.gsub("\\", "")),
"expected display name for filename: #{filename}\ncooked: #{cooked}"
expect(cooked).to include(filename),
"expected filename in cooked output for: #{filename}\ncooked: #{cooked}"
end
end
it "strips structural markdown characters ([, ], |) from upload labels" do
SiteSetting.authorized_extensions = "txt|jpg"
attachment = Fabricate(:upload, original_filename: "a]b[c|d.txt", extension: "txt")
image =
Fabricate(:upload, width: 1, height: 1, original_filename: "x|y[z].jpg", extension: "jpg")
expect(UploadMarkdown.new(attachment).attachment_markdown).to eq(
"[abcd.txt|attachment](#{attachment.short_url}) (#{attachment.human_filesize})",
)
expect(UploadMarkdown.new(image).image_markdown).to eq("![xyz.jpg|1x1](#{image.short_url})")
end
end
+12 -2
View File
@@ -613,7 +613,17 @@ RSpec.describe InlineUploads do
[test3|attachment](#{upload.short_url})
[test3|attachment](#{upload2.short_url})[test3|attachment](#{upload3.short_url})
[This is some \\_test\\_ here|attachment](#{upload3.short_url})
[This is some _test_ here|attachment](#{upload3.short_url})
MD
end
it "strips structural markdown characters from attachment labels so the link can't break" do
md = <<~MD
<a class="attachment" href="#{upload.url}">a]b|c_d</a>
MD
expect(InlineUploads.process(md)).to eq(<<~MD)
[abc_d|attachment](#{upload.short_url})
MD
end
@@ -762,7 +772,7 @@ RSpec.describe InlineUploads do
image_upload
end
expect(raw).to eq("look at this:\n![image\\]1](#{image_upload.short_url})")
expect(raw).to eq("look at this:\n![image1](#{image_upload.short_url})")
end
end
end