XML: return complete attribute values

Previously, $attr$ returned undefined for attributes containing entity
references, while $attrs returned only the first text fragment. libxml2
canonicalization expands the complete attribute value, creating an
interpretation difference for signed XML consumers.

Use xmlNodeGetContent() for both getters in the njs and QuickJS backends.
This preserves the existing explicit-attribute lookup and makes the
JavaScript value agree with canonicalization.

diff --git a/external/njs_xml_module.c b/external/njs_xml_module.c
index d58e222c0c3cc5128752caaaaced9894e54973bf..26c3cd1c215c1c001316cae5528bfe0a3d1c93ff 100644 (file)
--- a/external/njs_xml_module.c
+++ b/external/njs_xml_module.c
@@ -102,6 +102,8 @@ static njs_int_t njs_xml_node_tag_handler(njs_vm_t *vm, njs_value_t *value,
     njs_str_t *name, njs_value_t *setval, njs_value_t *retval);
 static njs_int_t njs_xml_node_tags_handler(njs_vm_t *vm, njs_value_t *value,
     njs_str_t *name, njs_value_t *setval, njs_value_t *retval);
+static njs_int_t njs_xml_attr_value(njs_vm_t *vm, xmlAttr *attr,
+    njs_value_t *retval);
 
 static xmlNode *njs_xml_external_node(njs_vm_t *vm, njs_value_t *value);
 static const u_char *njs_xml_string_to_c_string(njs_vm_t *vm, njs_str_t *str,
@@ -1157,7 +1159,7 @@ njs_xml_node_attr_handler(njs_vm_t *vm, xmlNode *current, njs_str_t *name,
     njs_int_t     ret;
     njs_str_t     str;
     xmlAttr       *attr;
-    const u_char  *content, *name_c, *value;
+    const u_char  *name_c, *value;
     u_char        name_buf[512], value_buf[1024];
 
     if (retval != NULL && setval == NULL) {
@@ -1176,15 +1178,7 @@ njs_xml_node_attr_handler(njs_vm_t *vm, xmlNode *current, njs_str_t *name,
                 continue;
             }
 
-            if (attr->children != NULL
-                && attr->children->next == NULL
-                && attr->children->type == XML_TEXT_NODE)
-            {
-                content = (const u_char *) attr->children->content;
-
-                return njs_vm_value_string_create(vm, retval, content,
-                                                  njs_strlen(content));
-            }
+            return njs_xml_attr_value(vm, attr, retval);
         }
 
         njs_value_undefined_set(retval);
@@ -1250,6 +1244,25 @@ njs_xml_node_attr_handler(njs_vm_t *vm, xmlNode *current, njs_str_t *name,
 }
 
 
+static njs_int_t
+njs_xml_attr_value(njs_vm_t *vm, xmlAttr *attr, njs_value_t *retval)
+{
+    njs_int_t  ret;
+    xmlChar    *text;
+
+    text = xmlNodeGetContent((xmlNode *) attr);
+    if (njs_slow_path(text == NULL)) {
+        njs_vm_internal_error(vm, "xmlNodeGetContent() failed");
+        return NJS_ERROR;
+    }
+
+    ret = njs_vm_value_string_create(vm, retval, text, njs_strlen(text));
+    xmlFree(text);
+
+    return ret;
+}
+
+
 static njs_int_t
 njs_xml_node_tag_remove(njs_vm_t *vm, njs_value_t *value, njs_str_t *name)
 {
@@ -2153,8 +2166,7 @@ njs_xml_attr_ext_prop_handler(njs_vm_t *vm, njs_object_prop_t *prop,
             continue;
         }
 
-        return njs_vm_value_string_create(vm, retval, node->children->content,
-                                          njs_strlen(node->children->content));
+        return njs_xml_attr_value(vm, node, retval);
     }
 
     njs_value_undefined_set(retval);

diff --git a/external/qjs_xml_module.c b/external/qjs_xml_module.c
index e210ad9eda0e382e465a7277558083cbd784cdb9..14cb06339a009df46804b7133abacb14f4a0560d 100644 (file)
--- a/external/qjs_xml_module.c
+++ b/external/qjs_xml_module.c
@@ -73,6 +73,7 @@ static JSValue qjs_xml_node_make(JSContext *cx, qjs_xml_doc_t *doc,
     xmlNode *node);
 static int qjs_xml_node_has_method(JSContext *cx, JSValueConst obj,
     JSAtom prop, njs_str_t *name);
+static JSValue qjs_xml_attr_value(JSContext *cx, xmlAttr *attr);
 static int qjs_xml_node_get_own_property(JSContext *cx,
     JSPropertyDescriptor *pdesc, JSValueConst obj, JSAtom prop);
 static int qjs_xml_node_get_own_property_names(JSContext *cx,
@@ -707,19 +708,32 @@ qjs_xml_node_attr_handler(JSContext *cx, qjs_xml_node_t *current,
             continue;
         }
 
-        if (attr->children != NULL
-            && attr->children->next == NULL
-            && attr->children->type == XML_TEXT_NODE
-            && attr->children->content != NULL)
-        {
-            return JS_NewString(cx, (char *) attr->children->content);
-        }
+        return qjs_xml_attr_value(cx, attr);
     }
 
     return JS_UNDEFINED;
 }
 
 
+static JSValue
+qjs_xml_attr_value(JSContext *cx, xmlAttr *attr)
+{
+    JSValue  value;
+    xmlChar  *text;
+
+    text = xmlNodeGetContent((xmlNode *) attr);
+    if (text == NULL) {
+        JS_ThrowInternalError(cx, "xmlNodeGetContent() failed");
+        return JS_EXCEPTION;
+    }
+
+    value = JS_NewString(cx, (char *) text);
+    xmlFree(text);
+
+    return value;
+}
+
+
 static int
 qjs_xml_node_attr_modify(JSContext *cx, JSValue current, const u_char *name,
     JSValue setval)
@@ -1829,7 +1843,6 @@ qjs_xml_attr_get_own_property(JSContext *cx, JSPropertyDescriptor *pdesc,
     JSValueConst obj, JSAtom prop)
 {
     size_t          size;
-    u_char          *text;
     xmlAttr         *attr;
     njs_str_t       name;
     qjs_xml_attr_t  *current;
@@ -1862,24 +1875,16 @@ qjs_xml_attr_get_own_property(JSContext *cx, JSPropertyDescriptor *pdesc,
 
         JS_FreeCString(cx, (char *) name.start);
 
-        text = xmlNodeGetContent(attr->children);
-        if (text == NULL) {
-            return 0;
-        }
-
         if (pdesc != NULL) {
             pdesc->flags = JS_PROP_ENUMERABLE;
             pdesc->getter = JS_UNDEFINED;
             pdesc->setter = JS_UNDEFINED;
-            pdesc->value  = JS_NewString(cx, (char *) text);
+            pdesc->value = qjs_xml_attr_value(cx, attr);
             if (JS_IsException(pdesc->value)) {
-                xmlFree(text);
                 return -1;
             }
         }
 
-        xmlFree(text);
-
         return 1;
     }
 

diff --git a/test/xml/saml_verify.t.mjs b/test/xml/saml_verify.t.mjs
index 65d4e06b7e9f0a191e5572b4954f13b6e812f2a6..7cdbfcf6444d80d9ec9b8d7ae12a8ff4ba94173d 100644 (file)
--- a/test/xml/saml_verify.t.mjs
+++ b/test/xml/saml_verify.t.mjs
@@ -294,9 +294,45 @@ let saml_verify_tsuite = {
         { saml: "response_assertion_and_message_signed.xml", expected: true },
 
         { saml: "auth_r.xml", sign: true, key: { sign_file: "rsa.pkcs8" }, expected: true },
-]};
+    ]};
+
+let entity_attribute_tsuite = {
+    name: "signed entity-backed attribute",
+    skip: () => (!has_njs() || !has_webcrypto()),
+    T: async (params) => {
+        let private_key = fs.readFileSync('test/webcrypto/rsa.pkcs8');
+        let public_key = fs.readFileSync('test/webcrypto/rsa.spki');
+        let source = xml.parse(fs.readFileSync('test/xml/auth_r.xml'));
+        let root = source.$root;
+
+        root.setAttribute('NotOnOrAfter', '2024-01-18T06:21:48Z');
+        await signSAML(source, private_key);
+
+        let saml = xml.parse(`<!DOCTYPE saml:Assertion [
+            <!ENTITY njs_entity "2024-01-18T06:21:48Z">
+        ]>${xml.c14n(source).toString().replace('2024-01-18T06:21:48Z',
+                                                 '&njs_entity;')}`);
+
+        if (!await verifySAMLSignature(saml, public_key)) {
+            throw Error('signature verification failed');
+        }
+
+        root = saml.$root;
+        if (root.$attr$NotOnOrAfter !== '2024-01-18T06:21:48Z'
+            || root.$attrs.NotOnOrAfter !== '2024-01-18T06:21:48Z')
+        {
+            throw Error('signed attribute value mismatch');
+        }
+
+        return 'SUCCESS';
+    },
+
+    tests: [
+        {},
+    ]};
 
 run([
     saml_verify_tsuite,
+    entity_attribute_tsuite,
 ])
 .then($DONE, $DONE);

