Security: Add central CSRF protection for API Platform and legacy AJAX - #8832
Open
AngelFQC wants to merge 27 commits into
Open
Security: Add central CSRF protection for API Platform and legacy AJAX#8832AngelFQC wants to merge 27 commits into
AngelFQC wants to merge 27 commits into
Conversation
AngelFQC
force-pushed
the
feature/central-csrf-protection
branch
from
August 6, 2026 22:55
ba08133 to
9134c54
Compare
23 tasks
…hamilo#8831 The unit test cannot prove the listener is actually plugged into the kernel. This one drives a real API Platform route through KernelBrowser and asserts a forged request comes back as 403, not 401, which is what shows the check runs ahead of the firewall. Requests are sent unauthenticated on purpose: the listener only needs the session cookie to treat a request as a CSRF candidate, so the status code alone identifies which gate stopped it. The session cookie name is read from the configured session instead of being hardcoded. Writes must carry a content type to reach the check at all, since API Platform negotiates the format in AddFormatListener (priority 28) and answers 415 first; noted in the listener docblock.
The two functional tests asserting a rejection came back 401 instead of 403: enforcement never reached the listener. EnvVarProcessor resolves $_ENV before $_SERVER, and setUp() only wrote $_SERVER, so the value won wherever .env leaves the variable undefined and lost wherever it does not -- which is CI, since .env.dist ships it. Both superglobals are now written and restored, and a sanity test asserts the parameter really arrives as true, so a future misconfiguration reports itself instead of surfacing as a puzzling 401. Also applies ecs to both test files: ecs.php covers tests/CoreBundle too, not just src/, so SelfStaticAccessorFixer and OrderedTypesFixer apply there.
…chamilo#8831 The existing functional cases run anonymously, so they only ever observe a status code. These two log in the admin account from the fixtures and issue the same write twice, changing nothing but the origin. Both assert on the rejection message rather than the status: the endpoint answers 403 to an admin for its own authorization reasons, so a bare status assertion could not tell which gate rejected the request -- and would have passed for the wrong reason.
Phase 1: the listener now answers 403 instead of only recording what it would have rejected. The parameter default flips too, not just .env.dist, so an installation that upgrades without regenerating its .env is protected rather than silently left in log-only mode. Measured on the full suite before flipping it, same database and fixtures: log-only: Tests: 653, Assertions: 2444, Errors: 34, Failures: 45, Skipped: 31 enforcing: Tests: 653, Assertions: 2444, Errors: 34, Failures: 45, Skipped: 31 Identical, so nothing in the suite depends on writing with a session cookie and no Origin. The remaining errors and failures are the ones master already has. What the suite cannot measure is third-party traffic. Installations with custom integrations should set CHAMILO_CSRF_ENFORCE=false first and check the log for clients writing without an Origin header; the warning carries the client IP and user agent to identify them.
…hamilo#8831 Uploading a document failed with "The security token is invalid." even though the request carried a correct Origin and Sec-Fetch-Site. The uploads go out through Uppy's XHRUpload, which builds its own XMLHttpRequest and never saw the axios interceptor, so it sent no csrf-token header. That alone would have been harmless -- the origin check accepts the request -- but SameOriginCsrfTokenManager remembers that a session once validated through double-submit and from then on demands it from every later request. So the SPA armed the session over axios and the uploader started getting 403s: a failure that only appears after some unrelated request, which is about the worst shape a bug can take. Patching each caller does not scale. The write paths include 7 Uppy uploaders, 29 PrimeVue FileUpload usages, plain fetch and jQuery, plus whatever plugins add, and any new one would reintroduce the same intermittent failure. So the double-submit is gone: the listener always asks for the origin check, and the axios interceptor, the legacy ajaxSend and the shared token module are removed. Browsers attach Origin and Sec-Fetch-Site to state-changing requests on their own, so no caller has to cooperate and a cross-site page still cannot forge them. What is lost is a second barrier against clients that send neither Origin nor Referer, which no current browser does. Verified with the exact request that failed: the upload now returns 201, repeats cleanly, and a cross-site Origin is still rejected with 403.
…hamilo#8831 Phase 2, first domain. CsrfProtectionListener now guards every state-changing API operation, so the per-endpoint token this domain carried is redundant: the provider no longer emits it, the processors no longer validate it, and the Vue views no longer track it. What goes away is a round trip's worth of coupling. The list and form GETs used to hand out a token purely so the next write could hand it back, the delete processor parsed the request body by hand just to read it, and both views kept it in component state -- which a full page reload discarded, forcing the GET to be redone. Verified end to end against a running install: the list and form responses no longer carry csrfToken, create returns 201, edit 200, delete 204 with no body at all, and a cross-site Origin on the same create is still rejected with 403. The two PHPStan findings in this directory (setDescriptionType on AbstractResource, and an always-present offset in the provider) predate this change; confirmed by analysing the same paths with the changes stashed.
AngelFQC
force-pushed
the
feature/central-csrf-protection
branch
from
August 7, 2026 17:04
35f9060 to
0e565d3
Compare
…refs chamilo#8831 Testing the Ticket cleanup surfaced this: a POST to /api/ticket/admin/projects carrying Origin: https://evil.example was answered with 201, and created the project. The listener only guarded requests whose route resolved an _api_resource_class, i.e. API Platform operations. Of the routes under /api, 469 are those and 246 are plain Symfony controllers -- ticket admin, announcements, translations, the question bank. Those 246 were never covered. It went unnoticed through phases 0 and 1 because those endpoints are precisely the ones carrying a hand-written check, which kept them safe. Removing the manual checks is what exposed the gap, and it would have shipped as a hole had Ticket been cleaned up without testing the attack. Everything under /api is now guarded. Plain controllers have no metadata, so they cannot opt out either -- the extraProperties escape hatch stays available only where there is an operation to declare it on. Verified on the endpoints that failed: cross-site POST and DELETE now 403, legitimate ones still 201 and 200, and JWT login with no cookie stays 200.
Phase 2, second domain. Removed across 14 files: 19 validateCsrfToken() calls in the two admin/workflow controllers, the emission in three providers, the csrfToken property on three API resources, the CSRF_TOKEN_ID constant on the workflow service, and the tracking in four Vue views plus the service. Ticket used both delivery paths -- the token in the JSON body and, for deletes, an X-CSRF-TOKEN header -- so seven service methods lose a parameter they only carried to pass it along. Two refreshCsrfToken() helpers also go. They existed because a token fetched at mount goes stale while a dialog sits open, so each mutation had to re-fetch one first. Verifying the request origin has no expiry, so the extra round trip before every write disappears with them. Verified against a running install: admin configuration and ticket list no longer return csrfToken, creating a project returns 201 and deleting it 200 with no token anywhere, and the same calls from a foreign Origin are rejected with 403 by the listener. The five PHPStan findings under these paths predate this change; confirmed by analysing the same paths with the changes stashed.
…o#8831 Phase 2, third domain and the most tangled so far: four separate token ids (announcement_manage, announcement_form, announcement_email, announcement_attachment) feeding three distinct properties on the API resources -- csrfToken, emailCsrfToken and attachmentCsrfToken. Removed across 16 files: the four constants, five validations, ten emissions, eight properties on five resources, and the tracking in three Vue views plus seven service methods that each carried a token parameter. The attachment controller used both delivery paths, a form field for uploads and an X-CSRF-TOKEN header for deletes, which is why the form provider had to mint three different tokens for a single page. Verified against a running install: list and form responses carry none of the three properties, create returns 201, visibility 204 and delete 204 with no token anywhere, while the same visibility and delete-all calls from a foreign Origin are rejected with 403 -- these are controller routes under /api, so they are covered by the widening in bb97ce8. The six PHPStan findings under these paths come from AnnouncementAccessHelper- Trait and predate this change; confirmed by analysing with the changes stashed.
… - refs chamilo#8831 Phase 2, fourth pass. CourseClass comes along because CourseClassListView.vue lives under views/courseUser/ and calls courseClassService: removing the token from the view without touching that service would have left it sending a parameter nobody reads. Removed across 19 files: the course_user_actions and course_class_management token ids, four validations written inline rather than through a helper, five emissions, six properties across the API resources, and the tracking in four Vue views plus two services. CourseUserImport also declared csrfToken in its multipart OpenAPI schema, so the documented contract for that endpoint no longer advertises a field clients must not send. Verified against a running install: the list, available, import and class-list responses no longer carry csrfToken; subscribe and unsubscribe reach their own validation (400 "No valid users were selected" for a user not in the course, i.e. past the CSRF gate), and both subscribe and the class add action are rejected with 403 from a foreign Origin. The four PHPStan findings under these paths belong to CourseUserManager and CourseClassManager, neither of which this commit touches.
…#8831 Phase 2, fifth domain and the most uniform: one intention, course_group_manage- ment, reached through CourseGroupManager::getCsrfIntention() from every provider and processor. Removed across 26 files: the constant and its accessor on the manager, five validations, seven emissions, seven properties on the API resources, and the tracking in six Vue views plus two service methods. CourseGroupImport also advertised csrfToken in its multipart OpenAPI schema. Removing the token left a `const response = await getImport(...)` in CourseGroupImportView with nothing left to read from it; eslint caught it and the call is now awaited without binding. Re-checked the four domains already merged for the same pattern -- none of them left a dead variable behind. Verified against a running install: the list, form, import and category-form responses no longer carry csrfToken, creating a group returns 201, and both the create action and the multipart import are rejected with 403 from a foreign Origin. The four PHPStan findings in CourseGroupManager are about create_category() argument types and predate this change; confirmed by analysing with the changes stashed.
The central listener verifies the origin of everything under /api, so the per-endpoint token this domain carried no longer buys anything. It cost a round trip on every page that writes, since the token was emitted by the GET provider and each Vue view had to fetch one before it could submit. 30 files, six token intentions gone: survey_action, survey_answer, survey_configuration, survey_invitation, survey_meeting and survey_question. Two things this domain had that the previous five did not: SurveyCsrfTokenValidationTrait goes entirely. It hunted the token across five delivery paths — the resource property, the JSON body, the form body and two spellings of the header — because the seven processors that used it could not agree on where the client put it. Origin verification needs none of that. Seven private validateCsrfToken() methods went with it, and they were already dead: the trait had superseded them and nothing called them. PHPStan was saying so on every run — its Survey findings drop from 12 to 5, and the 7 that disappear are exactly those methods. The remaining 5 are pre-existing. Also removed: actionCsrfToken, which SurveyListProvider stamped onto every row of the list so the bulk actions could read one back out, and the selectedSurveys computed in SurveyListView that existed only to find it. Verified against a running install: the list, configuration, questions and meeting GETs no longer carry a token; creating a survey, a question, copying, duplicating, emptying, publishing invitations, bulk-deleting and patching the configuration all work without one; and every one of those writes is rejected with 403 from Origin: https://evil.example.
The central listener verifies the origin of everything under /api, so the per-endpoint token this domain carried no longer buys anything. It cost a round trip on every page that writes, since the token was emitted by the GET provider and each Vue view had to fetch one before it could submit. 35 files, six token intentions gone: wiki_page_management, wiki_discussion, wiki_settings, wiki_category_management, wiki_page_form and wiki_page_restore. Unlike Survey, none of the seven private validateCsrfToken() methods was dead code — each had exactly one live caller, removed along with it. Three resources also carried the token under names a plain grep for csrfToken misses: managementCsrfToken on WikiPage and WikiReport, writeCsrfToken on WikiDiscussion. WikiPageFormView.releaseLock() loses !form.value.csrfToken from its guard. lockAcquired already covered that case: it could only become true through an acquireLock() that itself required a valid token. Verified against a running install: every write path under /api/wiki — create and edit a page, delete it, restore a version, lock, unlock, toggle visibility, protection and subscription, create and delete a category, save settings, delete the context — is rejected with 403 from Origin: https://evil.example, and the GET providers no longer emit a token.
The central listener verifies the origin of everything under /api, so the per-endpoint token this domain carried no longer buys anything. It cost a round trip on every page that writes, since the token was emitted by the GET provider and each Vue view had to fetch one before it could submit. 41 files, nine token intentions gone: exercise_list_action, exercise_configuration, exercise_question_action, exercise_question_editor, exercise_question_bank_action, exercise_question_import, exercise_category_management, exercise_runtime_report_bulk_action and exercise_runtime_report_email_action. Pure deletion: 258 lines removed, none added. ExerciseRuntimeReport carried its two tokens as bulkActionToken and emailActionToken, so it never showed up in a grep for csrfToken and had to be found by reading the processors that consumed them. Worth remembering for the domains still pending: the property name is not a reliable index. ExerciseCategoryManagerView.saveCategory() also loses a refetch guard that reloaded the whole category list when the token was still empty, with a comment explaining it existed solely to close the race on an early submit. That round trip is exactly what this change removes. Verified against a running install: creating and editing an exercise, the list bulk action, question actions, the question bank action, imports, the runtime report bulk delete and its email action are all rejected with 403 from Origin: https://evil.example, and the GET providers no longer emit a token.
…o#8831 The central listener verifies the origin of everything under /api, so the per-endpoint token this domain carried no longer buys anything. It cost a round trip on every page that writes, since the token was emitted by the GET provider and each Vue view had to fetch one before it could submit. 78 files and a single intention, learning_path_action, reached through LearningPathStateHelperTrait::validateActionToken() from 21 call sites. Only that method and its constant leave the trait; its other ten helpers — course, session and group context, resource links, visibility — have 36 consumers and stay. LearningPathRuntimeWriteProtection is deleted outright, with its test. The class did nothing but wrap a token check behind a JWT-request bypass, so once the check goes there is no class left. Its four callers lose the dependency. LearningPathActionTokenProvider is NOT deleted, despite the name. Besides the token it emits allowChamiloExport, which LpList.vue reads to decide whether the export button renders, so only the token leaves. The endpoint name is now a misnomer; renaming it would churn the URL and three callers, so it stays as is. In six lp components the token doubled as a permission flag through disabled: !props.csrfToken. It was only ever populated for users who could edit, so those guards now read props.canEdit, which is strictly narrower. The bindings that dropped the clause sit inside v-if="canEdit" already. One resource keeps its token on purpose: LpAdvancedAccess.vue talks to /resources/lp/{lpId}/advanced-access, a plain controller the listener does not reach — it guards /api and /main/inc/ajax/ only. Removing the token there would have left those writes undefended. Confirmed by attacking the route: a cross-site POST reaches the controller instead of being rejected. Verified against a running install: managing and deleting categories, deleting and editing builder items, quick tests, reporting reset and recalculate, LP management, SCORM import, update and commit, runtime restart and sync, the AI generator, builder reorder and resource attach are all rejected with 403 from Origin: https://evil.example, and the GET providers no longer emit a token.
…ession - refs chamilo#8831 The central listener verifies the origin of everything under /api, so the per-endpoint token these domains carried no longer buys anything. It cost a round trip on every page that writes, since the token was emitted by the GET provider and each Vue view had to fetch one before it could submit. Both domains are served entirely by API Platform under /api/course-progress/* and /api/course-sessions/*, so the listener reaches every one of their write paths — checked route by route before removing anything, because 239 write routes in this application sit outside /api and there the manual token is the only defense there is. Verified against a running install: creating a thematic, the thematic bulk delete, the completion update, and subscribing and unsubscribing session users are all rejected with 403 from Origin: https://evil.example, and the GET providers no longer emit a token.
…hamilo#8831 The central listener verifies the origin of everything under /api, so the per-endpoint token these domains carried no longer buys anything. Forum paid the round trip twice over. Its views called ensureToken() before every single action — delete a thread, lock it, approve a post, report one — and ForumList called loadToken() before eleven of them, each an extra request whose only purpose was to have a fresh token in hand. All of that is gone; the list now loads its settings once, at mount. /forum/action-token is NOT deleted, despite the name. Besides the token it emits the forum settings (default view, folded categories, post revisions, the category language filter), which ForumList still reads, so only the token leaves and the endpoint keeps its now-inaccurate name. Same call that was made for LearningPathActionTokenProvider. Where the token was the only thing a request fetched — the getActionToken() sitting inside the Promise.all of ForumThreadList, ForumPostList, ForumReply and ForumCreateThread — the whole call goes, saving one request per page load. Verified against a running install: deleting a forum category, toggling its lock and deleting a post are rejected with 403 from Origin: https://evil.example, and the GET providers no longer emit a token.
…hamilo#8831 Notebook, CourseInvitation, Admin (question bank), Ai and CourseSettings. The central listener verifies the origin of everything under /api, so the per-endpoint token they carried no longer buys anything. One token stays, deliberately. CourseSettingsManager keeps emitting generatePictureCsrfToken for /ai/generate_course_picture, which is a plain controller outside /api — the listener guards /api and /main/inc/ajax/ only, so removing it would leave that write with no CSRF defense at all. Every other call in courseSettingsService goes to /api/course-settings/*, and those lose the token: the eight upload and delete methods, the shared formData() helper that appended it to every multipart body, and the X-CSRF-TOKEN header that CoursePictureUploader handed to Uppy's XHRUpload. That uploader is the one that broke under the double-submit cookie tried earlier in this issue; under origin verification it needs to send nothing at all. The same split had to be made across the whole domain, endpoint by endpoint: in this application 239 write routes sit outside /api, and there the manual token is the only thing standing. Verified against a running install: saving course settings, creating a notebook entry, the question bank action and creating a course invitation are all rejected with 403 from Origin: https://evil.example, and the GET providers no longer emit a token.
…refs chamilo#8831 Scoping the listener to /api left 136 declared write routes outside it, and there the hand-written token was the only CSRF defense. LpAdvancedAccessController was the case that surfaced it: six write routes, one of which validated a token. A cross-site POST to /resources/lp/{lpId}/advanced-access/user reached the controller instead of being rejected. The obvious fix — an allowlist of protected prefixes — was rejected because it reproduces the very defect this issue set out to remove. A list of included prefixes has to be extended for every new route, and forgetting leaves that route silently unguarded, which is exactly how the per-endpoint token failed. So the default is inverted instead: everything the router resolved is guarded, whatever its prefix, and only explicit exceptions opt out. That flips the failure mode. Forgetting to exclude a route that genuinely needs it surfaces as a 403 the first time the feature is exercised — loud and immediate — instead of as a hole nobody sees. Almost nothing needs excluding, because the two guards already in isProtected() do the work: a request with no session cookie, or one carrying Authorization or X-Chamilo-Api-Key, is skipped. That covers every server-to-server caller — /oauth/*, /scim/v2/*, /mcp, the videoconference callback — and the login endpoints, which have no previous session by definition. It is also why the third-party endpoints that already live under /api have needed no exclusion since phase 1. What remains is the one category the guards cannot catch: a cross-site POST from a browser that does carry a Chamilo session. Today that is LTI Deep Linking's item_return, and it is the only entry in EXCLUDED_ROUTES. Legacy pages under public/main/ stay out, as before. The web server executes them and only then boots the kernel, so no route is ever resolved — that absence is now what keeps them out of scope, and their form posts go on validating their own FormValidator token. With the route covered centrally, LpAdvancedAccessController loses the manual check it had in saveUserGroups and the token it emitted from data(), and the Vue view stops carrying one. Rolling this out on an existing installation should start with CHAMILO_CSRF_ENFORCE=false, since it widens what gets checked: let the log run, see who would have been rejected, then enforce. Same procedure phase 1 used. Verified against a running install: all three advanced-access writes are now rejected with 403 from Origin: https://evil.example and work from the site's own origin; logging in still works; the /admin routes that carry their own token are unaffected; and a legacy page POST is still not touched by the listener.
…amilo#8831 Six CourseBundle entities still declared csrfToken in their operations' requestBody schemas — four Forum, two LearningPath — and listed it under 'required'. They were missed when those domains were cleaned up, because the schemas live on the entity while the work was scoped to State/ and ApiResource/. Nothing was broken by it: API Platform treats these schemas as documentation and does not enforce 'required' at runtime, which is why a multipart POST to /api/forum_posts/reply without the field still reached business validation. But the published contract said the field was mandatory while no processor read it any more and no caller sent it, so anyone generating a client from the spec would have shipped a dead field — and it would have turned into a real failure the day schema validation was switched on. The three requestBody descriptions that mentioned the token go too, and the two image-upload schemas on CForum lose their 'required' key entirely, since the token was the only entry in it. Verified: api:openapi:export now contains zero occurrences of csrfToken across the whole 15MB spec, ECS is clean, and the affected endpoints answer exactly as before — reply without the field reaches "Forum thread not found", and the same call from a foreign origin is still rejected by the listener.
…hamilo#8831 Phase 4 widened the listener to every routed request, so the /admin/usergroup* endpoints are now guarded centrally and the hand-written token they carried is dead weight. Six controllers, seven Vue views and two service methods: six token intentions gone (usergroup_list, usergroup_import, usergroup_courses, usergroup_sessions, usergroup_users, usergroup_add_users), along with the X-CSRF-Token headers the two delete methods sent. Three round trips disappear with them. UsergroupList called refreshCsrfToken() before every mutation — a whole list request with limit: 1, fetched only to get a fresh token, because the one loaded with the page went stale while a dialog sat open. UsergroupImport and UsergroupUserImport each did the same on mount through loadCsrf(). Origin verification has no expiry, so none of that is needed; both import views lose their onMounted entirely. Verified against a running install: POST and DELETE on /admin/usergroups-data and the CSV import are rejected with 403 from Origin: https://evil.example, and from the site's own origin reach business logic ("Not found", "No file uploaded"). ECS clean, ESLint back to 0 on all eight touched frontend files.
…on lists - refs chamilo#8831 The listener covers /admin since phase 4, so the tokens on the three list actions are redundant: user_list_action, admin_course_list and session_list_action go, along with the hidden _token input CourseList built into a dynamically created form. One token stays, and it is the interesting one. "Login as" is GET /admin/user-list-login-as, and the listener never inspects safe methods — it returns before anything else on isMethodSafe(). So that route is outside the gate no matter how wide the scope gets, and its login_as token is the only thing between an admin session and an attacker-chosen impersonation triggered by nothing more than an <img> tag on a page the admin visits. UserListController keeps emitting it, UserLoginAsController keeps validating it, and the reason is now written next to the code. Worth noting for the routes still to come: from here on the question is not only "is this path covered" but "is this method inspected at all". A state-changing GET is invisible to the gate by design. Verified against a running install: the three POST actions are rejected with 403 from Origin: https://evil.example and reach business logic from the site's own origin ("User not found."), while login-as with a bad token still answers 403 on its own.
…hamilo#8831 Nine POST endpoints under /admin/system-update, all covered by the listener since phase 4. The system_update intention goes, along with the two helpers built around it: isValidCsrfPayload() on the controller, which read the token from X-CSRF-Token and fell back to the JSON body, and withCsrfPayload() in the Vue view, which wrapped seven request payloads for no other purpose. The GET endpoints on this controller (status, progress) were never in scope — the listener skips safe methods — and they emitted nothing to validate anyway. Verified against a running install: check and apply-files are rejected with 403 from Origin: https://evil.example, and check from the site's own origin reaches the updater itself (it fails fetching the manifest, which is unrelated). ECS clean; ESLint on the view drops from 89 pre-existing warnings to 57.
…amilo#8831 With this, `grep -rli csrf assets/vue/` returns nothing: no Vue file emits, stores, reads or sends a CSRF token any more. The gate is entirely in the listener. Four leftovers, three of them invisible until the frontend was swept as a whole: LinkForm and AssignmentsCreate still passed builder.csrfToken to lpService.addBuilderResource(). They are the same pattern already removed from SurveyQuestionsView during the LearningPath phase, but they live under components/links/ and views/assignments/, so they fell outside that domain's scope. As there, the getBuilder() call above them existed only to read the token, so the whole request goes — one round trip less per link and per assignment added to a learning path. TermsEdit kept round-tripping a token to /api/terms_and_conditions_translation, whose backend stopped validating it two phases ago. Pure residue. The AI course-picture generator is the one place where frontend and backend had to move together: CourseSettingsManager emitted the token, AiController validated it, and the Vue panel carried it between them. All three go. This is also the token deliberately kept back in phase 3, when /ai/ was still outside the listener's reach — phase 4 covered it, so it can leave now. Not touched, on purpose: the csrf_token() calls in Twig templates and the controllers that validate them (AdminController, SecurityController, the AI course analyzer). That is Symfony's own form idiom and it works; there is no reason to convert it. Verified against a running install: generate_course_picture and the terms translation are rejected with 403 from Origin: https://evil.example, and from the site's own origin reach their business logic. ECS clean, and every touched frontend file is back at its pre-existing ESLint count.
…rf-protection # Conflicts: # assets/vue/components/lp/LpCardItem.vue # assets/vue/components/lp/LpRowItem.vue # assets/vue/views/lp/LpList.vue # src/CoreBundle/State/Exercise/ExerciseConfigurationProcessor.php # src/CoreBundle/State/LearningPath/LearningPathActionTokenProvider.php
23 tasks
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.
Refs #8831. A single central CSRF gate for the whole application, replacing ~40 hand-written token intentions. Enforcing by default.
Problem
API Platform ships no CSRF mechanism (
grep -rli csrf vendor/api-platform/returns 0 files; the docs warn about the risk and defer to the host framework). So protection was opt-in and done by hand: 177 files undersrc/injectedCsrfTokenManagerInterface, 156 of them insrc/CoreBundle/State/.Three problems, all structural:
validateCsrfToken()is silently unprotected and nothing flags it. Plain Doctrine-backed#[ApiResource]CRUD validated nothing at all.Security::check_token()applied unevenly.Approach
CsrfProtectionListenervalidates in one place atkernel.requestpriority 9 — after the router resolves the route, before the firewall, so a forged request is rejected before paying for authentication or course-context lookup.It verifies the request comes from this site via
Sec-Fetch-Site,OriginorReferer, using Symfony 7.4'sSameOriginCsrfTokenManager. Nothing is emitted server-side and nothing is expected from the client: browsers attach those headers to state-changing requests on their own. That matters here, where writes go out through axios, jQuery, Uppy, PrimeVue uploaders and plainfetch.Scope is default-protected. Every request whose route the router resolved is guarded, whatever its prefix. Three things sit outside, all deliberate:
Authorization/X-Chamilo-Api-KeyheaderEXCLUDED_ROUTES— currently one entryitem_returnlegitimately posts cross-site from a browserpublic/main/FormValidatortoken still appliesAn allowlist of protected prefixes was written and then thrown away: it has to be extended for every new route, and forgetting leaves that route silently unguarded — the exact defect this PR set out to remove. Defaulting to protected inverts the failure mode: forgetting to exclude a route surfaces as a 403 the first time the feature is used.
What this removes
CsrfTokenManagerInterfaceinjections insrc/: 177 → 8.grep -rli csrf assets/vue/returns nothing — no Vue file emits, stores, reads or sends a token any more.Also gone: the machinery each token needed.
UsergroupListfetched a whole list (limit: 1) before every mutation just to refresh its token;SystemUpdatewrapped seven payloads inwithCsrfPayload()mirrored byisValidCsrfPayload();LinkFormandAssignmentsCreateeach calledgetBuilder()solely to read one field off the response;CourseListbuilt a<form>in JavaScript to inject a hidden_token; Forum calledensureToken()before every action. Two classes existed only to wrap a token check and are deleted with their tests.What deliberately stays
csrf_token()and the controllers that validate it (AdminController,SecurityController, the AI course analyzer, ContactCategory, Account, OAuthServer). That is Symfony's own form idiom, the listener sits behind it as a second barrier, and converting it would buy nothing.login_askeeps its token./admin/user-list-login-asis a GET route, and the listener returns onisMethodSafe()before looking at anything else — so it is outside the gate no matter how wide the scope gets. That token is the only thing between an admin session and an attacker-chosen impersonation triggered by an<img>tag.Verification
Every domain was checked against a running install before being committed, and the check is not ceremony — it is what caught a real gap mid-way (details):
Origin: https://evil.examplecome back 403.Across the branch that is 60+ write endpoints attacked and rejected. Plus: 30 listener tests,
lint:container, ECS clean, PHPStan compared against a per-path baseline with zero new findings, and every touched frontend file back at its pre-existing ESLint count.When attacking in bulk, match on the response body (
{"error":"The security token is invalid."}) rather than the status code: a 403 can come from business logic, a 404/405 means the route was composed wrong, and a 415 means API Platform'sAddFormatListener(priority 28) rejected the content type before the gate (priority 9) ever ran.Rollout
Enforcement is on by default, and the parameter default flips too, not just
.env.dist— an installation upgrading without regenerating its.envends up protected rather than silently unguarded.An installation with custom integrations should set
CHAMILO_CSRF_ENFORCE=falsefirst, let it run, and check who would have been rejected:The one case that cannot be ruled out ahead of time is an integration that authenticates by session cookie rather than by a token — nothing can exempt that automatically. Three ways out, in order of preference: send the Bearer token it already has, send an
X-Chamilo-Api-Key, or add the route toEXCLUDED_ROUTES.Rollback needs no redeploy:
CHAMILO_CSRF_ENFORCE=falsereturns to log-only immediately, and the warnings carry path, method, client IP and user agent.