Issue 132: binaries don't diff, so provide links to download before and after versions
Status:  Released
Owner:
Closed:  Oct 2012
Reported by code-rev...@gtempaccount.com, Sep 24, 2009
Reported by JT Olds <jtolds@xnet5.com> on Fri Apr 10 14:04:13 PDT 2009
Source: JIRA GERRIT-132
Affected Version: 2.0.9

It would be cool if there was a way to download a changed binary file from the
web interface.
Sep 24, 2009
#1 code-rev...@gtempaccount.com
Comment by Shawn Pearce <sop@google.com> on Fri Apr 10 14:06:10 PDT 2009

Why can't you use one of the download commands to download the change to a
local repository and then view the files there?
Sep 24, 2009
#2 code-rev...@gtempaccount.com
Comment by JT Olds <jtolds@xnet5.com> on Fri Apr 10 15:41:28 PDT 2009

oh you can, it's just we had a commit that had a word doc in it (gross, i
know), and it would be nice if you could add non-developers to review changes
to stuff like that. we had a few people added to review the just the doc, but
they were frustrated they couldn't just click a link.
Sep 24, 2009
#3 code-rev...@gtempaccount.com
Comment by Shawn Pearce <sop@google.com> on Tue Apr 14 16:32:56 PDT 2009

Doing this securely with MSIE is a pain.

One would assume that serving files for download (setting Content-Disposition:
attachment; filename="evil.html" on the response) should not result in a XSS
risk, since the file is first downloaded to local disk on the user's system,
and then potentially opened just like any other local document (i.e., no
longer associated with the web site it was downloaded from).

Unfortunately, IE (as of version IE 6.0) violates this assumption (this could
be considered a bug): When IE is served a document with Content-Disposition:
attachment, it presents the user with a dialog box that permits the user to
either "Save" the document to disk, or "Open" it straight away.

If the document is of Content-Type text/html (and possibly others) and the
user selects "Open", IE opens the document in a new browser window and
associates with it the domain of the URL the document was loaded from. That
is, JavaScript included in the document runs in the context of the domain of
our application and can trivially mount XSS attacks.

The usual work around is to wrap the content into a zip file, forcing the user
to download and unpack the zip file first.
Sep 24, 2009
#4 code-rev...@gtempaccount.com
Comment by JT Olds <jtolds@xnet5.com> on Tue Apr 14 17:05:12 PDT 2009

oh good catch.

perhaps this could be a configurable option? We're running an internal gerrit
instance, so we have less to worry about in that regard (most of the users
with commit access have root on the gerrit box anyway).
Sep 24, 2009
#5 code-rev...@gtempaccount.com
Comment by Shawn Pearce <sop@google.com> on Tue Apr 14 18:49:29 PDT 2009

Partially fixed by https://review.source.android.com/9533

9533 forces everything into a ZIP file archive.  I'm thinking about allowing
the site admin to opt-out of ZIP archives for specific file patterns, once we
have scripting language support.  It may be trivial to create a two line
script to permit *.doc and *.xls to be downloaded as-is (for example).
Sep 24, 2009
#6 code-rev...@gtempaccount.com
Comment by Shawn Pearce <sop@google.com> on Tue Apr 14 19:10:45 PDT 2009

*sigh*.  And always putting things into a ZIP file also isn't always secure.
It permits the attacker to create faux JAR files and other containers that may
be interpreted by the browser e.g. by an <applet> tag.
Sep 24, 2009
#7 code-rev...@gtempaccount.com
Comment by JT Olds <jtolds@xnet5.com> on Tue Apr 14 19:18:26 PDT 2009

Hmm, maybe there should instead be a configurable list of approved file types.
Sep 24, 2009
#8 code-rev...@gtempaccount.com
Comment by Shawn Pearce <sop@google.com> on Wed Apr 15 09:22:55 PDT 2009

I randomized the file names in the ZIP archive, making it nearly impossible to
predict in advance what the item will be called within the faux JAR.

I've sent the change off to some peers for an independent code audit.  Its
going to wait until I get a response.
Sep 24, 2009
#9 code-rev...@gtempaccount.com
Comment by Shawn Pearce <sop@google.com> on Fri Apr 17 15:25:27 PDT 2009

Fixed by https://review.source.android.com/9533 but disabling the ZIP
container has been split out into GERRIT-139.
Sep 24, 2009
#10 code-rev...@gtempaccount.com
Update by Shawn Pearce <sop@google.com> on Fri Apr 17 15:25:27 PDT 2009

Fixed in version 2.0.10.
Status: Fixed
Sep 25, 2009
#11 code-rev...@gtempaccount.com
(No comment was entered for this change.)
Labels: FixedIn-2.0.10
Oct 25, 2012
#12 sop@google.com
(No comment was entered for this change.)
Status: Released