Refuse a duplicate id at register_id under the shadow (#356)

One finding from the review of the shadow path.

`register_id` raises "duplicated id." on the raw path when the id value it
is asked to register is already registered for another attribute, and
under the shadow it silently recorded the spec instead: the replay's
first-wins rule then skipped it, so the call succeeded while the caller's
`#id` reference resolved to whichever element claimed the value first —
an earlier registration, a DTD-declared id attribute or an xml:id.

The check is back at the call that makes the registration, where the fast
path has it, rather than at the replay: raising from a later sign/verify
would report the collision from the wrong call and then from every later
call on that document. `xmlGetID(doc, value) != attr` is assembled from
the two places a registration can live under the shadow — what lxml's own
parse declared, read back through XPath's id(), which crosses the library
boundary as strings and elements only, and what earlier register_id or
add_ids calls recorded in the registry. Re-registering the same attribute
stays the no-op the fast path performs, and add_ids keeps xmlSecAddIDs'
own first-wins semantics, which never raise.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: mxamin

developer.md

@@ -71,7 +71,7 @@ End always releases the copy, on success and on error.
 
 Plus three helpers for the call sites whose *semantics* differ per mode:
 `IsActive()` (which path is on), `ImportElement()` (encrypt_xml's template
-import into the shadow document) and `RecordId()` (ID registration, below).
+import into the shadow document) and `RegisterId()` (ID registration, below).
 
 Which pair a binding uses follows from what the xmlsec call does:
 
@@ -201,7 +201,7 @@ lxml's own dump of a tree it already parsed.
 `SignatureContext.register_id` and `tree.add_ids` used to write lxml's ID
 hash with our libxml2 — exactly the cross-library access the shadow forbids.
 Under the shadow they record the id-attribute specs in a registry keyed by
-document identity (`RecordId`), and every `BeginDoc` replays them onto its
+document identity (`RegisterId`, `RecordIds`), and every `BeginDoc` replays them onto its
 copy so that `#id` references resolve during sign/verify/decrypt. An entry
 keeps the registered elements themselves, so a spec is replayed at exactly
 the node it was registered for — that node alone for `register_id`, its
@@ -219,6 +219,19 @@ registration. The registry therefore tracks the documents still in use and
 never evicts a live one. The two bindings are the only places, together with
 encrypt_xml/decrypt's replacement bodies, that branch on `IsActive()`.
 
+`register_id` still refuses a duplicate id the way the fast path does, and at
+the same call: `xmlGetID(doc, value) != attr` — the test that raises
+`duplicated id.` — is assembled from the two places a registration can live
+under the shadow. What lxml's own parse declared (a DTD id attribute, an
+`xml:id`) is read back through XPath's `id()`, the one door into lxml's id
+hash that passes nothing but strings and elements; what earlier
+`register_id`/`add_ids` calls claimed is read from the registry, a subtree
+spec through one XPath over its scope. Deferring the check to the replay
+instead would raise from the wrong call — a later `sign`, and then from every
+later call on that document — and would leave the caller believing a
+registration took that can never win the lookup. `add_ids` keeps its own
+semantics: `xmlSecAddIDs` registers first-wins and never raises.
+
 ## Converting a binding
 
 1. Classify the xmlsec call with the table above.
@@ -246,7 +259,11 @@ All invisible to the documented API:
   document until grafted;
 - `encrypted_data_ensure_key_info(ns=...)` on an existing `KeyInfo` returns
   a new element object rather than the original proxy;
