OWASP / java-html-sanitizer

Takes third-party HTML and produces HTML that is safe to embed in your web application. Fast and easy to configure.
Other
850 stars 214 forks source link

Consider clarifying thread safety javadocs #250

Open lread opened 2 years ago

lread commented 2 years ago

Observation

After reading the javadocs on thread safety for the HTML policy builder, I am somewhat, but not entirely, confident about what I should be doing.

Very clear:

This class is not thread-safe.

2nd paragraph is pretty clear too:

The resulting policy will not violate its security guarantees as a result of race conditions, but is not thread safe because it maintains state to track whether text inside disallowed elements should be suppressed.

3d paragraph is maybe not so clear?:

The resulting policy can be reused, but if you use the {@link HtmlPolicyBuilder#toFactory()} method instead of {@link #build}, then binding policies to output channels is cheap so there's no need.

Proposal

Maybe reword the 2nd and 3rd paragraphs to focus on what the user needs to know?

The policy returned by {@link #build} is stateful and not thread-safe.

For thread-safety, use the policy factory returned by {@link #toFactory()}.

If I've got that right? Think I do, but not 100% sure.

Watcha think?

Maybe a good idea? Maybe not? Happy to hear back from those in know!