Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove SecurityManager #248

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Remove SecurityManager #248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 0 additions & 9 deletions src/main/java/org/codehaus/gmavenplus/mojo/AbstractToolsMojo.java
View file
Open in desktop
Expand Up @@ -70,15 +70,6 @@ public abstract class AbstractToolsMojo extends AbstractGroovyMojo {
@Parameter
protected Properties properties = new Properties();

/**
* Whether to allow System.exit() to be used. Should not be set to <code>false</code> when using parallel
* execution, as it isn't thread-safe.
*
* @since 1.2
*/
@Parameter(defaultValue = "true")
protected boolean allowSystemExits;

Comment on lines -73 to -81
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this would be a breaking change, keep it for a while and just print a warning for now. This way, users can fix their scripts.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, that's why I hadn't merged this -- Java isn't forcing me to yet.

Another option here might be to wrap the usage in if (Version.parseFromString(System.getProperty("java.version")).compareTo(new Version(24)) >=1) and print a warning if using a newer Java version that won't support it and ignore the option in that case. And in the meantime, print a warning and deprecate the parameter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure when they're planning on removing this (I used 24 as an example). I don't think they've decided when that will be (or at least I can't find any documentation about it).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it gets removed. The warning is there since 17 or 18.

Docs: https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/SecurityManager.html

@Deprecated(since="17",
            forRemoval=true)
public class SecurityManager
extends Object
Deprecated, for removal: This API element is subject to removal in a future version.
The Security Manager is deprecated and subject to removal in a future release. There is no replacement for the Security Manager. See JEP 411 for discussion and alternatives.

There's more
https://openjdk.org/jeps/411

Description

In Java 17, we will:

Deprecate, for removal, most Security Manager related classes and methods.

Issue a warning message at startup if the Security Manager is enabled on the command line.

Issue a warning message at run time if a Java application or library installs a Security Manager dynamically.

In Java 18, we will prevent a Java application or library from dynamically installing a Security Manager unless the end user has explicitly opted to allow it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option here might be to wrap the usage in if (Version.parseFromString(System.getProperty("java.version")).compareTo(new Version(24)) >=1)

But that would still print a ClassNotFoundException on 24 or so. Unless:

  1. You use reflection. A lot.
  2. Create a multi release jar
  3. Drop it all together and print a warning

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of using reflection

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather put that code into its own class.
Then, for Java 21+, create a Multi Release Jar which implements this as a no-op.

Easier to maintain, faster than reflection.

But still, why not start with a warning? 🤷🏻‍♂️ Ship early, ship often.

/**
* Whether to bind each property to a separate variable (otherwise binds properties to a single 'properties' variable).
*
Expand Down
10 changes: 0 additions & 10 deletions src/main/java/org/codehaus/gmavenplus/mojo/ConsoleMojo.java
View file
Open in desktop
Expand Up @@ -22,7 +22,6 @@
import org.apache.maven.plugins.annotations.Mojo;
import org.apache.maven.plugins.annotations.Parameter;
import org.apache.maven.plugins.annotations.ResolutionScope;
import org.codehaus.gmavenplus.util.NoExitSecurityManager;

import java.io.File;
import java.lang.reflect.InvocationTargetException;
Expand Down Expand Up @@ -86,12 +85,7 @@ public void execute() throws MojoExecutionException, MojoFailureException {
}

if (groovyVersionSupportsAction()) {
final SecurityManager sm = System.getSecurityManager();
try {
if (!allowSystemExits) {
System.setSecurityManager(new NoExitSecurityManager());
}
Comment on lines -91 to -93
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Print a warning in case this is false.


// get classes we need with reflection
Class<?> consoleClass;
try {
Expand Down Expand Up @@ -127,10 +121,6 @@ public void execute() throws MojoExecutionException, MojoFailureException {
throw new MojoExecutionException("Unable to access a method on a Groovy class from classpath.", e);
} catch (InstantiationException e) {
throw new MojoExecutionException("Error occurred while instantiating a Groovy class from classpath.", e);
} finally {
if (!allowSystemExits) {
System.setSecurityManager(sm);
}
}
} else {
getLog().error("Your Groovy version (" + classWrangler.getGroovyVersionString() + ") doesn't support running a console. The minimum version of Groovy required is " + minGroovyVersion + ". Skipping console startup.");
Expand Down
10 changes: 0 additions & 10 deletions src/main/java/org/codehaus/gmavenplus/mojo/ExecuteMojo.java
View file
Open in desktop
Expand Up @@ -23,7 +23,6 @@
import org.apache.maven.plugins.annotations.ResolutionScope;
import org.codehaus.gmavenplus.model.internal.Version;
import org.codehaus.gmavenplus.util.FileUtils;
import org.codehaus.gmavenplus.util.NoExitSecurityManager;

import java.io.BufferedReader;
import java.io.File;
Expand Down Expand Up @@ -137,12 +136,7 @@ protected synchronized void doExecute() throws MojoExecutionException {
}

if (groovyVersionSupportsAction()) {
final SecurityManager sm = System.getSecurityManager();
try {
if (!allowSystemExits) {
System.setSecurityManager(new NoExitSecurityManager());
}
Comment on lines -142 to -144
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, keep var and print warning (to be removed in…)


// get classes we need with reflection
Class<?> groovyShellClass = classWrangler.getClass("groovy.lang.GroovyShell");

Expand All @@ -159,10 +153,6 @@ protected synchronized void doExecute() throws MojoExecutionException {
throw new MojoExecutionException("Error occurred while instantiating a Groovy class from classpath.", e);
} catch (IllegalAccessException e) {
throw new MojoExecutionException("Unable to access a method on a Groovy class from classpath.", e);
} finally {
if (!allowSystemExits) {
System.setSecurityManager(sm);
}
}
} else {
getLog().error("Your Groovy version (" + classWrangler.getGroovyVersionString() + ") doesn't support script execution. The minimum version of Groovy required is " + minGroovyVersion + ". Skipping script execution.");
Expand Down
10 changes: 0 additions & 10 deletions src/main/java/org/codehaus/gmavenplus/mojo/ShellMojo.java
View file
Open in desktop
Expand Up @@ -22,7 +22,6 @@
import org.apache.maven.plugins.annotations.Parameter;
import org.apache.maven.plugins.annotations.ResolutionScope;
import org.codehaus.gmavenplus.model.internal.Version;
import org.codehaus.gmavenplus.util.NoExitSecurityManager;

import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
Expand Down Expand Up @@ -91,12 +90,7 @@ public void execute() throws MojoExecutionException {
}

if (groovyVersionSupportsAction()) {
final SecurityManager sm = System.getSecurityManager();
try {
if (!allowSystemExits) {
System.setSecurityManager(new NoExitSecurityManager());
}
Comment on lines -96 to -98
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above


// get classes we need with reflection
Class<?> shellClass = classWrangler.getClass(groovyAtLeast(GROOVY_4_0_0_ALPHA1) ? "org.apache.groovy.groovysh.Groovysh" : "org.codehaus.groovy.tools.shell.Groovysh");
Class<?> bindingClass = classWrangler.getClass("groovy.lang.Binding");
Expand All @@ -121,10 +115,6 @@ public void execute() throws MojoExecutionException {
throw new MojoExecutionException("Unable to access a method on a Groovy class from classpath.", e);
} catch (InstantiationException e) {
throw new MojoExecutionException("Error occurred while instantiating a Groovy class from classpath.", e);
} finally {
if (!allowSystemExits) {
System.setSecurityManager(sm);
}
}
} else {
getLog().error("Your Groovy version (" + classWrangler.getGroovyVersionString() + ") doesn't support running a shell. The minimum version of Groovy required is " + minGroovyVersion + ". Skipping shell startup.");
Expand Down
71 changes: 0 additions & 71 deletions src/main/java/org/codehaus/gmavenplus/util/NoExitSecurityManager.java
View file
Open in desktop

This file was deleted.