Fix env secrets being lost after pulling from server and saving - #1177
Merged
Conversation
Contributor
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRefactors env secret handling to store only secret key names instead of full variable metadata, and processes all env updates through the live server ChangesEnv Secret Handling via Live Server File
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🤖 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/UpdateEnv.php`:
- Around line 66-67: The `existingVariables()` method at line 66 silently falls
back to database values when the live `.env` read fails (returning an empty
string on SSHError instead of throwing). This allows the logic at lines 77-83 to
restore stale masked secrets from the database, creating a security risk. Modify
`$site->getEnv()` to throw an exception on SSH errors instead of returning an
empty string, and ensure `existingVariables()` does not catch or suppress this
exception. This will allow the SSH error to bubble up to the caller and prevent
the silent fallback to stale database state.
In `@tests/Feature/ApplicationTest.php`:
- Around line 637-654: Remove the three inline comments from within the test
method body (the comments starting with "// Save a secret through Vito...", "//
The secret is rotated directly on the server...", and "// User pulls the env,
changes a non-secret field..."). Either move this documentation to the PHPDoc
block at the method level or remove it entirely to keep the test body code-only,
following the repository's PHP coding standard that prohibits inline comments
inside function/method bodies except for migrations.
🪄 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: efee6336-9082-413b-9315-0b101facada2
📒 Files selected for processing (6)
app/Actions/Site/UpdateEnv.phpapp/Helpers/EnvParser.phpapp/Http/Controllers/API/SiteController.phpapp/Http/Controllers/ApplicationController.phptests/Feature/ApplicationTest.phptests/Unit/EnvParserTest.php
saeedvaziry
marked this pull request as draft
June 19, 2026 21:30
saeedvaziry
marked this pull request as ready for review
June 20, 2026 07:44
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/UpdateEnv.php`:
- Around line 82-84: The issue is that Site::getEnv() silently catches SSHError
exceptions and returns an empty string, which causes guardAgainstWipingSecrets()
to fail protecting masked secrets like APP_KEY when the live read fails. Replace
the call to getEnv() with a method that does not swallow exceptions and allows
errors to bubble up (following the coding guideline of not silently suppressing
provider/service errors), or alternatively broaden the
guardAgainstWipingSecrets() method to detect and block every empty incoming
secret value when the live read is unavailable. Apply this same fix to both the
location at lines 82-84 and the similar code mentioned at lines 132-139.
- Around line 44-46: The write operation in UpdateEnv.php is re-stringifying
parsed environment variables using EnvParser::stringify($variables), which loses
the original formatting including comments, blank lines, and ordering. Instead
of stringifying the parsed variables at Line 46, preserve and write the raw
submitted .env content directly. Use the original raw environment submission
data (that is being parsed at Lines 105-109) when calling the server's write
method, rather than reconstructing it from the parsed variables array, so that
all original formatting and unparsed content remains intact.
In `@app/Http/Controllers/API/SiteController.php`:
- Around line 160-162: Extract the env display pipeline (the sequence of
EnvParser::parse, EnvParser::classify, and EnvParser::maskSecrets) from the
SiteController method into a new Action class. Create an Action (e.g.,
ProcessSiteEnvironmentVariables or similar) that accepts the raw env string and
site env_variables, performs the three-step transformation, and returns the
result. Then replace the three-line EnvParser chain in the controller with a
single call to this new Action, ensuring the controller only handles the HTTP
request/response while the business logic resides in the dedicated Action class.
🪄 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: bf00530d-5b1d-4bc3-be68-b4a717de80c4
📒 Files selected for processing (10)
app/Actions/Site/UpdateEnv.phpapp/Actions/Worker/UpdateWorkerEnvironment.phpapp/Helpers/EnvParser.phpapp/Http/Controllers/API/SiteController.phpapp/Http/Controllers/ApplicationController.phpapp/Models/Site.phppublic/api-docs/openapi/sites.yamltests/Feature/ApplicationTest.phptests/Feature/WorkerEnvironmentTest.phptests/Unit/EnvParserTest.php
RichardAnderson
approved these changes
Jun 20, 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
2 participants
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.
Make the live server .env file the source of truth when restoring masked secrets on save and pull, so
secrets that drifted out of band are no longer silently overwritten with a stale DB copy.
Summary by CodeRabbit
Release Notes
Refactor
Tests