Skip to content

Commit 7e86465

Browse files
tvdijentvdijen
authored
Merge pull request #75 from ioigoume/xpath-autoregister-optional
xpath-autoregister-optional
2 parents 8864d8a + fa5e291 commit 7e86465

File tree

5 files changed

+191
-26
lines changed

5 files changed

+191
-26
lines changed

‎src/XPath/XPath.php‎

Lines changed: 53 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,29 @@
2020
*/
2121
class XPath
2222
{
23+
/**
24+
* Search for an element with a certain name among the children of a reference element.
25+
*
26+
* @param \DOMNode $ref The DOMDocument or DOMElement where encrypted data is expected to be found as a child.
27+
* @param string $name The name (possibly prefixed) of the element we are looking for.
28+
*
29+
* @return \DOMElement|false The element we are looking for, or false when not found.
30+
*
31+
* @throws \RuntimeException If no DOM document is available.
32+
*/
33+
public static function findElement(DOMNode $ref, string $name): DOMElement|false
34+
{
35+
$doc = $ref instanceof DOMDocument ? $ref : $ref->ownerDocument;
36+
if ($doc === null) {
37+
throw new RuntimeException('Cannot search, no DOMDocument available');
38+
}
39+
40+
$nodeset = self::getXPath($doc)->query('./' . $name, $ref);
41+
42+
return $nodeset->item(0) ?? false;
43+
}
44+
45+
2346
/**
2447
* Get an instance of DOMXPath associated with a DOMNode
2548
*
@@ -30,9 +53,10 @@ class XPath
3053
* custom prefixes declared anywhere up the tree can be used in queries.
3154
*
3255
* @param \DOMNode $node The associated node
56+
* @param bool $autoregister Whether to auto-register all namespaces used in the document
3357
* @return \DOMXPath
3458
*/
35-
public static function getXPath(DOMNode $node): DOMXPath
59+
public static function getXPath(DOMNode $node, bool $autoregister = false): DOMXPath
3660
{
3761
static $xpCache = null;
3862

@@ -53,8 +77,10 @@ public static function getXPath(DOMNode $node): DOMXPath
5377
// Enrich with ancestor-declared prefixes for this document context.
5478
$prefixToUri = self::registerAncestorNamespaces($xpCache, $node);
5579

56-
// Single, bounded subtree scan to pick up descendant-only declarations.
57-
self::registerSubtreePrefixes($xpCache, $node, $prefixToUri);
80+
if ($autoregister) {
81+
// Single, bounded subtree scan to pick up descendant-only declarations.
82+
self::registerSubtreePrefixes($xpCache, $node, $prefixToUri);
83+
}
5884

5985
return $xpCache;
6086
}
@@ -142,19 +168,35 @@ private static function registerSubtreePrefixes(DOMXPath $xp, DOMNode $node, arr
142168
return;
143169
}
144170

145-
$visited = 0;
171+
// $visited = 0;
146172

147-
/** @var array<\DOMElement> $queue */
148-
$queue = [$root];
173+
/** @var array<array{0:\DOMElement,1:int}> $queue */
174+
$queue = [[$root, 0]];
149175

150176
while ($queue) {
151177
/** @var \DOMElement $el */
152-
$el = array_shift($queue);
153-
154-
if (++$visited > C_XML::UNBOUNDED_LIMIT) {
155-
throw new \RuntimeException(__METHOD__ . ': exceeded subtree traversal limit');
178+
/** @var int $depth */
179+
[$el, $depth] = array_shift($queue);
180+
181+
// Depth guard: cap traversal at UNBOUNDED_LIMIT (root = depth 0).
182+
// Breaking here halts further descent to avoid pathological depth and excessive work,
183+
// which is safer in production than risking runaway traversal or hard failures.
184+
// Trade-off: deeper descendant-only prefixes may remain unregistered, so some
185+
// prefixed XPath queries might fail; overall processing continues gracefully.
186+
if ($depth >= C_XML::UNBOUNDED_LIMIT) {
187+
break;
156188
}
157189

190+
// if (++$visited > C_XML::UNBOUNDED_LIMIT) {
191+
// // Safety valve: stop further traversal to avoid unbounded work and noisy exceptions.
192+
// // Returning here halts namespace registration for this subtree, which is safer in
193+
// // production than risking pathological O(n) behavior or a hard failure (e.g. throwing
194+
// // \RuntimeException(__METHOD__ . ': exceeded subtree traversal limit')).
195+
// // Trade-off: some descendant-only prefixes may remain unregistered, so related XPath
196+
// // queries might fail, but overall processing continues gracefully.
197+
// break;
198+
// }
199+
158200
// Element prefix
159201
if ($el->prefix && !isset($prefixToUri[$el->prefix])) {
160202
$uri = $el->namespaceURI;
@@ -189,7 +231,7 @@ private static function registerSubtreePrefixes(DOMXPath $xp, DOMNode $node, arr
189231
// Enqueue children (only DOMElement to keep types precise)
190232
foreach ($el->childNodes as $child) {
191233
if ($child instanceof DOMElement) {
192-
$queue[] = $child;
234+
$queue[] = [$child, $depth + 1];
193235
}
194236
}
195237
}

‎tests/XML/SchemaValidatableElementTraitTest.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44

55
namespace SimpleSAML\Test\XML;
66

7-
use DOMDocument;
7+
use PHPUnit\Framework\Attributes\DoesNotPerformAssertions;
88
use PHPUnit\Framework\TestCase;
99
use SimpleSAML\Test\Helper\Base64BinaryElement;
1010
use SimpleSAML\Test\Helper\BooleanElement;
@@ -20,13 +20,13 @@
2020
*/
2121
final class SchemaValidatableElementTraitTest extends TestCase
2222
{
23+
#[DoesNotPerformAssertions]
2324
public function testSchemaValidationPasses(): void
2425
{
2526
$file = 'tests/resources/xml/ssp_StringElement.xml';
2627
$chunk = DOMDocumentFactory::fromFile($file);
2728

2829
$document = StringElement::schemaValidate($chunk);
29-
$this->assertInstanceOf(DOMDocument::class, $document);
3030
}
3131

3232

‎tests/XML/TypedTextContentTraitTest.php‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
namespace SimpleSAML\Test\XML;
66

7+
use PHPUnit\Framework\Attributes\DoesNotPerformAssertions;
78
use PHPUnit\Framework\TestCase;
89
use SimpleSAML\Test\Helper\Base64BinaryElement;
910
use SimpleSAML\Test\Helper\BooleanElement;
@@ -19,6 +20,7 @@
1920
*/
2021
final class TypedTextContentTraitTest extends TestCase
2122
{
23+
#[DoesNotPerformAssertions]
2224
public function testTypedContentPassesForString(): void
2325
{
2426
$file = 'tests/resources/xml/ssp_StringElement.xml';
@@ -27,10 +29,10 @@ public function testTypedContentPassesForString(): void
2729
$elt = $doc->documentElement;
2830

2931
$stringElt = StringElement::fromXML($elt);
30-
$this->assertInstanceOf(StringElement::class, $stringElt);
3132
}
3233

3334

35+
#[DoesNotPerformAssertions]
3436
public function testTypedContentPassesForBoolean(): void
3537
{
3638
$file = 'tests/resources/xml/ssp_BooleanElement.xml';
@@ -39,7 +41,6 @@ public function testTypedContentPassesForBoolean(): void
3941
$elt = $doc->documentElement;
4042

4143
$stringElt = BooleanElement::fromXML($elt);
42-
$this->assertInstanceOf(BooleanElement::class, $stringElt);
4344
}
4445

4546

‎tests/XPath/XPathTest.php‎

Lines changed: 110 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,7 @@ public function testNormalizationFromAttributeNode(): void
186186
$this->assertInstanceOf(\DOMElement::class, $elt);
187187

188188
$attr = $elt->getAttributeNodeNS('urn:bar', 'attr');
189-
$this->assertInstanceOf(\DOMAttr::class, $attr);
189+
/** @var \DOMAttr $attr */
190190

191191
// getXPath should normalize from DOMAttr to the element and ensure 'bar' is registered.
192192
$xp = XPath::getXPath($attr);
@@ -305,8 +305,26 @@ public static function xmlVariantsProviderForTopLevelSlatePerson(): array
305305
$base = dirname(__FILE__, 3) . '/tests/resources/xml';
306306

307307
return [
308-
"Register Subtree Prefixes" => [$base . '/success_response_a.xml'],
309-
"Register Ancestor Namespaces" => [$base . '/success_response_b.xml'],
308+
"Ancestor-declared 'slate'; top-level person AFTER attributes" => [
309+
$base . '/success_response_a.xml',
310+
false,
311+
false,
312+
],
313+
"Ancestor-declared 'slate'; top-level person BEFORE attributes" => [
314+
$base . '/success_response_b.xml',
315+
false,
316+
false,
317+
],
318+
"Descendant-only 'slate'; no ancestor binding (fails without autoregister)" => [
319+
$base . '/success_response_c.xml',
320+
false,
321+
true,
322+
],
323+
"Descendant-only 'slate'; no ancestor binding (succeeds with autoregister)" => [
324+
$base . '/success_response_c.xml',
325+
true,
326+
false,
327+
],
310328
];
311329
}
312330

@@ -316,21 +334,102 @@ public static function xmlVariantsProviderForTopLevelSlatePerson(): array
316334
* cas:attributes in the document, even when the slate prefix is only declared on the element itself.
317335
*/
318336
#[DataProvider('xmlVariantsProviderForTopLevelSlatePerson')]
319-
public function testAbsoluteXPathFindsTopLevelSlatePerson(string $filePath): void
320-
{
337+
public function testAbsoluteXPathFindsTopLevelSlatePerson(
338+
string $filePath,
339+
bool $autoregister,
340+
bool $shouldFail,
341+
): void {
321342
$doc = DOMDocumentFactory::fromFile($filePath);
322343

323344
$fooNs = 'https://example.org/foo';
324-
/** @var \DOMElement|null $authn */
325-
$authn = $doc->getElementsByTagNameNS($fooNs, 'authenticationSuccess')->item(0);
326-
$this->assertNotNull($authn, 'authenticationSuccess element not found');
345+
/** @var \DOMElement|null $attributesNode */
346+
$attributesNode = $doc->getElementsByTagNameNS($fooNs, 'attributes')->item(0);
347+
$this->assertNotNull($attributesNode, 'Attributes element not found');
327348

328-
$xp = XPath::getXPath($authn);
349+
$xp = XPath::getXPath($attributesNode, $autoregister);
329350
$query = '/foo:serviceResponse/foo:authenticationSuccess/slate:person';
330351

331-
$nodes = XPath::xpQuery($authn, $query, $xp);
352+
if ($shouldFail) {
353+
libxml_use_internal_errors(true);
354+
try {
355+
$this->expectException(\SimpleSAML\Assert\AssertionFailedException::class);
356+
$this->expectExceptionMessage('Malformed XPath query or invalid contextNode provided.');
357+
XPath::xpQuery($attributesNode, $query, $xp);
358+
} finally {
359+
$errors = libxml_get_errors();
360+
$this->assertNotEmpty($errors);
361+
$this->assertSame("Undefined namespace prefix\n", $errors[0]->message);
362+
libxml_clear_errors();
363+
libxml_use_internal_errors(false);
364+
}
365+
return;
366+
}
332367

333-
$this->assertSame(1, count($nodes), 'Expected exactly one top-level slate:person');
368+
$nodes = XPath::xpQuery($attributesNode, $query, $xp);
369+
$this->assertCount(1, $nodes);
334370
$this->assertSame('12345_top', trim($nodes[0]->textContent));
335371
}
372+
373+
374+
public function testFindElementFindsDirectChildUnprefixed(): void
375+
{
376+
$doc = new DOMDocument();
377+
$doc->loadXML('<root><target>t</target><other/></root>');
378+
379+
$root = $doc->documentElement;
380+
$this->assertInstanceOf(DOMElement::class, $root);
381+
382+
$found = XPath::findElement($root, 'target');
383+
$this->assertInstanceOf(DOMElement::class, $found);
384+
$this->assertSame('target', $found->localName);
385+
$this->assertSame('t', $found->textContent);
386+
}
387+
388+
389+
public function testFindElementFindsDirectChildWithPrefixWhenNsOnRoot(): void
390+
{
391+
$xml = <<<'XML'
392+
<?xml version="1.0" encoding="UTF-8"?>
393+
<root xmlns:foo="https://example.org/foo">
394+
<foo:item>ok</foo:item>
395+
</root>
396+
XML;
397+
$doc = new DOMDocument();
398+
$doc->loadXML($xml);
399+
400+
$root = $doc->documentElement;
401+
$this->assertInstanceOf(DOMElement::class, $root);
402+
403+
// Namespace is declared on root, so getXPath($doc) used by findElement knows 'foo'
404+
$found = XPath::findElement($root, 'foo:item');
405+
$this->assertInstanceOf(DOMElement::class, $found);
406+
$this->assertSame('item', $found->localName);
407+
$this->assertSame('https://example.org/foo', $found->namespaceURI);
408+
$this->assertSame('ok', $found->textContent);
409+
}
410+
411+
412+
public function testFindElementReturnsFalseWhenNotFoundAndDoesNotDescend(): void
413+
{
414+
// 'target' is a grandchild; findElement should only match direct children via './name'
415+
$doc = new DOMDocument();
416+
$doc->loadXML('<root><container><target/></container></root>');
417+
418+
$root = $doc->documentElement;
419+
$this->assertInstanceOf(DOMElement::class, $root);
420+
421+
$found = XPath::findElement($root, 'target');
422+
$this->assertFalse($found, 'Should return false for non-direct child');
423+
}
424+
425+
426+
public function testFindElementThrowsIfNoOwnerDocument(): void
427+
{
428+
// A standalone DOMElement (not created by a DOMDocument) has no ownerDocument
429+
$ref = new \DOMElement('container');
430+
431+
$this->expectException(\RuntimeException::class);
432+
$this->expectExceptionMessage('Cannot search, no DOMDocument available');
433+
XPath::findElement($ref, 'anything');
434+
}
336435
}

‎tests/resources/xml/success_response_c.xml‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
<foo:serviceResponse xmlns:foo="https://example.org/foo">
2+
<foo:authenticationSuccess xmlns:foo="https://example.org/foo">
3+
<foo:user>jdoe</foo:user>
4+
5+
<!-- slate prefix declared only here (descendant), not on ancestors -->
6+
<slate:person xmlns:slate="https://example.org/slate">12345_top</slate:person>
7+
8+
<foo:attributes>
9+
<foo:authenticationDate>2025-11-07T22:00:24+02:00</foo:authenticationDate>
10+
<foo:longTermAuthenticationRequestTokenUsed>true</foo:longTermAuthenticationRequestTokenUsed>
11+
<foo:isFromNewLogin>true</foo:isFromNewLogin>
12+
<foo:sn>Doe</foo:sn>
13+
<foo:firstname>John</foo:firstname>
14+
<foo:mail>jdoe@example.edu</foo:mail>
15+
<foo:eduPersonPrincipalName>jdoe@example.edu</foo:eduPersonPrincipalName>
16+
17+
<!-- slate prefix declared only on each element -->
18+
<slate:person xmlns:slate="https://example.org/slate">12345</slate:person>
19+
<slate:round xmlns:slate="https://example.org/slate">Fall-2025</slate:round>
20+
<slate:ref xmlns:slate="https://example.org/slate">ABC-123</slate:ref>
21+
</foo:attributes>
22+
</foo:authenticationSuccess>
23+
</foo:serviceResponse>

0 commit comments

Comments
 (0)