Issue 3098: Provide plain diff download URL
Status:  Accepted
Owner: ----
Reported by maddy210...@gmail.com, Jan 11, 2015
************************************************************
***** NOTE: THIS BUG TRACKER IS FOR GERRIT CODE REVIEW *****
***** DO NOT SUBMIT BUGS FOR CHROME, ANDROID, INTERNAL *****
***** ISSUES WITH YOUR COMPANY'S GERRIT SETUP, ETC.    *****
***** THOSE ISSUE BELONG IN DIFFERENT ISSUE TRACKERS!  *****
************************************************************

Affected Version: -

What steps will reproduce the problem?
1. open any review, activate the Download Drop Down
2. don't find a possibility to plain http download the diff file
3. it is inside the archived versions, but this don't help

What is the expected output? What do you see instead?
I want a possibility to not download a zip file containing the diff file, I want the diff file.

Please provide any additional information below.
-
Jan 11, 2015
Project Member #1 edwin.ke...@gmail.com
> 3. it is inside the archived versions, but this don't help

What's the problem with unzipping?

AFAIK it's zipped for security reasons, otherwise some browsers may try to inspect and excute the file content which opens the door for XSS attacks.
Jan 11, 2015
#2 maddy210...@gmail.com
set the mime type to text/plain and all browsers can deal with it. The zipped version is no more secure, but I can not use it in my workflow, because it expects a plain http download url of the diff file.
Jan 11, 2015
#3 ty...@helmuthummel.de
> AFAIK it's zipped for security reasons, otherwise some browsers may try to inspect and excute the file content which opens the door for XSS attacks


While I would accept any security related reasoning, I fail to understand this one. As maddy points out, just sending a text/plain mime type will force *any* browser to *not* parse the content as HTML, making XSS attacks impossible.

Besides that, even the simplest git code browsers like gitweb have this functionality, as well as the bigger players like GitHub.


Sure it is possible to just unpack the zipped patch, but why put this burden on the user if there is no technical reason for it. 

Related to this. The UI provides the possibility to download the patch as base64 encoded. What is the use case for such a thing anyway?
Jan 11, 2015
Project Member #4 edwin.ke...@gmail.com
I guess the reason is similar to why the file download in the Side-by-Side diff is also offered as zip only, Shawn provided a detailed explanation for this at [1] (not completely fitting to this case, but it goes into the same direction). As I understand it, the problem is that IE may ignore the Content-Type header.

[1] https://groups.google.com/forum/#!searchin/repo-discuss/zip$20Internet$20explorer/repo-discuss/h7Bvgns5cyY/0-R39qin51UJ
Jan 25, 2015
Project Member #5 David.Os...@gmail.com
Is IE still used in the wild? If not, why do we care? Moreover, if folks is using IE, they have more serious porblems, than this one, anyway.

In any events, there could be a configuration option that disabled per default to offer plain diff file for downloading.
Status: Accepted
Jan 26, 2015
Project Member #6 edwin.ke...@gmail.com
Agreed, a configuration option would make sense.
Jan 26, 2015
#7 ty...@helmuthummel.de
Thanks for accepting this request!

Actually, the security concerns regarding mime type sniffing are valid. The bad thing also is that not only IE but also Chrome has this behavior.

However, there is also a mitigation implemented in both browsers (starting from IE8 IIRC) which is sending the X-Content-Type-Options: nosniff HTTP header which disable mime type sniffing in both browsers.

http://stackoverflow.com/questions/18337630/what-is-x-content-type-options-nosniff
https://www.owasp.org/index.php/List_of_useful_HTTP_headers


With these headers sent I think it is fine to send a plaintext diff without a zip or base64 "encoding" and that might be the reason why platforms like github.com just do that. (e.g.: https://github.com/helhum/typo3_console/commit/1c8f284cabe2d2a06b8c1465b55de22bf5fa7c25.diff)

That said, you may want to reconsider making an option and just send the diff plain text, or at least making that a default. In any case, send the aforementioned header, when doing so.

Thanks again!
Jan 26, 2015
Project Member #8 edwin.ke...@gmail.com
Thanks for providing this additional background info. If setting X-Content-Type-Options to nosniff solves the problem we probably don't need a configuration option for this, but we can just always send the diff plain text.