Fix/356 shadow copy

[mxamin]

No description provided.

[Copilot]

🟡 Changes recommended

ID replay can resolve unintended nodes, live registrations can be evicted, and shadow mode introduces root-operation API regressions.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Introduces shadow copies to isolate lxml nodes from ABI-incompatible libxml2 instances.

Changes:

• Adds shadow creation, reflection, and ID registration infrastructure.
• Routes template, tree, signature, and encryption operations through shadows.
• Adds documentation, regression tests, source auditing, and forced-shadow CI runs.

File summaries

File	Description
src/lxml.c	Implements shadow copying, reflection, and ID replay.
src/lxml.h	Declares the shadow API.
src/template.c	Converts template operations to shadows.
src/tree.c	Converts tree searches and ID registration.
src/ds.c	Converts signing and verification.
src/enc.c	Converts encryption and decryption.
tests/test_templates.py	Tests reflected template mutations.
tests/test_shadow_audit.py	Audits shadow invariants.
tests/test_enc.py	Tests reflected content decryption.
tests/test_ds.py	Tests registered-ID signing.
developer.md	Documents the architecture and limitations.
.github/workflows/macosx.yml	Adds forced-shadow testing.
.github/workflows/linuxbrew.yml	Adds forced-shadow testing.
.github/scripts/manylinux_build_and_test.sh	Adds forced-shadow wheel testing.

Review details

Suppressed comments (2)

src/lxml.c:573

• Replaying an ID spec from the document root changes the scope of both APIs: register_id(node, ...) is supposed to register only that node, and add_ids(node, ...) only that subtree. Here, an earlier element elsewhere in the document with the same attribute/value can win xmlGetID, so a #id signature may resolve or verify against a node the caller never registered. Store the registered node/subtree path with each spec and replay only within that scope, preserving duplicate-ID errors.

