Fix Invalid header line for Content-Disposition string - incomplete continuation #108

glensc · 2020-08-19T09:55:25Z

Q	A
Bugfix	yes
Critical	yes

Description

https://tools.ietf.org/html/rfc2231 allows the sequence (internally $count) being missing:

        Content-Type: application/x-stuff;
         title*=us-ascii'en-us'This%20is%20%2A%2A%2Afun%2A%2A%2A

However ContentDisposition expected it to be number, and failed to cast empty string to numeric index. This resulted in throwing InvalidArgumentException(code: 0): Invalid header line for Content-Disposition string - incomplete continuation

Additionally, seems similar logic is needed ion ContentType class, but it's partly duplicated, ContentDisposition has more logic, and the exception is presented only in ContentDisposition class.

glensc · 2020-08-20T08:57:16Z

WDYT of having @codingStandardsIgnoreStart equivalent enabled for all tests? or set the line lenght limit to 512 or something biggger than 120, that 120 quota is very easy to reach:

➔ grep -r @codingStandardsIgnoreStart test/|wc -l
19

glensc · 2020-08-20T08:59:51Z

I don't know how to fight coverage, it says coverage increased, but still making status check failed

https://coveralls.io/builds/32879718

COVERAGE INCREASED (+0.09%) TO 62.259%

Signed-off-by: Elan Ruusamäe <glen@pld-linux.org>

glensc · 2020-08-21T08:13:26Z

@weierophinney how to indicate it's ready for merge from the developer? I removed draft status, but seems can't request a review as that probably requires being project member.

froschdesign · 2020-08-21T08:47:36Z

@glensc

but seems can't request a review as that probably requires being project member.

Which is correct:

Note: Pull request authors can't request reviews unless they are either a repository owner or collaborator with write access to the repository.

https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/requesting-a-pull-request-review

(But there is a bug here on GitHub so sometimes you can add a reviewer without special permissions.)

test/Header/ContentDispositionTest.php

Signed-off-by: Elan Ruusamäe <glen@pld-linux.org>

glensc · 2020-08-21T10:08:22Z

Updated, DCO & Travis pass.

glensc · 2020-08-24T09:30:36Z

ping!

glensc · 2020-08-31T10:55:05Z

another ping!

I'd like this to be in the next patch release. it's critical for us, and keeping production patched (hot fixed) is not very sustainable solution.

and maintaining a fork again its something I'd really like to avoid!

https://github.com/eventum/zend-mail

glensc · 2020-09-17T13:32:00Z

@weierophinney @froschdesign @samsonasik ping

https://outlook.office.com/mail/inbox seems to create such mails, so it's pretty common and wild.

glensc · 2020-09-18T07:06:47Z

and apparently Apple Mail as well: #111

glensc · 2020-10-04T19:28:34Z

@boesing seeing you active in this project (#113), can you do something to get this PR (#108) moving? waiting for the fix to be released for almost 50 days is depressive...

test/Header/ContentDispositionTest.php

Chunk from PR#111 by Cédric Anne <cedric.anne@gmail.com> Signed-off-by: Elan Ruusamäe <glen@pld-linux.org>

Signed-off-by: Elan Ruusamäe <glen@pld-linux.org>

boesing · 2020-10-04T21:15:12Z

@glensc I've created issues in every package today. ;-)

TBH, I am not that familiar with all that mail standards, e.g.
Furthermore, we voted to mark this package as security-only as (you can also see) we do not focus on this package anymore.

Laminas has a huge amount of packages (including all mezzio and laminas-api-tools) while we are just a bunch of people to maintain all these packages (most of us are volunteering tho).

I'd like to have another one have a look on this and then we probably get to merge this. Thanks for your patience and sorry that it might take some time to get things merged (as we are focussing on other packages currently).

boesing · 2020-10-04T21:19:23Z

From my perspective, this PR is fine. However, as I stated that I don't know much about all this stuff, I try to get someone to review this aswell.

cedric-anne

I had the same problem and applying this fix solves it.

glensc · 2020-11-13T16:34:25Z

Can this be merged and released, please?

Ocramius

LGTM 🚢

Sorry that it went under the radar 😱

Ocramius · 2020-12-31T11:40:57Z

Thanks @glensc - going out for release right now

glensc · 2020-12-31T11:41:55Z

It took a while, but at least still fixed in the same year :)

