Patchwork [STABLE] tests-subrepo-git: make the "pwned" message output in a stable order

# HG changeset patch
# User Danek Duvall <danek.duvall@oracle.com>
# Date 1464309643 25200
#      Thu May 26 17:40:43 2016 -0700
# Branch stable
# Node ID 821cafd65aac2b2199764acba0b1ede8d6e62b6e
# Parent  89bba2beb03ea62e7fc8bcf3272e3cda1065ad89
tests-subrepo-git: make the "pwned" message output in a stable order

The "pwned" message from this test gets gets sent to stderr, and so gets
emitted in different places from run to run in the rest of mercurial's
output.  This patch forces the message to go to a specific file instead,
whose contents we can examine in a stable point in the test's execution.

danek.duvall@oracle.com writes:

> # HG changeset patch
> # User Danek Duvall <danek.duvall@oracle.com>
> # Date 1464309643 25200
> #      Thu May 26 17:40:43 2016 -0700
> # Branch stable
> # Node ID 821cafd65aac2b2199764acba0b1ede8d6e62b6e
> # Parent  89bba2beb03ea62e7fc8bcf3272e3cda1065ad89
> tests-subrepo-git: make the "pwned" message output in a stable order
>
> The "pwned" message from this test gets gets sent to stderr, and so gets
> emitted in different places from run to run in the rest of mercurial's
> output.  This patch forces the message to go to a specific file instead,
> whose contents we can examine in a stable point in the test's execution.

Seems fine to me.

the theory sounds right, but i think the expected results need to be
generated from a system where git isn't broken.
i.e. `pwned` is not the proper expected output.

On Thu, May 26, 2016 at 8:47 PM,  <danek.duvall@oracle.com> wrote:
> # HG changeset patch
> # User Danek Duvall <danek.duvall@oracle.com>
> # Date 1464309643 25200
> #      Thu May 26 17:40:43 2016 -0700
> # Branch stable
> # Node ID 821cafd65aac2b2199764acba0b1ede8d6e62b6e
> # Parent  89bba2beb03ea62e7fc8bcf3272e3cda1065ad89
> tests-subrepo-git: make the "pwned" message output in a stable order
>
> The "pwned" message from this test gets gets sent to stderr, and so gets
> emitted in different places from run to run in the rest of mercurial's
> output.  This patch forces the message to go to a specific file instead,
> whose contents we can examine in a stable point in the test's execution.
>
> diff --git a/tests/test-subrepo-git.t b/tests/test-subrepo-git.t
> --- a/tests/test-subrepo-git.t
> +++ b/tests/test-subrepo-git.t
> @@ -1135,7 +1135,7 @@ make sure we show changed files, rather
>  test for Git CVE-2016-3068
>    $ hg init malicious-subrepository
>    $ cd malicious-subrepository
> -  $ echo "s = [git]ext::sh -c echo% pwned% >&2" > .hgsub
> +  $ echo "s = [git]ext::sh -c echo% pwned% >pwned.txt" > .hgsub
>    $ git init s
>    Initialized empty Git repository in $TESTTMP/tc/malicious-subrepository/s/.git/
>    $ cd s
> @@ -1149,19 +1149,20 @@ test for Git CVE-2016-3068
>    Cloning into '$TESTTMP/tc/malicious-subrepository-protected/s'... (glob)
>    fatal: transport 'ext' not allowed
>    updating to branch default
> -  cloning subrepo s from ext::sh -c echo% pwned% >&2
> +  cloning subrepo s from ext::sh -c echo% pwned% >pwned.txt
>    abort: git clone error 128 in s (in subrepo s)
>    [255]
>
>  whitelisting of ext should be respected (that's the git submodule behaviour)
>    $ env GIT_ALLOW_PROTOCOL=ext hg clone malicious-subrepository malicious-subrepository-clone-allowed
>    Cloning into '$TESTTMP/tc/malicious-subrepository-clone-allowed/s'... (glob)
> -  pwned
>    fatal: Could not read from remote repository.
>
>    Please make sure you have the correct access rights
>    and the repository exists.
>    updating to branch default
> -  cloning subrepo s from ext::sh -c echo% pwned% >&2
> +  cloning subrepo s from ext::sh -c echo% pwned% >pwned.txt
>    abort: git clone error 128 in s (in subrepo s)
>    [255]
> +  $ cat pwned.txt
> +  pwned
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

timeless wrote:

> the theory sounds right, but i think the expected results need to be
> generated from a system where git isn't broken.
> i.e. `pwned` is not the proper expected output.

Git isn't broken.  Mercurial is, with the use of GIT_ALLOW_PROTOCOL,
allowing the ext protocol, which is subvertible even with everything fixed,
which is why the fix for CVE-2016-3068 was to disallow that protocol unless
it's explicitly specified.  This test is for that behavior -- do we still
allow the ext protocol and the possible pwnage if the user begs for it.

Danek

Kevin Bullock wrote:

> > On May 26, 2016, at 19:47, danek.duvall@oracle.com wrote:
> > 
> > # HG changeset patch
> > # User Danek Duvall <danek.duvall@oracle.com>
> > # Date 1464309643 25200
> > #      Thu May 26 17:40:43 2016 -0700
> > # Branch stable
> > # Node ID 821cafd65aac2b2199764acba0b1ede8d6e62b6e
> > # Parent  89bba2beb03ea62e7fc8bcf3272e3cda1065ad89
> > tests-subrepo-git: make the "pwned" message output in a stable order
> > 
> > The "pwned" message from this test gets gets sent to stderr, and so gets
> > emitted in different places from run to run in the rest of mercurial's
> > output.  This patch forces the message to go to a specific file instead,
> > whose contents we can examine in a stable point in the test's execution.
> > 
> > diff --git a/tests/test-subrepo-git.t b/tests/test-subrepo-git.t
> > --- a/tests/test-subrepo-git.t
> > +++ b/tests/test-subrepo-git.t
> > @@ -1135,7 +1135,7 @@ make sure we show changed files, rather 
> > test for Git CVE-2016-3068
> >   $ hg init malicious-subrepository
> >   $ cd malicious-subrepository
> > -  $ echo "s = [git]ext::sh -c echo% pwned% >&2" > .hgsub
> > +  $ echo "s = [git]ext::sh -c echo% pwned% >pwned.txt" > .hgsub
> 
> We should check the contents of pwned.txt at each of these tests.

That's a good point; I'll whip up a V2.

> And as timeless pointed out, it should be empty on a correct system.

Perhaps I'm not understanding the test properly, but I don't think that's
correct.  Is my explanation flawed?

Never mind that I've no idea how you'd test both (either?) with a broken
git and (or) a fixed one.

Thanks,
Danek

On Fri, May 27, 2016 at 1:21 PM, Danek Duvall <danek.duvall@oracle.com> wrote:
> Kevin Bullock wrote:
>> And as timeless pointed out, it should be empty on a correct system.
>
> Perhaps I'm not understanding the test properly, but I don't think that's
> correct.  Is my explanation flawed?
>
> Never mind that I've no idea how you'd test both (either?) with a broken
> git and (or) a fixed one.

Build a modern version of git and add it to your path before
everything else. Then try running the current tests. You shouldn't get
a test failure.

i.e. `pwned` shouldn't appear.

That's the definition of "passing" the test. And it's how the tests
should pass after you improve them too (i.e. having no difference
between expected output and actual output, and not showing pwned).

Failing (showing pwned, having a difference from the expected output)
is expected for people (like me) running an old version of git.

Yes, it's awkward to be designing a test around the output from an old
version of git, but recording the output from a new version of git,
but that's what should be done.