Skip to content

fix: quoted email addresses and status responses #10980

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

SebastianKrupinski
Copy link
Contributor

@SebastianKrupinski SebastianKrupinski commented Apr 8, 2025

Resolves

Client ticket - No avatar 400 responses trigger security IP bans

Summary

  • Added email normalization
  • Unified email validation
  • Adjusted formatting
  • Added tests for email variations

Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

Looks good otherwise.

Comment on lines 116 to 126
public function testNormalizeEmailWithExtraSpaces() {
$email = ' [email protected] ';
$normalizedEmail = $this->invokePrivate($this->controller, 'normalizeEmail', [$email]);
$this->assertEquals('[email protected]', $normalizedEmail);
}

public function testNormalizeEmailWithSingleQuotes() {
$email = "'[email protected]'";
$normalizedEmail = $this->invokePrivate($this->controller, 'normalizeEmail', [$email]);
$this->assertEquals('[email protected]', $normalizedEmail);
}
Copy link
Member

@st3iny st3iny Apr 8, 2025

Choose a reason for hiding this comment

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

You could use a data provider here.

public static function normalizeEmailDataProvider(): array {
	return [
		// [input, expected]
		['   [email protected]   ', '[email protected]'],
		["'[email protected]'", '[email protected]'],
		// Add more tests at your discretion ...
	];
}

/**
 * @dataProvider normalizeEmailDataProvider
 */
public function testNormalizeEmail(string $email, string $expected): void {
	$normalizedEmail = $this->invokePrivate($this->controller, 'normalizeEmail', [$email]);
	$this->assertEquals($expected, $normalizedEmail);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Done

@kesselb
Copy link
Contributor

kesselb commented Apr 8, 2025

Having a ' in the local part (e.g. '[email protected]) seems to be a valid email address. normalizeEmail currently does not take that into account.

Seeing requests like /index.php/apps/mail/api/avatars/url/'[email protected]' is suspicious. I can replicate such requests by adding a contact with email address '[email protected]'.

image

I find the avatar endpoint the wrong place to normalize email addresses. That email address is already broken, and it just explodes over there.

Unless there's another explanation of those requests, I would prefer to change the AutoComplete controller and run all emails/contacts through filter_var to not suggests contacts with invalid email addresses.

@SebastianKrupinski
Copy link
Contributor Author

Having a ' in the local part (e.g. '[email protected]) seems to be a valid email address. normalizeEmail currently does not take that into account.

Seeing requests like /index.php/apps/mail/api/avatars/url/'[email protected]' is suspicious. I can replicate such requests by adding a contact with email address '[email protected]'.

image

I find the avatar endpoint the wrong place to normalize email addresses. That email address is already broken, and it just explodes over there.

Unless there's another explanation of those requests, I would prefer to change the AutoComplete controller and run all emails/contacts through filter_var to not suggests contacts with invalid email addresses.

Its happening on received emails, I'll share the ticket with you internally

@kesselb
Copy link
Contributor

kesselb commented Apr 9, 2025

Thanks, I reviewed the support ticket yesterday before replying here. I had a hunch about who created it because I worked on a similar case for them a while ago.

  1. All requests are like /index.php/apps/mail/api/avatars/url/'[email protected]'. It's unusual that the email address is wrapped in quotes. I haven't seen that before. Are we sure this isn't an issue in our frontend code where quotes are accidentally added?

  2. I dislike the idea of adding normalization to the Avatar endpoints. These endpoints expect a valid email address as input; if not, they should return an error. We should fix the code that is incorrectly calling these endpoints.

  3. You mentioned that the problem occurs for received emails. How do you know this from the provided data? The fetchAvatarUrlMemoized method generates that request and is called when rendering the user avatar for incoming messages (1) and also when composing a new message for contact autocompletion (2).

@SebastianKrupinski
Copy link
Contributor Author

Thanks, I reviewed the support ticket yesterday before replying here. I had a hunch about who created it because I worked on a similar case for them a while ago.

1. All requests are like `/index.php/apps/mail/api/avatars/url/'[email protected]'`. It's unusual that the email address is wrapped in quotes. I haven't seen that before. Are we sure this isn't an issue in our frontend code where quotes are accidentally added?

I've seen this in the past some desktop clients, smtp server and online service wrap the email addresses in quotes.

I can reproduce the issue in but modifying a email any manually uploading it. THIS IS NOT a issue with our front end.

image

2. I dislike the idea of adding normalization to the Avatar endpoints. These endpoints expect a valid email address as input; if not, they should return an error. We should fix the code that is incorrectly calling these endpoints.

There is nothing wrong with normalizing input for things like letter cases and spaces, you are just confirming that he input is in a proper format, especially when you need to match an existing item.

3. You mentioned that the problem occurs for received emails. How do you know this from the provided data? 

If you look at the logs that the client sent, you can see 14 failures in 1 second.

@kesselb
Copy link
Contributor

kesselb commented Apr 10, 2025

Hi,

As mentioned earlier, I reviewed the support ticket and the provided logs.

If you look at the logs that the client sent, you can see 14 failures in 1 second.

I was able to reproduce the problem with a broken contact, which led me to wonder: how do you determine from the customer's data that their issue is triggered by an incoming email rather than a malformed contact in the address book?

I can reproduce the issue in but modifying a email any manually uploading it.

Did you modify an email by adding quotes around the "From" header, like this:

Return-Path: <'[email protected]'>
Received: from localhost (unknown [172.17.0.1])
        by 2a1621b014d0 (Mailpit) with SMTP
        for <[email protected]>; Thu, 10 Apr 2025 12:34:28 +0200 (CEST)
From: '[email protected]'
To: [email protected]
Subject: Quoted from
Message-ID: <20250410103428.Horde.w4R3pQY8zELj5x1-wISStP8@pc>
User-Agent: Horde Application Framework 5
Date: Thu, 10 Apr 2025 10:34:28 +0000
Content-Type: text/html; charset=utf-8
Content-Description: HTML Version of Message
MIME-Version: 1.0

<html><meta http-equiv="content-type" content="text/html; charset=UTF-8"><body>Hi, email with a quoted from header</body><html>

There is nothing wrong with normalizing input for things like letter cases and spaces

I'm not opposed to normalizing the input, but we should do it correctly. As mentioned before, the avatar endpoint is where the issue becomes apparent. I assume most of our code expects the email address to be syntactically valid. For example, if we reply to an email with a quoted "From" header, the remote server might reject it due to an invalid format.

If we want to support such email formats, we should normalize them much earlier. Otherwise, we'll need to apply this normalization in many more places.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🏗️ In progress
Development

Successfully merging this pull request may close these issues.

3 participants