Repository navigation
Issue #124 (buildora) onderzoeken: vaststellen of datatable- en bulk-action-routes buiten de web-middleware (en dus buiten CSRF-verificatie) vallen, en zo nodig fixen. - #190
Open
zyra-ginkelsoft[bot] wants to merge 1 commit into
Conversation
…nkel via config
Audit: alle datatable- en bulk-action-routes in routes/buildora.php
(inclusief store/update/destroy en de switch-locale/settings/2FA-routes)
erven de 'web'-middlewaregroep uitsluitend via config('buildora.middleware').
Met de standaardconfig zit 'web' daar wel in, dus CSRF-verificatie is
vandaag actief (geverifieerd met een regressietest en een live POST tegen
een draaiende testbench-server: beide geven 419 zonder token).
Dit is echter fragiel: een consumerende applicatie die de gepubliceerde
config/buildora.php aanpast kan per ongeluk 'web' uit de middleware-lijst
verwijderen, waarmee CSRF-bescherming voor de gevoeligste routes (create,
update, delete, bulk-acties) stilzwijgend verdwijnt. routes/auth.php in
hetzelfde package past dit patroon niet toe en wrapt zijn routes altijd
hard in Route::middleware('web').
Fix: routes/buildora.php extra wrappen in een hard-coded
Route::middleware('web'), naast de bestaande config-gestuurde middleware.
Zo kan CSRF nooit via configuratie worden uitgeschakeld.
Regressietests tonen 419 zonder CSRF-token aan, zowel met de standaardconfig
als met een config waaruit 'web' is verwijderd.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
This is a package repo (Buildora, a Laravel admin package), not a full Laravel app. Let me look at routes and service providers.Goed, orchestra/testbench is aanwezig. Nu ga ik installeren en een reproductietest schrijven om te bewijzen of de CSRF-bescherming daadwerkelijk actief is op de datatable/bulk-action routes.Missende ext-gd. Ik installeer met de platform-negeer optie, zodat we kunnen testen (dit is alleen voor de proefomgeving).Er bestaat al een goed
.zyra/proef.shscript dat de ext-gd fallback al afhandelt. Laten we het testen en de composer install afmaken met dezelfde fallback.Composer install gelukt. Nu schrijf ik een reproductietest die een POST naar een bulk-action/datatable-route stuurt zonder CSRF-token.Missende app key. Ik voeg die toe in de TestCase setup zodat encryptie werkt (nodig voor session/csrf).Nu herschrijf ik de definitieve regressietest correct, met de env-override zodat CSRF echt wordt gecontroleerd, en test ik ook expliciet dat een geldig token wél doorlaat (om te bewijzen dat de test zinvol is en niet toevallig altijd 419 geeft).Nu test ik of dit werkt met een config zonder 'web' erin — precies het scenario dat de fix moet afdekken.De fix werkt: zelfs met een misconfigureerdebuildora.middleware(zonder 'web') blijft CSRF nu afgedwongen (419). Ik voeg dit toe als permanente regressietest en ruim het debug-bestand op.Beter: aparte bestanden voor duidelijkheid en PSR-12 conformiteit.Nu de volledige testsuite en lint/analyse draaien.Alle 42 tests slagen. Nu lint en static analyse.Bevestigd: dezelfde lint/phpstan-bevindingen (59 fouten) bestaan al op main, buiten mijn wijzigingen (die alleenroutes/entests/raken, wat niet doorsrc/linting wordt gescand). Mijn wijzigingen introduceren geen nieuwe lint/analyse-fouten. Nu controleer ik het.zyra/proef.shscript end-to-end.De installatiepagina reageert met HTTP 200. Nu een extra reproductie: een echte HTTP POST-aanvraag naar de switch-locale route zonder CSRF-token, tegen de draaiende server (buiten de testomgeving, dus zonder derunningUnitTests()-bypass).Bevestigd met een echte live-server: HTTP 419 zonder CSRF-token — precies zoals verwacht. Nu ruim ik het proefserver-proces op en commit ik het werk.Geen achtergrondprocessen meer actief. Nu opruimen van de database-artefacten die door het proefscript zijn aangemaakt, en dan committen.Goed, alles schoon. Nu committen.Laatste check: volledige testsuite nogmaals draaien vanaf de gecommitte staat om zeker te zijn dat alles schoon is.Alle 42 tests slagen. Werk is gecommit op de branch.Samenvatting
Onderzoek (issue #124): Ik heb
routes/buildora.php,routes/auth.phpen beide service providers doorgelicht op alle non-GET routes (switch-locale, settings, permissions, profile, 2FA, en vooral de dynamischeresource-routes voor store/update/destroy waar bulk-acties naartoe posten — bulk-actions zelf zijn user-gedefinieerde named routes, geen vaste package-routes).Bevinding: Alle routes zaten binnen
Route::prefix(...)->middleware(config('buildora.middleware', ['web', ...])). Met de standaardconfig zit'web'er dus in en is CSRF-verificatie vandaag actief — bevestigd met:runningUnitTests(), dat heb ik moeten omzeilen om een eerlijke meting te krijgen) → 419 zonder token;.zyra/proef.sh) zonder CSRF-token → HTTP 419, bewijs bijgevoegd in de sessie.Wél een fragiliteit: CSRF hing volledig af van
config('buildora.middleware'). Als een consumerende app die gepubliceerde config aanpast en per ongeluk'web'weglaat, verdwijnt CSRF-bescherming stilzwijgend op precies de gevoeligste routes (create/update/delete).routes/auth.phpin hetzelfde package gebruikt dit patroon niet en wrapt altijd hard inRoute::middleware('web')— inconsistent.Fix:
routes/buildora.phpextra gewrapt in een hard-codedRoute::middleware('web'), naast de bestaande config-gestuurde middleware, zodat CSRF nooit via configuratie kan worden uitgeschakeld.Tests:
tests/Feature/CsrfProtectionTest.php: 419 zonder token op switch-locale/store/destroy + assertie dat routes deweb-groep dragen.tests/Feature/CsrfProtectionWithoutWebInConfigTest.php: simuleert config zonder'web'→ nog steeds 419 (bewijst de hardening werkt).tests/TestCase.php: app-key toegevoegd (nodig voor sessie/CSRF-encryptie in tests, ontbrak eerder waardoor deze tests niet eens konden draaien).src/(geverifieerd opmainvóór mijn wijzigingen, ongewijzigd door dit werk — mijn wijzigingen raken alleenroutes/entests/, buiten scope van die scans).Open punten:
src/zijn niet aangepakt (buiten scope van dit issue); mogelijk apart issue waard..zyra/proef.shbestond al en werkt correct (composer install met ext-gd fallback, sqlite-db, migraties, serve op$PORT); niet gewijzigd.