Validate sort/direction against SQL injection on index screens - #5
Merged
Merged
Conversation
The index screens ordered by a column and direction taken straight from the query string. Eloquent binds values but not identifiers, so ?sort= was interpolated into the SQL: a 500 on a bad column at best, an injection surface at worst. The direction is checked by the framework, which throws on anything that is not asc or desc, so the column is the exploitable half. Call sites now go through a sortSafe macro that keeps the column only if it is a real column on the model's table, and the direction only if it is asc or desc. Each site falls back to the order it already used when no sort was given, so an invalid sort behaves like no sort instead of erroring. The macro is registered in this package's own provider rather than shared. The modules are installed independently and declare no common dependency, so one cannot rely on another having booted. The registration is guarded, so whichever module loads first wins and the definitions are identical. Part of the platform-wide sweep tracked on zerp-pk/zerp#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.
?sort=went straight intoorderBy(). Eloquent binds values but not identifiers, so the column name was interpolated into the SQL. (The direction is safe already: the framework throws on anything that is notasc/desc.)6 call site(s) now go through a
sortSafemacro that accepts the column only if it exists on the model's table, falling back to the order each screen already used with no sort.Registered in this package's provider, guarded, because modules are installed independently with no shared dependency.
Tests: 4 cases covering an injected column, an injected direction, a valid column, and an unknown column. Verified they fail when the validation is removed.
Part of the platform-wide sweep tracked on zerp-pk/zerp#39.