…isposition string - laminas/laminas-mail#108 - https://github.com/laminas/laminas-mail/releases/tag/2.12.5

glensc · 2021-03-21T11:52:08Z

src/Header/ContentDisposition.php

+                        throw new Exception\InvalidArgumentException(sprintf(
+                            "Invalid header line for Content-Disposition string".
+                            " - count expected to be numeric, got %s with value %s",
+                            $type,


I think the $type is always string|null as explode and list can't return anything else.

Fix Invalid header line for Content-Disposition string - incomplete continuation

glensc marked this pull request as draft August 19, 2020 10:09

glensc force-pushed the header-wrapping branch 5 times, most recently from 9e1c00d to a5b5d62 Compare August 20, 2020 08:55

glensc marked this pull request as ready for review August 20, 2020 08:57

Add default 0 for empty count in ContentDisposition

f3e5dd9

Signed-off-by: Elan Ruusamäe <glen@pld-linux.org>

glensc force-pushed the header-wrapping branch from a5b5d62 to 7cbe513 Compare August 20, 2020 09:07

samsonasik reviewed Aug 21, 2020

View reviewed changes

test/Header/ContentDispositionTest.php Outdated Show resolved Hide resolved

glensc added 2 commits August 21, 2020 12:08

Add test for Header Parameter Value Continuation parsing

d4042fe

Signed-off-by: Elan Ruusamäe <glen@pld-linux.org>

Add test for ContentType parameter wrapping from rfc2231

bec7864

Signed-off-by: Elan Ruusamäe <glen@pld-linux.org>

glensc force-pushed the header-wrapping branch from 7cbe513 to a55ec3d Compare August 21, 2020 09:09

glensc added 3 commits August 21, 2020 12:23

Split tests to appropriate test classes

fb26543

Signed-off-by: Elan Ruusamäe <glen@pld-linux.org>

Add test for two segments

ccad5c2

Signed-off-by: Elan Ruusamäe <glen@pld-linux.org>

Add testParameterWrappingExceptions test to increase coverage

ee6b687

Signed-off-by: Elan Ruusamäe <glen@pld-linux.org>

glensc force-pushed the header-wrapping branch from a55ec3d to ee6b687 Compare August 21, 2020 09:23

glensc mentioned this pull request Sep 18, 2020

Handle RFC5987 format in headers #111

Closed

glensc changed the title ~~Fix InvalidArgumentException for optional empty count for rfc2231 parameter wrapping~~ Fix Invalid header line for Content-Disposition string - incomplete continuation Sep 18, 2020

glensc closed this Oct 4, 2020

glensc reopened this Oct 4, 2020

boesing requested changes Oct 4, 2020

View reviewed changes

test/Header/ContentDispositionTest.php Show resolved Hide resolved

glensc added 2 commits October 5, 2020 00:13

Add another unit test from different PR

44988d0

Chunk from PR#111 by Cédric Anne <cedric.anne@gmail.com> Signed-off-by: Elan Ruusamäe <glen@pld-linux.org>

Add @codingStandardsIgnoreStart to long tests

cafc1ac

Signed-off-by: Elan Ruusamäe <glen@pld-linux.org>

glensc force-pushed the header-wrapping branch from 42f2d25 to cafc1ac Compare October 4, 2020 21:13

boesing approved these changes Oct 4, 2020

View reviewed changes

This was referenced Oct 22, 2020

Handle RFC5987 format in Content-Disposition header; fixes #8148 glpi-project/glpi#8161

Merged

Attached mail files are not saved glpi-project/glpi#8148

Closed

cedric-anne approved these changes Oct 26, 2020

View reviewed changes

cedric-anne mentioned this pull request Nov 13, 2020

Handle RFC5987 format in Content-Disposition header (single line) glpi-project/glpi#8265

Merged

glensc mentioned this pull request Dec 10, 2020

Add option novalidatecert to connect() #5

Open

glensc mentioned this pull request Dec 31, 2020

Fix issue #121: Correct return value #123

Merged

Ocramius approved these changes Dec 31, 2020

View reviewed changes

Ocramius added this to the 2.12.5 milestone Dec 31, 2020

Ocramius added the Bug Something isn't working label Dec 31, 2020

Ocramius self-assigned this Dec 31, 2020

Ocramius merged commit fdea3c4 into laminas:2.12.x Dec 31, 2020

glensc deleted the header-wrapping branch December 31, 2020 11:41

github-actions bot mentioned this pull request Dec 31, 2020

Merge release 2.12.5 into 2.13.x #125

Merged

glensc added a commit to glensc/eventum that referenced this pull request Dec 31, 2020

Update laminas-mail dependency: Fix Invalid header line for Content-D…

e406393

…isposition string - laminas/laminas-mail#108 - https://github.com/laminas/laminas-mail/releases/tag/2.12.5

glensc mentioned this pull request Dec 31, 2020

Update laminas-mail dependency: Fix Invalid header line for Content-Disposition string eventum/eventum#980

Merged

glensc added a commit to glensc/eventum that referenced this pull request Dec 31, 2020

Update laminas-mail dependency: Fix Invalid header line for Content-D…

5d07c69

…isposition string - laminas/laminas-mail#108 - https://github.com/laminas/laminas-mail/releases/tag/2.12.5

glensc added a commit to glensc/eventum that referenced this pull request Dec 31, 2020

Update laminas-mail dependency: Fix Invalid header line for Content-D…

1c6f195

…isposition string - laminas/laminas-mail#108 - https://github.com/laminas/laminas-mail/releases/tag/2.12.5

glensc commented Mar 21, 2021

View reviewed changes

artemii-karkusha pushed a commit to artemii-karkusha/laminas-mail that referenced this pull request May 24, 2023

Merge pull request laminas#108 from glensc/header-wrapping

f8adacf

Fix Invalid header line for Content-Disposition string - incomplete continuation

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Invalid header line for Content-Disposition string - incomplete continuation #108

Fix Invalid header line for Content-Disposition string - incomplete continuation #108

glensc commented Aug 19, 2020 •

edited

Loading

glensc commented Aug 20, 2020

glensc commented Aug 20, 2020

glensc commented Aug 21, 2020

froschdesign commented Aug 21, 2020

glensc commented Aug 21, 2020

glensc commented Aug 24, 2020

glensc commented Aug 31, 2020 •

edited

Loading

glensc commented Sep 17, 2020

glensc commented Sep 18, 2020

glensc commented Oct 4, 2020

boesing commented Oct 4, 2020

boesing commented Oct 4, 2020

cedric-anne left a comment

glensc commented Nov 13, 2020

Ocramius left a comment

Ocramius commented Dec 31, 2020

glensc commented Dec 31, 2020

glensc Mar 21, 2021

Fix Invalid header line for Content-Disposition string - incomplete continuation #108

Fix Invalid header line for Content-Disposition string - incomplete continuation #108

Conversation

glensc commented Aug 19, 2020 • edited Loading

Description

glensc commented Aug 20, 2020

glensc commented Aug 20, 2020

glensc commented Aug 21, 2020

froschdesign commented Aug 21, 2020

glensc commented Aug 21, 2020

glensc commented Aug 24, 2020

glensc commented Aug 31, 2020 • edited Loading

glensc commented Sep 17, 2020

glensc commented Sep 18, 2020

glensc commented Oct 4, 2020

boesing commented Oct 4, 2020

boesing commented Oct 4, 2020

cedric-anne left a comment

Choose a reason for hiding this comment

glensc commented Nov 13, 2020

Ocramius left a comment

Choose a reason for hiding this comment

Ocramius commented Dec 31, 2020

glensc commented Dec 31, 2020

glensc Mar 21, 2021

Choose a reason for hiding this comment

glensc commented Aug 19, 2020 •

edited

Loading

glensc commented Aug 31, 2020 •

edited

Loading