Fix handling of multipart/form-data #8280

That was my initial thought too. It might be a convenience if you avoid bytes for typical use, but a little confusing otherwise. Should we switch v4 to only create files when filename is passed or similar? We could use a sentinel so that filename=None would still create a file?

CHANGES/8280.breaking.rst

CHANGES/8280.bugfix.rst

aiohttp/multipart.py

codecov · 2024-04-05T16:34:48Z

Codecov Report

Attention: Patch coverage is 93.05556% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 97.54%. Comparing base (cbc0c86) to head (c02cab6).

Files	Patch %	Lines
aiohttp/multipart.py	91.11%	2 Missing and 2 partials ⚠️
aiohttp/formdata.py	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8280      +/-   ##
==========================================
+ Coverage   97.50%   97.54%   +0.04%     
==========================================
  Files         107      107              
  Lines       33026    33024       -2     
  Branches     3862     3872      +10     
==========================================
+ Hits        32201    32213      +12     
+ Misses        605      591      -14     
  Partials      220      220

Flag	Coverage Δ
CI-GHA	`97.45% <93.05%> (+0.03%)`	⬆️
OS-Linux	`97.12% <93.05%> (+0.03%)`	⬆️
OS-Windows	`95.61% <93.05%> (-0.03%)`	⬇️
OS-macOS	`96.83% <93.05%> (-0.03%)`	⬇️
Py-3.10.11	`95.46% <93.05%> (-0.03%)`	⬇️
Py-3.10.14	`96.92% <93.05%> (-0.03%)`	⬇️
Py-3.11.8	`97.12% <93.05%> (-0.03%)`	⬇️
Py-3.12.2	`97.24% <93.05%> (-0.03%)`	⬇️
Py-3.8.10	`95.39% <93.05%> (-0.03%)`	⬇️
Py-3.8.18	`96.77% <93.05%> (-0.03%)`	⬇️
Py-3.9.13	`95.43% <93.05%> (-0.03%)`	⬇️
Py-3.9.19	`96.90% <93.05%> (-0.02%)`	⬇️
Py-pypy7.3.15	`96.43% <93.05%> (?)`
VM-macos	`96.83% <93.05%> (-0.03%)`	⬇️
VM-ubuntu	`97.12% <93.05%> (+0.03%)`	⬆️
VM-windows	`95.61% <93.05%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

for more information, see https://pre-commit.ci

Dreamsorcerer · 2024-04-05T18:23:07Z

OK, I think this should be fine now. Will need a followup PR to do the actual removal in master.

bdraco · 2024-04-05T18:44:50Z

I'll be back home in an hour and can throw this on production to make sure there's no unexpected side effects

bdraco · 2024-04-05T19:15:29Z

The backport to 3.9 is messy because #3492 isn't in 3.9

bdraco · 2024-04-05T19:30:26Z

I squashed this, cherry-picked fcedc66, than cherry-picked this on to my 3.9 integration branch.

Testing that now

bdraco · 2024-04-05T19:35:32Z

Started up OK, and file uploads work. Watching the logs now.

bdraco · 2024-04-05T20:26:12Z

Nothing obvious showing up in the logs. Everything I could think to test still works

Dreamsorcerer · 2024-04-05T20:32:11Z

The backport to 3.9 is messy because #3492 isn't in 3.9

Guess we just backport that first then? Clearly got missed from any backports.

bdraco · 2024-04-05T20:46:24Z

The backport to 3.9 is messy because #3492 isn't in 3.9

Guess we just backport that first then? Clearly got missed from any backports.

While I'm not a big fan of the complexity added by #3492, given that PR is already accepted and merged, and this fix is developed on top of it, that seems like the most reasonable path forward.

Dreamsorcerer · 2024-04-05T21:13:06Z

The backport to 3.9 is messy because #3492 isn't in 3.9

Guess we just backport that first then? Clearly got missed from any backports.

While I'm not a big fan of the complexity added by #3492, given that PR is already accepted and merged, and this fix is developed on top of it, that seems like the most reasonable path forward.

Wait, is this multipart reader server-side or client-side? If it's server-side then the change will likely introduce another HTTP request smuggling vulnerability. If it's only server-side, then we'd probably be best reverting the commit. If it's both, then we probably want to backport it and add a followup PR to disable it when the parser is not in lax mode...

Dreamsorcerer · 2024-04-06T12:22:01Z

The backport to 3.9 is messy because #3492 isn't in 3.9

Guess we just backport that first then? Clearly got missed from any backports.

While I'm not a big fan of the complexity added by #3492, given that PR is already accepted and merged, and this fix is developed on top of it, that seems like the most reasonable path forward.

Wait, is this multipart reader server-side or client-side? If it's server-side then the change will likely introduce another HTTP request smuggling vulnerability. If it's only server-side, then we'd probably be best reverting the commit. If it's both, then we probably want to backport it and add a followup PR to disable it when the parser is not in lax mode...

I think this is server-side only, I think the best option right now is to revert that change as nobody seems to have missed the feature, given that it failed to make it into a release for 5 years.

for more information, see https://pre-commit.ci

Dreamsorcerer · 2024-04-06T18:15:10Z

There are a couple of lines of coverage dropped in multipart.py:
https://app.codecov.io/gh/aio-libs/aiohttp/pull/8280/indirect-changes

I'm unclear how to get that covered again, or whether the code should maybe be deleted now. If anyone figures it out by tomorrow, feel free to just edit the PR directly.

patchback · 2024-04-07T11:28:19Z

Backport to 3.9: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 7d0be3f on top of patchback/backports/3.9/7d0be3fee540a3d4161ac7dc76422f1f5ea60104/pr-8280

Backporting merged PR #8280 into master

Ensure you have a local repo clone of your fork. Unless you cloned it
from the upstream, this would be your origin remote.
Make sure you have an upstream repo added as a remote too. In these
instructions you'll refer to it by the name upstream. If you don't
have it, here's how you can add it:
```
$ git remote add upstream https://github.com/aio-libs/aiohttp.git
```

Ensure you have the latest copy of upstream and prepare a branch
that will hold the backported code:

$ git fetch upstream
$ git checkout -b patchback/backports/3.9/7d0be3fee540a3d4161ac7dc76422f1f5ea60104/pr-8280 upstream/3.9

Now, cherry-pick PR Fix handling of multipart/form-data #8280 contents into that branch:
```
$ git cherry-pick -x 7d0be3fee540a3d4161ac7dc76422f1f5ea60104
```
If it'll yell at you with something like fatal: Commit 7d0be3fee540a3d4161ac7dc76422f1f5ea60104 is a merge but no -m option was given., add -m 1 as follows instead:
```
$ git cherry-pick -m1 -x 7d0be3fee540a3d4161ac7dc76422f1f5ea60104
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR Fix handling of multipart/form-data #8280 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.9/7d0be3fee540a3d4161ac7dc76422f1f5ea60104/pr-8280

