Merge pull request #2904 from tdonohue/lgtm_fixes_2

Fixes for several LGTM reported severe bugs

dspace-api/src/main/java/org/dspace/app/itemimport/ItemImportServiceImpl.java

@@ -1519,6 +1519,12 @@ public class ItemImportServiceImpl implements ItemImportService, InitializingBea
                     if (!dir.exists() && !dir.mkdirs()) {
                         log.error("Unable to create directory: " + dir.getAbsolutePath());
                     }
+                    // Verify that the directory the entry is using is a subpath of zipDir (and not somewhere else!)
+                    if (!dir.toPath().normalize().startsWith(zipDir)) {
+                        throw new IOException("Bad zip entry: '" + entry.getName()
+                                                  + "' in file '" + zipfile.getAbsolutePath() + "'!"
+                                                  + " Cannot process this file.");
+                    }
 
                     //Entries could have too many directories, and we need to adjust the sourcedir
                     // file1.zip (SimpleArchiveFormat / item1 / contents|dublin_core|...
@@ -1539,9 +1545,16 @@ public class ItemImportServiceImpl implements ItemImportService, InitializingBea
                 }
                 byte[] buffer = new byte[1024];
                 int len;
+                File outFile = new File(zipDir + entry.getName());
+                // Verify that this file will be created in our zipDir (and not somewhere else!)
+                if (!outFile.toPath().normalize().startsWith(zipDir)) {
+                    throw new IOException("Bad zip entry: '" + entry.getName()
+                                              + "' in file '" + zipfile.getAbsolutePath() + "'!"
+                                              + " Cannot process this file.");
+                }
                 InputStream in = zf.getInputStream(entry);
                 BufferedOutputStream out = new BufferedOutputStream(
-                    new FileOutputStream(zipDir + entry.getName()));
+                    new FileOutputStream(outFile));
                 while ((len = in.read(buffer)) >= 0) {
                     out.write(buffer, 0, len);
                 }

dspace-api/src/main/java/org/dspace/app/sherpa/SHERPAResponse.java

