You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2022/07/10 10:10:59 UTC
[GitHub] [maven-surefire] sman-81 opened a new pull request, #554: [SUREFIRE-2109] Add suffix derived from current user to Surefire temp directory name and make directory writable for all
sman-81 opened a new pull request, #554:
URL: https://github.com/apache/maven-surefire/pull/554
This pull request offers a fix for bug [SUREFIRE-2109].
Following this checklist to help us incorporate your contribution quickly and easily:
- [x] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/SUREFIRE) filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
- [x] Each commit in the pull request should have a meaningful subject line and body.
- [x] Format the pull request title like `[SUREFIRE-XXX] - Fixes bug in ApproximateQuantiles`,
where you replace `SUREFIRE-XXX` with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Run `mvn clean install` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [x] You have run the integration tests successfully (`mvn -Prun-its clean install`).
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [x] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] sman-81 commented on a diff in pull request #554: [SUREFIRE-2109] Add suffix derived from current user to Surefire temp directory name and make directory writable for all
Posted by GitBox <gi...@apache.org>.
sman-81 commented on code in PR #554:
URL: https://github.com/apache/maven-surefire/pull/554#discussion_r1059393355
##########
surefire-api/src/main/java/org/apache/maven/surefire/api/util/TempFileManager.java:
##########
@@ -180,6 +180,8 @@ public synchronized File createTempFile( String prefix, String suffix )
throw new UncheckedIOException( new IOException(
"Unable to create temporary directory " + tempDir.getAbsolutePath() ) );
}
+ // try to make temp file directory writable for all
+ tempDir.setWritable( true, false );
Review Comment:
The aim of the original PR was to stop Surefire from bloating the system temp directory by instead having it write into a subdirectory 'surefire'. The subdirectory was only writeable by the user that created it. So Surefire would fail if another user ran tests on the same machine (before reboot or otherwise cleaning up temp). Thus the user suffix is introduced by this PR. As user names may contain characters illegal in directory names, there is a risk, even though small or theoretic, that two users have identically names temp subdirectories. By making the directory writeable for all, this risk is eliminated.
Until very recently Surefire wrote to system temp which by definition is shared by all users and was never a security concern to anyone. This PR leaves this semantic untouched.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] sman-81 commented on pull request #554: [SUREFIRE-2109] Add suffix derived from current user to Surefire temp directory name and make directory writable for all
Posted by GitBox <gi...@apache.org>.
sman-81 commented on PR #554:
URL: https://github.com/apache/maven-surefire/pull/554#issuecomment-1229985058
Hi @olamy is there anything left to do for me, or is this pr ready to be merged?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] andpab commented on a diff in pull request #554: [SUREFIRE-2109] Add suffix derived from current user to Surefire temp directory name and make directory writable for all
Posted by GitBox <gi...@apache.org>.
andpab commented on code in PR #554:
URL: https://github.com/apache/maven-surefire/pull/554#discussion_r1059513138
##########
surefire-api/src/main/java/org/apache/maven/surefire/api/util/TempFileManager.java:
##########
@@ -180,6 +180,8 @@ public synchronized File createTempFile( String prefix, String suffix )
throw new UncheckedIOException( new IOException(
"Unable to create temporary directory " + tempDir.getAbsolutePath() ) );
}
+ // try to make temp file directory writable for all
+ tempDir.setWritable( true, false );
Review Comment:
Thanks for the explanation.
I understand the rationale now, but what about the scenario that `java.io.tmpdir` is set to a user-specific location in an otherwise protected area? I still think there should be a difference in scrutiny applied between writing to a destination that is already world-writable by design and explicitly making the destination world-writable.
How about passing the username through `URLEncoder#encode` instead of removing special characters? That is guaranteed to create a directory name that is collision-free and valid on all file systems. It also handles the issue immediately at the code location where it arises rather than working around it somewhere else in a manner that is not particularly easy to grasp.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] sman-81 commented on a diff in pull request #554: [SUREFIRE-2109] Add suffix derived from current user to Surefire temp directory name
Posted by GitBox <gi...@apache.org>.
sman-81 commented on code in PR #554:
URL: https://github.com/apache/maven-surefire/pull/554#discussion_r1059859610
##########
surefire-api/src/main/java/org/apache/maven/surefire/api/util/TempFileManager.java:
##########
@@ -180,6 +180,8 @@ public synchronized File createTempFile( String prefix, String suffix )
throw new UncheckedIOException( new IOException(
"Unable to create temporary directory " + tempDir.getAbsolutePath() ) );
}
+ // try to make temp file directory writable for all
+ tempDir.setWritable( true, false );
Review Comment:
I've removed the "writable for all" part. The chance of two similar user names - working on the same machine! - resulting in identical sub directory names is theoretic at best. Having the user name as part of the directory name is helpful to quickly spot one's own directory. We are good leaving this part of the code as-is IMO. I look forward to your feedback.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] sman-81 commented on a diff in pull request #554: [SUREFIRE-2109] Add suffix derived from current user to Surefire temp directory name
Posted by GitBox <gi...@apache.org>.
sman-81 commented on code in PR #554:
URL: https://github.com/apache/maven-surefire/pull/554#discussion_r1059859610
##########
surefire-api/src/main/java/org/apache/maven/surefire/api/util/TempFileManager.java:
##########
@@ -180,6 +180,8 @@ public synchronized File createTempFile( String prefix, String suffix )
throw new UncheckedIOException( new IOException(
"Unable to create temporary directory " + tempDir.getAbsolutePath() ) );
}
+ // try to make temp file directory writable for all
+ tempDir.setWritable( true, false );
Review Comment:
I've removed the "writable for all" part. The chance of two similar user names - working on the same machine! - resulting in identical sub directory names is theoretic at best. Having the user name as part of the directory name is helpful to quickly spot one's own directory. We are good leaving this code as-is IMO. I look forward to your feedback
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] slawekjaranowski merged pull request #554: [SUREFIRE-2109] Add suffix derived from current user to Surefire temp directory name
Posted by GitBox <gi...@apache.org>.
slawekjaranowski merged PR #554:
URL: https://github.com/apache/maven-surefire/pull/554
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] cpfeiffer commented on a diff in pull request #554: [SUREFIRE-2109] Add suffix derived from current user to Surefire temp directory name and make directory writable for all
Posted by GitBox <gi...@apache.org>.
cpfeiffer commented on code in PR #554:
URL: https://github.com/apache/maven-surefire/pull/554#discussion_r1050900556
##########
surefire-api/src/main/java/org/apache/maven/surefire/api/util/TempFileManager.java:
##########
@@ -180,6 +180,8 @@ public synchronized File createTempFile( String prefix, String suffix )
throw new UncheckedIOException( new IOException(
"Unable to create temporary directory " + tempDir.getAbsolutePath() ) );
}
+ // try to make temp file directory writable for all
+ tempDir.setWritable( true, false );
Review Comment:
Security-wise, I don't think it's a good idea to have a shared directory with other users. Why would you need this? Also, why not use `java.nio.file.Files.createTempDirectory()` and specify 'surefire-' + the username as prefix?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] cpfeiffer commented on a diff in pull request #554: [SUREFIRE-2109] Add suffix derived from current user to Surefire temp directory name and make directory writable for all
Posted by GitBox <gi...@apache.org>.
cpfeiffer commented on code in PR #554:
URL: https://github.com/apache/maven-surefire/pull/554#discussion_r1059640012
##########
surefire-api/src/main/java/org/apache/maven/surefire/api/util/TempFileManager.java:
##########
@@ -180,6 +180,8 @@ public synchronized File createTempFile( String prefix, String suffix )
throw new UncheckedIOException( new IOException(
"Unable to create temporary directory " + tempDir.getAbsolutePath() ) );
}
+ // try to make temp file directory writable for all
+ tempDir.setWritable( true, false );
Review Comment:
If you only want to avoid clashes between multiple users, [Files.createTempDirectory("surefire-")](https://docs.oracle.com/javase/8/docs/api/java/nio/file/Files.html#createTempDirectory-java.lang.String-java.nio.file.attribute.FileAttribute...-) would create the unique, collision-free directory for you.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] andpab commented on a diff in pull request #554: [SUREFIRE-2109] Add suffix derived from current user to Surefire temp directory name and make directory writable for all
Posted by GitBox <gi...@apache.org>.
andpab commented on code in PR #554:
URL: https://github.com/apache/maven-surefire/pull/554#discussion_r1059654222
##########
surefire-api/src/main/java/org/apache/maven/surefire/api/util/TempFileManager.java:
##########
@@ -180,6 +180,8 @@ public synchronized File createTempFile( String prefix, String suffix )
throw new UncheckedIOException( new IOException(
"Unable to create temporary directory " + tempDir.getAbsolutePath() ) );
}
+ // try to make temp file directory writable for all
+ tempDir.setWritable( true, false );
Review Comment:
That would bloat the system temp directory again, so it would run counter to the motivation for the original change as described in SUREFIRE-2086.
As @sman-81 said: The aim of the original PR was to stop Surefire from bloating the system temp directory by instead having it create the new directory in a fixed subdirectory of the system temp directory, not the system temp directory itself.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] cpfeiffer commented on a diff in pull request #554: [SUREFIRE-2109] Add suffix derived from current user to Surefire temp directory name and make directory writable for all
Posted by GitBox <gi...@apache.org>.
cpfeiffer commented on code in PR #554:
URL: https://github.com/apache/maven-surefire/pull/554#discussion_r1059640012
##########
surefire-api/src/main/java/org/apache/maven/surefire/api/util/TempFileManager.java:
##########
@@ -180,6 +180,8 @@ public synchronized File createTempFile( String prefix, String suffix )
throw new UncheckedIOException( new IOException(
"Unable to create temporary directory " + tempDir.getAbsolutePath() ) );
}
+ // try to make temp file directory writable for all
+ tempDir.setWritable( true, false );
Review Comment:
If you only want to avoid clashes between multiple users, using [Files.createTempDirectory("surefire-")](https://docs.oracle.com/javase/8/docs/api/java/nio/file/Files.html#createTempDirectory-java.lang.String-java.nio.file.attribute.FileAttribute...-) would create the unique, collision-free directory for you.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] andpab commented on pull request #554: [SUREFIRE-2109] Add suffix derived from current user to Surefire temp directory name
Posted by GitBox <gi...@apache.org>.
andpab commented on PR #554:
URL: https://github.com/apache/maven-surefire/pull/554#issuecomment-1369670644
@olamy Can you merge this PR?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] sman-81 commented on pull request #554: [SUREFIRE-2109] Add suffix derived from current user to Surefire temp directory name
Posted by GitBox <gi...@apache.org>.
sman-81 commented on PR #554:
URL: https://github.com/apache/maven-surefire/pull/554#issuecomment-1369647504
This is ready to ship now.
Who can merge this PR?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] slawekjaranowski commented on pull request #554: [SUREFIRE-2109] Add suffix derived from current user to Surefire temp directory name
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on PR #554:
URL: https://github.com/apache/maven-surefire/pull/554#issuecomment-1370318893
Looks good enough ...
I see one another issue - what when JVM not exit immediately ... like in mvnd - classes are loaded once and can be reused. But it can be next issue ...
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] andpab commented on a diff in pull request #554: [SUREFIRE-2109] Add suffix derived from current user to Surefire temp directory name and make directory writable for all
Posted by GitBox <gi...@apache.org>.
andpab commented on code in PR #554:
URL: https://github.com/apache/maven-surefire/pull/554#discussion_r1058956056
##########
surefire-api/src/main/java/org/apache/maven/surefire/api/util/TempFileManager.java:
##########
@@ -180,6 +180,8 @@ public synchronized File createTempFile( String prefix, String suffix )
throw new UncheckedIOException( new IOException(
"Unable to create temporary directory " + tempDir.getAbsolutePath() ) );
}
+ // try to make temp file directory writable for all
+ tempDir.setWritable( true, false );
Review Comment:
I don't understand the "writable for all" part of the change either. If the temp directory name is user-specific, why would anyone else need to have write access?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] sman-81 commented on a diff in pull request #554: [SUREFIRE-2109] Add suffix derived from current user to Surefire temp directory name and make directory writable for all
Posted by GitBox <gi...@apache.org>.
sman-81 commented on code in PR #554:
URL: https://github.com/apache/maven-surefire/pull/554#discussion_r1059654803
##########
surefire-api/src/main/java/org/apache/maven/surefire/api/util/TempFileManager.java:
##########
@@ -180,6 +180,8 @@ public synchronized File createTempFile( String prefix, String suffix )
throw new UncheckedIOException( new IOException(
"Unable to create temporary directory " + tempDir.getAbsolutePath() ) );
}
+ // try to make temp file directory writable for all
+ tempDir.setWritable( true, false );
Review Comment:
Thanks for your input!
I'll amend and rebase the PR early next week.
Until then: Happy New Year!
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] sman-81 commented on pull request #554: [SUREFIRE-2109] Add suffix derived from current user to Surefire temp directory name and make directory writable for all
Posted by GitBox <gi...@apache.org>.
sman-81 commented on PR #554:
URL: https://github.com/apache/maven-surefire/pull/554#issuecomment-1368229596
Thanks for your input!
I'll amend and rebase the PR early next week.
Until then: Happy New Year!
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org