search

ciscoheat / erazor

A Haxe implementation of the Razor view engine
19 stars 15 forks source link

Add option to automatically escape content #15

Closed waneck closed 11 years ago

waneck commented 11 years ago

It would be great to add a way (maybe even default it) to add a escape function (e.g. htmlEscape) for the variable output, as not adding it opens up some possible security holes.

fponticelli commented 11 years ago

True, but we also need an explicit way to remove that filter when required.

On Thu, May 9, 2013 at 10:04 AM, Cauê Waneck notifications@github.comwrote:

It would be great to add a way (maybe even default it) to add a escape function (e.g. htmlEscape) for the variable output, as not adding it opens up some possible security holes.

— Reply to this email directly or view it on GitHubhttps://github.com/ciscoheat/erazor/issues/15 .

waneck commented 11 years ago

I think that adding a dynamic function called escape to the template class would be a good way to do it.

fponticelli commented 11 years ago

But that implies having to call that function manually, no?

On Mon, May 13, 2013 at 2:34 PM, Cauê Waneck notifications@github.comwrote:

I think that adding a dynamic function called escape to the template class would be a good way to do it.

— Reply to this email directly or view it on GitHubhttps://github.com/ciscoheat/erazor/issues/15#issuecomment-17838737 .

waneck commented 11 years ago

we can do it using the hscript interpreter / macro code

fponticelli commented 11 years ago

What if @(value)/@value always escapes the results, but @.(value)/@.value doesn't? It means introducing a new syntax but seems justified in this case.

On Mon, May 13, 2013 at 4:06 PM, Cauê Waneck notifications@github.comwrote:

we can do it using the hscript interpreter / macro code

— Reply to this email directly or view it on GitHubhttps://github.com/ciscoheat/erazor/issues/15#issuecomment-17844313 .

waneck commented 11 years ago

I'm fine with it! Do you think we still need to add a "escape" dynamic function this way, or just call htmlEscape directly?

2013/5/13 Franco Ponticelli notifications@github.com

What if @(value)/@value always escapes the results, but @.(value)/@.value doesn't? It means introducing a new syntax but seems justified in this case.

On Mon, May 13, 2013 at 4:06 PM, Cauê Waneck notifications@github.comwrote:

we can do it using the hscript interpreter / macro code

— Reply to this email directly or view it on GitHub< https://github.com/ciscoheat/erazor/issues/15#issuecomment-17844313> .

— Reply to this email directly or view it on GitHubhttps://github.com/ciscoheat/erazor/issues/15#issuecomment-17844849 .

fponticelli commented 11 years ago

I don't think we need an explicit escape function. It might be interesting to add an erazor helper library to inject commonly used helpers.

On Mon, May 13, 2013 at 4:19 PM, Cauê Waneck notifications@github.comwrote:

I'm fine with it! Do you think we still need to add a "escape" dynamic function this way, or just call htmlEscape directly?

2013/5/13 Franco Ponticelli notifications@github.com

What if @(value)/@value always escapes the results, but @.(value)/@.value doesn't? It means introducing a new syntax but seems justified in this case.

On Mon, May 13, 2013 at 4:06 PM, Cauê Waneck notifications@github.comwrote:

we can do it using the hscript interpreter / macro code

— Reply to this email directly or view it on GitHub< https://github.com/ciscoheat/erazor/issues/15#issuecomment-17844313> .

— Reply to this email directly or view it on GitHub< https://github.com/ciscoheat/erazor/issues/15#issuecomment-17844849> .

— Reply to this email directly or view it on GitHubhttps://github.com/ciscoheat/erazor/issues/15#issuecomment-17844968 .

astro75 commented 11 years ago

I implemented this new syntax on my fork https://github.com/astro75/erazor Default behavior is escaping only @.value but not @value Escape methods can be replaced easily.

I also found clemos fork that has ability to enable escaping https://github.com/clemos/erazor To disable escaping on specific strings they need to extend TStrig class. eg. new SafeString("")

I think clemos approach is better, because it moves the code for "do not escape this" away from template.

edit: I also made haxe 3 compatible version of clemos approach https://github.com/astro75/erazor/tree/auto_escape

waneck commented 11 years ago

