Repository navigation
Fix securefile, enum and show() bugs, enforce permissions server-side, and persist the saved search - #20
Open
eldmark wants to merge 14 commits into
Open
Fix securefile, enum and show() bugs, enforce permissions server-side, and persist the saved search#20eldmark wants to merge 14 commits into
eldmark wants to merge 14 commits into
Conversation
Set stateDuration so the saved DataTables state (including column filters) never expires instead of being wiped after the default 2 hour stateDuration. The duration is now configurable via csgtcrud.stateDuration and CrudController::setStateDuration(). Also give each rendered table a stable id derived from the request path so two crud tables on the same page no longer share the same localStorage state key.
- Import Illuminate\Support\Facades\Storage, used by data() for securefile fields but never imported, causing a fatal error. - Allow and default the filedisk key in setCampo() so securefile fields no longer fail the allowed-params check or read an undefined index in data(), and require it just like filepath. - Fix the "emum" typo so enum validation actually runs. - Remove a dead assignment to an undefined $campos array in update(); $fields is the variable that is actually used. - Make show() return a response on non-JSON requests instead of a blank page.
Uploaded file and image extensions were used unchecked to build the stored filename, so a disallowed extension such as .php could land in a web-served directory. Add an optional extensiones_permitidas config key (default null, matching current behaviour) that, when set to an array, rejects disallowed extensions with a validation error instead of silently accepting them. The extension check is case-insensitive.
data() passed the client-supplied "length" straight into limit(), so a request could ask for an unbounded number of rows. Add an optional max_page_length config key (default null, matching current behaviour). When set, a non-positive length falls back to perPage and an oversized length is clamped to the configured maximum. No test accompanies this change even though it touches data(): this branch has no tests/ directory or existing test suite to extend (phpunit.xml is present but unused).
Setting $.fn.dataTable.defaults.stateDuration mutated the global DataTables defaults, so a CRUD listing changed the state duration of every other table rendered on the same page. Pass it as an option of the table being initialised instead. The constructor added to the base controller was a silent failure mode: an application controller extending CrudController and defining its own __construct() without calling parent would never pick the config value up. Resolve the value where it is used instead, with the property defaulting to null to mean "not set explicitly".
$this->permisos was only read by the index view to hide buttons. store(), update(), destroy(), create() and edit() never checked it, so any user who could reach the route was able to create, edit or delete records regardless of what setPermisos() declared. Guard each action with abort(403), checked right after setup(), which is where the child controller populates the permissions: - edit() checks 'edit' when $aId is set and 'add' otherwise, which also covers create(), since it delegates to edit($request, null). - update() checks 'add' when $aId === 0 and 'edit' otherwise, which also covers store(), since it delegates to update($request, 0). - destroy() checks 'delete'. The flags are read with empty() rather than by index, so a setPermisos() callback returning a partial array denies the missing permission instead of raising an undefined key warning on PHP 8. That matches how the view already treats a missing key. BEHAVIOUR CHANGE: a route that was reachable despite a false permission flag now returns 403. This closes a real authorisation hole, but an application that relied on the button merely being hidden will start getting 403s, so it is worth calling out to anyone pinning this branch.
The .travis.yml in this repository had stopped doing anything. Travis ended free open-source builds in 2021, the file still listed PHP 5.3 to 5.6 plus hhvm, and its `script: phpunit` referred to a dependency this branch never declared. It described a build that could not run and a PHP range the code does not support. Replace it with a workflow that does run: lint every file under src/ on PHP 7.1, 7.4 and 8.3, on each push and pull request. The matrix was verified by linting this branch's tree in a container for each of those versions, rather than assumed from the composer.json constraint, which is not accurate on this branch.
State the PHP and Laravel range this branch actually supports, derived from the framework APIs the code calls and from linting the tree on each PHP version, not from the composer.json constraint, which was never kept up to date. Document that the permission flags are now enforced on the write actions rather than only hiding buttons, since that is a behaviour change an upgrading application has to act on. Document the configuration keys this release adds, all of which default to the previous behaviour.
A column whose values come from a closed list — an enum with its
enumarray, or a bool — was filtered by typing the value into a free text
box, so the user had to know the stored key rather than pick the label
they see in the listing.
getFilterColumns() now exposes those values as options and the filter
row renders a select for them, with an empty "all" entry. Columns
without a closed list keep the text box, and a date column keeps the
date picker it already had.
Those columns are also matched with = instead of LIKE. With LIKE the
pattern is wrapped in %...%, so filtering an enum whose keys are 1 and
10 by "1" returned both, and filtering the keys "a" and "i" by "a"
returned every row whose value merely contains the letter.
The filter payload is unchanged, so applyFiltersToQuery() still receives
the same {column, value} pairs.
Neither type rendered an empty option. When a record has no value, no option matches, so the browser selects the first one and submitting the form writes it as though the user had chosen it. A record with no status came back with whatever status happened to be first in the list. Add an empty option, selected when the value is null or an empty string. BEHAVIOUR CHANGE: a field that is not required now saves null where it previously saved the first option. A field that is required is caught by its own validation rule instead of being silently populated. Fields declaring a default in setCampo() are unaffected, since the default still matches an option.
The require block claimed PHP >=5.3.0 and, on most branches, named no illuminate package at all, so the package installed cleanly onto a framework it cannot run on. PHP is now >=7.1. That is the lowest version the tree was verified to parse on, by linting it in a container for each release; php:5.6 is the oldest official image, so nothing below it was tested and nothing below it is claimed. No upper bound is declared: every branch parses on 8.3. illuminate/support is now >=6.0.
A composer constraint only binds when the package is installed. The application brings its own illuminate packages, so a mismatch surfaced much later, as a missing method in the middle of a listing. boot() now compares app()->version() against the range this package version supports (Laravel >= 6.0, no upper bound) and throws a RuntimeException naming both versions and what to do about it. Only a genuine incompatibility aborts. A container that does not report a comparable version, such as Lumen, is left alone rather than guessed at, and PHP is not re-checked at runtime: composer already enforces its floor and there is no known PHP ceiling.
The views ship no assets of their own: jQuery, DataTables, moment, selectize and summernote all come from the host application. When one was absent the page simply stopped, with a jQuery error or nothing at all, and the cause was not visible without devtools. Each view now checks the libraries it actually uses before using them. A missing one throws and shows a red banner naming it. A library that is present but outside the range this package was tested against only warns on the console, since it will most likely work. The checks for selectize and summernote sit inside the conditionals that already guard their initialisation, so a form that needs neither does not demand them. The DataTables upper bound is not a guess: fnSetColumnVis, which the listing calls when the user may neither edit nor delete, is present in 1.13.8 and absent from 2.1.8.
The dependency check added earlier lives inside the javascript section, so it cannot report the one failure that hides it: a layout that never yields that section. The block does not render, the check does not run, and the view sits there empty with nothing in the console. A listener in the content block, which always renders because the markup lives there, now reports it. The script block sets window.csgtCrudBooted as its last statement, and once the page has loaded a missing flag means the block never finished. The message separates the two causes: jQuery absent, or the layout not yielding the section. This also covers a second gap. On branches whose script tag is a plain <script> rather than a module, a layout that yields the section inside <head> runs the check before document.body exists, so the dependency check could throw without painting its banner. The backstop runs on load, when the body is there. Applied to the listing and the form. The form on the branches where its script already lives inside the content block needs neither half: that block renders with the markup and is inside the body.
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
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.
Fixes five broken behaviours in
CrudController, closes a server-side authorisation hole, and makes the DataTables saved state survive longer than two hours.Correctness fixes
securefilewas fatal on this branch.data()callsStorage::disk(...)but the class was not imported, so in theCsgt\Crudnamespace it resolved toCsgt\Crud\Storageand threw. Other branches carryuse Storage;; this one lost it. Addeduse Illuminate\Support\Facades\Storage;.securefilewas unusable for a second reason.data()reads$fullCampoFixed['filedisk'], butfilediskwas missing fromsetCampo()'s$allowedlist, so passing it bailed out and omitting it produced an undefined index. Added it to$allowed, defaulted it, included it in the field array, and added thesecurefilerequiresfilediskcheck next to the existingfilepathone. Branches 5.5 and 5.6.bs4 already had this; 5.9 did not.'emum'typo meant theenumvalidation never ran. Fixed — see the note below, because fixing it alone would have been dangerous.$campos[$campo['campo']] = $filename;inupdate()assigned to an undefined variable that was never read; the line below already assigned to$fields. Removed.show()returned nothing for a non-JSON request, producing a blank page. It now always returns the JSON response, since the package ships no show view.Authorisation
$this->permisoswas only read by the index view to hide buttons.store(),update(),destroy(),create()andedit()never checked it, so anyone who could reach the route could create, edit or delete records regardless of whatsetPermisos()declared. Each action now guards withabort(403), checked aftersetup()has populated the permissions.The flags are read with
empty()rather than by index, so asetPermisos()callback returning a partial array denies the missing permission instead of raising an undefined key warning on PHP 8 — the same way the view already treats a missing key.Saved search
DataTables'
stateDurationdefaults to 7200 seconds, so the saved state — including the column filters this branch already persists viastateSaveParams— expired after two hours. It is now configurable through a newstateDurationconfig key and asetStateDuration()setter, defaulting to0(never expires), which is whatmasteralready ships as "fix: remember last state forever".The listing table also gets a unique
idderived from the request path. DataTables derives its localStorage key partly from the selector, so two CRUD listings on the same page were overwriting each other's saved state.Opt-in additions
Both default to
null, which preserves today's behaviour exactly, and do nothing until an application sets them:extensiones_permitidas— extension whitelist forfileandimageuploads. Without it,update()builds the filename fromgetClientOriginalExtension()with no validation and moves it intopublic_path(), so a.phpupload lands in a web-served directory.max_page_length— caps the client-suppliedlengthindata(), which otherwise goes straight intolimit().mkdir($path, 0777, true)was left untouched deliberately: a hosting setup may depend on the current mode, and tightening it is a behaviour change.Closed-list columns are filtered with a dropdown
A column whose values come from a closed list — an
enumwith itsenumarray, or abool— was filtered by typing into a free text box, so the user had to know the stored key rather than pick the label they see in the listing.getFilterColumns()now exposes those values as options and the filter row renders a<select>with an empty "all" entry. Columns without a closed list keep the text box, and a date column keeps the date picker it already had.Those columns are also matched with
=instead ofLIKE. The pattern was wrapped in%...%, so filtering an enum whose keys are1and10by1returned both, and filtering keysaandibyareturned every row whose value merely contains that letter.The filter payload is unchanged, so
applyFiltersToQuery()still receives the same{column, value}pairs.An enum or combobox field can be left empty
Neither type rendered an empty
<option>. When a record has no value, no option matches, so the browser selects the first one and submitting the form writes it as though the user had chosen it — a record with no status came back with whatever status happened to be first in the list.An empty option is now rendered, selected when the value is null or an empty string.
Supported versions are declared and enforced
The
requireblock claimed a PHP floor that was wrong on most branches and, on many, named noilluminatepackage at all — so the package installed cleanly onto a framework it cannot run on.>=5.3.0>=7.1>=6.0The PHP floor is the lowest version the tree was verified to parse on, by linting it in a container for each release.
php:5.6is the oldest official image, so nothing below it was tested and nothing below it is claimed. No upper bound is declared: every branch parses on 8.3.A composer constraint only binds at install time, and the application brings its own illuminate packages, so a mismatch surfaced much later as a missing method mid-listing.
boot()now comparesapp()->version()against the range this package version supports (>= 6.0with no upper bound) and throws aRuntimeExceptionnaming both versions and what to do.Only a genuine incompatibility aborts. A container that does not report a comparable version, such as Lumen, is left alone rather than guessed at, and PHP is not re-checked at runtime: composer already enforces its floor and there is no known PHP ceiling.
A missing frontend library says so
The views ship no assets of their own — jQuery, DataTables, moment, selectize and summernote all come from the host application. When one was absent the page simply stopped, with a jQuery error or nothing at all, and the cause was invisible without devtools.
Each view now checks the libraries it actually uses, before using them. A missing one throws and shows a red banner naming it. One that is present but outside the tested range only warns on the console, since it will most likely work. The selectize and summernote checks sit inside the conditionals that already guard their initialisation, so a form needing neither does not demand them.
The DataTables upper bound is not a guess:
fnSetColumnVis, which the listing calls when the user may neither edit nor delete, is present in 1.13.8 and absent from 2.1.8 — verified against both CDN builds.A view whose script block never ran says so too
The dependency check above lives inside the
javascriptsection, so it cannot report the one failure that hides it: a layout that never yields that section. The block does not render, the check does not run, and the view sits there empty with nothing in the console.A listener in the content block — which always renders, because the markup lives there — now reports it. The script block sets
window.csgtCrudBootedas its last statement, and once the page has loaded a missing flag means the block never finished. The message separates the two causes: jQuery absent, or the layout not yielding the section.This also closes a second gap. On branches whose script tag is a plain
<script>rather than a module, a layout that yields the section inside<head>runs the check beforedocument.bodyexists, so the dependency check could throw without painting its banner. The backstop runs onload, when the body is there.Applied to the listing and the form. A form whose script already lives inside the content block needs neither half: that block renders with the markup and sits inside the body.
Behaviour changes, called out deliberately
Per
docs/RULES.mdrule 2, these are the changes an application pinning this branch would notice:The permission guards return 403 where requests previously succeeded. This closes a real authorisation hole, but an application that relied on the button merely being hidden will start getting 403s. This is the one item that is arguably a rule-2 "new version branch" change; it is included here on the maintainer's explicit decision.
stateDurationmoves from DataTables' 7200s default to never expiring. This is the requested fix and matchesmaster.The table's new unique
idchanges its localStorage state key, so existing installations lose previously-saved state once, on first load after upgrading.An unset
enumorcomboboxnow saves null where it previously saved the first option in the list. A field that is required is caught by its own validation rule instead of being silently populated; fields declaring adefaultare unaffected, since the default still matches an option.The reactivated
enumvalidation is deliberately not in that list, and that took a change of approach:Why the enum fix does not abort
Fixing the
'emum'typo reactivated add()that had been dead for years. A field declaredenumwith a missing or emptyenumarraycurrently renders an empty<select>and the application keeps working, so turning that into a hard failure would have taken down running applications on upgrade.There was also a genuine PHP 8 fatal hiding in that same line.
$enumarrayis only normalised to[]when the key is absent, so an application passing'enumarray' => nullor a string reachedcount()with a non-array. On PHP 7.2 that was a warning returning1; since PHP 8.0 it is a fatalTypeError:The validation is now guarded with
is_array(), normalises$enumarrayto[]so the view never receives a non-array, and logs a warning throughLog::warning()instead of aborting. A misconfiguredenumkeeps rendering harmlessly, but now leaves a trace in the log, which it never did before.Notes for reviewers
toSql()/getBindings()test for anything touchingdata(), andmax_page_lengthdoes touch it. This branch has no test suite to extend — only an unusedphpunit.xml.masterhas one and its equivalent change is covered there.composer.jsondeclares"php": ">=5.3.0"and that is wrong by about four major versions. This branch uses??and[$a, $b] = explode(...), so the real floor is PHP 7.1+. Left untouched per rule 3's instruction not to move therequireblock; flagging it for a separate decision.applyOrderToQuery()passes a query builder toorderBy(), whichdocs/RULES.mdlists as a Laravel 6+ API. Worth noting since package 6.0's floor is 5.8+ — the package version and the Laravel floor run in opposite directions here..travis.ymlstill lists PHP 5.3–5.6 and hhvm. Dead config that contradicts the code; not touched here.