-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
[JENKINS-63343] Validate element types in RobustCollectionConverter
in some cases
#9727
base: master
Are you sure you want to change the base?
Conversation
</hudson.util.RobustCollectionConverterTest_-Numbers> | ||
"""; | ||
var actual = (Numbers) new XStream2().fromXML(xmlContent); | ||
assertEquals(expected, actual); |
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.
And I guess OldDataMonitor
will report the mistyped element?
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.
I did not check it, but yes that is the hope.
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.
As of 61db49c the error messages have been improved and the tests log the warnings. Here is an example of the warning for a bad list element:
Sep 18, 2024 5:09:31 PM hudson.diagnosis.OldDataMonitor report
WARNING: could not read hudson.util.RobustCollectionConverterTest$Data@2cc3ad05 (and Jenkins did not start up)
com.thoughtworks.xstream.converters.ConversionException: Invalid type for collection element
---- Debugging information ----
message : Invalid type for collection element
cause-exception : java.lang.ClassCastException
cause-message : Cannot cast java.lang.String to java.lang.Integer
path : /hudson.util.RobustCollectionConverterTest$Data/numbers/string
line number : 5
-------------------------------
at hudson.util.RobustCollectionConverter.populateCollection(RobustCollectionConverter.java:120)
at com.thoughtworks.xstream.converters.collections.CollectionConverter.unmarshal(CollectionConverter.java:81)
at hudson.util.RobustCollectionConverter.unmarshal(RobustCollectionConverter.java:101)
at com.thoughtworks.xstream.core.TreeUnmarshaller.convert(TreeUnmarshaller.java:74)
at com.thoughtworks.xstream.core.AbstractReferenceUnmarshaller.convert(AbstractReferenceUnmarshaller.java:72)
at com.thoughtworks.xstream.core.TreeUnmarshaller.convertAnother(TreeUnmarshaller.java:68)
at hudson.util.RobustReflectionConverter.unmarshalField(RobustReflectionConverter.java:461)
at hudson.util.RobustReflectionConverter.doUnmarshal(RobustReflectionConverter.java:350)
at hudson.util.RobustReflectionConverter.unmarshal(RobustReflectionConverter.java:289)
at com.thoughtworks.xstream.core.TreeUnmarshaller.convert(TreeUnmarshaller.java:74)
at com.thoughtworks.xstream.core.AbstractReferenceUnmarshaller.convert(AbstractReferenceUnmarshaller.java:72)
at com.thoughtworks.xstream.core.TreeUnmarshaller.convertAnother(TreeUnmarshaller.java:68)
at com.thoughtworks.xstream.core.TreeUnmarshaller.convertAnother(TreeUnmarshaller.java:52)
at com.thoughtworks.xstream.core.TreeUnmarshaller.start(TreeUnmarshaller.java:136)
at com.thoughtworks.xstream.core.AbstractTreeMarshallingStrategy.unmarshal(AbstractTreeMarshallingStrategy.java:32)
at com.thoughtworks.xstream.XStream.unmarshal(XStream.java:1464)
at hudson.util.XStream2.unmarshal(XStream2.java:230)
at hudson.util.XStream2.unmarshal(XStream2.java:201)
at com.thoughtworks.xstream.XStream.unmarshal(XStream.java:1441)
at com.thoughtworks.xstream.XStream.fromXML(XStream.java:1321)
at com.thoughtworks.xstream.XStream.fromXML(XStream.java:1312)
at hudson.util.RobustCollectionConverterTest.checkElementTypes(RobustCollectionConverterTest.java:207)
... surefire ...
at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:495)
Caused by: java.lang.ClassCastException: Cannot cast java.lang.String to java.lang.Integer
at java.base/java.lang.Class.cast(Class.java:3889)
at hudson.util.RobustCollectionConverter.populateCollection(RobustCollectionConverter.java:114)
... 65 more
Here is an example for a bad map value:
WARNING: could not read hudson.util.RobustMapConverterTest$Data@2a5b3fee (and Jenkins did not start up)
com.thoughtworks.xstream.converters.ConversionException: Invalid type for map entry key/value
---- Debugging information ----
message : Invalid type for map entry key/value
cause-exception : java.lang.ClassCastException
cause-message : Cannot cast java.lang.Integer to java.lang.String
path : /hudson.util.RobustMapConverterTest$Data/map/entry[2]/int
line number : 9
-------------------------------
at hudson.util.RobustMapConverter.read(RobustMapConverter.java:114)
at hudson.util.RobustMapConverter.putCurrentEntryIntoMap(RobustMapConverter.java:81)
at com.thoughtworks.xstream.converters.collections.MapConverter.populateMap(MapConverter.java:99)
at com.thoughtworks.xstream.converters.collections.MapConverter.populateMap(MapConverter.java:93)
at com.thoughtworks.xstream.converters.collections.MapConverter.unmarshal(MapConverter.java:88)
at com.thoughtworks.xstream.core.TreeUnmarshaller.convert(TreeUnmarshaller.java:74)
at com.thoughtworks.xstream.core.AbstractReferenceUnmarshaller.convert(AbstractReferenceUnmarshaller.java:72)
at com.thoughtworks.xstream.core.TreeUnmarshaller.convertAnother(TreeUnmarshaller.java:68)
at hudson.util.RobustReflectionConverter.unmarshalField(RobustReflectionConverter.java:461)
at hudson.util.RobustReflectionConverter.doUnmarshal(RobustReflectionConverter.java:350)
at hudson.util.RobustReflectionConverter.unmarshal(RobustReflectionConverter.java:289)
at com.thoughtworks.xstream.core.TreeUnmarshaller.convert(TreeUnmarshaller.java:74)
at com.thoughtworks.xstream.core.AbstractReferenceUnmarshaller.convert(AbstractReferenceUnmarshaller.java:72)
at com.thoughtworks.xstream.core.TreeUnmarshaller.convertAnother(TreeUnmarshaller.java:68)
at com.thoughtworks.xstream.core.TreeUnmarshaller.convertAnother(TreeUnmarshaller.java:52)
at com.thoughtworks.xstream.core.TreeUnmarshaller.start(TreeUnmarshaller.java:136)
at com.thoughtworks.xstream.core.AbstractTreeMarshallingStrategy.unmarshal(AbstractTreeMarshallingStrategy.java:32)
at com.thoughtworks.xstream.XStream.unmarshal(XStream.java:1464)
at hudson.util.XStream2.unmarshal(XStream2.java:230)
at hudson.util.XStream2.unmarshal(XStream2.java:201)
at com.thoughtworks.xstream.XStream.unmarshal(XStream.java:1441)
at com.thoughtworks.xstream.XStream.fromXML(XStream.java:1321)
at com.thoughtworks.xstream.XStream.fromXML(XStream.java:1312)
at hudson.util.RobustMapConverterTest.robustAgainstInvalidValueType(RobustMapConverterTest.java:244)
... surefire ...
at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:495)
Caused by: java.lang.ClassCastException: Cannot cast java.lang.Integer to java.lang.String
at java.base/java.lang.Class.cast(Class.java:3889)
at hudson.util.RobustMapConverter.read(RobustMapConverter.java:110)
... 67 more
// TODO: Is there a better way to make something like this work? Does it break any custom converters? | ||
if (converter == null && new RobustCollectionConverter(mapper, reflectionProvider).canConvert(type)) { | ||
converter = new RobustCollectionConverter(mapper, reflectionProvider, field.getGenericType()); | ||
} |
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.
In particular I am wondering about things like NodeList and AxisList which extend ArrayList
. They might be ok because of the exact equality comparisons here, but I am not sure. I guess also that those classes don't support any of the desired recoverability features in RobustCollectionConverter
, but that shouldn't be affected by my PR.
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.
I confirmed that NodeList
is unaffected by this change - its custom converter continues to be used and its deserialization is not robust against invalid values.
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.
Looking at #342, I think we only expect getLocalConverter
to matter for fields that have the @XStreamConverter
annotation, which seems to be more or less unused based on this search, and also none of those annotations are for collections or maps.
@@ -451,6 +451,13 @@ private boolean fieldDefinedInClass(Object result, String attrName) { | |||
|
|||
protected Object unmarshalField(final UnmarshallingContext context, final Object result, Class type, Field field) { | |||
Converter converter = mapper.getLocalConverter(field.getDeclaringClass(), field.getName()); | |||
if (converter == null) { | |||
if (new RobustCollectionConverter(mapper, reflectionProvider).canConvert(type)) { |
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.
It's a bit awkward to recreate this for every single field being unmarshalled, should we keep an instance as a local field just for these canConvert
calls?
@@ -451,6 +451,13 @@ private boolean fieldDefinedInClass(Object result, String attrName) { | |||
|
|||
protected Object unmarshalField(final UnmarshallingContext context, final Object result, Class type, Field field) { | |||
Converter converter = mapper.getLocalConverter(field.getDeclaringClass(), field.getName()); | |||
if (converter == null) { |
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.
See #9727 (comment) for discussion about cases where converter
is not null. In short, I don't think we care.
I am not sure though whether this method covers all of the cases we care about. It seems like we may need similar changes related to "implicit collections". I will look into that a bit.
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.
Specifically, prior to this line, I think we should look up the field and then add code comparable to what we added to RobustCollectionConverter
to validate the value type:
collection.add(value); |
Needs to be investigated with a test case.
See JENKINS-63343. I am just filing a draft PR to see if it breaks tests and as a reference in case anyone looks at the ticket in the future. I am not really sure about this approach, and it seems like maybe there is no good way to solve this issue in general given that XStream APIs use
java.lang.Class
rather thanjava.lang.reflect.Type
.Testing done
Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
Desired reviewers
@mention
Before the changes are marked as
ready-for-merge
:Maintainer checklist