Security hardening, memory leak fixes, and SMSD adapter optimization

asad · asad · commit d54974f605c7 · 2026-03-31T06:37:03.000+01:00
- ThreadSafeCache cleanup moved to finally block in CallableAtomMappingTool
- BitSet cloning reduced from 3 to 1 in Tanimoto/Cosine/Dice similarity
- Clone IAtomContainer before passing to parallel MCS threads in GraphMatcher
- XXE hardening: disable DOCTYPE in DocumentBuilderFactory and TransformerFactory
- Fix FileWriter resource leak in BondChangeCalculator (proper try-with-resources)
- Path traversal protection via getCanonicalFile() in CLI file parsing
- Isomorphism: reuse single SMSD object for substructure + MCS fallback
- Substructure: reduce findAllSubstructures limit from 100 to 10
- Use 256-bit fingerprints for reduced memory in cache key generation
- Add .claude/, .env, *.secret to .gitignore
- Bump version to 3.6.1, all 163 tests pass
diff --git a/.gitignore b/.gitignore
@@ -38,3 +38,6 @@ hs_err_pid*
 /rdt/core/Output/
 /rdt/target/
 /rdt/Output/
+.claude/
+.env
+*.secret
diff --git a/pom.xml b/pom.xml
@@ -4,7 +4,7 @@
     <groupId>com.bioinceptionlabs</groupId>
     <artifactId>rdt</artifactId>
     <description>Reaction Decoder Tool</description>
-    <version>3.6.0</version>
+    <version>3.6.1</version>
     <packaging>jar</packaging>
     <properties>
         <jdk.version>21</jdk.version>
