Usman Akinyemi <usmanakinyemi202@xxxxxxxxx> writes: > As some issues that can happen with a Git client can be operating system > specific, it can be useful for a server to know which OS a client is > using. In the same way it can be useful for a client to know which OS > a server is using. > > Our current agent capability is in the form of "package/version" (e.g., > "git/1.8.3.1"). Let's extend it to include the operating system name (os) > i.e in the form "package/version os" (e.g., "git/1.8.3.1 Linux"). Shouldn't this be "git/1.8.3.1-Linux" or something to avoid SP? The capability list in protocol v1 is on a single line that is whitespace separated (cf. connect.c:parse_feature_value()) without any escape mechanism. Side note. Does it pose a security hole, when we can set agent to any value? I do not think so, as it controls what this end sends to the other. If you are attacker in control of your own agent string to be sent to the other end, and use a string with a whitespace in it after "agent=" to claim that you support a capability you actually don't, that is not a new way to attack the other side available to you---you can write your own Git client to talk to the other side to send such a bogus capablity list anyway. > diff --git a/Documentation/gitprotocol-v2.txt b/Documentation/gitprotocol-v2.txt > index 1652fef3ae..f4831a8787 100644 > --- a/Documentation/gitprotocol-v2.txt > +++ b/Documentation/gitprotocol-v2.txt > @@ -184,11 +184,14 @@ form `agent=X`) to notify the client that the server is running version > the `agent` capability with a value `Y` (in the form `agent=Y`) in its > request to the server (but it MUST NOT do so if the server did not > advertise the agent capability). The `X` and `Y` strings may contain any > -printable ASCII characters except space (i.e., the byte range 32 < x < > -127), and are typically of the form "package/version" (e.g., > -"git/1.8.3.1"). The agent strings are purely informative for statistics > -and debugging purposes, and MUST NOT be used to programmatically assume > -the presence or absence of particular features. > +printable ASCII characters (i.e., the byte range 31 < x < 127), and are Patches 1 & 2 redacted non-printables and SP separately, because SP is considered printable. With this change you are allowing SP to be passed without getting redacted? I do not think it is a good idea (see above). While I'd prefer to keep the range the same as before, i.e. "any printable ASCII characters except space", "33 <= x <= 126" may be more readily recognisable that we are doing something unusual, as "32 <= x <= 126" is fairly easily recognisable as "ASCII printable". > +typically of the form "package/version os" (e.g., "git/1.8.3.1 Linux") So, I'd suggest using something other than " " between "version" and "os". Dot (as if the byte there were redacted) or slash or dash or whatever, anything that is not whitespace. > +where `os` is the operating system name (e.g., "Linux"). `X` and `Y` can > +be configured using the GIT_USER_AGENT environment variable and it takes > +priority. The `os` is retrieved using the 'sysname' field of the `uname(2)` > +system call or its equivalent. The agent strings are purely informative for > +statistics and debugging purposes, and MUST NOT be used to programmatically > +assume the presence or absence of particular features. Other than these nits, I find the above very well done. As to the additional implementation of git_user_agent_sanitized(), except for that same "do we really want SP there?" question, I see nothing questionable there, either. Overall very nicely done and presented. Thanks.
