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

Enable auto escaping of strings in Freemarker templates #2251

Merged
merged 1 commit into from Jan 22, 2018

Conversation

Projects
None yet
5 participants
@willp-bl
Contributor

willp-bl commented Jan 22, 2018

Problem

Output was not being auto escaped by Freemarker

Solution

Freemarker output format was not set [as HTML] which meant that auto escaping was not enabled. With this change it is enabled by default.

This might affect users who are using formats other than HTML, but
there should be enough benefit of making this a default. The current method for getting the output format setting through to the underlying freemarker library is not very pleasant.

Users relying on the existing behaviour may need to set individual
freemarker values as ?unsafe

Question

I couldn't find security contact information - is that something you are planning on adding?

Enable auto escaping of strings in Freemarker templates
This was not previously enabled because the output format had not
been set [as HTML].  With this change it is enabled by default.

Users relying on the existing behaviour may need to set individual
freemarker values as `?unsafe`.
@arteam

This comment has been minimized.

Member

arteam commented Jan 22, 2018

Thank you for the pull request! We've made some progress in this area in 1.3.* : with #2213 you can use ftlh templates with automatic escaping. Anyway, I think it makes sense to use the HTML escaping for ftl templates by default. I will make a maintenance release for the 1.2 branch.

@arteam

This comment has been minimized.

Member

arteam commented Jan 22, 2018

I couldn't find security contact information - is that something you are planning on adding?

Unfortunately, we don't have a security policy like Rails, Django, or Spring and don't provide a email address for secure communications. I guess Dropwizard isn't big enough for that, but having a security policy is definitely is a good practice for a mature project. For the time being, I think the best way to disclose security issues is to post a request to disclose to the dropwizard-dev mailing list and/or contact on of the maintainers privately.

@arteam arteam merged commit d23d65d into dropwizard:master Jan 22, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

arteam added a commit that referenced this pull request Jan 22, 2018

Enable auto escaping of strings in Freemarker templates (#2251)
This was not previously enabled because the output format had not
been set [as HTML].  With this change it is enabled by default.

Users relying on the existing behaviour may need to set individual
freemarker values as `?unsafe`.

(cherry picked from commit d23d65d)
@mattnelson

This comment has been minimized.

Contributor

mattnelson commented Jan 22, 2018

I think the best way to disclose security issues is to post a request to disclose to the dropwizard-dev mailing list and/or contact on of the maintainers privately.

Are there email services for dropwizard.io?

@arteam

This comment has been minimized.

Member

arteam commented Jan 22, 2018

Are there email services for dropwizard.io?

I don't think so. AFAIK, Rent the Runway sponsors the domain name which is managed by 101domain.com, but that's it. Maybe we could talk about setting up an email hosting service with them.

@joschi

This comment has been minimized.

Member

joschi commented Jan 22, 2018

@mattnelson See http://www.dropwizard.io/1.2.2/docs/about/index.html

If there's the need for it, we could probably create a "write-only" Google Group which only the maintainers can read.

@willp-bl

This comment has been minimized.

Contributor

willp-bl commented Jan 22, 2018

Thanks for merging

The failure on the 1.2 branch is probably down to the config interface/format changes in freemarker itself.

@willp-bl willp-bl deleted the willp-bl:auto_escape_freemarker branch Jan 22, 2018

@arteam

This comment has been minimized.

Member

arteam commented Jan 22, 2018

Yes, I've removed the explicit escaping in the test example: 8cd9bdf As soon as the tests pass on Travis CI, I will make a release.

@arteam

This comment has been minimized.

Member

arteam commented Jan 22, 2018

Unfortunately, Sonatype is down and I can't deploy the release. Will wait when they're up again.

@arteam

This comment has been minimized.

Member

arteam commented Jan 24, 2018

1.2.3 has been released

@jplock jplock added this to the 1.3.0 milestone Jan 24, 2018

@jplock jplock added the improvement label Jan 24, 2018

@jplock jplock modified the milestones: 1.3.0, 1.2.3 Jan 24, 2018

Silvmike added a commit to Silvmike/dropwizard that referenced this pull request Mar 3, 2018

Fixed template broken after enabling html escaping of strings in Free…
…marker templates by default, added integration tests.

See also: dropwizard#2251

Before you could see this on logs trying to render 'view_freemarker' template:

Causing: io.dropwizard.views.ViewRenderException: freemarker.core.ParseException: Syntax error in template "com/example/helloworld/views/freemarker/person.ftl" in line 8, column 38:
! Using ?html (legacy escaping) is not allowed when auto-escaping is on with a markup output format (HTML), to avoid double-escaping mistakes.

aaanders added a commit to aaanders/dropwizard that referenced this pull request Sep 20, 2018

Enable auto escaping of strings in Freemarker templates (dropwizard#2251
)

This was not previously enabled because the output format had not
been set [as HTML].  With this change it is enabled by default.

Users relying on the existing behaviour may need to set individual
freemarker values as `?unsafe`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment