Issue 1589: Security with draft patch sets below another patch set over anonymous HTTP
Status:  New
Owner: ----
Reported by hughdave...@gmail.com, Sep 27, 2012
************************************************************
***** 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. Reduce READ permissions for anonymous users on a project to not include all branches
2. Create a patchset in a branch that is READable by anonymous, this patchset should depend on another patchset which is a draft (ie shouldn't be readable)
3. Try to git ls-remote over http, it will not show the draft, but it will show the child of the draft
4. fetch the child of the draft over http, then git show HEAD^

What is the expected output? What do you see instead?
Should not be able to fetch children of draft patchsets
Step 4 above shows the draft.

Please provide any additional information below.

This is related to  issue 1588 .

Currently, if an anonymous user passes the
allRefsAreVisible() method on the ProjectControl
class then they can see everything. This method
only takes into account the READ permission and
not draft status.
 
If that function fails, then a VisibleRefFilter class
is instantiated to filter out what the user can see.
This filter calls that same method, but as it returned
false before, it will do the same now. It then finds
all the visible changes for that project (taking into
account the draft status).

This means that to enforce drafts when the anonymous
user has READ permissions on the entire project. That
is the reason for step 1.

The above description is  issue 1588 . An extension to
that is to limit children of patches as well. So the
VisibleRefFilter would need to traverse the git tree
from a potential ref to determine whether all it's
parents are also visible (taking into account draft
status)

I have a partial solution to this, with a few issues
but am happy to submit it to whereever you want it
to get feedback.
Sep 27, 2012
#1 sop@google.com
Scanning the history from a candidate ref to see if all of its parents are visible is horrifically expensive at ls-remote time.

Instead I would suggest we disallow this in the first place. A change should not be allowed to depend on another change that is in draft status unless the change is also in draft status. This way you can't publish a change and accidentally publish the ancestor.
Sep 27, 2012
#2 hughdave...@gmail.com
I would also agree with this. I just assumed it was a wanted feature (although weird that you can get in that situation). I also found that you could depend on a draft, that you then deleted, and the top change was still submittable. Not sure what that does as haven't clicked the button yet!