Closed Bug 419106 Opened 15 years ago Closed 15 years ago

nsSchemaLoader::LoadAsync fails

Categories

(Core Graveyard :: XForms, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aaronr, Assigned: aaronr)

References

()

Details

Attachments

(1 file, 3 obsolete files)

As per comments in bug 404818 and bug 392322, XMLHttpRequest now requires being init'd with the system principal when called from c++.  We'll need to update nsSchemaLoader in extensions/schema-validation to take this into account.
Attached patch patch (obsolete) — Splinter Review
Status: NEW → ASSIGNED
Attachment #305080 - Flags: review?(Olli.Pettay)
Attachment #305080 - Flags: review?(doronr)
Comment on attachment 305080 [details] [diff] [review]
patch

Don't you want to get the subject principal here, rather than the system principal? The former is the for the current script, while the latter has chrome privileges.
(In reply to comment #2)
> (From update of attachment 305080 [details] [diff] [review])
> Don't you want to get the subject principal here, rather than the system
> principal? The former is the for the current script, while the latter has
> chrome privileges.
> 

nsSchemaLoader::LoadAsync can be called from either JS or from c++, so we may or may not have a subject principal.  Right?  In the given testcase we are calling it from c++, but loadAsync lives in the .idl and is scriptable.  But I'm not a security type of guy so I'll do whatever you guys suggest.
well, nsSchemaLoader is not accessible to web content, but we should follow Neil's remark - best to check for an subject principal.
Attached patch patch2 (obsolete) — Splinter Review
changed patch to check for a subject principal first and if not found, it will use the system principal
Attachment #305080 - Attachment is obsolete: true
Attachment #305543 - Flags: review?(doronr)
Attachment #305080 - Flags: review?(doronr)
Attachment #305080 - Flags: review?(Olli.Pettay)
Comment on attachment 305543 [details] [diff] [review]
patch2

looks good.  The only caller to schema loader (xforms) already does same origin checks, so this should be fine.
Attachment #305543 - Flags: review?(doronr) → review+
Attachment #305543 - Flags: review?(Olli.Pettay)
Would it be possible to add some initialization method to schemaloader and that
could take principal as a parameter? Then it would be up to the caller
to pass the right principal (in xforms case the principal of document).
(In reply to comment #7)
> Would it be possible to add some initialization method to schemaloader and that
> could take principal as a parameter? Then it would be up to the caller
> to pass the right principal (in xforms case the principal of document).
> 

Assume that schema loader needs to be called from JS.  Can you get a principal inside JS to pass to this new schema loader initializer?  If not, then this new initializer isn't buying us much, is it?  I guess in that case it would be null and if it is null we can check for subject principal and if that isn't there than exit out.  Is that what you had in mind?
He means to send the principal for the url trying to use the schema loader (say the xforms url).

Of course, this will mean no cross-domain loading of schema files...
Uh, forgot to mention that per spec, you can include schemas from any location.  I could have sworn we did some cross-domain checks somewhere in xforms, but can't find them.
Attached patch patch3 (obsolete) — Splinter Review
added loadAsyncWithPrincipal (noscript) which is called by xforms passing in the system principal and which now does all the heavy lifting.  loadAsync still can be called via JS, but we'll only try to use the subject principal when loadAsync is called.  loadAsync will now call loadAsyncWithPrincipal passing in the subject principal.
Attachment #305543 - Attachment is obsolete: true
Attachment #305571 - Flags: review?(Olli.Pettay)
Attachment #305543 - Flags: review?(Olli.Pettay)
Comment on attachment 305571 [details] [diff] [review]
patch3

>+#include "nsIPrincipal.idl"
I believe this isn't needed because you have:
>+interface nsIPrincipal;
So remove the include, if it works without.

>+    // no executable code will run and doron says that you really can't get to
"doron says" :)

So we handle schemas like images or .js (right?). That should be ok, IMO
Attachment #305571 - Flags: review?(Olli.Pettay) → review+
well, for xforms js can't get at the schema data, since that instance of the loader isn't accessible via xforms idl.
Comment on attachment 305571 [details] [diff] [review]
patch3

I'll fix smaug's note at checkin.  You ok with it, Doron?
Attachment #305571 - Flags: review?(doronr)
Attachment #305571 - Flags: review?(doronr) → review+
Attached patch checked in patchSplinter Review
patch fixes olli's comment.
Attachment #305571 - Attachment is obsolete: true
Checked into trunk.  Trunk only patch, shouldn't go to the branch.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.