Fix #4012 encoding of content-disposition parameters #4031

kohtala · 2019-08-31T20:13:27Z

Posting form data with field names containing "[]" were not recognized
by other multipart/form-data parsers. The percent-encoding is only to
be used on the filename parameter that follows how of a "file:" URI
may be encoded according to RFC7578.

What do these changes do?

Only encode content-disposition filename parameter using percent-encoding.
Other parameters are encoded to quoted-string or RFC2231 extended parameter
value.

Are there changes in behavior for the user?

I do not know if there are some implementations that depend on the wrong encoding.

Related issue number

Related to #4012.

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new news fragment into the CHANGES folder
- name it <issue_id>.<type> for example (588.bugfix)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

codecov-io · 2019-08-31T20:36:45Z

Codecov Report

Merging #4031 (f26bded) into master (1879aa1) will decrease coverage by 0.01%.
The diff coverage is 93.10%.

@@            Coverage Diff             @@
##           master    #4031      +/-   ##
==========================================
- Coverage   97.54%   97.53%   -0.02%     
==========================================
  Files          43       43              
  Lines        8809     8798      -11     
  Branches     1415     1414       -1     
==========================================
- Hits         8593     8581      -12     
- Misses        101      103       +2     
+ Partials      115      114       -1

Flag	Coverage Δ
unit	`97.41% <93.10%> (+0.04%)`	⬆️

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

Impacted Files	Coverage Δ
aiohttp/payload.py	`95.32% <ø> (ø)`
aiohttp/helpers.py	`96.88% <93.10%> (+0.33%)`	⬆️
aiohttp/cookiejar.py	`98.78% <0.00%> (-0.39%)`	⬇️
aiohttp/connector.py	`96.48% <0.00%> (-0.32%)`	⬇️
aiohttp/web_server.py	`94.28% <0.00%> (-0.16%)`	⬇️
aiohttp/pytest_plugin.py	`97.45% <0.00%> (-0.05%)`	⬇️
aiohttp/web_runner.py	`97.76% <0.00%> (-0.01%)`	⬇️
aiohttp/web.py	`99.14% <0.00%> (-0.01%)`	⬇️
aiohttp/abc.py	`100.00% <0.00%> (ø)`
aiohttp/resolver.py	`93.18% <0.00%> (ø)`
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1879aa1...f26bded. Read the comment docs.

asvetlov · 2019-09-06T08:12:53Z

@kxepal would you review?

webknjaz · 2020-10-16T22:29:36Z

aiohttp/helpers.py

-            qval = quote(val, '') if quote_fields else val
-            lparams.append((key, '"%s"' % qval))
-            if key == 'filename':
-                lparams.append(('filename*', "utf-8''" + qval))


I'm not sure this is correct. filename* must be added if filename is not ASCII-only per RFC 6266, appendix D. The logic is supposed to be roughly https://github.com/cherrypy/cherrypy/pull/1851/files#diff-9d544371f1a0e2079e68ac3cca04bc6cd1ccaf92797f378b285f307871b8d6f0R32-R53. Note that NFKC is supposed to be used for normalization.

Looking closer, it seems you may be right about the newer RFC...

Hi. As this is the payload and not http Content-Disposition header, filename* can not be used. I believe your link was about the http header.

Ah, then I misread what you're doing here.

tests/test_client_functional.py

kohtala · 2020-11-23T14:08:44Z

I rebased the commit on top of current master. I do not see the macos failure related to this PR.

What is the CLA message? The link says

Error
There is no CLA to sign for aio-libs/aiohttp
(could not find linked item for owner aio-libs and repo aiohttp)

asvetlov · 2020-11-23T14:33:07Z

Please ignore the CLA, I've switched it off back -- and forever.

asvetlov

LGTM.
Thank you!

I'd like to get an additional review from aiohttp team, otherwise I'll merge this PR in a week.
It will be a part of aiohttp 3.8

tests/test_helpers.py

Posting form data with field names containing "[]" were not recognized by other multipart/form-data parsers. The percent-encoding is only to be used on the filename parameter that follows how of a "file:" URI may be encoded according to RFC7578.

asvetlov · 2020-11-26T14:52:05Z

Thanks!

aio-libs-github-bot · 2020-11-26T14:52:24Z

💔 Backport was not successful

The PR was attempted backported to the following branches:

❌ 3.8: Commit could not be cherrypicked due to conflicts

…s#4031) Posting form data with field names containing "[]" were not recognized by other multipart/form-data parsers. The percent-encoding is only to be used on the filename parameter that follows how of a "file:" URI may be encoded according to RFC7578.

Posting form data with field names containing "[]" were not recognized by other multipart/form-data parsers. The percent-encoding is only to be used on the filename parameter that follows how of a "file:" URI may be encoded according to RFC7578.

patchback · 2021-10-31T18:36:30Z

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

❌ Failed to cleanly apply aa11d78 on top of patchback/backports/3.8/aa11d78356e243be985f1d2a30f733d95417cf08/pr-4031

Backporting merged PR #4031 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.8/aa11d78356e243be985f1d2a30f733d95417cf08/pr-4031 upstream/3.8

Now, cherry-pick PR Fix #4012 encoding of content-disposition parameters #4031 contents into that branch:
```
$ git cherry-pick -x aa11d78356e243be985f1d2a30f733d95417cf08
```
If it'll yell at you with something like fatal: Commit aa11d78356e243be985f1d2a30f733d95417cf08 is a merge but no -m option was given., add -m 1 as follows intead:
```
$ git cherry-pick -m1 -x aa11d78356e243be985f1d2a30f733d95417cf08
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR Fix #4012 encoding of content-disposition parameters #4031 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.8/aa11d78356e243be985f1d2a30f733d95417cf08/pr-4031

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.

aiohttp now percent-encodes filenames, causing a test to fail. aio-libs/aiohttp#4031

kohtala requested review from asvetlov and webknjaz as code owners August 31, 2019 20:13

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Aug 31, 2019

kohtala force-pushed the fix/4012 branch from 1a72de7 to bf3480f Compare September 2, 2019 09:49

kohtala force-pushed the fix/4012 branch from bf3480f to 1b5aa25 Compare October 10, 2019 11:39

kohtala mentioned this pull request Oct 10, 2019

How to post file that filename include chinese name #4075

Open

kohtala force-pushed the fix/4012 branch from 1b5aa25 to 954fa1f Compare November 20, 2019 09:14

kohtala force-pushed the fix/4012 branch 2 times, most recently from f886860 to 781b507 Compare October 16, 2020 08:22

webknjaz reviewed Oct 16, 2020

View reviewed changes

tests/test_client_functional.py Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

kohtala force-pushed the fix/4012 branch from 781b507 to 93c86ba Compare November 23, 2020 13:53

asvetlov approved these changes Nov 23, 2020

View reviewed changes

asvetlov added the backport-3.8 label Nov 23, 2020

webknjaz reviewed Nov 23, 2020

View reviewed changes

tests/test_helpers.py Outdated Show resolved Hide resolved

kohtala force-pushed the fix/4012 branch from 93c86ba to f26bded Compare November 24, 2020 08:39

asvetlov approved these changes Nov 26, 2020

View reviewed changes

asvetlov merged commit aa11d78 into aio-libs:master Nov 26, 2020

tusharsnx mentioned this pull request Jun 24, 2021

Issue #4012 Updated the docs to reflect need of decoding filename #5831

Closed

5 tasks

webknjaz added backport-3.8 and removed backport-3.8 labels Oct 31, 2021

remram44 added a commit to remram44/taguette that referenced this pull request Nov 3, 2021

tests: Update filename sanitation for aiohttp 3.8

e7dc0d4

aiohttp now percent-encodes filenames, causing a test to fail. aio-libs/aiohttp#4031

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4012 encoding of content-disposition parameters #4031

Fix #4012 encoding of content-disposition parameters #4031

kohtala commented Aug 31, 2019 •

edited

codecov-io commented Aug 31, 2019 •

edited by codecov bot

asvetlov commented Sep 6, 2019

webknjaz Oct 16, 2020

webknjaz Oct 16, 2020

kohtala Oct 19, 2020

webknjaz Oct 19, 2020

This comment has been minimized.

kohtala commented Nov 23, 2020

asvetlov commented Nov 23, 2020

asvetlov left a comment

asvetlov commented Nov 26, 2020

aio-libs-github-bot bot commented Nov 26, 2020

patchback bot commented Oct 31, 2021 •

edited

Fix #4012 encoding of content-disposition parameters #4031

Fix #4012 encoding of content-disposition parameters #4031

Conversation

kohtala commented Aug 31, 2019 • edited

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

codecov-io commented Aug 31, 2019 • edited by codecov bot

Codecov Report

asvetlov commented Sep 6, 2019

webknjaz Oct 16, 2020

Choose a reason for hiding this comment

webknjaz Oct 16, 2020

Choose a reason for hiding this comment

kohtala Oct 19, 2020

Choose a reason for hiding this comment

webknjaz Oct 19, 2020

Choose a reason for hiding this comment

This comment has been minimized.

kohtala commented Nov 23, 2020

asvetlov commented Nov 23, 2020

asvetlov left a comment

Choose a reason for hiding this comment

asvetlov commented Nov 26, 2020

aio-libs-github-bot bot commented Nov 26, 2020

💔 Backport was not successful

patchback bot commented Oct 31, 2021 • edited

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

Backporting merged PR #4031 into master

kohtala commented Aug 31, 2019 •

edited

codecov-io commented Aug 31, 2019 •

edited by codecov bot

patchback bot commented Oct 31, 2021 •

edited