Skip to content

Commit 1fc1234

Browse files
Ezequiel-ValenciaEzequiel-Valencia
authored
Merge pull request #1531 from virtualcell/clean-xml
Clean XML
2 parents f3aed35 + 6abc96d commit 1fc1234

10 files changed

Lines changed: 93 additions & 14 deletions

File tree

‎python-restclient/docs/MathModelResourceApi.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -352,6 +352,7 @@ Name | Type | Description | Notes
352352
**200** | OK | - |
353353
**401** | Not Authorized | - |
354354
**403** | Not Allowed | - |
355+
**422** | Unprocessable content submitted | - |
355356
**500** | Data Access Exception | - |
356357

357358
[[Back to top]](#) [[Back to API list]](../README.md#documentation-for-api-endpoints) [[Back to Model list]](../README.md#documentation-for-models) [[Back to README]](../README.md)

‎python-restclient/vcell_client/api/math_model_resource_api.py‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1179,6 +1179,7 @@ def save_math_model(
11791179
'200': "str",
11801180
'401': "VCellHTTPError",
11811181
'403': None,
1182+
'422': "VCellHTTPError",
11821183
'500': "VCellHTTPError"
11831184

11841185
}
@@ -1257,6 +1258,7 @@ def save_math_model_with_http_info(
12571258
'200': "str",
12581259
'401': "VCellHTTPError",
12591260
'403': None,
1261+
'422': "VCellHTTPError",
12601262
'500': "VCellHTTPError"
12611263

12621264
}
@@ -1335,6 +1337,7 @@ def save_math_model_without_preload_content(
13351337
'200': "str",
13361338
'401': "VCellHTTPError",
13371339
'403': None,
1340+
'422': "VCellHTTPError",
13381341
'500': "VCellHTTPError"
13391342

13401343
}

‎tools/openapi.yaml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -907,6 +907,12 @@ paths:
907907
$ref: '#/components/schemas/VCellHTTPError'
908908
"403":
909909
description: Not Allowed
910+
"422":
911+
description: Unprocessable content submitted
912+
content:
913+
application/json:
914+
schema:
915+
$ref: '#/components/schemas/VCellHTTPError'
910916
"500":
911917
description: Data Access Exception
912918
content:

‎vcell-core/src/main/java/cbit/util/xml/XmlUtil.java‎

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -127,17 +127,34 @@ public static void writeXMLStringToFile(String xmlString, String filename, boole
127127
}
128128

129129

130+
/**
131+
* Throws exception if the xml contains entities not allowed.
132+
* @param xml
133+
* @throws IOException
134+
* @throws JDOMException
135+
*/
136+
public static void vetXMLForMaliciousEntities(String xml) throws IOException, JDOMException {
137+
SAXBuilder builder = new SAXBuilder();
138+
GenericXMLErrorHandler errorHandler = new GenericXMLErrorHandler();
139+
140+
builder.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); // https://semgrep.dev/docs/cheat-sheets/java-xxe
141+
builder.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
142+
143+
builder.build(new StringReader(xml));
144+
}
145+
146+
130147
//useful for the translators.
131148
public static Document readXML(Reader reader, String schemaLocation, String parserClass, String schemaLocationPropName) throws RuntimeException {
132149

133-
SAXBuilder builder = null;
150+
SAXBuilder builder = null;
134151
Document sDoc = null;
135152
GenericXMLErrorHandler errorHandler = new GenericXMLErrorHandler();
136153
try {
137154
if (schemaLocation != null && schemaLocation.length() > 0) { //ignores the parserClass, since xerces is the only validating parser we have
138155
builder = new SAXBuilder("org.apache.xerces.parsers.SAXParser", true);
139-
builder.setFeature("https://xml.org/sax/features/validation", true);
140-
builder.setFeature("https://apache.org/xml/features/validation/schema", true);
156+
builder.setFeature("http://xml.org/sax/features/validation", true);
157+
builder.setFeature("http://apache.org/xml/features/validation/schema", true);
141158
builder.setErrorHandler(errorHandler);
142159
builder.setProperty(schemaLocationPropName, schemaLocation);
143160
} else { //ignore schemaLocationPropName
@@ -149,6 +166,7 @@ public static Document readXML(Reader reader, String schemaLocation, String pars
149166
builder.setErrorHandler(errorHandler);
150167
}
151168
builder.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); // https://semgrep.dev/docs/cheat-sheets/java-xxe
169+
builder.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
152170
sDoc = builder.build(reader);
153171
// ----- Element root = null;
154172
// ----- root = sDoc.getRootElement();
@@ -175,6 +193,8 @@ public static Document readXML(File file) throws RuntimeException {
175193
Document sDoc = null;
176194
GenericXMLErrorHandler errorHandler = new GenericXMLErrorHandler();
177195
builder.setErrorHandler(errorHandler);
196+
builder.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); // https://semgrep.dev/docs/cheat-sheets/java-xxe
197+
builder.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
178198
try {
179199
sDoc = builder.build(file);
180200
// Element root = null;
@@ -202,6 +222,8 @@ public static Document readXML(InputStream inputStream) throws RuntimeException
202222
Document sDoc = null;
203223
GenericXMLErrorHandler errorHandler = new GenericXMLErrorHandler();
204224
builder.setErrorHandler(errorHandler);
225+
builder.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); // https://semgrep.dev/docs/cheat-sheets/java-xxe
226+
builder.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
205227
try {
206228
sDoc = builder.build(inputStream);
207229
// Element root = null;

‎vcell-rest/src/main/java/org/vcell/restq/handlers/BioModelResource.java‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package org.vcell.restq.handlers;
22

3+
import cbit.util.xml.XmlUtil;
34
import cbit.vcell.modeldb.BioModelRep;
45
import cbit.vcell.xml.XmlHelper;
56
import cbit.vcell.xml.XmlParseException;
@@ -9,11 +10,9 @@
910
import jakarta.ws.rs.*;
1011
import jakarta.ws.rs.core.MediaType;
1112
import org.eclipse.microprofile.openapi.annotations.Operation;
12-
import org.eclipse.microprofile.openapi.annotations.media.Content;
13-
import org.eclipse.microprofile.openapi.annotations.media.Schema;
1413
import org.eclipse.microprofile.openapi.annotations.parameters.Parameter;
1514
import org.eclipse.microprofile.openapi.annotations.parameters.RequestBody;
16-
import org.jboss.resteasy.reactive.Separator;
15+
import org.jdom2.JDOMException;
1716
import org.vcell.restq.Main;
1817
import org.vcell.restq.errors.exceptions.*;
1918
import org.vcell.restq.models.BioModel;
@@ -24,6 +23,7 @@
2423
import org.vcell.util.PermissionException;
2524
import org.vcell.util.document.*;
2625

26+
import java.io.IOException;
2727
import java.util.List;
2828
import java.util.Optional;
2929

@@ -110,13 +110,17 @@ public String getBioModelVCML(@PathParam("bioModelID") String bioModelID) throws
110110
vcellUser = Main.DUMMY_USER;
111111
}
112112
try {
113-
return bioModelRestService.getBioModel(vcellUser, new KeyValue(bioModelID));
113+
String vcml = bioModelRestService.getBioModel(vcellUser, new KeyValue(bioModelID));
114+
XmlUtil.vetXMLForMaliciousEntities(vcml);
115+
return vcml;
114116
}catch (ObjectNotFoundException e) {
115117
throw new NotFoundWebException(e.getMessage(), e);
116118
} catch (PermissionException e){
117119
throw new PermissionWebException(e.getMessage(), e);
118120
} catch (DataAccessException e) {
119121
throw new DataAccessWebException(e.getMessage(), e);
122+
} catch (IOException | JDOMException e) {
123+
throw new RuntimeWebException("The BioModel you are trying to retrieve seems to be malformed. Please contact VCell support with the BioModel ID: " + bioModelID, e);
120124
}
121125
}
122126

@@ -179,12 +183,15 @@ public String save(@RequestBody(name = "bioModelVCML", required = true, descript
179183
@QueryParam("simsRequiringUpdates") @Parameter(required = false, allowEmptyValue = true, description = simsRequiringUpdatesDescription) List<String> simNames) throws DataAccessWebException, UnprocessableContentWebException, NotAuthenticatedWebException {
180184
User user = userRestService.getUserFromIdentity(securityIdentity);
181185
try {
186+
XmlUtil.vetXMLForMaliciousEntities(bioModelVCML);
182187
cbit.vcell.biomodel.BioModel savedBioModel = bioModelRestService.save(user, bioModelVCML,
183188
newName.orElse(null), simNames.toArray(new String[0]));
184-
return XmlHelper.bioModelToXML(savedBioModel);
189+
String result = XmlHelper.bioModelToXML(savedBioModel);
190+
XmlUtil.vetXMLForMaliciousEntities(result);
191+
return result;
185192
} catch (DataAccessException e) {
186193
throw new DataAccessWebException(e.getMessage(), e);
187-
} catch (XmlParseException e){
194+
} catch (XmlParseException | IOException | JDOMException e){
188195
throw new UnprocessableContentWebException(e.getMessage(), e);
189196
}
190197
}

‎vcell-rest/src/main/java/org/vcell/restq/handlers/MathModelResource.java‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package org.vcell.restq.handlers;
22

33

4+
import cbit.util.xml.XmlUtil;
45
import io.quarkus.security.identity.SecurityIdentity;
56
import jakarta.annotation.security.RolesAllowed;
67
import jakarta.inject.Inject;
@@ -9,10 +10,8 @@
910
import org.eclipse.microprofile.openapi.annotations.Operation;
1011
import org.eclipse.microprofile.openapi.annotations.parameters.Parameter;
1112
import org.eclipse.microprofile.openapi.annotations.parameters.RequestBody;
12-
import org.vcell.restq.errors.exceptions.DataAccessWebException;
13-
import org.vcell.restq.errors.exceptions.NotAuthenticatedWebException;
14-
import org.vcell.restq.errors.exceptions.NotFoundWebException;
15-
import org.vcell.restq.errors.exceptions.PermissionWebException;
13+
import org.jdom2.JDOMException;
14+
import org.vcell.restq.errors.exceptions.*;
1615
import org.vcell.restq.services.MathModelService;
1716
import org.vcell.restq.services.UserRestService;
1817
import org.vcell.util.BigString;
@@ -21,6 +20,7 @@
2120
import org.vcell.util.PermissionException;
2221
import org.vcell.util.document.*;
2322

23+
import java.io.IOException;
2424
import java.util.ArrayList;
2525
import java.util.List;
2626
import java.util.Optional;
@@ -59,13 +59,16 @@ public String getMathModel(@PathParam("id") String id) throws DataAccessWebExcep
5959
User user = userRestService.getUserOrAnonymousFromIdentity(securityIdentity);
6060
try{
6161
BigString result = mathModelService.getMathModelVCML(user, new KeyValue(id));
62+
XmlUtil.vetXMLForMaliciousEntities(result.toString());
6263
return result.toString();
6364
} catch (ObjectNotFoundException e){
6465
throw new NotFoundWebException(e.getMessage(), e);
6566
} catch (PermissionException e){
6667
throw new PermissionWebException(e.getMessage(), e);
6768
} catch (DataAccessException e) {
6869
throw new DataAccessWebException(e.getMessage(), e);
70+
} catch (IOException | JDOMException e) {
71+
throw new RuntimeWebException("The MathModel you are trying to retrieve seems to be malformed, please contact VCell support with the Math Model ID: " + id, e);
6972
}
7073
}
7174

@@ -124,13 +127,17 @@ public String save(@RequestBody(name = "mathModelVCML", required = true) String
124127
@Parameter(name = "newName", required = false, description = "Name to save new MathModel under. Leave blank if re-saving existing MathModel.")
125128
@QueryParam("newName") Optional<String> newName,
126129
@Parameter(name = "simNames", required = false, description = BioModelResource.simsRequiringUpdatesDescription)
127-
@QueryParam("simsRequiringUpdates") List<String> simNames) throws DataAccessWebException, NotAuthenticatedWebException {
130+
@QueryParam("simsRequiringUpdates") List<String> simNames) throws DataAccessWebException, NotAuthenticatedWebException, UnprocessableContentWebException {
128131
User user = userRestService.getUserFromIdentity(securityIdentity);
129132
try{
133+
XmlUtil.vetXMLForMaliciousEntities(mathModelVCML);
130134
BigString result = mathModelService.saveModel(user, new BigString(mathModelVCML), newName.orElse(null), simNames.toArray(new String[0]));
135+
XmlUtil.vetXMLForMaliciousEntities(result.toString()); // partial saves might include already saved XML
131136
return result.toString();
132137
} catch (DataAccessException e) {
133138
throw new DataAccessWebException(e.getMessage(), e);
139+
} catch (IOException | JDOMException e) {
140+
throw new UnprocessableContentWebException(e.getMessage(), e);
134141
}
135142
}
136143

‎vcell-rest/src/test/java/org/vcell/restq/apiclient/BioModelApiTest.java‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import org.vcell.restq.config.CDIVCellConfigProvider;
2929
import org.vcell.restq.db.AgroalConnectionFactory;
3030
import org.vcell.restq.errors.exceptions.DataAccessWebException;
31+
import org.vcell.restq.errors.exceptions.UnprocessableContentWebException;
3132
import org.vcell.util.DataAccessException;
3233
import org.vcell.util.ObjectNotFoundException;
3334
import org.vcell.util.document.BioModelChildSummary;
@@ -36,6 +37,7 @@
3637

3738
import java.beans.PropertyVetoException;
3839
import java.io.IOException;
40+
import java.nio.file.Files;
3941
import java.sql.SQLException;
4042
import java.util.ArrayList;
4143

@@ -166,4 +168,17 @@ public void testGetBioModelContext() throws PropertyVetoException, XmlParseExcep
166168
Assertions.assertEquals(BioModelChildSummary.MathType.Deterministic, info.getBioModelChildSummary().getAppTypes()[0]);
167169
Assertions.assertEquals(new ArrayList<>(){{add("non-spatial ODE");}}, context.getSummary().getSimulationContextNames());
168170
}
171+
172+
@Test
173+
public void testBadXML() throws IOException, ApiException {
174+
BioModelResourceApi bioModelResourceApi = new BioModelResourceApi(aliceAPIClient);
175+
String badXML = Files.readString(TestEndpointUtils.getResourceFile("/BadXML.xml").toPath());
176+
try{
177+
bioModelResourceApi.saveBioModel(badXML, null, null);
178+
Assertions.fail();
179+
} catch (ApiException e){
180+
Assertions.assertEquals(UnprocessableContentWebException.HTTP_CODE, e.getCode());
181+
Assertions.assertTrue(e.getMessage().contains("DOCTYPE is disallowed"));
182+
}
183+
}
169184
}

