Skip to content
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

[Mime] Relaxing in-reply-to header validation #44732

Merged
merged 1 commit into from Dec 21, 2021

Conversation

ThomasLandauer
Copy link
Contributor

@ThomasLandauer ThomasLandauer commented Dec 20, 2021

Q A
Branch? 6.1
Bug fix? no
New feature? ?
Deprecations? no
Tickets Fix #37361
License MIT
Doc PR not necessary

@nicolas-grekas

  1. Is it OK to just use UnstructuredHeader?
  2. Some tests at IdentificationHeaderTest are irrelevant now (but still pass) - should I remove them? And create some new test cases in UnstructuredHeaderTest? Or rely on every aspect being tested with other headers there, and don't add anything?

@stof
Copy link
Member

@stof stof commented Dec 20, 2021

would be great to add a test to prevent regressions

@ThomasLandauer
Copy link
Contributor Author

@ThomasLandauer ThomasLandauer commented Dec 20, 2021

Hm, since the header type is passed as string...

$header = new UnstructuredHeader('Subject', 'Test');

...how can a regression be tested?
Create a new test for the entries in private const HEADER_CLASS_MAP?

@stof
Copy link
Member

@stof stof commented Dec 20, 2021

@ThomasLandauer create an Email with a In-Reply-To header (or a References header for the other test to add) being a value that does not respect the msg-id spec, i.e. exactly what fails with the current code of the component.

@ThomasLandauer
Copy link
Contributor Author

@ThomasLandauer ThomasLandauer commented Dec 20, 2021

OK, thanks - like this?

@stof
Copy link
Member

@stof stof commented Dec 21, 2021

@nicolas-grekas @fabpot based on the issue being solved, I suggest we consider this as a bugfix.

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Dec 21, 2021

Works for me

@stof stof removed this from the 6.1 milestone Dec 21, 2021
@stof stof added this to the 4.4 milestone Dec 21, 2021
stof
stof approved these changes Dec 21, 2021
@ThomasLandauer ThomasLandauer changed the title [Mime] Relexing in-reply-to header validation [Mime] Relaxing in-reply-to header validation Dec 21, 2021
fabpot
fabpot approved these changes Dec 21, 2021
@fabpot fabpot changed the base branch from 6.1 to 4.4 Dec 21, 2021
@fabpot
Copy link
Member

@fabpot fabpot commented Dec 21, 2021

Thank you @ThomasLandauer.

@fabpot fabpot merged commit 3526a32 into symfony:4.4 Dec 21, 2021
4 of 11 checks passed
@ThomasLandauer ThomasLandauer deleted the in-reply-to-header branch Dec 21, 2021
@fabpot fabpot mentioned this pull request Dec 29, 2021
@fabpot fabpot mentioned this pull request Dec 29, 2021
@fabpot fabpot mentioned this pull request Dec 29, 2021
@fabpot fabpot mentioned this pull request Dec 29, 2021
@micheh
Copy link
Contributor

@micheh micheh commented Dec 31, 2021

@nicolas-grekas Isn't this a BC break? The userland code has to be changed when updating from 5.4.1 to 5.4.2 (use the text header and add the brackets yourself).

Before:

$headers->addIdHeader('References', $messageIds);
$headers->addIdHeader('In-Reply-To', $messageId);

After:

$headers->addTextHeader('References', '<' . implode('> <', $messageIds) . '>');
$headers->addTextHeader('In-Reply-To', '<' . $messageId. '>');

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Dec 31, 2021

Please open issues if you want further discussion.

@ThomasLandauer
Copy link
Contributor Author

@ThomasLandauer ThomasLandauer commented Jan 18, 2022

Yeah, you're right, this might be a BC break - sorry about that!
You can solve it by changing ->addIdHeader() to the generic ->addHeader()

@micheh
Copy link
Contributor

@micheh micheh commented Jan 19, 2022

Yes, you can use the more generic addHeader method. But then you also have to remember to add the brackets to each message id yourself, as they are only added with the IdentificationHeader and not with the UnstructuredHeader.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants