Remove copy of body headers into request headers

[JohanLorenzo]

Thank you for this library! Mozilla uses aiohttp to upload artifacts on Amazon S3.

We recently migrated to 2.0.0, which gave this particular side-effect: mozilla-releng/scriptworker#98. We noticed 'Content-Disposition' is now added to PUT request that contain a data payload (like a text/plain document). This has the unwanted effect to confuse Firefox when downloading artifacts from S3. Before then, artifacts could be directly displayed in browser, but now they are considered as blobs to be downloaded.

I found headers are now extended by the ones detected from the body. in d69838a. I'm not sure what was the context at the time.

What do these changes do?

With this patch, body headers are not included in the actual headers of a request. Based on d69838a, this was the behavior before this commit.

What do you think @fafhrd91 ?

Are there changes in behavior for the user?

That fixes a regression from 1.x.

Related issue number

• uploaded tc logs appear to no longer be text/plain mozilla-releng/scriptworker#98

Checklist

• I think the code is well written
  (Side note: I just deleted some lines 😃 )

• Unit tests for the changes exist
  It seems this particular area wasn't tested. I haven't changed the tests.

• Documentation reflects the changes
  I'm not sure if changes are needed in the docs.

• If you provide code modification, please add yourself to CONTRIBUTORS.txt

• Add a new entry to CHANGES.rst

[fafhrd91]

I do not think this is good solution. I think default expectation is to have content-disposition header if you send file. What if we change attachment to inline type? That should solve your problem

[JohanLorenzo]

Thank you for your rapid answer 😃

I'm not sure to understand the necessity of Content-Disposition, though. Could you help me out, here?

From what I read on the Amazon S3 docs and the W3C specs, this is meant for clients to present the data differently (like changing a file name). Because this is not part of the HTTP specs, this is a nice to have, in my opinion.

Hence, forcing this header as a default on POST/PUT requests doesn't seem natural to me. Moreover, this might also represent a security risk. For example, one may not want to share the actual file name stored on a local disk. I also don't see a way filter out this header (with skip_auto_header for instance) before the request is sent.

[fafhrd91]

aiohttp adds this header only if you send file object. I'll replace attachment with inline for 2.0.4 release
and think about removing header in 2.1.0. does it sound ok for you?

[JohanLorenzo]

Sounds good to me. Let's go with this plan!

[fafhrd91]

@kxepal what do you think about inline by default?

@JohanLorenzo another option is to drop content-disposition for non-multipart requests

[JohanLorenzo]

Dropping it for non-multiparts makes sense to me.

[kxepal]

+1 to drop. This header is not what to add for the requests. It's the server response header.

[fafhrd91]

ok, then I'll leave attachment as default, and will drop content-disposition from stream

[fafhrd91]

fixed in 2.0 and master. will release new version later today

[fafhrd91]

2.0.4 is released

[JohanLorenzo]

Thank you :)

[lock]

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.