AssetInjector::prepareOutput() breaks HTML with “ in the markup

  • Resolved Bence Szalai

    (@grapestain)


    8 months, 2 weeks ago

    The AssetInjector::prepareOutput() injects unrelated HTML tags anywhere the string </head> appears in the page source.

    Now let’s consider a page with this script inside:

    <script>
var foo = {"htmlExample":"<!DOCTYPE html><html><head><link rel=\"stylesheet\" type=\"text\/css\" href=\"https:\/\/example.com\/style.css\" media=\"all\"><\/head><body><\/body><\/html>"};
</script>

    This is perfectly valid HTML containing a script that contains an object literal variable declaration that happens to store a piece of HTML under an object property.

    Smart Slider 3 injects < link rel="stylesheet" type="text/css" href="..." media="all"> etc. before the </head> inside the object literal breaking website in two ways:

    • It breaks the whole <script by injecting unescaped " character at arbitrary position into a JS object literal, triggering syntax error.
    • Or if the given string may happen to be written using single quotes, the script will not break, but now Smart Slider 3 is basically injecting unrelated links, scripts and styles to a HTML handled by an unrelated JS application. Don’t even think about how bad it can become if that application may later use the HTML as an actual page of a site. In that case Smart Slider 3 has just prepared an XSS on that site. Which is mundane considering the payload, but if that is an independent site, from their perspective I can now change a javascript file on my site to trigger an attack. Basically the plugin made me plant a backdoor to a HTML handled by my web-app.

    WordPress offers stable way of adding scripts and styles to the real head of pages: wp_enqueue_script, wp_enqueue_style. If those methods were used, this issue did not happen.

    Also it is a huge anti-pattern to process HTML using regex or similar low level tools, and there’s a reason to that! Even such simple thing that looking for the closing </head tag can go catastrophic breaking sites and applications.

    It is really not nice to waste time on troubleshooting issues that should not be there in the first place. Why reinvent the wheel by writing all these complex classes to do something simple as adding scripts and styles? WordPress has an API for a reason, and that actually works.

    • This topic was modified 8 months, 2 weeks ago by Bence Szalai.
Viewing 4 replies - 1 through 4 (of 4 total)
  • Plugin Support Gabor

    (@nextendweb_gabor)

    Hi @grapestain!

    I’m sorry for the troubles this created for you. We used to have wp_enqueue_script and wp_enqueue_style in our codes, but we changed that, because they made a lot of problems. People are trying to create custom themes and modify the code of their official themes all them time, and they are not using wp_head and wp_footer functions, which are used by the previous functions. Because of this, we received support tickets every single day about having our files missing on the websites of these people. This is why we modified our codes, because by using native PHP and not WordPress codes to insert CSS and JS into websites worked out a lot better. We know it is not perfect, so your script example is valid, but even like this, the error numbers have decreased incredibly. For the issue you have, I can suggest inserting your script into the <body>...</body> part of the website. We are using the first </head> to insert our files and codes before it, so in this case your code would be left alone.

    in what universe is this “Resolved”????? Your latest plugin version 3.5.1.23 is STILL injecting broken HTML strings because of this bug and that causes whitescreen of death on affected sites, they are unable to add post content or edit posts because javascript blows up from your slider injecting un-escaped quotation marks. What the heck are you doing!?!?!?!?!? If you’re going to reinvent wheels at least make them round!!!!!!!!!!! Fix the bug, don’t just mark it resolved because you get less support requests since your change. Shameful!

    Thread Starter Bence Szalai

    (@grapestain)

    Yeah, I just gave up and removed it. Still very very bad approach and precedent. They made a plugin that breaks proper sites, just to make it work better with some broken sites. If a theme does not implement wp_head and wp_footer that is a broken theme. This plugin should be put on a blacklist or at least be marked as potentially dangerous for not adhering to WordPress design principles. But I have other fights to fight…

    Plugin Support Gabor

    (@nextendweb_gabor)

    I’m sorry, but we won’t be able to change our system due to the reasons written down. But we improved our code, so it would identify the proper </head> tag better. This way the ticket’s example:

    <script>
var foo = {"htmlExample":"<!DOCTYPE html><html><head><link rel=\"stylesheet\" type=\"text\/css\" href=\"https:\/\/example.com\/style.css\" media=\"all\"><\/head><body><\/body><\/html>"};
</script>

    within the head tag won’t create issues anymore from the next Smart Slider version.

    And @damien_vancouver, while your problem has a connection to the file incalling, your problem will be different. Please contact us here and we will help resolving that issue.

Viewing 4 replies - 1 through 4 (of 4 total)
  • The topic ‘AssetInjector::prepareOutput() breaks HTML with “ in the markup’ is closed to new replies.