[PATCH] handle_remote_ls_ctx can parsing href starting at http://

[PATCH] handle_remote_ls_ctx can parsing href starting at http://

[Kirill A. Korinskiy]

The program call remote_ls() to get remote objects over http;
handle_remote_ls_ctx() is used to parse it's response to populated
"struct remote_ls_ctx" that is returned from remote_ls().

The handle_remote_ls_ctx() function assumed that the server will
returned local path in href field, but RFC 4918 demand of support full
URI (http://localhost/repo.git for example).

This resulted in push failure (git-http-push ask server
PROPFIND /repo.git/alhost:8080/repo.git/refs/) when a server returned
full URI.

Signed-off-by: Kirill A. Korinskiy <catap@catap.ru>
---
 http-push.c |   25 +++++++++++++++++++------
 1 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/http-push.c b/http-push.c
index 7c6460919bf3eba10c46cede11ffdd9c53fd2dd2..a4b7d08663504a57008f66a39fffe293f62c1d08 100644
--- a/http-push.c
+++ b/http-push.c
@@ -87,6 +87,7 @@ static struct object_list *objects;
 struct repo
 {
 	char *url;
+	char *path;
 	int path_len;
 	int has_info_refs;
 	int can_update_info_refs;
@@ -1424,9 +1425,19 @@ static void handle_remote_ls_ctx(struct xml_ctx *ctx, int tag_closed)
 				ls->userFunc(ls);
 			}
 		} else if (!strcmp(ctx->name, DAV_PROPFIND_NAME) && ctx->cdata) {
-			ls->dentry_name = xmalloc(strlen(ctx->cdata) -
+			char *path = ctx->cdata;
+			if (*ctx->cdata == 'h') {
+				path = strstr(path, "//");
+				if (path) {
+					path = strchr(path+2, '/');
+				}
+			}
+			if (path) {
+				path += remote->path_len;
+			}
+			ls->dentry_name = xmalloc(strlen(path) -
 						  remote->path_len + 1);
-			strcpy(ls->dentry_name, ctx->cdata + remote->path_len);
+			strcpy(ls->dentry_name, path + remote->path_len);
 		} else if (!strcmp(ctx->name, DAV_PROPFIND_COLLECTION)) {
 			ls->dentry_flags |= IS_DIR;
 		}
@@ -2206,10 +2217,11 @@ int main(int argc, char **argv)
 		if (!remote->url) {
 			char *path = strstr(arg, "//");
 			remote->url = arg;
+			remote->path_len = strlen(arg);
 			if (path) {
-				path = strchr(path+2, '/');
-				if (path)
-					remote->path_len = strlen(path);
+				remote->path = strchr(path+2, '/');
+				if (remote->path)
+					remote->path_len = strlen(remote->path);
 			}
 			continue;
 		}
@@ -2238,8 +2250,9 @@ int main(int argc, char **argv)
 		rewritten_url = xmalloc(strlen(remote->url)+2);
 		strcpy(rewritten_url, remote->url);
 		strcat(rewritten_url, "/");
+		remote->path = rewritten_url + (remote->path - remote->url);
+		remote->path_len++;
 		remote->url = rewritten_url;
-		++remote->path_len;
 	}
 
 	/* Verify DAV compliance/lock support */
-- 
1.5.6.5

Re: [PATCH] handle_remote_ls_ctx can parsing href starting at http://

[Junio C Hamano]

"Kirill A. Korinskiy" <catap@catap.ru> writes:

> The program call remote_ls() to get remote objects over http;
> handle_remote_ls_ctx() is used to parse it's response to populated
> "struct remote_ls_ctx" that is returned from remote_ls().
>
> The handle_remote_ls_ctx() function assumed that the server will
> returned local path in href field, but RFC 4918 demand of support full
> URI (http://localhost/repo.git for example).
>
> This resulted in push failure (git-http-push ask server
> PROPFIND /repo.git/alhost:8080/repo.git/refs/) when a server returned
> full URI.

Thanks.

Do you mean PROPFIND was made to that garbage with :8080 in it when the
server returned a full URI http://localhost/repo.git as in the example in
the previous paragraph, or are you using a different example here?

I am contemplating of munging your commit log message like this...

commit e1f33efe07b9a520505fccd71bea1292fc9448dd
Author: Kirill A. Korinskiy <catap@catap.ru>
Date:   Tue Dec 23 11:31:15 2008 +0300

    http-push: support full URI in handle_remote_ls_ctx()
    
    The program calls remote_ls() to get list of files from the server over
    HTTP; handle_remote_ls_ctx() is used to parse its response to populate
    "struct remote_ls_ctx" that is returned from remote_ls().
    
    The handle_remote_ls_ctx() function assumed that the server returns a
    local path in href field, but RFC 4918 (14.7) demand of support full URI
    (e.g. "http://localhost:8080/repo.git").
    
    This resulted in push failure (e.g. git-http-push issues a PROPFIND
    request to "/repo.git/alhost:8080/repo.git/refs/" to the server).
    
    Signed-off-by: Kirill A. Korinskiy <catap@catap.ru>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

[PATCH] http-push: support full URI in handle_remote_ls_ctx()

[Kirill A. Korinskiy]

The program calls remote_ls() to get list of files from the server
over HTTP; handle_remote_ls_ctx() is used to parse its response to
populate "struct remote_ls_ctx" that is returned from remote_ls().

The handle_remote_ls_ctx() function assumed that the server returns a
local path in href field, but RFC 4918 (14.7) demand of support full
URI (e.g. "http://localhost:8080/repo.git").

This resulted in push failure (e.g. git-http-push issues a PROPFIND
request to "/repo.git/alhost:8080/repo.git/refs/" to the server).

Signed-off-by: Kirill A. Korinskiy <catap@catap.ru>
---
 http-push.c |   25 +++++++++++++++++++------
 1 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/http-push.c b/http-push.c
index 7c6460919bf3eba10c46cede11ffdd9c53fd2dd2..a4b7d08663504a57008f66a39fffe293f62c1d08 100644
--- a/http-push.c
+++ b/http-push.c
@@ -87,6 +87,7 @@ static struct object_list *objects;
 struct repo
 {
 	char *url;
+	char *path;
 	int path_len;
 	int has_info_refs;
 	int can_update_info_refs;
@@ -1424,9 +1425,19 @@ static void handle_remote_ls_ctx(struct xml_ctx *ctx, int tag_closed)
 				ls->userFunc(ls);
 			}
 		} else if (!strcmp(ctx->name, DAV_PROPFIND_NAME) && ctx->cdata) {
-			ls->dentry_name = xmalloc(strlen(ctx->cdata) -
+			char *path = ctx->cdata;
+			if (*ctx->cdata == 'h') {
+				path = strstr(path, "//");
+				if (path) {
+					path = strchr(path+2, '/');
+				}
+			}
+			if (path) {
+				path += remote->path_len;
+			}
+			ls->dentry_name = xmalloc(strlen(path) -
 						  remote->path_len + 1);
-			strcpy(ls->dentry_name, ctx->cdata + remote->path_len);
+			strcpy(ls->dentry_name, path + remote->path_len);
 		} else if (!strcmp(ctx->name, DAV_PROPFIND_COLLECTION)) {
 			ls->dentry_flags |= IS_DIR;
 		}
@@ -2206,10 +2217,11 @@ int main(int argc, char **argv)
 		if (!remote->url) {
 			char *path = strstr(arg, "//");
 			remote->url = arg;
+			remote->path_len = strlen(arg);
 			if (path) {
-				path = strchr(path+2, '/');
-				if (path)
-					remote->path_len = strlen(path);
+				remote->path = strchr(path+2, '/');
+				if (remote->path)
+					remote->path_len = strlen(remote->path);
 			}
 			continue;
 		}
@@ -2238,8 +2250,9 @@ int main(int argc, char **argv)
 		rewritten_url = xmalloc(strlen(remote->url)+2);
 		strcpy(rewritten_url, remote->url);
 		strcat(rewritten_url, "/");
+		remote->path = rewritten_url + (remote->path - remote->url);
+		remote->path_len++;
 		remote->url = rewritten_url;
-		++remote->path_len;
 	}
 
 	/* Verify DAV compliance/lock support */
