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.

Yes, kill xforms-load.

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.

(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?

(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?

(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 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".

(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?

(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.

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.

(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?

(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?

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

(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?

(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.

(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.

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.

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 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

If you select an item in the tree, you can edit. Clear will then clear and you can add a new one.

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

(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.

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 on attachment 222866 [details] [diff] [review] fix the minor bug r=me

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

Fixed on trunk

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

(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 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.

Use enums instead. Fixes VC6 build problems, and makes things nicer :-)

Comment on attachment 223150 [details] [diff] [review] vc6 fix What do you say to it Doron?

(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.