Re: [PATCH v4] setup: allow cwd/.git to be a symlink to a directory
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Feb 17, 2026, 11:26 UTC
- Message-ID
- <CAOLa=ZTeTWhb0Yc8rPEv8vONTHtSg3bSvW6FBC-AWrZzi12oCA@mail.gmail.com>
- In-Reply-To
- <20260217084124.150366-1-a3205153416@gmail.com>
Tian Yuchen <a3205153416@gmail.com> writes:
Show 14 quoted lines
> Strictly enforcing 'lstat()' and 'S_ISREG()' on '.git' prevents valid > workflows where '.git' is a symbolic link pointing to a real git > directory (e.g. created via 'ln -s'). > > Refactor 'setup_git_directory_gently_1()' to use 'stat()' instead of > 'lstat()'. This allows the filesystem to automatically resolve symbolic > links. > > To ensure safety and correctness, the logic flow is updated to: > > 1. Ignore 'ENOENT' (file missing). > 2. Check 'IS_A_DIR' cases via 'is_git_directory()'. > 3. Explicitly reject 'NOT_A_FILE' cases (FIFOs or sockets). >
Small nit, it would have been a bit nicer to separate these out into individual commits with tests added per commit.
> Add a new test script t/t0009-setup-security.sh which verifies: >
Wouldn't something like 't0009-git-dir-validation.sh' be a better name?
Show 29 quoted lines
> - Valid .git symlinks to real directories are accepted.
> - .git as a named pipe (FIFO) is rejected.
> - .git as a symlink to a named pipe is rejected.
> - .git with garbage content is rejected.
> - Empty .git directories are ignored.
>
> Signed-off-by: Tian Yuchen <a3205153416@gmail.com>
> ---
> setup.c | 39 ++++++++++++++-------
> setup.h | 2 ++
> t/t0009-setup-security.sh | 72 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 101 insertions(+), 12 deletions(-)
> create mode 100755 t/t0009-setup-security.sh
>
> diff --git a/setup.c b/setup.c
> index 3a6a048620..269aa9faaa 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -939,8 +939,14 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
> static struct strbuf realpath = STRBUF_INIT;
>
> if (stat(path, &st)) {
> - /* NEEDSWORK: discern between ENOENT vs other errors */
> - error_code = READ_GITFILE_ERR_STAT_FAILED;
> + if (errno == ENOENT)
> + error_code = READ_GITFILE_ERR_STAT_ENOENT;
> + else
> + error_code = READ_GITFILE_ERR_STAT_FAILED;
> + goto cleanup_return;So this differentiates between 'stat()' failing and the path not existing. Ok.
Show 5 quoted lines
> + }
> + if (S_ISDIR(st.st_mode)) {
> + error_code = READ_GITFILE_ERR_IS_A_DIR;
> goto cleanup_return;
> }Okay so if the '.git' file is a directory, we set the appropriate error. So this error code is new and we introduce it in this patch.
Show 11 quoted lines
> if (!S_ISREG(st.st_mode)) {
> @@ -994,7 +1000,9 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
> cleanup_return:
> if (return_error_code)
> *return_error_code = error_code;
> - else if (error_code)
> + else if (error_code &&
> + error_code != READ_GITFILE_ERR_STAT_ENOENT &&
> + error_code != READ_GITFILE_ERR_IS_A_DIR)
> read_gitfile_error_die(error_code, path, dir);
>I understand the exclusion here (they are non-fatal flows), but wouldn't it more make sense to add these two exclusions within `read_gitfile_error_die()` which already has two such exclusions? By separating this out, it gets really confusing.
Show 8 quoted lines
> free(buf); > @@ -1576,20 +1584,27 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir, > if (offset > min_offset) > strbuf_addch(dir, '/'); > strbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT); > - gitdirenv = read_gitfile_gently(dir->buf, die_on_error ? > - NULL : &error_code); > + gitdirenv = read_gitfile_gently(dir->buf, &error_code);
Okay so we unconditionally read the error into errorcode, quick question that comes to mind: Wouldn't this break the previous flow for when `die_on_error = 1`? Where `read_gitfile_error_die()` would've been called?
Show 23 quoted lines
> if (!gitdirenv) {
> - if (die_on_error ||
> - error_code == READ_GITFILE_ERR_NOT_A_FILE) {
> - /* NEEDSWORK: fail if .git is not file nor dir */
> + if (error_code == READ_GITFILE_ERR_STAT_ENOENT) {
> + ;
> + } else if (error_code == READ_GITFILE_ERR_IS_A_DIR) {
> if (is_git_directory(dir->buf)) {
> gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
> gitdir_path = xstrdup(dir->buf);
> }
> - } else if (error_code != READ_GITFILE_ERR_STAT_FAILED)
> - return GIT_DIR_INVALID_GITFILE;
> - } else
> - gitfile = xstrdup(dir->buf);
> + } else if (error_code == READ_GITFILE_ERR_NOT_A_FILE) {
> + if (die_on_error)
> + die(_("Invalid %s: not a regular file or directory"), dir->buf);
> + else
> + return GIT_DIR_INVALID_GITFILE;
> + } else if (error_code != READ_GITFILE_ERR_STAT_FAILED) {
> + if (die_on_error)
> + read_gitfile_error_die(error_code, dir->buf, NULL);Well seems a bit convoluted as we explicitly skip this in our call to `read_gitfile_gently()` to then call it ourselves.
Show 7 quoted lines
> + else > + return GIT_DIR_INVALID_GITFILE; > + } > + } > /* > * Earlier, we tentatively added DEFAULT_GIT_DIR_ENVIRONMENT > * to check that directory for a repository.
[snip]