bug in HTTP protocol spec

bug in HTTP protocol spec

[Dorian Taylor]

Hello list,

I had been banging my head all morning trying to figure out why I couldn’t get a little HTTP implementation to clone/push via the smart protocol (just wrapping git-receive-pack/git-upload-pack). I kept getting the following (likely familiar to some) error:

```
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
```

I didn’t get an insight until I ran with GIT_TRACE_PACKET=true on a known-good remote (i.e. GitHub), that the null packet-line `0000` has to follow the service line. This is not reflected in the example here:

https://github.com/git/git/blob/6464679d9620d91b639e2681b9cc6473f3856d09/Documentation/technical/http-protocol.txt#L216

It is also not reflected in the BNF:

https://github.com/git/git/blob/6464679d9620d91b639e2681b9cc6473f3856d09/Documentation/technical/http-protocol.txt#L279

(Note these links are from the most recent commit of this file as of this writing.)

Just thought somebody would like to know.

Regards,

--
Dorian Taylor
Make things. Make sense.
https://doriantaylor.com

Re: bug in HTTP protocol spec

[Jeff King]

On Wed, Feb 21, 2018 at 10:29:35AM -0800, Dorian Taylor wrote:

> I didn’t get an insight until I ran with GIT_TRACE_PACKET=true on a known-good remote (i.e. GitHub), that the null packet-line `0000` has to follow the service line. This is not reflected in the example here:
> 
> https://github.com/git/git/blob/6464679d9620d91b639e2681b9cc6473f3856d09/Documentation/technical/http-protocol.txt#L216

This is missing the trailing flush after the ref advertisement, too.

> It is also not reflected in the BNF:
> 
> https://github.com/git/git/blob/6464679d9620d91b639e2681b9cc6473f3856d09/Documentation/technical/http-protocol.txt#L279
> 
> (Note these links are from the most recent commit of this file as of this writing.)
> 
> Just thought somebody would like to know.

Thanks, I agree the document is buggy. Do you want to submit a patch?

-Peff

Re: bug in HTTP protocol spec

[Dorian Taylor]

> On Feb 21, 2018, at 2:15 PM, Jeff King <peff@peff.net> wrote:
> 
> Thanks, I agree the document is buggy. Do you want to submit a patch?

Will this do?

Note I am not sure what the story is behind that `version 1` element, whether it's supposed to go before or after the null packet or if there should be another null packet or what. Perhaps somebody more fluent with the smart protocol can advise.

---
Documentation/technical/http-protocol.txt | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/http-protocol.txt b/Documentation/technical/http-protocol.txt
index a0e45f2889e6e..19d73f7efb338 100644
--- a/Documentation/technical/http-protocol.txt
+++ b/Documentation/technical/http-protocol.txt
@@ -214,14 +214,17 @@ smart server reply:
   S: Cache-Control: no-cache
   S:
   S: 001e# service=git-upload-pack\n
+   S: 0000
   S: 004895dcfa3633004da0049d3d0fa03f80589cbcaf31 refs/heads/maint\0multi_ack\n
   S: 0042d049f6c27a2244e12041955e262a404c7faba355 refs/heads/master\n
   S: 003c2cb58b79488a98d2721cea644875a8dd0026b115 refs/tags/v1.0\n
   S: 003fa3c2e2402b99163d1d59756e5f207ae21cccba4c refs/tags/v1.0^{}\n
+   S: 0000

The client may send Extra Parameters (see
Documentation/technical/pack-protocol.txt) as a colon-separated string
-in the Git-Protocol HTTP header.
+in the Git-Protocol HTTP header. Note as well that there is *no* newline
+after the `0000`.

Dumb Server Response
^^^^^^^^^^^^^^^^^^^^
@@ -264,8 +267,8 @@ Servers MUST set $servicename to be the request parameter value.
Servers SHOULD include an LF at the end of this line.
Clients MUST ignore an LF at the end of the line.

-Servers MUST terminate the response with the magic `0000` end
-pkt-line marker.
+Servers MUST follow the first pkt-line, as well as terminate the
+response, with the magic `0000` end pkt-line marker.

The returned response is a pkt-line stream describing each ref and
its known value.  The stream SHOULD be sorted by name according to
@@ -278,6 +281,7 @@ Extra Parameter.

  smart_reply     =  PKT-LINE("# service=$servicename" LF)
		     *1("version 1")
