...maybe this is an actual bug in Phabricator where some pathways are just missing the "sudo" wrapper?

I think the story here is a bit complicated. Phabricator may be configured to sudo to a different user (phd.user) when writing to repositories, but currently does not sudo in any cases when reading from repositories. In particular, Conduit queries like diffusion.browsequery do not sudo.

I don't recall the entire history here, but I think this likely supports simple mirror-only use cases, i.e. lets you get Phabricator working and doing something useful without having to touch sudoers configuration. I don't want to casually break this, although the current behavior is a bit informal and probably "less correct" than the rule "always sudo" would be.

Either fix is probably still on the table:

• Always Sudo: We never sudo by default, and if administrators have configured phd.user that implies that sudo works, and sudo will be used for writes. So any non-broken configuration shouldn't be broken by always using sudo.
• Mark Directory Safe: DiffusionGitCommandEngine->newFormattedCommand(...) can add --config with a bit of tweaking: it currently doesn't know which directory it will be executed in, but this information is knowable.

There's perhaps an argument that not sudoing for reads is a security feature (i.e., that bypassing this Git check is actually more secure than adding more sudo), since it means more of the attack surface (pathways which can execute git) runs as a user which (maybe) can't write to the repositories. But this is a big maybe (there's no guarantee the web user is actually any less privileged than the daemon user) and kind of way off in hypothetical land anyway.

I think I'm just going to pursue "Always Sudo" since that generally reduces the amount of special behavior we have and I'm reasonably sure it won't break any not-already-broken configurations.

Just for visibility, the error messages you'll see if you're affected by this issue look something like this:

ERR-CONDUIT-CORE: <diffusion.browsequery> Command failed with error #128! COMMAND git ls-tree -z -l ... --
STDOUT (empty)
STDERR fatal: unsafe repository ('/path/to/some/repo' is owned by someone else) To add an exception for this directory, call: git config --global --add safe.directory ...

D21756 effectively makes all Git pathways call setSudoAsDaemon(true).

I narrowly deployed this patch to an affected Phacility production shard and saw everything start working again. I believe this patch breaks no configurations which were not already broken (configured with a phd.user which the web user can't actually sudo to) and is likely the least-bad approach of available approaches -- notably, it represents an overall reduction in the amount of special-cased behavior. The only downside is a purely theoretical loss of web vs daemon privilege separation which I suspect no real instance is actually impacted by.

(If I turn out to be wrong about this, I believe I could back this out and pursue a not-much-more-complicated --config safe.directory patch instead.)

Since this seems to be working in practice and I've convinced myself it's reasonable, I'm planning to land this change, cut a release, and deploy it. (However, the baby is about to wake up -- so I may not get this rolling until at least the next time she goes down for a nap.)

Just for visibility, this is I believe the change that broke Diffusion (which was fixed in rP52df4ff515b7), where the error message is something like

Unable to Retrieve Paths
Command failed with error #128! COMMAND /usr/bin/sudo -E -n -u phab-daemon -- git config -l -f /tmp/exjghzd2cx4ok04w/60402-8Yau8N STDOUT (empty) STDERR fatal: unable to read config file '/tmp/exjghzd2cx4ok04w/60402-8Yau8N': Permission denied