Replace GroovyPagesTemplateEngine for Zebra templates - #5943
Conversation
mdpearson
commented
May 26, 2026
There was a problem hiding this comment.
Pull request overview
This PR hardens the Zebra template rendering endpoints by removing Groovy/GSP evaluation of user-provided document content and replacing it with a restricted, Spring BeanWrapper-based ${...} property-resolution engine, along with shared document lookup/type validation and new tests.
Changes:
- Consolidates Zebra template document loading/type validation into a shared
loadZebraTemplate()helper used by all four Zebra endpoints. - Replaces the prior Groovy/GSP-backed template rendering with
BeanPropertyTemplateServiceusing Apache Commons TextStringSubstitutor+ SpringBeanWrapperImpl. - Adds unit tests covering controller-level document type checks and service-level expression restrictions/escaping behavior.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
grails-app/controllers/org/pih/warehouse/core/DocumentController.groovy |
Switches Zebra endpoints to BeanPropertyTemplateService and centralizes Zebra template loading/type validation. |
grails-app/services/org/pih/warehouse/core/TemplateService.groovy |
Removes the GroovyPagesTemplateEngine-backed service previously used for template evaluation. |
grails-app/services/org/pih/warehouse/core/BeanPropertyTemplateService.groovy |
Introduces a restricted ${...} resolver based on BeanWrapperImpl with binding/property/type constraints and value escaping. |
src/test/groovy/org/pih/warehouse/core/DocumentControllerZebraTemplateSpec.groovy |
Adds controller unit tests ensuring Zebra endpoints reject non-ZEBRA_TEMPLATE documents and missing IDs. |
src/test/groovy/org/pih/warehouse/core/BeanPropertyTemplateServiceSpec.groovy |
Adds service tests for allowed property paths, blocked expression types, blocked properties/types, and escaping behavior. |
build.gradle |
Adds Apache Commons Text dependency for StringSubstitutor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release/0.9.8 #5943 +/- ##
================================================
Coverage ? 10.63%
Complexity ? 1635
================================================
Files ? 807
Lines ? 47595
Branches ? 11248
================================================
Hits ? 5063
Misses ? 41716
Partials ? 816 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The four Zebra template endpoints (build, print, render, export)
accepted any document ID and evaluated its content as Groovy code via
GroovyPagesTemplateEngine.
Three layers of fixes:
1. Consolidate document lookup and type validation into
loadZebraTemplate(), used by all four endpoints. It returns 404 for
missing documents and throws IllegalArgumentException for documents
whose type is not ZEBRA_TEMPLATE.
2. Replace TemplateService (which wraps GroovyPagesTemplateEngine) with
BeanPropertyTemplateService, which resolves ${...} expressions by
delegating the full property path to Spring's BeanWrapperImpl. No
arbitrary Groovy evaluation; unresolvable expressions throw rather
than appearing in label output. Bracket-indexed property access is
permitted; GSP tags and method calls are rejected with an exception.
BeanWrapperImpl, being Spring-based, will throw if it encounters a
Groovy safe-navigation delimiter (`?.`). That's a breaking change
compared to the previous implementation, but I'm not sure any of our
templates rely on this behavior. Similarly with `?:`.
Property traversal is further restricted:
- Block 'class' and 'metaClass' by name, plus Grails domain
internals (`constraints`, `domainClass`, `errors`, `mapping`,
`properties`).
- Block properties of type Class, ClassLoader, MetaClass, or
ProtectionDomain.
- Restrict variable access to ALLOWED_BINDING_NAMES (which includes
product and facility), rejecting names outside the set even if
present in bindings, to prevent access to framework objects such
as `grailsApplication`.
3. Escape the ${ sequence in resolved values so property values
containing ${...} are not re-interpreted as template expressions on
a second pass. Literal dollar signs are preserved (e.g. "AB$202" is
not corrupted to "AB$$202").
Signed-off-by: Matthew Pearson <matthewpearson@gmail.com>
f7f0ab1 to
ea5ba4b
Compare