fix: harden entrypoint output handling in fullsend action

[waynesun09]

Summary

• Path traversal mitigation: validate that the CLI-returned artifact path is absolute before passing it to actions/upload-artifact
• GITHUB_OUTPUT injection prevention: use heredoc delimiter instead of inline = assignment, preventing newline injection from CLI stdout

Note: the echo in ARTIFACT_DIR="$(echo fullsend entrypoint ...)" is an intentional stub — the fullsend entrypoint CLI subcommand is not yet implemented (see also internal/layers/workflows.go:175). These hardening fixes prepare the action for when the real command replaces the stub.

Test plan

• Confirm the step fails with a clear error if the CLI returns a relative or empty path
• Verify heredoc output format is correctly parsed by downstream steps

Hardens output handling introduced in #210.

[ralphbean]

Security hardening review — three issues to address before merging.

The approach is sound (heredoc delimiter for GITHUB_OUTPUT, absolute path validation), but there are gaps that leave the same injection vectors partially open. See inline comments.

[ralphbean]

Delimiter injection / no newline validation

The static FULLSEND_EOF delimiter can be forged if an attacker controls CLI stdout. If ARTIFACT_DIR contains FULLSEND_EOF on its own line, the heredoc terminates early and arbitrary GITHUB_OUTPUT variables can be injected — the same class of attack this PR is trying to prevent.

More fundamentally, ARTIFACT_DIR is not validated for embedded newlines or carriage returns, which is the primary injection vector.

Suggested fix (both layers):

# Reject paths with newlines/CRs
if [[ "${ARTIFACT_DIR}" =~ $'\n' || "${ARTIFACT_DIR}" =~ $'\r' ]]; then
  echo "::error::fullsend entrypoint returned a path containing control characters"
  exit 1
fi

# Use a randomized delimiter
DELIMITER="FULLSEND_EOF_$(head -c 16 /dev/urandom | xxd -p)"
{
  echo "artifact_dir<<${DELIMITER}"
  echo "${ARTIFACT_DIR}"
  echo "${DELIMITER}"
} >> "${GITHUB_OUTPUT}"

The newline rejection is the most important fix — it neutralizes delimiter collision as a secondary effect.

[ralphbean]

Untrusted input in ::error:: annotation

ARTIFACT_DIR comes from CLI stdout and is untrusted. Interpolating it directly into a ::error:: workflow command could allow workflow command injection (e.g., if the value contains :: sequences or encoded newlines).

Suggested fix — log the value separately instead of interpolating it into the annotation:

echo "::error::fullsend entrypoint returned a non-absolute path (see logs)"
printf 'Rejected path value: %s\n' "${ARTIFACT_DIR}"

[ralphbean]

No path canonicalization

A path like /tmp/../../../etc/shadow passes the absolute-path check. Consider canonicalizing with realpath and optionally enforcing a prefix:

ARTIFACT_DIR="$(realpath -m "${ARTIFACT_DIR}")"

The -m flag resolves logically without requiring the path to exist. This would also pair well with a prefix check (e.g., must be under ${GITHUB_WORKSPACE} or ${RUNNER_TEMP}) for defense-in-depth.

[waynesun09]

Closing — the underlying pattern this PR was hardening no longer exists on main.

The action now uses fullsend run --output-dir "${GITHUB_WORKSPACE}/output", which passes a predetermined directory to the CLI instead of reading one from CLI stdout. This removes the attack surface (GITHUB_OUTPUT injection, path traversal from untrusted stdout, delimiter forging) that this PR and the review comments were addressing.

All three review findings from @ralphbean were valid for the old fullsend entrypoint pattern but are no longer applicable.