Snapshot URLs can conflict with two similar but unique tags

Snapshot URLs can conflict with two similar but unique tags

[wub]

Steps to reproduce bug:

  Create a repository with two tags, one prefixed with "v"-character and
  one without. These tags should reference two unique SHA-1 objects.

  For the sake of argument and following along, I will use an existing
  repository as a following example to reproduce the issue.

    $ git clone https://git.pantsu.cat/pantsu/pomf/
    Cloning into 'pomf'...
    Checking connectivity... done.
    $ cd pomf/
    $ git tag | grep "1.0.0$"
    1.0.0
    v1.0.0
    $ git log --oneline --decorate -n 1 1.0.0
    7f9d1cb (tag: 1.0.0) Add package.json and Gruntfile.js
    $ git log --oneline --decorate -n 1 v1.0.0
    687b1be (tag: v1.0.0) Merge branch 'nuck-dev'

  Setup cgit with this repository.

  Configure `snapshots` option in `cgitrc` for at least one file
  supported snapshot format (e.g. `.tar`, `.zip`) to enable snapshots.

    # Allow download of tar.gz, tar.bz2 and zip-files
    snapshots=tar.gz tar.xz zip

  Open a browser and go to `$schema://$cgit_uri/$repository/` (example:
  [1]). Look for a snapshot URL on the index page or in tag detail page at
  /$repository/tag/?h=$tag.

Expected behavior:

  (git.pantsu.cat is used as an example.)

  In somewhat simplified version, the HTML index document may be
  expected to look like:

    Tag     Download
    [...]
    v1.0.0  pomf-v1.0.0.zip pomf-v1.0.0.tar.gz pomf-v1.0.0.tar.xz
    [...]
    1.0.0   pomf-1.0.0.zip  pomf-1.0.0.tar.gz  pomf-1.0.0.tar.xz 

  In somewhat simplified version, the HTML tag detail document may be
  expected to look like the following example for tag v1.0.0:

    download  pomf-v1.0.0.zip
              pomf-v1.0.0.tar.gz
              pomf-v1.0.0.tar.xz

  In example, the download URL for 1.0.0 .zip snapshot is expected to be
  found at one or more of the following unique locations (adapt for
  other file extensions):

    https://git.pantsu.cat/pantsu/pomf/snapshot/1.0.0/pomf-1.0.0.zip
    https://git.pantsu.cat/pantsu/pomf/snapshot/pomf-1.0.0.zip

  In example, the download URL for v1.0.0 .zip snapshot is expected to
  be found at one or more of the following unique locations (adapt for
  other file extensions):

    https://git.pantsu.cat/pantsu/pomf/snapshot/v1.0.0/pomf-1.0.0.zip
    https://git.pantsu.cat/pantsu/pomf/snapshot/pomf-v1.0.0.zip

  The contents of both snapshots when extracted are expected to match
  those of v1.0.0 and 1.0.0 tags, respectively.

  The /$repository/snapshot/$tag/$project-$version.$ext syntax is a
  suggested enhancement to resolve the issue while keeping traditional
  file names without the "v"-prefix, if such functionality is desired.

Actual behavior:

  (git.pantsu.cat is used as an example.)

  In somewhat simplified version, the HTML document actually looks like:

    Tag     Download
    [...]
    v1.0.0  pomf-1.0.0.zip  pomf-1.0.0.tar.gz  pomf-1.0.0.tar.xz
    [...]
    1.0.0   pomf-1.0.0.zip  pomf-1.0.0.tar.gz  pomf-1.0.0.tar.xz 

  In somewhat simplified version, the HTML tag detail document for tag
  v1.0.0 actually looks like:

    download  pomf-1.0.0.zip
              pomf-1.0.0.tar.gz
              pomf-1.0.0.tar.xz

  In example, the .zip snapshot for tag 1.0.0 is hyperlinked to the
  following URL (adapt for other file extensions):

    https://git.pantsu.cat/pantsu/pomf/snapshot/pomf-1.0.0.zip

  In example, the .zip snapshot for tag v1.0.0 is hyperlinked to the
  same URL as 1.0.0 (adapt for other file extensions):

    https://git.pantsu.cat/pantsu/pomf/snapshot/pomf-1.0.0.zip

  The contents of snapshot downloaded from v1.0.0 tag hyperlink when
  extracted match that of 1.0.0 tag.

  In other words, cgit knows of snapshot named pomf-v1.0.0.zip (or
  alternative configured snapshot extension) and will happily serve it
  correctly when requested, but never links to it.

