Fix SEF plugin breaking HTML while trying to make inline background url paths absolute by ggppdk · Pull Request #11266 · joomla/joomla-cms

ggppdk · 2016-07-23T13:38:45Z

Pull Request for Issue #11264 and issue: #7267

The SEF plugin attempts to make inline background url paths absolute but adding / or /jfolder/ to them

Example of broken cases:

<div id='bg-image' style='background-image:url(database/bg-home.jpg);'></div>
<div id='bg-image4' style='background-image:url("database/bg-home.jpg");'></div>
<div id='bg-image5' style='background-image:url(&34;database/bg-home.jpg&34;);'></div>

the above will become:

<div id='bg-image' style="background-image: url('/jfolder/database/bg-home.jpg');'></div>
<div id='bg-image4' style="background-image: url('/jfolder/database/bg-home.jpg');'></div>
<div id='bg-image5' style="background-image: url('/jfolder/&34;database/bg-home.jpg');'></div>

which are obviously broken

Summary of Changes

Added handling of cases

style='...url("...")'
style='...url(&#34;...&#34;);'
style="...url(&#39;...&#39;);"

Testing Instructions

Apply patch and at the bottom of index.php of protostar (just before </body> TAG) add:

<!-- RELATIVE urls (this must be replaced) -->
        <!-- WITHOUT quotes: url() -->
    <div id='rel-image1' style='background-image:url(relative/bg-home.jpg);'></div>
    <div id='rel-image2' style="background-image:url(relative/bg-home.jpg);"></div>
    <div id='rel-image1a' style='color:red; background-image:url(relative/bg-home.jpg);'></div>
    <div id='rel-image2a' style="color:red; background-image:url(relative/bg-home.jpg);"></div>

        <!-- WITH single/double quotes:  url("") or url('') -->
    <div id='rel-image3' style="background-image:url('relative/bg-home.jpg');"></div>
    <div id='rel-image4' style='background-image:url("relative/bg-home.jpg");'></div>
    <div id='rel-image3a' style="color:red; background-image:url('relative/bg-home.jpg');"></div>
    <div id='rel-image4a' style='color:red; background-image:url("relative/bg-home.jpg");'></div>    

        <!-- Single/double quotes written as special characters &#34; &#39;-->
    <div id='rel-image5' style='background-image:url(&#34;relative/bg-home.jpg&#34;);'></div>
    <div id='rel-image6' style="background-image:url(&#39;relative/bg-home.jpg&#39;);"></div>
    <div id='rel-image7' style='background-image:url(&#034;relative/bg-home.jpg&#034;);'></div>
    <div id='rel-image8' style="background-image:url(&#039;relative/bg-home.jpg&#039;);"></div>


<!-- ABSOLUTE urls (plugin must NOT replace) -->
        <!-- WITHOUT quotes: url() -->
    <div id='abs-image1' style='background-image:url(http://absolute_url_dom.org/absolute/bg-home.jpg);'></div>
    <div id='abs-image2' style="background-image:url(http://absolute_url_dom.org/absolute/bg-home.jpg);"></div>
    <div id='abs-image3' style='background-image:url(/absolute/bg-home.jpg);'></div>
    <div id='abs-image4' style="background-image:url(/absolute/bg-home.jpg);"></div>

        <!-- WITH single/double quotes:  url("") or url('') and ABSOLUTE urls-->        
    <div id='abs-image7' style="background-image:url('http://absolute_url_dom.org/absolute/bg-home.jpg');"></div>
    <div id='abs-image8' style='background-image:url("http://absolute_url_dom.org/absolute/bg-home.jpg");'></div>
    <div id='abs-image9' style="background-image:url('/absolute/bg-home.jpg');"></div>
    <div id='abs-image10' style='background-image:url("/absolute/bg-home.jpg");'></div>

        <!-- Case with single/double quotes written as special characters for ABSOLUTE URLs -->
    <div id='bg-image11' style='background-image:url(&#034;http://absolute_url_dom.org/absolute/bg-home.jpg&#034;);'></div>
    <div id='bg-image12' style="background-image:url(&#039;http://absolute_url_dom.org/absolute/bg-home.jpg&#039;);"></div>
    <div id='bg-image13' style='background-image:url(&#34;http://absolute_url_dom.org/absolute/bg-home.jpg&#34;);'></div>
    <div id='bg-image14' style="background-image:url(&#39;http://absolute_url_dom.org/absolute/bg-home.jpg&#39;);"></div>
    <div id='bg-image15' style='background-image:url(&#034;/absolute/bg-home.jpg&#034;);'></div>
    <div id='bg-image16' style="background-image:url(&#039;/absolute/bg-home.jpg&#039;);"></div>
    <div id='bg-image17' style='background-image:url(&#34;/absolute/bg-home.jpg&#34;);'></div>
    <div id='bg-image18' style="background-image:url(&#39;/absolute/bg-home.jpg&#39;);"></div>

after patch the will be no broken HTML

PS: there is almost zero performance impact to the regexp , we simply capture the quotes and use them, the regexp has no other changes

HLeithner · 2016-07-26T11:04:49Z

Works for me now. Thx

ggppdk · 2016-07-26T12:31:40Z

@HLeithner

please visit
https://issues.joomla.org/tracker/joomla-cms/11266

and click "Test this" and the select "Tested successfuly" to mark a successful test

HLeithner · 2016-07-26T14:22:44Z

I have tested this item ✅ successfully on ff76cd8

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11266.}

hardiktailored · 2016-08-01T11:18:07Z

I have tested this item ✅ successfully on ff76cd8

Works perfectly.

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11266.}

hardiktailored · 2016-08-01T11:20:06Z

See results:

Before Patch:

After Patch:
(1)

(2)

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11266.}

brianteeman · 2016-08-01T11:33:45Z

Rtc

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11266.}

ggppdk · 2016-08-03T18:56:51Z

No longer RTC, it needs retesting, since i added handling of:

style='background-image:url(&#34;...&#34;);'
style="background-image:url(&#39;...&#39;);"

to fix issue: #7267

csthomas · 2016-12-10T21:56:08Z

plugins/system/sef/sef.php

 		{
-			$regex  = '#style=\s*[\'\"](.*):\s*url\s*\([\'\"]?(?!/|' . $protocols . '|\#)([^\)\'\"]+)[\'\"]?\)#m';
-			$buffer = preg_replace($regex, 'style="$1: url(\'' . $base . '$2$3\')', $buffer);
+			$regex_url  = '\s*url\s*\(([\'\"]|\&\#0?3[4|9];)?(?!/|\&\#0?3[4|9];|' . $protocols . '|\#)([^\)\'\"]+)([\'\"]|\&\#0?3[4|9];)?\)';


Do you need pipe in [4|9]?

yes, in total we want to detect these:
34, 39, 034, 039

but now i see, yes pipe is a mistake, although it should not cause a problem since character #\03| does not exist

…ackground url paths absolute

ggppdk · 2016-12-11T04:57:15Z

I have removed the redudant pipe character, if that case was ever encountered then HTML is already broken

csthomas · 2017-01-14T21:11:18Z

plugins/system/sef/sef.php

-			$buffer = preg_replace($regex, 'style="$1: url(\'' . $base . '$2$3\')', $buffer);
+			$regex_url  = '\s*url\s*\(([\'\"]|\&\#0?3[49];)?(?!/|\&\#0?3[49];|' . $protocols . '|\#)([^\)\'\"]+)([\'\"]|\&\#0?3[49];)?\)';
+			$regex  = '#style=\s*([\'\"])(.*):' . $regex_url . '#m';
+			$buffer = preg_replace($regex, 'style=$1$2: url($3' . $base . '$4$5$6)', $buffer);


Please remove '$6' from '$4$5$6)' and I can mark as a successful test.

I do not remember now, but by looking at $6 it is the closing quote ? which is either ' or "

Whole regex:

#style=\s*([\'\"])(.*):\s*url\s*$([\'\"]|\&\#0?3[49];)?(?!/|\&\#0?3[49];|' . $protocols . '|\#)([^$\'\"]+)([\'\"]|\&\#0?3[49];)?\)# #style=\s*(______)(__):\s*url\s*$(___________________)?(?!/|\&\#0?3[49];|' . $protocols . '|\#)(__________)(___________________)?$#

I counted to five:)

There are six, did you test it without $6 ?

Yes, I tested on your examples. If I remove $5 too then ' or " are missing. Without $6 everything is OK.
(?!....) - it is not captured by regex.

Yes, i have taken time to look at it again and counted the blocks,
you are right, the $6 capturing block does not exist,
there are only 5 capturing blocks in the regular expressions, i will update the PR,
I missed it , as it does not cause error because $6 capturing block is always empty

ggppdk · 2017-01-20T05:39:14Z

I have removed the redudant $6 replacement of the 6th capturing block, as it does not the 6th capturing block, does not exist.
It should continue to work as before

csthomas · 2017-01-20T09:04:50Z

I have tested this item ✅ successfully on b616e60

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/11266.}

dgrammatiko · 2017-01-20T10:10:29Z

I have tested this item ✅ successfully on b616e60

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/11266.}

dgrammatiko · 2017-01-20T10:11:01Z

RTC

Thanks @ggppdk

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/11266.}

ggppdk changed the title ~~Fix SEF plugin some times breaking HTML while trying to make inline background url paths absolute~~ Fix SEF plugin breaking HTML while trying to make inline background url paths absolute Jul 30, 2016

brianteeman added RTC This Pull Request is Ready To Commit PR-staging labels Aug 1, 2016

andrepereiradasilva mentioned this pull request Aug 3, 2016

system/sef plugin background image parser regexp bug #11412

Closed

brianteeman removed the RTC This Pull Request is Ready To Commit label Aug 3, 2016

brianteeman mentioned this pull request Aug 3, 2016

SEF Plugin breaks style tag #11264

Closed

brianteeman added the RTC This Pull Request is Ready To Commit label Sep 2, 2016

brianteeman added this to the Joomla 3.6.3 milestone Sep 2, 2016

wilsonge removed the RTC This Pull Request is Ready To Commit label Sep 2, 2016

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 2, 2016

wilsonge modified the milestone: Joomla 3.6.3 Sep 2, 2016

wilsonge removed the RTC This Pull Request is Ready To Commit label Sep 2, 2016

csthomas reviewed Dec 10, 2016

View reviewed changes

ggppdk added 3 commits December 11, 2016 06:52

Fix SEF plugin some times breaking HTML while trying to make inline b…

ec0a433

…ackground url paths absolute

Added handling of &joomla#34; and &joomla#39;

5cb88b1

Code style, fix too long line

e6855d7

ggppdk force-pushed the patch-14 branch from 9b7aa97 to e6855d7 Compare December 11, 2016 04:52

Removed redudant pipe character

5d5057e

csthomas reviewed Jan 14, 2017

View reviewed changes

Update sef.php

b616e60

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 20, 2017

zero-24 added this to the Joomla 3.7.0 milestone Jan 20, 2017

rdeutz merged commit 7d34d41 into joomla:staging Jan 20, 2017

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 20, 2017

Uh oh!

Conversation

ggppdk commented Jul 23, 2016 • edited by joomla-cms-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Testing Instructions

Uh oh!

HLeithner commented Jul 26, 2016

Uh oh!

ggppdk commented Jul 26, 2016

Uh oh!

HLeithner commented Jul 26, 2016

Uh oh!

hardiktailored commented Aug 1, 2016

Uh oh!

hardiktailored commented Aug 1, 2016

Uh oh!

brianteeman commented Aug 1, 2016

Uh oh!

ggppdk commented Aug 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggppdk Dec 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggppdk commented Dec 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggppdk commented Jan 20, 2017

Uh oh!

csthomas commented Jan 20, 2017

Uh oh!

dgrammatiko commented Jan 20, 2017

Uh oh!

dgrammatiko commented Jan 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

ggppdk commented Jul 23, 2016 •

edited by joomla-cms-bot

Loading

ggppdk Dec 11, 2016 •

edited

Loading