From cf98844e273cfc498db8af55562de1fc61e93428 Mon Sep 17 00:00:00 2001 From: 35C4n0r Date: Tue, 19 May 2026 10:06:04 +0000 Subject: [PATCH] chore: address coder-agents-comments --- registry/coder/modules/git-clone/README.md | 7 +- registry/coder/modules/git-clone/main.test.ts | 81 ++++++++++++++++--- registry/coder/modules/git-clone/main.tf | 3 +- registry/coder/modules/git-clone/run.sh | 18 ++--- 4 files changed, 88 insertions(+), 21 deletions(-) diff --git a/registry/coder/modules/git-clone/README.md b/registry/coder/modules/git-clone/README.md index 2d3a1a82..513ba511 100644 --- a/registry/coder/modules/git-clone/README.md +++ b/registry/coder/modules/git-clone/README.md @@ -189,9 +189,14 @@ module "git-clone" { Pass any additional flags through `extra_args` (one element per argument). This lets you enable anything `git clone` supports without the module having -to expose it explicitly — for example a shallow clone, submodules, parallel +to expose it explicitly, for example a shallow clone, submodules, parallel fetches, or partial clones. +> Do not put secrets in `extra_args`. The resolved `git clone` command +> (including every element of `extra_args`) is echoed to the workspace +> startup log, so values like `--config=http.extraHeader=Authorization: Bearer ` +> would appear there in plaintext. + ```tf module "git-clone" { count = data.coder_workspace.me.start_count diff --git a/registry/coder/modules/git-clone/main.test.ts b/registry/coder/modules/git-clone/main.test.ts index 93eaa32a..e3175ed6 100644 --- a/registry/coder/modules/git-clone/main.test.ts +++ b/registry/coder/modules/git-clone/main.test.ts @@ -29,6 +29,20 @@ const executeScriptInContainer = async ( }; }; +// Drops a fake `git` onto PATH that prints each argv entry on its own line. +// Lets tests prove that arguments (including ones with embedded spaces) reach +// `git clone` as single argv tokens, which the echo line cannot show because +// it joins with spaces. +const installFakeGit = [ + "cat > /usr/local/bin/git <<'SHIM'", + "#!/bin/sh", + 'for arg in "$@"; do', + ' printf "argv:%s\\n" "$arg"', + "done", + "SHIM", + "chmod +x /usr/local/bin/git", +].join("\n"); + describe("git-clone", async () => { await runTerraformInit(import.meta.dir); @@ -56,7 +70,7 @@ describe("git-clone", async () => { expect(output.stdout).toEqual([ "Creating directory /root/fake-url...", "Cloning fake-url to /root/fake-url...", - "Running: git clone fake-url /root/fake-url", + "Running: git clone fake-url /root/fake-url", ]); expect(output.stderr.join(" ")).toContain("fatal"); expect(output.stderr.join(" ")).toContain("fake-url"); @@ -233,7 +247,7 @@ describe("git-clone", async () => { expect(output.stdout).toEqual([ "Creating directory /root/repo-tests.log...", "Cloning https://github.com/michaelbrewer/repo-tests.log to /root/repo-tests.log on branch feat/branch...", - "Running: git clone -b feat/branch https://github.com/michaelbrewer/repo-tests.log /root/repo-tests.log", + "Running: git clone -b feat/branch https://github.com/michaelbrewer/repo-tests.log /root/repo-tests.log", ]); }); @@ -247,7 +261,7 @@ describe("git-clone", async () => { expect(output.stdout).toEqual([ "Creating directory /root/repo-tests.log...", "Cloning https://gitlab.com/mike.brew/repo-tests.log to /root/repo-tests.log on branch feat/branch...", - "Running: git clone -b feat/branch https://gitlab.com/mike.brew/repo-tests.log /root/repo-tests.log", + "Running: git clone -b feat/branch https://gitlab.com/mike.brew/repo-tests.log /root/repo-tests.log", ]); }); @@ -269,7 +283,7 @@ describe("git-clone", async () => { expect(output.stdout).toEqual([ "Creating directory /root/repo-tests.log...", "Cloning https://github.com/michaelbrewer/repo-tests.log to /root/repo-tests.log on branch feat/branch...", - "Running: git clone -b feat/branch https://github.com/michaelbrewer/repo-tests.log /root/repo-tests.log", + "Running: git clone -b feat/branch https://github.com/michaelbrewer/repo-tests.log /root/repo-tests.log", ]); }); @@ -322,18 +336,67 @@ describe("git-clone", async () => { url: "fake-url", }); const script = findResourceInstance(state, "coder_script").script; - expect(script).toContain('EXTRA_ARGS_B64=""'); + expect(script).toContain('EXTRA_ARGS=""'); }); it("passes extra_args to git clone", async () => { const state = await runTerraformApply(import.meta.dir, { agent_id: "foo", url: "fake-url", - extra_args: '["--recurse-submodules", "--jobs=8"]', + extra_args: JSON.stringify([ + "--recurse-submodules", + "--jobs=8", + "--config=user.name=Coder User", + "-c", + "core.sshCommand=ssh -i /tmp/key", + ]), }); - const output = await executeScriptInContainer(state, "alpine/git"); - expect(output.stdout).toContain( - "Running: git clone --recurse-submodules --jobs=8 fake-url /root/fake-url", + const output = await executeScriptInContainer( + state, + "alpine/git", + installFakeGit, + ); + expect(output.exitCode).toBe(0); + expect(output.stdout.join("\n")).toContain( + [ + "argv:clone", + "argv:--recurse-submodules", + "argv:--jobs=8", + "argv:--config=user.name=Coder User", + "argv:-c", + "argv:core.sshCommand=ssh -i /tmp/key", + "argv:fake-url", + "argv:/root/fake-url", + ].join("\n"), + ); + }); + + it("passes extra_args alongside branch_name in the correct order", async () => { + const state = await runTerraformApply(import.meta.dir, { + agent_id: "foo", + url: "fake-url", + branch_name: "feat/branch", + extra_args: JSON.stringify([ + "--recurse-submodules", + "--config=user.name=Coder User", + ]), + }); + const output = await executeScriptInContainer( + state, + "alpine/git", + installFakeGit, + ); + expect(output.exitCode).toBe(0); + expect(output.stdout.join("\n")).toContain( + [ + "argv:clone", + "argv:--recurse-submodules", + "argv:--config=user.name=Coder User", + "argv:-b", + "argv:feat/branch", + "argv:fake-url", + "argv:/root/fake-url", + ].join("\n"), ); }); diff --git a/registry/coder/modules/git-clone/main.tf b/registry/coder/modules/git-clone/main.tf index 2a2f2e72..ab2279fe 100644 --- a/registry/coder/modules/git-clone/main.tf +++ b/registry/coder/modules/git-clone/main.tf @@ -97,8 +97,7 @@ locals { encoded_post_clone_script = var.post_clone_script != null ? base64encode(var.post_clone_script) : "" # Encode the pre_clone_script for passing to the shell script encoded_pre_clone_script = var.pre_clone_script != null ? base64encode(var.pre_clone_script) : "" - # Encode extra clone args (newline-separated) so the shell script can split them into an array safely - encoded_extra_args = base64encode(join("\n", var.extra_args)) + encoded_extra_args = base64encode(join("\n", var.extra_args)) } output "repo_dir" { diff --git a/registry/coder/modules/git-clone/run.sh b/registry/coder/modules/git-clone/run.sh index 752e84bc..bad790f8 100644 --- a/registry/coder/modules/git-clone/run.sh +++ b/registry/coder/modules/git-clone/run.sh @@ -7,7 +7,7 @@ CLONE_PATH="${CLONE_PATH}" BRANCH_NAME="${BRANCH_NAME}" # Expand home if it's specified! CLONE_PATH="$${CLONE_PATH/#\~/$${HOME}}" -EXTRA_ARGS_B64="${EXTRA_ARGS}" +EXTRA_ARGS="${EXTRA_ARGS}" POST_CLONE_SCRIPT="${POST_CLONE_SCRIPT}" PRE_CLONE_SCRIPT="${PRE_CLONE_SCRIPT}" @@ -47,11 +47,11 @@ if [ -n "$PRE_CLONE_SCRIPT" ]; then fi # Build optional git clone flags -CLONE_FLAGS=() -if [ -n "$EXTRA_ARGS_B64" ]; then +extra_args=() +if [ -n "$EXTRA_ARGS" ]; then while IFS= read -r arg || [ -n "$arg" ]; do - [ -n "$arg" ] && CLONE_FLAGS+=("$arg") - done < <(echo "$EXTRA_ARGS_B64" | base64 -d) + [ -n "$arg" ] && extra_args+=("$arg") + done < <(echo "$EXTRA_ARGS" | base64 -d) fi # Check if the directory is empty @@ -59,12 +59,12 @@ fi if [ -z "$(ls -A "$CLONE_PATH")" ]; then if [ -z "$BRANCH_NAME" ]; then echo "Cloning $REPO_URL to $CLONE_PATH..." - echo "Running: git clone $${CLONE_FLAGS[*]} $REPO_URL $CLONE_PATH" - git clone "$${CLONE_FLAGS[@]}" "$REPO_URL" "$CLONE_PATH" + echo "Running: git clone $${extra_args[@]:+$${extra_args[@]} }$REPO_URL $CLONE_PATH" + git clone $${extra_args[@]+"$${extra_args[@]}"} "$REPO_URL" "$CLONE_PATH" else echo "Cloning $REPO_URL to $CLONE_PATH on branch $BRANCH_NAME..." - echo "Running: git clone $${CLONE_FLAGS[*]} -b $BRANCH_NAME $REPO_URL $CLONE_PATH" - git clone "$${CLONE_FLAGS[@]}" -b "$BRANCH_NAME" "$REPO_URL" "$CLONE_PATH" + echo "Running: git clone $${extra_args[@]:+$${extra_args[@]} }-b $BRANCH_NAME $REPO_URL $CLONE_PATH" + git clone $${extra_args[@]+"$${extra_args[@]}"} -b "$BRANCH_NAME" "$REPO_URL" "$CLONE_PATH" fi else echo "$CLONE_PATH already exists and isn't empty, skipping clone!"