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

Add optional flag for quoting fields #916

Merged
merged 1 commit into from
Feb 7, 2017
Merged

Conversation

lukasz-madon
Copy link
Contributor

@lukasz-madon lukasz-madon commented Jun 6, 2016

What do these changes do?

Add a flag for quoting fields.

RFC standard requires all fields to be US-ASCII. Additional quote
may provide extra security (RFC-1806 p. 2.3), but does not allow
for interoperability with API that requires ASCII characters outside
quote set e.g. emails[] becomes emails%5B%5D

Are there changes in behavior for the user?

There will be no change for current users (the parameter defaults to True). Users that want to skip quoting can pass the flag to constructor.

form = helpers.FormData(quote_fields=False)

Related issue number

fixes #903

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes

RFC stanard requires all fields to be US-ASCII. Additional `quote`
may provide extra security (RFC-1806 p. 2.3), but does not allow
for interoperability with API that requires ASCII characters outside
`quote` set e.g. emails[] becomes emails%5B%5D

fixes aio-libs#903
@asvetlov
Copy link
Member

Looks good.
@lukasz-madon would you add documentation update also?

@lukasz-madon
Copy link
Contributor Author

Actually, the problem is a bit bigger. Aiohttp doesn't implement specification properly see http://greenbytes.de/tech/webdav/rfc2616.html#rfc.section.19.5.1

Field can be a token (a limited ascii set) OR ascii string inside a quotation mark. Aiohttp only allows tokens. I think this should be a default behaviour, but it may break if users want to update the library.

@jchampio
Copy link
Contributor

@lukasz-madon: You'll want to find the latest MIME RFCs to point to instead. Not only is RFC 2616 obsolete, but I don't think it ever applied to multipart payloads to begin with.

https://tools.ietf.org/html/rfc7578#section-4.2, https://tools.ietf.org/html/rfc2183, and https://tools.ietf.org/html/rfc2231 might be good starting points? I am not a MIME expert...

@lukasz-madon
Copy link
Contributor Author

@jchampio not a MIME expert, but looking at RFC and other implementations filename should allow any US-ASCII inside "" https://github.com/pallets/werkzeug/blob/master/tests/test_http.py#L250 https://github.com/pallets/werkzeug/blob/master/werkzeug/http.py#L180

@kxepal
Copy link
Member

kxepal commented Sep 16, 2016

@lukasz-madon This case is invalid according test suite we used to implement Content-Disposition parser. Though major part of browsers allows that, the spec says the other.

@fafhrd91
Copy link
Member

@kxepal thoughts?

@kxepal
Copy link
Member

kxepal commented Jan 26, 2017

Hm...I was wrong in previous message about it's wrong - I miss the quotes in test.

@fafhrd91 need a bit time to make sure that's correct, but so far LGFM.

@fafhrd91
Copy link
Member

@kxepal merge whenever ready

@kxepal
Copy link
Member

kxepal commented Jan 26, 2017

@fafhrd91 ok

@fafhrd91
Copy link
Member

fafhrd91 commented Feb 6, 2017

@kxepal i need you to make a decision

@kxepal
Copy link
Member

kxepal commented Feb 7, 2017

@fafhrd91 Sorry, I forgot about.

@kxepal kxepal merged commit 298029d into aio-libs:master Feb 7, 2017
@fafhrd91
Copy link
Member

fafhrd91 commented Feb 7, 2017

Thanks!

owobot-contrib pushed a commit to whats-this/owo.py that referenced this pull request Feb 27, 2019
The reason for the `aiohttp2` file existing is now redundant (aio-libs/aiohttp#916 got merged), so migrated to use regular aiohttp `MultipartWriter`.
@lock
Copy link

lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to pass array to form data (multipart/form-data)
5 participants