Add markdown file support in partials

[lukasschlueter]

This is a fix for gobuffalo/docs#404, but not necessarily a good one:

What is bad?

In contrast to the js support in partial_helper.go#47, the markdown support activates if the content type includes markdown or if the file ends in .md (JS support requires both to be true). This is inconsistent and might lead to confusion.

Why did I still do it this way?

Concluding from my tests, buffalo doesn't set a markdown content type but uses text/html. I could not yet find the place to change this in buffalo. I'm not sure if buffalo can change the content type for markdown files as r.HTML seems to define it on initialization where the file type might not yet be clear: https://gobuffalo.io/en/docs/rendering#automatic-extensions

Solution

To be honest, I don't think this really is a big deal. If someone can give me some pointers on how to fix it in buffalo though, I'd be happy to give it a try and make this fix a bit better.

[sio4]

Well, It makes sense we need to include a markdown within HTML output in some cases. But the output itself is already rendered HTML and it means, the content-type should be text/html. So IMO, checking of ct is not necessary. If the ct is really text/markdown, I cannot sure this is useful or not anyway, we just send the part as is, without doing Markdown().

[sio4]

Additionally, doing JSEscapeString() is a solution for embedding an HTML within Javascript. So for markdown, it should be rendered before JS handling (with faking extension after render) or it needs another checking if ct contains javascript and in this case, it also needs to be escaped.

[markbates]

I agree with Yonghwan. I think it’s ok to say, if you want to render a partial as md you should use md extension. I know support auto extensions, and technically this constitutes a breaking change; but i would be surprised if there are a lot of users using a md partial with no extension given in the partial call. I wrote that feature, and didn’t realize you could call md partials without the extension and it would be smart enough md it. I’m buffalo at some point there has to be something that looks up the file from disk to feed the feeder, so at that point you should have the extension anyway. I think. Don’t quote me. I just woke up. :)

…

On Nov 21, 2018, 6:24 AM -0500, Yonghwan SO ***@***.***>, wrote: Additionally, doing JSEscapeString() is a solution for embedding an HTML within Javascript. So for markdown, it should be rendered before JS handling (with faking extension after render) or it needs another checking if ct contains javascript and in this case, it also needs to be escaped. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.

[lukasschlueter]

Markdown partials without extension are somewhat covered by buffalo: https://github.com/gobuffalo/buffalo/blob/7dc6819f6cce0150869218165c210c445c3f3d93/render/template.go#L46
But I don't think this actually gets called at some point as the content type doesn't match. I didn't investigate this too much though.

[lukasschlueter]

Added some changes.
@sio4 is this what you requested regarding the JSEscapeString()?

[sio4]

Added some changes.
@sio4 is this what you requested regarding the JSEscapeString()?

Yes, basically. And for getting into the block of JSEscapeString(), need to change the name to have suffix .html or add a similar test.

Anyway, if it has layout too? (but the layout is not markdown)

[lukasschlueter]

I will test the layouts, thanks.

And for getting into the block of JSEscapeString(), need to change the name to have suffix .html or add a similar test.

Can you explain this a bit more?
What exactly does JSEscapeString do in this case?
Can you show me an example where this would actually be needed?

[sio4]

I will test the layouts, thanks.

When there is a layout for the partial, it just do same action with the layout as calling partialHelper() again recursively. So there are no chances to do Markdown() for the partial. It must be rendered before calling return partialHelper() for layout.

And for getting into the block of JSEscapeString(), need to change the name to have suffix .html or add a similar test.

Can you explain this a bit more?
What exactly does JSEscapeString do in this case?
Can you show me an example where this would actually be needed?

I guess the test case below shows you what it does, even though the example is not perfact and the partialHelper() for .html returns javascript code-like thing. (since it was just copied from test case above to show the difference) This function is provided by html/template and it escapes quotation marks and others so it can be used with javascript safely.

plush/partial_helper_test.go

Lines 218 to 232 in 2171edb

	func Test_PartialHelper_Javascript_With_HTML(t *testing.T) {
	r := require.New(t)
	name := "index.html"
	data := map[string]interface{}{}
	help := HelperContext{Context: NewContext()}
	help.Set("contentType", "application/javascript")
	help.Set("partialFeeder", func(string) (string, error) {
	return `alert('\'Hello\'');`, nil
	})
	html, err := partialHelper(name, data, help)
	r.NoError(err)
	r.Equal(`alert(\'\\\'Hello\\\'\');`, string(html))
	}

For this PR, if the markdown partial is used with javascript output, as something like following example, and there are some quotation marks inside of rendered partial,

prettyAlert("<%= partial("warning_message.md" %>")

and

You **cannot** use "markdown" partial *without* this PR!

[sio4]

I added a PR to this branch so please check the PR. #78.
When I added this function the first time, I did not consider layout and other file types enough. (It's my fault :-)

[lukasschlueter]

Thanks for the PR 👍

The only remaining point is whether we need to escape partials in a situation like this:

<!-- sample.html -->
<h1>Some HTML with JS</h1>

<script>
    console.log("<%= partial("someText.txt") %>")
</script>

// _someText.txt
Text with characters "breaking" the JS 'code'

_someText.txt would not get JS escaped as the content-type is text/html.

I don't know a simple way to fix this and I'm not sure if we should escape it.
From my understanding, this would be a lot of work including teaching plush how to parse html.
I think it's fine this way.

[sio4]

I don't know a simple way to fix this and I'm not sure if we should escape it.
From my understanding, this would be a lot of work including teaching plush how to parse html.
I think it's fine this way.

Yes! that is a self-made code-injection attack! right? haha...
Since HTML can embed javascript and more, even if we introduce something like front-matter for markdown for all partials, I think there is no perfect solution. Remaining part of the problem can be solved with "restrictions of using partial" :-)

I don't think those cases are not common but if some users still want to similar things, then the user can choose some alternatives like... using a step-by-step approach:

<script>
    <% let message = partial("someText.txt") %>
    console.log("<%= jsEscape(inspect(message)) %>")
</script>

This code will not work fine since there are some problems like type mismatch, type conversion, and so on. but if we... finally should handle this problem, I think this approach is one of the possibles.

[lukasschlueter]

As this is out of the scope for this PR, I extracted this to #79 for further discussion, but as I already stated, I don't think this is something needed to be fixed.

@sio4 Thanks again for the help
@markbates This should now be ready for review and merge

[sio4]

As this is out of the scope for this PR, I extracted this to #79 for further discussion, but as I already stated, I don't think this is something needed to be fixed.

I also think so.