+		     "0000"
		     ref_list
		     "0000"
  ref_list        =  empty_list / non_empty_list

--
Dorian Taylor
Make things. Make sense.
https://doriantaylor.com

Re: bug in HTTP protocol spec

[Jonathan Nieder]

(+cc: bmwill@, who is working on protocol v2)
Hi,

Dorian Taylor wrote:
> On Feb 21, 2018, at 2:15 PM, Jeff King <peff@peff.net> wrote:

>> Thanks, I agree the document is buggy. Do you want to submit a patch?
>
> Will this do?

Thanks for writing it.

Do you mind if we forge your sign-off?  (See Documentation/SubmittingPatches
item '(5) Certify your work' for details about what this means.)

> Note I am not sure what the story is behind that `version 1`
> element, whether it's supposed to go before or after the null packet
> or if there should be another null packet or what. Perhaps somebody
> more fluent with the smart protocol can advise.

I believe the 'version 1' goes after the flush-packet.

Thanks,
Jonathan

Re: bug in HTTP protocol spec

[Dorian Taylor]

[-- Attachment #1: Type: text/plain, Size: 2774 bytes --]


> On Feb 21, 2018, at 9:37 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> 
> Thanks for writing it.
> 
> Do you mind if we forge your sign-off?  (See Documentation/SubmittingPatches
> item '(5) Certify your work' for details about what this means.)

Sure, or I can just re-paste:

Signed-off-by: Dorian Taylor <dorian.taylor.lists@gmail.com>

---
Documentation/technical/http-protocol.txt | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/http-protocol.txt b/Documentation/technical/http-protocol.txt
index a0e45f2889e6e..19d73f7efb338 100644
--- a/Documentation/technical/http-protocol.txt
+++ b/Documentation/technical/http-protocol.txt
@@ -214,14 +214,17 @@ smart server reply:
  S: Cache-Control: no-cache
  S:
  S: 001e# service=git-upload-pack\n
+   S: 0000
  S: 004895dcfa3633004da0049d3d0fa03f80589cbcaf31 refs/heads/maint\0multi_ack\n
  S: 0042d049f6c27a2244e12041955e262a404c7faba355 refs/heads/master\n
  S: 003c2cb58b79488a98d2721cea644875a8dd0026b115 refs/tags/v1.0\n
  S: 003fa3c2e2402b99163d1d59756e5f207ae21cccba4c refs/tags/v1.0^{}\n
+   S: 0000

The client may send Extra Parameters (see
Documentation/technical/pack-protocol.txt) as a colon-separated string
-in the Git-Protocol HTTP header.
+in the Git-Protocol HTTP header. Note as well that there is *no* newline
+after the `0000`.

Dumb Server Response
^^^^^^^^^^^^^^^^^^^^
@@ -264,8 +267,8 @@ Servers MUST set $servicename to be the request parameter value.
Servers SHOULD include an LF at the end of this line.
Clients MUST ignore an LF at the end of the line.

-Servers MUST terminate the response with the magic `0000` end
-pkt-line marker.
+Servers MUST follow the first pkt-line, as well as terminate the
+response, with the magic `0000` end pkt-line marker.

The returned response is a pkt-line stream describing each ref and
its known value.  The stream SHOULD be sorted by name according to
@@ -278,6 +281,7 @@ Extra Parameter.

 smart_reply     =  PKT-LINE("# service=$servicename" LF)
		     *1("version 1")
+		     "0000"
		     ref_list
		     "0000"
 ref_list        =  empty_list / non_empty_list

---

> 
>> Note I am not sure what the story is behind that `version 1`
>> element, whether it's supposed to go before or after the null packet
>> or if there should be another null packet or what. Perhaps somebody
>> more fluent with the smart protocol can advise.
> 
> I believe the 'version 1' goes after the flush-packet.

I took a traipse through the code and couldn’t determine it one way or another, but my money is on that looking something like `000aversion 1\n` on the wire.

--
Dorian Taylor
Make things. Make sense.
https://doriantaylor.com


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 842 bytes --]

Re: bug in HTTP protocol spec

[Jeff King]

On Wed, Feb 21, 2018 at 11:23:52PM -0800, Dorian Taylor wrote:

> diff --git a/Documentation/technical/http-protocol.txt b/Documentation/technical/http-protocol.txt
> index a0e45f2889e6e..19d73f7efb338 100644
> --- a/Documentation/technical/http-protocol.txt
> +++ b/Documentation/technical/http-protocol.txt
> @@ -214,14 +214,17 @@ smart server reply:
>   S: Cache-Control: no-cache
>   S:
>   S: 001e# service=git-upload-pack\n
> +   S: 0000
>   S: 004895dcfa3633004da0049d3d0fa03f80589cbcaf31 refs/heads/maint\0multi_ack\n
>   S: 0042d049f6c27a2244e12041955e262a404c7faba355 refs/heads/master\n
>   S: 003c2cb58b79488a98d2721cea644875a8dd0026b115 refs/tags/v1.0\n
>   S: 003fa3c2e2402b99163d1d59756e5f207ae21cccba4c refs/tags/v1.0^{}\n
> +   S: 0000

This indentation is funny. But I suspect it is because your whole patch
seems to have been whitespace-damaged (see the section on gmail in
"git help git-format-patch").

> The client may send Extra Parameters (see
> Documentation/technical/pack-protocol.txt) as a colon-separated string
> -in the Git-Protocol HTTP header.
> +in the Git-Protocol HTTP header. Note as well that there is *no* newline
> +after the `0000`.

I guess I'm not opposed to calling that out, but this is normal for
pktline (the flush packet has no data; in the other lines the newline is
not a syntactic part of the pktline stream, but is actually data
contained inside each of those pktlines).

> Dumb Server Response
> ^^^^^^^^^^^^^^^^^^^^
> @@ -264,8 +267,8 @@ Servers MUST set $servicename to be the request parameter value.
> Servers SHOULD include an LF at the end of this line.
> Clients MUST ignore an LF at the end of the line.
> 
> -Servers MUST terminate the response with the magic `0000` end
> -pkt-line marker.
> +Servers MUST follow the first pkt-line, as well as terminate the
> +response, with the magic `0000` end pkt-line marker.

In theory there can actually be one or more headers after the "service"
line. But I don't think they've ever been used (and the current client
just throws them away).

> The returned response is a pkt-line stream describing each ref and
> its known value.  The stream SHOULD be sorted by name according to
> @@ -278,6 +281,7 @@ Extra Parameter.
> 
>  smart_reply     =  PKT-LINE("# service=$servicename" LF)
> 		     *1("version 1")
> +		     "0000"
> 		     ref_list
> 		     "0000"

I think Jonathan is right that the version must go after the flush
packet (just looking at the v2 protocol patches elsewhere on the list,
the version tag is really part of the ref_list).

Not related to your patch, but I suspect it should also be
PKT-LINE("version 1").

-Peff

Re: bug in HTTP protocol spec

[Dorian Taylor]

[-- Attachment #1: Type: text/plain, Size: 509 bytes --]


> On Feb 22, 2018, at 2:08 AM, Jeff King <peff@peff.net> wrote:
> 
> 
> This indentation is funny. But I suspect it is because your whole patch
> seems to have been whitespace-damaged (see the section on gmail in
> "git help git-format-patch").

That is, bit-for-bit, what came out of that submitGit thing that is advertised when you try to open a pull request on the git repository on Github. I was a bit surprised myself.

--
Dorian Taylor
Make things. Make sense.
https://doriantaylor.com


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 842 bytes --]

Re: bug in HTTP protocol spec

[Brandon Williams]

