Comment 5 for bug 596796

On Thu, Jun 24, 2010 at 12:28 AM, Aaron Bentley <email address hidden> wrote:
> The changes introduced in a prerequisite branch are not in the scope of
> a code review.  This is why they are not shown in the diff.

Agreed,

> If you could change/add/remove the prerequisite branch, you would be
> changing the scope of the code review.  By deleting a prerequisite
> branch, you could retroactively cause an approval of changes that the
> reviewer had never seen.  By adding a merge proposal, you could remove
> changes that the reviewer might consider vital.

You can change the scope of a code review in arbitrary ways if there
is a pre-requisite branch at all.

That is, the change 'alter from prerequisite None to prerequisite
myhack' is equivalent to 'alter from prerequisite
myharmlessbranch@rev-GOOD to myharmlessbranch@rev-BAD'.

So we need to lock down/protect/whatever the revision of the
prerequisite branch rather than the branch that it happens to be
pointing at. We could lock down the branch it points at, but doing so
does not add security. And such a lock down makes it less obvious how
the user changes the setting (unless we have hidden actions - edit
the prerequisite branch -> does a resubmit').

So I'm going to reopen this, because I think its a good idea to make
it obvious to the user how to change the prerequisite branch. If the
code team really don't want a *UI* that lets the user discover how to
make it happen directly, please do close it again.

I am happy with any implementation: if clicking on the edit icon
results in a resubmit, thats fine - the user visible 'how do I make it
work' will be clear.

Details on the attack-without-changing-the-prerequisite setting
follow, in case I wasn't clear enough above.

For instance.
Take a project A
Submit an empty branch ('E') for landing in A
Submit another branch ('G') with good code dependendent on 'E'

Now, you can change the diff in 'G' by changing the content of 'E'.
You can push a 'small change' which is a merge from E containing a sec
vulnerability - for instance - and as long as you push the vuln to E
at the same time, the reviewer won't see it.