Skip to content

limit link schemes in in messages in e-mail push notifications#19741

Open
RamReso wants to merge 2 commits into
element-hq:developfrom
RamReso:misc-mail-notification-sanitization
Open

limit link schemes in in messages in e-mail push notifications#19741
RamReso wants to merge 2 commits into
element-hq:developfrom
RamReso:misc-mail-notification-sanitization

Conversation

@RamReso
Copy link
Copy Markdown
Contributor

@RamReso RamReso commented Apr 29, 2026

Resolves #2860. This pull request limits the allowed link schemes from messages contained in e-mail push notifications.

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Code style is correct (run the linters)

@RamReso RamReso requested a review from a team as a code owner April 29, 2026 14:35
Comment thread synapse/push/mailer.py
attributes=ALLOWED_ATTRS,
# bleach master has this, but it isn't released yet
# protocols=ALLOWED_SCHEMES,
protocols=ALLOWED_SCHEMES,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally: add a test case (or otherwise) demonstrate that safe_markup makes use of the this schema allowlist.

-- @DMRobertson, matrix-org/synapse#2860 (comment)

A test would be good. Have you at least tested this manually? Please provide some instructions

Comment thread synapse/push/mailer.py
@@ -112,7 +112,7 @@
"img": ["src"],
}
# When bleach release a version with this option, we can specify schemes
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment is no longer relevant since we're using a version of bleach that supports this

Suggested change
# When bleach release a version with this option, we can specify schemes

Comment thread synapse/push/mailer.py
attributes=ALLOWED_ATTRS,
# bleach master has this, but it isn't released yet
# protocols=ALLOWED_SCHEMES,
protocols=ALLOWED_SCHEMES,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we even need to define this?

The list of ALLOWED_PROTOCOLS now defaults to http, https
and mailto

-- https://github.com/mozilla/bleach/releases/tag/v1.5

And I can see it in their code: https://github.com/mozilla/bleach/blob/913ab75992b845e2c9c060c41f24d46921db4693/bleach/sanitizer.py#L37-L38

Comment thread synapse/push/mailer.py
attributes=ALLOWED_ATTRS,
# bleach master has this, but it isn't released yet
# protocols=ALLOWED_SCHEMES,
protocols=ALLOWED_SCHEMES,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make it more clear, the minimum version of bleach needed to use the protocols argument is 1.5 (https://github.com/mozilla/bleach/releases/tag/v1.5)

We're now using bleach>=3.2.0 as our minimum so we're good on this front ✅

synapse/pyproject.toml

Lines 75 to 76 in 8eb220a

# 3.2.0 updates collections.abc imports to avoid Python 3.10 incompatibility.
"bleach>=3.2.0",

Comment thread synapse/push/mailer.py
Comment on lines 114 to +115
# When bleach release a version with this option, we can specify schemes
# ALLOWED_SCHEMES = ["http", "https", "ftp", "mailto"]
ALLOWED_SCHEMES = ["http", "https", "mailto"]
Copy link
Copy Markdown
Contributor

@MadLittleMods MadLittleMods May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending on the answer to #19741 (comment) we can at-least use this PR remove this entire comment (and the commented out usage).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Upgrade to Bleach 1.5, to limit link schemes (of links in messages in e-mail push notifications) to an allowlist

2 participants