fix: breaking/missing svg nodes

Author: haditariq

app/javascript/src/components/structureEditor/StructureEditorModal.js

@@ -278,7 +278,7 @@ export default class StructureEditorModal extends React.Component {
 
   async handleSaveStructureKet2(structure, editor) {
     try {
-      const molfile = await structure.editor.getMolfile();
+      const molfile = await structure.editor.getMolfile('V2000');
       const imgfile = await structure.editor.generateImage(molfile, { outputFormat: 'svg' });
       const text = await imgfile.text();
       const updatedSvg = await transformSvgIdsAndReferences(text);

app/javascript/src/utilities/SvgUtils.js

@@ -18,6 +18,10 @@ const transformSvgIdsAndReferences = async (svgText) => {
   elementsWithId.forEach((el) => {
     const originalId = el.getAttribute('id');
     if (mappedIdPattern.test(originalId)) return svgText;
+
+    if (!originalId.includes('glyph')) return;
+
+    // Otherwise, add the suffix
     const uniqueId = `${originalId}_${uniqueSuffix}`;
     idMap[originalId] = uniqueId;
     el.setAttribute('id', uniqueId);
@@ -35,4 +39,4 @@ const transformSvgIdsAndReferences = async (svgText) => {
   return svg;
 };
 
-export { transformSvgIdsAndReferences, mappedIdPattern };
+export { transformSvgIdsAndReferences, mappedIdPattern };

lib/chemotion/sanitizer.rb

@@ -28,6 +28,8 @@ def base_scrub_ml(fragment, type: :xml, encoding: nil, remap_glyph_ids: false)
                    Loofah.scrub_xml_fragment(result, :strip)
                  when :html
                    Loofah.scrub_html5_fragment(result, :strip)
+                 when :svg
+                   Chemotion::SvgSanitizer.sanitize(result)
                  else
                    Loofah.scrub_fragment(result, :strip)
                  end.to_s
@@ -85,9 +87,8 @@ def map_defs_ids
       @current_node.xpath('svg:defs//svg:g[@id]', svg_namespace).each do |element|
         # Check if the element has an id attribute or skip if it has a unique id ending
         # (from SecureRandom.hex(4))
-        next if !element['id'] || element['id'].match?(/_[0-9a-f]{8}$/)
+        next if !element['id'] || element['id'].exclude?('glyph') || element['id'].match?(/_[0-9a-f]{8}$/)
 
-        # Generate a new id, store the mapping, and update the element's id
         new_id = "#{element['id']}_#{SecureRandom.hex(4)}"
         @id_map[element['id']] = new_id
         element['id'] = new_id

lib/chemotion/svg_sanitizer.rb

@@ -0,0 +1,108 @@
+# frozen_string_literal: true
+
+require 'nokogiri'
+
+module Chemotion
+  class SvgSanitizer
+    BLOCKED_TAGS = %w[
+      script foreignobject iframe embed object audio video style link meta
+    ].freeze
+
+    BLOCKED_ATTRIBUTES = %w[
+      onabort onclick ondblclick onerror onfocus onkeydown onkeypress
+      onkeyup onload onmousedown onmousemove onmouseout onmouseover
+      onmouseup onreset onresize onscroll onselect onsubmit onunload
+      onchange oninput onanimationstart onanimationend onanimationiteration
+      onblur oncancel oncanplay oncontextmenu
+    ].freeze
+
+    BLOCKED_SCHEMES = %w[javascript vbscript livescript mocha data file about mhtml].freeze
+
+    class << self
+      def sanitize(svg_string)
+        doc = parse_svg(svg_string)
+        clean_document(doc)
+        format_output(doc)
+      end
+
+      private
+
+      def parse_svg(svg_string)
+        Nokogiri::XML(svg_string, &:recover)
+      end
+
+      def clean_document(doc)
+        doc.traverse { |node| process_node(node) }
+      end
+
+      def process_node(node)
+        return unless node.element?
+
+        return node.remove if blocked_tag?(node)
+
+        clean_attributes(node)
+      end
+
+      def blocked_tag?(node)
+        BLOCKED_TAGS.include?(node.name.downcase)
+      end
+
+      def clean_attributes(node)
+        node.attribute_nodes.each { |attr| check_and_clean_attribute(node, attr) }
+      end
+
+      def check_and_clean_attribute(node, attr)
+        attribute = AttributeValidator.new(attr)
+        return unless attribute.valid?
+
+        remove_if_dangerous(node, attribute)
+      end
+
+      def remove_if_dangerous(node, attribute)
+        node.remove_attribute(attribute.name) if attribute.dangerous?
+      end
+
+      def format_output(doc)
+        doc.to_xml(save_with: Nokogiri::XML::Node::SaveOptions::NO_DECLARATION)
+      end
+    end
+
+    # Encapsulates attribute validation logic
+    class AttributeValidator
+      attr_reader :name, :value
+
+      def initialize(attr)
+        @name = attr.name.downcase
+        @value = attr.value.to_s.strip
+      end
+
+      def valid?
+        name && value
+      end
+
+      def dangerous?
+        dangerous_name? || dangerous_value?
+      end
+
+      private
+
+      def dangerous_name?
+        name.start_with?('on') || BLOCKED_ATTRIBUTES.include?(name)
+      end
+
+      def dangerous_value?
+        dangerous_scheme? || contains_javascript?
+      end
+
+      def dangerous_scheme?
+        return false unless value =~ /\A([a-z0-9+-]+):/i
+
+        BLOCKED_SCHEMES.include?(::Regexp.last_match(1).downcase)
+      end
+
+      def contains_javascript?
+        value.match?(/\bjavascript\s*:/i)
+      end
+    end
+  end
+end

spec/lib/chemotion/sanitizer_spec.rb

@@ -103,6 +103,47 @@
       expect(result).to eq(svg_reaction_remapped)
     end
   end
+
+  describe 'scrub_svg with dangerous tags' do
+    it 'removes <script> tags' do
+      svg = '<svg><script>alert("xss")</script><circle/></svg>'
+      sanitized = sanitizer.scrub_svg(svg)
+      expect(sanitized).not_to include('<script>')
+      expect(sanitized).to include('<circle')
+    end
+
+    it 'removes <iframe> tags' do
+      svg = '<svg><iframe src="https://malicious.com"/></svg>'
+      sanitized = sanitizer.scrub_svg(svg)
+      expect(sanitized).not_to include('<iframe>')
+    end
+
+    it 'removes <embed> tags' do
+      svg = '<svg><embed src="evil.swf"/></svg>'
+      sanitized = sanitizer.scrub_svg(svg)
+      expect(sanitized).not_to include('<embed>')
+    end
+
+    it 'removes onclick attributes' do
+      svg = '<svg><circle onclick="alert(1)"/></svg>'
+      sanitized = sanitizer.scrub_svg(svg)
+      expect(sanitized).not_to include('onclick')
+      expect(sanitized).to include('<circle')
+    end
+
+    it 'removes javascript: URLs' do
+      svg = '<svg><a href="javascript:alert(1)">Click</a></svg>'
+      sanitized = sanitizer.scrub_svg(svg)
+      expect(sanitized).not_to include('javascript:')
+    end
+
+    it 'keeps safe SVG elements and attributes' do
+      svg = '<svg viewBox="0 0 100 100"><circle cx="50" cy="50" r="40" fill="red"/></svg>'
+      sanitized = sanitizer.scrub_svg(svg)
+      expect(sanitized).to include('<circle')
+      expect(sanitized).to include('fill="red"')
+    end
+  end
 end
 # rubocop:enable RSpec/MultipleMemoizedHelpers
 # rubocop:enable Rspec/IndexedLet