Last Comment Bug 315407 - Add proper support for xforms-load permission
: Add proper support for xforms-load permission
Status: RESOLVED FIXED
: fixed1.8.0.5, fixed1.8.1
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Doron Rosenberg (IBM)
: Stephen Pride
Mentors:
Depends on:
Blocks: 331209 339284
  Show dependency treegraph
 
Reported: 2005-11-07 09:41 PST by aaronr
Modified: 2016-07-15 14:46 PDT (History)
2 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
remove the xforms-load check (1.35 KB, patch)
2005-11-08 07:07 PST, Doron Rosenberg (IBM)
allan: review-
Details | Diff | Splinter Review
new patch (35.09 KB, patch)
2006-05-18 14:58 PDT, Doron Rosenberg (IBM)
allan: review-
Details | Diff | Splinter Review
Testcase (3.99 KB, application/xhtml+xml)
2006-05-19 01:58 PDT, Allan Beaufour
no flags Details
better patch (32.75 KB, patch)
2006-05-19 13:51 PDT, Doron Rosenberg (IBM)
allan: review-
Details | Diff | Splinter Review
fix the minor bug (32.77 KB, patch)
2006-05-22 08:13 PDT, Doron Rosenberg (IBM)
allan: review+
aaronr: review+
Details | Diff | Splinter Review
vc6 fix (5.40 KB, patch)
2006-05-24 01:38 PDT, Allan Beaufour
neil: review+
doronr: review+
Details | Diff | Splinter Review

Description aaronr 2005-11-07 09:41:38 PST
We need to get rid of xforms-load pref check in nsXFormsUtils.cpp.  We've got xforms-xd pref now which accomplishes the same thing.
Comment 1 Allan Beaufour 2005-11-08 04:34:14 PST
Yes, kill xforms-load.
Comment 2 Doron Rosenberg (IBM) 2005-11-08 07:07:29 PST
Created attachment 202249 [details] [diff] [review]
remove the xforms-load check
Comment 3 Allan Beaufour 2005-11-09 02:22:03 PST
Comment on attachment 202249 [details] [diff] [review]
remove the xforms-load check

