Skip to content

TRUNK-6218: Enable XStream whitelisting on OpenMRS Core 2.7.0 and after #253

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 22, 2024
Merged

TRUNK-6218: Enable XStream whitelisting on OpenMRS Core 2.7.0 and after #253

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions api/src/main/java/org/openmrs/module/reporting/serializer/ReportingSerializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import org.openmrs.api.APIException;
import org.openmrs.api.context.Context;
import org.openmrs.module.VersionComparator;
import org.openmrs.module.serialization.xstream.XStreamShortSerializer;
import org.openmrs.module.serialization.xstream.mapper.CGLibMapper;
import org.openmrs.module.serialization.xstream.mapper.HibernateCollectionMapper;
Expand All @@ -32,6 +33,7 @@
import com.thoughtworks.xstream.io.xml.DomDriver;
import com.thoughtworks.xstream.mapper.Mapper;
import com.thoughtworks.xstream.mapper.MapperWrapper;
import org.openmrs.util.OpenmrsConstants;


public class ReportingSerializer extends XStreamShortSerializer {
Expand Down Expand Up @@ -85,6 +87,11 @@ public Object unmarshal(HierarchicalStreamReader reader, Object root) {
xstream.registerConverter(new IndicatorConverter(mapper, converterLookup));

xstream.registerConverter(new ReportDefinitionConverter(mapper, converterLookup));

// Only setup XStreamSecurity only on versions that are after 2.7.0
if (new VersionComparator().compare(OpenmrsConstants.OPENMRS_VERSION, "2.7.0") >= 0) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkayiwa do we need this check now?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you planning to run the reporting module on only platform 2.7? 😊

Copy link
Member Author

@wikumChamith wikumChamith Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check was to only set up XStream security on 2.7.0 and higher.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do not have that check, you are going to end up with versions of openmrs core before 2.7 freezing on startup. Isn't that what you were fixing here? openmrs/openmrs-core@f0910d4

Or has your fix now confused you? 😊

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we did that fix we don't need this check now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about those who are not yet ready to upgrade their platform versions?

setupXStreamSecurity(xstream);
}
}

@Override
Expand Down Expand Up @@ -115,4 +122,22 @@ public void serializeToStream(Object object, OutputStream out) {
throw new IllegalStateException("Unsupported encoding", e);
}
}

private void setupXStreamSecurity(XStream xstream) throws SerializationException {
try {
Copy link
Member

@dkayiwa dkayiwa Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that instead of checking for the running version of openmrs, it is this method implementation that needs to be fixed regardless of which openmrs version we are running.

Copy link
Member Author

@wikumChamith wikumChamith Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkayiwa there is an issue on the core that caused it not to throw the API exception. I made a fix for that: openmrs/openmrs-core#4843

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not necessarily swallow the exception, but results into a situation where, while refreshing the spring application context, getRegisteredComponent does not find a bean, hence throwing an exception whose constructor calls Context.getMessageSourceService()` resulting into a deadlock here: https://github.com/openmrs/openmrs-core/blob/2.6.9/api/src/main/java/org/openmrs/api/context/ServiceContext.java#L681

SimpleXStreamSerializer serializer = Context.getRegisteredComponent("simpleXStreamSerializer", SimpleXStreamSerializer.class);
if (serializer != null) {
try {
Method method = serializer.getClass().getMethod("initXStream", XStream.class);
method.invoke(serializer, xstream);
}
catch (Exception ex) {
throw new SerializationException("Failed to set up XStream Security", ex);
}
}
}
catch (APIException ex) {
//Ignore APIException("Error during getting registered component) for platform versions below 2.7.0
}
}
}
Loading