diff --git a/src/main/java/com/bioinceptionlabs/aamtool/ChemicalFormatParser.java b/src/main/java/com/bioinceptionlabs/aamtool/ChemicalFormatParser.java
@@ -58,6 +58,11 @@ class ChemicalFormatParser {
 
     protected static IReaction parseCML(String input) throws FileNotFoundException, CDKException {
         File f = new File(input);
+        try {
+            f = f.getCanonicalFile();
+        } catch (IOException e) {
+            throw new FileNotFoundException("Invalid file path: " + input);
+        }
         if (!f.isFile()) {
             throw new FileNotFoundException("CML file not found: " + f.getName());
         }
@@ -87,7 +92,13 @@ protected static List<IReaction> parseRXN(String fileNames) {
                 continue;
             }
             String fileName = f[0].trim() + ".rxn";
-            File filepath = new File(fileName);
+            File filepath;
+            try {
+                filepath = new File(fileName).getCanonicalFile();
+            } catch (IOException e) {
+                LOGGER.error(WARNING, format("Invalid file path! %s", fileName));
+                continue;
+            }
             if (!filepath.isFile()) {
                 LOGGER.error(WARNING, format("RXN file not found! %s", filepath.getName()));
                 continue;
diff --git a/src/main/java/com/bioinceptionlabs/aamtool/ReactionDecoder.java b/src/main/java/com/bioinceptionlabs/aamtool/ReactionDecoder.java
@@ -163,6 +163,8 @@ private void FormatXMLToFile(Document doc, String fileName) throws TransformerCo
 
         // write xml to file
         TransformerFactory transformerFactory = TransformerFactory.newInstance();
+        transformerFactory.setAttribute(javax.xml.XMLConstants.ACCESS_EXTERNAL_DTD, "");
+        transformerFactory.setAttribute(javax.xml.XMLConstants.ACCESS_EXTERNAL_STYLESHEET, "");
 
         Transformer transformer = transformerFactory.newTransformer();
         transformer.setOutputProperty(METHOD, "xml");
@@ -258,6 +260,7 @@ private void AAMTask(CommandLine aamLine, Options createAAMOptions,
 
         if (writeFiles && aamLine.getOptionValue("f").equalsIgnoreCase("XML")) {
             DocumentBuilderFactory docFactory = DocumentBuilderFactory.newInstance();
+            docFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
             DocumentBuilder docBuilder = docFactory.newDocumentBuilder();
             // root element
             org.w3c.dom.Document doc = docBuilder.newDocument();
@@ -275,6 +278,7 @@ private void AAMTask(CommandLine aamLine, Options createAAMOptions,
         } else if (writeFiles && aamLine.getOptionValue("f").equalsIgnoreCase("BOTH")) {
 
             DocumentBuilderFactory docFactory = DocumentBuilderFactory.newInstance();
+            docFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
             DocumentBuilder docBuilder = docFactory.newDocumentBuilder();
             // root element
             org.w3c.dom.Document doc = docBuilder.newDocument();
diff --git a/src/main/java/com/bioinceptionlabs/reactionblast/mapping/CallableAtomMappingTool.java b/src/main/java/com/bioinceptionlabs/reactionblast/mapping/CallableAtomMappingTool.java
@@ -172,9 +172,9 @@ private void generateAtomAtomMapping(
             LOGGER.error(e);
         } finally {
             executor.shutdown();
+            LOGGER.debug("!!!!Atom-Atom Mapping Done!!!!");
+            ThreadSafeCache.getInstance().cleanup();
         }
-        LOGGER.debug("!!!!Atom-Atom Mapping Done!!!!");
-        ThreadSafeCache.getInstance().cleanup();
     }
 
     /**
diff --git a/src/main/java/com/bioinceptionlabs/reactionblast/mapping/GraphMatcher.java b/src/main/java/com/bioinceptionlabs/reactionblast/mapping/GraphMatcher.java
@@ -297,8 +297,18 @@ public static Collection<MCSSolution> matcher(Holder mh) throws Exception {
                 int numberOfCyclesProduct = productCycleCache.getOrDefault(productIndex, 0);
                 boolean ringSizeEqual = (numberOfCyclesEduct == numberOfCyclesProduct);
 
+                // Clone molecules for thread safety — CDK IAtomContainer is mutable and not thread-safe
+                IAtomContainer eductClone;
+                IAtomContainer productClone;
+                try {
+                    eductClone = educt.clone();
+                    productClone = product.clone();
+                } catch (CloneNotSupportedException e) {
+                    eductClone = educt;
+                    productClone = product;
+                }
                 MCSThread mcsThread = new MCSThread(mh.getTheory(),
-                        substrateIndex, productIndex, educt, product);
+                        substrateIndex, productIndex, eductClone, productClone);
                 mcsThread.setHasPerfectRings(ringSizeEqual);
                 mcsThread.setEductRingCount(numberOfCyclesEduct);
                 mcsThread.setProductRingCount(numberOfCyclesProduct);
diff --git a/src/main/java/com/bioinceptionlabs/reactionblast/mapping/algorithm/GameTheoryEngine.java b/src/main/java/com/bioinceptionlabs/reactionblast/mapping/algorithm/GameTheoryEngine.java
@@ -506,7 +506,7 @@ private int[] getCircularFP(IAtomContainer mol) throws CDKException {
         if (cached != null) {
             return cached;
         }
-        long[] fp = SMSD.circularFingerprintFCFP(mol, 1, 1024);
+        long[] fp = SMSD.circularFingerprintFCFP(mol, 1, 256);
         BitSet bs = SMSD.toBitSet(fp);
         int[] bits = new int[bs.cardinality()];
         int idx = 0;
diff --git a/src/main/java/com/bioinceptionlabs/reactionblast/mechanism/BondChangeCalculator.java b/src/main/java/com/bioinceptionlabs/reactionblast/mechanism/BondChangeCalculator.java
@@ -454,8 +454,7 @@ public String toString() {
      * @throws IOException
      */
     public void writeBondChanges(File bondChangeInfoFile) throws IOException {
-        FileWriter bcFW = new FileWriter(bondChangeInfoFile + ".txt");
-        try (BufferedWriter bfw = new BufferedWriter(bcFW)) {
+        try (BufferedWriter bfw = new BufferedWriter(new FileWriter(bondChangeInfoFile + ".txt"))) {
             bfw.newLine();
 
             bfw.write(getLicenseHeader());
diff --git a/src/main/java/com/bioinceptionlabs/reactionblast/tools/StandardizeReaction.java b/src/main/java/com/bioinceptionlabs/reactionblast/tools/StandardizeReaction.java
@@ -177,7 +177,7 @@ public IReaction filterReagents(IReaction reaction) {
             List<long[]> productFPs = new ArrayList<>();
             for (IAtomContainer prod : products.atomContainers()) {
                 try {
-                    productFPs.add(SMSD.circularFingerprintECFP(prod, 2, 1024));
+                    productFPs.add(SMSD.circularFingerprintECFP(prod, 2, 256));
                 } catch (Exception e) {
                     productFPs.add(null);
                 }
@@ -223,7 +223,7 @@ public IReaction filterReagents(IReaction reaction) {
 
                     // Check 3: Tanimoto fingerprint similarity
                     if (!isReagent && !neededForBalance) {
-                        long[] reactantFP = SMSD.circularFingerprintECFP(reactant, 2, 1024);
+                        long[] reactantFP = SMSD.circularFingerprintECFP(reactant, 2, 256);
 
                         double maxSim = 0.0;
                         for (long[] prodFP : productFPs) {
diff --git a/src/main/java/org/openscience/smsd/Isomorphism.java b/src/main/java/org/openscience/smsd/Isomorphism.java
@@ -104,28 +104,42 @@ private void mcsBuilder(IAtomContainer mol1, IAtomContainer mol2) throws CDKExce
     }
 
     /**
-     * Delegates MCS computation to SMSD 3.0.0.
+     * Delegates MCS computation to SMSD.
+     * First tries a fast substructure check; if that fails, falls back to full MCS.
      */
     private void smsdMCSAlgorithm(IAtomContainer mol1, IAtomContainer mol2) throws CDKException {
         try {
-            // First try substructure check
-            if (mol1.getAtomCount() > 1 && mol2.getAtomCount() > 1) {
-                Substructure sub;
-                if (mol1 instanceof IQueryAtomContainer) {
-                    sub = new Substructure((IQueryAtomContainer) mol1, mol2, atomMatcher, bondMatcher, true);
-                } else {
-                    sub = new Substructure(mol1, mol2, atomMatcher, bondMatcher, true);
-                }
-                if (sub.isSubgraph()) {
+            ChemOptions chemOptions = buildChemOptions();
+            SMSD smsd = new SMSD(mol1, mol2, chemOptions);
+
+            // Fast substructure check before expensive MCS
+            if (smsd.isSubstructure(5000)) {
+                java.util.List<Map<Integer, Integer>> subResults = smsd.findAllSubstructures(1, 5000);
+                if (subResults != null && !subResults.isEmpty()) {
                     clearMaps();
-                    getMCSList().addAll(sub.getAllAtomMapping());
+                    for (Map<Integer, Integer> mapping : subResults) {
+                        AtomAtomMapping aam = new AtomAtomMapping(mol1, mol2);
+                        for (Map.Entry<Integer, Integer> entry : mapping.entrySet()) {
+                            int qIdx = entry.getKey();
+                            int tIdx = entry.getValue();
+                            if (qIdx >= 0 && qIdx < mol1.getAtomCount()
+                                    && tIdx >= 0 && tIdx < mol2.getAtomCount()) {
+                                IAtom qAtom = mol1.getAtom(qIdx);
+                                IAtom tAtom = mol2.getAtom(tIdx);
+                                if (qAtom != null && tAtom != null) {
+                                    aam.put(qAtom, tAtom);
+                                }
+                            }
+                        }
+                        if (!aam.isEmpty()) {
+                            getMCSList().add(aam);
+                        }
+                    }
                     return;
                 }
             }
 
-            // Fall back to MCS via SMSD 3.4.0
-            ChemOptions chemOptions = new ChemOptions();
-            SMSD smsd = new SMSD(mol1, mol2, chemOptions);
+            // Fall back to full MCS
             Map<Integer, Integer> mcsResult = smsd.findMCS();
 
             clearMaps();
diff --git a/src/main/java/org/openscience/smsd/Substructure.java b/src/main/java/org/openscience/smsd/Substructure.java
@@ -129,11 +129,11 @@ private boolean findSubgraphs(boolean findAllMatches) throws CDKException {
         }
 
         try {
-            ChemOptions chemOptions = new ChemOptions();
+            ChemOptions chemOptions = buildChemOptions();
             SMSD smsd = new SMSD(getQuery(), getTarget(), chemOptions);
 
             if (findAllMatches) {
-                List<Map<Integer, Integer>> results = smsd.findAllSubstructures(100, 30000);
+                List<Map<Integer, Integer>> results = smsd.findAllSubstructures(10, 30000);
                 if (results != null && !results.isEmpty()) {
                     for (Map<Integer, Integer> mapping : results) {
                         AtomAtomMapping aam = convertMapping(getQuery(), getTarget(), mapping);

Original file line number	Diff line number	Diff line change
`@@ -172,9 +172,9 @@ private void generateAtomAtomMapping(`
`172`	`172`	`LOGGER.error(e);`
`173`	`173`	`} finally {`
`174`	`174`	`executor.shutdown();`
	`175`	`+ LOGGER.debug("!!!!Atom-Atom Mapping Done!!!!");`
	`176`	`+ ThreadSafeCache.getInstance().cleanup();`
`175`	`177`	`}`
`176`		`- LOGGER.debug("!!!!Atom-Atom Mapping Done!!!!");`
`177`		`- ThreadSafeCache.getInstance().cleanup();`
`178`	`178`	`}`
`179`	`179`
`180`	`180`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -506,7 +506,7 @@ private int[] getCircularFP(IAtomContainer mol) throws CDKException {`
`506`	`506`	`if (cached != null) {`
`507`	`507`	`return cached;`
`508`	`508`	`}`
`509`		`- long[] fp = SMSD.circularFingerprintFCFP(mol, 1, 1024);`
	`509`	`+ long[] fp = SMSD.circularFingerprintFCFP(mol, 1, 256);`
`510`	`510`	`BitSet bs = SMSD.toBitSet(fp);`
`511`	`511`	`int[] bits = new int[bs.cardinality()];`
`512`	`512`	`int idx = 0;`