Skip to content

Commit 38174b9

Browse files
mxaminclaude
andcommitted
Replay id registrations at the node they were registered for (#356)
The shadow's id registry recorded only the attribute name and namespace, so the replay registered every matching attribute of the private copy. That is not a harmless superset: an element the caller never registered, sharing the id value and coming first, claims the value at xmlGetID and the intended element is skipped — a "#id" signature reference then resolves to content the caller never vouched for. The registry now keeps the registered elements themselves, and the replay applies each spec at the copy's counterpart of its element: that node alone for register_id, its subtree for add_ids (the scope xmlSecAddIDs walks). Liveness stays decidable without weak references — every element proxy holds a reference to its document, so an entry is dead when the document's reference count is exactly what the registry holds and nothing else holds its elements. Two other faithfulness gaps in the copy, from the same review: - the whole-document and subtree parses dropped the source document's base URI; they now carry docinfo.URL, so relative references resolve where they did before the copy. - the reflection recorded a text-slot sync only for a parent that gained a fresh node, so a call that *removed* a node and left nothing fresh behind (an EncryptedData decrypting to empty content) went unnoticed. Each pre-existing node is now tagged with its child count, and a parent whose tagged children changed is synced too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 97cb993 commit 38174b9

6 files changed

Lines changed: 437 additions & 136 deletions

File tree

developer.md

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -194,17 +194,22 @@ lxml's own dump of a tree it already parsed.
194194
hash with our libxml2 — exactly the cross-library access the shadow forbids.
195195
Under the shadow they record the id-attribute specs in a registry keyed by
196196
document identity (`RecordId`), and every `BeginDoc` replays them onto its
197-
copy so that `#id` references resolve during sign/verify/decrypt. The replay
198-
scans the whole copy for the recorded attribute names — a superset of the
199-
single-node registration on the raw path, mirroring what `xmlSecAddIDs` does
200-
from the root. lxml's classes refuse weak references, so an entry keeps a
201-
strong reference to its document instead: the key (the document's address)
202-
can then never go stale, and a document referenced by nothing but the
203-
registry is provably unreachable, so its entry — and the document with it —
204-
is dropped before the next registration. The registry therefore tracks the
205-
documents still in use and never evicts a live one. The two bindings are the only places,
206-
together with encrypt_xml/decrypt's replacement bodies, that branch on
207-
`IsActive()`.
197+
copy so that `#id` references resolve during sign/verify/decrypt. An entry
198+
keeps the registered elements themselves, so a spec is replayed at exactly
199+
the node it was registered for — that node alone for `register_id`, its
200+
subtree for `add_ids`, the scope `xmlSecAddIDs` walks. Registering every
201+
matching attribute of the copy instead would be unsafe, not merely generous:
202+
an unrelated element sharing the id value would claim it first and a `#id`
203+
reference could then resolve to content the caller never registered. lxml's
204+
classes refuse weak references, so the entry keeps strong references (to the
205+
document and to those elements) instead: the key (the document's address) can
206+
then never go stale, and since every element proxy holds a reference to its
207+
document, a document whose reference count is exactly what the registry holds
208+
— and whose registered elements nothing else holds — is provably unreachable,
209+
so its entry, and the document with it, is dropped before the next
210+
registration. The registry therefore tracks the documents still in use and
211+
never evicts a live one. The two bindings are the only places, together with
212+
encrypt_xml/decrypt's replacement bodies, that branch on `IsActive()`.
208213

209214
## Converting a binding
210215

src/ds.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,8 @@ static PyObject* PyXmlSec_SignatureContextRegisterId(PyObject* self, PyObject* a
220220
PyErr_SetString(PyXmlSec_Error, "missing attribute.");
221221
goto ON_FAIL;
222222
}
223-
if (PyXmlSec_LxmlShadowRecordId(node, id_attr, id_ns) < 0) {
223+
// Scope 0: this node alone, exactly what the fast path registers.
224+
if (PyXmlSec_LxmlShadowRecordId(node, id_attr, id_ns, 0) < 0) {
224225
goto ON_FAIL;
225226
}
226227
PYXMLSEC_DEBUGF("%p: register id - ok", self);

0 commit comments

Comments
 (0)