Validate XML chars in name and sbmlName setters

vcell-core/src/main/java/cbit/vcell/biomodel/BioModel.java

@@ -1014,7 +1014,7 @@ public void setName(java.lang.String name) throws java.beans.PropertyVetoExcepti
 
     public void setSbmlName(String newString) throws PropertyVetoException{
         String oldValue = this.sbmlName;
-        String newValue = SpeciesContext.fixSbmlName(newString);
+        String newValue = SpeciesContext.fixAndValidateSbmlName(newString, this);
 
         fireVetoableChange("sbmlName", oldValue, newValue);
         this.sbmlName = newValue;

vcell-core/src/main/java/cbit/vcell/mapping/AssignmentRule.java

@@ -99,8 +99,8 @@ public void setName(String name) {
 	}
 	public void setSbmlName(String newString) throws PropertyVetoException {
 		String oldValue = this.sbmlName;
-		String newValue = SpeciesContext.fixSbmlName(newString);
-		
+		String newValue = SpeciesContext.fixAndValidateSbmlName(newString, this);
+
 		fireVetoableChange("sbmlName", oldValue, newValue);
 		this.sbmlName = newValue;
 		firePropertyChange("sbmlName", oldValue, newValue);

vcell-core/src/main/java/cbit/vcell/mapping/BioEvent.java

@@ -384,8 +384,8 @@ public void setName(String newValue) throws PropertyVetoException {
 	}
 	public void setSbmlName(String newString) throws PropertyVetoException {
 		String oldValue = this.sbmlName;
-		String newValue = SpeciesContext.fixSbmlName(newString);
-		
+		String newValue = SpeciesContext.fixAndValidateSbmlName(newString, this);
+
 		fireVetoableChange("sbmlName", oldValue, newValue);
 		this.sbmlName = newValue;
 		firePropertyChange("sbmlName", oldValue, newValue);

vcell-core/src/main/java/cbit/vcell/mapping/RateRule.java

@@ -112,8 +112,8 @@ public void setName(String name) {
 	}
 	public void setSbmlName(String newString) throws PropertyVetoException {
 		String oldValue = this.sbmlName;
-		String newValue = SpeciesContext.fixSbmlName(newString);
-		
+		String newValue = SpeciesContext.fixAndValidateSbmlName(newString, this);
+
 		fireVetoableChange("sbmlName", oldValue, newValue);
 		this.sbmlName = newValue;
 		firePropertyChange("sbmlName", oldValue, newValue);

vcell-core/src/main/java/cbit/vcell/model/Model.java

@@ -1035,7 +1035,7 @@ public void setSbmlId(String newValue) throws PropertyVetoException{
 
         public void setSbmlName(String newString) throws PropertyVetoException{
             String oldValue = this.sbmlName;
-            String newValue = SpeciesContext.fixSbmlName(newString);
+            String newValue = SpeciesContext.fixAndValidateSbmlName(newString, this);
 
             fireVetoableChange("sbmlName", oldValue, newValue);
             this.sbmlName = newValue;

vcell-core/src/main/java/cbit/vcell/model/ReactionStep.java

@@ -28,6 +28,7 @@
 import org.vcell.util.IssueContext.ContextType;
 import org.vcell.util.document.Identifiable;
 import org.vcell.util.document.KeyValue;
+import org.vcell.util.xml.XmlChars;
 
 import cbit.vcell.model.Kinetics.KineticsParameter;
 import cbit.vcell.model.Membrane.MembraneVoltage;
@@ -1065,7 +1066,7 @@ public void setSbmlId(String newValue) throws PropertyVetoException{
 
     public void setSbmlName(String newString) throws PropertyVetoException{
         String oldValue = this.sbmlName;
-        String newValue = SpeciesContext.fixSbmlName(newString);
+        String newValue = SpeciesContext.fixAndValidateSbmlName(newString, this);
 
         fireVetoableChange("sbmlName", oldValue, newValue);
         this.sbmlName = newValue;
@@ -1167,6 +1168,11 @@ public void vetoableChange(PropertyChangeEvent e) throws PropertyVetoException{
             if(((String) (e.getNewValue())).trim().length() > 255){
                 throw new PropertyVetoException("reactionStep name for reaction \'" + (String) (e.getNewValue()) + "\' cannot be longer than 255 characters", e);
             }
+            try {
+                XmlChars.requireValidAttributeContent((String) e.getNewValue(), "Reaction.name");
+            } catch (IllegalArgumentException ex) {
+                throw new PropertyVetoException(ex.getMessage(), e);
+            }
         }
         if(e.getSource() == this && e.getPropertyName().equals("chargeCarrierValence")){
             if((e.getNewValue() != null) && ((Integer) (e.getNewValue())).intValue() != 0){

vcell-core/src/main/java/cbit/vcell/model/SpeciesContext.java

@@ -10,6 +10,8 @@
 
 package cbit.vcell.model;
 
+import org.vcell.util.xml.XmlChars;
+
 import java.beans.PropertyChangeEvent;
 import java.beans.PropertyChangeListener;
 import java.beans.PropertyVetoException;
@@ -299,10 +301,23 @@ public static String fixSbmlName(String newString) {
 	String newValue = sb.toString();
 	return newValue;
 }
+
+public static String fixAndValidateSbmlName(String newString, Object source) throws PropertyVetoException {
+	String newValue = fixSbmlName(newString);
+	if (newValue == null) return null;
+	try {
+		XmlChars.requireValidAttributeContent(newValue, "sbmlName");
+	} catch (IllegalArgumentException ex) {
+		throw new PropertyVetoException(ex.getMessage(),
+				new PropertyChangeEvent(source, "sbmlName", null, newValue));
+	}
+	return newValue;
+}
+
 public void setSbmlName(String newString) throws PropertyVetoException {
 	String oldValue = this.sbmlName;
-	String newValue = fixSbmlName(newString);
-	
+	String newValue = fixAndValidateSbmlName(newString, this);
+
 	fireVetoableChange("sbmlName", oldValue, newValue);
 	this.sbmlName = newValue;
 	firePropertyChange("sbmlName", oldValue, newValue);

vcell-core/src/main/java/cbit/vcell/model/Structure.java

@@ -68,8 +68,8 @@ public String getSbmlName() {
 	}
 	public void setSbmlName(String newString) throws PropertyVetoException {
 		String oldValue = this.sbmlName;
-		String newValue = SpeciesContext.fixSbmlName(newString);
-		
+		String newValue = SpeciesContext.fixAndValidateSbmlName(newString, this);
+
 		fireVetoableChange("sbmlName", oldValue, newValue);
 		this.sbmlName = newValue;
 		firePropertyChange("sbmlName", oldValue, newValue);

vcell-core/src/test/resources/org/vcell/sbml/vcml_published/biomodel_256364414.vcml

@@ -2913,7 +2913,7 @@ cloned from 'Simulation0' owned by user temp</Annotation>
           <GroupAccess Type="1" />
         </Version>
       </MathDescription>
-      <Simulation Name="Vary Bleach Radius, FluorD=1, Figure 2 is 4� micrometer radius">
+      <Simulation Name="Vary Bleach Radius, FluorD=1, Figure 2 is 4u micrometer radius">
         <Annotation>cloned from 'Vary Bleach Radius, FluorD=1_1' owned by user temp
 cloned from 'Simulation0_1' owned by user temp
 cloned from 'Simulation0_1' owned by user temp
@@ -2936,7 +2936,7 @@ cloned from 'Simulation0' owned by user temp</Annotation>
         <MeshSpecification>
           <Size X="402" Y="402" Z="3" />
         </MeshSpecification>
-        <Version Name="Vary Bleach Radius, FluorD=1, Figure 2 is 4� micrometer radius" KeyValue="256364412" BranchId="256354044" Archived="0" Date="24-May-2023 20:41:20" FromVersionable="false">
+        <Version Name="Vary Bleach Radius, FluorD=1, Figure 2 is 4u micrometer radius" KeyValue="256364412" BranchId="256354044" Archived="0" Date="24-May-2023 20:41:20" FromVersionable="false">
           <Owner Name="les" Identifier="6" />
           <GroupAccess Type="1" />
           <Annotation>cloned from 'Vary Bleach Radius, FluorD=1_1' owned by user temp

vcell-util/src/main/java/org/vcell/util/TokenMangler.java

@@ -10,6 +10,8 @@
 
 package org.vcell.util;
 
+import org.vcell.util.xml.XmlChars;
+
 import java.beans.PropertyChangeEvent;
 import java.beans.PropertyVetoException;
 
@@ -453,12 +455,17 @@ public static void checkLoginID(String loginID) throws IllegalArgumentException
  */
 public static void checkNameProperty(Object source, String owner, PropertyChangeEvent evt) throws PropertyVetoException {
 	if (evt.getSource() == source && evt.getPropertyName().equals("name") && evt.getNewValue()!=null){
-		if (evt.getNewValue() == null || ((String)evt.getNewValue()).trim().length()==0){
+		String newName = (String) evt.getNewValue();
+		if (newName.trim().length()==0){
 			throw new PropertyVetoException("A name must be given to save " + owner + "s", evt);
-		} 
-//		else if (((String)evt.getNewValue()).contains("'")){
-//			throw new PropertyVetoException("Apostrophe is not allowed in " + owner + " names",evt);
-//		}
+		}
+		try {
+			// allow whitespace (entity names like BioModel may contain spaces)
+			// but reject control characters and U+FFFD
+			XmlChars.requireValidAttributeContent(newName, owner + ".name");
+		} catch (IllegalArgumentException ex) {
+			throw new PropertyVetoException(ex.getMessage(), evt);
+		}
 	}
 }
 

vcell-util/src/main/java/org/vcell/util/xml/XmlChars.java

@@ -0,0 +1,120 @@
+package org.vcell.util.xml;
+
+/**
+ * XML character validation helper. Used to keep invalid-XML chars out of
+ * model strings before they reach a CLOB or VCML attribute. See PR drafted
+ * after observing two stored BioModels (311226221, 311875206) whose cached
+ * VCML contained C0 control characters in reaction-name attributes and
+ * therefore failed to load.
+ *
+ * <p>What we forbid (in addition to XML 1.0's own rules):
+ * <ul>
+ *   <li>{@code U+FFFD REPLACEMENT CHARACTER} — almost always evidence of
+ *       upstream charset corruption, never legitimate in a model identifier
+ *       or attribute.</li>
+ * </ul>
+ *
+ * <p>Two contexts are distinguished:
+ * <ul>
+ *   <li><b>Name mode</b> — for entity names ({@code Reaction.name},
+ *       {@code Species.name}, etc.). Forbids whitespace as well.</li>
+ *   <li><b>Attribute-content mode</b> — for general XML attribute values.
+ *       Allows TAB/LF/CR.</li>
+ * </ul>
+ *
+ * <p>All methods are stateless and thread-safe.
+ */
+public final class XmlChars {
+
+    private XmlChars() {}
+
+    /**
+     * @param cp Unicode codepoint
+     * @return true if {@code cp} is allowed inside an XML 1.0 attribute value
+     *         (and additionally not {@code U+FFFD}, not an unpaired surrogate,
+     *         not a non-character codepoint).
+     */
+    public static boolean isValidXml10Char(int cp) {
+        if (cp < 0x20) {
+            return cp == 0x09 || cp == 0x0A || cp == 0x0D;
+        }
+        if (cp >= 0xD800 && cp <= 0xDFFF) return false;            // unpaired surrogates
+        if (cp == 0xFFFD) return false;                             // replacement char (project policy)
+        if (cp >= 0xFDD0 && cp <= 0xFDEF) return false;             // non-characters
+        if ((cp & 0xFFFE) == 0xFFFE) return false;                  // U+nFFFE / U+nFFFF
+        return cp <= 0x10FFFF;
+    }
+
+    /**
+     * Same as {@link #isValidXml10Char(int)} but additionally forbids any
+     * whitespace codepoint. Use for entity names.
+     */
+    public static boolean isValidNameChar(int cp) {
+        if (!isValidXml10Char(cp)) return false;
+        return !Character.isWhitespace(cp);
+    }
+
+    /**
+     * Scans {@code s} for the first invalid char.
+     *
+     * @param nameMode if true, applies {@link #isValidNameChar(int)};
+     *                 else applies {@link #isValidXml10Char(int)}.
+     * @return the {@code char} index (UTF-16 unit) of the first invalid
+     *         char, or {@code -1} if none.
+     */
+    public static int firstInvalidIndex(CharSequence s, boolean nameMode) {
+        if (s == null) return -1;
+        int i = 0;
+        while (i < s.length()) {
+            int cp = Character.codePointAt(s, i);
+            boolean ok = nameMode ? isValidNameChar(cp) : isValidXml10Char(cp);
+            if (!ok) return i;
+            i += Character.charCount(cp);
+        }
+        return -1;
+    }
+
+    /**
+     * Validates {@code value} as an entity name. Throws if any char is
+     * forbidden. The exception message identifies the field, the offset,
+     * the offending codepoint, and a short snippet for context.
+     */
+    public static void requireValidName(String value, String fieldDesc) {
+        require(value, fieldDesc, true);
+    }
+
+    /**
+     * Validates {@code value} as XML attribute content. Throws if any char
+     * is forbidden.
+     */
+    public static void requireValidAttributeContent(String value, String fieldDesc) {
+        require(value, fieldDesc, false);
+    }
+
+    private static void require(String value, String fieldDesc, boolean nameMode) {
+        if (value == null) return;
+        int idx = firstInvalidIndex(value, nameMode);
+        if (idx < 0) return;
+        int cp = Character.codePointAt(value, idx);
+        throw new IllegalArgumentException(format(value, fieldDesc, idx, cp));
+    }
+
+    private static String format(String value, String fieldDesc, int idx, int cp) {
+        StringBuilder snippet = new StringBuilder();
+        int from = Math.max(0, idx - 10);
+        int to = Math.min(value.length(), idx + 10);
+        for (int i = from; i < to; i++) {
+            char c = value.charAt(i);
+            if (c < 0x20 && c != 0x09) {
+                snippet.append(String.format("\\x%02x", (int) c));
+            } else if (c == 0xFFFD) {
+                snippet.append("<U+FFFD>");
+            } else {
+                snippet.append(c);
+            }
+        }
+        return String.format(
+                "invalid character in %s at index %d (codepoint 0x%04X): \"%s\"",
+                fieldDesc, idx, cp, snippet.toString());
+    }
+}