You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2021/01/28 09:41:54 UTC
[GitHub] [commons-io] boris-unckel opened a new pull request #195: [IO-710] Utilize java.io.UncheckedIOException; rethrow SecurityException
boris-unckel opened a new pull request #195:
URL: https://github.com/apache/commons-io/pull/195
Fixes [https://issues.apache.org/jira/browse/IO-710](https://issues.apache.org/jira/browse/IO-710)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] boris-unckel closed pull request #195: [IO-710] Utilize java.io.UncheckedIOException; rethrow SecurityException
Posted by GitBox <gi...@apache.org>.
boris-unckel closed pull request #195:
URL: https://github.com/apache/commons-io/pull/195
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] boris-unckel commented on pull request #195: [IO-710] Utilize java.io.UncheckedIOException; rethrow SecurityException
Posted by GitBox <gi...@apache.org>.
boris-unckel commented on pull request #195:
URL: https://github.com/apache/commons-io/pull/195#issuecomment-769752005
Some portions are now here: [https://github.com/apache/commons-io/commit/c48ccaf1e60eb91b7849cba8d081f71bb9da630d](https://github.com/apache/commons-io/commit/c48ccaf1e60eb91b7849cba8d081f71bb9da630d)
The SecurityException part is now here: [https://github.com/apache/commons-io/pull/197](https://github.com/apache/commons-io/pull/197) dealt in [https://issues.apache.org/jira/browse/IO-712](https://issues.apache.org/jira/browse/IO-712)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] boris-unckel commented on a change in pull request #195: [IO-710] Utilize java.io.UncheckedIOException; rethrow SecurityException
Posted by GitBox <gi...@apache.org>.
boris-unckel commented on a change in pull request #195:
URL: https://github.com/apache/commons-io/pull/195#discussion_r566680956
##########
File path: src/main/java/org/apache/commons/io/FileUtils.java
##########
@@ -1232,12 +1233,16 @@ public static boolean deleteQuietly(final File file) {
if (file.isDirectory()) {
cleanDirectory(file);
}
+ } catch (final SecurityException sec) {
Review comment:
@garydgregory This is a important one.
In general: A single JavaDoc should not override global design principles. The SecurityManager has cross functional purpose. [Edit:] I agree that the missing documentation to throw a SecurityExcpetion is necessary. My fault, will correct that.
UseCase A: One wants to configure the SecurityManager and grant permissions. Part of the application is to delete a file. If the permission is missing, cleaning does not work. The missing exception does not allow to recognize that.
UseCase B: One has activated the SecurityManager. An attacker abuses the deleteQuietly method. The missing SecurityException hides this attempt, you're IDS can't alarm.
UseCase C: One utilizes the SecurityManager to test the system, to ensure every property (like file location) is set properly. The missing SecurityException does not support this UseCase.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] boris-unckel commented on a change in pull request #195: [IO-710] Utilize java.io.UncheckedIOException; rethrow SecurityException
Posted by GitBox <gi...@apache.org>.
boris-unckel commented on a change in pull request #195:
URL: https://github.com/apache/commons-io/pull/195#discussion_r566680956
##########
File path: src/main/java/org/apache/commons/io/FileUtils.java
##########
@@ -1232,12 +1233,16 @@ public static boolean deleteQuietly(final File file) {
if (file.isDirectory()) {
cleanDirectory(file);
}
+ } catch (final SecurityException sec) {
Review comment:
@garydgregory This is a important one.
In general: A single JavaDoc should not override global design principles. The SecurityManager has cross functional purpose.
UseCase A: One wants to configure the SecurityManager and grant permissions. Part of the application is to delete a file. If the permission is missing, cleaning does not work. The missing exception does not allow to recognize that.
UseCase B: One has activated the SecurityManager. An attacker abuses the deleteQuietly method. The missing SecurityException hides this attempt, you're IDS can't alarm.
UseCase C: One utilizes the SecurityManager to test the system, to ensure every property (like file location) is set properly. The missing SecurityException does not support this UseCase.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] boris-unckel commented on a change in pull request #195: [IO-710] Utilize java.io.UncheckedIOException; rethrow SecurityException
Posted by GitBox <gi...@apache.org>.
boris-unckel commented on a change in pull request #195:
URL: https://github.com/apache/commons-io/pull/195#discussion_r566680956
##########
File path: src/main/java/org/apache/commons/io/FileUtils.java
##########
@@ -1232,12 +1233,16 @@ public static boolean deleteQuietly(final File file) {
if (file.isDirectory()) {
cleanDirectory(file);
}
+ } catch (final SecurityException sec) {
Review comment:
@garydgregory This is a important one.
In general: A single JavaDoc should not override global design principles. The SecurityManager has cross functional purpose. [Edit:] I agrees that the missing documentation to throw a SecurityExcpetion is necessary. My fault, will correct that.
UseCase A: One wants to configure the SecurityManager and grant permissions. Part of the application is to delete a file. If the permission is missing, cleaning does not work. The missing exception does not allow to recognize that.
UseCase B: One has activated the SecurityManager. An attacker abuses the deleteQuietly method. The missing SecurityException hides this attempt, you're IDS can't alarm.
UseCase C: One utilizes the SecurityManager to test the system, to ensure every property (like file location) is set properly. The missing SecurityException does not support this UseCase.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a change in pull request #195: [IO-710] Utilize java.io.UncheckedIOException; rethrow SecurityException
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #195:
URL: https://github.com/apache/commons-io/pull/195#discussion_r566178127
##########
File path: src/main/java/org/apache/commons/io/FileUtils.java
##########
@@ -1232,12 +1233,16 @@ public static boolean deleteQuietly(final File file) {
if (file.isDirectory()) {
cleanDirectory(file);
}
+ } catch (final SecurityException sec) {
Review comment:
I am -1 to this as it defeats the simple purpose off the method.
As an aside, and what makes it worse is that the change is not even documented in the Javadoc.
I suppose the Javadoc could be improved to say that no `Exception`s are thrown as opposed to "exceptions".
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org