Skip to content

GROOVY-12271: Confine snippet file resolution to the snippet-files di… - #2808

Merged
paulk-asert merged 1 commit into
apache:masterfrom
paulk-asert:groovy12271
Aug 19, 2026
Merged

GROOVY-12271: Confine snippet file resolution to the snippet-files di…#2808
paulk-asert merged 1 commit into
apache:masterfrom
paulk-asert:groovy12271

Conversation

@paulk-asert

Copy link
Copy Markdown
Contributor

…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.

@codecov-commenter

codecov-commenter commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.1823%. Comparing base (389ab8c) to head (d241dda).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...g/codehaus/groovy/tools/groovydoc/TagRenderer.java 66.6667% 4 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@                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     
Files with missing lines Coverage Δ
...g/codehaus/groovy/tools/groovydoc/TagRenderer.java 71.9415% <66.6667%> (+0.3583%) ⬆️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@testlens-app

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@paulk-asert
paulk-asert merged commit 066803a into apache:master Aug 19, 2026
30 checks passed
@paulk-asert
paulk-asert deleted the groovy12271 branch August 19, 2026 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants