Skip to content

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

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 1 commit into from
Dec 21, 2021

Conversation

ThomasLandauer
Copy link
Contributor

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 commented Dec 20, 2021

would be great to add a test to prevent regressions

@ThomasLandauer
Copy link
Contributor Author

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 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

OK, thanks - like this?

@stof
Copy link
Member

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

Works for me

@stof stof modified the milestones: 6.1, 4.4 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
Copy link
Member

fabpot commented Dec 21, 2021

Thank you @ThomasLandauer.

@fabpot fabpot merged commit 3526a32 into symfony:4.4 Dec 21, 2021
@ThomasLandauer ThomasLandauer deleted the in-reply-to-header branch December 21, 2021 23:11
@micheh
Copy link
Contributor

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

Please open issues if you want further discussion.

@ThomasLandauer
Copy link
Contributor Author

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 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.

fabpot added a commit that referenced this pull request Aug 10, 2022
…o' and 'References' headers (AlbinoDrought)

This PR was merged into the 6.2 branch.

Discussion
----------

[Mime] Re-allow addIdHeader to be used for 'In-Reply-To' and 'References' headers

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | no (arguable: undo BC break)
| New feature?  | yes (arguable: undo BC break)
| Deprecations? | no
| Tickets       | Fix #45097
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

#44732 causes messages using `addIdHeader('In-Reply-To', ...)` or `addIdHeader('References', ...)` to fail the headers check (see #44732 (comment) ), requiring users to manually format the header value themselves.

This PR re-allows the previous behaviour of `addIdHeader` while keeping the new unstructured behaviour in-place as the default.

-----

(I'm not sure if this counts as a new feature or a bug fix since it "reintroduces" a previous feature)

Commits
-------

ffde0f1 [Mime] Re-allow addIdHeader to be used for 'In-Reply-To' and 'References' headers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Mailer] Too strict syntax validation for In-Reply-To and References headers
6 participants