‎vcell-rest/src/test/resources/BadXML.xml‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
<?xml version="1.0" encoding="UTF-8"?>
2+
<!DOCTYPE note [
3+
<!ENTITY company SYSTEM "https://vcell.org">
4+
]>
5+
<note>
6+
<to>VCell</to>
7+
<from>Ezequiel</from>
8+
<heading>Bad XML</heading>
9+
<body>This XML tries to reference an external entity!</body>
10+
</note>

‎vcell-restclient/api/openapi.yaml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -931,6 +931,12 @@ paths:
931931
description: Not Authorized
932932
"403":
933933
description: Not Allowed
934+
"422":
935+
content:
936+
application/json:
937+
schema:
938+
$ref: '#/components/schemas/VCellHTTPError'
939+
description: Unprocessable content submitted
934940
"500":
935941
content:
936942
application/json:

‎vcell-restclient/docs/MathModelResourceApi.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -643,6 +643,7 @@ public class Example {
643643
| **200** | OK | - |
644644
| **401** | Not Authorized | - |
645645
| **403** | Not Allowed | - |
646+
| **422** | Unprocessable content submitted | - |
646647
| **500** | Data Access Exception | - |
647648

648649
## saveMathModelWithHttpInfo
@@ -718,5 +719,6 @@ ApiResponse<**String**>
718719
| **200** | OK | - |
719720
| **401** | Not Authorized | - |
720721
| **403** | Not Allowed | - |
722+
| **422** | Unprocessable content submitted | - |
721723
| **500** | Data Access Exception | - |
722724

0 commit comments

Comments
 (0)