You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "gnodet (via GitHub)" <gi...@apache.org> on 2025/03/20 19:36:14 UTC
[PR] Check namespaces when reading XML models [maven]
gnodet opened a new pull request, #2170:
URL: https://github.com/apache/maven/pull/2170
* validate namespace consistency in XML
* make sure a valid namespace is used for modelVersion > 4.0.0
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8639] Check namespaces when reading XML models [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #2170:
URL: https://github.com/apache/maven/pull/2170#discussion_r2006965673
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultModelXmlFactory.java:
##########
@@ -48,8 +48,37 @@
@Singleton
public class DefaultModelXmlFactory implements ModelXmlFactory {
@Override
+ @Nonnull
public Model read(@Nonnull XmlReaderRequest request) throws XmlReaderException {
nonNull(request, "request");
+ Model model = doRead(request);
+ if (isModelVersionGreaterThan400(model)
+ && !model.getNamespaceUri().startsWith("http://maven.apache.org/POM/")) {
+ throw new XmlReaderException(
+ "Invalid namespace '" + model.getNamespaceUri() + "' for model version " + model.getModelVersion(),
+ null,
+ null);
+ }
+ return model;
+ }
+
+ private boolean isModelVersionGreaterThan400(Model model) {
+ String version = model.getModelVersion();
+ if (version == null) {
+ return false;
+ }
+ try {
+ String[] parts = version.split("\\.");
+ int major = Integer.parseInt(parts[0]);
+ int minor = parts.length > 1 ? Integer.parseInt(parts[1]) : 0;
+ return major > 4 || (major == 4 && minor > 0);
+ } catch (NumberFormatException | ArrayIndexOutOfBoundsException e) {
Review Comment:
Definitely, I've change the code to reflect that.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8639] Check namespaces when reading XML models [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #2170:
URL: https://github.com/apache/maven/pull/2170#discussion_r2007862028
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultModelXmlFactory.java:
##########
@@ -48,8 +48,37 @@
@Singleton
public class DefaultModelXmlFactory implements ModelXmlFactory {
@Override
+ @Nonnull
public Model read(@Nonnull XmlReaderRequest request) throws XmlReaderException {
nonNull(request, "request");
+ Model model = doRead(request);
+ if (isModelVersionGreaterThan400(model)
+ && !model.getNamespaceUri().startsWith("http://maven.apache.org/POM/")) {
+ throw new XmlReaderException(
+ "Invalid namespace '" + model.getNamespaceUri() + "' for model version " + model.getModelVersion(),
+ null,
+ null);
+ }
+ return model;
+ }
+
+ private boolean isModelVersionGreaterThan400(Model model) {
+ String version = model.getModelVersion();
+ if (version == null) {
+ return false;
+ }
+ try {
Review Comment:
That's not the point. The point is that the reader needs to support existing models which have no namespace, and that cannot be enforced because they are in Maven Central. So we can only enforce for model > 4.0.0.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8639] Check namespaces when reading XML models [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet merged PR #2170:
URL: https://github.com/apache/maven/pull/2170
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8639] Check namespaces when reading XML models [maven]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #2170:
URL: https://github.com/apache/maven/pull/2170#discussion_r2006925005
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultModelXmlFactory.java:
##########
@@ -48,8 +48,37 @@
@Singleton
public class DefaultModelXmlFactory implements ModelXmlFactory {
@Override
+ @Nonnull
public Model read(@Nonnull XmlReaderRequest request) throws XmlReaderException {
nonNull(request, "request");
+ Model model = doRead(request);
+ if (isModelVersionGreaterThan400(model)
+ && !model.getNamespaceUri().startsWith("http://maven.apache.org/POM/")) {
+ throw new XmlReaderException(
+ "Invalid namespace '" + model.getNamespaceUri() + "' for model version " + model.getModelVersion(),
+ null,
+ null);
+ }
+ return model;
+ }
+
+ private boolean isModelVersionGreaterThan400(Model model) {
+ String version = model.getModelVersion();
+ if (version == null) {
+ return false;
+ }
+ try {
+ String[] parts = version.split("\\.");
+ int major = Integer.parseInt(parts[0]);
+ int minor = parts.length > 1 ? Integer.parseInt(parts[1]) : 0;
+ return major > 4 || (major == 4 && minor > 0);
+ } catch (NumberFormatException | ArrayIndexOutOfBoundsException e) {
Review Comment:
Isn't IOOBE enough to catch?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8639] Check namespaces when reading XML models [maven]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #2170:
URL: https://github.com/apache/maven/pull/2170#discussion_r2007753105
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultModelXmlFactory.java:
##########
@@ -48,8 +48,37 @@
@Singleton
public class DefaultModelXmlFactory implements ModelXmlFactory {
@Override
+ @Nonnull
public Model read(@Nonnull XmlReaderRequest request) throws XmlReaderException {
nonNull(request, "request");
+ Model model = doRead(request);
+ if (isModelVersionGreaterThan400(model)
+ && !model.getNamespaceUri().startsWith("http://maven.apache.org/POM/")) {
Review Comment:
Will getNamespaceUri return an empty string (i,.e. not null) when there is no namespace?
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultModelXmlFactory.java:
##########
@@ -48,8 +48,37 @@
@Singleton
public class DefaultModelXmlFactory implements ModelXmlFactory {
@Override
+ @Nonnull
public Model read(@Nonnull XmlReaderRequest request) throws XmlReaderException {
nonNull(request, "request");
+ Model model = doRead(request);
+ if (isModelVersionGreaterThan400(model)
+ && !model.getNamespaceUri().startsWith("http://maven.apache.org/POM/")) {
+ throw new XmlReaderException(
+ "Invalid namespace '" + model.getNamespaceUri() + "' for model version " + model.getModelVersion(),
+ null,
+ null);
+ }
+ return model;
+ }
+
+ private boolean isModelVersionGreaterThan400(Model model) {
+ String version = model.getModelVersion();
+ if (version == null) {
+ return false;
+ }
+ try {
Review Comment:
There's no need to do all this parsing. The namespace URI should be a fixed, known string and that remains true in future versions. We should not change the namespace URI in new versions.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8639] Check namespaces when reading XML models [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #2170:
URL: https://github.com/apache/maven/pull/2170#discussion_r2007859875
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultModelXmlFactory.java:
##########
@@ -48,8 +48,37 @@
@Singleton
public class DefaultModelXmlFactory implements ModelXmlFactory {
@Override
+ @Nonnull
public Model read(@Nonnull XmlReaderRequest request) throws XmlReaderException {
nonNull(request, "request");
+ Model model = doRead(request);
+ if (isModelVersionGreaterThan400(model)
+ && !model.getNamespaceUri().startsWith("http://maven.apache.org/POM/")) {
Review Comment:
Yes, there are unit tests to support that.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8639] Check namespaces when reading XML models [maven]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #2170:
URL: https://github.com/apache/maven/pull/2170#discussion_r2008763401
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultModelXmlFactory.java:
##########
@@ -48,8 +48,37 @@
@Singleton
public class DefaultModelXmlFactory implements ModelXmlFactory {
@Override
+ @Nonnull
public Model read(@Nonnull XmlReaderRequest request) throws XmlReaderException {
nonNull(request, "request");
+ Model model = doRead(request);
+ if (isModelVersionGreaterThan400(model)
+ && !model.getNamespaceUri().startsWith("http://maven.apache.org/POM/")) {
Review Comment:
Here you can just check the namespace for full equality rather than using startsWith to check only the prefix.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultModelXmlFactory.java:
##########
@@ -48,8 +48,37 @@
@Singleton
public class DefaultModelXmlFactory implements ModelXmlFactory {
@Override
+ @Nonnull
public Model read(@Nonnull XmlReaderRequest request) throws XmlReaderException {
nonNull(request, "request");
+ Model model = doRead(request);
+ if (isModelVersionGreaterThan400(model)
+ && !model.getNamespaceUri().startsWith("http://maven.apache.org/POM/")) {
+ throw new XmlReaderException(
+ "Invalid namespace '" + model.getNamespaceUri() + "' for model version " + model.getModelVersion(),
+ null,
+ null);
+ }
+ return model;
+ }
+
+ private boolean isModelVersionGreaterThan400(Model model) {
+ String version = model.getModelVersion();
+ if (version == null) {
+ return false;
+ }
+ try {
Review Comment:
My mistake. I was confusing model version with the namespace URI
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8639] Check namespaces when reading XML models [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #2170:
URL: https://github.com/apache/maven/pull/2170#discussion_r2008796371
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultModelXmlFactory.java:
##########
@@ -48,8 +48,37 @@
@Singleton
public class DefaultModelXmlFactory implements ModelXmlFactory {
@Override
+ @Nonnull
public Model read(@Nonnull XmlReaderRequest request) throws XmlReaderException {
nonNull(request, "request");
+ Model model = doRead(request);
+ if (isModelVersionGreaterThan400(model)
+ && !model.getNamespaceUri().startsWith("http://maven.apache.org/POM/")) {
Review Comment:
Well, currently the code needs to accept both `http://maven.apache.org/POM/4.0.0` and `http://maven.apache.org/POM/4.1.0`, so I did not want enforce too much right now, whatever the outcome of that other discussion is.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org