Issue 3586: User with "Create Reference" can bypass code review
Status:  New
Owner: ----
Project Member Reported by dougk....@gmail.com, Sep 28, 2015
Affected Version: 2.10.6

What steps will reproduce the problem?
1. Grant a user explicit "create reference" under some portion of the repo (i.e. refs/heads/development/*)
2. Create a commit not on any branch
3. Push the commit directly to a new branch (i.e. refs/heads/development/test)

What is the expected output? What do you see instead?
The change is merged directly to the repository without review, but the change should require review.


Please provide any additional information below.
The issue seems to be on line 264 on of RefControl.java (from 2.10.6):
      return admin
          || (owner && !isBlocked(Permission.CREATE))
          || (canPerform(Permission.CREATE) && (!existsOnServer && canUpdate() || projectControl
              .canReadCommit(rw, (RevCommit) object)));

Looking at this, I'm not sure if it's an order of operations issue (but the order of operations is certainly unclear in that third "or" clause), but I believe this should read:
      return admin
          || (owner && !isBlocked(Permission.CREATE))
          || (canPerform(Permission.CREATE) && ((!existsOnServer && canUpdate()) || (
              (existsOnServer && projectControl.canReadCommit(rw, (RevCommit) object)));
Sep 28, 2015
Project Member #1 dougk....@gmail.com
Correction, I meant to write:
      return admin
          || (owner && !isBlocked(Permission.CREATE))
          || (canPerform(Permission.CREATE) && ((!existsOnServer && canUpdate()) ||
              (existsOnServer && projectControl.canReadCommit(rw, (RevCommit) object))));

Sep 28, 2015
Project Member #2 dougk....@gmail.com
One last correction, and it *appears* to be working right in my test environment.

ReceiveCommits.java line 1006:
    if (ctl.canCreate(rp.getRevWalk(), obj, allRefs.values().contains(obj))) {

should now read:
    if (ctl.canCreate(rp.getRevWalk(), obj, rp.getAdvertisedObjects().contains(obj))) {

Upon doing this, existence on the server is evaluated properly *but* it doesn't check to ensure the change is merged into a branch.  Previously, existsOnServer was always evaluating to false.  If the change is pushed for review with only the modifications here, a user can again bypass code review.
Sep 28, 2015
Project Member #3 dougk....@gmail.com
This ugly blob seems to behave in a way I imagine would be "sane":

    boolean containsObject = false;
    RevWalk rw = rp.getRevWalk();
    for (Ref ref : allRefs.values()) {
      if (ref.getName().startsWith(REFS_CHANGES)
          || ref.getName().startsWith(RefNames.REFS_CACHE_AUTOMERGE)) {
        continue;
      }
      try {
        containsObject = containsObject || rw.isMergedInto(rw.parseCommit(obj.getId()), rw.parseCommit(ref.getObjectId()));
      } catch (IOException e) {
        // Ignore exceptions.
      }
    }

Clearly not ideal.  But works.
Sep 29, 2015
Project Member #4 dougk....@gmail.com
OK, going back and looking at this on the advice of David Pursehouse, it seems this was fixed in 2.11.  Should this be noted accordingly for older versions?
Labels: FixedIn-2.11
Sep 29, 2015
Project Member #5 david.pu...@sonymobile.com
Looks like it was fixed by these commits which went into 2.11

https://gerrit-review.googlesource.com/#/c/59043/
https://gerrit-review.googlesource.com/#/c/59081/

Labels: -NonPublic