On 02/21, Dorian Taylor wrote:
> 
> > On Feb 21, 2018, at 9:37 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> > 
> > Thanks for writing it.
> > 
> > Do you mind if we forge your sign-off?  (See Documentation/SubmittingPatches
> > item '(5) Certify your work' for details about what this means.)
> 
> Sure, or I can just re-paste:
> 
> Signed-off-by: Dorian Taylor <dorian.taylor.lists@gmail.com>
> 
> ---
> Documentation/technical/http-protocol.txt | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/technical/http-protocol.txt b/Documentation/technical/http-protocol.txt
> index a0e45f2889e6e..19d73f7efb338 100644
> --- a/Documentation/technical/http-protocol.txt
> +++ b/Documentation/technical/http-protocol.txt
> @@ -214,14 +214,17 @@ smart server reply:
>   S: Cache-Control: no-cache
>   S:
>   S: 001e# service=git-upload-pack\n
> +   S: 0000
>   S: 004895dcfa3633004da0049d3d0fa03f80589cbcaf31 refs/heads/maint\0multi_ack\n
>   S: 0042d049f6c27a2244e12041955e262a404c7faba355 refs/heads/master\n
>   S: 003c2cb58b79488a98d2721cea644875a8dd0026b115 refs/tags/v1.0\n
>   S: 003fa3c2e2402b99163d1d59756e5f207ae21cccba4c refs/tags/v1.0^{}\n
> +   S: 0000
> 
> The client may send Extra Parameters (see
> Documentation/technical/pack-protocol.txt) as a colon-separated string
> -in the Git-Protocol HTTP header.
> +in the Git-Protocol HTTP header. Note as well that there is *no* newline
> +after the `0000`.
> 
> Dumb Server Response
> ^^^^^^^^^^^^^^^^^^^^
> @@ -264,8 +267,8 @@ Servers MUST set $servicename to be the request parameter value.
> Servers SHOULD include an LF at the end of this line.
> Clients MUST ignore an LF at the end of the line.
> 
> -Servers MUST terminate the response with the magic `0000` end
> -pkt-line marker.
> +Servers MUST follow the first pkt-line, as well as terminate the
> +response, with the magic `0000` end pkt-line marker.
> 
> The returned response is a pkt-line stream describing each ref and
> its known value.  The stream SHOULD be sorted by name according to
> @@ -278,6 +281,7 @@ Extra Parameter.
> 
>  smart_reply     =  PKT-LINE("# service=$servicename" LF)
> 		     *1("version 1")
> +		     "0000"
> 		     ref_list
> 		     "0000"
>  ref_list        =  empty_list / non_empty_list
> 
> ---
> 
> > 
> >> Note I am not sure what the story is behind that `version 1`
> >> element, whether it's supposed to go before or after the null packet
> >> or if there should be another null packet or what. Perhaps somebody
> >> more fluent with the smart protocol can advise.
> > 
> > I believe the 'version 1' goes after the flush-packet.
> 
> I took a traipse through the code and couldn’t determine it one way or another, but my money is on that looking something like `000aversion 1\n` on the wire.

Yes the version string goes along with the ref_list in v1 like so:

  # service=<service>
  0000
  version 1
  ref_list
  0000

This is because it is part of the payload which is actually delivered to
the git fetch/push binary where as the "# service" bit is used by the
remote helper to identify smart vs not smart servers.

> 
> --
> Dorian Taylor
> Make things. Make sense.
> https://doriantaylor.com
> 



-- 
Brandon Williams

Re: bug in HTTP protocol spec

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This indentation is funny. But I suspect it is because your whole patch
> seems to have been whitespace-damaged (see the section on gmail in
> "git help git-format-patch").

I saw somewhere "Apple-Mail" and a phrase "repaste".  So perhaps
copy&paste on the client is involved in the whitespace damage (of
course the original could be broken, but I somehow doubt it).

>> The client may send Extra Parameters (see
>> Documentation/technical/pack-protocol.txt) as a colon-separated string
>> -in the Git-Protocol HTTP header.
>> +in the Git-Protocol HTTP header. Note as well that there is *no* newline
>> +after the `0000`.
>
> I guess I'm not opposed to calling that out, but this is normal for
> pktline (the flush packet has no data; in the other lines the newline is
> not a syntactic part of the pktline stream, but is actually data
> contained inside each of those pktlines).

For what it's worth, I am slightly negative on this addition.  It
makes it inconsistent if we only mention it about _this_ flush and
not any other flush.  It even gets in the way of learning the
protocol by new people reading it, by giving an impression that
somehow LF is outside and in between packet line.

Thanks.

Re: bug in HTTP protocol spec

[Dorian Taylor]

[-- Attachment #1: Type: text/plain, Size: 1243 bytes --]


> On Feb 22, 2018, at 12:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> I saw somewhere "Apple-Mail" and a phrase "repaste".  So perhaps
> copy&paste on the client is involved in the whitespace damage (of
> course the original could be broken, but I somehow doubt it).

https://doriantaylor.com/file/well-ill-be-damned.png

> For what it's worth, I am slightly negative on this addition.  It
> makes it inconsistent if we only mention it about _this_ flush and
> not any other flush.  It even gets in the way of learning the
> protocol by new people reading it, by giving an impression that
> somehow LF is outside and in between packet line.
> 
> Thanks.

This patch exists because I was asked to write it. I don’t know squat about this protocol other than the fact that I followed the spec and it didn’t work. I traced a known-good protocol endpoint and found it contained content that didn’t agree with the spec. I then obliged the request to submit a patch with *what I knew to be true* about the sample that actually worked. I then followed the recommendations *advertised on GitHub* for submitting the patch.

You’re welcome.

--
Dorian Taylor
Make things. Make sense.
https://doriantaylor.com


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 842 bytes --]

[PATCH] smart-http: document flush after "# service" line

[Jeff King]

On Thu, Feb 22, 2018 at 12:12:54PM -0800, Dorian Taylor wrote:

> This patch exists because I was asked to write it. I don’t know squat
> about this protocol other than the fact that I followed the spec and
> it didn’t work. I traced a known-good protocol endpoint and found it
> contained content that didn’t agree with the spec. I then obliged the
> request to submit a patch with *what I knew to be true* about the
> sample that actually worked. I then followed the recommendations
> *advertised on GitHub* for submitting the patch.

I take it that a revised patch is not forthcoming, then. :-/

Let's wrap up this topic with this, then, which adds a commit message
and fixes the flush/version-1 ordering issue.

-- >8 --
Subject: smart-http: document flush after "# service" line

The http-protocol.txt spec fails to mention that a flush
packet comes in the smart server response after sending any
the "service" header.

Technically the client code is actually ready to receive an
arbitrary number of headers here, but since we haven't
introduced any other headers in the past decade (and the
client would just throw them away), let's not mention it in
the spec.

This fixes both BNF and the example. While we're fixing the
latter, let's also add the missing flush after the ref list.

Reported-by: Dorian Taylor <dorian.taylor.lists@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/technical/http-protocol.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/technical/http-protocol.txt b/Documentation/technical/http-protocol.txt
index a0e45f2889..64f49d0bbb 100644
--- a/Documentation/technical/http-protocol.txt
+++ b/Documentation/technical/http-protocol.txt
@@ -214,10 +214,12 @@ smart server reply:
    S: Cache-Control: no-cache
    S:
    S: 001e# service=git-upload-pack\n
+   S: 0000
    S: 004895dcfa3633004da0049d3d0fa03f80589cbcaf31 refs/heads/maint\0multi_ack\n
    S: 0042d049f6c27a2244e12041955e262a404c7faba355 refs/heads/master\n
    S: 003c2cb58b79488a98d2721cea644875a8dd0026b115 refs/tags/v1.0\n
    S: 003fa3c2e2402b99163d1d59756e5f207ae21cccba4c refs/tags/v1.0^{}\n
+   S: 0000
 
 The client may send Extra Parameters (see
 Documentation/technical/pack-protocol.txt) as a colon-separated string
@@ -277,6 +279,7 @@ The returned response contains "version 1" if "version=1" was sent as an
 Extra Parameter.
 
   smart_reply     =  PKT-LINE("# service=$servicename" LF)
+		     "0000"
 		     *1("version 1")
 		     ref_list
 		     "0000"
-- 
2.16.2.708.g0b2ed7f536

Re: [PATCH] smart-http: document flush after "# service" line

[Eric Sunshine]

On Sat, Mar 3, 2018 at 12:27 AM, Jeff King <peff@peff.net> wrote:
> Subject: smart-http: document flush after "# service" line
>
> The http-protocol.txt spec fails to mention that a flush
> packet comes in the smart server response after sending any
> the "service" header.

"any the"?

> Technically the client code is actually ready to receive an
> arbitrary number of headers here, but since we haven't
> introduced any other headers in the past decade (and the
> client would just throw them away), let's not mention it in
> the spec.
>
> This fixes both BNF and the example. While we're fixing the
> latter, let's also add the missing flush after the ref list.
>
> Reported-by: Dorian Taylor <dorian.taylor.lists@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>

Re: [PATCH] smart-http: document flush after "# service" line

[Jeff King]

On Sat, Mar 03, 2018 at 03:28:47AM -0500, Eric Sunshine wrote:

> On Sat, Mar 3, 2018 at 12:27 AM, Jeff King <peff@peff.net> wrote:
> > Subject: smart-http: document flush after "# service" line
> >
> > The http-protocol.txt spec fails to mention that a flush
> > packet comes in the smart server response after sending any
> > the "service" header.
> 
> "any the"?

Oops, should just be "the".

-Peff