Skip to content

Commit

Permalink
XWIKI-19611: Improve filename escaping in attachment upload
Browse files Browse the repository at this point in the history
* Use escapeHTML() when displaying filenames in upload.js
* Add a test in AttachmentIT to check for proper HTML escaping during upload

(cherry picked from commit 910a501)
  • Loading branch information
pjeanjean authored and michitux committed Jan 24, 2024
1 parent bb232dd commit 8b8a2d8
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ class AttachmentIT

private static final String SECOND_ATTACHMENT = "SmallAttachment2.txt";

private static final String ESCAPED_ATTACHMENT = "<strong>EscapedAttachment.txt";

private static final String IMAGE_ATTACHMENT = "image.gif";

private static final String SMALL_SIZE_ATTACHMENT = "SmallSizeAttachment.png";
Expand Down Expand Up @@ -441,6 +443,23 @@ void deleteAttachmentWithSpecialChar(TestUtils setup, TestReference testReferenc
basePage.getXWikiMessageContent());
}

@Test
@Order(9)
void checkEscapingInAttachmentName(TestUtils setup, TestReference testReference,
TestConfiguration testConfiguration)
{
setup.loginAsSuperAdmin();
setup.createPage(testReference, "Empty content");
AttachmentsPane attachmentsPane = new AttachmentsViewPage().openAttachmentsDocExtraPane();

attachmentsPane.setFileToUpload(getFileToUpload(testConfiguration, ESCAPED_ATTACHMENT).getAbsolutePath());
attachmentsPane.waitForUploadToFinish(ESCAPED_ATTACHMENT);
attachmentsPane.clickHideProgress();

assertTrue(attachmentsPane.attachmentExistsByFileName(ESCAPED_ATTACHMENT));
attachmentsPane.deleteAttachmentByFileByName(ESCAPED_ATTACHMENT);
}

private String getAttachmentsMacroContent(DocumentReference docRef)
{
StringBuilder sb = new StringBuilder();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
This is an attachment whose name should be escaped.
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ var XWiki = (function(XWiki) {

if (this.options.enableFileInfo) {
statusUI.FILE_INFO = UploadUtils.createDiv('file-info');
(statusUI.FILE_NAME = UploadUtils.createSpan('file-name', this.file.name)).title = this.file.type;
(statusUI.FILE_NAME = UploadUtils.createSpan('file-name', this.file.name.escapeHTML())).title = this.file.type;
statusUI.FILE_SIZE = UploadUtils.createSpan('file-size', ' (' + UploadUtils.bytesToSize(this.file.size) + ')');
statusUI.FILE_CANCEL = UploadUtils.createButton("$services.localization.render('core.widgets.html5upload.item.cancel')", this.cancelUpload.bindAsEventListener(this));
// TODO MIME type icon?
Expand Down Expand Up @@ -295,7 +295,7 @@ var XWiki = (function(XWiki) {
this.statusUI.FILE_CANCEL.addClassName('hidden');
}
this.formData.input.fire('xwiki:html5upload:message', {content: 'UPLOAD_FINISHING', type: 'inprogress', source: this,
parameters : {name : this.file.name}
parameters : {name : this.file.name.escapeHTML()}

This comment has been minimized.

Copy link
@mflorea

mflorea Oct 23, 2024

Member

I find it strange to escape the file name as HTML in the event data, as if any code that will listen to this event will inject the file name in the HTML. You don't know this. I'd rather escape the file name in the places where it is injected in HTML.

});
},

Expand Down Expand Up @@ -324,7 +324,7 @@ var XWiki = (function(XWiki) {
}
}
this.formData.input.fire('xwiki:html5upload:message', {content: 'UPLOAD_FINISHED', type: 'done', source: this,
parameters : {name : this.file.name, size : UploadUtils.bytesToSize(this.file.size)}
parameters : {name : this.file.name.escapeHTML(), size : UploadUtils.bytesToSize(this.file.size)}
});
this.formData.input.fire('xwiki:html5upload:fileFinished', {source: this});
clearInterval(this.timer);
Expand Down Expand Up @@ -354,7 +354,8 @@ var XWiki = (function(XWiki) {
*/
abnormalUploadFinish : function (message) {
clearInterval(this.timer);
this.formData.input.fire('xwiki:html5upload:message', {content: message, type: 'error', source: this, parameters : {name : this.file.name}});
this.formData.input.fire('xwiki:html5upload:message', {content: message, type: 'error', source: this, parameters :
{name : this.file.name.escapeHTML()}});
this.formData.input.fire('xwiki:html5upload:fileFinished', {source: this});
}
});
Expand Down Expand Up @@ -517,7 +518,7 @@ var XWiki = (function(XWiki) {
}
} catch (ex) {
this.showMessage(ex, 'error', {size : UploadUtils.bytesToSize(this.options && this.options.maxFilesize),
name : file.name, type: file.type
name : file.name.escapeHTML(), type: file.type
});
}
}
Expand Down

0 comments on commit 8b8a2d8

Please sign in to comment.