-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Disallow arbitrary namespaces in Maven and Metadata readers #11185
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avoid abbreviations
@Test | ||
void acceptsPom41() throws Exception { | ||
MavenStaxReader reader = new MavenStaxReader(); | ||
Model m = reader.read(new StringReader(POM_41), /*strict*/ true, NO_INPUT_SOURCE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m --> model
@Test | ||
void acceptsPomWithoutNamespace() throws Exception { | ||
MavenStaxReader reader = new MavenStaxReader(); | ||
Model m = reader.read(new StringReader(POM_NO_NS), true, NO_INPUT_SOURCE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m --> model
XMLStreamException ex = assertThrows( | ||
XMLStreamException.class, () -> reader.read(new StringReader(POM_BAD_NS), true, NO_INPUT_SOURCE)); | ||
// sanity check: message mentions unrecognized namespace | ||
assertTrue(ex.getMessage().toLowerCase().contains("unrecognized pom namespace")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Locale.ROOT
@Test | ||
void acceptsMetadata110() throws Exception { | ||
MetadataStaxReader reader = new MetadataStaxReader(); | ||
Metadata md = reader.read(new StringReader(META_110), /*strict*/ true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
md --> metadata
src/mdo/reader-stax.vm
Outdated
} | ||
|
||
// Enforce root namespace per model (strict mode). | ||
String rootNs = parser.getNamespaceURI(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rootNamespace
src/mdo/reader-stax.vm
Outdated
|
||
// Enforce root namespace per model (strict mode). | ||
String rootNs = parser.getNamespaceURI(); | ||
boolean hasNs = rootNs != null && !rootNs.isEmpty(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hasNamespace
25d4fb8
to
c5bbb38
Compare
src/mdo/reader-stax.vm
Outdated
* any. | ||
* @return ${root.name} | ||
*/ | ||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the formatting of this file, this completely breaks the format of the generated java file.
src/mdo/reader-stax.vm
Outdated
if (strict && hasNs) { | ||
if ("project".equals("${rootTag}")) { | ||
// Accept any official POM namespace version (e.g., 4.0.0, 4.1.0) | ||
if (!rootNs.startsWith("http://maven.apache.org/POM/")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These hardcoded values are wrong. We generate much more readers than just those two: settings, lifecycles, extensions, plugin, toolchains...
In addition, accepting metadata namespace when reading a POM is just plain wrong.
The way to go would be to define a variable for the template in the POM that generates the writer and only accept that one.
Another thing to keep in mind is that the plugin configuration in the POM can actually contain external namespaces and this is valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An example of such a variable is ${packageToolV4}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoding the namespace is wrong.
Accept POM 4.1.0 and METADATA 1.1.0 plus no-namespace, reject others Add JUnit tests for valid and invalid namespaces
c8fe7a7
to
da106d1
Compare
@gnodet please do another pass |
final boolean hasNs = rootNs != null && !rootNs.isEmpty(); | ||
|
||
if ("project".equals("${rootTag}")) { | ||
if (hasNs && !rootNs.startsWith("http://maven.apache.org/POM/")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wrong. We have generators to generate specific readers, not a generic one which covers all use cases. So it's wrong to put hardcoded values for one or two models in the output of all readers.
The way to go is to retrieve the namespace using modello. The helper
needs to be enhanced:
https://github.com/codehaus-plexus/modello/blob/ca30aba14346bf0e183efd2c442f061024c40111/modello-plugins/modello-plugin-velocity/src/main/java/org/codehaus/modello/plugin/velocity/Helper.java
It needs a new method xmlModelMetadata
which would return the XmlModelMetadata
from which we can get the target namespace.
Once modello is fixed, we can update the velocity templates to leverage that using Helper.xmlModelMetadata(model).namespace
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, I think we need to enhance modello to provide access to the target namespace using $Helper.xmlModelMetadata(model).namespace
.
Accept POM 4.1.0 and METADATA 1.1.0 plus no-namespace, reject others Add JUnit tests for valid and invalid namespaces
Following this checklist to help us incorporate your
contribution quickly and easily:
Note that commits might be squashed by a maintainer on merge.
This may not always be possible but is a best-practice.
mvn verify
to make sure basic checks pass.A more thorough check will be performed on your pull request automatically.
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.