tighten / jigsaw

Simple static sites with Laravel’s Blade.
https://jigsaw.tighten.com
MIT License
2.13k stars 182 forks source link

Blade-Markdown and HTML escaping issues with Collections #292

Closed cossssmin closed 5 years ago

cossssmin commented 5 years ago

Started work on refactoring Maizzle to use Collections, and I stumbled across an issue with .blade.md files that contain HTML and Collections in Jigsaw.

What happens is that the first tag (and its closing) of the HTML in my .blade.md file is Markdown-parsed and escaped, so something like this:

---
extends: _layouts.master
section: content
---
<table class="wrapper" cellpadding="0" cellspacing="0" role="presentation">
  <!-- ... -->
</table>

... is transformed to this:

<p>&lt;table class=&quot;wrapper&quot; cellpadding=&quot;0&quot; cellspacing=&quot;0&quot; role=&quot;presentation&quot;&gt;
  </p>
  <!-- ... -->
<p></p>

This only happens when:

I've published a branch for Maizzle that demonstrates the issue. Simply install PHP and Node dependencies, and run npm run stage - you will get a build_staging directory, inside of which the brand directory shows the output in both cases (using @section and not using it).

Of course, if I use the @section directive, all is good. But this means I can no longer use Markdown inside it, it'll just be output as a string.

damiani commented 5 years ago

@hellocosmin Thanks for the issue, I'll take a look!

zaherg commented 5 years ago

Its is related to the escape function in the Parsedown class.

you may need to extend that function and overwrite the function to be something like

    protected static function escape($text, $allowQuotes = false)
    {
        return $text;
    }

This is not the best solution, but it worked for me when I was dealing with adding serverside highlighting to the code section of my posts

cossssmin commented 5 years ago

Thanks @linuxjuggler. However this isn't happening if you're not using a collection (that's my current setup), so I think this should be handled in Jigsaw instead of binding some custom Parsedown implementation.

zaherg commented 5 years ago

to be honest, I didn't try it without collections, my posts are all markdown files, and to be able to generate the server-side code highlighting I needed to extend Parsedown (or actually I extended ParsedownExtra).

but if this is the case, then you are right, it should be handled by Jigsaw.

But lets think of it like this, Jigsaw has the ability to work with remote collections, if they didn't escape the HTML from it, so anyone who has access to that remote collection can inject HTML code that will be executed at your end, so escaping HTML is important in collections, unless they have some flag which enable/disable the escape for local collections and enable it for remote one.

Its juts an idea, I have limited information about how the internal code works within Jigsaw

cossssmin commented 5 years ago

Well, even if it were so, what's happening is not a full HTML escaping - as you can see in the example, only the first tag is escaped, the rest is left untouched.

Being a SSG, I actually think it should be our responsibility as devs to escape content in such (remote) scenarios. Like with Blade in layouts, for example. I might be wrong, but it would feel backwards to always have to undo escaping.

zaherg commented 5 years ago

actually its a full HTML escaping, from your example, the whole table tag has been escaped, and the p tag was added due to the fact that newlines is translated to p in markdown.

again, maybe am wrong, and from what I can tell, damiani is going to look into this more deeply.

cossssmin commented 5 years ago

Sorry, my example was perhaps a bit misleading. That comment inside the escaped markup example stands for edited-out unescaped HTML - you can see it in full if you try out the branch.

Here's the entire HTML that is being output - as you can see, everything else is not escaped:

<p>&lt;table class=&quot;wrapper w-full bg-grey-light all-font-sans&quot; cellpadding=&quot;0&quot; cellspacing=&quot;0&quot; lang=&quot;en&quot; role=&quot;presentation&quot;&gt;
</p>
<tr>
  <td class="sm-w-full" align="center" style="padding-top: 48px; padding-bottom: 48px;">
    <table class="sm-w-full" cellpadding="0" cellspacing="0" role="presentation" width="600">
      <tr>
        <td align="left" style="padding-left: 24px; padding-right: 24px;">
          <table cellpadding="0" cellspacing="0" role="presentation" style="border-radius: 2px; box-shadow: 0 2px 4px 0 rgba(0, 0, 0, .1);" width="100%" bgcolor="#ffffff">
            <tr>
              <td style="border-top-left-radius: 2px; border-top-right-radius: 2px; padding-top: 2px; padding-bottom: 2px;" bgcolor="#3490dc"></td>
            </tr>
            <tr>
              <td align="center" class="sm-px-12" style="padding-left: 24px; padding-right: 24px; padding-top: 64px; padding-bottom: 64px;">
                <table cellpadding="0" cellspacing="0" role="presentation" width="100%">
                  <tr>
                    <td align="center" style="padding-left: 8px; padding-right: 8px;">
                      <h2 style="margin-top: 0; font-size: 20px;">👋 there, John!</h2>
                      <p style="margin: 0; color: #606f7b; font-size: 18px;">Please click the following link to confirm that john@example.com is your email address:</p>
                    </td>
                  </tr>
                </table>
                <div class="sm-py-12 sm-leading-0" style="line-height: 32px;">‌</div>
                <table cellpadding="0" cellspacing="0" role="presentation">
                  <tr>
                    <th class="hover-bg-green-dark" style="mso-padding-alt: 14px 48px; border-radius: 9999px;" bgcolor="#38c172">
                      <a href="http://example.com" style="display: block; line-height: 100%; padding-top: 14px; padding-bottom: 14px; padding-left: 48px; padding-right: 48px; color: #ffffff; font-size: 16px; text-decoration: none;">Confirm Email
                      </a>
                    </th>
                  </tr>
                </table>
              </td>
            </tr>
          </table>
        </td>
      </tr>
    </table>
    <table class="sm-w-full" cellpadding="0" cellspacing="0" role="presentation" width="600">
      <tr>
        <td align="center" style="line-height: 24px; padding: 24px; color: #8795a1; font-size: 12px;">
          <p style="margin: 0; font-size: 16px;">
            If this email doesn't make any sense, please
            <a href="https://example.com" class="hover-no-underline" style="color: #3490dc;">let us know</a>!
          </p>
        </td>
      </tr>
    </table>
  </td>
</tr>
<p></p>
zaherg commented 5 years ago

am out of ideas to be honest.

alpenist commented 5 years ago

Better override the element function after extending Parsedownand just add the $Element['text'] to $markup ... IMHO when it reaches this point all incoming input should be sanitized already!


    protected function element(array $Element)
    {
          //...
          //$markup .= self::escape($Element['text'], true);
          $markup .= $Element['text'];
          //...
    }
m1guelpf commented 5 years ago

@hellocosmin Are you using {{ $page->getContent() }} or {!! $page->getContents() !!}?

damiani commented 5 years ago

@hellocosmin Are you still experiencing this issue? I cannot reproduce, neither in my own tests nor using your repo.

cossssmin commented 5 years ago

@damiani apparently not anymore - don't know why 🤷‍♂️

@m1guelpf just using @yield('content')

Let's close this for now, will re-open if I start seeing it again. Thanks!