@astro75 , thank you for your implementation. Some thoughts:

  • It would be best to invert, and make everything escape by default - and explicitly drop escaping with the new @.value syntax. This follows the rule that the most secure must be the default.
  • I like clemos' the approach of leaving the escaping handling to a new class - so the user doesn't have to replace a bunch of dynamic functions in every template he uses. However I still think there should be a @.value syntax to override escaping in some specific places. This could still be achieved by making a new StringBuf wrapper, which may have an "addUnsafe" function, as well as the normal "add" (which would escape by default)
  • Can you add some unit tests for the new syntax and the escaping behavior?

Thanks!

astro75 commented 11 years ago

Erazor is not only for html. Maybe there could be HtmlTemplate that extends Template and makes escaping the default behavior. I don't like the @.value syntax any more. I think that @Raw(value) or @Html.Raw(value) is better. (identical to original Razor templates). This makes it more obvious than a single dot.

fponticelli commented 11 years ago

I agree that Erazor is not for Html only and I see many potential advantages in having an HtmlTemplate class with specific behaviors. The first that comes to my mind is being able to stuff like @if(cond)

It is true!
(not the absence of curly braces). About using @Raw I like the idea (except for the capital R). The only problem I see is that so far we don't have a standardized way to inject helpers and all the methods must come from the model. Even worse this method reverses the standard behavior.

On Tue, Oct 1, 2013 at 4:46 AM, astro75 notifications@github.com wrote:

Erazor is not only for html. Maybe there could be HtmlTemplate that extends Template and makes escaping the default behavior. I don't like the @.value syntax any more. I think that @Raw(value) or @Html.Raw(value) is better. (identical to original Razor templates). This makes it more obvious than a single dot.

— Reply to this email directly or view it on GitHubhttps://github.com/ciscoheat/erazor/issues/15#issuecomment-25439840 .

waneck commented 11 years ago

So where do we stand? I'm fine with either @. or @raw(). But I'd like to have an outlook of what it will take to implement this. While I understand that erazor is not only used for HTML templates, I still think that we should make it "secure by default", even if this means a version bump

fponticelli commented 11 years ago

I don't stand anywhere :) ... I think that if we go the @raw way we need proper injection of helpers first. If we go @. maybe we can define 2 standard functions for escaped/unescaped that return the same raw value in Template but they are overridden in HtmlTemplate.

On Tue, Oct 1, 2013 at 10:04 AM, Cauê Waneck notifications@github.comwrote:

So where do we stand? I'm fine with either @. or @raw(). But I'd like to have an outlook of what it will take to implement this. While I understand that erazor is not only used for HTML templates, I still think that we should make it "secure by default", even if this means a version bump

— Reply to this email directly or view it on GitHubhttps://github.com/ciscoheat/erazor/issues/15#issuecomment-25463534 .

waneck commented 11 years ago

Sorry if I sounded rude :) As I said, I'm fine with both possiblities. But I don't have enough experience with the original razor templates to think of a good way to deal with helpers - so I'm more inclined to go with the first. About the HtmlTemplate, what do you think if add a constructor argument that will control this? This way we still let the user choose and not generate bugs or security errors

fponticelli commented 11 years ago

No worries. I have no time right now to look into this more into depth so I would say that the decision should fall on the first that is going to provide a satisfactory solution and implementation.

On Tue, Oct 1, 2013 at 10:37 AM, Cauê Waneck notifications@github.comwrote:

Sorry if I sounded rude :) As I said, I'm fine with both possiblities. But I don't have enough experience with the original razor templates to think of a good way to deal with helpers - so I'm more inclined to go with the first. About the HtmlTemplate, what do you think if add a constructor argument that will control this? This way we still let the user choose and not generate bugs or security errors

— Reply to this email directly or view it on GitHubhttps://github.com/ciscoheat/erazor/issues/15#issuecomment-25466658 .

waneck commented 11 years ago

Agreed!

astro75 commented 11 years ago

Just implemented HtmlTemplate and macro.HtmlTemplate using @raw syntax. I also made a simple way for injecting helpers into Template. macro.Template currently allows injection only by extending it.

https://github.com/astro75/erazor/tree/auto_escape

fponticelli commented 11 years ago

Like it. can you submit a PR?

fponticelli commented 11 years ago

PR applied, many thanks.