Skip to content

Require a local-part to be a valid email address in AUTOMAIL_RE. #1165

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Aug 11, 2021

Conversation

nzlosh
Copy link
Contributor

@nzlosh nzlosh commented Aug 1, 2021

Valid email addresses require a local-part before the @ followed by domain. This PR updates the AUTOMAIL_RE to use the + quantifier to require 1 or more characters in the local-part instead of the * quantifier which allows 0 characters.

The current regex allows the invalid addresses in the form <@domain>. My particular use case is formatting messages in Slack, where mentioned users take the form <@Uxxxxx>. markdown is incorrectly processing the mention string form as an email address. I still want markdown's automail feature for processing email addressing in the text.

The below code demonstrates the current and new behaviour:

>>> AUTOMAIL_RE = r'<([^<> !]*@[^@<> ]*)>'
>>> print(re.match(AUTOMAIL_RE, "<@localhost>"))
<re.Match object; span=(0, 12), match='<@localhost>'>
>>> print(re.match(AUTOMAIL_RE, "<bob@localhost>"))
<re.Match object; span=(0, 15), match='<bob@localhost>'>

>>> AUTOMAIL_RE = r'<([^<> !]+@[^@<> ]*)>'
>>> print(re.match(AUTOMAIL_RE, "<@localhost>"))
None
>>> print(re.match(AUTOMAIL_RE, "<bob@localhost>"))
<re.Match object; span=(0, 15), match='<bob@localhost>'>

@mitya57
Copy link
Collaborator

mitya57 commented Aug 1, 2021

Make it makes sense to also replace * with + in the domain part? With your fix it still allows for addresses like bob@:

>>> AUTOMAIL_RE = r'<([^<> !]+@[^@<> ]*)>'
>>> re.match(AUTOMAIL_RE, "<bob@>")
<re.Match object; span=(0, 6), match='<bob@>'>

@nzlosh
Copy link
Contributor Author

nzlosh commented Aug 1, 2021

I've updated the regex to require both local-part and domain.

@nzlosh
Copy link
Contributor Author

nzlosh commented Aug 2, 2021

I've added tests for the missing local-part and domain. I noticed that the <> characters are encoded in html form &lt;&gt; for the missing local-part, but not for the missing domain part. The tests mirror this behaviour, but I don't know if it's expected behaviour or not.

Copy link
Member

@waylan waylan left a comment

Choose a reason for hiding this comment

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

All new tests should use the new testing framework. It appears that no changes have been made to the autolinks since introducing the new framework so we don't have a file for that yet. One should be added at tests/test_syntax/inline/autolinks.py. You only need to include the tests for the behaviors relevant to this PR. However, you may add additional tests if you would like.

Regarding the differing behaviors, I see that as a problem. If any part of the autolink does not match, then the behavior should be the same.

@waylan waylan added the requires-changes Awaiting updates after a review. label Aug 2, 2021
@nzlosh nzlosh force-pushed the automail_fix branch 2 times, most recently from 80eee41 to c44c06e Compare August 4, 2021 09:18
@nzlosh
Copy link
Contributor Author

nzlosh commented Aug 4, 2021

Thanks for the pointers, I've updated the PR with the recommended changes.

nzlosh added a commit to nzlosh/markdown that referenced this pull request Aug 8, 2021
nzlosh added a commit to nzlosh/markdown that referenced this pull request Aug 8, 2021
nzlosh added a commit to nzlosh/markdown that referenced this pull request Aug 8, 2021
@waylan waylan merged commit 9aa4586 into Python-Markdown:master Aug 11, 2021
@waylan waylan added approved The pull request is ready to be merged. and removed requires-changes Awaiting updates after a review. labels Aug 11, 2021
@nzlosh nzlosh deleted the automail_fix branch August 11, 2021 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved The pull request is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants