Certain code tag content breaks AMP output

[kevinansfield]

Issue Summary

Adding '$example$' to the markdown truncates the AMP output before the second $ causing broken browser rendering through missing closing tags.

To Reproduce

1. Create a new post and set the content to:

Testing

`'$example$'` asdf

123

2. Publish the post and open the AMP version on the blog front-end. Note the broken display and missing content after '$example.

The HTML output is clearly malformed. the trailing $' is removed and the rest of the helper output is added to the bottom of the post with the final section duplicated:

This results in the browser injecting lots of extra <code></code> tags to try and deal with the malformed HTML.

Removing the SafeString call so that handlebars outputs the raw string appears to show that there's a nested async helper issue:

<div class="kg-card-markdown">
  <p>Testing</p>
  <p>
    <code>'$example__aSyNcId_<_ZziCMbqm__#x27;</code>
    asdf
  </p>
  <p>123</p>
</div>

Narrowing the troublesome content down it appears to be caused by a trailing $ or $' in any element. This can be demonstrated with simply putting $ as the only markdown content in a post.

Technical details:

• Ghost Version: 1.24.7
• Node Version: 8.10.0
• Browser/OS: n/a
• Database: SQLite

[lunaticmonk]

Seems the problem is at

Ghost/core/server/apps/amp/lib/helpers/amp_content.js

Line 193 in c186347

cleanHTML = sanitizeHtml(ampHTML, {

. ampHTML if logged, prints the code fine to the console. But when we pass it to the sanitizeHtml function, it breaks. Don't know if we missed something in options passed to the sanitizeHtml. Please correct me if I am wrong. Taking a look at this.

[aileen]

@lunaticmonk I think you're on the right track. That's where I saw this glitch happening too. I couldn't find a solution within the sanitize-html module tho. Would be super nice, if you'd find a solution here 🤗

[jtwebman]

My guess it is a markdown issue as even the non-amp page adds the end code block in the wrong place. I am taking a look at this as well.

[jtwebman]

I was wrong it looks like the mobiledoc issue as this is what gets posted to the server: "mobiledoc": { "version": "0.3.1", "atoms": [], "cards": [ [ "code", { "code": "'$example$' asdfg\n\n123" } ] ], "markups": [], "sections": [ [ 10, 0 ], [ 1, "p", [] ] ] } Notice code seems to be nested. Will keep digging deeper.

[jtwebman]

Ok never mind on all of that was using the wrong editor type. Fixed and now looking at the sanitize html code.

[jtwebman]

Ok on the hunt farther down as this might be a handlebars issue as the output from everything in amp_content.js is <p><code>'$example$'</code> asdf</p><p>123</p> which looks correct.

[jtwebman]

Ok this is a bug in express-hbs it uses Javascript string replace which has special string syntax. (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace#Specifying_a_string_as_a_parameter)
Working on the fix now by moving it to a function replace. This line https://github.com/barc/express-hbs/blob/master/lib/hbs.js#L500 needs to be this res = res.replace(id, function() { return values[id]; });

[jtwebman]

Opened an issue in express-hds to solve this issue: TryGhost/express-hbs#144

[jtwebman]

Opened a PR. TryGhost/express-hbs#149

Once it is merged and published in a new version we can open a PR to update the version in this repo.

[m1guelpf]

The PR was merged! 🎉

[jtwebman]

Still waiting for the permissions to publish the merge and then we can make sure this fixes this issue by testing out the new version here.

[ErisDS]

This bug is known and understood, it is expected to be fixed by the update to express-hbs, which has an open PR #10288. Currently waiting on a bug check/fix before merging.

[kevinansfield]

Re-opening because we had to revert the express-hbs bump. See #10643 for further details.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[naz]

@allouis as #10750 landed in master this issue can be now closed?