[Fix] Confine site env path reads and gate raw secrets on write access - #1219
Open
RichardAnderson wants to merge 5 commits into
Open
[Fix] Confine site env path reads and gate raw secrets on write access#1219RichardAnderson wants to merge 5 commits into
RichardAnderson wants to merge 5 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughEnvironment retrieval and editing now support validated paths, permission-based secret disclosure, raw and structured modes, improved parsing, safer SSH commands, stable variable rows, and expanded API and test coverage. ChangesEnvironment access and editing
Frontend formatting cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Editor
participant ApplicationController
participant EnvActions
participant Site
participant SSHOS
Editor->>ApplicationController: request environment path
ApplicationController->>Site: authorise and resolve path
ApplicationController->>EnvActions: load, parse, or stringify environment
EnvActions->>SSHOS: read or write environment file
SSHOS-->>EnvActions: file content or command result
EnvActions-->>Editor: masked or editable environment data
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Actions/Site/UpdateEnv.php (1)
47-56: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winWrap the two database writes in a transaction.
$site->save()and$site->jsonUpdate('type_data', 'env_path', $path)are separate writes; a failure between them leaves the secret-key list updated whileenv_pathstill points at the previous file, so subsequent reads and secret restoration target the wrong path. The remote write can't be rolled back, but the DB state should be atomic.♻️ Proposed fix
- $site->env_variables = $this->secretKeys($variables); - $site->save(); - - $site->jsonUpdate('type_data', 'env_path', $path); + DB::transaction(function () use ($site, $variables, $path): void { + $site->env_variables = $this->secretKeys($variables); + $site->save(); + + $site->jsonUpdate('type_data', 'env_path', $path); + });As per coding guidelines, "Wrap multi-step mutations in
DB::transaction()".🤖 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 `@app/Actions/Site/UpdateEnv.php` around lines 47 - 56, Wrap the database mutations in UpdateEnv’s action flow—$site->save() and $site->jsonUpdate('type_data', 'env_path', $path)—inside a DB::transaction() callback so env_variables and env_path commit atomically. Keep the remote server write outside the transaction, and preserve the existing values and ordering within the transaction.Source: Coding guidelines
🤖 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 `@app/Actions/Site/ParseEnv.php`:
- Around line 16-18: Update the validation rules in ParseEnv’s Validator::make
call to add a maximum length constraint for content, matching StringifyEnv’s
existing max:1000 limit while preserving the present, nullable, and string
rules.
In `@app/Helpers/SSH.php`:
- Around line 302-305: Update the remote write command in the SSH helper’s
asUser()->exec flow to always remove tmpRemotePath when cat or redirection
fails, while preserving the original command’s exit status. Add a regression
test covering failed writes and verifying temporary-file cleanup.
In `@app/SSH/OS/OS.php`:
- Around line 153-164: Update InstallServer#createUser(), the sole non-test
caller of OS#getPublicKey(), to catch the expected SSHCommandError from
unreadable public keys and mark the installation job as an intended failure.
Preserve the existing failure handling flow and prevent the raw command
exception from being exposed to tenants.
In `@resources/js/pages/application/components/env-variable-row.tsx`:
- Around line 89-96: Update the Input in the environment variable row so the
read-only explanation for isMultiLine is exposed through an always-visible title
or aria-description rather than placeholder text. Preserve the existing readOnly
behavior and retain the normal placeholder for editable values.
In `@resources/js/pages/application/components/env.tsx`:
- Around line 275-287: Extract the repeated parsed-to-variable mapping into a
shared toVariables(parsed) helper near the component utilities, preserving id
generation via rowId(), isSecret from v.is_secret, and isNew: true. Replace the
duplicated parsed.map(...) logic in the upload handler, paste handler, and
switchMode with this helper so all three entry points use the same
transformation.
- Around line 119-149: Move editor state synchronization out of the queryFn in
the application environment query and into an effect driven by query.data (or
derive the values), so cached path switches restore the data for the current
committedPath. Update variables, rawContent, canEdit, and the clean-state flag
from query.data, then include query.isFetching in the busy condition used by the
Save button to prevent saves during refetches.
In `@resources/views/ssh/os/read-file.blade.php`:
- Line 1: The read-file template must enforce shell quoting for every caller. In
resources/views/ssh/os/read-file.blade.php at lines 1-1, apply
escapeshellarg($path) to both path interpolations; in app/SSH/OS/OS.php at lines
258-258, remove call-site escaping and pass the raw path, and make the same
change in getPublicKey() at line 155 so both callers rely on the template.
In `@tests/Unit/SiteEnvPathTest.php`:
- Around line 10-20: Update SiteEnvPathTest to use the repository’s PHPUnit
fixture conventions: include the RefreshDatabase trait and replace the direct
Site construction in the site() helper with Site::factory()->make(...),
preserving the configured path and optional env_path test data.
---
Outside diff comments:
In `@app/Actions/Site/UpdateEnv.php`:
- Around line 47-56: Wrap the database mutations in UpdateEnv’s action
flow—$site->save() and $site->jsonUpdate('type_data', 'env_path', $path)—inside
a DB::transaction() callback so env_variables and env_path commit atomically.
Keep the remote server write outside the transaction, and preserve the existing
values and ordering within the transaction.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c50f0f2f-929d-45b2-a452-e9b801e5c9d6
📒 Files selected for processing (30)
app/Actions/Site/GetEnv.phpapp/Actions/Site/ParseEnv.phpapp/Actions/Site/StringifyEnv.phpapp/Actions/Site/UpdateEnv.phpapp/Helpers/EnvParser.phpapp/Helpers/SSH.phpapp/Http/Controllers/API/SiteController.phpapp/Http/Controllers/ApplicationController.phpapp/Models/Site.phpapp/Policies/SitePolicy.phpapp/SSH/OS/OS.phpapp/Support/Testing/SSHFake.phppublic/api-docs/openapi/sites.yamlresources/js/lib/utils.tsresources/js/pages/application/components/env-variable-row.tsxresources/js/pages/application/components/env.tsxresources/js/pages/backups/files.tsxresources/js/pages/backups/index.tsxresources/js/pages/redirects/components/edit-redirect.tsxresources/js/pages/redirects/components/redirect-form-fields.tsxresources/js/pages/site-settings/components/basic-auth.tsxresources/js/pages/workers/components/env-dialog.tsxresources/js/types/env.d.tsresources/views/ssh/os/read-file.blade.phptests/Feature/API/SitesTest.phptests/Feature/ApplicationTest.phptests/Unit/EnvParserTest.phptests/Unit/Jobs/UpdateVitoAgentConfigJobTest.phptests/Unit/SSH/Services/Webserver/DeploySplashTest.phptests/Unit/SiteEnvPathTest.php
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Actions/Site/UpdateEnv.php (1)
34-35: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReject an explicit null raw environment value.
{"env": null}makes$hasEnvtrue whenvariablesis absent, then Lines 49 and 53 overwrite the remote file with an empty string and clear stored secret keys. Treat only a non-nullenvas supplied; retain''as the intentional “clear file” value.Proposed fix
- $hasEnv = array_key_exists('env', $input) - && ($input['env'] !== null || ! array_key_exists('variables', $input)); + $hasEnv = array_key_exists('env', $input) + && $input['env'] !== null;🤖 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 `@app/Actions/Site/UpdateEnv.php` around lines 34 - 35, The $hasEnv condition in UpdateEnv must treat an explicit null env as absent while preserving an empty string as supplied; require the env value itself to be non-null and retain the existing variables-based handling so null does not overwrite the remote file or clear stored secret keys.
🤖 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 `@resources/js/pages/application/components/env.tsx`:
- Around line 475-481: Update the Save control’s disabled condition in the form
component to include `canEdit !== true`, ensuring read-only users cannot
initiate the mutation even when the response is non-empty. Preserve all existing
processing, busy, error, path, duplicate, and empty-variable checks.
In `@resources/views/ssh/os/write-file.blade.php`:
- Around line 1-6: Update the shell flow in the write-file template so cat
writes to a unique temporary file beside the destination rather than directly to
the destination; rename that temporary file to the destination only after cat
succeeds, and clean up both temporary files on success or failure while
preserving the existing destination when copying fails.
In `@tests/Unit/Helpers/SSHTest.php`:
- Around line 13-22: Update
test_write_removes_the_temporary_file_on_both_the_success_and_failure_branches
to configure SSH::fake or the rendered script execution so the remote cat
command fails, then assert that the write operation propagates the exception and
removes the temporary file. Keep the existing success-path coverage while adding
assertions that verify actual failure cleanup rather than only inspecting
generated script text.
---
Outside diff comments:
In `@app/Actions/Site/UpdateEnv.php`:
- Around line 34-35: The $hasEnv condition in UpdateEnv must treat an explicit
null env as absent while preserving an empty string as supplied; require the env
value itself to be non-null and retain the existing variables-based handling so
null does not overwrite the remote file or clear stored secret keys.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c77b174c-13c5-470f-aff0-1a2ddb76b766
📒 Files selected for processing (10)
app/Actions/Site/UpdateEnv.phpapp/Helpers/SSH.phpapp/SSH/OS/OS.phpresources/js/pages/application/components/env-variable-row.tsxresources/js/pages/application/components/env.tsxresources/views/ssh/os/read-file.blade.phpresources/views/ssh/os/write-file.blade.phptests/Unit/Helpers/SSHTest.phptests/Unit/Jobs/UpdateVitoAgentConfigJobTest.phptests/Unit/SiteEnvPathTest.php
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Unit/Helpers/SSHTest.php (1)
54-61: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCover quote and shell-metacharacter paths.
This regression test only exercises a space. Add a destination containing at least a single quote plus characters such as
;,$()or backticks, then assert the complete escaped shell argument. This is required by the PR objective to cover shell-significant input, not just whitespace.Suggested test case
- $this->server->ssh()->write('/home/vito/legacy path/.env', 'APP_NAME=TestApp', 'vito'); + $remotePath = "/home/vito/legacy'; touch /tmp/vito-owned `#/.env`"; + $this->server->ssh()->write($remotePath, 'APP_NAME=TestApp', 'vito'); - SSH::assertExecutedContains("> '/home/vito/legacy path/.env'"); + SSH::assertExecutedContains('> '.escapeshellarg($remotePath));🤖 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 `@tests/Unit/Helpers/SSHTest.php` around lines 54 - 61, Update test_write_quotes_the_destination_path to use a destination containing a single quote and shell metacharacters such as ;, $(), or backticks, then assert the complete correctly escaped shell argument in SSH::assertExecutedContains. Preserve the existing write invocation and verify the entire destination remains safely quoted.
🤖 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 `@tests/Unit/Helpers/SSHTest.php`:
- Around line 22-27: Update the SSH test method around the write failure
assertion to use PHPUnit’s expected-exception handling instead of an explicit
try/catch with an inline comment. Preserve the cleanup assertion by keeping it
in a finally block or equivalent teardown path, and ensure the PHP method body
contains no inline comments.
---
Outside diff comments:
In `@tests/Unit/Helpers/SSHTest.php`:
- Around line 54-61: Update test_write_quotes_the_destination_path to use a
destination containing a single quote and shell metacharacters such as ;, $(),
or backticks, then assert the complete correctly escaped shell argument in
SSH::assertExecutedContains. Preserve the existing write invocation and verify
the entire destination remains safely quoted.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b1b5be18-eb63-41bb-a3d5-515720fc61c9
📒 Files selected for processing (2)
resources/js/pages/application/components/env.tsxtests/Unit/Helpers/SSHTest.php
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #1218 by hardening site environment file reads/writes against path injection and tightening permissions around raw .env access and secret value visibility across both web and API surfaces.
Changes:
- Confines caller-supplied env-path overrides to the site directory (with validation) and shell-escapes paths at the SSH command boundary.
- Restricts raw
.envcontent + unmasked secret values to callers with write access (andwriteability for API tokens), adding UI support for classic/raw editing and secret revealability. - Improves env parsing/stringifying robustness and extends test coverage for security and edge cases.
Reviewed changes
Copilot reviewed 31 out of 32 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Unit/SSH/Services/Webserver/DeploySplashTest.php | Updates expectations for quoted write redirections. |
| tests/Unit/SiteEnvPathTest.php | Adds unit coverage for env path confinement/validation behavior. |
| tests/Unit/Jobs/UpdateVitoAgentConfigJobTest.php | Updates assertions to match new quoting/cleanup behavior. |
| tests/Unit/Helpers/SSHTest.php | Adds coverage for write failure propagation and temp cleanup. |
| tests/Unit/EnvParserTest.php | Expands coverage for parsing/stringifying and representability checks. |
| tests/Feature/ApplicationTest.php | Adds feature coverage for path override gating and secret reveal rules. |
| tests/Feature/API/SitesTest.php | Verifies env payload differs for read vs write-scoped tokens. |
| resources/views/ssh/os/write-file.blade.php | New SSH template for safer writes + cleanup on both branches. |
| resources/views/ssh/os/read-file.blade.php | Escapes path arguments to prevent command injection. |
| resources/js/types/env.d.ts | Adds stable id for env rows in UI state. |
| resources/js/pages/workers/components/env-dialog.tsx | Uses stable row ids for env-variable rendering. |
| resources/js/pages/site-settings/components/basic-auth.tsx | Reuses shared rowId() utility. |
| resources/js/pages/redirects/components/redirect-form-fields.tsx | Minor formatting-only change. |
| resources/js/pages/redirects/components/edit-redirect.tsx | Minor formatting-only change. |
| resources/js/pages/backups/index.tsx | Minor formatting-only change. |
| resources/js/pages/backups/files.tsx | Minor formatting-only change. |
| resources/js/pages/application/components/env.tsx | Adds classic/raw editor mode, path “commit” semantics, and can-edit gating. |
| resources/js/pages/application/components/env-variable-row.tsx | Adds revealability + safer editing UX for multiline/secret values. |
| resources/js/lib/utils.ts | Introduces shared rowId() helper. |
| public/api-docs/openapi/sites.yaml | Updates OpenAPI to reflect gated raw env + new error response. |
| app/Support/Testing/SSHFake.php | Adds exec-failure simulation for SSH fake. |
| app/SSH/OS/OS.php | Throws on empty public key reads to surface failures. |
| app/Policies/SitePolicy.php | Adds revealEnv policy that also checks token ability. |
| app/Models/Site.php | Adds resolveEnvPath() and routes env reads through it. |
| app/Http/Controllers/ApplicationController.php | Gates env overrides and adds parse/stringify endpoints. |
| app/Http/Controllers/API/SiteController.php | Gates raw env/secret reveal via revealEnv policy. |
| app/Helpers/SSH.php | Switches write to template-based approach with quoting and cleanup. |
| app/Helpers/EnvParser.php | Improves escape handling + adds representability checks. |
| app/Actions/Site/UpdateEnv.php | Validates inputs, resolves confined paths, and supports classic vs variables update flows. |
| app/Actions/Site/StringifyEnv.php | New action to stringify variables into dotenv content. |
| app/Actions/Site/ParseEnv.php | New action to parse dotenv and report representability. |
| app/Actions/Site/GetEnv.php | Returns masked variables by default; raw env only when reveal is allowed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/Support/Testing/SSHFake.php (1)
97-102: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMake the fake observe the upload permission.
SSHFake::upload()accepts$permissionbut discards it, so tests cannot detect if the production code stops passing the required600mode. Store the value and expose an assertion or getter for it.🤖 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 `@app/Support/Testing/SSHFake.php` around lines 97 - 102, Update SSHFake::upload() to retain the passed $permission value, and add a getter or assertion method exposing the recorded permission so tests can verify uploads use the required 600 mode.app/Helpers/SSH.php (1)
260-275: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winValidate and shell-escape
$permissionbefore interpolation.Line 260 introduces a free-form permission argument, which is inserted directly into
chmodcommands at Lines 270 and 275. A value such as600; <command>would alter the remote shell command. Restrict the value to a valid octal mode and quote it before execution.Proposed fix
public function upload(string $local, string $remote, ?string $owner = null, ?string $log = null, ?int $siteId = null, string $permission = '644'): void { + if (! preg_match('/\A[0-7]{3,4}\z/', $permission)) { + throw new \InvalidArgumentException('Invalid file permission.'); + } + + $permission = escapeshellarg($permission);🤖 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 `@app/Helpers/SSH.php` around lines 260 - 275, Update SSH::upload to validate $permission as a valid octal mode and shell-escape it before interpolating it into the chmod commands executed via $this->exec. Apply the protected value consistently to both chmod invocations while preserving the existing upload and ownership behavior.
🤖 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 `@app/Facades/SSH.php`:
- Line 20: Update the `upload` facade method annotation in `SSH` to document a
`void` return type instead of `string`, matching both implementations and the
actual caller contract.
In `@tests/Feature/ApplicationTest.php`:
- Around line 1486-1501: Add a regression test alongside
test_update_env_rejects_a_submission_with_both_keys that submits env as null
with a non-empty variables array, then assert the application.update-env request
succeeds and is not rejected by env validation. Reuse the existing
authentication, SSH fake, server, site, and variable setup while changing only
the payload needed to cover the hasEnv special case.
---
Outside diff comments:
In `@app/Helpers/SSH.php`:
- Around line 260-275: Update SSH::upload to validate $permission as a valid
octal mode and shell-escape it before interpolating it into the chmod commands
executed via $this->exec. Apply the protected value consistently to both chmod
invocations while preserving the existing upload and ownership behavior.
In `@app/Support/Testing/SSHFake.php`:
- Around line 97-102: Update SSHFake::upload() to retain the passed $permission
value, and add a getter or assertion method exposing the recorded permission so
tests can verify uploads use the required 600 mode.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 29a4d396-e593-412c-89a8-02e02208ee96
📒 Files selected for processing (5)
app/Actions/Site/UpdateEnv.phpapp/Facades/SSH.phpapp/Helpers/SSH.phpapp/Support/Testing/SSHFake.phptests/Feature/ApplicationTest.php
saeedvaziry
approved these changes
Jul 29, 2026
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
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.
Closes #1218 by confining the env query parameter to the site's own directory and shell-escaping it before sudo cat — the unescaped path made this command injection as a root-capable user, not just the reported arbitrary file read - and restricts raw .env content and unmasked secret values to callers with write access on both the web and API endpoints, while adding a write-access-only Classic mode raw editor and revealable saved secrets.
Summary by CodeRabbit
.envcontent and real secret values to authorised users..envand secret values; reveal/edit controls honour revealability.