Update email validation to be strict

[EgoMaw]

fixes #5576

[MrSoulPenguin]

Might be worth expanding this to the StoreSubuserRequest validation as well.

But that will probably require a retooling of the test for excessively long subuser emails, since I believe email:strict imposes character limit constraints on different parts of the email, not just a single max length.

[EgoMaw]

From what I can tell, it imposes a single max length of 254 characters.

https://github.com/egulias/EmailValidator/blob/4.x/src/EmailParser.php#L15

but the test test_subuser_with_excessively_long_email_cannot_be_created() does indeed fail

[MrSoulPenguin]

The email gets split into different parts, the local before the @ and the domain after. Each with their own warnings.

A max total domain limit and limit per domain label.
https://github.com/egulias/EmailValidator/blob/4.x/src/Parser/DomainPart.php#L26

And a max local limit.
https://github.com/egulias/EmailValidator/blob/4.x/src/Warning/LocalTooLong.php#L8

Which are then recombined and checked against a max full email length of 254.

[EgoMaw]

Ok yeah i just saw that, my bad. It shouldn't be too difficult to fix the test, since we know the limits

[MrSoulPenguin]

I actually took a crack at this a couple days ago, which is when I found the issue with the subuser test and have updated tests. Just hadn't made a PR yet in case there was something I was missing.

If it helps, I can provided snippets of the tests I put together, see if anything could be changed.

[EgoMaw]

Sure thing

[MrSoulPenguin]

Alright,

Here is the tweaked long email test in CreateServerSubuserTest.php, kept it to just testing the column limit:

/**
 * Throws some bad data at the API and ensures that a subuser cannot be created.
 */
public function testSubuserWithExcessivelyLongEmailCannotBeCreated()
{
    [$user, $server] = $this->generateTestAccount();

    /*
     * RFCs limit certain parts of an email to certain character limits.
     *
     * A limit of <= 64 for the local, then <= 63 for each domain label.
     * We will stay below the limit to make sure we're within the 191 column limit for emails.
     */
    $local = str_repeat(Str::random(10), 6) . '1234';
    $label = str_repeat(Str::random(10), 6) . '1';

    // Make sure we're within the column limit
    $email = "$local@$label.$label.au";

    $this->assertSame(64, strlen($local));
    $this->assertSame(61, strlen($label));
    $this->assertSame(191, strlen($email));

    $response = $this->actingAs($user)->postJson($this->link($server) . '/users', [
        'email' => $email,
        'permissions' => [
            Permission::ACTION_USER_CREATE,
        ],
    ]);

    $response->assertOk();

    // Exceed column limit of 1 >= and <= 191
    $email = "$local@$label.$label.com";

    $this->assertSame(192, strlen($email));

    $response = $this->actingAs($user)->postJson($this->link($server) . '/users', [
        'email' => $email,
        'permissions' => [
            Permission::ACTION_USER_CREATE,
        ],
    ]);

    $response->assertStatus(Response::HTTP_UNPROCESSABLE_ENTITY);
    $response->assertJsonPath('errors.0.detail', 'The email must be between 1 and 191 characters.');
    $response->assertJsonPath('errors.0.meta.source_field', 'email');
}

And here is a new test I added to AccountControllerTest.php to test the actual RFC constraints, didn't test the upper 254 limit since the database column limit would take effect before that was hit:

/**
 * Test the the user's email follows different RFC constrains that apply.
 */
public function testEmailRejectsLocalAndLabelOverflow()
{
    $user = User::factory()->create();

    /*
     * RFCs limit certain parts of an email to certain character limits.
     *
     * A limit of <= 64 for the local, then <= 63 for each domain label.
     * 
     * We will stay below the limit initially to make sure we're within the 191 column limit for emails, so
     * later we can test each part separately without exceed the column limit without intending to.
     */
    $local = str_repeat(Str::random(10), 6) . '1234';
    $label = str_repeat(Str::random(10), 6) . '1';

    $email = "$local@$label.$label";

    $this->assertSame(64, strlen($local));
    $this->assertSame(61, strlen($label));
    $this->assertSame(188, strlen($email));

    // Baseline
    $this->actingAs($user)->putJson('/api/client/account/email', [
        'email' => $email,
        'password' => 'password',
    ])->assertNoContent();

    // Exceed local limit of <= 64
    $email = "1$local@$label.$label";

    $this->assertSame(65, strlen("1$local"));
    $this->assertSame(189, strlen($email));

    $response = $this->actingAs($user)->putJson('/api/client/account/email', [
        'email' => $email,
        'password' => 'password',
    ]);

    $response->assertStatus(Response::HTTP_UNPROCESSABLE_ENTITY);
    $response->assertJsonPath('errors.0.detail', 'The email must be a valid email address.');
    $response->assertJsonPath('errors.0.meta.source_field', 'email');

    // Exceed label limit of <= 63
    $email = "$local@123$label.$label";

    $this->assertSame(64, strlen("123$label"));
    $this->assertSame(191, strlen($email));

    $response = $this->actingAs($user)->putJson('/api/client/account/email', [
        'email' => $email,
        'password' => 'password',
    ]);

    $response->assertStatus(Response::HTTP_UNPROCESSABLE_ENTITY);
    $response->assertJsonPath('errors.0.detail', 'The email must be a valid email address.');
    $response->assertJsonPath('errors.0.meta.source_field', 'email');
}

[EgoMaw]

I dont think we need an entire new test for the RFC limits, I merged the assertions into the testEmailIsNotUpdatedWhenNotValid test instead

[MrSoulPenguin]

Makes sense, since it wasn't testing the full spec anyway.