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.

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

Add automatic module name #45

Merged
merged 3 commits into from
Jul 20, 2021
Merged

Add automatic module name #45

merged 3 commits into from
Jul 20, 2021

Conversation

casid
Copy link
Contributor

@casid casid commented Feb 7, 2021

Hey, I'm using OWASP Java Encoder in my template engine (https://jte.gg).

I'm currently adding Java 9 modules to jte and noticed that OWASP Java Encoder does not yet have an automatic module name. This would allow me to reference the encoder in my module.info.

What do you think?

PS: Thanks for this awesome library, the Writer based escape methods are blazingly fast!

@casid
Copy link
Contributor Author

casid commented Feb 7, 2021

Sorry for the rushed initial PR. I noticed that there are three modules, not just one. I reverted the initial change and added the automatic module name via apache felix bundle plugin. When building locally I now get the desired output for each module.

Please feel free to adjust the proposed module names as you see fit:

  • org.owasp.encoder
  • org.owasp.encoder.esapi
  • org.owasp.encoder.jsp

@jmanico
Copy link
Member

jmanico commented Feb 7, 2021 via email

@casid
Copy link
Contributor Author

casid commented Feb 7, 2021

Hey @jmanico, thank you for looking into it!

There's no rush on my end. I'm happy that I consolidated my packages and have now an automatic module name set for all jte modules.

Whenever this is merged and part of a owasp encoder release, I'll move forward.

@kwwall
Copy link
Contributor

kwwall commented Feb 7, 2021

I was under the (possibly mistaken) belief that the Java modules officially introduced in Java 9 and based on JSR 376 would not necessarily be compatible for use by earlier versions of Java. Java modules seems like a great idea but that belief is one reason that ESAPI has been avoiding them. If that would mean that those using Java 8 and earlier can no longer use the Java Encoder, I think that would be a show stopper. I've only read more about the concept but don't know many of the implementation details, which is why I am asking about it. Thanks.

@jmanico
Copy link
Member

jmanico commented Feb 7, 2021 via email

@kwwall
Copy link
Contributor

kwwall commented Feb 7, 2021 via email

@jmanico
Copy link
Member

jmanico commented Feb 7, 2021 via email

@casid
Copy link
Contributor Author

casid commented Feb 8, 2021

Hey @kwwall thank you for looking into it.

I think older Java versions will work fine. This PR adds an Automatic-Module-Name entry to each MANIFEST.MF file, nothing else.

For the core module:
Automatic-Module-Name: org.owasp.encoder

For the esapi module:
Automatic-Module-Name: org.owasp.encoder.esapi

For the jsp module:
Automatic-Module-Name: org.owasp.encoder.jsp

Java versions before 9 don't know and ignore that entry. Java versions 9 and greater however can use the automatic module name to reference those jars on the module path.

The only harm of this PR I can think of is, that I picked bad names for the modules, since it would be hard to change them later on, without potentially breaking builds who referenced them in their module info. E.g. I'm not sure if you'd like to have org.owasp.encoder or org.owasp.encoder.core for the core module.

There's some further information in these blog post:
http://branchandbound.net/blog/java/2017/12/automatic-module-name/
https://blog.frankel.ch/hard-look-state-java-modularization/

@kwwall
Copy link
Contributor

kwwall commented Feb 8, 2021

@casid - Thanks for the useful links. I agree, if all this does is result in adding entries for Automatic-Module-Name to MANIFEST.MF in the jar (which those linked posts seem to confirm), that it indeed should cause no problems. My original concern was that it would maybe do something to the .class files contained within the jar so that they would no longer work with versions of Java prior to Java 9. Good to see that is not the case.

As for the "bad names for the modules", that is something that needs to be chosen by the OWASP Java Encoder maintainers. Although, personally, I think org.owasp.encoder.jim_manicos_mother_dresses_him_funny would be a better name than than what you suggested, even if @jmanico doesn't agree. Fortunately, I don't get a vote. :-)

@jmanico
Copy link
Member

jmanico commented Feb 8, 2021 via email

@kwwall
Copy link
Contributor

kwwall commented Feb 8, 2021 via email

@jmanico jmanico merged commit 6320a6d into OWASP:main Jul 20, 2021
@casid casid deleted the patch-1 branch July 22, 2021 19:15
@casid
Copy link
Contributor Author

casid commented Jul 22, 2021

@jmanico thanks for the merge! Do you know when the next release of owasp-java-encoder is planned?

@jmanico
Copy link
Member

jmanico commented Jul 22, 2021 via email

@casid
Copy link
Contributor Author

casid commented Jul 23, 2021

No, it's not really pressing. I'll keep a watch on your repo and upgrade once the next release is out. Thanks! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants