From 41585757c9256a56d2523fe805dd6b536864af2b Mon Sep 17 00:00:00 2001
From: Umut CAN <umutcan.edizaslan@gmail.com>
Date: Tue, 15 Sep 2026 13:22:56 +0300
Subject: [PATCH] fix(task-2): close the trailing-newline tag hole and gate
 DXF-illegal layer names

Two ways a malformed block_define request slipped past the pre-write
validator and reached the engine:

* `ATTDEF_TAG_RE.match` with a `$` anchor accepts 'TAG\n' because `$`
  matches before a trailing newline. ezdxf strips the newline on write, so
  'TAG\n' + 'TAG' landed as two ATTDEFs with one tag, defeating the
  duplicate-tag refusal, and the tag-keyed attribute readers then dropped
  one silently. Now `fullmatch`.

* `_layer` only required a non-empty string, so a DXF-illegal name such as
  'BAD<LAYER' passed and ezdxf raised DXFValueError mid-loop, after the
  block existed and earlier primitives were written (with overwrite=True,
  after the old definition was already wiped). The gate now refuses on the
  same forbidden-character rule `sanitize_symbol_name` uses, exposed as
  `security.illegal_symbol_name_chars`, with a TypeError naming
  `entities[i]` and 'layer'.

Tests cover both repros and fail against the previous code.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FKFStjEFzpGyLq5bMpWxTd
---
 backends/block_specs.py   | 17 +++++++++++++++--
 security.py               | 13 ++++++++++++-
 tests/test_block_specs.py | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 60 insertions(+), 3 deletions(-)

diff --git a/backends/block_specs.py b/backends/block_specs.py
index 75c352c..629b20b 100644
--- a/backends/block_specs.py
+++ b/backends/block_specs.py
@@ -11,8 +11,12 @@ from __future__ import annotations
 import math
 import re
 
+from security import illegal_symbol_name_chars
+
 ALLOWED_TYPES = ("line", "circle", "arc", "polyline", "text", "solid")
 TEXT_ALIGNMENTS = ("left", "center", "right", "middle_center")