Version number:

  cgit v0.12

Operating system:

  CentOS 7

Notes:

  The "Pro Git" book by Scott Chacon and Ben Straub suggests prefixing
  tags with "v".[2] git.git follows this convention.[3]

[1]: https://git.pantsu.cat/pantsu/pomf/
[2]: https://git-scm.com/book/en/v2/Git-Basics-Tagging
[3]: https://git.kernel.org/cgit/git/git.git/refs/tags

Snapshot URLs can conflict with two similar but unique tags

[wub]

Sorry, the email subject is a little misleading as I edited this later.
It should say: "Snapshots may link to a wrong/different snapshot with
two similarly named but uniquely referenced tags". :)

[PATCH] Avoid ambiguities when prettifying snapshot names

[lfleischer]

When composing snapshot file names for a tag with a prefix of the form
v[0-9] (resp. V[0-9]), the leading "v" (resp. "V") is stripped. This
leads to conflicts if a tag with the stripped name already exists or if
there are tags only differing in the capitalization of the leading "v".
Make sure we do not strip the "v" in these cases.

Reported-by: Juuso Lapinlampi <wub at partyvan.eu>
Signed-off-by: Lukas Fleischer <lfleischer at lfos.de>
---
Note: The code that actually generates the snapshots works fine as it
always tries an exact match first.

 ui-refs.c   | 24 +++++++++---------------
 ui-shared.c | 26 +++++++++++++++++++++-----
 ui-shared.h |  2 ++
 3 files changed, 32 insertions(+), 20 deletions(-)

diff --git a/ui-refs.c b/ui-refs.c
index 5b4530e..75f2789 100644
--- a/ui-refs.c
+++ b/ui-refs.c
@@ -93,34 +93,28 @@ static void print_tag_header(void)
 static void print_tag_downloads(const struct cgit_repo *repo, const char *ref)
 {
 	const struct cgit_snapshot_format* f;
-	struct strbuf filename = STRBUF_INIT;
 	const char *basename;
-	int free_ref = 0;
+	struct strbuf filename = STRBUF_INIT;
+	size_t prefixlen;
 
 	if (!ref || strlen(ref) < 1)
 		return;
 
 	basename = cgit_repobasename(repo->url);
-	if (!starts_with(ref, basename)) {
-		if ((ref[0] == 'v' || ref[0] == 'V') && isdigit(ref[1]))
-			ref++;
-		if (isdigit(ref[0])) {
-			ref = fmtalloc("%s-%s", basename, ref);
-			free_ref = 1;
-		}
-	}
-
+	if (starts_with(ref, basename))
+		strbuf_addstr(&filename, ref);
+	else
+		cgit_compose_snapshot_prefix(&filename, basename, ref);
+	prefixlen = filename.len;
 	for (f = cgit_snapshot_formats; f->suffix; f++) {
 		if (!(repo->snapshots & f->bit))
 			continue;
-		strbuf_reset(&filename);
-		strbuf_addf(&filename, "%s%s", ref, f->suffix);
+		strbuf_setlen(&filename, prefixlen);
+		strbuf_addstr(&filename, f->suffix);
 		cgit_snapshot_link(filename.buf, NULL, NULL, NULL, NULL, filename.buf);
 		html("&nbsp;&nbsp;");
 	}
 
-	if (free_ref)
-		free((char *)ref);
 	strbuf_release(&filename);
 }
 
diff --git a/ui-shared.c b/ui-shared.c
index 2c88b72..1c449f3 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -1069,18 +1069,34 @@ void cgit_print_filemode(unsigned short mode)
 	html_fileperm(mode);
 }
 
