Re: [PATCH v2 2/3] credentials: make line reading Windows compatible

From: Jeff King <peff@peff.net>
Date: Sep 29, 2020, 00:30 UTC
Message-ID: <20200929003000.GA898702@coredump.intra.peff.net>
In-Reply-To: <CAPUEspgW9CFO3WtbiuTUsmXp05fPqr2Cs81piDJFJ0g3KcTy3A@mail.gmail.com>

On Mon, Sep 28, 2020 at 04:26:38PM -0700, Carlo Arenas wrote:

Show 15 quoted lines

> > -test_expect_success 'get: credentials with DOS line endings are invalid if path is relevant' '
> > +test_expect_success 'get: credentials with DOS line endings are valid if path is relevant' '
> 
> note that this test was put in place to protect users from regressions
> like the one we got after the release of 2.26.1 where users that had
> '\r' as part of their credentials were getting an error[1]
> 
> while I am sympathetic to the change (indeed I proposed something
> similar, but was reminded by Peff that while it looks like a text file
> it was designed to be considered opaque and therefore should use UNIX
> LF as record terminator by specification), I am concerned this could
> result in a similar regression since we know they are still users out
> there that had modified this file manually (something that was not
> recommended) and are currently relying on the fact that these lines
> are invalid and therefore silently ignored.

Going over that old thread, I'm not sure that anybody raised a real use case where somebody expected a CR at the end of a line. The discussion was mostly about whether proposed changes would or would not be compatible with existing behavior.

I think that:

  - we'd never write a raw CR ourselves, as we'd urlencode the character

  - if somebody did put in a raw CR manually like:

      https://example.com\r\n

    then we'd currently fail to match "example.com". Which is probably
    not what they wanted. I suspect that \r in a hostname is bogus
    anyway (certainly curl will complain about it).

  - you could do the same in a path:

      https://example.com/foo\r\n

    which _is_ legal, but I think we'd generally ignore it completely
    unless credential.usehttppath is set (for network-accessible
    requests, that is; you could probably point your local cert file at
    something bogus, but I think the main areas of interest here are
    people manipulating the credential protocol via malicious urls).

So I'm OK loosening the format in the name of convenience, as long as we're confident that it's not opening up any security problems. I can't think of any such problems, though.

It sounds from your email like you may have found me arguing the opposite. That's entirely possible. ;) But I couldn't find it in the thread Junio linked.

-Peff