Feat/v1.1.0#1
Open
NX1X wants to merge 17 commits into
Open
Conversation
…eanup logic - Change StrictHostKeyChecking default from 'no' to 'accept-new' - Validate connect-timeout and server-alive-interval as integers - Disable glob expansion (set -f) before host list loops - Use grep -F for fixed-string hostname matching in verify step - Track known_hosts creation in state file for selective cleanup - Document full security posture in CHANGELOG
…eanup logic - Change StrictHostKeyChecking default from 'no' to 'accept-new' - Validate connect-timeout and server-alive-interval as integers - Disable glob expansion (set -f) before host list loops - Use grep -F for fixed-string hostname matching in verify step - Track known_hosts creation in state file for selective cleanup - Update SECURITY.md and CHANGELOG.md for v1.1.0
…ings - Update HOST_KEY_CHECK assertions from 'no' to 'accept-new' - Replace fragile awk block extraction with sed in multi-host test - Add shellcheck disable comments for intentional SC2016/SC2088 - Quote variables to fix SC2086 - Remove unused variables (SC2034)
- Step 16: connect-timeout/server-alive-interval numeric validation - Step 17: cleanup awk Host block removal (middle/first/last/no-op/adjacent) - Step 18: retry loop mechanics (first-attempt/third-attempt/exhaust/mixed) - Step 19: glob protection with set -f (wildcard/question mark hostnames) - Step 20: known_hosts selective cleanup via KNOWN_HOSTS_CREATED flag - Step 21: idempotency - duplicate blocks on re-run, awk removes all - Step 22: edge case hostnames (@host, user@, IPv6, multiple @) - Step 23: ssh-extra-config boundary cases (empty lines, Match all, multi-host) - Step 24: duplicate hosts in input - config gen and cleanup behavior - Step 25: drift guard - grep action.yml to verify patterns match test assumptions
- Step 16: connect-timeout/server-alive-interval numeric validation - Step 17: cleanup awk Host block removal (middle/first/last/no-op/adjacent) - Step 18: retry loop mechanics (first-attempt/third-attempt/exhaust/mixed) - Step 19: glob protection with set -f (wildcard/question mark hostnames) - Step 20: known_hosts selective cleanup via KNOWN_HOSTS_CREATED flag - Step 21: idempotency - duplicate blocks on re-run, awk removes all - Step 22: edge case hostnames (@host, user@, IPv6, multiple @) - Step 23: ssh-extra-config boundary cases (empty lines, Match all, multi-host) - Step 24: duplicate hosts in input - config gen and cleanup behavior - Step 25: drift guard - grep action.yml to verify patterns match assumptions - Fix: grep in cleanup/action.yml needs || true for pipefail safety
- 10 new test steps (16-25) for comprehensive coverage - Fix grep pipefail in cleanup/action.yml (|| true) - Fix grep -c || echo producing duplicate output in tests
- 10 new test steps (16-25) for comprehensive coverage - Fix grep pipefail in cleanup/action.yml and tests (|| true) - Fix grep -c producing duplicate output with || echo - Fix drift guard: narrow wrapper check to run: block only - Fix drift guard: avoid literal in YAML to prevent parser error - Fix drift guard: use grep -F instead of broken regex pattern
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR introduces v1.1.0 of the Cloudflare Tunnel SSH action with multi-host support, configurable retry logic, optional known-hosts setup, custom SSH directives, and a new cleanup action. The core action is enhanced with inputs for ChangesSSH Action Enhancement with Cleanup & Multi-host Support
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 7
🧹 Nitpick comments (1)
scripts/_gen_social_preview.py (1)
23-35: 💤 Low valueConsider adding an existence check for the SVG file.
The script reads
SRC.read_bytes()without first checking if the file exists. While the script would fail clearly with aFileNotFoundError, adding an explicit check with a helpful message would improve user experience.🛡️ Optional improvement
svg = SRC.read_bytes() +if not SRC.exists(): + raise SystemExit(f"Source SVG not found: {SRC}") +svg = SRC.read_bytes()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/_gen_social_preview.py` around lines 23 - 35, Before calling SRC.read_bytes() verify the SVG exists and is a file: check SRC.exists() and SRC.is_file() (or equivalent) and if missing raise SystemExit with a clear message including the SRC path; update the top of the block where SRC.read_bytes() is used (references: variable SRC and the save/OUT logic) to perform this guard so the script exits with a helpful error instead of a raw FileNotFoundError.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test.yml:
- Around line 67-75: The "Verify outputs" step only asserts CF_VERSION and
merely prints TEST_RESULT; add an assertion that the TEST_RESULT output is
present and/or equal to the expected value (e.g., non-empty or "success") so
wiring breaks fail the job; update the run block to check [ -n "$TEST_RESULT" ]
(or compare "$TEST_RESULT" to the expected token) and exit non‑zero on failure,
referencing the CF_VERSION and TEST_RESULT env names in the check.
In @.github/workflows/unit-tests.yml:
- Around line 715-719: The tests assert that ~/.ssh/known_hosts has mode "644",
which is too permissive for SSH trust data; update the assertions to expect a
restrictive mode "600" instead. Modify the calls to assert_file_perms that
reference "$HOME/.ssh/known_hosts" (the occurrences around the current diff and
the similar block at the other location) to use "600" as the expected permission
string, ensuring the test enforces correct SSH file permissions.
- Line 418: Replace the insecure fixture setting "StrictHostKeyChecking no" with
the hardened default "StrictHostKeyChecking accept-new" in the workflow prints
so CI no longer disables host key checking; update each occurrence of the exact
string "StrictHostKeyChecking no" (seen in .github/workflows/unit-tests.yml and
the other reported occurrences) to "StrictHostKeyChecking accept-new" so the
fixtures reflect the intended no-known-hosts behavior without turning off
verification.
- Around line 1919-1922: The current cleanup check uses a test ([ ! -f
"$KEY_PATH" ] && [ ! -f "$HOME/.cloudflared-ssh" ] && [ ! -f
"$HOME/.cloudflared-ssh-state" ]) but then always echoes "PASS", causing a
false-positive; modify the block so the echo and increment of TESTS only happen
when the test succeeds and otherwise emit a failure message and set a non-zero
exit code; locate the check around the TESTS variable and KEY_PATH references
and wrap the file existence tests in an if ... then ... else ... fi (or use &&
to gate the echo and TESTS increment) so the PASS is conditional on the test
passing.
In `@action.yml`:
- Around line 417-432: The state file write currently uses echo
"SSH_HOST=${SSH_HOST}" which allows newlines in SSH_HOST to break the key=value
format; before writing the state in the run block replace/newline-normalize
SSH_HOST value (and similarly KNOWN_HOSTS if needed) by converting newlines to
spaces (or another single-token separator) and then write KEY_PATH,
WRAPPER_PATH, the normalized SSH_HOST, and KNOWN_HOSTS_CREATED to
~/.cloudflared-ssh-state so downstream/cleanup grep logic reads a single-line
SSH_HOST value; update the code that emits SSH_HOST in that run block to use the
normalized variable instead of ${SSH_HOST}.
In `@CHANGELOG.md`:
- Around line 24-26: There are two duplicate "### Added" headings in the v1.1.0
changelog; merge them by moving the bullet "Social preview image
(`assets/social-preview.svg`, 1280x640) for GitHub repository card" under the
existing "### Added" section for v1.1.0 and remove the extra "### Added" heading
so there's a single consolidated Added section for that release.
In `@cleanup/action.yml`:
- Around line 61-85: Add glob expansion protection around the SSH host iteration
in action.yml: before building HOST_LIST/entering the "for entry in $HOST_LIST"
loop (which processes SSH_HOST_RAW and uses ENTRY_HOST), invoke set -f to
disable globbing and after the loop restore behavior with set +f; ensure these
changes mirror the existing pattern used elsewhere so the for-loop over
$HOST_LIST is executed with globbing disabled.
---
Nitpick comments:
In `@scripts/_gen_social_preview.py`:
- Around line 23-35: Before calling SRC.read_bytes() verify the SVG exists and
is a file: check SRC.exists() and SRC.is_file() (or equivalent) and if missing
raise SystemExit with a clear message including the SRC path; update the top of
the block where SRC.read_bytes() is used (references: variable SRC and the
save/OUT logic) to perform this guard so the script exits with a helpful error
instead of a raw FileNotFoundError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 121a313d-3447-4a5f-b132-0946bdd9ecef
⛔ Files ignored due to path filters (2)
assets/social-preview.pngis excluded by!**/*.pngassets/social-preview.svgis excluded by!**/*.svg
📒 Files selected for processing (11)
.github/workflows/ci.yml.github/workflows/test.yml.github/workflows/unit-tests.yml.gitignoreCHANGELOG.mdGUIDE.mdREADME.mdSECURITY.mdaction.ymlcleanup/action.ymlscripts/_gen_social_preview.py
📜 Review details
🧰 Additional context used
🪛 markdownlint-cli2 (0.22.1)
CHANGELOG.md
[warning] 24-24: Multiple headings with the same content
(MD024, no-duplicate-heading)
🔇 Additional comments (26)
.github/workflows/ci.yml (1)
74-90: LGTM!action.yml (7)
1-88: LGTM!
100-127: LGTM!
138-149: LGTM!
194-266: LGTM!
268-321: LGTM!
323-398: LGTM!
400-408: LGTM!cleanup/action.yml (3)
1-29: LGTM!
31-59: LGTM!
87-97: LGTM!README.md (4)
1-51: LGTM!
52-60: LGTM!
63-99: LGTM!
100-131: LGTM!GUIDE.md (5)
134-160: LGTM!
366-419: LGTM!
421-472: LGTM!
495-495: LGTM!
188-188: ⚡ Quick winNo action needed —
actions/checkout@v6exists as a valid release tag.CHANGELOG.md (2)
9-23: LGTM!
27-41: LGTM!SECURITY.md (2)
33-35: LGTM!
41-41: LGTM!.gitignore (1)
40-43: LGTM!scripts/_gen_social_preview.py (1)
1-16: LGTM!
Comment on lines
+67
to
75
| - name: Verify outputs | ||
| env: | ||
| CF_VERSION: ${{ steps.setup-ssh.outputs.cloudflared-version }} | ||
| TEST_RESULT: ${{ steps.setup-ssh.outputs.connection-test-result }} | ||
| run: | | ||
| echo "cloudflared-version: $CF_VERSION" | ||
| echo "connection-test-result: $TEST_RESULT" | ||
| [ -n "$CF_VERSION" ] || { echo "FAIL: version output empty"; exit 1; } | ||
|
|
There was a problem hiding this comment.
Assert connection-test-result output, not just print it.
This step currently validates only cloudflared-version; if output wiring for connection-test-result breaks, this workflow won’t catch it.
Suggested fix
echo "cloudflared-version: $CF_VERSION"
echo "connection-test-result: $TEST_RESULT"
[ -n "$CF_VERSION" ] || { echo "FAIL: version output empty"; exit 1; }
+ [ -n "$TEST_RESULT" ] || { echo "FAIL: connection-test-result output empty"; exit 1; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Verify outputs | |
| env: | |
| CF_VERSION: ${{ steps.setup-ssh.outputs.cloudflared-version }} | |
| TEST_RESULT: ${{ steps.setup-ssh.outputs.connection-test-result }} | |
| run: | | |
| echo "cloudflared-version: $CF_VERSION" | |
| echo "connection-test-result: $TEST_RESULT" | |
| [ -n "$CF_VERSION" ] || { echo "FAIL: version output empty"; exit 1; } | |
| - name: Verify outputs | |
| env: | |
| CF_VERSION: ${{ steps.setup-ssh.outputs.cloudflared-version }} | |
| TEST_RESULT: ${{ steps.setup-ssh.outputs.connection-test-result }} | |
| run: | | |
| echo "cloudflared-version: $CF_VERSION" | |
| echo "connection-test-result: $TEST_RESULT" | |
| [ -n "$CF_VERSION" ] || { echo "FAIL: version output empty"; exit 1; } | |
| [ -n "$TEST_RESULT" ] || { echo "FAIL: connection-test-result output empty"; exit 1; } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/test.yml around lines 67 - 75, The "Verify outputs" step
only asserts CF_VERSION and merely prints TEST_RESULT; add an assertion that the
TEST_RESULT output is present and/or equal to the expected value (e.g.,
non-empty or "success") so wiring breaks fail the job; update the run block to
check [ -n "$TEST_RESULT" ] (or compare "$TEST_RESULT" to the expected token)
and exit non‑zero on failure, referencing the CF_VERSION and TEST_RESULT env
names in the check.
|
|
||
| { | ||
| printf 'Host %s\n' "$SSH_HOST" | ||
| printf ' StrictHostKeyChecking no\n' |
There was a problem hiding this comment.
Align stale StrictHostKeyChecking fixtures with hardened default.
These fixtures still encode StrictHostKeyChecking no, which weakens the security contract and can hide regressions in the intended no-known-hosts path.
Suggested fix
- printf ' StrictHostKeyChecking no\n'
+ printf ' StrictHostKeyChecking accept-new\n'Also applies to: 450-450, 549-549, 767-767
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/unit-tests.yml at line 418, Replace the insecure fixture
setting "StrictHostKeyChecking no" with the hardened default
"StrictHostKeyChecking accept-new" in the workflow prints so CI no longer
disables host key checking; update each occurrence of the exact string
"StrictHostKeyChecking no" (seen in .github/workflows/unit-tests.yml and the
other reported occurrences) to "StrictHostKeyChecking accept-new" so the
fixtures reflect the intended no-known-hosts behavior without turning off
verification.
Comment on lines
+715
to
+719
| chmod 644 ~/.ssh/known_hosts | ||
|
|
||
| assert_file_exists "known_hosts created" "$HOME/.ssh/known_hosts" | ||
| assert_file_perms "known_hosts has 644" "$HOME/.ssh/known_hosts" "644" | ||
|
|
There was a problem hiding this comment.
Use restrictive known_hosts permissions in tests.
The tests currently assert mode 644; this encodes broader read access than necessary for SSH trust data and can drift from the hardened behavior you want to enforce.
Suggested fix
- chmod 644 ~/.ssh/known_hosts
+ chmod 600 ~/.ssh/known_hosts
...
- assert_file_perms "known_hosts has 644" "$HOME/.ssh/known_hosts" "644"
+ assert_file_perms "known_hosts has 600" "$HOME/.ssh/known_hosts" "600"Also applies to: 1698-1700
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/unit-tests.yml around lines 715 - 719, The tests assert
that ~/.ssh/known_hosts has mode "644", which is too permissive for SSH trust
data; update the assertions to expect a restrictive mode "600" instead. Modify
the calls to assert_file_perms that reference "$HOME/.ssh/known_hosts" (the
occurrences around the current diff and the similar block at the other location)
to use "600" as the expected permission string, ensuring the test enforces
correct SSH file permissions.
Comment on lines
+1919
to
+1922
| TESTS=$((TESTS + 1)) | ||
| # All files already gone, this should not error | ||
| [ ! -f "$KEY_PATH" ] && [ ! -f "$HOME/.cloudflared-ssh" ] && [ ! -f "$HOME/.cloudflared-ssh-state" ] | ||
| echo " PASS: second cleanup is a no-op (no errors)" |
There was a problem hiding this comment.
Fix false-positive pass in “cleanup run twice” check.
The current && chain is followed by an unconditional PASS message, so this can report success even when a file still exists.
Suggested fix
- [ ! -f "$KEY_PATH" ] && [ ! -f "$HOME/.cloudflared-ssh" ] && [ ! -f "$HOME/.cloudflared-ssh-state" ]
- echo " PASS: second cleanup is a no-op (no errors)"
+ if [ ! -f "$KEY_PATH" ] && [ ! -f "$HOME/.cloudflared-ssh" ] && [ ! -f "$HOME/.cloudflared-ssh-state" ]; then
+ echo " PASS: second cleanup is a no-op (no errors)"
+ else
+ echo " FAIL: second cleanup left artifacts"
+ FAILURES=$((FAILURES + 1))
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| TESTS=$((TESTS + 1)) | |
| # All files already gone, this should not error | |
| [ ! -f "$KEY_PATH" ] && [ ! -f "$HOME/.cloudflared-ssh" ] && [ ! -f "$HOME/.cloudflared-ssh-state" ] | |
| echo " PASS: second cleanup is a no-op (no errors)" | |
| TESTS=$((TESTS + 1)) | |
| # All files already gone, this should not error | |
| if [ ! -f "$KEY_PATH" ] && [ ! -f "$HOME/.cloudflared-ssh" ] && [ ! -f "$HOME/.cloudflared-ssh-state" ]; then | |
| echo " PASS: second cleanup is a no-op (no errors)" | |
| else | |
| echo " FAIL: second cleanup left artifacts" | |
| FAILURES=$((FAILURES + 1)) | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/unit-tests.yml around lines 1919 - 1922, The current
cleanup check uses a test ([ ! -f "$KEY_PATH" ] && [ ! -f
"$HOME/.cloudflared-ssh" ] && [ ! -f "$HOME/.cloudflared-ssh-state" ]) but then
always echoes "PASS", causing a false-positive; modify the block so the echo and
increment of TESTS only happen when the test succeeds and otherwise emit a
failure message and set a non-zero exit code; locate the check around the TESTS
variable and KEY_PATH references and wrap the file existence tests in an if ...
then ... else ... fi (or use && to gate the echo and TESTS increment) so the
PASS is conditional on the test passing.
Comment on lines
+417
to
+432
| shell: bash | ||
| env: | ||
| SSH_KEY_PATH: ${{ inputs.ssh-key-path }} | ||
| SSH_HOST: ${{ inputs.ssh-host }} | ||
| KNOWN_HOSTS: ${{ inputs.known-hosts }} | ||
| run: | | ||
| echo "Testing SSH: ${SSH_USER}@${SSH_HOST}" | ||
| ssh -o BatchMode=yes \ | ||
| "${SSH_USER}@${SSH_HOST}" \ | ||
| "echo 'cloudflare-tunnel-ssh-action: connection verified'" | ||
| echo "Connection test passed." | ||
| KEY_PATH="${SSH_KEY_PATH/#\~/$HOME}" | ||
| { | ||
| echo "KEY_PATH=${KEY_PATH}" | ||
| echo "WRAPPER_PATH=${HOME}/.cloudflared-ssh" | ||
| echo "SSH_HOST=${SSH_HOST}" | ||
| if [ -n "$KNOWN_HOSTS" ]; then | ||
| echo "KNOWN_HOSTS_CREATED=true" | ||
| fi | ||
| } > ~/.cloudflared-ssh-state | ||
| echo "State file written to ~/.cloudflared-ssh-state" |
There was a problem hiding this comment.
Newline-separated hosts corrupt the state file format.
When SSH_HOST contains newlines (as allowed per the input description), echo "SSH_HOST=${SSH_HOST}" produces a multi-line output that breaks the key=value format. The cleanup action's grep would only capture the first host.
Example: Input host1\nhost2 produces:
SSH_HOST=host1
host2
Proposed fix: normalize newlines to spaces before writing
{
echo "KEY_PATH=${KEY_PATH}"
echo "WRAPPER_PATH=${HOME}/.cloudflared-ssh"
- echo "SSH_HOST=${SSH_HOST}"
+ echo "SSH_HOST=$(echo "$SSH_HOST" | tr '\n' ' ')"
if [ -n "$KNOWN_HOSTS" ]; then
echo "KNOWN_HOSTS_CREATED=true"
fi
} > ~/.cloudflared-ssh-state📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| shell: bash | |
| env: | |
| SSH_KEY_PATH: ${{ inputs.ssh-key-path }} | |
| SSH_HOST: ${{ inputs.ssh-host }} | |
| KNOWN_HOSTS: ${{ inputs.known-hosts }} | |
| run: | | |
| echo "Testing SSH: ${SSH_USER}@${SSH_HOST}" | |
| ssh -o BatchMode=yes \ | |
| "${SSH_USER}@${SSH_HOST}" \ | |
| "echo 'cloudflare-tunnel-ssh-action: connection verified'" | |
| echo "Connection test passed." | |
| KEY_PATH="${SSH_KEY_PATH/#\~/$HOME}" | |
| { | |
| echo "KEY_PATH=${KEY_PATH}" | |
| echo "WRAPPER_PATH=${HOME}/.cloudflared-ssh" | |
| echo "SSH_HOST=${SSH_HOST}" | |
| if [ -n "$KNOWN_HOSTS" ]; then | |
| echo "KNOWN_HOSTS_CREATED=true" | |
| fi | |
| } > ~/.cloudflared-ssh-state | |
| echo "State file written to ~/.cloudflared-ssh-state" | |
| shell: bash | |
| env: | |
| SSH_KEY_PATH: ${{ inputs.ssh-key-path }} | |
| SSH_HOST: ${{ inputs.ssh-host }} | |
| KNOWN_HOSTS: ${{ inputs.known-hosts }} | |
| run: | | |
| KEY_PATH="${SSH_KEY_PATH/#\~/$HOME}" | |
| { | |
| echo "KEY_PATH=${KEY_PATH}" | |
| echo "WRAPPER_PATH=${HOME}/.cloudflared-ssh" | |
| echo "SSH_HOST=$(echo "$SSH_HOST" | tr '\n' ' ')" | |
| if [ -n "$KNOWN_HOSTS" ]; then | |
| echo "KNOWN_HOSTS_CREATED=true" | |
| fi | |
| } > ~/.cloudflared-ssh-state | |
| echo "State file written to ~/.cloudflared-ssh-state" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@action.yml` around lines 417 - 432, The state file write currently uses echo
"SSH_HOST=${SSH_HOST}" which allows newlines in SSH_HOST to break the key=value
format; before writing the state in the run block replace/newline-normalize
SSH_HOST value (and similarly KNOWN_HOSTS if needed) by converting newlines to
spaces (or another single-token separator) and then write KEY_PATH,
WRAPPER_PATH, the normalized SSH_HOST, and KNOWN_HOSTS_CREATED to
~/.cloudflared-ssh-state so downstream/cleanup grep logic reads a single-line
SSH_HOST value; update the code that emits SSH_HOST in that run block to use the
normalized variable instead of ${SSH_HOST}.
Comment on lines
+61
to
+85
| # Remove Host entries from SSH config | ||
| if [ -f ~/.ssh/config ] && [ -n "$SSH_HOST_RAW" ]; then | ||
| HOST_LIST=$(echo "$SSH_HOST_RAW" | tr '\n' ' ') | ||
| # shellcheck disable=SC2086 | ||
| for entry in $HOST_LIST; do | ||
| [ -z "$entry" ] && continue | ||
| if [[ "$entry" == *@* ]]; then | ||
| ENTRY_HOST="${entry#*@}" | ||
| else | ||
| ENTRY_HOST="$entry" | ||
| fi | ||
|
|
||
| echo "Removing SSH config entry for: $ENTRY_HOST" | ||
| # Use awk to remove the Host block and all its indented directives | ||
| awk -v host="$ENTRY_HOST" ' | ||
| /^Host / { if ($2 == host) { skip=1; next } else { skip=0 } } | ||
| skip && /^[[:space:]]/ { next } | ||
| skip { skip=0 } | ||
| { print } | ||
| ' ~/.ssh/config > ~/.ssh/config.tmp | ||
| mv ~/.ssh/config.tmp ~/.ssh/config | ||
| done | ||
| chmod 600 ~/.ssh/config | ||
| echo "Cleaned SSH config entries" | ||
| fi |
There was a problem hiding this comment.
Missing glob expansion protection in host iteration loop.
The main action.yml consistently uses set -f before iterating over hosts (lines 230, 308, 351) but the cleanup action omits it. While glob characters in hostnames are unlikely, this is inconsistent with the security hardening applied elsewhere.
Proposed fix: add set -f/+f around the loop
if [ -f ~/.ssh/config ] && [ -n "$SSH_HOST_RAW" ]; then
HOST_LIST=$(echo "$SSH_HOST_RAW" | tr '\n' ' ')
+ set -f # Disable glob expansion
# shellcheck disable=SC2086
for entry in $HOST_LIST; do
...
done
+ set +f
chmod 600 ~/.ssh/config
echo "Cleaned SSH config entries"
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Remove Host entries from SSH config | |
| if [ -f ~/.ssh/config ] && [ -n "$SSH_HOST_RAW" ]; then | |
| HOST_LIST=$(echo "$SSH_HOST_RAW" | tr '\n' ' ') | |
| # shellcheck disable=SC2086 | |
| for entry in $HOST_LIST; do | |
| [ -z "$entry" ] && continue | |
| if [[ "$entry" == *@* ]]; then | |
| ENTRY_HOST="${entry#*@}" | |
| else | |
| ENTRY_HOST="$entry" | |
| fi | |
| echo "Removing SSH config entry for: $ENTRY_HOST" | |
| # Use awk to remove the Host block and all its indented directives | |
| awk -v host="$ENTRY_HOST" ' | |
| /^Host / { if ($2 == host) { skip=1; next } else { skip=0 } } | |
| skip && /^[[:space:]]/ { next } | |
| skip { skip=0 } | |
| { print } | |
| ' ~/.ssh/config > ~/.ssh/config.tmp | |
| mv ~/.ssh/config.tmp ~/.ssh/config | |
| done | |
| chmod 600 ~/.ssh/config | |
| echo "Cleaned SSH config entries" | |
| fi | |
| # Remove Host entries from SSH config | |
| if [ -f ~/.ssh/config ] && [ -n "$SSH_HOST_RAW" ]; then | |
| HOST_LIST=$(echo "$SSH_HOST_RAW" | tr '\n' ' ') | |
| set -f # Disable glob expansion | |
| # shellcheck disable=SC2086 | |
| for entry in $HOST_LIST; do | |
| [ -z "$entry" ] && continue | |
| if [[ "$entry" == *@* ]]; then | |
| ENTRY_HOST="${entry#*@}" | |
| else | |
| ENTRY_HOST="$entry" | |
| fi | |
| echo "Removing SSH config entry for: $ENTRY_HOST" | |
| # Use awk to remove the Host block and all its indented directives | |
| awk -v host="$ENTRY_HOST" ' | |
| /^Host / { if ($2 == host) { skip=1; next } else { skip=0 } } | |
| skip && /^[[:space:]]/ { next } | |
| skip { skip=0 } | |
| { print } | |
| ' ~/.ssh/config > ~/.ssh/config.tmp | |
| mv ~/.ssh/config.tmp ~/.ssh/config | |
| done | |
| set +f | |
| chmod 600 ~/.ssh/config | |
| echo "Cleaned SSH config entries" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cleanup/action.yml` around lines 61 - 85, Add glob expansion protection
around the SSH host iteration in action.yml: before building HOST_LIST/entering
the "for entry in $HOST_LIST" loop (which processes SSH_HOST_RAW and uses
ENTRY_HOST), invoke set -f to disable globbing and after the loop restore
behavior with set +f; ensure these changes mirror the existing pattern used
elsewhere so the for-loop over $HOST_LIST is executed with globbing disabled.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Closes #
Type of Change
Checklist
actionlintpassesshellcheckSummary by CodeRabbit
New Features
retry-countandretry-delayfor resilient connections.known-hostssupport for strict SSH host key checking.user@hostoverrides.ssh-extra-configinput for custom SSH directives.cloudflared-versionandconnection-test-result.Documentation