Hmmm, maybe I was too quick saying that. We have "xforms-xd" that allows hosts to "submit to another domain", right? We need another permission that allows hosts to "load data from another domain". So "CheckSameOrigin" should take the permission to check as a parameter, and submission should use "xforms-xd", and instance and label (others?, message?) should use "xforms-load". Hmm, but should we actually check for same origin for label? Do we care about that? Isn't it only instance loading that's a problem, as that might end up getting submitted.
Comment 4 Doron Rosenberg (IBM) 2005-11-09 08:32:29 PST
(In reply to comment #3)
> (From update of attachment 202249 [details] [diff] [review] [edit])
> Hmmm, maybe I was too quick saying that. We have "xforms-xd" that allows hosts
> to "submit to another domain", right? We need another permission that allows
> hosts to "load data from another domain". So "CheckSameOrigin" should take the
> permission to check as a parameter, and submission should use "xforms-xd", and
> instance and label (others?, message?) should use "xforms-load". Hmm, but
> should we actually check for same origin for label? Do we care about that?
> Isn't it only instance loading that's a problem, as that might end up getting
> submitted.
> 

could one copy the label contents into the instance data somehow?
Comment 5 aaronr 2005-11-09 09:30:14 PST
(In reply to comment #3)
> (From update of attachment 202249 [details] [diff] [review] [edit])
> Hmmm, maybe I was too quick saying that. We have "xforms-xd" that allows hosts
> to "submit to another domain", right? We need another permission that allows
> hosts to "load data from another domain". So "CheckSameOrigin" should take the
> permission to check as a parameter, and submission should use "xforms-xd", and
> instance and label (others?, message?) should use "xforms-load". Hmm, but
> should we actually check for same origin for label? Do we care about that?
> Isn't it only instance loading that's a problem, as that might end up getting
> submitted.
> 


Personally, I've always thought of them as the same issue.  Its the question of whether the user will trust forms from site 'x' to connect to domains other than 'x'.  Maybe we just need to reword the UI or documentation?
Comment 6 Allan Beaufour 2005-11-11 07:13:08 PST
(In reply to comment #5)
> Personally, I've always thought of them as the same issue.  Its the question of
> whether the user will trust forms from site 'x' to connect to domains other
> than 'x'.  Maybe we just need to reword the UI or documentation?

I think that is way too restrictive. That means you can not get label text from http://example.com/standard-text.txt. I see it as being equivalent to doing including:
<img src="http://www.w3.org/Icons/valid-xhtml11"/>
on a document on beaufour.dk. That should definately be allowed.

The permission has to do with security/privacy. We should not allow instance data to cross domains.
Comment 7 Allan Beaufour 2005-11-14 01:01:58 PST
Comment on attachment 202249 [details] [diff] [review]
remove the xforms-load check

I say, that we kill the check for label, and keep two permissions. One for sending data to another domain, and one for loading data from another domain. data == "instance data".
Comment 8 Doron Rosenberg (IBM) 2006-01-04 14:01:04 PST
(In reply to comment #7)
> (From update of attachment 202249 [details] [diff] [review] [edit])
> I say, that we kill the check for label, and keep two permissions. One for
> sending data to another domain, and one for loading data from another domain.
> data == "instance data".
> 

Why not have just one permission?  Is there a reason to just allow a site to load data form anywhere vs allowing it to load/send anywhere?
Comment 9 Allan Beaufour 2006-01-09 07:20:02 PST
(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 202249 [details] [diff] [review] [edit] [edit])
> > I say, that we kill the check for label, and keep two permissions. One for
> > sending data to another domain, and one for loading data from another domain.
> > data == "instance data".
> > 
> 
> Why not have just one permission?  Is there a reason to just allow a site to
> load data form anywhere vs allowing it to load/send anywhere?

I would personally allow more domains cross-domain load-access, than I would allow to send anywhere.
Comment 10 Doron Rosenberg (IBM) 2006-03-21 15:07:01 PST
Thinking more about it, the policy should be:

Never allow web pages to access the content of data from another domain.  Meaning, cross-domain src including in a label would be bad, since the contents are in anonymous content.

Though if a message includes another domain, not that bad, since the page can't access those contents.

So I think we need to keep the existing checks.
Comment 11 aaronr 2006-03-21 15:18:12 PST
(In reply to comment #10)
> Thinking more about it, the policy should be:
> 
> Never allow web pages to access the content of data from another domain. 
> Meaning, cross-domain src including in a label would be bad, since the contents
> are in anonymous content.
> 
> Though if a message includes another domain, not that bad, since the page can't
> access those contents.
> 
> So I think we need to keep the existing checks.
> 


If we keep the same checks, then we need UI so that the user can give permission for xforms-load items.  Maybe a list of the trusted domains like we have now and for each item in the list have a checkbox for submitting cross domain and a checkbox for loading cross domain?
Comment 12 Allan Beaufour 2006-03-22 00:21:34 PST
(In reply to comment #10)
> Thinking more about it, the policy should be:
> 
> Never allow web pages to access the content of data from another domain. 
> Meaning, cross-domain src including in a label would be bad, since the contents
> are in anonymous content.
> 
> Though if a message includes another domain, not that bad, since the page can't
> access those contents.
> 
> So I think we need to keep the existing checks.

Basically you agree with what I say in the above, or?
Comment 13 Doron Rosenberg (IBM) 2006-05-02 13:05:04 PDT
http://landfill.mozilla.org/mxr-test/mozilla/source/netwerk/base/public/nsIPermissionManager.idl shows that the permission manager can store mult-state values.

So, we could take xforms-xd and add more values:
1: current value - allow cross domain submission
2: may only fetch data cross domain
3: fetch/store cross domain

We would need our own dialog then (I guess like the current one, but with checkboxes) and clone/modify permissions.xul/permissions.js
Comment 14 Allan Beaufour 2006-05-16 01:51:33 PDT
(In reply to comment #13)
> So, we could take xforms-xd and add more values:
> 1: current value - allow cross domain submission
> 2: may only fetch data cross domain
> 3: fetch/store cross domain

Shouldn't it be:
1: may fetch data cross domain
2: allow cross domain submission (current)
3: fetch/store cross domain

If you allow it to submit, then you also allow replace="instance", ie. fetching data?

With that, I think it's a good idea. Are you on top of this Doron?
Comment 15 Doron Rosenberg (IBM) 2006-05-16 12:13:21 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > So, we could take xforms-xd and add more values:
> > 1: current value - allow cross domain submission
> > 2: may only fetch data cross domain
> > 3: fetch/store cross domain
> 
> Shouldn't it be:
> 1: may fetch data cross domain
> 2: allow cross domain submission (current)
> 3: fetch/store cross domain
> 
> If you allow it to submit, then you also allow replace="instance", ie. fetching
> data?

I was thinking that 1 and 2 were exclusive, and 3 means allow fetching and getting.

Replace instance would only work for cross domain with the 3rd state set.

As for doing this, not sure exactly when, since I have non-XForms deadlines.  I will play with the UI a bit to see how hard this would be, hopefully I'll have it by end of the month.
Comment 16 Allan Beaufour 2006-05-17 00:42:24 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > So, we could take xforms-xd and add more values:
> > > 1: current value - allow cross domain submission
> > > 2: may only fetch data cross domain
> > > 3: fetch/store cross domain
> > 
> > Shouldn't it be:
> > 1: may fetch data cross domain
> > 2: allow cross domain submission (current)
> > 3: fetch/store cross domain
> > 
> > If you allow it to submit, then you also allow replace="instance", ie. fetching
> > data?
> 
> I was thinking that 1 and 2 were exclusive, and 3 means allow fetching and
> getting.
> 
> Replace instance would only work for cross domain with the 3rd state set.

Ok. Sounds good.
Comment 17 Doron Rosenberg (IBM) 2006-05-17 11:43:23 PDT
I actually got the UI working yesterday (forked the ff permission manager, since it isn't easily extensible), turned out to be easier than I thought.  It looks like the current dialog, but has 2 checkboxes (one for loading cross domain, one for sending cross domain) and the Allow button is active if at least one of the checkbox is checked and a url field is non-empty.

So next thing is to have XFormsUtils::CheckSameOrigin take a new argument, aMode or such, which corresponds to either 1 (load) or 2 (send), which is then compared to the permission manager value (1 being load, 2 send, 3 load/send).  Replace instance would be considered a load for example.

That should probably cover all we need, and be extensible in case we need to add more states for whatever reason.
Comment 18 Doron Rosenberg (IBM) 2006-05-18 14:58:56 PDT
Created attachment 222552 [details] [diff] [review]
new patch

Here is the patch.  The permission dialog is still a bit messy (the checkbox labels are long), but it works.

I made Send be state 1 because that is was xforms-xd meant before, so this will be backwards compat.  Load is 2 and Load/Send 3.
Comment 19 Allan Beaufour 2006-05-19 01:58:17 PDT
Created attachment 222592 [details]
Testcase
Comment 20 Allan Beaufour 2006-05-19 02:20:30 PDT
Comment on attachment 222552 [details] [diff] [review]
new patch

> Index: extensions/xforms/nsXFormsUtils.h
> ===================================================================
 
> +  static const PRUint8 kXFormsActionSend = 1;
> +  static const PRUint8 kXFormsActionLoad = 2;
> +  static const PRUint8 kXFormsActionLoadSend = 3;

Please add documentation

>    /**
>     * @return true if aTestURI has the same origin as aBaseDocument
>     */

Please add documentation for the parameters

>    static NS_HIDDEN_(PRBool) CheckSameOrigin(nsIDocument *aBaseDocument,
> -                                            nsIURI *aTestURI);
> +                                            nsIURI *aTestURI,
> +                                            PRUint8 aMode=kXFormsActionLoad);

nit: add spaces, ' = '
 

> Index: extensions/xforms/resources/locale/en-US/xforms.dtd
> ===================================================================

> +<!ENTITY xforms.crossdomain.ui.label.allowLoad "Allow cross-domain load">
> +<!ENTITY xforms.crossdomain.ui.label.allowSend "Allow cross-domain submission">
> +

Maybe we should refrain from using "submission", and just use load and send only? Since the button is called "Allow", maybe we could just have:
<!ENTITY xforms.crossdomain.ui.label.allowLoad "Load">
<!ENTITY xforms.crossdomain.ui.label.allowSend "Send">

Or:
Allow: [ ] Send [ ] Load  [Add site]


> Index: extensions/xforms/resources/locale/en-US/xforms.properties
> ===================================================================

> +# Used in the permissions table to show the state
> +xformsXDPermissionLoad = Load
> +xformsXDPermissionSend = Send
> +xformsXDPermissionLoadSend = Load/Send

"Load + Send" or "Load and Send"

> --- /dev/null	2006-03-13 03:43:19.564586250 -0600
> +++ extensions/xforms/resources/content/xforms-permissions.js	2006-05-18 16:33:12.000000000 -0500

> +    if (!exists) {
> +      host = (host.charAt(0) == ".") ? host.substring(1,host.length) : host;
> +      var uri = ioService.newURI("http://" + host, null, null);
> +      dump(this._type + " " + permission);

Leftover dump()


Other things:

It is annoying that you cannot change an existing entry. Clicking on the "Status" part could cycle through options?

Possibly the column should not be called "Status"? "Allowed to"?

And maybe a "Nothing" value too, if you temporarily want to disable access for a site? Dunno...

The code looks fine otherwise, but I really would like to see that "cycle through options functionality". r- for that only
Comment 21 Doron Rosenberg (IBM) 2006-05-19 13:51:38 PDT
Created attachment 222672 [details] [diff] [review]
better patch

If you select an item in the tree, you can edit.  Clear will then clear and you can add a new one.
Comment 22 Allan Beaufour 2006-05-22 07:58:05 PDT
Comment on attachment 222672 [details] [diff] [review]
better patch

I'm not too happy about the save button. I would not expect that I would need to click it to activate my changes. Can we not activate the changes right away?

Oh, and:
1. Go to dialog
2. Click existing host
3. Change permissions
4. Click save
5. Click same host again

-> The old values are chosen in the checkboxes
Comment 23 Allan Beaufour 2006-05-22 07:59:09 PDT
(In reply to comment #22)
> (From update of attachment 222672 [details] [diff] [review] [edit])
> I'm not too happy about the save button. I would not expect that I would need
> to click it to activate my changes. Can we not activate the changes right away?

This is no biggie though.
Comment 24 Doron Rosenberg (IBM) 2006-05-22 08:13:12 PDT
Created attachment 222866 [details] [diff] [review]
fix the minor bug

I think we should cleanup the UI later, but we need something now for 0.6, and I won't have much time to work on this this week.
Comment 25 Allan Beaufour 2006-05-22 08:39:37 PDT
Comment on attachment 222866 [details] [diff] [review]
fix the minor bug

r=me
Comment 26 aaronr 2006-05-22 23:43:06 PDT
Comment on attachment 222866 [details] [diff] [review]
fix the minor bug

>? extensions/xforms/attachment.cgi?id=191913
>? extensions/xforms/attachment.cgi?id=203428
>? extensions/xforms/attachment.cgi?id=208291
>? extensions/xforms/attachment.cgi?id=209040
>? extensions/xforms/attachment.cgi?id=212909
>? extensions/xforms/attachment.cgi?id=221830
>? extensions/xforms/resources/defaults
>? extensions/xforms/resources/content/xforms-permissions.js
>? extensions/xforms/resources/content/xforms-permissions.xul
>Index: extensions/xforms/jar.mn
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/jar.mn,v
>retrieving revision 1.18
>diff -u -r1.18 jar.mn
>--- extensions/xforms/jar.mn	12 Apr 2006 07:57:55 -0000	1.18
>+++ extensions/xforms/jar.mn	22 May 2006 15:11:48 -0000
>@@ -8,6 +8,8 @@
> * content/xforms/xforms-prefs.xul              (resources/content/xforms-prefs.xul)
> * content/xforms/xforms-prefs-ui.xul           (resources/content/xforms-prefs-ui.xul)
> * content/xforms/xforms-prefs.js               (resources/content/xforms-prefs.js)
>+* content/xforms/xforms-permissions.xul        (resources/content/xforms-permissions.xul)
>+* content/xforms/xforms-permissions.js        (resources/content/xforms-permissions.js)

nit: alignment

r=me
Comment 27 Allan Beaufour 2006-05-23 01:20:13 PDT
Fixed on trunk
Comment 28 alexander :surkov 2006-05-23 01:25:24 PDT
Why xforms-permissions.xul/xforms-permissions.js are antedated by 2003 and 2005 years?
Comment 29 Allan Beaufour 2006-05-23 01:29:28 PDT
(In reply to comment #28)
> Why xforms-permissions.xul/xforms-permissions.js are antedated by 2003 and 2005
> years?

Heh. Because they are copied from permissions.xul/js :)
Comment 30 neil@parkwaycc.co.uk 2006-05-24 01:24:03 PDT
Comment on attachment 222866 [details] [diff] [review]
fix the minor bug

>+  static const PRUint8 kXFormsActionSend = 1;
>+  static const PRUint8 kXFormsActionLoad = 2;
>+  static const PRUint8 kXFormsActionLoadSend = 3;
MSVC doesn't like this, but enums will work.
Comment 31 Allan Beaufour 2006-05-24 01:38:03 PDT
Created attachment 223150 [details] [diff] [review]
vc6 fix

Use enums instead. Fixes VC6 build problems, and makes things nicer :-)
Comment 32 Allan Beaufour 2006-05-24 03:56:25 PDT
Comment on attachment 223150 [details] [diff] [review]
vc6 fix

What do you say to it Doron?
Comment 33 Allan Beaufour 2006-05-26 01:10:45 PDT
(In reply to comment #31)
> Created an attachment (id=223150) [edit]
> vc6 fix
> 
> Use enums instead. Fixes VC6 build problems, and makes things nicer :-)

Fixed on trunk.

Note You need to log in before you can comment on or make changes to this bug.