Skip to content

Commit 066803a

Browse files
committed
GROOVY-12271: Confine snippet file resolution to the snippet-files directory
{@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.
1 parent 389ab8c commit 066803a

2 files changed

Lines changed: 104 additions & 2 deletions

File tree

subprojects/groovy-groovydoc/src/main/java/org/codehaus/groovy/tools/groovydoc/TagRenderer.java

Lines changed: 45 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828

2929
import java.io.IOException;
3030
import java.nio.file.Files;
31+
import java.nio.file.InvalidPathException;
3132
import java.nio.file.Path;
3233
import java.nio.file.Paths;
3334

@@ -477,6 +478,12 @@ private static int renderSnippetAt(String text, int start, int nameEnd, StringBu
477478
* GROOVY-11938 stage 2: look up a snippet file in the current package's
478479
* {@code snippet-files/} directory under any configured sourcepath and
479480
* return its contents, or {@code null} if not found.
481+
* <p>
482+
* The file name comes from a doc comment, so it is treated as relative to the package's
483+
* {@code snippet-files/} directory and resolution is confined to it. An absolute name is
484+
* refused, and a relative one which would escape the directory, whether by {@code ..}
485+
* segments or by way of a symbolic link, resolves to nothing rather than to a file
486+
* elsewhere on the machine running the tool.
480487
*/
481488
private static String loadSnippetFile(String fileName, GroovyRootDoc rootDoc, SimpleGroovyClassDoc classDoc, boolean keepHeader) {
482489
if (classDoc == null || !(rootDoc instanceof SimpleGroovyRootDoc)) return null;
@@ -488,8 +495,8 @@ private static String loadSnippetFile(String fileName, GroovyRootDoc rootDoc, Si
488495
int lastSlash = full.lastIndexOf('/');
489496
String pkgPath = lastSlash >= 0 ? full.substring(0, lastSlash) : "";
490497
for (String sourcepath : sourcepaths) {
491-
Path path = Paths.get(sourcepath, pkgPath, "snippet-files", fileName);
492-
if (Files.isRegularFile(path)) {
498+
Path path = resolveWithinSnippetFiles(sourcepath, pkgPath, fileName);
499+
if (path != null) {
493500
try {
494501
String text = Files.readString(path);
495502
return keepHeader ? text : stripLicenseHeader(text);
@@ -501,6 +508,42 @@ private static String loadSnippetFile(String fileName, GroovyRootDoc rootDoc, Si
501508
return null;
502509
}
503510

511+
/**
512+
* Resolves a doc-comment-supplied file name against one sourcepath's {@code snippet-files/}
513+
* directory, returning the file only when it is a regular file genuinely inside that
514+
* directory.
515+
*
516+
* @param sourcepath the sourcepath to resolve under
517+
* @param pkgPath the package path of the class being documented
518+
* @param fileName the file name taken from the doc comment
519+
* @return the contained file, or {@code null} if there is no such file within the directory
520+
*/
521+
private static Path resolveWithinSnippetFiles(String sourcepath, String pkgPath, String fileName) {
522+
Path base;
523+
Path candidate;
524+
try {
525+
Path name = Paths.get(fileName);
526+
// JEP 413 treats the file attribute as relative to the snippet path. Rejecting an
527+
// absolute name outright keeps that, and keeps a doc comment from depending on
528+
// where the project happens to sit on disk: an absolute path that resolved on the
529+
// author's machine would not resolve on a build agent.
530+
if (name.isAbsolute()) return null;
531+
base = Paths.get(sourcepath, pkgPath, "snippet-files").toAbsolutePath().normalize();
532+
candidate = base.resolve(name).toAbsolutePath().normalize();
533+
} catch (InvalidPathException e) {
534+
return null;
535+
}
536+
if (!candidate.startsWith(base) || !Files.isRegularFile(candidate)) return null;
537+
// Containment above is textual; re-check it after following any symbolic links so a
538+
// link inside snippet-files cannot point out of it.
539+
try {
540+
if (!candidate.toRealPath().startsWith(base.toRealPath())) return null;
541+
} catch (IOException e) {
542+
return null;
543+
}
544+
return candidate;
545+
}
546+
504547
/**
505548
* If the file begins with a C-style block comment that looks like a
506549
* license or copyright notice, strip that comment and any whitespace

subprojects/groovy-groovydoc/src/test/groovy/org/codehaus/groovy/tools/groovydoc/GroovyDocToolTest.java

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -310,6 +310,65 @@ public void testSnippetMarkupRegexCannotHangTheBuild() throws Exception {
310310
elapsedMs < 30_000L);
311311
}
312312

313+
// GROOVY-12271: the file attribute is relative to snippet-files/ per JEP 413, so an
314+
// absolute name is refused even when it would land inside the directory. Otherwise a doc
315+
// comment would resolve on the author's machine and not on a build agent.
316+
public void testSnippetTagExternalFormRefusesAbsoluteFileName() throws Exception {
317+
String pkg = "org/codehaus/groovy/tools/groovydoc/testfiles/docfiles";
318+
Path tmp = Files.createTempDirectory("snippet-absolute-");
319+
Path pkgDir = tmp.resolve(pkg);
320+
Path snippetDir = pkgDir.resolve("snippet-files");
321+
Files.createDirectories(snippetDir);
322+
String marker = "INSIDE_SNIPPET_FILES_MARKER";
323+
Files.writeString(snippetDir.resolve("Inside.groovy"), marker + "\n");
324+
325+
Files.writeString(pkgDir.resolve("AbsoluteName.groovy"),
326+
"package " + pkg.replace('/', '.') + "\n" +
327+
"/**\n" +
328+
" * absolute: {@snippet file=\"" + snippetDir.resolve("Inside.groovy") + "\" keepHeader=true}\n" +
329+
" * relative: {@snippet file=\"Inside.groovy\" keepHeader=true}\n" +
330+
" */\n" +
331+
"class AbsoluteName {}\n");
332+
333+
String doc = renderSingle(tmp, pkg, "AbsoluteName");
334+
assertNotNull(doc);
335+
// The relative form resolves, so exactly one of the two snippets rendered.
336+
int first = doc.indexOf(marker);
337+
assertTrue("Expected the relative form to resolve in:\n" + doc, first >= 0);
338+
assertEquals("Absolute file name should not have resolved in:\n" + doc,
339+
-1, doc.indexOf(marker, first + 1));
340+
}
341+
342+
// GROOVY-12271: the file name comes from a doc comment, so snippet resolution is confined
343+
// to the package's snippet-files/ directory. A name escaping it must resolve to nothing
344+
// rather than to a file elsewhere on the machine running the tool.
345+
public void testSnippetTagExternalFormCannotEscapeSnippetFiles() throws Exception {
346+
String pkg = "org/codehaus/groovy/tools/groovydoc/testfiles/docfiles";
347+
Path tmp = Files.createTempDirectory("snippet-traversal-");
348+
Path pkgDir = tmp.resolve(pkg);
349+
Files.createDirectories(pkgDir);
350+
Files.createDirectories(pkgDir.resolve("snippet-files"));
351+
352+
// Something worth stealing, outside snippet-files but reachable by walking up.
353+
String secretText = "TOP_SECRET_CREDENTIAL_VALUE";
354+
Files.writeString(tmp.resolve("secret.txt"), secretText);
355+
356+
// Depth from snippet-files/ back up to tmp: package segments plus snippet-files itself.
357+
String upToTmp = "../".repeat(pkg.split("/").length + 1);
358+
Files.writeString(pkgDir.resolve("Traversal.groovy"),
359+
"package " + pkg.replace('/', '.') + "\n" +
360+
"/**\n" +
361+
" * relative: {@snippet file=\"" + upToTmp + "secret.txt\" keepHeader=true}\n" +
362+
" * absolute: {@snippet file=\"" + tmp.resolve("secret.txt") + "\" keepHeader=true}\n" +
363+
" */\n" +
364+
"class Traversal {}\n");
365+
366+
String doc = renderSingle(tmp, pkg, "Traversal");
367+
assertNotNull(doc);
368+
assertFalse("Snippet file resolution escaped snippet-files/ in:\n" + doc,
369+
doc.contains(secretText));
370+
}
371+
313372
/** Renders one class from a temporary source tree and returns its page. */
314373
private String renderSingle(Path sourcePath, String pkg, String simpleName) throws Exception {
315374
GroovyDocTool tool = new GroovyDocTool(

0 commit comments

Comments
 (0)