Closed Bug 335366 Opened 15 years ago Closed 15 years ago

Add support for <xsd:include>

Categories

(Core Graveyard :: XForms, enhancement)

x86
Windows XP
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sspeiche, Assigned: doronr)

References

()

Details

(Keywords: fixed1.8.0.9, fixed1.8.1)

Attachments

(2 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2

In utilizing reword XML-based applications, their data models are most often represented in reusable modules utilizing the XML Schema construction <xsd:include>

This is the case of schemas I/we have seen from groups like US Center for Disease Control, etc.

(opening separate enhancement request for xsd:import and xsd:redefine, as they have different priorities and may require different amount of effort)

Reproducible: Always
See related bug 335367
*** Bug 334956 has been marked as a duplicate of this bug. ***
Attached patch patch (obsolete) — Splinter Review
I hacked this up today, it needs more testing with large schemas and all.  This does most of the required "is the included file a schema file" checks, other than the targetNamespace check (XXX: is in the patch for that).
Assignee: aaronr → doronr
Status: UNCONFIRMED → ASSIGNED
Attachment #219791 - Attachment is obsolete: true
Attachment #220048 - Flags: review?(aaronr)
Comment on attachment 220048 [details] [diff] [review]
patch that does relative urls and security checks

>Index: extensions/webservices/schema/src/nsSchemaLoader.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/webservices/schema/src/nsSchemaLoader.cpp,v
>retrieving revision 1.48
>diff -u -r1.48 nsSchemaLoader.cpp
>--- extensions/webservices/schema/src/nsSchemaLoader.cpp	15 Mar 2006 17:13:09 -0000	1.48
>+++ extensions/webservices/schema/src/nsSchemaLoader.cpp	27 Apr 2006 19:47:54 -0000

>@@ -871,8 +841,144 @@
>       if (NS_SUCCEEDED(rv)) {
>         rv = schemaInst->AddModelGroup(modelGroup);
>       }
>+    }
>+    else if (tagName == nsSchemaAtoms::sInclude_atom) {
>+      /* http://www.w3.org/TR/2004/REC-xmlschema-1-20041028/structures.html#element-include
>+         If we include a schema, it must either
>+           (a) have the same targetNamespace as the including schema document or
>+           (b) no targetNamespace at all
>+
>+         If the uri to load doesn't resolve, it isn't a error.  It is if its an
>+         invalid XML document or not a schema file
>+       */
>+
>+      PRBool hasSchemaLocationAttr = PR_FALSE;
>+      childElement->HasAttribute(NS_LITERAL_STRING("schemaLocation"),
>+                                 &hasSchemaLocationAttr);
>+
>+      // no schema location attribute, skip it
>+      if (!hasSchemaLocationAttr)
>+        continue;
>+
>+      nsAutoString schemalocation;
>+      childElement->GetAttribute(NS_LITERAL_STRING("schemaLocation"),
>+                                 schemalocation);
>+

shouldn't you use a named literal string since you are (potentially) using the same string twice in the same function?  How about testing if schemalocation is an empty string before going on?

>+      nsCOMPtr<nsIIOService> ios = do_GetIOService();
>+      NS_ENSURE_STATE(ios);
>+
>+      nsCOMPtr<nsIDOMDocument> document;
>+      aElement->GetOwnerDocument(getter_AddRefs(document));
>+
>+      nsCOMPtr<nsIDocument> doc = do_QueryInterface(document);
>+      NS_ENSURE_STATE(doc);
>+
>+      nsCOMPtr<nsIURI> uri;
>+
>+      ios->NewURI(NS_ConvertUTF16toUTF8(schemalocation),
>+                  doc->GetDocumentCharacterSet().get(),
>+                  doc->GetDocumentURI(),
>+                  getter_AddRefs(uri));
>+      NS_ENSURE_STATE(uri);
>+
>+      nsCAutoString spec;
>+      uri->GetSpec(spec);

GetSpec should probably be moved below the principal stuff.  No since getting the spec if the principals are just going to fail.

>+
>+      // make sure we can load it - do a principal same origin check.
>+      // get the base document's principal
>+      nsIPrincipal *basePrincipal = doc->GetNodePrincipal();
>+      NS_ENSURE_STATE(basePrincipal);
>+

I thought that they did away with GetNodePrincipal.  Shouldn't this be NodePrincipal()?

>+      // XXX: check the target namespace requirements

make sure that you open a bug on this so we don't forget to do it.


>@@ -3254,3 +3360,51 @@
> 
>   return rv;
> }
>+
>+nsresult
>+nsSchemaLoader::GetDocumentFromURI(const nsAString& aUri,
>+                                   nsIDOMDocument** aDocument)
>+{
>+  nsCOMPtr<nsIURI> resolvedURI;
>+  nsresult rv = GetResolvedURI(aUri, "load", getter_AddRefs(resolvedURI));
>+  if (NS_FAILED(rv)) {
>+    return rv;
>+  }

what, is NS_ENSURE_SUCCESS(rv, rv) too good for you?  Can use it mult. times below here, too.

>+  nsCAutoString spec;
>+  resolvedURI->GetSpec(spec);
>+  
>+  nsCOMPtr<nsIXMLHttpRequest> request(do_CreateInstance(NS_XMLHTTPREQUEST_CONTRACTID, &rv));
>+  if (!request) {
>+    return rv;
>+  }

move spec get below this return otherwise it is wasted.  You could also use NS_ENSURE_TRUE(request, rv) instead of the if test.

Nothing major in my comments, so with those fixed, r=me
Attachment #220048 - Flags: review?(aaronr) → review+
peterv, could you give this a look?  We'd like this for ff 2.0.  thanks!
Attachment #221317 - Flags: review?(peterv)
Comment on attachment 221317 [details] [diff] [review]
with comments addressed

>Index: extensions/webservices/schema/src/nsSchemaLoader.cpp
>===================================================================

>@@ -864,22 +834,164 @@ nsSchemaLoader::ProcessSchemaElement(nsI

>+         If the uri to load doesn't resolve, it isn't a error.  It is if its an

it's

>+      NS_NAMED_LITERAL_STRING(schemaLocationStr, "schemaLocation");
>+      PRBool hasSchemaLocationAttr = PR_FALSE;
>+      childElement->HasAttribute(schemaLocationStr, &hasSchemaLocationAttr);
>+
>+      // no schema location attribute, skip it
>+      if (!hasSchemaLocationAttr)
>+        continue;

No need for this block, the schemalocation.IsEmpty() check below will catch this.

>+      nsCOMPtr<nsIIOService> ios = do_GetIOService();
>+      NS_ENSURE_STATE(ios);

No need for this if you use NS_NewURI below.

>+      nsCOMPtr<nsIURI> uri;
>+
>+      ios->NewURI(NS_ConvertUTF16toUTF8(schemalocation),
>+                  doc->GetDocumentCharacterSet().get(),
>+                  doc->GetDocumentURI(),
>+                  getter_AddRefs(uri));
>+      NS_ENSURE_STATE(uri);

I'm confused how this fits with "If the uri to load doesn't resolve, it isn't a error."
Also do:
      rv = NS_NewURI(getter_AddRefs(uri), NS_ConvertUTF16toUTF8(schemalocation),
                     doc->GetDocumentCharacterSet().get(),
                     doc->GetDocumentURI());
      NS_ENSURE_SUCCESS(rv, rv);

>+      rv = GetDocumentFromURI(NS_ConvertUTF8toUTF16(spec), getter_AddRefs(includedDocument));

Long line.

>+          if (nextSibling) {
>+            rv = aElement->InsertBefore(importedNode, nextSibling,
>+                                        getter_AddRefs(dummy));
>+            NS_ENSURE_SUCCESS(rv, rv);
>+          } else {
>+            rv = aElement->AppendChild(importedNode, getter_AddRefs(dummy));
>+            NS_ENSURE_SUCCESS(rv, rv);
>+          }

Just do:

          rv = aElement->InsertBefore(importedNode, nextSibling,
                                      getter_AddRefs(dummy));
          NS_ENSURE_SUCCESS(rv, rv);

InsertBefore can deal with a null refChild (it'll append).

>+nsSchemaLoader::GetDocumentFromURI(const nsAString& aUri,

>+  nsCOMPtr<nsIXMLHttpRequest> request(do_CreateInstance(NS_XMLHTTPREQUEST_CONTRACTID, &rv));

Long line, I'd do:

  nsCOMPtr<nsIXMLHttpRequest> request =
    do_CreateInstance(NS_XMLHTTPREQUEST_CONTRACTID, &rv);

>+  NS_ENSURE_TRUE(request, rv);

NS_ENSURE_SUCCESS(rv, rv);

>+    document.swap(*aDocument);

You should set *aDocument to nsnull at the beginning of the method.
Attachment #221317 - Flags: review?(peterv) → review+
checked into trunk
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment on attachment 221317 [details] [diff] [review]
with comments addressed

Would like this for ff 2.0 - xforms needs it,and this is low-risk (since only web services uses it).
Attachment #221317 - Flags: approval-branch-1.8.1?
Attachment #221317 - Flags: approval-branch-1.8.1? → approval1.8.1?
Blocks: 335367
What are risks with this patch?  It is pretty large, and that makes me nervous.  Unfortunately, this code can be reached w/o the XForms extensions, so we have to worry somewhat about regressions to Firefox (even though there are probably next to no websites making use of our webservices stack).  Thoughts?

I really wish there were some regression/unit tests for this code so we could have some extra confidence when making these kinds of changes :-)
(In reply to comment #10)
> What are risks with this patch?  It is pretty large, and that makes me nervous.
>  Unfortunately, this code can be reached w/o the XForms extensions, so we have
> to worry somewhat about regressions to Firefox (even though there are probably
> next to no websites making use of our webservices stack).  Thoughts?
> 

A large part of the patch was moving code into a function so that it can be reused.  The rest is new functionality, and it complies with the Mozilla security guidelines (cross domain principal checking).

I did run working webservices tests we have on mozilla.org and they didn't show any regressions.
Comment on attachment 221317 [details] [diff] [review]
with comments addressed

a=darin on behalf of drivers
Attachment #221317 - Flags: approval1.8.1? → approval1.8.1+
thanks for approval!
Keywords: fixed1.8.1
Comment on attachment 221317 [details] [diff] [review]
with comments addressed

Approved bug 335367 actually needs this patch to work, so asking for approval, though might be too late for 1.8.0.9?
Attachment #221317 - Flags: approval1.8.0.9?
Comment on attachment 221317 [details] [diff] [review]
with comments addressed

Approved for 1.8.0 branch, a=jay for drivers.  Please land this asap.  Thanks!
Attachment #221317 - Flags: approval1.8.0.9? → approval1.8.0.9+
checked into MOZILLA_1_8_0_BRANCH
Keywords: fixed1.8.0.9
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.