security: reject symlinks in PR-supplied Firebase preview artifact by wecbaiyk-blip · Pull Request #3654 · angular/dev-infra

wecbaiyk-blip · 2026-05-09T00:29:36Z

Summary

upload-artifacts-to-firebase extracts a PR-supplied tarball (unsafe-artifact.zip -> deploy-artifact.tar.gz) into the Firebase public directory and then deploys it to a Firebase Hosting preview channel. The artifact is explicitly annotated as RISK in the action's own comments, and the description points at the GitHub Security Lab guidance on preventing pwn requests.

This PR adds a defense-in-depth check after extraction: if firebase-public-dir ends up containing any symlinks, the deploy step fails fast with an explanatory error.

Why

Currently tar -xvzf materialises any symlinks the PR author put in their artifact onto the runner's firebase-public-dir. Downstream tooling that walks firebase-public-dir to enumerate the files to upload (e.g. firebase-tools listFiles) calls fs.readFile* on each entry, which follows symlinks at the OS layer. A tar entry like

public/leak -> /proc/self/environ
public/leak -> ~/.config/gcloud/application_default_credentials.json
public/leak -> ~/.ssh/id_rsa
public/leak -> /run/secrets/<anything>

would therefore have the target's bytes uploaded to the publicly-readable preview CDN under a path the attacker chose.

There is no legitimate reason for a static-hosting public/ artifact to contain symlinks (the Hosting CDN serves files, not symlinks), so refusing them outright is safer than enumerating "safe" symlink targets.

Changes

In the Extracting workflow artifact into Firebase public directory. step:

Add set -euo pipefail so a shell error during extraction does not silently fall through to the deploy step.
After tar -xvzf, run find "$publicDir" -type l and exit non-zero with a ::error:: annotation if any entry is found. -type l catches symlink-to-file and symlink-to-directory regardless of whether their target is reachable.
No-op when the artifact is well-formed (no symlinks).

The check intentionally fails loudly rather than silently deleting symlinks, so the contributor sees the security policy and the maintainer can audit any legitimate edge case.

Backward compatibility

Workflows whose preview artifacts contain only regular files and directories are unaffected. Workflows that intentionally include symlinks in their preview artifact would start failing - but the maintainers' own framing (RISK annotation, pwn-request reference) suggests symlinks were never an intended use case.

Why not rely on the upstream `firebase-tools` fix alone

There are in-flight firebase-tools PRs on the same threat model:

firebase/firebase-tools#10477 - default-flip on fsAsync.readdirRecursive ignoreSymlinks.
firebase/firebase-tools#10478 - drop symlinks from Hosting deploy upload list (listFiles).

Those fix the same bug class at the deployment-tool layer. This PR is the action-layer belt-and-suspenders: even if those land later, get partially reverted, or are bypassed by a downstream tool that re-introduces symlink-following, the artifact never reaches the deploy stage with symlinks present in the first place.

Testing

Manual validation locally:

mkdir -p /tmp/pub
echo "<!doctype html>" > /tmp/pub/index.html
ln -s /etc/passwd /tmp/pub/leak
# new check rejects:
find /tmp/pub -type l   # prints /tmp/pub/leak

mkdir -p /tmp/pub2
echo "<!doctype html>" > /tmp/pub2/index.html
# new check passes:
find /tmp/pub2 -type l  # prints nothing

CI for this action runs in the Angular dev-infra integration suite; no test changes are needed because the action is shell-based and the new step is exercised end-to-end whenever a preview deploy runs.

gemini-code-assist

Code Review

This pull request introduces security hardening to the Firebase artifact upload process by adding 'set -euo pipefail' and a symlink detection check to prevent data leakage. The review feedback identifies several opportunities to further improve security, including preventing shell injection by passing inputs via environment variables, ensuring the symlink check does not fail-open, and sanitizing output to prevent malicious filenames from being interpreted as workflow commands.

gemini-code-assist · 2026-05-09T00:31:40Z

+        # `public` artifact destined for static hosting to contain
+        # symlinks, so refusing them outright is the safest default.
+        # See https://securitylab.github.com/research/github-actions-preventing-pwn-requests/.
+        symlinks=$(find "$publicDir" -type l 2>/dev/null || true)


The || true and 2>/dev/null suffixes make this security check "fail-open". If the find command fails for any reason (e.g., due to permission errors on directories created by the untrusted artifact), the symlinks variable will be empty, and the check will silently pass. \n\nSince set -e is active, removing || true ensures the workflow fails if the check cannot be completed. Removing 2>/dev/null ensures that any errors encountered during the scan are visible in the logs for debugging.

