Add proper support for xforms-load permission

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
12 years ago
10 months ago

People

(Reporter: aaronr, Assigned: Doron Rosenberg (IBM))

Tracking

({fixed1.8.0.5, fixed1.8.1})

Trunk
fixed1.8.0.5, fixed1.8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

12 years ago
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

12 years ago
Yes, kill xforms-load.
Hardware: PC → All
(Assignee)

Comment 2

12 years ago
Created attachment 202249 [details] [diff] [review]
remove the xforms-load check
Attachment #202249 - Flags: review?(allan)

Comment 3

12 years ago
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.
(Assignee)

Comment 4

12 years ago
(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?
(Reporter)

Comment 5

12 years ago
(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

12 years ago
(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

12 years ago
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".
Attachment #202249 - Flags: review?(allan) → review-
(Assignee)

Comment 8

12 years ago
(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

12 years ago
(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.
(Assignee)

Comment 10

11 years ago
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.
(Reporter)

Comment 11

11 years ago
(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

11 years ago
(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?
(Reporter)

Updated

11 years ago
Blocks: 331209
(Assignee)

Comment 13

11 years ago
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

11 years ago
(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?
(Assignee)

Comment 15

11 years ago
(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

11 years ago
(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.
(Assignee)

Comment 17

11 years ago
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.
(Assignee)

Comment 18

11 years ago
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.
Attachment #222552 - Flags: review?(allan)

Updated

11 years ago
Summary: get rid of xforms-load if no longer needed → Add proper support for xforms-load permission

Comment 19

11 years ago
Created attachment 222592 [details]
Testcase

Comment 20

11 years ago
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
Attachment #222552 - Flags: review?(allan) → review-
(Assignee)

Comment 21

11 years ago
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.
Attachment #222552 - Attachment is obsolete: true
Attachment #222672 - Flags: review?(allan)

Comment 22

11 years ago
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
Attachment #222672 - Flags: review?(allan) → review-

Comment 23

11 years ago
(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.
(Assignee)

Comment 24

11 years ago
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.
Attachment #222866 - Flags: review?(allan)

Comment 25

11 years ago
Comment on attachment 222866 [details] [diff] [review]
fix the minor bug

r=me
Attachment #222866 - Flags: review?(allan) → review+
(Assignee)

Updated

11 years ago
Attachment #222866 - Flags: review?(aaronr)
(Reporter)

Comment 26

11 years ago
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
Attachment #222866 - Flags: review?(aaronr) → review+

Comment 27

11 years ago
Fixed on trunk
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch

Comment 28

11 years ago
Why xforms-permissions.xul/xforms-permissions.js are antedated by 2003 and 2005 years?

Comment 29

11 years ago
(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

11 years ago
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

11 years ago
Created attachment 223150 [details] [diff] [review]
vc6 fix

Use enums instead. Fixes VC6 build problems, and makes things nicer :-)
Attachment #202249 - Attachment is obsolete: true
Attachment #222672 - Attachment is obsolete: true
Attachment #223150 - Flags: review?(neil)

Updated

11 years ago
Attachment #223150 - Flags: review?(neil) → review+

Comment 32

11 years ago
Comment on attachment 223150 [details] [diff] [review]
vc6 fix

What do you say to it Doron?
Attachment #223150 - Flags: review?

Updated

11 years ago
Attachment #223150 - Flags: review? → review?(doronr)
(Assignee)

Updated

11 years ago
Attachment #223150 - Flags: review?(doronr) → review+

Updated

11 years ago
Blocks: 339284

Comment 33

11 years ago
(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.

Updated

11 years ago
Keywords: fixed1.8.1

Updated

11 years ago
Keywords: fixed1.8.0.5

Updated

11 years ago
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.