This path was a submodule for all directories, never used submodules

Bug
#1

Reproduction Instructions

  1. Point browser to https://<host>/diffusion/
  2. Click on a repo to be taken to the “Code” view of that repo listing all of the files and directories in that repo.
  3. Click on any directory. (browser path looks like https://<host>/source/<repo>/browse/master/<directory>/)

Actual:
This path was a submodule at... error message is displayed with very little other output.

Expected:
Web UI should show all of the files and directories within this directory.

Context:
This is happening for all of the directories in all of the repos I have tried. I have never used submodules with this phabricator install. I also hand-modified the URL to access deeper directories and got the same result. This is a common workflow for me, and has broken recently.

Phabricator/Arcanist Version

arcanist:
commit b2e715fc5a9ccd36048ae469850f293b0780403a
Author: epriestley git@epriestley.com
Date: Tue Jan 12 10:33:47 2021 -0800

phabricator:
commit 9cbbbe2a8712929b18344608826a1b4a17ff9bab
Author: epriestley git@epriestley.com
Date: Wed Jan 27 15:14:00 2021 -0800

Other Version Information
|php|5.6.40|apache2handler|
|diff|3.3|/usr/bin/diff|
|git|1.8.3.1|/usr/bin/git|
|hg|Not Available||
|pygmentize|Not Available||
|svn|1.7.14|/usr/bin/svn|

#2

Hi @mattfaus,

Using your version of git (1.8.3), what do these commands output:

  1. git cat-file -t -- <commit-hash>:<valid-path-of-directory>
  2. git ls-tree <commit-hash> -- <valid-path-of-directory>

?

#3 
$ git cat-file -t -- ff410bd6545ff4a7c3505852885029f3f1371c7a:/home/user/tmp/testrepo/dir1
usage: git cat-file (-t|-s|-e|-p|<type>|--textconv) <object>
   or: git cat-file (--batch|--batch-check) < <list_of_objects>

<type> can be one of: blob, tree, commit, tag
    -t                    show object type
    -s                    show object size
    -e                    exit with zero when there's no error
    -p                    pretty-print object's content
    --textconv            for blob objects, run textconv on object's content
    --batch               show info and content of objects fed from the standard input
    --batch-check         show info about objects fed from the standard input

$ git ls-tree ff410bd6545ff4a7c3505852885029f3f1371c7a -- /home/user/tmp/testrepo/dir1
040000 tree 3c712d1e31234f269a07845582c126d863e29024	dir1

Here are some other things I tried:

$ git cat-file -t ff410bd6545ff4a7c3505852885029f3f1371c7a:/home/user/tmp/testrepo/dir1/
fatal: Not a valid object name ff410bd6545ff4a7c3505852885029f3f1371c7a:/home/ec2-user/tmp/testrepo/dir1/

$ git cat-file -t ff410bd6545ff4a7c3505852885029f3f1371c7a:dir1
tree

$ git cat-file -t ff410bd6545ff4a7c3505852885029f3f1371c7a:dir1/
tree

$ git ls-tree ff410bd6545ff4a7c3505852885029f3f1371c7a -- dir1
040000 tree 3c712d1e31234f269a07845582c126d863e29024	dir1

$ git ls-tree ff410bd6545ff4a7c3505852885029f3f1371c7a -- dir1/
100644 blob b023018cabc396e7692c70bbf5784a93d3f738ab	dir1/hi1.txt
#4

I think @avivey is on the right track and that this is some kind of version issue – how about these commands?

$ git cat-file -t -- ff410bd6545ff4a7c3505852885029f3f1371c7a:dir1/
$ echo $?

The expected sequence internally is something like this, if the URI is .../browse/master/src/:

$ git cat-file -t -- master:src/
# Expect "tree" output, with 0 return code.
$ git ls-tree -z -l master -- src/
# Expect a \0-delimited list of trees/blobs.

We should only be able to reach the error you describe if this happens:

$ git cat-file -t -- master:src/
# This exits nonzero.
$ git ls-tree master -- src/
# This exits 0, with output flagged with the 0x160000 mode.

(If you run out of patience for hunting this down and updating Git is straightforward, it is likely to fix things – Git 1.8.3.1 was released nearly 8 years ago.)

#5

Ok, I updated git to 2.24 and the issue seems resolved.

It seems like the problem was that the older git cat-file did not understand the -- argument, but the new version does. Just guessing here, but maybe you could keep backwards compat by removing the --?

$ git --version
git version 2.24.3

$ git cat-file -t ff410bd6545ff4a7c3505852885029f3f1371c7a:dir1/
tree

$ git cat-file -t -- ff410bd6545ff4a7c3505852885029f3f1371c7a:dir1/
tree

From my scrollback history (i.e. git 1.x), you can see the git cat-file failing when the -- argument is present (and my previous reply shows it succeeding when it is omitted. I didn’t capture the $?, but I believe it was 129.

$ git cat-file -t -- ff410bd6545ff4a7c3505852885029f3f1371c7a:dir1
usage: git cat-file (-t|-s|-e|-p|<type>|--textconv) <object>
   or: git cat-file (--batch|--batch-check) < <list_of_objects>

<type> can be one of: blob, tree, commit, tag
    -t                    show object type
    -s                    show object size
    -e                    exit with zero when there's no error
    -p                    pretty-print object's content
    --textconv            for blob objects, run textconv on object's content
    --batch               show info and content of objects fed from the standard input
    --batch-check         show info about objects fed from the standard input
#6

This is dangerous in the general case where an attacker may provide a branch name like --delete-everything-forever=, which is why this was changed in the first place. See also https://secure.phabricator.com/T13589 and https://secure.phabricator.com/T13491.

Although other protections (including gitsprintf(), introduced alongside this change) also defuse this class of attack, I don’t want to unconditionally remove the “--” because it improves safety and correctness in versions of Git which support it.

Possible approaches might be: remove the -- in older versions of Git; or bump the minimum required version to whatever version of Git added support for --, since it’s likely 6-7 years old.

#7

FYI: We have updated Phabricator a few days ago and did get the same error. We are still using a Debian 8 (Jessie). While that is not ideal, the git version bundled with Jessie is v2.1.4. So this version too is not enough. We have installed a backport of v2.11.0 for Jessie. That works fine. Here and here are some pointers for others with an old debian.