Whitelist create_bulk_envs() fields to stop resourceable hijacking - #199
Merged
Conversation
updateOrCreate() passed the raw request item straight through as $values. On the update-existing-row path, fill($values)->save() then applied it unconditionally, letting a caller retarget an env var's resourceable_id/resourceable_type onto an arbitrary resource - including one owned by a different team. The create-new-row path was already safe (Eloquent re-applies the correct foreign/morph attributes after fill on create), so only the update path was exploitable.
Same root cause as the resourceable-hijacking fix: the previous commit's whitelist change also fixes this for free, since $values now explicitly carries the normalized $key instead of $item's raw one.
…s-mass-assignment
…s-mass-assignment
…s-mass-assignment # Conflicts: # tests/v4/Feature/Api/ServicesEnvsTest.php
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.
Summary
Found via a fresh
/code-reviewpass onapp/Http/Controllers/Api/ServicesController.php(issue #70). Closes 2 findings from that pass - they collapse into one fix because they're the exact same line of code.Finding 1 (security):
create_bulk_envs()passed the raw request item straight through asupdateOrCreate()'s$valuesargument.EnvironmentVariable'sresourceable_id/resourceable_typeare both$fillable, and the validator only checkskey/value/is_literal/is_multiline/is_shown_once/comment- it never rejects unknown fields, unlikecreate_service()/update_by_uuid(), which explicitly diff the request against an allowed-fields list.Verified against the actual Eloquent implementation in this Laravel version: the create-new-row path is already safe -
HasOneOrMany::create()callssetForeignAttributesForCreate()afternewInstance($attributes), so the correctresourceable_id/resourceable_typealways win regardless of attacker input. But the update-existing-row path inupdateOrCreate()calls$instance->fill($values)->save()unconditionally, with no re-application of the foreign/morph attributes - a caller who already knows (or can enumerate) an existing env var's key can retarget it onto an arbitrary resource, including one owned by a different team.Finding 2 (correctness): the normalized
$keycomputed for theupdateOrCreate()lookup was discarded on save, since the raw$item['key'](un-normalized) rode along in$itemand got re-applied byfill().Fix
Build an explicit
$valuesarray from only the validated fields (keyset to the already-normalized$key,value, and the optional booleans/comment only when present), matching the patterncreate_env()/update_env_by_uuid()already use, instead of passing the raw request item through. This one change closes both findings.Verification
TDD-proved, both tests confirmed failing against the pre-fix code and passing after:
ignores an attacker-supplied resourceable_id/resourceable_type when bulk-updating an existing env varkeeps the normalized key when bulk-updating an existing env var with a raw un-normalized keyAll 15 tests in
ServicesEnvsTest.phppass. Pint/PHPStan clean on the changed file.