diff --git a/test/xml/xml.t.mjs b/test/xml/xml.t.mjs
index 220c4b7e01321910fb0cdd78f08b687a3ad05098..51b94bab21c180290839aa7bee1d4d00bc37bc8d 100644 (file)
--- a/test/xml/xml.t.mjs
+++ b/test/xml/xml.t.mjs
@@ -168,6 +168,24 @@ let parse_tsuite = {
           get: (doc) => JSON.stringify([doc['俄语'].$name[1],doc['俄语']['$attr$լեզու'][7],doc['俄语'].$text[5]]),
           expected: '["语","ն","е"]' },
 
+        { doc: `<!DOCTYPE r [<!ENTITY e "value">]><r a="&e;"/>`,
+          get: (doc) => JSON.stringify([doc.r.$attr$a, doc.r.$attrs.a]),
+          expected: '["value","value"]' },
+        { doc: `<!DOCTYPE r [<!ENTITY e "middle">]><r a="left-&e;-right"/>`,
+          get: (doc) => JSON.stringify([doc.r.$attr$a, doc.r.$attrs.a,
+                                        xml.serializeToString(doc.r)]),
+          expected: '["left-middle-right","left-middle-right",'
+                    + '"<r a=\\"left-middle-right\\"></r>"]' },
+        { doc: `<!DOCTYPE r [<!ENTITY a "A"><!ENTITY b "&a;B">]><r a="&b;"/>`,
+          get: (doc) => JSON.stringify([doc.r.$attr$a, doc.r.$attrs.a]),
+          expected: '["AB","AB"]' },
+        { doc: `<!DOCTYPE r [<!ENTITY e "">]><r a="&e;"/>`,
+          get: (doc) => JSON.stringify([doc.r.$attr$a, doc.r.$attrs.a]),
+          expected: '["",""]' },
+        { doc: `<!DOCTYPE r [<!ATTLIST r a CDATA "default">]><r/>`,
+          get: (doc) => JSON.stringify([doc.r.$attr$a, doc.r.$attrs]),
+          expected: '[null,null]' },
+
         { get: (doc) => JSON.stringify(Object.getOwnPropertyNames(doc)),
           expected: '["note"]' },
         { get: (doc) => JSON.stringify(Object.getOwnPropertyNames(doc.note)),
@@ -291,6 +309,9 @@ let c14n_tsuite = {
         { doc: `<note><to a="foo" b="bar">Tove</to><from>Jani</from></note>`,
           call: (doc) => xml.exclusiveC14n(doc.note, doc.note.to),
           expected: `<note><from>Jani</from></note>` },
+        { doc: `<!DOCTYPE r [<!ENTITY e "middle">]><r a="left-&e;-right"/>`,
+          call: (doc) => xml.c14n(doc.r),
+          expected: `<r a="left-middle-right"></r>` },
         { doc: `<r><a>A</a></r>`,
           call: (doc) => {
               let other = xml.parse(`<other></other>`);