[PATCH] Disallow downloading disabled snapshot formats

[PATCH] Disallow downloading disabled snapshot formats

[cgit]

We did only display enabled snapshot formats but we did not prevent from
downloading disabled formats when requested. Fix this by adding an
appropriate check.

Also, add a test case that checks whether downloading disabled snapshot
formats is denied, as expected.

Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
---
 tests/t0107-snapshot.sh | 5 +++++
 ui-snapshot.c           | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/t0107-snapshot.sh b/tests/t0107-snapshot.sh
index 6cf7aaa..01e8d22 100755
--- a/tests/t0107-snapshot.sh
+++ b/tests/t0107-snapshot.sh
@@ -79,4 +79,9 @@ test_expect_success UNZIP 'verify unzipped file-5' '
 	test_line_count = 1 master/file-5
 '
 
+test_expect_success 'try to download a disabled snapshot format' '
+	cgit_url "foo/snapshot/master.tar.xz" |
+	grep "Unsupported snapshot format"
+'
+
 test_done
diff --git a/ui-snapshot.c b/ui-snapshot.c
index 8f82119..ab20a4a 100644
--- a/ui-snapshot.c
+++ b/ui-snapshot.c
@@ -205,7 +205,7 @@ void cgit_print_snapshot(const char *head, const char *hex,
 	}
 
 	f = get_format(filename);
-	if (!f) {
+	if (!f || (snapshots & f->bit) == 0) {
 		show_error("Unsupported snapshot format: %s", filename);
 		return;
 	}
-- 
1.8.5.2

[PATCH] Disallow downloading disabled snapshot formats

[john]

On Fri, Jan 10, 2014 at 03:38:06PM +0100, Lukas Fleischer wrote:
> We did only display enabled snapshot formats but we did not prevent from
> downloading disabled formats when requested. Fix this by adding an
> appropriate check.
> 
> Also, add a test case that checks whether downloading disabled snapshot
> formats is denied, as expected.
> 
> Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
> ---
>  tests/t0107-snapshot.sh | 5 +++++
>  ui-snapshot.c           | 2 +-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/t0107-snapshot.sh b/tests/t0107-snapshot.sh
> index 6cf7aaa..01e8d22 100755
> --- a/tests/t0107-snapshot.sh
> +++ b/tests/t0107-snapshot.sh
> @@ -79,4 +79,9 @@ test_expect_success UNZIP 'verify unzipped file-5' '
>  	test_line_count = 1 master/file-5
>  '
>  
> +test_expect_success 'try to download a disabled snapshot format' '
> +	cgit_url "foo/snapshot/master.tar.xz" |
> +	grep "Unsupported snapshot format"

I really dislike seeing pipes in the test suite.  Can we redirect to
file instead and then grep the file?  This helps ensure that the exit
code from CGit is correct (I don't know if we expect it to be zero or
non-zero here, but if the latter then at least test_must_fail checks
that the process didn't segfault - I suspect it should be zero though).

> +'
> +
>  test_done
> diff --git a/ui-snapshot.c b/ui-snapshot.c
> index 8f82119..ab20a4a 100644
> --- a/ui-snapshot.c
> +++ b/ui-snapshot.c
> @@ -205,7 +205,7 @@ void cgit_print_snapshot(const char *head, const char *hex,
>  	}
>  
>  	f = get_format(filename);
> -	if (!f) {
> +	if (!f || (snapshots & f->bit) == 0) {
>  		show_error("Unsupported snapshot format: %s", filename);
>  		return;
>  	}

[PATCH] Disallow downloading disabled snapshot formats

[cgit]

On Fri, 10 Jan 2014 at 15:50:14, John Keeping wrote:
> On Fri, Jan 10, 2014 at 03:38:06PM +0100, Lukas Fleischer wrote:
> [...]
> > +test_expect_success 'try to download a disabled snapshot format' '
> > +     cgit_url "foo/snapshot/master.tar.xz" |
> > +     grep "Unsupported snapshot format"
> 
> I really dislike seeing pipes in the test suite.  Can we redirect to
> file instead and then grep the file?  This helps ensure that the exit
> code from CGit is correct (I don't know if we expect it to be zero or
> non-zero here, but if the latter then at least test_must_fail checks
> that the process didn't segfault - I suspect it should be zero though).

Does this matter here at all? If cgit_url fails in any (unpredictable)
way, e.g. segfaults, it probably won't print the string "Unsupported
snapshot format" and the whole test case will fail. The Git test suite
itself also uses grep(1) with a pipe several times. I agree that we
shouldn't use pipes when inverting the result (looking for non-existent
patterns), though.

> 
> > +'
> > +
> >  test_done
> > diff --git a/ui-snapshot.c b/ui-snapshot.c
> > index 8f82119..ab20a4a 100644
> > --- a/ui-snapshot.c
> > +++ b/ui-snapshot.c
> > @@ -205,7 +205,7 @@ void cgit_print_snapshot(const char *head, const char *hex,
> >       }
> >  
> >       f = get_format(filename);
> > -     if (!f) {
> > +     if (!f || (snapshots & f->bit) == 0) {
> >               show_error("Unsupported snapshot format: %s", filename);
> >               return;
> >       }

[PATCH] Disallow downloading disabled snapshot formats

[Jason]

On Fri, Jan 10, 2014 at 3:38 PM, Lukas Fleischer <cgit at cryptocrack.de> wrote:
> We did only display enabled snapshot formats but we did not prevent from
> downloading disabled formats when requested. Fix this by adding an
> appropriate check.

Previously:
http://lists.zx2c4.com/pipermail/cgit/2012-June/000641.html
http://lists.zx2c4.com/pipermail/cgit/2012-October/000792.html

[PATCH] Disallow downloading disabled snapshot formats

[cgit]

On Fri, 10 Jan 2014 at 17:11:46, Jason A. Donenfeld wrote:
> On Fri, Jan 10, 2014 at 3:38 PM, Lukas Fleischer <cgit at cryptocrack.de> wrote:
> > We did only display enabled snapshot formats but we did not prevent from
> > downloading disabled formats when requested. Fix this by adding an
> > appropriate check.
> 
> Previously:
> http://lists.zx2c4.com/pipermail/cgit/2012-June/000641.html
> http://lists.zx2c4.com/pipermail/cgit/2012-October/000792.html

I was also thinking of server load. If we want to leave it as-is, we
should fix the documentation which currently says:

    repo.snapshots::
    	A mask of allowed snapshot-formats for this repo, restricted by
    	the "snapshots" global setting. Default value: <snapshots>.

There should at least be a note stating that "allowed" only means
"linked" here.