HtmlFlow icon indicating copy to clipboard operation
HtmlFlow copied to clipboard

Safe output?

Open esiqveland opened this issue 4 years ago • 7 comments

It seems that even when I use the Dynamic View:

  .dynamic(el -> el.text(model.title))

The output is not safe for viewing? My example is that a user has entered some XSS in the title, such as: <script>alert('1')</script>.

Am I using the library wrong?

esiqveland avatar Feb 06 '20 15:02 esiqveland

The purpose of dynamic is usually related to data binding. All HTML inside the lambda passed to dynamic (e.g. el.text(model.title)) is not considered static and thus, the resulting HTML may change from render to render. This is the only goal of dynamic. Nothing related with safety.

We only considered safety scenarios in HtmlFlow related with type safety and thread safety.

We did not deal with any scenario related with XSS safety. But maybe our documentation induced you in error. Tell me if is that the case.

Nevertheless, if you argue that HtmlFlow should include any kind of guard for XSS safety we may discuss it and further include it. In my opinion, maybe I am wrong, but I think that kind of control should be taken by the framework or developer using HtmlFLow, and not by HtmlFlow itself.

Thanks for your feedback

fmcarvalho avatar Feb 06 '20 16:02 fmcarvalho

@esiqveland following my previous comment, I would say that in your example you should rather use:

.dynamic(el -> el.text(HtmlUtils.htmlEscape(model.title)))

Or any other escape utility function. In this case I am using HtmlUtils from Spring.

fmcarvalho avatar Feb 07 '20 10:02 fmcarvalho

Hi, thank you for quick reply.

I would expect a text rendering engine to deal with unsafe user input natively, but that just be my assumptions playing into it.

The call to .dynamic(...) is the last (and only) place where we know we might be dealing with user input. After render it is too late. I would argue the default should be to escape input values so that if someone changes the post title to </div></body> it doesn't break your markup. If you don't escape input content, there is no guarantee anymore that the lib generated the markup expressed in the code.

esiqveland avatar Feb 07 '20 11:02 esiqveland

Kotlinx.html as a similar behavior behavior to that pointed here by @esiqveland. In Kotlinx.html text is always safe unless programmer explicit use unsafe call such as:

head {
    style {
        unsafe { 
            raw("some text...")
        }
    }
}

We should enhance HtmlFlow API with something like this.

fmcarvalho avatar Feb 18 '20 12:02 fmcarvalho

@lcduarte What do you think about making .text(String text) of HtmlApi always escape the content text? It seems to be the default behavior of most engines including Kotlinx.html. Yet, I am worried about the overhead of that escape processing. I noticed that in spring-template-benchmark the kotlinx view includes a unsafe { raw(it.summary) } rather than text(it.summary) to avoid that overhead here: https://github.com/jreijn/spring-comparing-template-engines/blob/master/src/main/java/com/jeroenreijn/examples/view/KotlinxHtmlIndexView.kt#L45

Thus, if we make escape the default behavior of .text() than we should also include an alternative .raw to avoid that overhead.

Additionally we can add a configuration property to set the default behavior of .text() to escape, or not. We could make the visitText() pass the text.getValue() through a escaping function that could be initialized on class loading with a method reference to escapeHtml() or to an effectless identity function.

https://github.com/xmlet/HtmlFlow/blob/48f5410c2349ffbb8c970fd0658d1247946834c8/src/main/java/htmlflow/HtmlVisitorCache.java#L165

fmcarvalho avatar Apr 09 '21 15:04 fmcarvalho

It makes sense. We change the default behavior to improve security and keep an option for users which rather have the performance improvement than the security added by content escaping.

lcduarte avatar Apr 13 '21 13:04 lcduarte

The main idea is to replace write(text.getValue()) by write(escapeHtml.apply(text.getValue()))

And an additional static field: static final Function<String, String> escapeHtml;

And then initialize it in the static constructor depending of a system property, such as -Dhtmlflow.ignoreEscaping=true

static {
	// Read System properties
        if(... escaped ...)
		escapeHtml = HtmlUtils::htmlEscape;
        else 
                escapeHtml = html -> html;
}

Also I tend to not add any dependency for an auxiliary library and copy paste the code instead.

fmcarvalho avatar May 24 '21 14:05 fmcarvalho

Hi; have you had any further thoughts on implementing this feature? The approach you outlined looks like it would solve the problem in a clean way, but it would also be handy to have a method that allowed raw content to be passed through unescaped.

mhw avatar May 19 '23 15:05 mhw

Yes, I gave low priority to this feature. While it seems not to hard to implement, I did not have time yet.

Moreover, since then we have made a deep refactor in all HtmlFlow internals that implied a new API 4.0 to support asynchronous data models . So, I didn't wanted to mix it before finishing the new API.

I will try to address this feature to the next month.

fmcarvalho avatar May 19 '23 17:05 fmcarvalho

Any updates on when this will be available in HtmlFlow? I don't feel comfortable using this library until it is implemented. It is too easy to forget to html escape text that should be.

scott-taylor avatar Jan 18 '24 13:01 scott-taylor

I will release it next week in 4.4

fmcarvalho avatar Jan 18 '24 13:01 fmcarvalho

DONE release 4.4

fmcarvalho avatar Jan 26 '24 15:01 fmcarvalho

@mhw It is DONE on release 4.4.

fmcarvalho avatar Jan 26 '24 15:01 fmcarvalho

@esiqveland @scott-taylor @mhw As you may be aware, I am the author and primary advocate for the HtmlFlow Java library. As part of my continuous endeavors, I am required to provide justification for this project to my university.

HtmlFlow serves as a versatile auxiliary tool utilized across various sectors, facilitating software development by streamlining tasks such as HTML reporting automation, web templating, and more.

If you have utilized HtmlFlow, I kindly request your testimony regarding my dedication to addressing issues, implementing new features, or otherwise aligning with your requirements and/or your company within the scope of a specific project, task, or work that utilized HtmlFlow during a certain timeframe.

Your testimony will play a crucial role in demonstrating my work to my university.

Please feel free to send your testimonial to my email address at ISEL: [email protected]

Thank you in advance for your attention and assistance.

fmcarvalho avatar Mar 21 '24 15:03 fmcarvalho

@esiqveland @scott-taylor @mhw Even if you are not currently using HtmlFlow, I kindly request your acknowledgment that I was actively engaged in maintaining, tracking issues, or new features during the time of this issue. It is imperative for me to justify my efforts on HtmlFlow during the period of this issue to my university.

I would greatly appreciate it if you could once again provide your acknowledgment to my email address at ISEL: [email protected].

Thank you in advance for your attention and assistance.

fmcarvalho avatar Mar 26 '24 11:03 fmcarvalho