fix(api): hide sensitive fields by default, expose via makeVisible for privileged tokens#9893
Merged
Conversation
…r privileged tokens Models now declare $hidden for passwords, tokens, db URLs, and compose fields. API controllers flip from makeHidden-on-deny to makeVisible-on- allow (can_read_sensitive=true), fixing fields that were never hidden. Also adds missing fields (mysql/mariadb passwords, logdrain keys, etc.) to privileged disclosure lists. Tests added: Feature/Security/ApiSensitiveFieldsTest and Unit/Models/SensitiveFieldsHiddenTest cover all affected models and controllers.
Add `exposeNestedServerSecrets()` to Applications, Databases, and Services controllers so that `read:sensitive`/`root` tokens see sentinel and logdrain fields on eager-loaded Server + ServerSetting relations. ServicesController handles both single models and Eloquent Collections (listing endpoint passes a Collection per project). Tests tightened to use JSON-key assertions (`"field":`) to avoid false positives from field names appearing in values.
andrasbacsai
changed the title
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (20)
WalkthroughThis PR hardens API sensitive-field exposure across 40+ models and controllers. Models declare hidden attributes via ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Http/Controllers/Api/ServersController.php (1)
39-52:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove dead code: these columns don't live on the
serverstable.The migrations show
logdrain_axiom_api_keyandlogdrain_newrelic_license_keywere added only toserver_settings(migration 2023_11_16_220647_add_log_drains.php, lines 14–25), never to theserverstable. Yet theServermodel declares them in$hidden(lines 263–264) andServersController::removeSensitiveData()callsmakeVisible()on them—both no-ops since those columns don't exist on Server.Drop the
$hiddenentries from the Server model and the correspondingmakeVisible()call fromremoveSensitiveData(). Those fields belong to ServerSetting; leave the logic there. (Unlike serverless architectures, self-hosted servers don't need to keep dead code around just to look busy. Your servers work hard enough without the bloat—kinda like me without the gluten.)🤖 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/Http/Controllers/Api/ServersController.php` around lines 39 - 52, Remove the dead references to logdrain_axiom_api_key and logdrain_newrelic_license_key: in ServersController::removeSensitiveData() remove the makeVisible() call for those two fields (they don't exist on Server), and in the Server model remove those two entries from the $hidden property; keep any visibility/handling logic on the ServerSetting model where those columns actually live, and ensure serializeApiResponse($server) is called without attempting to expose non-existent attributes.
🤖 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/Http/Controllers/Api/DatabasesController.php`:
- Around line 62-83: Add a strict model type to the exposeNestedServerSecrets
parameter and eager-load nested relations in the API list to avoid N+1 queries:
change the exposeNestedServerSecrets($model) signature to accept
\Illuminate\Database\Eloquent\Model (or a more specific model interface used in
this controller) so the parameter is typed, and update the databases() endpoint
query that fetches $project->databases() (the code path that later calls
removeSensitiveData() which invokes exposeNestedServerSecrets()) to eager-load
the nested relations using with(['destination.server.settings']) (apply this for
the privileged/expanded variant of the query as other code in the repo does).
In `@app/Http/Controllers/Api/ServicesController.php`:
- Around line 47-82: Eager-load nested server relations when the caller has the
`read:sensitive` or `root` ability and simplify the exposeNestedServerSecrets
signature: in the controller methods services() and service_by_uuid(), check the
token abilities and, only for privileged requests, add
->with('destination.server.settings') (or equivalent eager-load) to the query so
destination.server and server.settings aren't lazy-loaded per item; in
exposeNestedServerSecrets replace the redundant "instanceof Collection ||
instanceof \Illuminate\Database\Eloquent\Collection" check with a single type
check against \Illuminate\Support\Collection, and tighten the method parameter
type to accept
\Illuminate\Support\Collection|\Illuminate\Database\Eloquent\Model (or
\Illuminate\Database\Eloquent\Model) instead of an untyped $model so the
signature reflects supported types while keeping the return type void.
In `@app/Models/Application.php`:
- Around line 224-234: The Application model's $hidden array omits the PR
compose fields allowing sensitive compose payloads to be serialized; add
'docker_compose_pr' and 'docker_compose_pr_raw' to the protected $hidden array
in the Application class so these fields are excluded from default serialization
(modify the $hidden property where 'docker_compose', 'docker_compose_raw', etc.
are declared).
---
Outside diff comments:
In `@app/Http/Controllers/Api/ServersController.php`:
- Around line 39-52: Remove the dead references to logdrain_axiom_api_key and
logdrain_newrelic_license_key: in ServersController::removeSensitiveData()
remove the makeVisible() call for those two fields (they don't exist on Server),
and in the Server model remove those two entries from the $hidden property; keep
any visibility/handling logic on the ServerSetting model where those columns
actually live, and ensure serializeApiResponse($server) is called without
attempting to expose non-existent attributes.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: aa029e43-4506-4cd9-9703-fe0204debd90
📒 Files selected for processing (19)
app/Http/Controllers/Api/ApplicationsController.phpapp/Http/Controllers/Api/DatabasesController.phpapp/Http/Controllers/Api/ServersController.phpapp/Http/Controllers/Api/ServicesController.phpapp/Models/Application.phpapp/Models/EnvironmentVariable.phpapp/Models/Server.phpapp/Models/ServerSetting.phpapp/Models/Service.phpapp/Models/StandaloneClickhouse.phpapp/Models/StandaloneDragonfly.phpapp/Models/StandaloneKeydb.phpapp/Models/StandaloneMariadb.phpapp/Models/StandaloneMongodb.phpapp/Models/StandaloneMysql.phpapp/Models/StandalonePostgresql.phpapp/Models/StandaloneRedis.phptests/Feature/Security/ApiSensitiveFieldsTest.phptests/Unit/Models/SensitiveFieldsHiddenTest.php
👮 Files not reviewed due to content moderation or server errors (4)
- app/Http/Controllers/Api/ApplicationsController.php
- tests/Unit/Models/SensitiveFieldsHiddenTest.php
- tests/Feature/Security/ApiSensitiveFieldsTest.php
- app/Models/EnvironmentVariable.php
Add compose PR fields to the Application hidden attributes and allow database list queries to eager load nested server settings when sensitive access is permitted.
Only eager load nested server settings for service responses when the API token has read:sensitive, preventing read-only tokens from exposing sentinel fields while preserving sensitive access.
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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/Http/Controllers/Api/DatabasesController.php`:
- Around line 63-84: In exposeNestedServerSecrets(Model $model) remove the
redundant "?? null" on the nullsafe expression ($model->destination?->server)
and avoid assigning a separate $settings = $server->settings ?? null; — instead
check $server and directly inspect $server->settings with the nullsafe or plain
isset check; keep the existing makeVisible calls for the server and its settings
so behavior is unchanged while eliminating the dead "?? null" fallbacks.
In `@templates/service-templates-latest.json`:
- Line 302: The bootstrap step currently downloads pdsadmin.sh from the mutable
main branch using the command that saves to /usr/local/bin/pdsadmin.sh; change
it to fetch the script from a fixed commit SHA (pin the raw.githubusercontent
URL to a specific commit) and add a checksum verification step (compare the
downloaded file's sha256 against a stored expected value) before making it
executable and running it; update the compose template entries that contain the
"curl -o /usr/local/bin/pdsadmin.sh" invocation and the referenced "pdsadmin.sh"
filename to use the pinned URL and a verification/checksum variable, failing
fast if the checksum does not match.
- Line 1611: The compose snippet uses unpinned images
(docker.getoutline.com/outlinewiki/outline:latest and
logiclabshq/open-archiver:latest) which must be pinned for reproducible
self‑hosted deployments; replace those literal ":latest" tags in the compose
string with explicit version tags or immutable digests (sha256) for the images
referenced (search for the strings
"docker.getoutline.com/outlinewiki/outline:latest" and
"logiclabshq/open-archiver:latest" in the "compose" value), update any related
environment/documentation that references them, and verify by reloading the
compose manifest and running a test deploy to ensure the pinned images are
pulled correctly.
- Line 733: The compose blob under the "compose" key includes environment
variable patterns using the mandatory-fail form ${VAR:?default} which causes
container exit when VAR is unset; update all instances to the optional-default
form ${VAR:-default} (e.g. replace occurrences like
'${CONVEX_RELEASE_VERSION_DEV:?…}', '${CONVEX_URL_BACKEND:?…}',
'${DATABASE_URL:?…}', '${DO_NOT_REQUIRE_SSL:?…}', etc.) so variables fall back
to the provided defaults instead of failing to start; ensure every '${...:?…}'
occurrence in the base64/encoded compose string is changed to '${...:-…}' and
verify the encoded JSON string remains valid after replacements.
- Line 3411: The Valkey service healthcheck runs "redis-cli ping" without
authentication; update the valkey service healthcheck (look for the "valkey"
service and the healthcheck "test" entry that runs "redis-cli ping") to call
redis-cli with the -a flag supplying the same password variable used by the
service (e.g. -a "$VALKEY_REDIS_PASSWORD" or the existing
SERVICE_PASS/REDIS_PASSWORD variable pattern used elsewhere), so the test
becomes something like redis-cli -a "$<existing_password_env_var>" ping and
matches the auth pattern used by castopod/chatwoot/getoutline/etc.
In `@templates/service-templates.json`:
- Line 1611: The compose payload under the "compose" value pins floating images
and upgrades Postgres: replace docker.getoutline.com/outlinewiki/outline:latest
with a specific release tag (e.g., outline:VERSION), change redis:alpine to a
specific redis tag (e.g., redis:7.0-alpine), and replace postgres:12-alpine with
postgres:16-alpine or postgres:17-alpine; also add a brief migration note near
the Postgres image entry explaining that Postgres 12 is EOL and documenting any
required migration steps for existing users (references: the "compose" JSON
value and the image strings 'docker.getoutline.com/outlinewiki/outline:latest',
'redis:alpine', 'postgres:12-alpine').
- Line 3411: The compose template currently uses a floating image tag
"logiclabshq/open-archiver:latest" (the image field under the open-archiver
service in the "compose" JSON); replace that floating tag with a concrete,
immutable version (e.g., a semantic version or digest) — pick the appropriate
release tag for logiclabshq/open-archiver and update the image value to that tag
so deployments are reproducible; ensure any other occurrences of
"logiclabshq/open-archiver:latest" in the file are changed too.
In `@tests/Feature/Security/ApiSensitiveFieldsTest.php`:
- Around line 66-72: The negative-leak assertions are checking raw substrings
(e.g., in the block using $body = $response->getContent() and
expect($body)->not->toContain('sentinel_token')) which can produce false
positives; update each negative expect(...)->not->toContain(...) to assert
against the JSON key shape (for example include the quote and colon like
"\"sentinel_token\":") for the sentinel_token, sentinel_custom_url,
logdrain_axiom_api_key, logdrain_newrelic_license_key, logdrain_custom_config
and the other occurrences mentioned (e.g., postgres_password, internal_db_url)
so the tests look for the actual JSON field names rather than bare substrings.
- Around line 16-35: Add an array-shape PHPDoc to the makeTeamUser() helper to
document its return tuple; specifically, add a docblock above function
makeTeamUser() with a `@return` annotation like array{Team, User} (using the Team
and User types from the test context) so IDEs and callers know the exact order
and types of the returned array.
- Around line 57-100: In the "read token does not leak sentinel or logdrain
fields" test you only assert the field names are absent; also assert the actual
secret value and its stored/encrypted representation do not appear in the
response body by adding checks against the known plaintext sentinel (e.g.,
"super-secret-sentinel-token") and the encrypted form returned by your app (use
the same encryption helper or model getter you use elsewhere to compute the
stored/encrypted value) — add these assertions after $body is set in that test
(the same $body variable) so the test fails if either the plaintext or its
encrypted blob are present; reference makeApiToken and the test closure name
when locating where to add the assertions.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 644c9a77-c5af-4322-8d24-e066ef0c6210
📒 Files selected for processing (7)
app/Http/Controllers/Api/DatabasesController.phpapp/Models/Application.phpapp/Models/Project.phptemplates/service-templates-latest.jsontemplates/service-templates.jsontests/Feature/Security/ApiSensitiveFieldsTest.phptests/Unit/Models/SensitiveFieldsHiddenTest.php
Add model-level hidden fields for secrets, tokens, keys, notification credentials, deployment logs, and environment values. Allow explicit read:sensitive API access to reveal gated private keys and deployment logs, and cover the behavior with feature and unit tests.
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/Models/TelegramNotificationSettings.php (1)
76-84:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale cast key leaves docker-cleanup notification flags inconsistently typed.
$fillableusesdocker_cleanup_success_telegram_notifications/docker_cleanup_failure_telegram_notifications, but$castsstill has singulardocker_cleanup_telegram_notifications(Line 83). The current success/failure flags won’t be normalized as booleans.🔧 Proposed fix
- 'docker_cleanup_telegram_notifications' => 'boolean', + 'docker_cleanup_success_telegram_notifications' => 'boolean', + 'docker_cleanup_failure_telegram_notifications' => 'boolean',Also applies to: 97-99
🤖 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/Models/TelegramNotificationSettings.php` around lines 76 - 84, The $casts array in the TelegramNotificationSettings model still contains the stale key docker_cleanup_telegram_notifications, so the actual boolean fields docker_cleanup_success_telegram_notifications and docker_cleanup_failure_telegram_notifications (defined in $fillable) aren’t being cast; update the $casts property in class TelegramNotificationSettings to include both docker_cleanup_success_telegram_notifications and docker_cleanup_failure_telegram_notifications as booleans (and likewise ensure any other singular/stale keys referenced in $casts around the same area—see the entries mentioned for lines ~97-99—are replaced with the corresponding success/failure keys present in $fillable) so all notification flags are normalized to boolean.app/Http/Controllers/Api/ServicesController.php (1)
27-46: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winHasta la vista, untyped signature —
removeSensitiveData()is missing param/return types.The function was modified in this PR but still has
private function removeSensitiveData($service)with no parameter type and no return type. Per project guidelines, every method needs explicit type hints. Since this helper accepts a Service, an EnvironmentVariable, or whatever elseserializeApiResponse()digests, the right hint is\Illuminate\Database\Eloquent\Model(or a tighter union if you want to be picky). Self-hosted strictness > serverless mystery boxes. 🌮♻️ Proposed signature tightening
- private function removeSensitiveData($service) + private function removeSensitiveData(Model $service): mixed {As per coding guidelines: "Use explicit return type declarations and type hints for all method parameters".
🤖 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/Http/Controllers/Api/ServicesController.php` around lines 27 - 46, The method removeSensitiveData currently lacks type hints; update its signature to accept a model and declare a return type — change to private function removeSensitiveData(\Illuminate\Database\Eloquent\Model $service): mixed — ensure you import or fully-qualify the Model type, keep the existing body (including calls to exposeNestedServerSecrets($service) and serializeApiResponse($service)), and adjust any callers if they pass non-model types or rely on a different return shape.
♻️ Duplicate comments (2)
tests/Feature/Security/ApiSensitiveFieldsTest.php (2)
30-38: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
makeTeamUser()still missing the array-shape PHPDoc.Callers destructure this as
[$team, $user], and project guidelines explicitly require array-shape PHPDoc for tuple returns. Without it, IDEs play "guess the index" — which is roughly as fun as discovering your serverless function billed you $400 for one cron job. 🌮📝 Proposed PHPDoc
+/** + * `@return` array{0: Team, 1: User} + */ function makeTeamUser(): array {As per coding guidelines: "Use array shape type definitions in PHPDoc blocks".
🤖 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/Feature/Security/ApiSensitiveFieldsTest.php` around lines 30 - 38, Add an array-shape PHPDoc to the makeTeamUser() function so IDEs and static analyzers know it returns a tuple of Team and User; specifically annotate the return as something like `@return` array{0:\App\Models\Team,1:\App\Models\User} (or use the appropriate Team and User FQN/imports) above the makeTeamUser() declaration to match callers that destructure as [$team, $user].
70-74:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNegative leak assertions still use raw substrings — they will silently rot.
Lines 70-74 (servers) and 276-278 (databases) check for bare tokens like
'sentinel_token','postgres_password','internal_db_url'. The moment someone adds an unrelated boolean likeis_sentinel_token_enabledorhas_postgres_passwordto the response, the negative test gets hasta-la-vista'd by a false positive while you sleep. Match the actual JSON-key shape — your positive assertions on lines 87, 129, 141, 183, 195, 227-229, 291-293, and 331-333 already do this; the negatives should follow suit. Consistency: also a good idea for taco fillings.Also worth tightening: the secret value (
super-secret-sentinel-token,axiom-key-secret,super-secret-db-password) is never asserted absent. For a security-gating test, "the encrypted blob didn't leak via some other field" is the actually-load-bearing assertion.♻️ Suggested tightening for both blocks
- expect($body)->not->toContain('sentinel_token'); - expect($body)->not->toContain('sentinel_custom_url'); - expect($body)->not->toContain('logdrain_axiom_api_key'); - expect($body)->not->toContain('logdrain_newrelic_license_key'); - expect($body)->not->toContain('logdrain_custom_config'); + expect($body)->not->toContain('"sentinel_token":'); + expect($body)->not->toContain('"sentinel_custom_url":'); + expect($body)->not->toContain('"logdrain_axiom_api_key":'); + expect($body)->not->toContain('"logdrain_newrelic_license_key":'); + expect($body)->not->toContain('"logdrain_custom_config":'); + expect($body)->not->toContain('super-secret-sentinel-token');And around lines 276-278:
- expect($body)->not->toContain('postgres_password'); - expect($body)->not->toContain('internal_db_url'); - expect($body)->not->toContain('external_db_url'); + expect($body)->not->toContain('"postgres_password":'); + expect($body)->not->toContain('"internal_db_url":'); + expect($body)->not->toContain('"external_db_url":'); + expect($body)->not->toContain('super-secret-db-password');Also applies to: 276-278
🤖 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/Feature/Security/ApiSensitiveFieldsTest.php` around lines 70 - 74, Replace the fragile negative substring checks on $body that look for bare names (e.g. 'sentinel_token', 'postgres_password', 'internal_db_url') with JSON-key shaped checks that include the quote+colon pattern (e.g. assert $body does not contain '"sentinel_token":' or '"postgres_password":') so similar keys like is_sentinel_token_enabled won't cause false positives; update both the servers block (current checks around $body expect not->toContain for sentinel_* and logdrain_*) and the databases block accordingly. In addition, add explicit negative assertions that the secret values themselves are not present (e.g. the literal secret strings such as the sentinel token, axiom key, DB password used in fixtures) to ensure values aren’t leaked. Locate these changes in tests/Feature/Security/ApiSensitiveFieldsTest.php where $body is asserted and mirror the JSON-shaped positive assertions already present elsewhere in the file.
🤖 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/Http/Controllers/Api/ApplicationsController.php`:
- Around line 58-59: Narrow the parameter/type usage for
exposeNestedServerSecrets() to Application so its intent is explicit: update the
method signature/annotations (e.g., exposeNestedServerSecrets(Application
$application) or add a phpdoc `@param` Application $application) and any internal
type hints where it’s called (e.g., callers passing $model should be
cast/checked or only pass Application instances), ensuring references to
EnvironmentVariable are not used for this method; this keeps the nullsafe usage
($model->destination?->server ?? null) valid while improving static analysis and
readability.
In `@app/Models/CloudProviderToken.php`:
- Around line 14-16: Update CloudProviderTokensController's
removeSensitiveData() to conditionally expose the token field for authorized
requests: inside removeSensitiveData() check
request()->attributes->get('can_read_sensitive', false) === true and, when true,
call $token->makeVisible(['token']) (otherwise leave the model hidden as defined
by CloudProviderToken::$hidden). This mirrors the pattern used in
ServersController/ApplicationsController/DatabasesController so authorized
callers with read:sensitive/root scope can see the token value while others
cannot.
In `@app/Models/SslCertificate.php`:
- Around line 23-25: The file SslCertificate.php has formatting issues; run the
project's PHP formatter to fix them and commit the formatted file. Open the
class SslCertificate and ensure the property declaration protected $hidden =
['ssl_private_key']; conforms to the project's coding style, then run
./vendor/bin/pint and re-stage the formatted file before pushing.
- Around line 23-25: The exposeNestedServerSecrets() methods in
ApplicationsController, DatabasesController, and ServicesController need to also
make any loaded SslCertificate models' private keys visible for
can_read_sensitive tokens; update each exposeNestedServerSecrets()
implementation to, after making server and serverSettings visible, call
$server->sslCertificates->each->makeVisible(['ssl_private_key']) so that any
eager-loaded sslCertificates relation is handled consistently; reference the
SslCertificate model's hidden property 'ssl_private_key' and the
server->sslCertificates relation when adding this line.
In `@tests/Feature/Security/ApiSensitiveFieldsTest.php`:
- Around line 351-378: The test currently only asserts that at least one batched
query containing '"server_settings"."server_id" in' ran, which could miss
regressions that still run per-row lookups in addition to the batched query;
update the test (inside the test named "read sensitive service list eager loads
nested server settings once") to also assert the total number of server_settings
queries is bounded by a small constant (use the existing $serverSettingsQueries
collection captured by DB::listen and add an assertion such as expecting its
count to be <= 2) so the test fails if extra per-service queries are introduced.
In `@tests/Unit/Models/SensitiveFieldsHiddenTest.php`:
- Around line 197-204: The test "TelegramNotificationSettings casts actual
docker cleanup thread ids as encrypted" is in the wrong describe; move it out of
the "Sensitive model fields are hidden by default" describe and either (a) place
it into a new describe like "Sensitive fields are encrypted at rest" or (b)
merge its assertion into the existing Telegram hidden-fields test so the
expectation on (new TelegramNotificationSettings)->getCasts() sits with other
encryption-related assertions; update the test grouping/name accordingly and
ensure the spec references the TelegramNotificationSettings class and the same
expectation keys ('telegram_notifications_docker_cleanup_success_thread_id' and
'telegram_notifications_docker_cleanup_failure_thread_id').
---
Outside diff comments:
In `@app/Http/Controllers/Api/ServicesController.php`:
- Around line 27-46: The method removeSensitiveData currently lacks type hints;
update its signature to accept a model and declare a return type — change to
private function removeSensitiveData(\Illuminate\Database\Eloquent\Model
$service): mixed — ensure you import or fully-qualify the Model type, keep the
existing body (including calls to exposeNestedServerSecrets($service) and
serializeApiResponse($service)), and adjust any callers if they pass non-model
types or rely on a different return shape.
In `@app/Models/TelegramNotificationSettings.php`:
- Around line 76-84: The $casts array in the TelegramNotificationSettings model
still contains the stale key docker_cleanup_telegram_notifications, so the
actual boolean fields docker_cleanup_success_telegram_notifications and
docker_cleanup_failure_telegram_notifications (defined in $fillable) aren’t
being cast; update the $casts property in class TelegramNotificationSettings to
include both docker_cleanup_success_telegram_notifications and
docker_cleanup_failure_telegram_notifications as booleans (and likewise ensure
any other singular/stale keys referenced in $casts around the same area—see the
entries mentioned for lines ~97-99—are replaced with the corresponding
success/failure keys present in $fillable) so all notification flags are
normalized to boolean.
---
Duplicate comments:
In `@tests/Feature/Security/ApiSensitiveFieldsTest.php`:
- Around line 30-38: Add an array-shape PHPDoc to the makeTeamUser() function so
IDEs and static analyzers know it returns a tuple of Team and User; specifically
annotate the return as something like `@return`
array{0:\App\Models\Team,1:\App\Models\User} (or use the appropriate Team and
User FQN/imports) above the makeTeamUser() declaration to match callers that
destructure as [$team, $user].
- Around line 70-74: Replace the fragile negative substring checks on $body that
look for bare names (e.g. 'sentinel_token', 'postgres_password',
'internal_db_url') with JSON-key shaped checks that include the quote+colon
pattern (e.g. assert $body does not contain '"sentinel_token":' or
'"postgres_password":') so similar keys like is_sentinel_token_enabled won't
cause false positives; update both the servers block (current checks around
$body expect not->toContain for sentinel_* and logdrain_*) and the databases
block accordingly. In addition, add explicit negative assertions that the secret
values themselves are not present (e.g. the literal secret strings such as the
sentinel token, axiom key, DB password used in fixtures) to ensure values aren’t
leaked. Locate these changes in
tests/Feature/Security/ApiSensitiveFieldsTest.php where $body is asserted and
mirror the JSON-shaped positive assertions already present elsewhere in the
file.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: fbe209c8-b6bd-4c53-a4cb-b8a261562fb7
📒 Files selected for processing (25)
app/Http/Controllers/Api/ApplicationsController.phpapp/Http/Controllers/Api/DatabasesController.phpapp/Http/Controllers/Api/DeployController.phpapp/Http/Controllers/Api/SecurityController.phpapp/Http/Controllers/Api/ServicesController.phpapp/Models/Application.phpapp/Models/ApplicationDeploymentQueue.phpapp/Models/CloudInitScript.phpapp/Models/CloudProviderToken.phpapp/Models/DiscordNotificationSettings.phpapp/Models/EmailNotificationSettings.phpapp/Models/EnvironmentVariable.phpapp/Models/InstanceSettings.phpapp/Models/OauthSetting.phpapp/Models/PrivateKey.phpapp/Models/PushoverNotificationSettings.phpapp/Models/S3Storage.phpapp/Models/SharedEnvironmentVariable.phpapp/Models/SlackNotificationSettings.phpapp/Models/SslCertificate.phpapp/Models/TelegramNotificationSettings.phpapp/Models/WebhookNotificationSettings.phpresources/views/components/notification/navbar.blade.phptests/Feature/Security/ApiSensitiveFieldsTest.phptests/Unit/Models/SensitiveFieldsHiddenTest.php
Show cloud provider token values only when the API request has sensitive read permission, and cover read, read:sensitive, and root token behavior.
Require read:sensitive for nested server logdrain and sentinel fields in application and database API responses. Limit deployment configuration column migration SQL to PostgreSQL.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
app/Models/ApplicationDeploymentQueue.php (1)
12-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOpenAPI schema and serialized output are now out of sync.
$hiddenremoveslogs,configuration_snapshot, andconfiguration_difffrom serialized responses, but the schema still advertises them as standard properties. That creates API contract drift for clients generated from OpenAPI.Suggested schema alignment
#[OA\Schema( description: 'Project model', type: 'object', properties: [ @@ - 'configuration_snapshot' => ['type' => 'object', 'nullable' => true], - 'configuration_diff' => ['type' => 'object', 'nullable' => true], @@ - 'logs' => ['type' => 'string'],Also applies to: 86-90
🤖 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/Models/ApplicationDeploymentQueue.php` around lines 12 - 43, The OpenAPI OA\Schema for ApplicationDeploymentQueue lists properties (logs, configuration_snapshot, configuration_diff) that are excluded from JSON output via the model's $hidden, causing API contract drift; update the OA\Schema declarations used in ApplicationDeploymentQueue (and the duplicate schema block at the other occurrence) to remove these hidden properties (or mark them explicitly as writeOnly/absent from responses) so the documented schema matches the model's serialized output, ensuring the OA\Schema and the model's $hidden stay in sync.app/Http/Controllers/Api/ServersController.php (2)
45-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLoad the sensitive server columns before calling
makeVisible().
makeVisible()cannot resurrect attributes that were never selected.servers()still fetches onlyid,name,uuid,ip,user,port, anddescriptionon Line 93, soread:sensitiveandroottokens never receive these two logdrain keys from the list endpoint. Not even Skynet can expose columns that are not in memory.🛠️ Minimal fix
- $servers = ModelsServer::whereTeamId($teamId)->select('id', 'name', 'uuid', 'ip', 'user', 'port', 'description')->get()->load(['settings'])->map(function ($server) { + $columns = ['id', 'name', 'uuid', 'ip', 'user', 'port', 'description']; + if ($request->attributes->get('can_read_sensitive', false) === true) { + $columns[] = 'logdrain_axiom_api_key'; + $columns[] = 'logdrain_newrelic_license_key'; + } + + $servers = ModelsServer::whereTeamId($teamId)->select($columns)->get()->load(['settings'])->map(function ($server) {🤖 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/Http/Controllers/Api/ServersController.php` around lines 45 - 49, The sensitive logdrain fields are never selected from the DB so calling $server->makeVisible(...) has no effect; update the ServersController->servers() query to include the columns 'logdrain_axiom_api_key' and 'logdrain_newrelic_license_key' when request()->attributes->get('can_read_sensitive', false) is true (e.g., conditionally add select/addSelect of those columns in the servers() query), then call $server->makeVisible(...) as currently done.
720-749:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the server IP/domain uniqueness guard on PATCH.
This update path now writes
ipthrough$server->update($updateFields)without the conflict check thatcreate_server()keeps on Lines 548-555. A PATCH can point two server records—even across teams—at the same endpoint, which is how one metal box ends up wearing two nametags. That is bad for data routing and worse for operator trust.🛠️ Minimal fix
$updateFields = $request->only(['name', 'description', 'ip', 'port', 'user']); + if ($request->filled('ip')) { + $conflictingServer = ModelsServer::whereIp($request->ip) + ->whereKeyNot($server->id) + ->first(); + + if ($conflictingServer) { + if ($conflictingServer->team_id === $teamId) { + return response()->json(['message' => 'A server with this IP/Domain already exists in your team.'], 400); + } + + return response()->json(['message' => 'A server with this IP/Domain is already in use by another team.'], 400); + } + } + if ($request->filled('private_key_uuid')) { $privateKey = PrivateKey::whereTeamId($teamId)->whereUuid($request->private_key_uuid)->first(); if (! $privateKey) { return response()->json(['message' => 'Private key not found.'], 404); }🤖 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/Http/Controllers/Api/ServersController.php` around lines 720 - 749, The PATCH path updates $server->update($updateFields) without checking whether the new ip/port/domain conflicts with another Server record (the same guard exists in create_server()), so add a uniqueness check before calling $server->update: when $request->filled('ip') (or ip/port/domain fields present) query the servers table for an existing Server with the same ip and port/domain that is not the current $server->id (and consider cross-team matches if create_server() prevents those), and if one exists return the same validation error/response used by create_server() (e.g., 422 with a descriptive message) instead of proceeding to $server->update; ensure you reference the ServersController update flow around $server->update($updateFields) and re-use the same conflict logic/response as create_server().app/Models/StandaloneDragonfly.php (1)
67-78: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse
casts()instead ofprotected $castsinapp/Models/StandaloneDragonfly.php(machines love consistency; self-hosting loves conventions 😈🌮)This model currently defines casts via
protected $casts = [...], but the repo convention for model casts is to use acasts()method instead (for consistency acrossapp/Models).Suggested patch
- protected $casts = [ - 'health_check_enabled' => 'boolean', - 'health_check_interval' => 'integer', - 'health_check_timeout' => 'integer', - 'health_check_retries' => 'integer', - 'health_check_start_period' => 'integer', - 'dragonfly_password' => 'encrypted', - 'public_port_timeout' => 'integer', - 'restart_count' => 'integer', - 'last_restart_at' => 'datetime', - 'last_restart_type' => 'string', - ]; + protected function casts(): array + { + return [ + 'health_check_enabled' => 'boolean', + 'health_check_interval' => 'integer', + 'health_check_timeout' => 'integer', + 'health_check_retries' => 'integer', + 'health_check_start_period' => 'integer', + 'dragonfly_password' => 'encrypted', + 'public_port_timeout' => 'integer', + 'restart_count' => 'integer', + 'last_restart_at' => 'datetime', + 'last_restart_type' => 'string', + ]; + }🤖 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/Models/StandaloneDragonfly.php` around lines 67 - 78, The model StandaloneDragonfly currently defines casts via the protected $casts property; replace that property with a casts() method returning the same array to follow project convention. Remove the protected $casts declaration and add a public function casts(): array { return [ ...same key=>type pairs as before... ]; } preserving all cast entries (e.g., 'dragonfly_password' => 'encrypted', 'last_restart_at' => 'datetime') and keep the exact keys and types so behavior remains unchanged.
🤖 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/Http/Controllers/Api/ApplicationsController.php`:
- Line 64: The hideNestedServerSecrets/exposeNestedServerSecrets helpers
traverse destination -> server -> settings and are causing N+1 queries because
callers use plain ->get(); fix by eager-loading that chain before serialization:
where the controller uses ->get() in the list/read paths (and the other spots
that call hideNestedServerSecrets/exposeNestedServerSecrets) update the query to
->with(['destination.server.settings']) (or call
->loadMissing(['destination.server.settings']) on the Model/Collection before
calling the helper) so the nested relations are loaded up-front and no
lazy-loading occurs during hideNestedServerSecrets/exposeNestedServerSecrets.
- Line 207: The OpenAPI request bodies in ApplicationsController mark both
environment_name and environment_uuid as required, but the controller runtime
check accepts either one; update the OpenAPI attributes so generated clients
match runtime: either remove one of the two from the required arrays (leave only
environment_uuid or environment_name) in each request-body definition where you
see the required: ['project_uuid', 'server_uuid', 'environment_name',
'environment_uuid', ...] array, or replace those request-body schemas with an
explicit OA\OneOf that requires oneOf {environment_name, environment_uuid};
ensure you make the same change at every occurrence of that required-array and
keep the runtime acceptance logic unchanged.
In `@app/Http/Controllers/Api/DatabasesController.php`:
- Around line 54-56: The hideNestedServerSecrets method is causing N+1
lazy-loads because it accesses $model->destination?->server and
$server->settings when can_read_sensitive is false; either eager-load the
destination.server and server.settings relations in the non-privileged branch
before calling hideNestedServerSecrets, or change hideNestedServerSecrets to
first check relationLoaded('destination') and relationLoaded('server') (and
isset($server->settings) only if loaded) and skip touching nested server data if
relations are not loaded; update both places that call hideNestedServerSecrets
to follow the same pattern to avoid the N+1 query hazard.
In
`@database/migrations/2026_05_29_000000_encrypt_application_deployment_configuration_columns.php`:
- Around line 28-29: The down migration currently does ALTER COLUMN ... TYPE
json USING configuration_snapshot::json and configuration_diff::json which will
fail if those text columns contain encrypted/non-JSON blobs; change the two
DB::statement calls so the columns are converted back to text safely (e.g. ALTER
TABLE ... ALTER COLUMN configuration_snapshot TYPE text USING
configuration_snapshot::text and same for configuration_diff) or otherwise avoid
forcing a json cast (only cast when valid JSON). Update the DB::statement lines
that reference configuration_snapshot and configuration_diff to use ::text (or a
conditional/SAFE-CAST) instead of ::json so rollbacks won’t error on encrypted
payloads.
---
Outside diff comments:
In `@app/Http/Controllers/Api/ServersController.php`:
- Around line 45-49: The sensitive logdrain fields are never selected from the
DB so calling $server->makeVisible(...) has no effect; update the
ServersController->servers() query to include the columns
'logdrain_axiom_api_key' and 'logdrain_newrelic_license_key' when
request()->attributes->get('can_read_sensitive', false) is true (e.g.,
conditionally add select/addSelect of those columns in the servers() query),
then call $server->makeVisible(...) as currently done.
- Around line 720-749: The PATCH path updates $server->update($updateFields)
without checking whether the new ip/port/domain conflicts with another Server
record (the same guard exists in create_server()), so add a uniqueness check
before calling $server->update: when $request->filled('ip') (or ip/port/domain
fields present) query the servers table for an existing Server with the same ip
and port/domain that is not the current $server->id (and consider cross-team
matches if create_server() prevents those), and if one exists return the same
validation error/response used by create_server() (e.g., 422 with a descriptive
message) instead of proceeding to $server->update; ensure you reference the
ServersController update flow around $server->update($updateFields) and re-use
the same conflict logic/response as create_server().
In `@app/Models/ApplicationDeploymentQueue.php`:
- Around line 12-43: The OpenAPI OA\Schema for ApplicationDeploymentQueue lists
properties (logs, configuration_snapshot, configuration_diff) that are excluded
from JSON output via the model's $hidden, causing API contract drift; update the
OA\Schema declarations used in ApplicationDeploymentQueue (and the duplicate
schema block at the other occurrence) to remove these hidden properties (or mark
them explicitly as writeOnly/absent from responses) so the documented schema
matches the model's serialized output, ensuring the OA\Schema and the model's
$hidden stay in sync.
In `@app/Models/StandaloneDragonfly.php`:
- Around line 67-78: The model StandaloneDragonfly currently defines casts via
the protected $casts property; replace that property with a casts() method
returning the same array to follow project convention. Remove the protected
$casts declaration and add a public function casts(): array { return [ ...same
key=>type pairs as before... ]; } preserving all cast entries (e.g.,
'dragonfly_password' => 'encrypted', 'last_restart_at' => 'datetime') and keep
the exact keys and types so behavior remains unchanged.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 10345f23-cef1-41d2-8b32-05d8969b199f
📒 Files selected for processing (20)
app/Http/Controllers/Api/ApplicationsController.phpapp/Http/Controllers/Api/CloudProviderTokensController.phpapp/Http/Controllers/Api/DatabasesController.phpapp/Http/Controllers/Api/ServersController.phpapp/Models/Application.phpapp/Models/ApplicationDeploymentQueue.phpapp/Models/CloudProviderToken.phpapp/Models/S3Storage.phpapp/Models/Server.phpapp/Models/Service.phpapp/Models/StandaloneClickhouse.phpapp/Models/StandaloneDragonfly.phpapp/Models/StandaloneKeydb.phpapp/Models/StandaloneMariadb.phpapp/Models/StandaloneMongodb.phpapp/Models/StandaloneMysql.phpapp/Models/StandalonePostgresql.phpapp/Models/StandaloneRedis.phpdatabase/factories/CloudProviderTokenFactory.phpdatabase/migrations/2026_05_29_000000_encrypt_application_deployment_configuration_columns.php
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
app/Models/ApplicationDeploymentQueue.php (1)
12-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOpenAPI schema and serialized output are now out of sync.
$hiddenremoveslogs,configuration_snapshot, andconfiguration_difffrom serialized responses, but the schema still advertises them as standard properties. That creates API contract drift for clients generated from OpenAPI.Suggested schema alignment
#[OA\Schema( description: 'Project model', type: 'object', properties: [ @@ - 'configuration_snapshot' => ['type' => 'object', 'nullable' => true], - 'configuration_diff' => ['type' => 'object', 'nullable' => true], @@ - 'logs' => ['type' => 'string'],Also applies to: 86-90
🤖 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/Models/ApplicationDeploymentQueue.php` around lines 12 - 43, The OpenAPI OA\Schema for ApplicationDeploymentQueue lists properties (logs, configuration_snapshot, configuration_diff) that are excluded from JSON output via the model's $hidden, causing API contract drift; update the OA\Schema declarations used in ApplicationDeploymentQueue (and the duplicate schema block at the other occurrence) to remove these hidden properties (or mark them explicitly as writeOnly/absent from responses) so the documented schema matches the model's serialized output, ensuring the OA\Schema and the model's $hidden stay in sync.app/Http/Controllers/Api/ServersController.php (2)
45-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLoad the sensitive server columns before calling
makeVisible().
makeVisible()cannot resurrect attributes that were never selected.servers()still fetches onlyid,name,uuid,ip,user,port, anddescriptionon Line 93, soread:sensitiveandroottokens never receive these two logdrain keys from the list endpoint. Not even Skynet can expose columns that are not in memory.🛠️ Minimal fix
- $servers = ModelsServer::whereTeamId($teamId)->select('id', 'name', 'uuid', 'ip', 'user', 'port', 'description')->get()->load(['settings'])->map(function ($server) { + $columns = ['id', 'name', 'uuid', 'ip', 'user', 'port', 'description']; + if ($request->attributes->get('can_read_sensitive', false) === true) { + $columns[] = 'logdrain_axiom_api_key'; + $columns[] = 'logdrain_newrelic_license_key'; + } + + $servers = ModelsServer::whereTeamId($teamId)->select($columns)->get()->load(['settings'])->map(function ($server) {🤖 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/Http/Controllers/Api/ServersController.php` around lines 45 - 49, The sensitive logdrain fields are never selected from the DB so calling $server->makeVisible(...) has no effect; update the ServersController->servers() query to include the columns 'logdrain_axiom_api_key' and 'logdrain_newrelic_license_key' when request()->attributes->get('can_read_sensitive', false) is true (e.g., conditionally add select/addSelect of those columns in the servers() query), then call $server->makeVisible(...) as currently done.
720-749:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the server IP/domain uniqueness guard on PATCH.
This update path now writes
ipthrough$server->update($updateFields)without the conflict check thatcreate_server()keeps on Lines 548-555. A PATCH can point two server records—even across teams—at the same endpoint, which is how one metal box ends up wearing two nametags. That is bad for data routing and worse for operator trust.🛠️ Minimal fix
$updateFields = $request->only(['name', 'description', 'ip', 'port', 'user']); + if ($request->filled('ip')) { + $conflictingServer = ModelsServer::whereIp($request->ip) + ->whereKeyNot($server->id) + ->first(); + + if ($conflictingServer) { + if ($conflictingServer->team_id === $teamId) { + return response()->json(['message' => 'A server with this IP/Domain already exists in your team.'], 400); + } + + return response()->json(['message' => 'A server with this IP/Domain is already in use by another team.'], 400); + } + } + if ($request->filled('private_key_uuid')) { $privateKey = PrivateKey::whereTeamId($teamId)->whereUuid($request->private_key_uuid)->first(); if (! $privateKey) { return response()->json(['message' => 'Private key not found.'], 404); }🤖 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/Http/Controllers/Api/ServersController.php` around lines 720 - 749, The PATCH path updates $server->update($updateFields) without checking whether the new ip/port/domain conflicts with another Server record (the same guard exists in create_server()), so add a uniqueness check before calling $server->update: when $request->filled('ip') (or ip/port/domain fields present) query the servers table for an existing Server with the same ip and port/domain that is not the current $server->id (and consider cross-team matches if create_server() prevents those), and if one exists return the same validation error/response used by create_server() (e.g., 422 with a descriptive message) instead of proceeding to $server->update; ensure you reference the ServersController update flow around $server->update($updateFields) and re-use the same conflict logic/response as create_server().app/Models/StandaloneDragonfly.php (1)
67-78: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse
casts()instead ofprotected $castsinapp/Models/StandaloneDragonfly.php(machines love consistency; self-hosting loves conventions 😈🌮)This model currently defines casts via
protected $casts = [...], but the repo convention for model casts is to use acasts()method instead (for consistency acrossapp/Models).Suggested patch
- protected $casts = [ - 'health_check_enabled' => 'boolean', - 'health_check_interval' => 'integer', - 'health_check_timeout' => 'integer', - 'health_check_retries' => 'integer', - 'health_check_start_period' => 'integer', - 'dragonfly_password' => 'encrypted', - 'public_port_timeout' => 'integer', - 'restart_count' => 'integer', - 'last_restart_at' => 'datetime', - 'last_restart_type' => 'string', - ]; + protected function casts(): array + { + return [ + 'health_check_enabled' => 'boolean', + 'health_check_interval' => 'integer', + 'health_check_timeout' => 'integer', + 'health_check_retries' => 'integer', + 'health_check_start_period' => 'integer', + 'dragonfly_password' => 'encrypted', + 'public_port_timeout' => 'integer', + 'restart_count' => 'integer', + 'last_restart_at' => 'datetime', + 'last_restart_type' => 'string', + ]; + }🤖 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/Models/StandaloneDragonfly.php` around lines 67 - 78, The model StandaloneDragonfly currently defines casts via the protected $casts property; replace that property with a casts() method returning the same array to follow project convention. Remove the protected $casts declaration and add a public function casts(): array { return [ ...same key=>type pairs as before... ]; } preserving all cast entries (e.g., 'dragonfly_password' => 'encrypted', 'last_restart_at' => 'datetime') and keep the exact keys and types so behavior remains unchanged.
🤖 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/Http/Controllers/Api/ApplicationsController.php`:
- Line 64: The hideNestedServerSecrets/exposeNestedServerSecrets helpers
traverse destination -> server -> settings and are causing N+1 queries because
callers use plain ->get(); fix by eager-loading that chain before serialization:
where the controller uses ->get() in the list/read paths (and the other spots
that call hideNestedServerSecrets/exposeNestedServerSecrets) update the query to
->with(['destination.server.settings']) (or call
->loadMissing(['destination.server.settings']) on the Model/Collection before
calling the helper) so the nested relations are loaded up-front and no
lazy-loading occurs during hideNestedServerSecrets/exposeNestedServerSecrets.
- Line 207: The OpenAPI request bodies in ApplicationsController mark both
environment_name and environment_uuid as required, but the controller runtime
check accepts either one; update the OpenAPI attributes so generated clients
match runtime: either remove one of the two from the required arrays (leave only
environment_uuid or environment_name) in each request-body definition where you
see the required: ['project_uuid', 'server_uuid', 'environment_name',
'environment_uuid', ...] array, or replace those request-body schemas with an
explicit OA\OneOf that requires oneOf {environment_name, environment_uuid};
ensure you make the same change at every occurrence of that required-array and
keep the runtime acceptance logic unchanged.
In `@app/Http/Controllers/Api/DatabasesController.php`:
- Around line 54-56: The hideNestedServerSecrets method is causing N+1
lazy-loads because it accesses $model->destination?->server and
$server->settings when can_read_sensitive is false; either eager-load the
destination.server and server.settings relations in the non-privileged branch
before calling hideNestedServerSecrets, or change hideNestedServerSecrets to
first check relationLoaded('destination') and relationLoaded('server') (and
isset($server->settings) only if loaded) and skip touching nested server data if
relations are not loaded; update both places that call hideNestedServerSecrets
to follow the same pattern to avoid the N+1 query hazard.
In
`@database/migrations/2026_05_29_000000_encrypt_application_deployment_configuration_columns.php`:
- Around line 28-29: The down migration currently does ALTER COLUMN ... TYPE
json USING configuration_snapshot::json and configuration_diff::json which will
fail if those text columns contain encrypted/non-JSON blobs; change the two
DB::statement calls so the columns are converted back to text safely (e.g. ALTER
TABLE ... ALTER COLUMN configuration_snapshot TYPE text USING
configuration_snapshot::text and same for configuration_diff) or otherwise avoid
forcing a json cast (only cast when valid JSON). Update the DB::statement lines
that reference configuration_snapshot and configuration_diff to use ::text (or a
conditional/SAFE-CAST) instead of ::json so rollbacks won’t error on encrypted
payloads.
---
Outside diff comments:
In `@app/Http/Controllers/Api/ServersController.php`:
- Around line 45-49: The sensitive logdrain fields are never selected from the
DB so calling $server->makeVisible(...) has no effect; update the
ServersController->servers() query to include the columns
'logdrain_axiom_api_key' and 'logdrain_newrelic_license_key' when
request()->attributes->get('can_read_sensitive', false) is true (e.g.,
conditionally add select/addSelect of those columns in the servers() query),
then call $server->makeVisible(...) as currently done.
- Around line 720-749: The PATCH path updates $server->update($updateFields)
without checking whether the new ip/port/domain conflicts with another Server
record (the same guard exists in create_server()), so add a uniqueness check
before calling $server->update: when $request->filled('ip') (or ip/port/domain
fields present) query the servers table for an existing Server with the same ip
and port/domain that is not the current $server->id (and consider cross-team
matches if create_server() prevents those), and if one exists return the same
validation error/response used by create_server() (e.g., 422 with a descriptive
message) instead of proceeding to $server->update; ensure you reference the
ServersController update flow around $server->update($updateFields) and re-use
the same conflict logic/response as create_server().
In `@app/Models/ApplicationDeploymentQueue.php`:
- Around line 12-43: The OpenAPI OA\Schema for ApplicationDeploymentQueue lists
properties (logs, configuration_snapshot, configuration_diff) that are excluded
from JSON output via the model's $hidden, causing API contract drift; update the
OA\Schema declarations used in ApplicationDeploymentQueue (and the duplicate
schema block at the other occurrence) to remove these hidden properties (or mark
them explicitly as writeOnly/absent from responses) so the documented schema
matches the model's serialized output, ensuring the OA\Schema and the model's
$hidden stay in sync.
In `@app/Models/StandaloneDragonfly.php`:
- Around line 67-78: The model StandaloneDragonfly currently defines casts via
the protected $casts property; replace that property with a casts() method
returning the same array to follow project convention. Remove the protected
$casts declaration and add a public function casts(): array { return [ ...same
key=>type pairs as before... ]; } preserving all cast entries (e.g.,
'dragonfly_password' => 'encrypted', 'last_restart_at' => 'datetime') and keep
the exact keys and types so behavior remains unchanged.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 10345f23-cef1-41d2-8b32-05d8969b199f
📒 Files selected for processing (20)
app/Http/Controllers/Api/ApplicationsController.phpapp/Http/Controllers/Api/CloudProviderTokensController.phpapp/Http/Controllers/Api/DatabasesController.phpapp/Http/Controllers/Api/ServersController.phpapp/Models/Application.phpapp/Models/ApplicationDeploymentQueue.phpapp/Models/CloudProviderToken.phpapp/Models/S3Storage.phpapp/Models/Server.phpapp/Models/Service.phpapp/Models/StandaloneClickhouse.phpapp/Models/StandaloneDragonfly.phpapp/Models/StandaloneKeydb.phpapp/Models/StandaloneMariadb.phpapp/Models/StandaloneMongodb.phpapp/Models/StandaloneMysql.phpapp/Models/StandalonePostgresql.phpapp/Models/StandaloneRedis.phpdatabase/factories/CloudProviderTokenFactory.phpdatabase/migrations/2026_05_29_000000_encrypt_application_deployment_configuration_columns.php
🛑 Comments failed to post (4)
app/Http/Controllers/Api/ApplicationsController.php (2)
64-64:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop the nested-secret helpers from lazy-loading relations during serialization.
hideNestedServerSecrets()andexposeNestedServerSecrets()walkdestination -> server -> settings, but the list action still does a plain->get()on Line 176. That turns response shaping into an N+1 query machine for bothreadandread:sensitivetokens. I like servers; I do not like surprise query storms.🛠️ Minimal fix
private function hideNestedServerSecrets($model): void { + if (! $model->relationLoaded('destination')) { + return; + } + $server = $model->destination?->server ?? null; if (! $server) { return; } @@ private function exposeNestedServerSecrets($model): void { + if (! $model->relationLoaded('destination')) { + return; + } + $server = $model->destination?->server ?? null; if (! $server) { return; }Also applies to: 70-92, 100-120
🤖 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/Http/Controllers/Api/ApplicationsController.php` at line 64, The hideNestedServerSecrets/exposeNestedServerSecrets helpers traverse destination -> server -> settings and are causing N+1 queries because callers use plain ->get(); fix by eager-loading that chain before serialization: where the controller uses ->get() in the list/read paths (and the other spots that call hideNestedServerSecrets/exposeNestedServerSecrets) update the query to ->with(['destination.server.settings']) (or call ->loadMissing(['destination.server.settings']) on the Model/Collection before calling the helper) so the nested relations are loaded up-front and no lazy-loading occurs during hideNestedServerSecrets/exposeNestedServerSecrets.
207-207:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix the OpenAPI contract for environment selection.
These request bodies now mark both
environment_nameandenvironment_uuidas required, but the runtime check on Lines 1005-1006 accepts either one. Auto-generated clients will think valid requests are invalid before they ever reach your beautifully self-hosted API. That is not judgment day; that is just bad docs.🛠️ Minimal fix
- required: ['project_uuid', 'server_uuid', 'environment_name', 'environment_uuid', 'git_repository', 'git_branch', 'build_pack'], + required: ['project_uuid', 'server_uuid', 'git_repository', 'git_branch', 'build_pack'],Apply the same correction at Lines 373, 539, and 842, or model the "one of these two fields is required" rule explicitly with an OpenAPI
oneOf.As per coding guidelines, "API responses should use OpenAPI 3.0 attributes (use OpenApi\Attributes as OA) for auto-generated documentation".
Also applies to: 373-373, 539-539, 842-842
🤖 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/Http/Controllers/Api/ApplicationsController.php` at line 207, The OpenAPI request bodies in ApplicationsController mark both environment_name and environment_uuid as required, but the controller runtime check accepts either one; update the OpenAPI attributes so generated clients match runtime: either remove one of the two from the required arrays (leave only environment_uuid or environment_name) in each request-body definition where you see the required: ['project_uuid', 'server_uuid', 'environment_name', 'environment_uuid', ...] array, or replace those request-body schemas with an explicit OA\OneOf that requires oneOf {environment_name, environment_uuid}; ensure you make the same change at every occurrence of that required-array and keep the runtime acceptance logic unchanged.app/Http/Controllers/Api/DatabasesController.php (1)
54-56:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winN+1 query hazard when
can_read_sensitiveis false.I'll be back... with bad news about your query performance. When
can_read_sensitiveis false,hideNestedServerSecrets()still accesses$model->destination?->serverand$server->settings, which triggers lazy-loading since relations aren't eager-loaded in that branch (lines 149-151 only eager-load when privileged).This causes N+1 queries for every database in the list response — like a Cyberdyne assembly line of inefficiency. Self-hosting deserves better query discipline than serverless hype.
Two possible fixes:
- If you want nested server data with secrets hidden: Also eager-load for non-privileged callers:
$databaseRelations = $request->attributes->get('can_read_sensitive', false) === true ? ['destination.server.settings'] - : []; + : ['destination.server.settings'];
- If you don't want nested server data at all for non-privileged callers: Skip hiding if relations aren't loaded:
private function hideNestedServerSecrets(Model $model): void { + if (! $model->relationLoaded('destination')) { + return; + } $server = $model->destination?->server;Also applies to: 61-83
🤖 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/Http/Controllers/Api/DatabasesController.php` around lines 54 - 56, The hideNestedServerSecrets method is causing N+1 lazy-loads because it accesses $model->destination?->server and $server->settings when can_read_sensitive is false; either eager-load the destination.server and server.settings relations in the non-privileged branch before calling hideNestedServerSecrets, or change hideNestedServerSecrets to first check relationLoaded('destination') and relationLoaded('server') (and isset($server->settings) only if loaded) and skip touching nested server data if relations are not loaded; update both places that call hideNestedServerSecrets to follow the same pattern to avoid the N+1 query hazard.database/migrations/2026_05_29_000000_encrypt_application_deployment_configuration_columns.php (1)
28-29:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winRollback cast can fail once encrypted payloads exist
Line 28 and Line 29 cast
textback tojsondirectly. After this PR, these columns can contain encrypted non-JSON blobs, so rollback will error on PostgreSQL and block recovery paths.Suggested fix
public function down(): void { if (DB::connection()->getDriverName() !== 'pgsql') { return; } - DB::statement('ALTER TABLE application_deployment_queues ALTER COLUMN configuration_snapshot TYPE json USING configuration_snapshot::json'); - DB::statement('ALTER TABLE application_deployment_queues ALTER COLUMN configuration_diff TYPE json USING configuration_diff::json'); + // Irreversible when encrypted text has already been written. + // Keep rollback non-destructive and explicit. + throw new RuntimeException( + 'Cannot safely cast encrypted configuration_snapshot/configuration_diff back to json.' + ); }🤖 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 `@database/migrations/2026_05_29_000000_encrypt_application_deployment_configuration_columns.php` around lines 28 - 29, The down migration currently does ALTER COLUMN ... TYPE json USING configuration_snapshot::json and configuration_diff::json which will fail if those text columns contain encrypted/non-JSON blobs; change the two DB::statement calls so the columns are converted back to text safely (e.g. ALTER TABLE ... ALTER COLUMN configuration_snapshot TYPE text USING configuration_snapshot::text and same for configuration_diff) or otherwise avoid forcing a json cast (only cast when valid JSON). Update the DB::statement lines that reference configuration_snapshot and configuration_diff to use ::text (or a conditional/SAFE-CAST) instead of ::json so rollbacks won’t error on encrypted payloads.
Expose GitHub app secrets and file storage content only when the request has sensitive read access. Hide LocalFileVolume content by default and resolve application UUIDs from route parameters.
Prevent database detail responses from exposing log drain and Sentinel settings when callers lack sensitive read access.
Privileged API tokens can read hidden resource fields in environment and resource responses, including instance-admin team tokens with team_id 0. Configuration hashes now include hidden environment variable values so secret edits trigger restart detection.
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
can_read_sensitiveistrue(viamakeVisible) instead of hidden whenfalse$hiddenarrays to models:Application,EnvironmentVariable,Server,ServerSetting,Service, and allStandalone*DB models now declare sensitive fields hidden at the model levelexposeNestedServerSecrets()method onApplicationsController,DatabasesController, andServicesControllercallsmakeVisibleon eager-loadedServerandServerSettingrelationslogdrain_axiom_api_key,logdrain_newrelic_license_key,sentinel_custom_url, and logdrain config fields; databases add MySQL, MariaDB passwords andinit_scriptsEnvironmentVariablestatic closure type hint:static::createdandstatic::savinghooks now use the aliasedModelsEnvironmentVariabletype to avoid ambiguityApiSensitiveFieldsTest): verifiesread,read:sensitive, androottokens against/api/v1/servers,/api/v1/applications, and/api/v1/databasesendpointsSensitiveFieldsHiddenTest): verifies$hiddenarrays andtoArray()behavior for all affected modelsBreaking Changes
API responses for
read-scoped tokens will no longer include sensitive fields (passwords, webhook secrets, Dockerfile contents, DB URLs, server credentials) that were previously visible. Clients requiring these fields must use aread:sensitiveorroottoken.