+#: Checked with ``fullmatch`` — ``$`` alone would accept a trailing newline, which
+#: ezdxf then strips on write, turning two "distinct" tags into one duplicate.
 ATTDEF_TAG_RE = re.compile(r"^[A-Z][A-Z0-9_]{0,30}$")
 
 _REQUIRED = {
@@ -73,7 +77,16 @@ def _layer(spec: dict, where: str) -> str:
     layer = spec.get("layer", "0")
     if not isinstance(layer, str) or not layer.strip():
         raise TypeError(f"{where}: 'layer' must be a non-empty string")
-    return layer.strip()
+    layer = layer.strip()
+    # Gate the DXF name rule here rather than letting ezdxf raise DXFValueError
+    # on `add_*(dxfattribs={"layer": ...})` mid-loop, after the block exists and
+    # earlier primitives are already written (and, with overwrite, the old
+    # definition is already gone).
+    offenders = illegal_symbol_name_chars(layer)
+    if offenders:
+        shown = ", ".join(repr(c) for c in offenders)
+        raise TypeError(f"{where}: 'layer' is not a legal DXF layer name (contains {shown})")
+    return layer
 
 
 def validate_entity_specs(entities) -> list[dict]:
@@ -145,7 +158,7 @@ def validate_attdef_specs(attdefs) -> list[dict]:
         if not isinstance(spec, dict):
             raise TypeError(f"{where}: must be an object")
         tag = spec.get("tag")
-        if not isinstance(tag, str) or not ATTDEF_TAG_RE.match(tag):
+        if not isinstance(tag, str) or not ATTDEF_TAG_RE.fullmatch(tag):
             raise TypeError(f"{where}: 'tag' must match {ATTDEF_TAG_RE.pattern}, got {tag!r}")
         if tag in seen:
             raise TypeError(f"{where}: duplicate tag {tag!r}")
diff --git a/security.py b/security.py
index 83819a3..453dd45 100644
--- a/security.py
+++ b/security.py
@@ -384,6 +384,17 @@ _FORBIDDEN_SYMBOL_CHARS = set('<>/\\":;?*|,=`') | set("\n\r\t")
 _MAX_SYMBOL_NAME = 255
 
 
+def illegal_symbol_name_chars(name: str) -> list[str]:
+    """The characters in `name` that DXF forbids in a symbol-table name, sorted.
+
+    The one rule behind every symbol-name gate in this repo: `sanitize_symbol_name`
+    refuses on it for macro interpolation, and pre-write validators (block
+    primitive specs, for one) refuse on it so ezdxf's own `DXFValueError` never
+    fires mid-write after earlier entities have already landed.
+    """
+    return sorted({c for c in name if c in _FORBIDDEN_SYMBOL_CHARS or ord(c) < 32})
+
+
 def sanitize_symbol_name(name: str, *, kind: str = "symbol") -> str:
     """Return `name` if it is a legal DXF symbol-table name, else refuse.
 
@@ -397,7 +408,7 @@ def sanitize_symbol_name(name: str, *, kind: str = "symbol") -> str:
         raise ToolError(f"{kind} name rejected: empty.")
     if len(candidate) > _MAX_SYMBOL_NAME:
         raise ToolError(f"{kind} name rejected: longer than {_MAX_SYMBOL_NAME} characters.")
-    offenders = sorted({c for c in candidate if c in _FORBIDDEN_SYMBOL_CHARS or ord(c) < 32})
+    offenders = illegal_symbol_name_chars(candidate)
     if offenders:
         shown = ", ".join(repr(c) for c in offenders)
         raise ToolError(
diff --git a/tests/test_block_specs.py b/tests/test_block_specs.py
index 16cb4de..3cd6e88 100644
--- a/tests/test_block_specs.py
+++ b/tests/test_block_specs.py
@@ -58,6 +58,9 @@ def test_each_type_normalises_to_floats_and_layer_zero():
         ({"type": "solid", "points": [[0, 0], [1, 0]]}, "points"),
         ({"type": "solid", "points": [[0, 0], [1, 0], [1, 1], [0, 1], [2, 2]]}, "at most 4"),
         ({"type": "line", "x1": 0, "y1": 0, "x2": 1, "y2": 0, "layer": ""}, "layer"),
+        ({"type": "line", "x1": 0, "y1": 0, "x2": 1, "y2": 0, "layer": "BAD<LAYER"}, "layer"),
+        ({"type": "line", "x1": 0, "y1": 0, "x2": 1, "y2": 0, "layer": "A;B"}, "layer"),
+        ({"type": "line", "x1": 0, "y1": 0, "x2": 1, "y2": 0, "layer": "A\x01B"}, "layer"),
         ({"type": "line", "x1": True, "y1": 0, "x2": 1, "y2": 0}, "x1"),
     ],
 )
@@ -68,6 +71,22 @@ def test_bad_entity_names_the_index_and_key(bad, fragment):
     assert fragment in str(exc.value)
 
 
+def test_illegal_layer_name_is_refused_before_any_write():
+    """ezdxf would raise DXFValueError on this layer mid-loop, after earlier
+    primitives (and, with overwrite, the old definition) were already written.
+    The gate has to be ours, typed, and indexed."""
+    with pytest.raises(TypeError, match=r"entities\[0\].*'layer'") as exc:
+        validate_entity_specs(
+            [{"type": "line", "x1": 0, "y1": 0, "x2": 1, "y2": 0, "layer": "BAD<LAYER"}]
+        )
+    assert "'<'" in str(exc.value)
+    # Legal names, including the ones with spaces and dots, still pass.
+    out = validate_entity_specs(
+        [{"type": "line", "x1": 0, "y1": 0, "x2": 1, "y2": 0, "layer": " PID.Valves-1 "}]
+    )
+    assert out[0]["layer"] == "PID.Valves-1"
+
+
 def test_attdefs_normalise_and_default():
     out = validate_attdef_specs([{"tag": "TAG", "x": 0, "y": 3, "height": 2.5}])
     assert out == [
@@ -90,6 +109,7 @@ def test_attdefs_normalise_and_default():
     [
         ({"tag": "tag", "x": 0, "y": 0, "height": 1}, "tag"),
         ({"tag": "T-1", "x": 0, "y": 0, "height": 1}, "tag"),
+        ({"tag": "TAG\n", "x": 0, "y": 0, "height": 1}, "tag"),
         ({"tag": "TAG", "x": 0, "y": 0, "height": 0}, "height"),
         ({"tag": "TAG", "x": 0, "y": 0, "height": 1, "invisible": 1}, "invisible"),
         ({"tag": "TAG", "x": 0, "y": 0, "height": 1, "default": 3}, "default"),
@@ -111,6 +131,19 @@ def test_duplicate_attdef_tags_are_refused():
         )
 
 
+def test_trailing_newline_cannot_smuggle_a_duplicate_tag():
+    """`$` matches before a trailing newline; ezdxf strips it on write, so
+    'TAG\n' + 'TAG' would land as two ATTDEFs with the same tag and the
+    attribute readers (dict keyed by tag) would silently drop one."""
+    with pytest.raises(TypeError, match=r"attdefs\[0\].*'tag'"):
+        validate_attdef_specs(
+            [
+                {"tag": "TAG\n", "x": 0, "y": 0, "height": 1},
+                {"tag": "TAG", "x": 0, "y": 0, "height": 1},
+            ]
+        )
+
+
 def test_solid_vertices_use_dxf_bowtie_order():
     quad = [(0.0, 0.0), (1.0, 0.0), (1.0, 1.0), (0.0, 1.0)]
     assert solid_vertices(quad) == [(0.0, 0.0), (1.0, 0.0), (0.0, 1.0), (1.0, 1.0)]