-- `register_id` skips the live duplicate-id check (it runs per copy instead);
+- `register_id`'s duplicate-id check cannot see an id value that contains
+  whitespace among lxml's declared ids (XPath's `id()` would read it as a
+  list of ids), so such a value is compared against the registry alone; a
+  registration whose element has since been adopted into another document
+  claims nothing, as at replay;
 - `encrypt_xml` encrypts a *copy* of the template, so the caller's template
   proxy is not the returned element; a template attached in the target's own
   document is unlinked afterwards (keeping its tail text where libxml2 would

src/ds.c

@@ -119,50 +119,6 @@ static int PyXmlSec_SignatureContextKeySet(PyObject* self, PyObject* value, void
     return 0;
 }
 
-// Non-zero when `node` carries an attribute whose *local* name is `name`,
-// whatever its namespace, -1 with an exception set on failure. The fast
-// path's xmlHasProp() matches that way, while lxml's node.get(name) only
-// finds an unqualified attribute; the shadow's validation goes through this
-// so that both modes accept the same calls.
-static int PyXmlSec_LxmlHasAttrByLocalName(PyXmlSec_LxmlElementPtr node, const char* name) {
-    PyObject* keys;
-    PyObject* seq;
-    Py_ssize_t i;
-    Py_ssize_t n;
-    int found = 0;
-
-    keys = PyObject_CallMethod((PyObject*)node, "keys", NULL);
-    if (keys == NULL) {
-        return -1;
-    }
-    seq = PySequence_Fast(keys, "unexpected attribute names.");
-    Py_DECREF(keys);
-    if (seq == NULL) {
-        return -1;
-    }
-
-    n = PySequence_Fast_GET_SIZE(seq);
-    for (i = 0; i < n && !found; ++i) {
-        PyObject* key = PySequence_Fast_GET_ITEM(seq, i);  // borrowed
-        const char* local;
-        const char* end;
-        if (!PyUnicode_Check(key) || (local = PyUnicode_AsUTF8(key)) == NULL) {
-            if (!PyErr_Occurred()) {
-                PyErr_SetString(PyXmlSec_InternalError, "unexpected attribute name.");
-            }
-            Py_DECREF(seq);
-            return -1;
-        }
-        // lxml spells a namespaced attribute "{href}local".
-        if (local[0] == '{' && (end = strchr(local, '}')) != NULL) {
-            local = end + 1;
-        }
-        found = strcmp(local, name) == 0;
-    }
-    Py_DECREF(seq);
-    return found;
-}
-
 static const char PyXmlSec_SignatureContextRegisterId__doc__[] = \
     "register_id(node, id_attr = 'ID', id_ns = None) -> None\n"
     "Registers new id.\n\n"
@@ -191,37 +147,11 @@ static PyObject* PyXmlSec_SignatureContextRegisterId(PyObject* self, PyObject* a
     }
 
     // Shadow mode: never touch lxml's document (its ID hash) with our libxml2
-    // (issue #356). Validate through lxml's API and record the spec; the
-    // whole-document shadows (sign/verify/decrypt) replay it onto their
-    // private copies. The duplicate-id check runs there, per copy.
+    // (issue #356). The registration is validated through lxml's API and
+    // recorded instead; the whole-document shadows (sign/verify/decrypt)
+    // replay it onto their private copies.
     if (PyXmlSec_LxmlShadowIsActive()) {
-        int found;
-        if (id_ns != NULL) {
-            PyObject* key = PyUnicode_FromFormat("{%s}%s", id_ns, id_attr);
-            PyObject* value;
-            if (key == NULL) {
-                goto ON_FAIL;
-            }
-            value = PyObject_CallMethod((PyObject*)node, "get", "O", key);
-            Py_DECREF(key);
-            if (value == NULL) {
-                goto ON_FAIL;
-            }
-            found = value != Py_None;
-            Py_DECREF(value);
-        } else {
-            // As xmlHasProp() does on the fast path: by local name.
-            found = PyXmlSec_LxmlHasAttrByLocalName(node, id_attr);
-            if (found < 0) {
-                goto ON_FAIL;
-            }
-        }
-        if (!found) {
-            PyErr_SetString(PyXmlSec_Error, "missing attribute.");
-            goto ON_FAIL;
-        }
-        // Scope 0: this node alone, exactly what the fast path registers.
-        if (PyXmlSec_LxmlShadowRecordId(node, id_attr, id_ns, 0) < 0) {
+        if (PyXmlSec_LxmlShadowRegisterId(node, id_attr, id_ns) < 0) {
             goto ON_FAIL;
         }
         PYXMLSEC_DEBUGF("%p: register id - ok", self);