Skip to content
This repository has been archived by the owner on Jan 27, 2021. It is now read-only.

Commit

Permalink
Improved article quoting.
Browse files Browse the repository at this point in the history
  • Loading branch information
@dvuckovic
dvuckovic committed Sep 11, 2019
1 parent 27f9972 commit 03ca8f3
Show file tree
Hide file tree
Showing 5 changed files with 101 additions and 33 deletions.
22 changes: 14 additions & 8 deletions Kernel/Modules/AgentTicketActionCommon.pm
View file
Expand Up @@ -1329,14 +1329,20 @@ sub Run {
# set Body var to calculated content
$GetParam{Body} = $Body;

# Strip out external content if needed.
if ( $ConfigObject->Get('Ticket::Frontend::BlockLoadingRemoteContent') ) {
my %SafetyCheckResult = $Kernel::OM->Get('Kernel::System::HTMLUtils')->Safety(
String => $GetParam{Body},
NoExtSrcLoad => 1,
);
$GetParam{Body} = $SafetyCheckResult{String};
}
my %SafetyCheckResult = $Kernel::OM->Get('Kernel::System::HTMLUtils')->Safety(
String => $GetParam{Body},

# Strip out external content if BlockLoadingRemoteContent is enabled.
NoExtSrcLoad => $ConfigObject->Get('Ticket::Frontend::BlockLoadingRemoteContent'),

# Disallow potentially unsafe content.
NoApplet => 1,
NoObject => 1,
NoEmbed => 1,
NoSVG => 1,
NoJavaScript => 1,
);
$GetParam{Body} = $SafetyCheckResult{String};

if ( $Self->{ReplyToArticle} ) {
my $TicketSubjectRe = $ConfigObject->Get('Ticket::SubjectRe') || 'Re';
Expand Down
22 changes: 14 additions & 8 deletions Kernel/Modules/AgentTicketCompose.pm
View file
Expand Up @@ -1180,14 +1180,20 @@ sub Run {
UploadCacheObject => $UploadCacheObject,
);

# Strip out external content if needed.
if ( $ConfigObject->Get('Ticket::Frontend::BlockLoadingRemoteContent') ) {
my %SafetyCheckResult = $Kernel::OM->Get('Kernel::System::HTMLUtils')->Safety(
String => $Data{Body},
NoExtSrcLoad => 1,
);
$Data{Body} = $SafetyCheckResult{String};
}
my %SafetyCheckResult = $Kernel::OM->Get('Kernel::System::HTMLUtils')->Safety(
String => $Data{Body},

# Strip out external content if BlockLoadingRemoteContent is enabled.
NoExtSrcLoad => $ConfigObject->Get('Ticket::Frontend::BlockLoadingRemoteContent'),

# Disallow potentially unsafe content.
NoApplet => 1,
NoObject => 1,
NoEmbed => 1,
NoSVG => 1,
NoJavaScript => 1,
);
$Data{Body} = $SafetyCheckResult{String};

# restrict number of body lines if configured
if (
Expand Down
22 changes: 14 additions & 8 deletions Kernel/Modules/AgentTicketForward.pm
View file
Expand Up @@ -295,14 +295,20 @@ sub Form {
AttachmentsInclude => 1,
);

# Strip out external content if needed.
if ( $ConfigObject->Get('Ticket::Frontend::BlockLoadingRemoteContent') ) {
my %SafetyCheckResult = $Kernel::OM->Get('Kernel::System::HTMLUtils')->Safety(
String => $Data{Body},
NoExtSrcLoad => 1,
);
$Data{Body} = $SafetyCheckResult{String};
}
my %SafetyCheckResult = $Kernel::OM->Get('Kernel::System::HTMLUtils')->Safety(
String => $Data{Body},

# Strip out external content if BlockLoadingRemoteContent is enabled.
NoExtSrcLoad => $ConfigObject->Get('Ticket::Frontend::BlockLoadingRemoteContent'),

# Disallow potentially unsafe content.
NoApplet => 1,
NoObject => 1,
NoEmbed => 1,
NoSVG => 1,
NoJavaScript => 1,
);
$Data{Body} = $SafetyCheckResult{String};

if ( $LayoutObject->{BrowserRichText} ) {

Expand Down
22 changes: 14 additions & 8 deletions Kernel/Modules/AgentTicketPhone.pm
View file
Expand Up @@ -351,14 +351,20 @@ sub Run {
$Article{ContentType} = 'text/plain';
}

# Strip out external content if needed.
if ( $ConfigObject->Get('Ticket::Frontend::BlockLoadingRemoteContent') ) {
my %SafetyCheckResult = $Kernel::OM->Get('Kernel::System::HTMLUtils')->Safety(
String => $Article{Body},
NoExtSrcLoad => 1,
);
$Article{Body} = $SafetyCheckResult{String};
}
my %SafetyCheckResult = $Kernel::OM->Get('Kernel::System::HTMLUtils')->Safety(
String => $Article{Body},

# Strip out external content if BlockLoadingRemoteContent is enabled.
NoExtSrcLoad => $ConfigObject->Get('Ticket::Frontend::BlockLoadingRemoteContent'),

# Disallow potentially unsafe content.
NoApplet => 1,
NoObject => 1,
NoEmbed => 1,
NoSVG => 1,
NoJavaScript => 1,
);
$Article{Body} = $SafetyCheckResult{String};

# show customer info
if ( $ConfigObject->Get('Ticket::Frontend::CustomerInfoCompose') ) {
Expand Down
46 changes: 45 additions & 1 deletion scripts/test/Selenium/Agent/AgentTicketForward.t
View file
Expand Up @@ -175,7 +175,15 @@ $Selenium->RunTest(
ArticleType => 'email-external',
SenderType => 'customer',
Subject => 'some short description',
Body => '<!DOCTYPE html><html><body>' . $RandomID . '<br /><img src="' . $ImgSource . '"/></body></html>',
Body =>
'<!DOCTYPE html><html><body>'
. $RandomID . '<br /><img src="' . $ImgSource . '"/>'
. '<applet code="foo.class"></applet>'
. '<object data="bar.swf"></object>'
. '<embed src="baz.swf">'
. '<svg width="100" height="100"><circle cx="50" cy="50" r="40" fill="yellow" /></svg>'
. '<script type="text/javascript">alert(1);</script>'
. '</body></html>',
Charset => 'utf-8',
MimeType => 'text/html',
HistoryType => 'EmailCustomer',
Expand Down Expand Up @@ -203,6 +211,24 @@ $Selenium->RunTest(
"BlockLoadingRemoteContent is disabled - external content is loaded"
);

# Verify potentially unsafe tags are not present.
$Self->False(
$Selenium->execute_script("return CKEDITOR.instances.RichText.getData().indexOf('<applet') > -1;"),
'APPLET tag is stripped from the quoted content'
);
$Self->False(
$Selenium->execute_script("return CKEDITOR.instances.RichText.getData().indexOf('<embed') > -1;"),
'EMBED tag is stripped from the quoted content'
);
$Self->False(
$Selenium->execute_script("return CKEDITOR.instances.RichText.getData().indexOf('<svg') > -1;"),
'SVG tag is stripped from the quoted content'
);
$Self->False(
$Selenium->execute_script("return CKEDITOR.instances.RichText.getData().indexOf('<script') > -1;"),
'SCRIPT tag is stripped from the quoted content'
);

# Enable global external content blocking.
$Helper->ConfigSettingChange(
Valid => 1,
Expand All @@ -223,6 +249,24 @@ $Selenium->RunTest(
"BlockLoadingRemoteContent is enabled - external content is not loaded"
);

# Verify potentially unsafe tags are not present.
$Self->False(
$Selenium->execute_script("return CKEDITOR.instances.RichText.getData().indexOf('<applet') > -1;"),
'APPLET tag is stripped from the quoted content'
);
$Self->False(
$Selenium->execute_script("return CKEDITOR.instances.RichText.getData().indexOf('<embed') > -1;"),
'EMBED tag is stripped from the quoted content'
);
$Self->False(
$Selenium->execute_script("return CKEDITOR.instances.RichText.getData().indexOf('<svg') > -1;"),
'SVG tag is stripped from the quoted content'
);
$Self->False(
$Selenium->execute_script("return CKEDITOR.instances.RichText.getData().indexOf('<script') > -1;"),
'SCRIPT tag is stripped from the quoted content'
);

# Delete created test ticket.
my $Success = $TicketObject->TicketDelete(
TicketID => $TicketID,
Expand Down

1 comment on commit 03ca8f3

@22tcp
Copy link

@22tcp 22tcp commented on 03ca8f3 Oct 17, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got a display of a scalar variable after applying this patch when adding a new, empty note - richtext is on -
Dumper tells me that HTMLUtils->Safety is returning a ref and the code above does not catch that.
I had to ${$SafetyCheckResult{String}} to get an empty CKEditor when adding a note. Else it displays the scalar with the reference address instead of no text at all.

Please sign in to comment.