On Dec 15, 2007 10:08 AM, Ævar Arnfjörð Bjarmason <avar@xxxxxxxx> wrote: > > Looks good. I'll queue only so that I won't lose it and wait for Acks > > from Mart[iy]ns. Please sign off your patch. I have been mulling a bit about this change. Seems correct from a code flow POV. But anon pserver support was added with a lot of trepidation. Authenticated, write-enabled pserver support fills me with dread ;-) A few things that come to mind - git/config is very likely to be readable if the site is served via other means, like dumb http protocol, or git+ssh. So even if the password scrambling is mickey-mouse. it might make sense to force the password data to live elsewhere. - umasks/file permissions will probably be handled by the underlying git tools and core.sharedrepository, since we no longer update refs "manually". So after a bit of thinking, this should not be an issue. - I still worry about running cvsserver with an acct that has write privileges. With anon, all we need write access is the sqlite db, and that's not even based on user input. And it can be relocated elsewhere via git/config. So, wondering about input validation and related matters re-read most of cvsserver. The only suspicious bit I find is a caller to transmitfile @ lines 1117, where the 2nd parameter doesn't come from tempfile() - if a nasty filename sneaked in, we could (perhaps) be in for a surprise. We do that for diff header prettyness - we could use --label / -L instead. Will try and prep a patch for that tomorrow if noone beats me to it (bedtime in nz!). Hmmm. Does "worried ACK" count as an ACK? Can we add big fat warnings along the lines of "run this in a locked down environment, pretty please"? cheers, m - To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
