Harden Git metadata project identity - #403
Conversation
PR Summary by QodoHarden Git metadata project identity resolution
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR hardens Git-based project identity resolution to prevent repository-controlled metadata from selecting another checkout’s persistent LCM state, while keeping supported Git layouts working and ensuring Git config parsing stays bounded and linear.
Changes:
- Enforces authenticated, bidirectional topology evidence for external/standalone Git metadata (via
core.worktree) and contains repository-localcommondirwithin the authenticated Git admin root. - Replaces ambiguous worktree-config matching with a bounded, linear Git config parser that implements “final assignment wins” and rejects includes/ambiguous syntax.
- Adds focused adversarial/race tests, updates user documentation, and includes a patch changeset.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/git-project.test.ts | Adds adversarial/race tests for directory authentication, external metadata backlinks, and config parsing semantics. |
| src/git-project.ts | Implements directory authentication/revalidation, external backlink proof, commondir containment checks, and a new linear config parser. |
| docs/project-identity.md | Documents the new identity proof requirements and parsing rules (including rejecting include/includeIf). |
| .changeset/secure-git-metadata-identity.md | Patch changeset describing the user-facing hardening and parsing behavior. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71247aa56c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Code Review by Qodo
Context usedReview mode:
🧠 Deep: This security-sensitive change introduces substantial new metadata authentication, race validation, bounded config parsing, and topology logic across many independent edit sites, making multiple review passes materially valuable. 1. Invalid subsection escapes accepted
|
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/git-project.ts:567
const bool = asciiLower(parsedValue!.value!)uses a double non-null assertion. Even if currently safe, it makes the invariant implicit and would turn a future bug into silently wrong behavior. Add a small guard that treats a missing parsed value as invalid worktreeConfig syntax (fail-closed) and removes the assertions.
const bool = asciiLower(parsedValue!.value!);
src/git-project.ts:561
- In parseGitConfig(),
coreWorktree = parsedValue!.valuerelies on a non-null assertion even though the code path depends onparseConfigValue()returning avaluewhen valid. Adding an explicitparsedValue?.valueguard here both documents the invariant and prevents future refactors from accidentally accepting an undefined value as a validcore.worktree.
This issue also appears on line 567 of the same file.
if (implicit) {
valid = false;
} else {
coreWorktree = parsedValue!.value;
}
test/git-project.test.ts:142
- makeStandaloneMetadata() builds the relative
.gitpointer withmetadata.split("/"), which is not path-separator safe (breaks on Windows) and implicitly assumescheckoutandmetadataare siblings. This can produce an invalid pointer and make tests pass for the wrong reason. Consider guarding the sibling-layout precondition and splitting on both/and\\when constructing the relative pointer.
writeFileSync(
join(checkout, ".git"),
`gitdir: ${absolutePointer ? metadata : `../${metadata.split("/").at(-1)!}`}\n`,
);
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 436217dd3c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Bernardo Donadio <bcdonadio@bcdonadio.com>
Signed-off-by: Bernardo Donadio <bcdonadio@bcdonadio.com>
Signed-off-by: Bernardo Donadio <bcdonadio@bcdonadio.com>
436217d to
7105e86
Compare
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/git-project.ts:453
parseSectionName()currently allows a section header like[extensions]#commentby treating#/;as a comment even when it is not preceded by whitespace. Per git-config syntax, comment markers must be at the beginning of the line or preceded by whitespace; without whitespace this should fail closed as invalid syntax to avoid diverging from Git’s parsing rules.
let tail = close + 1;
while (tail < line.length && isConfigWhitespace(line[tail]!)) tail += 1;
if (tail < line.length && line[tail] !== "#" && line[tail] !== ";") {
return { valid: false };
src/git-project.ts:538
parseConfigValue()treats any unquoted#/;as starting an inline comment. Git only recognizes inline comments when the comment marker is preceded by whitespace; otherwise it is part of the value (e.g.true#comment). Treating it as a comment here can make LCM accept configs as valid/true that Git would treat as a different (often invalid) value.
if (!quoted && (char === "#" || char === ";")) break;
src/git-project.ts:386
- Git config comments (
#/;) are only recognized at the start of a line or when preceded by whitespace.scanPhysicalConfigLine()currently treats any unquoted#/;as starting a comment, so values likeworktreeConfig = true#not-a-comment(which Git interprets as literaltrue#not-a-comment) get truncated and may be mis-parsed, diverging from Git semantics used for identity hardening.
This issue also appears in the following locations of the same file:
- line 450
- line 538
if (!quoted && (char === "#" || char === ";")) {
return { continues: false, quoted };
}
test/git-project.test.ts:1421
- The config-syntax regression tests don’t currently cover Git’s rule that
#/;start inline comments only when preceded by whitespace. Adding cases likeworktreeConfig = true#commentand[extensions]#commenthelps ensure the parser stays aligned with git-config semantics for ambiguous/adversarial inputs.
for (const config of [
"[extensions]\nworktreeConfig = false\n",
"[extensions]\nworktreeConfig = maybe\n",
"[extensions]\nworktreeConfig # invalid implicit comment\n",
"[extensions]\nworktreeConfig trailing\n",
"[extensions]\nworktreeConfig = \\\n",
"[extensions]\nworktreeConfig = \\\\q\n",
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7105e86c35
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Accept verified case-only spellings without admitting bind, symlink, ambiguous, or cross-checkout aliases. Linearize directory authentication on a no-follow descriptor and preserve bounded fallback validation. Signed-off-by: Bernardo Donadio <bcdonadio@bcdonadio.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/git-project.ts:778
- When
config.worktreeparsing is invalid, this error still points at${gitDir}/config, which can mislead debugging (the invalid bytes are inconfig.worktree). Consider selecting the path based on which parse failed.
const values = readConfigValues(gitDir);
if (!values.commonValues.valid || values.worktreeValues?.valid === false) {
throw new Error(`invalid Git config metadata at ${join(gitDir, "config")}: ambiguous worktree configuration`);
}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eecde00646
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Bernardo Donadio <bcdonadio@bcdonadio.com>
Signed-off-by: Bernardo Donadio <bcdonadio@bcdonadio.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 581614621f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a48569a85
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Bernardo Donadio <bcdonadio@bcdonadio.com>
Signed-off-by: Bernardo Donadio <bcdonadio@bcdonadio.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/git-project.ts:963
- readExternalWorktreeEvidence() authenticates configuredPath with authenticateDirectory(configuredPath, ...) using the default expectedRealPath=configuredPath. On case-insensitive filesystems where realpathSync normalizes the stored on-disk casing, a case-only core.worktree value can resolve to the same directory but still fail authentication because realpathSync(configuredPath) will likely return worktreeRoot (canonical casing) rather than the configured spelling. That would incorrectly reject valid case-only backlinks (and contradict the documented support for case-only spellings).
configuredIdentity = authenticateDirectory(
configuredPath,
"Git core.worktree directory",
);
Signed-off-by: Bernardo Donadio <bcdonadio@bcdonadio.com>
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/git-project.ts:368
- resolveCommonDir() calls lstatSync(target) without wrapping errors. For missing/unreadable commondir targets, this will throw a raw Node errno error rather than a consistent “invalid Git common directory …” error, which makes failures harder to diagnose and less consistent with other metadata validation errors.
const target = resolve(gitDir, relative);
const targetStat = lstatSync(target);
if (targetStat.isSymbolicLink()) {
throw new Error(`invalid Git common directory at ${target}`);
}
src/git-project.ts:307
- parseGitDir() calls lstatSync(targetPath) without error wrapping. If the gitdir target is missing or unreadable (ENOENT/EACCES), this will bubble up as a raw Node errno error and lose the “invalid Git …” context used elsewhere in this module for repository-controlled metadata failures.
This issue also appears on line 364 of the same file.
const targetPath = resolve(worktreeRoot, target);
const targetStat = lstatSync(targetPath);
if (targetStat.isSymbolicLink()) {
throw new Error(`invalid Git directory at ${targetPath}`);
}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4268cf1d47
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| configuredIdentity = authenticateDirectory( | ||
| configuredPath, | ||
| "Git core.worktree directory", | ||
| ); |
There was a problem hiding this comment.
Pass the canonical path when authenticating case variants
When core.worktree differs from worktreeRoot only by casing on a case-insensitive host whose realpathSync canonicalizes component names, this call retains the default expectedRealPath = configuredPath; authenticateDirectory therefore rejects as soon as the real path equals worktreeRoot, before the device/inode comparison can accept it. Fresh evidence after the earlier claimed fix is that the final tree still omits worktreeRoot here, whereas parseGitDir explicitly passes its canonical gitDir. Standalone pointers and submodules with valid case-only backlinks consequently still fail on such hosts.
Useful? React with 👍 / 👎.
| if ( | ||
| key === "path" | ||
| && (sectionBase === "include" || sectionBase === "includeif") | ||
| ) { |
There was a problem hiding this comment.
Reject only actual Git include directives
For a standalone pointer whose otherwise valid config contains a custom subsection such as [include "metadata"] followed by path = value (or deprecated [include.foo]), sectionBase is include, so this rejects the entire repository even though Git does not treat that key as an include directive. git config -h describes --includes as “respect include directives on lookup”; checked with Git 2.43, git config --includes --file ... --list leaves this as include.metadata.path and does not load the target. Restrict this fail-closed branch to the exact [include] section and the actual conditional [includeIf "..."] form.
Useful? React with 👍 / 👎.
Summary
Verification
Release
Includes a patch changeset and user-facing identity documentation.
Fixes #342
Fixes #401
Fixes #402
Greptile Summary
This PR hardens Git metadata project identity resolution against path substitution, TOCTOU races, and config injection attacks by replacing regex-based config scanning with a full bounded parser, adding descriptor-bound directory authentication, and requiring cryptographically-verified
core.worktreebacklinks for every standalone external.gitpointer.authenticateDirectory: opens directories withO_RDONLY | O_DIRECTORY | O_NOFOLLOWon supported platforms, then cross-checksfstatSyncdev/ino against a subsequentlstatSyncto close the TOCTOU window; falls back to a multi-lstat path on Windows.parseGitConfig: replaces simple regexes with a complete, linear config parser covering line continuation, quoted values, subsection syntax, and integer booleans — rejectinginclude/includeifpaths and marking the config invalid on any ambiguous syntax.readExternalWorktreeEvidence: for every.gitfile pointer wherecommonDir === gitDir, requirescore.worktreeto resolve and authenticate to the exact same inode as the caller's worktree root; the canonical result is now fail-closed rather than falling back to an unverified anchor..gitdirectories whosecommondirresolves outside the.gitroot are now rejected outright, eliminating the local-indirection escape path.Confidence Score: 5/5
Safe to merge. All topology paths (normal repo, linked worktree, submodule, separate-git-dir) are preserved and each is exercised by the expanded test suite at 100% coverage.
Every changed code path has been analyzed: the new config parser correctly handles all Git config syntax edge cases; the fd-based directory authenticator correctly falls back on platforms without O_DIRECTORY/O_NOFOLLOW; the external-evidence flow correctly requires core.worktree for all standalone .git file pointers while preserving linked-worktree and submodule layouts. No logic gaps or race-window expansions were found.
Files Needing Attention: No files require special attention.
Important Files Changed
Reviews (8): Last reviewed commit: "fix(git): enforce section header grammar" | Re-trigger Greptile
Context used: