Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Template with {contentType ...} rendered to string should not modify global application state #106

Conversation

@jkuchar
Copy link
Contributor

jkuchar commented Mar 19, 2016

Anything rendered to string should not modify global application state. Unfortunately it is not a case with macro {contentType mime-type} which sets headers directly. ref to latte code

Expected behaviour would be to:

  1. render() method modifies application output directly, it is expected to modify output headers
  2. renderToString() should NOT modify headers - there should be some other API to get headers set by template
@jkuchar jkuchar force-pushed the jkuchar:fix-contentType-should-not-modify-headers-directly branch 2 times, most recently from 0f5ab19 to 347438c Mar 19, 2016
@fprochazka

This comment has been minimized.

Copy link
Contributor

fprochazka commented Mar 19, 2016

Sounds logical. Imho sending of the header should be removed altogether, as "sending a headers from templating engine" sounds very wrong when you say it out loud like this. Sadly, that would be a giant BC Break.

@jkuchar

This comment has been minimized.

Copy link
Contributor Author

jkuchar commented Mar 19, 2016

I think it is great that latte file knows it's content type. That's why latte is what it is - content escaping.

render() should behave the same as it behaves now. Method is dependent on script output anyway so I think it should send headers. Headers needs to be sent before output and it will be really tricky to implement without sending it directly from template because you know content type only at runtime. (e.g. {if $a}{contentType text}{/if} or {contentType $a})

(btw. this kind of type dynamic makes static analysis impossible) @JanTvrdik

renderToString(): It should just return string with output as it does now.

There can introduced something like renderToBuffer() which returns DTO with "response body" and headers.

@jkuchar jkuchar force-pushed the jkuchar:fix-contentType-should-not-modify-headers-directly branch from 347438c to ac05e6b Mar 19, 2016
@jkuchar

This comment has been minimized.

Copy link
Contributor Author

jkuchar commented Mar 19, 2016

Alternate solution can be to declare content type in static way. e.g. using template headers (something very similar to file-level docblock)

Then:

  • render() then will not need to send headers anymore, because it will be easy to determine content-type before template execution
  • renderToString() will remains the same
  • there will be need for API to get root content type of given latte file
@fprochazka

This comment has been minimized.

Copy link
Contributor

fprochazka commented Mar 20, 2016

I think it is great that latte file knows it's content type. That's why latte is what it is - content escaping.

I agree, I just think the macro shouldn't be sending HTTP Headers.


In Lavito, we've implemented "the symfony way" of naming templates (while integrating latte into symfony)... that means .txt.latte is content type text, .html.latte is content type html. Imho it's cleaner, since you set the content type from outside, and you can read it easily from the prepared (and not yet rendered) object and send the header yourself if you must. We're using it for email templates, that are not yet in HTML, but plain. And we wouldn't want that to be sending any headers to the output - makes no sense.

it will be really tricky to implement without sending it directly from template because you know content type only at runtime

I disagree. We could make it that you have to set the type from control/presenter (some template object setter - compiler has that ability already) and deprecate the macro - which would solve the chicken egg problem of fetching the content type, sending the header and rendering the template. The real BC Break would be only those few templates where you have xml content-type macro for your exports/rss - and it can be easily fixed, since you can grep your project for the macro and find the template usage.

I agree the render() should not be changed for the compatibility's sake. But still I think in 3.* releases it might be a good idea to break that and it's good that you brought up this issue.

@jkuchar

This comment has been minimized.

Copy link
Contributor Author

jkuchar commented Mar 20, 2016

We could also make it that you have to set the type from control/presenter (some template object setter - compiler has that ability already)

I do not agree with this. I has two downsites:

  • no way for static analysis to know what content type is inside (you are defining content-type again at runtime)
  • you can interpret one latte template from to places with two different content types which can lead into XSS attack vulnerability)

Template for itself must know what is inside. I think there are two meaningfull ways of doing that:

  • filename as proposed by Filip
  • by introducing latte header (e.g. must be first macro in file, etc.)
    • plus: latte itself knows what is inside; there will be not API change to Compiler:compile() and Engine
    • plus: contentType can act as "header macro" (first macro in latte; no variables) will be easy to introduce without BC
    • when using PresenterResponse there will be no BC, because it will send header retrieved from template
    • minus: requires to add new syntax into latte
@dg dg force-pushed the nette:master branch 21 times, most recently from 9201aa6 to 1bb5b1a Apr 21, 2016
@dg dg force-pushed the nette:master branch 5 times, most recently from 0866aa4 to 959c81c Apr 30, 2016
@dg dg force-pushed the nette:master branch 5 times, most recently from 5792fb0 to a3c72b8 May 9, 2016
@dg

This comment has been minimized.

Copy link
Member

dg commented May 11, 2016

Just note: {contentType} sends HTTP header only when you pass full mimetype ie {contentType application/javascript}. For content sensitive escaping is enough to specify {contentType javascript} and no header is sent.

@dg dg force-pushed the nette:master branch 14 times, most recently from 34fd4ef to 25ccd58 May 12, 2016
@dg dg force-pushed the nette:master branch 3 times, most recently from 81e91cf to d3f86a5 May 28, 2016
@dg dg closed this in feca9d1 May 30, 2016
@jkuchar jkuchar deleted the jkuchar:fix-contentType-should-not-modify-headers-directly branch Jul 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.