Skip to content

[Mailer] Fix parsing Dsn with empty user/password #39531

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 17, 2020
Merged

[Mailer] Fix parsing Dsn with empty user/password #39531

merged 1 commit into from
Dec 17, 2020

Conversation

OskarStark
Copy link
Contributor

@OskarStark OskarStark commented Dec 17, 2020 •
edited
Loading

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets no
License MIT
Doc PR no

While working on a PR for Notifier that user and password would be parsed as an empty string, which is not wrong, but not expected IMO. Thi

scheme://@symfony.com and scheme://:@symfony.com should be a valid scheme with user and pass null

Another fix would be to check for ://@ and ://:@ and throw an InvalidArgumentException WDYT?

The final solution will then be applied to the Notifier DSN in 5.1

@OskarStark
Copy link
Contributor Author

Ready to merge from my side 👍

@nicolas-grekas nicolas-grekas added this to the 4.4 milestone Dec 17, 2020
@nicolas-grekas nicolas-grekas changed the base branch from 5.x to 4.4 December 17, 2020 08:40
@nicolas-grekas nicolas-grekas changed the title [Mailer] Fix Dsn [Mailer] Fix parsing Dsn with empty user/password Dec 17, 2020
nicolas-grekas
nicolas-grekas reviewed Dec 17, 2020
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Now, I'm wondering why we should do this?
If ppl mess up with their DSN, that's their issue.
And if ppl have the empty string as user/pass, why prevent sending them?

@OskarStark
Copy link
Contributor Author

Now, I'm wondering why we should do this?
If ppl mess up with their DSN, that's their issue.
And if ppl have the empty string as user/pass, why prevent sending them?

Let me explain it while using an example from Notifier component based on this code:

symfony/src/Symfony/Component/Notifier/Bridge/Sendinblue/SendinblueTransportFactory.php

Lines 27 to 46 in 29d62df

/**
* @return SendinblueTransport
*/
public function create(Dsn $dsn): TransportInterface
{
if (!$sender = $dsn->getOption('sender')) {
throw new IncompleteDsnException('Missing sender.', $dsn->getOriginalDsn());
}
$scheme = $dsn->getScheme();
$apiKey = $this->getUser($dsn);
$host = 'default' === $dsn->getHost() ? null : $dsn->getHost();
$port = $dsn->getPort();
if ('sendinblue' === $scheme) {
return (new SendinblueTransport($apiKey, $sender, $this->client, $this->dispatcher))->setHost($host)->setPort($port);
}
throw new UnsupportedSchemeException($dsn, 'sendinblue', $this->getSupportedSchemes());
}

We provide a getUser method which should throw an exception if the user is null (same for password):

symfony/src/Symfony/Component/Notifier/Transport/AbstractTransportFactory.php

Lines 47 to 65 in 29d62df

protected function getUser(Dsn $dsn): string
{
$user = $dsn->getUser();
if (null === $user) {
throw new IncompleteDsnException('User is not set.', $dsn->getOriginalDsn());
}
return $user;
}
protected function getPassword(Dsn $dsn): string
{
$password = $dsn->getPassword();
if (null === $password) {
throw new IncompleteDsnException('Password is not set.', $dsn->getOriginalDsn());
}
return $password;
}

And if it is null we throw an exception, but if it is an empty string this would not bubble up and and empty string in the password could lead to a bad experience.

What I want to achieve with this PR is, that the user who implement this bridge can rely on the getUser method and get an exception if this is the case. For me its better to handle this here.

@fabpot
Copy link
Member

fabpot commented Dec 17, 2020

Thank you @OskarStark.

@fabpot fabpot merged commit 72fb034 into symfony:4.4 Dec 17, 2020
@OskarStark OskarStark deleted the fix-dsn branch December 17, 2020 16:58
@OskarStark OskarStark mentioned this pull request Dec 17, 2020
Merged
fabpot added a commit that referenced this pull request Dec 18, 2020
@fabpot
bug #39538 [Notifier] Fix parsing Dsn with empty user/password (Oskar…
24334cb 
…Stark)

This PR was squashed before being merged into the 5.1 branch.

Discussion
----------

[Notifier] Fix parsing Dsn with empty user/password

| Q             | A
| ------------- | ---
| Branch?       | 5.1
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | ---
| License       | MIT
| Doc PR        | ---

Same like #39531, but for Notifier component.

I backported the DsnTest from `5.2` to `5.1`

Commits
-------

a80409a [Notifier] Fix parsing Dsn with empty user/password
This was referenced Dec 18, 2020
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Bug Mailer Status: Reviewed
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
4 participants
@OskarStark @fabpot @nicolas-grekas @carsonbot