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. -Peff