@@ -48,6 +48,9 @@ public class SHERPAResponse {
             factory.setValidating(false);
             factory.setIgnoringComments(true);
             factory.setIgnoringElementContentWhitespace(true);
+            // disallow DTD parsing to ensure no XXE attacks can occur.
+            // See https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
+            factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
 
             DocumentBuilder db = factory.newDocumentBuilder();
             Document inDoc = db.parse(xmlData);

dspace-api/src/main/java/org/dspace/content/BitstreamFormatServiceImpl.java

@@ -153,7 +153,7 @@ public class BitstreamFormatServiceImpl implements BitstreamFormatService {
         // If the exception was thrown, unknown will == null so goahead and
         // load s. If not, check that the unknown's registry's name is not
         // being reset.
-        if (unknown == null || unknown.getID() != bitstreamFormat.getID()) {
+        if (unknown == null || !unknown.getID().equals(bitstreamFormat.getID())) {
             bitstreamFormat.setShortDescriptionInternal(shortDescription);
         }
     }
@@ -208,7 +208,7 @@ public class BitstreamFormatServiceImpl implements BitstreamFormatService {
         // Find "unknown" type
         BitstreamFormat unknown = findUnknown(context);
 
-        if (unknown.getID() == bitstreamFormat.getID()) {
+        if (unknown.getID().equals(bitstreamFormat.getID())) {
             throw new IllegalArgumentException("The Unknown bitstream format may not be deleted.");
         }
 

dspace-api/src/main/java/org/dspace/content/MetadataField.java

@@ -168,11 +168,11 @@ public class MetadataField implements ReloadableEntity<Integer> {
             return false;
         }
         Class<?> objClass = HibernateProxyHelper.getClassWithoutInitializingProxy(obj);
-        if (getClass() != objClass) {
+        if (!getClass().equals(objClass)) {
             return false;
         }
         final MetadataField other = (MetadataField) obj;
-        if (this.getID() != other.getID()) {
+        if (!this.getID().equals(other.getID())) {
             return false;
         }
         if (!getMetadataSchema().equals(other.getMetadataSchema())) {

dspace-api/src/main/java/org/dspace/content/MetadataSchema.java

@@ -67,11 +67,11 @@ public class MetadataSchema implements ReloadableEntity<Integer> {
             return false;
         }
         Class<?> objClass = HibernateProxyHelper.getClassWithoutInitializingProxy(obj);
-        if (getClass() != objClass) {
+        if (!getClass().equals(objClass)) {
             return false;
         }
         final MetadataSchema other = (MetadataSchema) obj;
-        if (this.id != other.id) {
+        if (!this.id.equals(other.id)) {
             return false;
         }
         if ((this.namespace == null) ? (other.namespace != null) : !this.namespace.equals(other.namespace)) {

dspace-api/src/main/java/org/dspace/content/MetadataValue.java

@@ -239,17 +239,17 @@ public class MetadataValue implements ReloadableEntity<Integer> {
             return false;
         }
         Class<?> objClass = HibernateProxyHelper.getClassWithoutInitializingProxy(obj);
-        if (getClass() != objClass) {
+        if (!getClass().equals(objClass)) {
             return false;
         }
         final MetadataValue other = (MetadataValue) obj;
-        if (this.id != other.id) {
+        if (!this.id.equals(other.id)) {
             return false;
         }
-        if (this.getID() != other.getID()) {
+        if (!this.getID().equals(other.getID())) {
             return false;
         }
-        if (this.getDSpaceObject().getID() != other.getDSpaceObject().getID()) {
+        if (!this.getDSpaceObject().getID().equals(other.getDSpaceObject().getID())) {
             return false;
         }
         return true;

dspace-api/src/main/java/org/dspace/content/WorkspaceItem.java

@@ -156,11 +156,11 @@ public class WorkspaceItem
             return true;
         }
         Class<?> objClass = HibernateProxyHelper.getClassWithoutInitializingProxy(o);
-        if (getClass() != objClass) {
+        if (!getClass().equals(objClass)) {
             return false;
         }
         final WorkspaceItem that = (WorkspaceItem) o;
-        if (this.getID() != that.getID()) {
+        if (!this.getID().equals(that.getID())) {
             return false;
         }
 

dspace-api/src/main/java/org/dspace/content/packager/METSManifest.java

@@ -272,13 +272,17 @@ public class METSManifest {
         // Set validation feature
         if (validate) {
             builder.setFeature("http://apache.org/xml/features/validation/schema", true);
-        }
 
             // Tell the parser where local copies of schemas are, to speed up
-        // validation.  Local XSDs are identified in the configuration file.
+            // validation & avoid XXE attacks from remote schemas. Local XSDs are identified in the configuration file.
             if (localSchemas.length() > 0) {
                 builder.setProperty("http://apache.org/xml/properties/schema/external-schemaLocation", localSchemas);
             }
+        } else {
+            // disallow DTD parsing to ensure no XXE attacks can occur.
+            // See https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
+            builder.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
+        }
 
         // Parse the METS file
         Document metsDocument;

dspace-api/src/main/java/org/dspace/ctask/general/MetadataWebService.java

@@ -199,6 +199,9 @@ public class MetadataWebService extends AbstractCurationTask implements Namespac
         DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
         factory.setNamespaceAware(true);
         try {
+            // disallow DTD parsing to ensure no XXE attacks can occur.
+            // See https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
+            factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
             docBuilder = factory.newDocumentBuilder();
         } catch (ParserConfigurationException pcE) {
             log.error("caught exception: " + pcE);

dspace-api/src/main/java/org/dspace/harvest/HarvestScheduler.java

@@ -134,11 +134,13 @@ public class HarvestScheduler implements Runnable {
         if (maxActiveThreads == 0) {
             maxActiveThreads = 3;
         }
-        minHeartbeat = ConfigurationManager.getIntProperty("oai", "harvester.minHeartbeat") * 1000;
+        minHeartbeat = ConfigurationManager.getIntProperty("oai", "harvester.minHeartbeat");
+        minHeartbeat = minHeartbeat * 1000; // multiple by 1000 to turn seconds to ms
         if (minHeartbeat == 0) {
             minHeartbeat = 30000;
         }
-        maxHeartbeat = ConfigurationManager.getIntProperty("oai", "harvester.maxHeartbeat") * 1000;
+        maxHeartbeat = ConfigurationManager.getIntProperty("oai", "harvester.maxHeartbeat");
+        maxHeartbeat = maxHeartbeat * 1000; // multiple by 1000 to turn seconds to ms
         if (maxHeartbeat == 0) {
             maxHeartbeat = 3600000;
         }

dspace-api/src/main/java/org/dspace/license/CCLicenseConnectorServiceImpl.java

@@ -75,6 +75,10 @@ public class CCLicenseConnectorServiceImpl implements CCLicenseConnectorService,
                 .disableAutomaticRetries()
                 .setMaxConnTotal(5)
                 .build();
+
+        // disallow DTD parsing to ensure no XXE attacks can occur.
+        // See https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
+        parser.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
     }
 
     /**

dspace-api/src/main/java/org/dspace/rdf/negotiation/Negotiator.java

@@ -15,6 +15,7 @@ import java.util.Iterator;
 import javax.servlet.http.HttpServletResponse;
 
 import org.apache.commons.lang3.StringUtils;
+import org.apache.commons.validator.routines.UrlValidator;
 import org.apache.logging.log4j.Logger;
 import org.dspace.rdf.RDFUtil;
 import org.dspace.services.factory.DSpaceServicesFactory;
@@ -197,6 +198,7 @@ public class Negotiator {
         if (extraPathInfo == null) {
             extraPathInfo = "";
         }
+        UrlValidator urlValidator = new UrlValidator(UrlValidator.ALLOW_LOCAL_URLS);
 
         StringBuilder urlBuilder = new StringBuilder();
         String lang = null;
@@ -256,12 +258,15 @@ public class Negotiator {
                 urlBuilder.append(handle).append("/").append(extraPathInfo);
             }
             String url = urlBuilder.toString();
-
+            if (urlValidator.isValid(url)) {
                 log.debug("Will forward to '" + url + "'.");
                 response.setStatus(HttpServletResponse.SC_SEE_OTHER);
                 response.setHeader("Location", url);
                 response.flushBuffer();
                 return true;
+            } else {
+                throw new IOException("Invalid URL '" + url + "', cannot redirect.");
+            }
         }
 
         // currently we cannot serve statistics as rdf
@@ -287,10 +292,14 @@ public class Negotiator {
         urlBuilder.append("/handle/").append(handle);
         urlBuilder.append("/").append(lang);
         String url = urlBuilder.toString();
+        if (urlValidator.isValid(url)) {
             log.debug("Will forward to '" + url + "'.");
             response.setStatus(HttpServletResponse.SC_SEE_OTHER);
             response.setHeader("Location", url);
             response.flushBuffer();
             return true;
+        } else {
+            throw new IOException("Invalid URL '" + url + "', cannot redirect.");
+        }
     }
 }

dspace-api/src/main/java/org/dspace/submit/lookup/ArXivService.java

@@ -113,6 +113,9 @@ public class ArXivService {
                 factory.setValidating(false);
                 factory.setIgnoringComments(true);
                 factory.setIgnoringElementContentWhitespace(true);
+                // disallow DTD parsing to ensure no XXE attacks can occur.
+                // See https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
+                factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
 
                 DocumentBuilder db = factory.newDocumentBuilder();
                 Document inDoc = db.parse(response.getEntity().getContent());

dspace-api/src/main/java/org/dspace/submit/lookup/CiNiiService.java

@@ -102,6 +102,9 @@ public class CiNiiService {
                 factory.setValidating(false);
                 factory.setIgnoringComments(true);
                 factory.setIgnoringElementContentWhitespace(true);
+                // disallow DTD parsing to ensure no XXE attacks can occur.
+                // See https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
+                factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
 
                 DocumentBuilder db = factory.newDocumentBuilder();
                 Document inDoc = db.parse(response.getEntity().getContent());
@@ -178,6 +181,9 @@ public class CiNiiService {
                 factory.setValidating(false);
                 factory.setIgnoringComments(true);
                 factory.setIgnoringElementContentWhitespace(true);
+                // disallow DTD parsing to ensure no XXE attacks can occur.
+                // See https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
+                factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
 
                 DocumentBuilder db = factory.newDocumentBuilder();
                 Document inDoc = db.parse(response.getEntity().getContent());

dspace-api/src/main/java/org/dspace/submit/lookup/CrossRefService.java

@@ -99,6 +99,9 @@ public class CrossRefService {
                             factory.setValidating(false);
                             factory.setIgnoringComments(true);
                             factory.setIgnoringElementContentWhitespace(true);
+                            // disallow DTD parsing to ensure no XXE attacks can occur.
+                            // See https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
+                            factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
 
                             DocumentBuilder db = factory
                                 .newDocumentBuilder();

dspace-api/src/main/java/org/dspace/submit/lookup/PubmedService.java

@@ -119,6 +119,9 @@ public class PubmedService {
                 factory.setValidating(false);
                 factory.setIgnoringComments(true);
                 factory.setIgnoringElementContentWhitespace(true);
+                // disallow DTD parsing to ensure no XXE attacks can occur.
+                // See https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
+                factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
 
                 DocumentBuilder builder;
                 try {
@@ -156,6 +159,9 @@ public class PubmedService {
                 factory.setValidating(false);
                 factory.setIgnoringComments(true);
                 factory.setIgnoringElementContentWhitespace(true);
+                // disallow DTD parsing to ensure no XXE attacks can occur.
+                // See https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
+                factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
 
                 DocumentBuilder builder = factory.newDocumentBuilder();
                 Document inDoc = builder.parse(stream);
@@ -216,6 +222,9 @@ public class PubmedService {
             factory.setValidating(false);
             factory.setIgnoringComments(true);
             factory.setIgnoringElementContentWhitespace(true);
+            // disallow DTD parsing to ensure no XXE attacks can occur.
+            // See https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
+            factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
 
             DocumentBuilder builder = factory.newDocumentBuilder();
             Document inDoc = builder

dspace-api/src/main/java/org/dspace/versioning/Version.java

@@ -135,12 +135,12 @@ public class Version implements ReloadableEntity<Integer> {
             return true;
         }
         Class<?> objClass = HibernateProxyHelper.getClassWithoutInitializingProxy(o);
-        if (getClass() != objClass) {
+        if (!getClass().equals(objClass)) {
             return false;
         }
 
         final Version that = (Version) o;
-        if (this.getID() != that.getID()) {
+        if (!this.getID().equals(that.getID())) {
             return false;
         }
 

dspace-api/src/main/java/org/dspace/versioning/VersionHistory.java

@@ -93,12 +93,12 @@ public class VersionHistory implements ReloadableEntity<Integer> {
             return true;
         }
         Class<?> objClass = HibernateProxyHelper.getClassWithoutInitializingProxy(o);
-        if (getClass() != objClass) {
+        if (!getClass().equals(objClass)) {
             return false;
         }
 
         final VersionHistory that = (VersionHistory) o;
-        if (this.getID() != that.getID()) {
+        if (!this.getID().equals(that.getID())) {
             return false;
         }
 

dspace-rdf/src/main/java/org/dspace/rdf/providing/LocalURIRedirectionServlet.java

@@ -86,7 +86,8 @@ public class LocalURIRedirectionServlet extends HttpServlet {
             response.sendError(HttpServletResponse.SC_NOT_FOUND);
             return;
         }
-
+        // use object's reported handle for redirect (just in case user provided handle had odd characters)
+        handle = dso.getHandle();
         // close the context and send forward.
         context.abort();
         Negotiator.sendRedirect(response, handle, "", requestedMimeType, true);

dspace-rest/src/main/webapp/static/reports/restReport.js

@@ -661,6 +661,7 @@ var HtmlUtil = function() {
         a.append(val);
         a.attr("href", href);
         a.attr("target", "_blank");
+        a.attr("rel", "noopener noreferrer");
         return a;
     }
 

dspace-server-webapp/src/main/java/org/dspace/app/rest/OpenSearchController.java

@@ -34,6 +34,7 @@ import org.dspace.content.service.CollectionService;
 import org.dspace.content.service.CommunityService;
 import org.dspace.core.Context;
 import org.dspace.core.LogManager;
+import org.dspace.core.Utils;
 import org.dspace.discovery.DiscoverQuery;
 import org.dspace.discovery.DiscoverResult;
 import org.dspace.discovery.IndexableObject;
@@ -103,7 +104,8 @@ public class OpenSearchController {
 
             // do some sanity checking
             if (!openSearchService.getFormats().contains(format)) {
-                String err = "Format " + format + " is not supported.";
+                // Since we are returning error response as HTML, escape any HTML in "format" param
+                String err = "Format " + Utils.addEntities(format) + " is not supported.";
                 response.setContentType("text/html");
                 response.setContentLength(err.length());
                 response.getWriter().write(err);

dspace-server-webapp/src/main/java/org/dspace/app/rest/security/PoolTaskRestPermissionEvaluatorPlugin.java

@@ -75,7 +75,7 @@ public class PoolTaskRestPermissionEvaluatorPlugin extends RestObjectPermissionE
             XmlWorkflowItem workflowItem = poolTask.getWorkflowItem();
 
             PoolTask poolTask2 = poolTaskService.findByWorkflowIdAndEPerson(context, workflowItem, ePerson);
-            if (poolTask2 != null && poolTask2.getID() == poolTask.getID()) {
+            if (poolTask2 != null && poolTask2.getID().equals(poolTask.getID())) {
                 return true;
             }
         } catch (SQLException | AuthorizeException | IOException e) {

dspace-server-webapp/src/main/java/org/dspace/app/rest/security/jwt/JWTTokenRestAuthenticationServiceImpl.java

@@ -154,6 +154,7 @@ public class JWTTokenRestAuthenticationServiceImpl implements RestAuthentication
         Cookie cookie = new Cookie(AUTHORIZATION_COOKIE, "");
         cookie.setHttpOnly(true);
         cookie.setMaxAge(0);
+        cookie.setSecure(true);
         response.addCookie(cookie);
     }
 

dspace-sword/src/main/java/org/dspace/sword/DepositManager.java

@@ -245,6 +245,8 @@ public class DepositManager {
 
         String filenameBase =
             "sword-" + deposit.getUsername() + "-" + (new Date()).getTime();
+        // No dots or slashes allowed in filename
+        filenameBase = filenameBase.replaceAll("\\.", "").replaceAll("/", ""). replaceAll("\\\\", "");
 
         File packageFile = new File(path, filenameBase);
         File headersFile = new File(path, filenameBase + "-headers");