symlinks=$(find "$publicDir" -type l)

Fixed in 4cb600f — find is now symlinks=$(find "$publicDir" -type l) with no || true / 2>/dev/null. Under set -euo pipefail the step now fails closed if the scan cannot complete, instead of silently treating the artifact as symlink-free.

gemini-code-assist · 2026-05-09T00:31:40Z

+        set -euo pipefail
+
        extractDir="$RUNNER_TEMP/artifact-unpack"
+        publicDir='${{inputs.firebase-public-dir}}'


Interpolating ${{ inputs.firebase-public-dir }} directly into the shell script is vulnerable to shell injection if the input contains single quotes (e.g., path'with'quotes). Although this input is marked as TRUSTED in this repository, it is a security best practice in GitHub Actions to pass inputs via environment variables to avoid them being interpreted as part of the script body.

Fixed in 4cb600f — firebase-public-dir is now passed via a step-level env: FIREBASE_PUBLIC_DIR and read as publicDir="$FIREBASE_PUBLIC_DIR"; it is no longer interpolated into the script body, so its value can't be parsed as part of the shell script.

gemini-code-assist · 2026-05-09T00:31:40Z


        unzip unsafe-artifact.zip -d "$extractDir"
-        tar -xvzf "$extractDir/deploy-artifact.tar.gz" -C '${{inputs.firebase-public-dir}}'
+        tar -xvzf "$extractDir/deploy-artifact.tar.gz" -C "$publicDir"


Consider removing the -v (verbose) flag from the tar command. Since the artifact is PR-supplied and potentially malicious, a filename containing a newline followed by a GitHub Actions workflow command (e.g., \n::error ...) could be interpreted by the runner when printed to stdout during extraction.

tar -xzf "$extractDir/deploy-artifact.tar.gz" -C "$publicDir"

Agreed on this one, please make this change.

Fixed in 4cb600f — dropped -v from tar (tar -xzf ...). A crafted entry name can no longer be echoed to stdout during extraction and interpreted as a workflow command.

Done in 4cb600f — tar no longer uses -v.

gemini-code-assist · 2026-05-09T00:31:40Z

+        if [ -n "$symlinks" ]; then
+          echo "::error title=Symlinks rejected in artifact::The deploy artifact contains symlinks, which are not permitted in a Hosting public directory for security reasons (a symlink can leak the contents of files outside the source tree onto the public Firebase Hosting CDN)."
+          echo "Symlinks found:"
+          echo "$symlinks"


When printing the list of found symlinks, it is safer to indent the output. This prevents a malicious filename containing a newline from being interpreted as a GitHub Actions workflow command if it happens to start at the beginning of a line. Using printf is also safer than echo for printing arbitrary strings.

printf '%s\n' "$symlinks" | sed 's/^/ /'

Fixed in 4cb600f — the symlink list is now printed with printf '%s\n' "$symlinks" | sed 's/^/ /', indenting each line so a filename containing a newline cannot start a line with :: and be interpreted as a workflow command.

josephperrott

AGENT: I have reviewed the PR and left some inline suggestions.

Additionally, please check the commit message:
The commit message header uses type 'security', which is not a standard type in Angular commit guidelines. Consider changing it to:
fix(github-actions): reject symlinks in PR-supplied Firebase preview artifact
to match the guidelines (since 'fix' is for bug/vulnerability fixes and 'github-actions' is a valid scope).

josephperrott · 2026-05-12T18:12:51Z


        unzip unsafe-artifact.zip -d "$extractDir"
-        tar -xvzf "$extractDir/deploy-artifact.tar.gz" -C '${{inputs.firebase-public-dir}}'
+        tar -xvzf "$extractDir/deploy-artifact.tar.gz" -C "$publicDir"


Agreed on this one, please make this change.

josephperrott · 2026-06-01T14:11:44Z

+        set -euo pipefail
+
        extractDir="$RUNNER_TEMP/artifact-unpack"
+        publicDir='${{inputs.firebase-public-dir}}'


AGENT: Instead of using single-quoted shell interpolation, it is safer to pass this input via a step-level environment variable to avoid potential bash syntax or injection issues if the input contains special characters.

Example structure:

- name: Extracting workflow artifact into Firebase public directory. shell: bash env: FIREBASE_PUBLIC_DIR: ${{inputs.firebase-public-dir}} run: | set -euo pipefail extractDir="$RUNNER_TEMP/artifact-unpack" publicDir="$FIREBASE_PUBLIC_DIR"

