Making trigger minimal makes submit fail (weird^3)

RESOLVED FIXED

Status

P1
normal
RESOLVED FIXED
12 years ago
2 years ago

People

(Reporter: allan, Assigned: allan)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

2.06 KB, application/xhtml+xml
Details
1.94 KB, application/xhtml+xml
Details
(Assignee)

Description

12 years ago
Ok, this is mighty weird. If I
1) have an externally loaded instance
2) use a minimal trigger to activate a submission for a different instance
... submission fails?
(Assignee)

Comment 1

12 years ago
Created attachment 221799 [details]
Testcase - fails
(Assignee)

Comment 2

12 years ago
Created attachment 221800 [details]
Testcase - works

Comment 3

12 years ago
I debugged this.  Looks like the XPath expression evaluation is failing in the case of the minimal trigger whereas it isn't in the case of the regular trigger.  XPath is calling nsContentUtils::CanCallerAccess to make sure that we are allowed to access the contextnode.  The context node is the external instance root in the failing testcase.  CanCallerAccess calls nsScriptSecurityManager::IsCapabilityEnabled looking to see if "UniversalBrowserRead" access is enabled.  This returns true in the case of the regular trigger because there is no script code on the stack.  So xpath will continue evaluating the expression.  In the case of the minimal trigger, there IS script code on the stack so it will continue on in IsCapabilityEnabled and eventually return false.  So xpath evaluation will halt there and fail with NS_ERROR_DOM_SECURITY_ERR.

The difference in the codepath is that a minimal trigger generates its own DOMActivate event.  So there are two ways to approach this.  The best solution, if it works, is probably to dispatch all of our events from c++.  That will fix the most number of scenarios where this same failure could occur.  If that doesn't work, then we should be able to fix this at least for triggers by making the minimal trigger an HTML button that is styled to look just like text.  That way we get the DOMActivate event for free.

Of course the BEST best way would be to somehow tag any external document that XForms loads as trusted by the entire system (since we have our own security trust rules) but I doubt anyone in Mozilla would go for that. :=)

Comment 4

12 years ago
Well, dispatching the event from the c++ side of things didn't help.  I created a patch that moved the building and dispatching of events from JS to c++ and we still had the same problem.

Other options to fix this that are probably more complete than the trigger specific fix:

1) change xpath so that nsXPathExpression::EvaluateWithContext doesn't call CanCallerAccess if the expression was built with a nsIXFormsXPathEvaluator.  Problem with this will be convincing the XPath guys to agree to it.

2) override nsXPathExpression in XPath so that xforms xpath evaluator uses our own custom expression that doesn't perform this check but rather calls back into XForms to perform our normal security-type checks.  Same problem as #1

3) limit the number of xpath expression evaluations that are triggered by JS.  We could make all of the stub elements evaluate just like controls...do it upon initialization and again whenever the MDG determines that one of their dependency nodes has gone dirty.  Then we keep around the node result for these xforms elements in member data similar to how we already do it for bound nodes (mBoundNode).  That will keep all of those evaluations on the c++ side of things.  The problem here is that there will still undoubtedly be holes that we miss and we'll be doing expression evaluations that we would otherwise never need to do.

This is all that I plan to do on this bug for now, in case someone else wants to pick this bug up.  Copying sicking on this bug in case he has any ideas.
This issue is making me nervous, as security stuff tends to.

What documents does xforms load that scripts couldn't normally access? One solution that has been used elsewhere is to push a new script context that has access to perform the operation that you want to perform. But i'd really like to know why this is needed at all first.

I don't want to build a solution using the XFormsXPathEvaluator or other custom classes in content/xslt since all xforms specific code there is about to go away.

Comment 6

12 years ago
We allow users to whitelist urls so that they can load instance documents from other domains (using the permission manager stuff). 
I see three possible, though more or less good, solutions to this:

1. Hook your security preferences up to caps so that caps is aware of the
   whitelist of allowed servers.
2. Change the principal of the loaded document so that it's the same as the
   principal of the loading document.
3. Push a null principal on the context stack before calling the function that
   currently fail.

3 is the simplest and least intrusive.

2 might be a good solution too, not sure if we do this anywhere else in the tree. It's sort of how we do principals for data: and javascript: documents.

1 would need a more thorough security review since there might be unintended consequences.
(Assignee)

Updated

12 years ago
Priority: -- → P1
(Assignee)

Updated

12 years ago
Assignee: aaronr → xforms
(Assignee)

Updated

12 years ago
Blocks: 338451
(Assignee)

Comment 8

12 years ago
I believe this is also causing problems for the flickr example (bug 334937)
Blocks: 334937
(Assignee)

Comment 9

12 years ago
I've got something working for this I think.
Assignee: xforms → allan
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 10

12 years ago
Got as far as fixing bug 334937 and bug 338451 (also on trunk), but the minimal trigger is still refusing to work.
(Assignee)

Comment 11

12 years ago
*** Bug 332292 has been marked as a duplicate of this bug. ***

Comment 12

12 years ago
Allan, can we close this as a dupe of 338451 or WORKSFORME?
(Assignee)

Updated

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.