// Registers every attribute named `name` (under `ns` when given) in the copy
// as an XML ID — a superset of the fast path's registrations (single node for
// register_id, subtree for add_ids), which is the safe direction: it mirrors
// what xmlSecAddIDs does from the root.
static void PyXmlSec_LxmlShadowApplyIdSpec(xmlDocPtr doc, xmlNodePtr n, const xmlChar* name, const xmlChar* ns) {

src/enc.c:592

• A root EncryptedData produced with Type=Element decrypts successfully on the existing path but is unconditionally rejected in shadow mode. The replacement is an lxml-owned parsed element and can be installed with the document tree's _setroot() API; otherwise decrypt behavior unexpectedly depends on how libxml2 was linked.

    if (parent == Py_None) {
        // Decryption replaced the document root; lxml's API offers no way to
        // swap a document's root element, so this cannot be reflected (the
        // live tree is untouched). Re-parse the document into a wrapper or
        // decrypt a non-root EncryptedData instead.
        PyXmlSec_LxmlShadowDiscard(&shadow);
        PyErr_SetString(PyXmlSec_Error,
            "decrypting the document root is not supported when lxml and xmlsec use different libxml2 libraries");

• Files reviewed: 14/14 changed files
• Comments generated: 4
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

ID replay introduces a security-sensitive resolution error, while reflection and node cleanup contain correctness and memory-safety defects.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 13/13 changed files
• Comments generated: 4
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Shadow parsing, ID-registry lifetime, and partial-registration paths contain unresolved correctness issues.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 13/13 changed files
• Comments generated: 5
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Shadow reflection misses existing text updates, attached templates diverge between modes, and duplicate IDs can resolve to the wrong element.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/lxml.c:984

• The shadow replay does not implement the fast path's duplicate-ID check. If two nodes with the same value are registered (for example, register_id(decoy, 'ID') followed by register_id(real, 'ID')), the fast path raises duplicated id., but replay silently keeps the first xmlGetID() entry and signs/verifies against it. That mode-dependent behavior can redirect a #id reference to the wrong element. Make replay fail when the existing ID belongs to a different attribute and propagate that failure from BeginDoc.

    if (xmlGetID(doc, value) == NULL) {
        xmlAddID(NULL, doc, value, attr);
    }

• Files reviewed: 14/14 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Shadow ID replay can silently resolve duplicate ID values to the wrong element.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/enc.c:616

• The successful shadow reflection path for encrypt_uri is not covered. The existing test_encrypt_uri calls ctx.encrypt_binary(...), so only the new failure path reaches encrypt_uri; a regression that fails to copy the generated CipherValue back would pass the suite even under PYXMLSEC_FORCE_SHADOW. Update that test to invoke encrypt_uri and assert the reflected value.

• Files reviewed: 14/14 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Shadow mode still has ID-identity parity bugs, external-DTD handling gaps, and unbounded recursive traversals.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

src/lxml.c:1017

• Element identity is not enough to identify the attribute already registered as an ID. For <A xml:id="dup" ID="dup"/>, register_id(A, "ID") must raise duplicated id. because xmlGetID() points to the distinct xml:id attribute; this check sees the same element and accepts the registration, after which replay silently leaves xml:id as the winner. Compare the requested attribute's identity, not only its owning element.

        // lxml hands out one proxy per node, so identity is node identity.
        taken = match != (PyObject*)element;

src/lxml.c:1071

• This treats attributes with the same local name as the same attribute even when id_ns selects different namespaces. Registering {urn:a}Id="dup" and then {urn:b}Id="dup" on one element is accepted here as an idempotent registration, while the fast path sees distinct xmlAttrPtrs and raises duplicated id.; replay then silently keeps the first attribute. Include the selected namespace/attribute identity in the mine comparison.

        // The same attribute of the same element: registering it again is
        // the no-op the fast path performs, whoever recorded it first. The
        // names alone decide it, as xmlHasProp's own matching does.
        mine = strcmp(spec_name, name) == 0;

• Files reviewed: 14/14 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Shadow mode still has ID-collision parity bugs, loses external-DTD ID typing, and can overflow the C stack on deeply nested trees.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

src/lxml.c:355

• The shadow parser accepts huge_tree inputs, but this recursive count (and the later recursive TagNodes/CollectSites walks) has no depth guard. On libxml2 versions where XML_PARSE_HUGE removes the nesting cap, a deeply nested programmatically built tree can overflow the C stack before the 256-step path code gets a chance to reject it. Make these traversals iterative or enforce a depth limit before every recursive descent.

static int PyXmlSec_LxmlShadowCountNodes(xmlNodePtr node) {
    int count = 0;
    for (; node != NULL; node = node->next) {
        count += 1 + PyXmlSec_LxmlShadowCountNodes(node->children);
    }

src/lxml.c:251

• This parse never loads an external DTD subset. If the source was parsed with an external DTD that declares an ID attribute, serialization keeps the SYSTEM doctype but the private document loses the ID typing, so #id sign/verify can succeed on the fast path and fail in shadow mode. Carry whether lxml loaded an external subset and parse the copy with XML_PARSE_DTDLOAD while retaining the source URL and XML_PARSE_NONET, or otherwise replay those declared IDs.

#define PYXMLSEC_SHADOW_PARSE_OPTIONS (XML_PARSE_NONET | XML_PARSE_HUGE)

src/lxml.c:1071

• This treats equal local names as the same attribute even when register_id supplied an explicit namespace. If one element has a:Id="dup" and b:Id="dup", registering the a attribute and then the b attribute is accepted here as an idempotent registration, whereas the fast path selects two distinct attributes with xmlHasNsProp and raises duplicated id.. Pass the requested namespace into this check and require namespace equality when it is explicit.

        // The same attribute of the same element: registering it again is
        // the no-op the fast path performs, whoever recorded it first. The
        // names alone decide it, as xmlHasProp's own matching does.
        mine = strcmp(spec_name, name) == 0;

• Files reviewed: 14/14 changed files
• Comments generated: 1
• Review effort level: Balanced