TRUNK-6188 Upgrade xstream and migrate to whitelisting #4377
Conversation
|
Please let me handle the merge and back-porting after reviews are done. |
| List<GlobalProperty> gpTypes = getGlobalPropertiesBySuffix( | ||
| AdministrationService.GP_SUFFIX_SERIALIZER_WHITELIST_TYPES); | ||
| List<String> whitelistTypes = new ArrayList<>(); | ||
| whitelistTypes.add("org.openmrs.**"); |
There was a problem hiding this comment.
So, one of the things we'd ideally want to prevent is serializing or deserializing org.openmrs.util.OpenmrsClassLoader.
What would you think of a rule like:
xstream.allowTypeHierachy(OpenmrsObject.class);
// may be others, but these are the common exceptions to the above:
xstream.allowTypes(LayoutTemplate.class, ComplexData.class);
There was a problem hiding this comment.
Yeah, I was thinking of any classes that may be risky in org.openmrs, but I didn't come up with an obvious OpenmrsClassLoader... It seems that it may be better indeed to be less permissive and do as you suggested. I'll need to check what is needed for the Reporting module so that most obvious use cases are covered out of the box.
There was a problem hiding this comment.
Generally looks good to me @rkorytkowski . Made a few comments.
| * @param adminService | ||
| */ | ||
| public static void setupXStreamSecurity(XStream xstream, AdministrationService adminService) { | ||
| xstream.allowTypesByWildcard(new String[] {"org.openmrs.**"}); |
There was a problem hiding this comment.
Why add this here, if we are also adding this as a hard-coded value within the getSerializerWhitelistTypes method? Can we just add the fixed, hard-coded values in one place?
There was a problem hiding this comment.
It's duplicated in cases when adminService is not passed so we still have some basic setup. I'll move that line to the else statement just to be explicit with the intention.
|
|
||
| // cached xstream object | ||
| public XStream xstream = null; | ||
| private volatile XStream xstream; |
There was a problem hiding this comment.
Can we rename this to just clearly distinguish between this private member variable and the xstream argument passed into the static methods below?
There was a problem hiding this comment.
Renamed arguments in methods instead.
d643469
to
e0fe381
Compare
| List<Class<?>> types = Arrays.asList(OpenmrsObject.class, OpenmrsMetadata.class, OpenmrsData.class, | ||
| CustomDatatype.class, SingleCustomValue.class, CustomValueDescriptor.class, Customizable.class, | ||
| LayoutTemplate.class, LayoutSupport.class, ComplexData.class, PresentationMessage.class, | ||
| PersonMergeLogData.class); |
There was a problem hiding this comment.
@ibacher updated to use a hierarchy of common types.
|
tl;dr our action detected no activity on this PR and will close it in 30 days if the stale label is not removed. OpenMRS welcomes your contribution! It means a lot to us that you want to contribute to equity in healthcare! This PR has not seen any activity in the last 5 months. That is why we wanted to check whether you are still working on it or need assistance from our side. If you do not have time to continue the work or have moved on you don’t need to do anything. We will automatically close the PR in 30 days. We hope to see you back soon :) |
|
tl;dr our action detected no activity on this PR and will close it in 30 days if the stale label is not removed. OpenMRS welcomes your contribution! It means a lot to us that you want to contribute to equity in healthcare! This PR has not seen any activity in the last 5 months. That is why we wanted to check whether you are still working on it or need assistance from our side. If you do not have time to continue the work or have moved on you don’t need to do anything. We will automatically close the PR in 30 days. We hope to see you back soon :) |
|
@ibacher do we have a strategy for this? Because after merging this, the reference application fails with errors like you can see here: qa-refapp.openmrs.org |
|
@dkayiwa That was definitely a risk with this PR, but getting on XStream whitelisting seemed like a must-have. The error on the qa-refapp seems to be this: So the The mechanism this PR has for modules that need to extend the supported types is to add a GP like the one in this test that ends with We can, however, find other places in code we're using it via this search. It looks like the other module that will need work is |
|
We need to revert this one. I'm sorry I haven't marked it as not yet ready for merging explicitly. I didn't mean it be merged before modules are adjusted. I'll get back to this in 2 weeks and fix what's needed. |
Description of what I changed
Issue I worked on
see https://issues.openmrs.org/browse/TRUNK-6188
Checklist: I completed these to help reviewers :)
My IDE is configured to follow the code style of this project.
No? Unsure? -> configure your IDE, format the code and add the changes with
git add . && git commit --amendI have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)
No? -> write tests and add them to this commit
git add . && git commit --amendI ran
mvn clean packageright before creating this pull request andadded all formatting changes to my commit.
No? -> execute above command
All new and existing tests passed.
No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.
No? Unsure? -> execute command
git pull --rebase upstream master