Done in 4cb600f — moved to a step-level env: FIREBASE_PUBLIC_DIR: ${{inputs.firebase-public-dir}} and read it as publicDir="$FIREBASE_PUBLIC_DIR", exactly as suggested.

josephperrott · 2026-06-01T14:11:50Z

+        # `public` artifact destined for static hosting to contain
+        # symlinks, so refusing them outright is the safest default.
+        # See https://securitylab.github.com/research/github-actions-preventing-pwn-requests/.
+        symlinks=$(find "$publicDir" -type l 2>/dev/null || true)


AGENT: Consider removing || true and 2>/dev/null from the find command.

If find fails for any reason, we want the step to fail (which it will due to set -e if || true is removed). Keeping || true might silently bypass the security check if find fails to execute properly. Removing 2>/dev/null ensures that any errors are visible in the workflow logs for debugging.

Suggested change

symlinks=$(find "$publicDir" -type l 2>/dev/null || true)

symlinks=$(find "$publicDir" -type l)

Done in 4cb600f — removed || true and 2>/dev/null; the check now fails closed under set -e.

…artifact `upload-artifacts-to-firebase` extracts a PR-supplied tarball (`unsafe-artifact.zip` -> `deploy-artifact.tar.gz`) into the Firebase public directory and then deploys it to a Firebase Hosting preview channel. The artifact is explicitly annotated as `RISK` in this action's comments and the README points at GitHub Security Lab's guidance on preventing pwn requests. Currently `tar -xzf` materialises any symlinks that the PR author chose to put in their artifact onto the runner's `firebase-public-dir`. Downstream tooling that walks `firebase-public-dir` to enumerate the files to upload (e.g. `firebase-tools` `listFiles`) calls `fs.readFile*` on each entry, which follows symlinks at the OS layer. A tar entry like `public/leak -> /proc/self/environ` or `public/leak -> ~/.config/gcloud/application_default_credentials.json` would therefore have its target's bytes uploaded to the publicly-readable preview CDN. Defense in depth: after extraction, fail the deploy if the public directory contains any symlinks. There is no legitimate reason for a static-hosting artifact to contain symlinks, so refusing them outright is safer than trying to enumerate safe symlink targets. Hardening applied from review: - Pass `firebase-public-dir` via a step-level `env` var instead of interpolating it into the script body (avoids shell injection). - `find -type l` is no longer suffixed with `|| true` / `2>/dev/null`, so under `set -euo pipefail` the check fails closed if the scan cannot complete instead of silently passing. - Drop `-v` from `tar` so a crafted entry name cannot emit a workflow command to stdout during extraction. - Indent the reported symlink list via `printf | sed` so a malicious filename containing a newline cannot be interpreted as a workflow command. This patch complements upstream `firebase-tools` hardening (separate PRs against `firebase/firebase-tools`) and stays defensive even if those land later or are partially reverted.

wecbaiyk-blip · 2026-06-01T15:20:07Z

Thanks for the review @josephperrott. Pushed 4cb600f addressing every point:

Commit message — retyped header as fix(github-actions): ... per the Angular guidelines (was security:, which isn't a valid type — this was failing the commit-message lint).
firebase-public-dir via env — now passed as a step-level env: FIREBASE_PUBLIC_DIR and read as publicDir="$FIREBASE_PUBLIC_DIR", not interpolated into the script body.
find fail-open — removed || true and 2>/dev/null; under set -euo pipefail the check now fails closed.
tar -v — removed; extraction no longer echoes attacker-controlled entry names to stdout.
Symlink list output — now printf '%s\n' "$symlinks" | sed 's/^/ /' so a newline in a filename can't be parsed as a workflow command.

Kept it as a single squashed commit. Happy to adjust further.

gemini-code-assist Bot reviewed May 9, 2026

View reviewed changes

josephperrott reviewed Jun 1, 2026

View reviewed changes

wecbaiyk-blip force-pushed the security/upload-artifacts-reject-symlinks branch from beb6b1d to 4cb600f Compare June 1, 2026 15:18

	symlinks=$(find "$publicDir" -type l 2>/dev/null \|\| true)
	symlinks=$(find "$publicDir" -type l)

Conversation

wecbaiyk-blip commented May 9, 2026

Summary

Why

Changes

Backward compatibility

Why not rely on the upstream firebase-tools fix alone

Testing

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot May 9, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 9, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 9, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 9, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josephperrott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wecbaiyk-blip commented Jun 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Why not rely on the upstream `firebase-tools` fix alone