ramanathan1504 commented on code in PR #4229:
URL: https://github.com/apache/logging-log4j2/pull/4229#discussion_r3894147827
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/action/PosixViewAttributeAction.java:
##########
@@ -369,6 +369,10 @@ protected FileVisitor<Path> createFileVisitor(final Path
basePath, final List<Pa
return new SimpleFileVisitor<Path>() {
@Override
public FileVisitResult visitFile(final Path file, final
BasicFileAttributes attrs) throws IOException {
+ if (!isFollowSymbolicLinks() && attrs.isSymbolicLink()) {
Review Comment:
Withdrawing my earlier suggestion here, sorry — under `FOLLOW_LINKS` a valid
link is `stat`-resolved before `visitFile` sees it so the gate changes nothing,
and on a *dangling* link it throws `NoSuchFileException` out of `walkFileTree`
and ends the scan; your original line was right.
```suggestion
if (attrs.isSymbolicLink()) {
```
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/rolling/action/PosixViewAttributeActionTest.java:
##########
Review Comment:
`testSymbolicLinksAreFollowedWhenConfigured` passes with and without the
change to 372, so the dangling-link case is the one that actually guards F1 —
sketch below rather than a suggestion block, since it is a whole method:
```java
@Test
void testBrokenSymbolicLinkDoesNotAbortTheScan(@TempDir final Path tempDir)
throws Exception {
final Path baseDir = Files.createDirectory(tempDir.resolve("logs"));
final Path regularFile = baseDir.resolve("app-1.log");
Files.write(regularFile, "log".getBytes(StandardCharsets.UTF_8));
Files.setPosixFilePermissions(regularFile,
PosixFilePermissions.fromString("rw-------"));
Files.createSymbolicLink(baseDir.resolve("app-0-broken.log"),
tempDir.resolve("gone.txt"));
final Configuration config = new BasicConfigurationFactory().new
BasicConfiguration();
final PosixViewAttributeAction action =
PosixViewAttributeAction.newBuilder()
.setBasePath(baseDir.toString())
.setFollowLinks(true)
.setMaxDepth(1)
.setPathConditions(PathCondition.EMPTY_ARRAY)
.setConfiguration(config)
.setFilePermissionsString("rw-rw-rw-")
.build();
action.execute();
assertEquals(
"rw-rw-rw-",
PosixFilePermissions.toString(Files.getPosixFilePermissions(regularFile)),
"a dangling link must not stop the scan");
}
```
##########
src/changelog/.2.x.x/fix_posix_view_attribute_symlink.xml:
##########
@@ -0,0 +1,8 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+ xmlns="https://logging.apache.org/xml/ns";
+ xsi:schemaLocation="https://logging.apache.org/xml/ns
https://logging.apache.org/xml/ns/log4j-changelog-0.xsd";
+ type="fixed">
+ <issue id="4229" link="https://github.com/apache/logging-log4j2/pull/4229"/>
+ <description format="asciidoc">Stop `PosixViewAttribute` from applying
permissions and ownership through symbolic links found in `basePath` unless
`followLinks` is set to `true`.</description>
Review Comment:
```suggestion
<description format="asciidoc">Stop `PosixViewAttribute` from applying
permissions and ownership through symbolic links found in `basePath` unless
`followLinks` is set to `true`</description>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
