On Sat, Feb 15, 2025 at 3:37 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Usman Akinyemi <usmanakinyemi202@xxxxxxxxx> writes: Hi Junio, > > > 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. Yeah, I almost missed this function. Thanks for pointing it out. > > 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. Thanks for this explanation. > > > 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. Yeah, Noted. Thanks. > > > +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. Thank you. > > Thanks.
