[SUREFIRE-2109] Add suffix derived from current user to Surefire temp directory name #554
Conversation
|
Hi @olamy is there anything left to do for me, or is this pr ready to be merged? |
| @@ -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 ); | |||
There was a problem hiding this 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?
There was a problem hiding this 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?
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this comment.
If you only want to avoid clashes between multiple users, Files.createTempDirectory("surefire-") would create the unique, collision-free directory for you.
There was a problem hiding this 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.
There was a problem hiding this comment.
Thanks for your input!
I'll amend and rebase the PR early next week.
Until then: Happy New Year!
There was a problem hiding this 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.
spannm
changed the title
There was a problem hiding this comment.
Fine with me. 👍
For the record: The URLEncoder approach would also have kept the username unchanged for the character set you have in your regex. From the javadoc:
- The alphanumeric characters "a" through "z", "A" through "Z" and "0" through "9" remain the same.
- The special characters ".", "-", "*", and "_" remain the same.
|
This is ready to ship now. |
|
@olamy Can you merge this PR? |
|
Looks good enough ... |
This pull request offers a fix for bug [SUREFIRE-2109].
Following this checklist to help us incorporate your contribution quickly and easily:
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.
[SUREFIRE-XXX] - Fixes bug in ApproximateQuantiles,where you replace
SUREFIRE-XXXwith the appropriate JIRA issue. Best practiceis to use the JIRA issue title in the pull request title and in the first line of the
commit message.
mvn clean installto make sure basic checks pass. A more thorough check willbe performed on your pull request automatically.
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 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
you have to acknowledge this by using the following check-box.
I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.