Create a PR, ensure that the CI is green. If it's not — update it so that
the tests and any other checks pass. This is it!
Now relax and wait for the maintainers to process your pull request
when they have some cycles to do reviews. Don't worry — they'll tell you if
any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback · 2024-04-07T11:28:25Z

Backport to 3.10: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 7d0be3f on top of patchback/backports/3.10/7d0be3fee540a3d4161ac7dc76422f1f5ea60104/pr-8280

Backporting merged PR #8280 into master

Ensure you have a local repo clone of your fork. Unless you cloned it
from the upstream, this would be your origin remote.
Make sure you have an upstream repo added as a remote too. In these
instructions you'll refer to it by the name upstream. If you don't
have it, here's how you can add it:
```
$ git remote add upstream https://github.com/aio-libs/aiohttp.git
```

Ensure you have the latest copy of upstream and prepare a branch
that will hold the backported code:

$ git fetch upstream
$ git checkout -b patchback/backports/3.10/7d0be3fee540a3d4161ac7dc76422f1f5ea60104/pr-8280 upstream/3.10

Now, cherry-pick PR Fix handling of multipart/form-data #8280 contents into that branch:
```
$ git cherry-pick -x 7d0be3fee540a3d4161ac7dc76422f1f5ea60104
```
If it'll yell at you with something like fatal: Commit 7d0be3fee540a3d4161ac7dc76422f1f5ea60104 is a merge but no -m option was given., add -m 1 as follows instead:
```
$ git cherry-pick -m1 -x 7d0be3fee540a3d4161ac7dc76422f1f5ea60104
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR Fix handling of multipart/form-data #8280 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.10/7d0be3fee540a3d4161ac7dc76422f1f5ea60104/pr-8280

Create a PR, ensure that the CI is green. If it's not — update it so that
the tests and any other checks pass. This is it!
Now relax and wait for the maintainers to process your pull request
when they have some cycles to do reviews. Don't worry — they'll tell you if
any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

https://datatracker.ietf.org/doc/html/rfc7578 (cherry picked from commit 7d0be3f)

Fix handling of multipart/form-data

cf9a42c

Dreamsorcerer added backport-3.9 labels Apr 1, 2024

Dreamsorcerer requested a review from asvetlov as a code owner April 1, 2024 13:55

pre-commit-ci bot and others added 2 commits April 1, 2024 13:56

[pre-commit.ci] auto fixes from pre-commit.com hooks

cbea0c6

for more information, see https://pre-commit.ci

Create 8280.bugfix.rst

d9b039b

Dreamsorcerer requested a review from webknjaz as a code owner April 1, 2024 13:56

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Apr 1, 2024

Dreamsorcerer and others added 14 commits April 1, 2024 15:16

Update multipart.py

970fda1

Update multipart.py

1cec2cf

[pre-commit.ci] auto fixes from pre-commit.com hooks

56c0d6e

for more information, see https://pre-commit.ci

Update multipart.py

af851dd

[pre-commit.ci] auto fixes from pre-commit.com hooks

91af09f

for more information, see https://pre-commit.ci

Update multipart.py

1c6fcd7

Update multipart.py

e874e3a

Update multipart.py

d992d10

Update test_multipart.py

502adac

Update test_web_functional.py

37a9414

Update formdata.py

57c71e1

Update test_client_functional.py

1adcd0a

Create 8280.breaking.rst

3751264

Update multipart.py

4d11802

Dreamsorcerer commented Apr 1, 2024

View reviewed changes

tests/test_web_functional.py Outdated Show resolved Hide resolved

webknjaz reviewed Apr 3, 2024

View reviewed changes

CHANGES/8280.breaking.rst Outdated Show resolved Hide resolved

webknjaz reviewed Apr 3, 2024

View reviewed changes

CHANGES/8280.bugfix.rst Outdated Show resolved Hide resolved

webknjaz reviewed Apr 3, 2024

View reviewed changes

aiohttp/multipart.py Outdated Show resolved Hide resolved

webknjaz reviewed Apr 3, 2024

View reviewed changes

aiohttp/multipart.py Show resolved Hide resolved

Dreamsorcerer added 4 commits April 5, 2024 17:11

Update test_client_functional.py

2231e8c

Update formdata.py

846f02c

Update test_web_functional.py

391e725

Update test_web_functional.py

60204e3

Dreamsorcerer and others added 3 commits April 5, 2024 18:41

Add tests for _charset_

39a2d0a

[pre-commit.ci] auto fixes from pre-commit.com hooks

0ef28a7

for more information, see https://pre-commit.ci

Update test_multipart.py

f6c816d

Dreamsorcerer and others added 2 commits April 6, 2024 18:23

Merge branch 'master' into Dreamsorcerer-patch-6

be5b98c

[pre-commit.ci] auto fixes from pre-commit.com hooks

c02cab6

for more information, see https://pre-commit.ci

Dreamsorcerer merged commit 7d0be3f into master Apr 7, 2024
35 of 37 checks passed

Dreamsorcerer deleted the Dreamsorcerer-patch-6 branch April 7, 2024 11:28

Dreamsorcerer added a commit that referenced this pull request Apr 7, 2024

Fix handling of multipart/form-data (#8280)

2fdcf49

https://datatracker.ietf.org/doc/html/rfc7578 (cherry picked from commit 7d0be3f)

Dreamsorcerer added a commit that referenced this pull request Apr 7, 2024

Fix handling of multipart/form-data (#8280)

de19957

https://datatracker.ietf.org/doc/html/rfc7578 (cherry picked from commit 7d0be3f)

Dreamsorcerer added a commit that referenced this pull request Apr 7, 2024

Fix handling of multipart/form-data (#8280) (#8301)

767c413

https://datatracker.ietf.org/doc/html/rfc7578 (cherry picked from commit 7d0be3f)

Dreamsorcerer added a commit that referenced this pull request Apr 7, 2024

Fix handling of multipart/form-data (#8280) (#8302)

cebe526

https://datatracker.ietf.org/doc/html/rfc7578 (cherry picked from commit 7d0be3f)

beagold mentioned this pull request Apr 12, 2024

Fix warning raised in aiohttp 3.9.4 regarding deprecated content_transfer_encoding argument hikari-py/hikari#1881

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix handling of multipart/form-data #8280

Fix handling of multipart/form-data #8280

Dreamsorcerer commented Apr 1, 2024

bdraco commented Apr 2, 2024 •

edited

Loading

bdraco commented Apr 2, 2024

Dreamsorcerer commented Apr 3, 2024 •

edited

Loading

codecov bot commented Apr 5, 2024 •

edited

Loading

Dreamsorcerer commented Apr 5, 2024

bdraco commented Apr 5, 2024

bdraco commented Apr 5, 2024 •

edited

Loading

bdraco commented Apr 5, 2024

bdraco commented Apr 5, 2024

bdraco commented Apr 5, 2024

Dreamsorcerer commented Apr 5, 2024

bdraco commented Apr 5, 2024 •

edited

Loading

Dreamsorcerer commented Apr 5, 2024

Dreamsorcerer commented Apr 6, 2024

Dreamsorcerer commented Apr 6, 2024 •

edited

Loading

patchback bot commented Apr 7, 2024 •

edited

Loading

patchback bot commented Apr 7, 2024 •

edited

Loading

Fix handling of multipart/form-data #8280

Fix handling of multipart/form-data #8280

Conversation

Dreamsorcerer commented Apr 1, 2024

bdraco commented Apr 2, 2024 • edited Loading

bdraco commented Apr 2, 2024

Dreamsorcerer commented Apr 3, 2024 • edited Loading

codecov bot commented Apr 5, 2024 • edited Loading

Codecov Report

Dreamsorcerer commented Apr 5, 2024

bdraco commented Apr 5, 2024

bdraco commented Apr 5, 2024 • edited Loading

bdraco commented Apr 5, 2024

bdraco commented Apr 5, 2024

bdraco commented Apr 5, 2024

Dreamsorcerer commented Apr 5, 2024

bdraco commented Apr 5, 2024 • edited Loading

Dreamsorcerer commented Apr 5, 2024

Dreamsorcerer commented Apr 6, 2024

Dreamsorcerer commented Apr 6, 2024 • edited Loading

patchback bot commented Apr 7, 2024 • edited Loading

Backport to 3.9: 💔 cherry-picking failed — conflicts found

Backporting merged PR #8280 into master

patchback bot commented Apr 7, 2024 • edited Loading

Backport to 3.10: 💔 cherry-picking failed — conflicts found

Backporting merged PR #8280 into master

bdraco commented Apr 2, 2024 •

edited

Loading

Dreamsorcerer commented Apr 3, 2024 •

edited

Loading

codecov bot commented Apr 5, 2024 •

edited

Loading

bdraco commented Apr 5, 2024 •

edited

Loading

bdraco commented Apr 5, 2024 •

edited

Loading

Dreamsorcerer commented Apr 6, 2024 •

edited

Loading

patchback bot commented Apr 7, 2024 •

edited

Loading

patchback bot commented Apr 7, 2024 •

edited

Loading