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

[#286] disable SecurityManager starting with Java 21 #287

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[#286] disable SecurityManager starting with Java 21 #287

wants to merge 2 commits into from

Conversation

bmarwell
Copy link
Contributor

@bmarwell bmarwell commented Oct 19, 2023 •
edited
Loading

fixes #286

@bmarwell bmarwell mentioned this pull request Oct 20, 2023
Closed
@bmarwell bmarwell marked this pull request as ready for review October 21, 2023 11:43
@bmarwell
Copy link
Contributor Author

You can ignore the Maven warning for now, it will go away with the next maven-compiler-plugin release: apache/maven-compiler-plugin#202

@bmarwell
Copy link
Contributor Author

@keeganwitt please review (cannot tag you as a reviewer)

@bmarwell bmarwell mentioned this pull request Oct 23, 2023
Closed
@keeganwitt
Copy link
Member

Do you think we should include something about -Djava.security.manager=allow? for Java 18, 19, 20 users? I thought it might be useful, but then started second-guessing it since those aren't LTS releases.

keeganwitt
keeganwitt reviewed Oct 23, 2023
View reviewed changes
pom.xml Show resolved Hide resolved
@keeganwitt
Copy link
Member

keeganwitt commented Oct 23, 2023 •
edited
Loading

Do you think we should include something about -Djava.security.manager=allow? for Java 18, 19, 20 users? I thought it might be useful, but then started second-guessing it since those aren't LTS releases.

Actually this applies to Java 21 users as well. I don't think it's been removed (yet), just turned off by default. This being the case, should we really be turning it off entirely?

@bmarwell
Copy link
Contributor Author

Do you think we should include something about -Djava.security.manager=allow? for Java 18, 19, 20 users? I thought it might be useful, but then started second-guessing it since those aren't LTS releases.

Actually this applies to Java 21 users as well. I don't think it's been removed (yet), just turned off by default. This being the case, should we really be turning it off entirely?

That's true.
Other things we could do:

  • guess any future version (Java 24, Java 30, whatever) and change all java21 occurrences to any future version
  • For now, just print a warning that the feature will be removed (regardless of the -XX options of the user)
  • and/or add the META-INF/versions/javaXX code path later

@bmarwell
Copy link
Contributor Author

@keeganwitt this pr is flawed, it should be META-INF/versions/21, not ../java21. Will update the PR.

@keeganwitt
Copy link
Member

For now, I'm gonna proceed with the warning. But this PR will likely prove useful down the road.

@keeganwitt
Merge remote-tracking branch 'origin/master' into #286_disable_securi…
9f0e5ce 
…ty_manager_java21

# Conflicts:
#	pom.xml
#	src/main/java/org/codehaus/gmavenplus/mojo/ShellMojo.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keeganwitt keeganwitt keeganwitt left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove Security Manager (for Java 21+)
2 participants
@bmarwell @keeganwitt