+void cgit_compose_snapshot_prefix(struct strbuf *filename, const char *base,
+				  const char *ref)
+{
+	unsigned char sha1[20];
+
+	/*
+	 * Prettify snapshot names by stripping leading "v" or "V" if the tag
+	 * name starts with {v,V}[0-9] and the prettify mapping is injective,
+	 * i.e. each stripped tag can be inverted without ambiguities.
+	 */
+	if (get_sha1(fmt("refs/tags/%s", ref), sha1) == 0 &&
+	    (ref[0] == 'v' || ref[0] == 'V') && isdigit(ref[1]) &&
+	    ((get_sha1(fmt("refs/tags/%s", ref + 1), sha1) == 0) +
+	     (get_sha1(fmt("refs/tags/v%s", ref + 1), sha1) == 0) +
+	     (get_sha1(fmt("refs/tags/V%s", ref + 1), sha1) == 0) == 1))
+		ref++;
+
+	strbuf_addf(filename, "%s-%s", base, ref);
+}
+
 void cgit_print_snapshot_links(const char *repo, const char *head,
 			       const char *hex, int snapshots)
 {
 	const struct cgit_snapshot_format* f;
 	struct strbuf filename = STRBUF_INIT;
 	size_t prefixlen;
-	unsigned char sha1[20];
 
-	if (get_sha1(fmt("refs/tags/%s", hex), sha1) == 0 &&
-	    (hex[0] == 'v' || hex[0] == 'V') && isdigit(hex[1]))
-		hex++;
-	strbuf_addf(&filename, "%s-%s", cgit_repobasename(repo), hex);
+	cgit_compose_snapshot_prefix(&filename, cgit_repobasename(repo), hex);
 	prefixlen = filename.len;
 	for (f = cgit_snapshot_formats; f->suffix; f++) {
 		if (!(snapshots & f->bit))
diff --git a/ui-shared.h b/ui-shared.h
index b457c97..87799f1 100644
--- a/ui-shared.h
+++ b/ui-shared.h
@@ -71,6 +71,8 @@ __attribute__((format (printf,3,4)))
 extern void cgit_print_error_page(int code, const char *msg, const char *fmt, ...);
 extern void cgit_print_pageheader(void);
 extern void cgit_print_filemode(unsigned short mode);
+extern void cgit_compose_snapshot_prefix(struct strbuf *filename,
+					 const char *base, const char *ref);
 extern void cgit_print_snapshot_links(const char *repo, const char *head,
 				      const char *hex, int snapshots);
 extern void cgit_add_hidden_formfields(int incl_head, int incl_search,
-- 
2.8.3

[PATCH] Avoid ambiguities when prettifying snapshot names

[lfleischer]

On Tue, 24 May 2016 at 18:15:18, Lukas Fleischer wrote:
> When composing snapshot file names for a tag with a prefix of the form
> v[0-9] (resp. V[0-9]), the leading "v" (resp. "V") is stripped. This
> leads to conflicts if a tag with the stripped name already exists or if
> there are tags only differing in the capitalization of the leading "v".
> Make sure we do not strip the "v" in these cases.
> 
> Reported-by: Juuso Lapinlampi <wub at partyvan.eu>
> Signed-off-by: Lukas Fleischer <lfleischer at lfos.de>
> ---
> Note: The code that actually generates the snapshots works fine as it
> always tries an exact match first.
> 
>  ui-refs.c   | 24 +++++++++---------------
>  ui-shared.c | 26 +++++++++++++++++++++-----
>  ui-shared.h |  2 ++
>  3 files changed, 32 insertions(+), 20 deletions(-)

Any comments on this?

[PATCH] Avoid ambiguities when prettifying snapshot names

[Jason]

Hey Lukas,

I'll probably merge this patch this week. It's a good basis for
another fix I'm working on with snapshot names for tags that include a
forward-slash ('/') character.

Jason

[PATCH] Avoid ambiguities when prettifying snapshot names

[Jason]

Hey Lukas,

I appreciate that this unifies the 'v' detection code as well. I also
like the "(a==0 + b==0 + c==0) == 1" trick. I'll merge this, and base
the fixes for tags with / in them on this commit.

Thanks,
Jason