fix: create parent directory before moving workflow clone#1531
Open
guzalv wants to merge 1 commit intoambient-code:mainfrom
Open
fix: create parent directory before moving workflow clone#1531guzalv wants to merge 1 commit intoambient-code:mainfrom
guzalv wants to merge 1 commit intoambient-code:mainfrom
Conversation
The workflow clone code in both hydrate.sh (init container) and workflow.py (runtime endpoint) failed to create /workspace/workflows/ before moving the cloned repo into it. The subpath-found branch had mkdir -p but the subpath-fallback and no-subpath branches did not. Also fixes: - Path traversal vulnerability in workflow.py subpath handling - Missing chown/chmod for /workspace/workflows in hydrate.sh
✅ Deploy Preview for cheerful-kitten-f556a0 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
📝 WalkthroughWalkthroughAdds path traversal prevention to runtime workflow switching by validating that subpath stays within the temporary clone directory. Consolidates file operations using ChangesSecure Runtime Workflow Switching
🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
components/runners/ambient-runner/ambient_runner/endpoints/workflow.py (2)
47-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact credentials before logging workflow URL
Line 47 logs
git_urldirectly; URLs can carry embedded tokens/userinfo and leak secrets into logs. Log a redacted value.Suggested fix
- logger.info(f"Workflow change request: {git_url}@{branch} (path: {path})") + logger.info( + "Workflow change request: %s@%s (path: %s)", + redact_secrets(git_url), + branch, + path, + )As per coding guidelines,
components/runners/ambient-runner/**/*.py: - Check subprocess handling, timeout management, and that secrets are not logged.🤖 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 `@components/runners/ambient-runner/ambient_runner/endpoints/workflow.py` at line 47, The log call at logger.info(f"Workflow change request: {git_url}@{branch} (path: {path})") exposes potential credentials in git_url; before logging, sanitize git_url by parsing and removing any userinfo (username:password@) or token fragments so only host/path remain, then log the redacted URL along with branch and path (update the logger.info call near the Workflow change request in ambient_runner/endpoints/workflow.py to use the sanitized variable instead of raw git_url).
122-136:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a timeout to
git clonesubprocess wait
await process.communicate()(Line 135) has no timeout, so a hung clone can block this request and hold_workflow_change_lockindefinitely. Wrap it withasyncio.wait_for(...)and kill on timeout.Suggested fix
- stdout, stderr = await process.communicate() + try: + stdout, stderr = await asyncio.wait_for(process.communicate(), timeout=60) + except asyncio.TimeoutError: + process.kill() + await process.communicate() + logger.error("Failed to clone workflow: git clone timed out after 60s") + return False, ""As per coding guidelines,
components/runners/ambient-runner/**/*.py: - Check subprocess handling, timeout management, and that secrets are not logged.🤖 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 `@components/runners/ambient-runner/ambient_runner/endpoints/workflow.py` around lines 122 - 136, The git clone subprocess uses await process.communicate() without a timeout which can hang and hold _workflow_change_lock; wrap the communicate call in asyncio.wait_for with a sensible timeout constant (e.g. CLONE_TIMEOUT) and if wait_for raises asyncio.TimeoutError terminate the subprocess (process.kill(), await process.communicate() to drain) and raise/return a timeout error; apply this change inside the function that spawns the subprocess (the code that creates `process` and calls `communicate`) and ensure you do not log any secrets or the full clone_url when reporting the timeout.components/runners/state-sync/hydrate.sh (1)
327-336:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBlock path traversal in workflow subpath extraction
Line 328 concatenates
WORKFLOW_PATHdirectly, and Line 332 only checks-d;../segments can escape"$WORKFLOW_TEMP"and copy host/container directories (e.g.,/etc) into the workspace. Add canonical-path confinement before copying.Suggested fix
if [ -n "$WORKFLOW_PATH" ]; then - SUBPATH_FULL="$WORKFLOW_TEMP/$WORKFLOW_PATH" - echo " Checking for subpath: $SUBPATH_FULL" - ls -la "$SUBPATH_FULL" 2>&1 || echo " Subpath does not exist" - - if [ -d "$SUBPATH_FULL" ]; then + WORKFLOW_TEMP_REAL="$(cd "$WORKFLOW_TEMP" && pwd -P)" + SUBPATH_FULL="$(cd "$WORKFLOW_TEMP" 2>/dev/null && cd "$WORKFLOW_PATH" 2>/dev/null && pwd -P)" + echo " Checking for subpath: $WORKFLOW_PATH" + + if [ -n "$SUBPATH_FULL" ] && [[ "$SUBPATH_FULL" == "$WORKFLOW_TEMP_REAL"/* ]] && [ -d "$SUBPATH_FULL" ]; then echo " Extracting subpath: $WORKFLOW_PATH" mkdir -p "$(dirname "$WORKFLOW_FINAL")" cp -r "$SUBPATH_FULL" "$WORKFLOW_FINAL" rm -rf "$WORKFLOW_TEMP" echo " ✓ Workflow extracted from subpath to /workspace/workflows/${WORKFLOW_NAME}" else🤖 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 `@components/runners/state-sync/hydrate.sh` around lines 327 - 336, The code currently builds SUBPATH_FULL by concatenating WORKFLOW_PATH into WORKFLOW_TEMP and copies it without ensuring it stays inside the temp dir, allowing path traversal; before using SUBPATH_FULL in the -d check and cp, resolve a canonical path (via realpath or readlink -f) for both WORKFLOW_TEMP and SUBPATH_FULL (e.g., compute WORKFLOW_TEMP_REAL and SUBPATH_REAL), verify SUBPATH_REAL starts with WORKFLOW_TEMP_REAL (string-prefix check) and only proceed to mkdir/cp/rm when that containment check passes; otherwise log an error and abort so WORKFLOW_PATH cannot escape the temp directory.
🤖 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.
Outside diff comments:
In `@components/runners/ambient-runner/ambient_runner/endpoints/workflow.py`:
- Line 47: The log call at logger.info(f"Workflow change request:
{git_url}@{branch} (path: {path})") exposes potential credentials in git_url;
before logging, sanitize git_url by parsing and removing any userinfo
(username:password@) or token fragments so only host/path remain, then log the
redacted URL along with branch and path (update the logger.info call near the
Workflow change request in ambient_runner/endpoints/workflow.py to use the
sanitized variable instead of raw git_url).
- Around line 122-136: The git clone subprocess uses await process.communicate()
without a timeout which can hang and hold _workflow_change_lock; wrap the
communicate call in asyncio.wait_for with a sensible timeout constant (e.g.
CLONE_TIMEOUT) and if wait_for raises asyncio.TimeoutError terminate the
subprocess (process.kill(), await process.communicate() to drain) and
raise/return a timeout error; apply this change inside the function that spawns
the subprocess (the code that creates `process` and calls `communicate`) and
ensure you do not log any secrets or the full clone_url when reporting the
timeout.
In `@components/runners/state-sync/hydrate.sh`:
- Around line 327-336: The code currently builds SUBPATH_FULL by concatenating
WORKFLOW_PATH into WORKFLOW_TEMP and copies it without ensuring it stays inside
the temp dir, allowing path traversal; before using SUBPATH_FULL in the -d check
and cp, resolve a canonical path (via realpath or readlink -f) for both
WORKFLOW_TEMP and SUBPATH_FULL (e.g., compute WORKFLOW_TEMP_REAL and
SUBPATH_REAL), verify SUBPATH_REAL starts with WORKFLOW_TEMP_REAL (string-prefix
check) and only proceed to mkdir/cp/rm when that containment check passes;
otherwise log an error and abort so WORKFLOW_PATH cannot escape the temp
directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a82faba0-f4e5-4c42-9a5a-ccc6e23926c8
📒 Files selected for processing (2)
components/runners/ambient-runner/ambient_runner/endpoints/workflow.pycomponents/runners/state-sync/hydrate.sh
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.
Summary
hydrate.sh(init container) andworkflow.py(runtime endpoint) failed to create/workspace/workflows/before moving the cloned repo into it. The subpath-found branch already hadmkdir -pbut the subpath-fallback and no-subpath branches did not, causingmv/shutil.moveto fail silently.../traversal in user-supplied subpath input.chown/chmodfor/workspace/workflowsalongside the existing/workspace/repospermissions block, so the runner container (user 1001) can read cloned workflow files.Changed files
components/runners/state-sync/hydrate.shmkdir -pbeforemvin subpath-fallback and no-subpath branches; addchown/chmodfor/workspace/workflowscomponents/runners/ambient-runner/ambient_runner/endpoints/workflow.pymkdir -pbeforeshutil.movein subpath-fallback and no-subpath branches; add path traversal validation for subpathTest plan
/workspace/workflows/<name>/../sequences is rejected with an error log/workspace/workflows/has correct ownership (1001:0) after initSummary by CodeRabbit
New Features
Bug Fixes