Repository navigation
upload/image: Require login and escape markup in the JSON answer - #511
Open
guidol-ive wants to merge 1 commit into
Open
guidol-ive wants to merge 1 commit into
guidol-ive wants to merge 1 commit into
Conversation
The image upload endpoint accepted uploads from anonymous visitors and echoed the client-side file name in a text/html response without escaping. Refuse the upload with HTTP 403 unless the visitor is logged in, and encode the JSON with the JSON_HEX_* flags so no markup characters appear in the response body. The content type stays text/html, since the editor reads the answer from a hidden iframe. Fixes PlanBCode#416 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
guidol-ive
added
Type: Bug
Level: Easy
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 #416: the image upload endpoint accepted uploads without login and reflected the file name unescaped in a
text/htmlresponse.Change
In
system/core/pages.php,upload/image:[{"error": ...}], the format the editor already shows as an alert, and the existing "login-to-perform-action" message. This happens before any file handling, so nothing is stored.JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT, so<,>,&,'and"are sent as\uXXXX. The decoded values are unchanged, so the editor still gets the original file name for the alt text.The content type stays
text/htmlon purpose: the WYMeditor plugins post the upload into a hidden iframe and parsebody.html()of that iframe. Withapplication/json, some browsers show their JSON viewer in the iframe and the upload breaks.The endpoint still has no CSRF token check; with this change only members reach it, and the session cookie is
SameSite=Strict. A token in the editor's upload form could be a follow-up.Testing
Tested in a
php:<version>-apachecontainer on PHP 8.5, and on PHP 8.2 with #475 (now merged intomain), against a copy of a ~1,330-page data set. The uploads were a 1×1 PNG, posted asmultipart/form-datalike the editor does.maindata/imagesphoto <b>bold.png<b>in the response<b>; decodes to the same nameThe results are the same on 8.2 and 8.5. No new PHP warnings.
php -lpasses on both.A side note from testing: PHP keeps only the part after the last
/of an uploaded file name, so a name with a closing tag such as</b>arrives cut off. That is PHP's own behaviour and not changed here.🤖 Generated with Claude Code