-- 
1.5.6.5

Re: [PATCH] http-push: support full URI in handle_remote_ls_ctx()

[Mike Hommey]

On Mon, Dec 29, 2008 at 05:32:15AM +0300, Kirill A. Korinskiy wrote:
> The program calls remote_ls() to get list of files from the server
> over HTTP; handle_remote_ls_ctx() is used to parse its response to
> populate "struct remote_ls_ctx" that is returned from remote_ls().
> 
> The handle_remote_ls_ctx() function assumed that the server returns a
> local path in href field, but RFC 4918 (14.7) demand of support full
> URI (e.g. "http://localhost:8080/repo.git").
> 
> This resulted in push failure (e.g. git-http-push issues a PROPFIND
> request to "/repo.git/alhost:8080/repo.git/refs/" to the server).
> 
> Signed-off-by: Kirill A. Korinskiy <catap@catap.ru>
> ---
>  http-push.c |   25 +++++++++++++++++++------
>  1 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/http-push.c b/http-push.c
> index 7c6460919bf3eba10c46cede11ffdd9c53fd2dd2..a4b7d08663504a57008f66a39fffe293f62c1d08 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -87,6 +87,7 @@ static struct object_list *objects;
>  struct repo
>  {
>  	char *url;
> +	char *path;
>  	int path_len;
>  	int has_info_refs;
>  	int can_update_info_refs;
> @@ -1424,9 +1425,19 @@ static void handle_remote_ls_ctx(struct xml_ctx *ctx, int tag_closed)
>  				ls->userFunc(ls);
>  			}
>  		} else if (!strcmp(ctx->name, DAV_PROPFIND_NAME) && ctx->cdata) {
> -			ls->dentry_name = xmalloc(strlen(ctx->cdata) -
> +			char *path = ctx->cdata;
> +			if (*ctx->cdata == 'h') {
> +				path = strstr(path, "//");

I would make that a "://"

> +				if (path) {
> +					path = strchr(path+2, '/');

and s/2/3/, accordingly.

But I realize the existing code already does something like what your
are doing...

Mike

Re: [PATCH] http-push: support full URI in handle_remote_ls_ctx()

[Junio C Hamano]

Thanks; already applied ;-)

[PATCH] http-push: support full URI in handle_remote_ls_ctx()

[Kirill A. Korinskiy]

The program calls remote_ls() to get list of files from the server
over HTTP; handle_remote_ls_ctx() is used to parse its response to
populate "struct remote_ls_ctx" that is returned from remote_ls().

The handle_remote_ls_ctx() function assumed that the server returns a
local path in href field, but RFC 4918 (14.7) demand of support full
URI (e.g. "http://localhost:8080/repo.git").

This resulted in push failure (e.g. git-http-push issues a PROPFIND
request to "/repo.git/alhost:8080/repo.git/refs/" to the server).

Signed-off-by: Kirill A. Korinskiy <catap@catap.ru>
---
 http-push.c |   25 +++++++++++++++++++------
 1 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/http-push.c b/http-push.c
index 7c6460919bf3eba10c46cede11ffdd9c53fd2dd2..d1749fe2ffd6a59a4eea514997a62b5d7c80b438 100644
--- a/http-push.c
+++ b/http-push.c
@@ -87,6 +87,7 @@ static struct object_list *objects;
 struct repo
 {
 	char *url;
+	char *path;
 	int path_len;
 	int has_info_refs;
 	int can_update_info_refs;
@@ -1424,9 +1425,19 @@ static void handle_remote_ls_ctx(struct xml_ctx *ctx, int tag_closed)
 				ls->userFunc(ls);
 			}
 		} else if (!strcmp(ctx->name, DAV_PROPFIND_NAME) && ctx->cdata) {
-			ls->dentry_name = xmalloc(strlen(ctx->cdata) -
+			char *path = ctx->cdata;
+			if (*ctx->cdata == 'h') {
+				path = strstr(path, "://");
+				if (path) {
+					path = strchr(path+3, '/');
+				}
+			}
+			if (path) {
+				path += remote->path_len;
+			}
+			ls->dentry_name = xmalloc(strlen(path) -
 						  remote->path_len + 1);
-			strcpy(ls->dentry_name, ctx->cdata + remote->path_len);
+			strcpy(ls->dentry_name, path + remote->path_len);
 		} else if (!strcmp(ctx->name, DAV_PROPFIND_COLLECTION)) {
 			ls->dentry_flags |= IS_DIR;
 		}
@@ -2206,10 +2217,11 @@ int main(int argc, char **argv)
 		if (!remote->url) {
 			char *path = strstr(arg, "//");
 			remote->url = arg;
+			remote->path_len = strlen(arg);
 			if (path) {
-				path = strchr(path+2, '/');
-				if (path)
-					remote->path_len = strlen(path);
+				remote->path = strchr(path+2, '/');
+				if (remote->path)
+					remote->path_len = strlen(remote->path);
 			}
 			continue;
 		}
@@ -2238,8 +2250,9 @@ int main(int argc, char **argv)
 		rewritten_url = xmalloc(strlen(remote->url)+2);
 		strcpy(rewritten_url, remote->url);
 		strcat(rewritten_url, "/");
+		remote->path = rewritten_url + (remote->path - remote->url);
+		remote->path_len++;
 		remote->url = rewritten_url;
-		++remote->path_len;
 	}
 
 	/* Verify DAV compliance/lock support */
-- 
1.5.6.5

Re: [PATCH] http-push: support full URI in handle_remote_ls_ctx()

[Junio C Hamano]

"Kirill A. Korinskiy" <catap@catap.ru> writes:

> @@ -1424,9 +1425,19 @@ static void handle_remote_ls_ctx(struct xml_ctx *ctx, int tag_closed)
>  				ls->userFunc(ls);
>  			}
>  		} else if (!strcmp(ctx->name, DAV_PROPFIND_NAME) && ctx->cdata) {
> -			ls->dentry_name = xmalloc(strlen(ctx->cdata) -
> +			char *path = ctx->cdata;
> +			if (*ctx->cdata == 'h') {
> +				path = strstr(path, "://");
> +				if (path) {
> +					path = strchr(path+3, '/');
> +				}
> +			}

Is this "://" (and +3) the only change from the previous one that has
already been queued?  I didn't have a problem with the old "//" one.

The check to see if it begins with 'h' bothers me much much more.

If you want to be defensively tight, you should be checking if it begins
with either "http://" or "https://", the only two protocols you are
prepared to handle, and nothing else, so that you won't trigger this
codepath when the other end gave you "hqrt://..", on the basis that your
code won't know if hqrt:// protocol works the same way as http and https.

On the other hand, if you want to be optimistically loose, expecting
whatever people would implement that can be handled with the existing DAV
code would behave the same way as http and https, you shouldn't be
limiting yourself to an unknown protocol name that happens to begin with
an 'h', only accepting "hqrt://" but not "ittp://" URLs.

Your "first byte of the protocol name must be 'h'" does not do either.

Re: [PATCH] handle_remote_ls_ctx can parsing href starting at http://

[Junio C Hamano]

"Kirill A. Korinskiy" <catap@catap.ru> writes:

> The program call remote_ls() to get remote objects over http;
> handle_remote_ls_ctx() is used to parse it's response to populated
> "struct remote_ls_ctx" that is returned from remote_ls().
>
> The handle_remote_ls_ctx() function assumed that the server will
> returned local path in href field, but RFC 4918 demand of support full
> URI (http://localhost/repo.git for example).

Do you mean "the client should support both server-relative '/repo.git'
and full 'http://localhost/repo.git'", or "the client should reject
'/repo.git' and insist on full 'http://localhost/repo.git'"?  I am
guessing the former but it is not quite clear.  Where in 4918 is this
specified?

> This resulted in push failure (git-http-push ask server
> PROPFIND /repo.git/alhost:8080/repo.git/refs/) when a server returned
> full URI.

This is an interesting but confusing example.

Do you mean the bug is:

 (1) the client asks PROPFIND /repo.git/;

 (2) the server gives http://localhost/repo.git/refs back;

 (3) the client incorrectly assumes that the response would start with
     /repo.git/ (e.g. "/repo.git/refs"), so strips 10 bytes from the
     beginning of this result and uses the remainder as the "new"
     information to dig deeper; i.e. "alhost/repo.git/refs";

 (4) the new part is appended to the original path and the client forms
     the next request "PROPFIND /repo.git/alhost/repo.git/refs/";

 (5) instead, the client should strip the proto://host part (if exists)
     and request "PROPFIND /repo.git/refs/".

> @@ -1424,9 +1425,10 @@ static void handle_remote_ls_ctx(struct xml_ctx *ctx, int tag_closed)
>  				ls->userFunc(ls);
>  			}
>  		} else if (!strcmp(ctx->name, DAV_PROPFIND_NAME) && ctx->cdata) {
> -			ls->dentry_name = xmalloc(strlen(ctx->cdata) -
> +			char *path = strstr(ctx->cdata, remote->path);
> +			ls->dentry_name = xmalloc(strlen(path) -
>  						  remote->path_len + 1);

What if you are talking to http://repo.git/repo.git/?  Doesn't this
strstr() misbehave?  Instead, shouldn't you be checking if the response
begins with proto://host/ and stripping it iff so?

[PATCH] handle_remote_ls_ctx can parsing href starting at http://

[Kirill A. Korinskiy]

The program call remote_ls() to get remote objects over http;
handle_remote_ls_ctx() is used to parse it's response to populated
"struct remote_ls_ctx" that is returned from remote_ls().

The handle_remote_ls_ctx() function assumed that the server will
returned local path in href field, but RFC 4918 demand of support full
URI (http://localhost/repo.git for example).

This resulted in push failure (git-http-push ask server
PROPFIND /repo.git/alhost:8080/repo.git/refs/) when a server returned
full URI.

Signed-off-by: Kirill A. Korinskiy <catap@catap.ru>
---
 http-push.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/http-push.c b/http-push.c
index 7c6460919bf3eba10c46cede11ffdd9c53fd2dd2..abf0269e0fe20a43a6247fc7584dc2d58322a8fa 100644
--- a/http-push.c
+++ b/http-push.c
@@ -87,6 +87,7 @@ static struct object_list *objects;
 struct repo
 {
 	char *url;
+	char *path;
 	int path_len;
 	int has_info_refs;
 	int can_update_info_refs;
@@ -1424,9 +1425,10 @@ static void handle_remote_ls_ctx(struct xml_ctx *ctx, int tag_closed)
 				ls->userFunc(ls);
 			}
 		} else if (!strcmp(ctx->name, DAV_PROPFIND_NAME) && ctx->cdata) {
-			ls->dentry_name = xmalloc(strlen(ctx->cdata) -
+			char *path = strstr(ctx->cdata, remote->path);
+			ls->dentry_name = xmalloc(strlen(path) -
 						  remote->path_len + 1);
-			strcpy(ls->dentry_name, ctx->cdata + remote->path_len);
+			strcpy(ls->dentry_name, path + remote->path_len);
 		} else if (!strcmp(ctx->name, DAV_PROPFIND_COLLECTION)) {
 			ls->dentry_flags |= IS_DIR;
 		}
@@ -2206,10 +2208,11 @@ int main(int argc, char **argv)
 		if (!remote->url) {
 			char *path = strstr(arg, "//");
 			remote->url = arg;
+			remote->path_len = strlen(arg);
 			if (path) {
-				path = strchr(path+2, '/');
-				if (path)
-					remote->path_len = strlen(path);
+				remote->path = strchr(path+2, '/');
+				if (remote->path)
+					remote->path_len = strlen(remote->path);
 			}
 			continue;
 		}
@@ -2238,8 +2241,9 @@ int main(int argc, char **argv)
 		rewritten_url = xmalloc(strlen(remote->url)+2);
 		strcpy(rewritten_url, remote->url);
 		strcat(rewritten_url, "/");
+		remote->path = rewritten_url + (remote->path - remote->url);
+		remote->path_len++;
 		remote->url = rewritten_url;
-		++remote->path_len;
 	}
 
 	/* Verify DAV compliance/lock support */
-- 
1.5.6.5