-
Notifications
You must be signed in to change notification settings - Fork 365
Detect and handle non-100644 file modes (symlinks, executables) in push_signed_commits #26259
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,6 +48,102 @@ async function pushSignedCommits({ githubClient, owner, repo, branch, baseRef, c | |
| core.info(`pushSignedCommits: replaying ${shas.length} commit(s) via GraphQL createCommitOnBranch (branch: ${branch}, repo: ${owner}/${repo})`); | ||
|
|
||
| try { | ||
| // Pre-scan ALL commits: collect file changes and check for unsupported file modes | ||
| // BEFORE starting any GraphQL mutations. If a symlink is found mid-loop after some | ||
| // commits have already been signed, the remote branch diverges and the git push | ||
| // fallback would be rejected as non-fast-forward. | ||
| // | ||
| // The GitHub GraphQL createCommitOnBranch mutation only supports regular file mode 100644: | ||
| // - Symlinks (120000) would be silently converted to regular files containing the link target path | ||
| // - Executable bits (100755) are silently dropped | ||
| /** @type {Map<string, Array<{path: string, contents: string}>>} */ | ||
| const additionsMap = new Map(); | ||
| /** @type {Map<string, Array<{path: string}>>} */ | ||
| const deletionsMap = new Map(); | ||
|
|
||
| for (const sha of shas) { | ||
| /** @type {Array<{path: string, contents: string}>} */ | ||
| const additions = []; | ||
| /** @type {Array<{path: string}>} */ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Smoke Test Review (Run 24417642151): The pre-scan approach for detecting unsupported file modes before any GraphQL mutations is a good defensive pattern — it prevents partial signed-commit pushes that would leave the remote in a diverged state. |
||
| const deletions = []; | ||
|
|
||
| // Use git diff-tree --raw to obtain file mode information per changed file. | ||
| // Format: :<srcMode> <dstMode> <srcHash> <dstHash> <status>[score]\t<path>[<\t><newPath>] | ||
| // Fields: [0]=srcMode, [1]=dstMode, [2]=srcHash, [3]=dstHash, [4]=status | ||
| const { stdout: rawDiffOut } = await exec.getExecOutput("git", ["diff-tree", "-r", "--raw", sha], { cwd }); | ||
|
|
||
| for (const line of rawDiffOut.trim().split("\n").filter(Boolean)) { | ||
| // Raw format lines start with ':'; skip the commit SHA header line and any other non-raw lines | ||
| if (!line.startsWith(":")) continue; | ||
|
|
||
| const tabIdx = line.indexOf("\t"); | ||
| if (tabIdx === -1) continue; | ||
|
|
||
| const modeFields = line.slice(1, tabIdx).split(" "); // strip leading ':' | ||
| if (modeFields.length < 5) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The copy handling ( |
||
| core.warning(`pushSignedCommits: unexpected diff-tree output format, skipping line: ${line}`); | ||
| continue; | ||
| } | ||
| const dstMode = modeFields[1]; // destination file mode (e.g. 100644, 100755, 120000) | ||
| const status = modeFields[4]; // A=Added, M=Modified, D=Deleted, R=Renamed, C=Copied | ||
|
|
||
| const paths = line.slice(tabIdx + 1).split("\t"); | ||
| const filePath = paths[0]; | ||
|
|
||
| if (status === "D") { | ||
| deletions.push({ path: filePath }); | ||
| } else if (status && status.startsWith("R")) { | ||
| // Rename: source path is deleted, destination path is added | ||
| const renamedPath = paths[1]; | ||
| if (!renamedPath) { | ||
| core.warning(`pushSignedCommits: rename entry missing destination path, skipping: ${line}`); | ||
| continue; | ||
| } | ||
| deletions.push({ path: filePath }); | ||
| if (dstMode === "120000") { | ||
| core.warning(`pushSignedCommits: symlink ${renamedPath} cannot be pushed as a signed commit, falling back to git push`); | ||
| throw new Error("symlink file mode requires git push fallback"); | ||
| } | ||
| if (dstMode === "100755") { | ||
| core.warning(`pushSignedCommits: executable bit on ${renamedPath} will be lost in signed commit (GitHub GraphQL does not support mode 100755)`); | ||
| } | ||
| const content = fs.readFileSync(path.join(cwd, renamedPath)); | ||
| additions.push({ path: renamedPath, contents: content.toString("base64") }); | ||
| } else if (status && status.startsWith("C")) { | ||
| // Copy: source path is kept (no deletion), only the destination path is added | ||
| const copiedPath = paths[1]; | ||
| if (!copiedPath) { | ||
| core.warning(`pushSignedCommits: copy entry missing destination path, skipping: ${line}`); | ||
| continue; | ||
| } | ||
| if (dstMode === "120000") { | ||
| core.warning(`pushSignedCommits: symlink ${copiedPath} cannot be pushed as a signed commit, falling back to git push`); | ||
| throw new Error("symlink file mode requires git push fallback"); | ||
| } | ||
| if (dstMode === "100755") { | ||
| core.warning(`pushSignedCommits: executable bit on ${copiedPath} will be lost in signed commit (GitHub GraphQL does not support mode 100755)`); | ||
| } | ||
| const content = fs.readFileSync(path.join(cwd, copiedPath)); | ||
| additions.push({ path: copiedPath, contents: content.toString("base64") }); | ||
| } else { | ||
| // Added or Modified | ||
| if (dstMode === "120000") { | ||
| core.warning(`pushSignedCommits: symlink ${filePath} cannot be pushed as a signed commit, falling back to git push`); | ||
| throw new Error("symlink file mode requires git push fallback"); | ||
| } | ||
| if (dstMode === "100755") { | ||
| core.warning(`pushSignedCommits: executable bit on ${filePath} will be lost in signed commit (GitHub GraphQL does not support mode 100755)`); | ||
| } | ||
| const content = fs.readFileSync(path.join(cwd, filePath)); | ||
| additions.push({ path: filePath, contents: content.toString("base64") }); | ||
| } | ||
| } | ||
|
|
||
| additionsMap.set(sha, additions); | ||
| deletionsMap.set(sha, deletions); | ||
| } | ||
|
|
||
| // All commits passed the mode checks. Replay via GraphQL. | ||
| /** @type {string | undefined} */ | ||
| let lastOid; | ||
| for (let i = 0; i < shas.length; i++) { | ||
|
|
@@ -110,30 +206,8 @@ async function pushSignedCommits({ githubClient, owner, repo, branch, baseRef, c | |
| const body = message.split("\n").slice(1).join("\n").trim(); | ||
| core.info(`pushSignedCommits: commit message headline: "${headline}"`); | ||
|
|
||
| // File changes for this commit (supports Add/Modify/Delete/Rename/Copy) | ||
| const { stdout: nameStatusOut } = await exec.getExecOutput("git", ["diff", "--name-status", `${sha}^`, sha], { cwd }); | ||
| /** @type {Array<{path: string, contents: string}>} */ | ||
| const additions = []; | ||
| /** @type {Array<{path: string}>} */ | ||
| const deletions = []; | ||
|
|
||
| for (const line of nameStatusOut.trim().split("\n").filter(Boolean)) { | ||
| const parts = line.split("\t"); | ||
| const status = parts[0]; | ||
| if (status === "D") { | ||
| deletions.push({ path: parts[1] }); | ||
| } else if (status.startsWith("R") || status.startsWith("C")) { | ||
| // Rename or Copy: parts[1] = old path, parts[2] = new path | ||
| deletions.push({ path: parts[1] }); | ||
| const content = fs.readFileSync(path.join(cwd, parts[2])); | ||
| additions.push({ path: parts[2], contents: content.toString("base64") }); | ||
| } else { | ||
| // Added or Modified | ||
| const content = fs.readFileSync(path.join(cwd, parts[1])); | ||
| additions.push({ path: parts[1], contents: content.toString("base64") }); | ||
| } | ||
| } | ||
|
|
||
| const additions = additionsMap.get(sha) || []; | ||
| const deletions = deletionsMap.get(sha) || []; | ||
| core.info(`pushSignedCommits: file changes: ${additions.length} addition(s), ${deletions.length} deletion(s)`); | ||
|
|
||
| /** @type {any} */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -574,4 +574,108 @@ describe("push_signed_commits integration tests", () => { | |
| expect(callArg.message.body).toBeUndefined(); | ||
| }); | ||
| }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Smoke Test Review (Run 24417642151): New test suite for file mode handling looks comprehensive — covers symlink fallback, executable-bit warning, and the regular-file baseline. Good coverage of the three cases. |
||
|
|
||
| // ────────────────────────────────────────────────────── | ||
| // File mode handling – symlinks and executables | ||
| // ────────────────────────────────────────────────────── | ||
|
|
||
| describe("file mode handling", () => { | ||
| it("should fall back to git push and warn when commit contains a symlink", async () => { | ||
| execGit(["checkout", "-b", "symlink-branch"], { cwd: workDir }); | ||
|
|
||
| // Create a regular file to serve as symlink target | ||
| fs.writeFileSync(path.join(workDir, "target.txt"), "Symlink target\n"); | ||
| execGit(["add", "target.txt"], { cwd: workDir }); | ||
| execGit(["commit", "-m", "Add target file"], { cwd: workDir }); | ||
| execGit(["push", "-u", "origin", "symlink-branch"], { cwd: workDir }); | ||
|
|
||
| // Add a symlink in a new commit | ||
| fs.symlinkSync("target.txt", path.join(workDir, "link.txt")); | ||
| execGit(["add", "link.txt"], { cwd: workDir }); | ||
| execGit(["commit", "-m", "Add symlink"], { cwd: workDir }); | ||
| execGit(["push", "origin", "symlink-branch"], { cwd: workDir }); | ||
|
|
||
| global.exec = makeRealExec(workDir); | ||
| const githubClient = makeMockGithubClient(); | ||
|
|
||
| await pushSignedCommits({ | ||
| githubClient, | ||
| owner: "test-owner", | ||
| repo: "test-repo", | ||
| branch: "symlink-branch", | ||
| // Only replay the symlink commit | ||
| baseRef: "symlink-branch^", | ||
| cwd: workDir, | ||
| }); | ||
|
|
||
| // GraphQL should NOT have been called – symlink triggers fallback before mutation | ||
| expect(githubClient.graphql).not.toHaveBeenCalled(); | ||
| // Warning about symlink must be emitted | ||
| expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("symlink link.txt cannot be pushed as a signed commit")); | ||
| expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("falling back to git push")); | ||
|
|
||
| // The commit should be present on the remote via git push fallback | ||
| const lsRemote = execGit(["ls-remote", bareDir, "refs/heads/symlink-branch"], { cwd: workDir }); | ||
| const remoteOid = lsRemote.stdout.trim().split(/\s+/)[0]; | ||
| const localOid = execGit(["rev-parse", "HEAD"], { cwd: workDir }).stdout.trim(); | ||
| expect(remoteOid).toBe(localOid); | ||
| }); | ||
|
|
||
| it("should warn about executable bit loss but continue with GraphQL signed commit", async () => { | ||
| execGit(["checkout", "-b", "executable-branch"], { cwd: workDir }); | ||
|
|
||
| // Create an executable file | ||
| fs.writeFileSync(path.join(workDir, "script.sh"), "#!/bin/bash\necho hello\n"); | ||
| fs.chmodSync(path.join(workDir, "script.sh"), 0o755); | ||
| execGit(["add", "script.sh"], { cwd: workDir }); | ||
| execGit(["commit", "-m", "Add executable script"], { cwd: workDir }); | ||
| execGit(["push", "-u", "origin", "executable-branch"], { cwd: workDir }); | ||
|
|
||
| global.exec = makeRealExec(workDir); | ||
| const githubClient = makeMockGithubClient(); | ||
|
|
||
| await pushSignedCommits({ | ||
| githubClient, | ||
| owner: "test-owner", | ||
| repo: "test-repo", | ||
| branch: "executable-branch", | ||
| baseRef: "origin/main", | ||
| cwd: workDir, | ||
| }); | ||
|
|
||
| // GraphQL SHOULD still be called – executable bit triggers a warning but not a fallback | ||
| expect(githubClient.graphql).toHaveBeenCalledTimes(1); | ||
| // Warning about executable bit must be emitted | ||
| expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("executable bit on script.sh will be lost in signed commit")); | ||
| // The file content should be in the additions payload | ||
| const callArg = githubClient.graphql.mock.calls[0][1].input; | ||
| expect(callArg.fileChanges.additions).toHaveLength(1); | ||
| expect(callArg.fileChanges.additions[0].path).toBe("script.sh"); | ||
| expect(Buffer.from(callArg.fileChanges.additions[0].contents, "base64").toString()).toContain("echo hello"); | ||
| }); | ||
|
|
||
| it("should not warn for regular files (mode 100644)", async () => { | ||
| execGit(["checkout", "-b", "regular-file-branch"], { cwd: workDir }); | ||
| fs.writeFileSync(path.join(workDir, "regular.txt"), "Regular file content\n"); | ||
| execGit(["add", "regular.txt"], { cwd: workDir }); | ||
| execGit(["commit", "-m", "Add regular file"], { cwd: workDir }); | ||
| execGit(["push", "-u", "origin", "regular-file-branch"], { cwd: workDir }); | ||
|
|
||
| global.exec = makeRealExec(workDir); | ||
| const githubClient = makeMockGithubClient(); | ||
|
|
||
| await pushSignedCommits({ | ||
| githubClient, | ||
| owner: "test-owner", | ||
| repo: "test-repo", | ||
| branch: "regular-file-branch", | ||
| baseRef: "origin/main", | ||
| cwd: workDir, | ||
| }); | ||
|
|
||
| expect(githubClient.graphql).toHaveBeenCalledTimes(1); | ||
| // No warnings should be emitted for regular files | ||
| expect(mockCore.warning).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good approach pre-scanning all commits before starting any GraphQL mutations. This prevents the partial-push problem where a symlink found mid-loop after some commits were already signed would leave the remote in a diverged state.