GROOVY-12271: Confine snippet file resolution to the snippet-files di… - #2808
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2808 +/- ##
==================================================
- Coverage 70.1877% 70.1823% -0.0054%
- Complexity 35849 35854 +5
==================================================
Files 1562 1562
Lines 132556 132569 +13
Branches 24384 24387 +3
==================================================
+ Hits 93038 93040 +2
- Misses 31108 31118 +10
- Partials 8410 8411 +1
🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
…rectory
{@snippet file="..."} took the file name verbatim from a doc comment and
joined it onto the package's snippet-files/ directory with no normalization
or containment check, so ../ segments escaped to anywhere the user running
groovydoc could read. JEP 413 confines javadoc's snippet resolution to
--snippet-path; the port added in GROOVY-11938 omitted the check.
This matters because it grants the author of documented source a capability
at doc time rather than at run time: a doc comment in a pull request can
read a file from the machine building the docs and publish its contents in
the rendered page.
Treat the file attribute as relative to snippet-files/, as JEP 413 does,
and confine resolution to that directory:
- an absolute name is refused outright, rather than accepted when it
happens to land inside the directory, so that a doc comment cannot
resolve on its author's machine and fail on a build agent;
- a relative name is normalized and required to stay inside;
- containment is re-checked after following symbolic links, so a link
within the directory cannot point out of it;
- an unusable name renders nothing instead of throwing.
Covered by two tests: one placing a file outside snippet-files/ and
referencing it relatively and absolutely, asserting its contents never
reach the rendered page; one referencing a file that genuinely is inside,
by both an absolute and a relative name, asserting only the relative form
resolves. Removing either guard fails the corresponding test.
773f8df to
d241dda
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes GroovyDoc’s {@snippet file="..."} external-form file resolution to match JEP 413 by treating file as relative to the package’s snippet-files/ directory and strictly confining resolution to that directory (including after symlink resolution), preventing .. traversal and absolute-path usage.
Changes:
- Add safe path resolution (
resolveWithinSnippetFiles) that rejects absolute paths, normalizes relative paths, enforces containment, and re-checks containment after following symlinks. - Update snippet loading to use the confined resolver and render nothing (instead of throwing) when the file name is unusable.
- Add regression tests covering absolute-path refusal and traversal escape prevention.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| subprojects/groovy-groovydoc/src/test/groovy/org/codehaus/groovy/tools/groovydoc/GroovyDocToolTest.java | Adds regression tests ensuring snippet external-form file resolution cannot use absolute paths or escape snippet-files/. |
| subprojects/groovy-groovydoc/src/main/java/org/codehaus/groovy/tools/groovydoc/TagRenderer.java | Implements confined snippet file resolution (reject absolute, normalize/contain, re-check after symlinks) and uses it during snippet loading. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…rectory
{@snippet file="..."} took the file name verbatim from a doc comment and joined it onto the package's snippet-files/ directory with no normalization or containment check, so ../ segments escaped to anywhere the user running groovydoc could read. JEP 413 confines javadoc's snippet resolution to --snippet-path; the port added in GROOVY-11938 omitted the check.
This matters because it grants the author of documented source a capability at doc time rather than at run time: a doc comment in a pull request can read a file from the machine building the docs and publish its contents in the rendered page.
Treat the file attribute as relative to snippet-files/, as JEP 413 does, and confine resolution to that directory:
Covered by two tests: one placing a file outside snippet-files/ and referencing it relatively and absolutely, asserting its contents never reach the rendered page; one referencing a file that genuinely is inside, by both an absolute and a relative name, asserting only the relative form resolves. Removing either guard fails the corresponding test.