On Wed, May 31, 2017 at 6:50 AM, Jeff King <peff@xxxxxxxx> wrote: > On Fri, May 26, 2017 at 11:22:37AM -0500, Elliott Cable wrote: > >> Hi! Thanks for the responses (I hope reply-all isn't bad mailing-list >> etiquette? Feel free to yell at with a direct reply!). For whatever it's >> worth, as a random user, here's my thoughts: > > No, reply-all is the preferred method on this list. > >> > The other approach is to declare that a url rewrite resets the >> > protocol-from-user flag to 1. IOW, since the "persistent-https" protocol >> > comes from our local config, it's not dangerous and we should behave as >> > if the user themselves gave it to us. That makes Elliott's case work out >> > of the box. >> >> Well, now that I'm aware of security concerns, `GIT_PROTOCOL_FROM_USER` >> and `GIT_ALLOW_PROTOCOL`, and so on, I wouldn't *at all* expect >> `insteadOf` to disable that behaviour. Instead, one of two things seems >> like a more ideal solution: >> >> 1. Most simply, better documentation: mention `GIT_PROTOCOL_FROM_USER` >> explicitly in the documentation of/near `insteadOf`, most >> particularly in the README for `contrib/persistent-https`. >> >> 2. Possibly, special-case “higher-security” porcelain (like >> `git-submodule`, as described in 33cfccbbf3) to ignore `insteadOf` >> rewrite-rules without additional, special configuration. This way, >> `git-submodule` works for ignorant users (like me) out of the box, >> just as it previously did, and there's no possible security >> compramise. > > After my other email, I was all set to write a patch to set > "from_user=1" when we rewrite a URL. But I think it actually is a bit > risky, because we don't know which parts of the URL are > security-sensitive versus which parts were rewritten. A modification of > a tainted string doesn't necessarily untaint it (but sometimes it does, > as in your case). > > We could actually have a flag as part of the rewrite config, like: > > [url "persistent-https"] > insteadOf = "https" > untaint = true > > but I don't think that really buys anything. If you know about the > problem, you could just as easily do: > > [url "persistent-https"] > insteadOf = "https" > [protocol "persistent-https"] > allow = always > > It really is an issue of the user knowing about the problem (and how to > solve it), and I don't think we can get around that securely. So better > documentation probably is the right solution. > > I'll see if I can cook something up. I was going to say: A way to have our cake & eat it too here would be to just support *.insteadOfRegex, i.e. "url.persistent-https://.insteadOfRegex="^https://";. But in this case, even if we can just do un-anchored string replacement, isn't a way around this just to do "url.persistent-https://.insteadOf=https://"; & untaint & document that you should do that? Although that would potentially leave people who have existing protocol rewrites without :// open to rewrites they didn't intend for submodules...