[2.x] fix: stop email values being parsed as markup - #4874
Merged
Conversation
Email bodies are translation strings containing markup — a markdown link whose text is a discussion title, say — rendered by the formatter after their parameters have been substituted in. That order puts user values in front of the parser: a title containing ](...) closes the intended link and opens its own, so a notification sent by the forum points wherever its author likes, and an image reference turns the mail into a beacon that fires when it is opened. Display names reach the same templates, and are often self-service. Email views now get a translator that replaces parameter values with opaque markers, and a formatter that puts the values back, escaped, once rendering is done. The parser never sees them. Marking happens per call with the value carried inside the marker, so templates that render several times — core's notification view converts three times, and extension views render a preview alongside the body — cannot mix values between renders. Swapping the pair for email views rather than fixing each template means no template changes, which matters because the templates at risk live mostly in extensions, many of which will never be updated. Reported by Arpit Jain (@arpitjain099).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
Notification email bodies are translation strings containing markup, with user-supplied values as the link text:
{poster_display_name} just posted in a discussion you're following: [{title}]({url}).The parameters are substituted before the string is handed to the formatter, so the parser sees the values. With the markdown extension enabled — bundled, and on most forums — a discussion title of:
closes the intended link and opens its own, producing a notification, sent by the forum with its correct headers and styling, whose visible link goes wherever the discussion's author chose:
An image reference in a title (
) instead turns the email into a tracking beacon that fires on open, disclosing the recipient's address and client to a third party. 80 characters is ample for either.The surface is wider than the title: the same templates interpolate display names (
{poster_display_name},{mentioner_display_name},{replier_display_name},{user_display_name}), which are often self-service, acrosssubscriptions,mentionsandmessages. Even without the markdown extension, core's own autolinker turns a bare URL in a title into a clickable link in the email.The fix
Email views are given a translator that replaces parameter values with opaque markers, and a formatter that puts the values back — HTML-escaped — once rendering is done. The parser never sees user data.
MailServiceProvider) rather than fixing nine call sites matters because most affected templates live in extensions, and many will never be updated. Third-party notification emails are fixed by upgrading core.convert()three times, and extension views render a preview beside the body — cannot mix values between renders.+/=are altered by markdown parsing and URL encoding; letters and digits survive both, so{url}in a link destination still round-trips.Version 2\.0 \- what\'s new\!) and, for one payload, destroys the notification link entirely — a user-visible regression on every forum, and hand-rolled CommonMark escaping is a future bug in its own right.Tests
framework/core— 8 tests covering what core alone guarantees: a value can never be autolinked, embed an image, redirect a link or inject HTML, while the trusted template still renders and ordinary titles are not littered.extensions/subscriptions— 6 tests with markdown enabled, using the real body template, covering the full exploit.flarum/markdownadded torequire-devfor this.Both suites were written first and observed failing on the real attack. Verified live on a forum with markdown enabled: both payloads render as inert text inside the correct link.
Regression: core mail/notification (20), forum/frontend (49), subscriptions (21), mentions (82), messages (12), suspend (19), PHPStan — all pass.
Note on the test suite
The subscriptions test inlines the body template rather than translating its key, because integration tests do not register
Extend\Localestranslations (#4600) —trans()would return the key and leave nothing to attack. There is a@todoto switch back when that is fixed in 2.1.Credit
Reported by Arpit Jain (@arpitjain099), who verified it by reading the source and was explicit about not having run it. No advisory or CVE is issued, per the policy of not treating pre-stable releases as covered — 2.x is still RC.