Validate sort/direction on index screens against SQL injection - #65
Merged
Conversation
Ten index controllers ordered by a column and direction taken straight
from the query string: orderBy(request('sort'), request('direction')).
Eloquent binds values but not identifiers, so the column name reached the
SQL text unescaped. A crafted ?sort= was at best a 500 from a non-existent
column and at worst an injection surface.
Every call site now uses the sortSafe query macro shipped in v1.2.3
(zerp/account), which only accepts a column that exists on the table
(Schema::hasColumn) and a direction of asc or desc, falling back to a safe
default otherwise. Each existing fallback and default direction is
preserved, including UserController's demo/superadmin ordering, so only
invalid input changes behaviour: from a 500 or injection to a safe order.
Closes #39
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.
Closes #39.
Ten
index()controllers ordered byorderBy(request('sort'), request('direction'))with the column taken straight from the query string. Eloquent binds values but not identifiers, so a crafted?sort=reached the SQL text unescaped, a 500 at best and an injection surface at worst.Fix
Every call site now uses the
sortSafequery macro already shipped in v1.2.3 (zerp/account): it accepts a column only ifSchema::hasColumnconfirms it exists on the table, and a direction only if it isasc/desc, falling back to a safe default otherwise. No new code, the macro is registered globally andzerp/accountis a hard dependency.Each controller keeps its existing fallback and default direction, including
UserController's demo/superadmin ordering andOrderController'sid desc. Only invalid input changes: from a 500 or injection to a safe order.Verified locally
sortSafe("id; DROP TABLE users", ...)compiles toorder by \id` asc`; a valid column passes through./users?sort=id;DROP TABLE users--returns 200 (was a 500/injection surface); valid?sort=email&direction=descreturns 200;userstable intact.Module packages (account already done, plus budget-planner, contract, double-entry) carry the same pattern in their own repos and are tracked separately.