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

Allow the developer to choose the escaping strategy #387

Open
tschuehly opened this issue Oct 1, 2024 · 10 comments
Open

Allow the developer to choose the escaping strategy #387

tschuehly opened this issue Oct 1, 2024 · 10 comments

Comments

@tschuehly
Copy link
Contributor

Currently when outputting values in an attribute JTE only checks if the attribute starts with on and then escapes it as javascript attribute:

if (attributeName.startsWith("on")) {
  Escape.javaScriptAttribute(value, templateOutput);
} else {
  Escape.htmlAttribute(value, templateOutput);
}

if (attributeName.startsWith("on")) {

If I'm using alpine.js x-data or htmx hx-on this doesn't get triggered and it leads to an error as the attribute is rendered as a multiline string:

This JTE template:

x-data="{ showThread: false , editComment: false, commentText: '${comment.escapedText()}'}"

renders to this:

x-data="{ showThread: false , editComment: false, commentText: 'asd#
asd
asd'}"

leading to an JavaScript exception:
image

If the correct Escaping strategy is chosen it renders correctly

x-data="{ showThread: false , editComment: false, commentText: 'asd#\nasd\nasd'}"

I think we should introduce an additional syntax similar to $unsafe{comment.escapedText()} but instead using $js{comment.escapedText()} that then either chooses gg.jte.html.escape.Escape#javaScriptBlock or gg.jte.html.escape.Escape#javaScriptAttribute to escape the value.

@kelunik
Copy link
Collaborator

kelunik commented Oct 1, 2024

Interpolation of user data directly into JS expressions is something that easily results in XSS, so we don't really plan to support context dependent escaping for JS.

The parse error you get can be avoided by using a JSON serializer.

@tschuehly
Copy link
Contributor Author

But I want it to be escaped? I just want it to be escaped correctly.
If the same code was in an onClick expression it would be escaped properly.

@casid
Copy link
Owner

casid commented Oct 2, 2024

Hm, the whole idea of output escaping in jte is, that the user does not need to do it manually. So adding a new keyword to escape JS really would not fit into very well.

The gg.jte.html.OwaspHtmlTemplateOutput is just one implementation of HtmlTemplateOutput. In theory, it would be possible to create an HtmlTemplateOutput that is aware of Alpine or HTMX and put something like this in there:

if (attributeName.equals("x-data")) {
  Escape.javaScriptAttribute(value, templateOutput);
}

@tschuehly
Copy link
Contributor Author

Hm, the whole idea of output escaping in jte is, that the user does not need to do it manually. So adding a new keyword to escape JS really would not fit into very well.

Well but it is certainly not exhaustive. Currently it just leads to incorrect behaviour, and the developer doesn't have the choice. I would see the $js{} as an optional way. Default wouldn't change. Like you can currently do with $unsafe.

For reference in Thymeleaf there they have th:inline="script" so that all values get rendered correctly to JS syntax.
https://www.thymeleaf.org/doc/tutorials/2.1/usingthymeleaf.html#script-inlining-javascript-and-dart

The gg.jte.html.OwaspHtmlTemplateOutput is just one implementation of HtmlTemplateOutput. In theory, it would be possible to create an HtmlTemplateOutput that is aware of Alpine or HTMX and put something like this in there:

if (attributeName.equals("x-data")) {
  Escape.javaScriptAttribute(value, templateOutput);
}

As I wrote in the other issue, there is currently no way to plug in to this behaviour right?

I know you guys are very into the "safe" template but it's very restricting.

@tschuehly
Copy link
Contributor Author

Similar thing like here: #326

Instead of the user having to configure a custom policy or using unsafe, there could be $attr{} that knows how to properly escape a whole "attribute='value'" combination.
Also reference here: https://www.thymeleaf.org/doc/tutorials/2.1/usingthymeleaf.html#setting-the-value-of-any-attribute

I think an extensible system like this could lead to more advanced usages with JTE

Default should lead to safe templates but the developers should be able to either extend JTE or choose the correct strategy for the use case.

@kelunik
Copy link
Collaborator

kelunik commented Oct 9, 2024

I know you guys are very into the "safe" template but it's very restricting.

Yes, it is very restricting, by design. It should be hard to do the wrong thing and easy to do the right thing.

Instead of the user having to configure a custom policy or using unsafe, there could be $attr{} that knows how to properly escape a whole "attribute='value'" combination.

That opens a whole can of worms. Which attributes are safe?

@tschuehly
Copy link
Contributor Author

Well for example $attr{} would have two parameters.

${"hx-on:" + eventName, "executeFunction()"}

The attributes which are in the html standard are safe. Currently everything that doesn't start with on* is escaped like a normal attribute.

@kelunik
Copy link
Collaborator

kelunik commented Oct 9, 2024

The attributes which are in the html standard are safe. Currently everything that doesn't start with on* is escaped like a normal attribute.

No, for example href shouldn't allow the javascript: scheme, because this isn't safe.

@tschuehly
Copy link
Contributor Author

tschuehly commented Oct 9, 2024

But the problem with the solution is not that it is not safe but it is not smart enough to know about other custom elements and thereby doesn't choose the correct escaping technique.
tschuehly@23afcaa

In this case it doesn't escape \n even tho it should.

image

The question is what is the correct way to enable developers to user JTE with custom attributes like this.

@casid
Copy link
Owner

casid commented Oct 9, 2024

@tschuehly you can do

new MyAlpineHtmxHtmlTemplateOutput(stringOutput)

and pass this template output to render()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants