253476 - PAC: fixup uris entered in prefs

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

seamonkey version of bug 243397-

it would be nice to accept "www.foo.bar/proxy.pac" in prefs, w/o having to
prepend http://

this could be done by having the prefs frontend use nsIURIFixup to create a real
uri, and store that spec as the pref.

backend can't really do that, since it'd create a necko->docshell dependency

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

(probably the asciiSpec, to avoid problems w/ IDN. the rest of the url is
probably a lost cause, since there's no reasonable charset to assume...)

Comment 2

[Darin Fisher]

using uri fixup on the entered pref value sounds like a good idea to me.

Comment 3

[benc]

I've always disliked this idea, because the pref is a URL, but if we fixup in
the front-end UI, and always save a URL, well, what the heck.

Comment 4

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

->me

Comment 5

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch

Comment 6

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 157375 [details] [diff] [review]
Patch

I don't understand comment #1.	Hopefully I did it correctly anyway.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 157375 [details] [diff] [review]
Patch

+  var url = URIFixup.createFixupURI(proxyURLValue,
nsIURIFixup.FIXUP_FLAGS_MAKE_ALTERNATE_URI).spec;

so in comment 1 I meant to use .asciiSpec here instead of .spec. But now that I
think about it, I'm not so sure that that's a good idea, because users would
see the xn-* hostname...

However, I do think you should wrap your service getting and URI creation in a
try..catch block.

Comment 8

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch v2

Comment 9

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch v3

Comment 10

[[e-mail]]

Comment on attachment 157405 [details] [diff] [review]
Patch v3

Nits:

>+function FixProxyURL()
>+{
>+  const nsIURIFixup = Components.interfaces.nsIURIFixup;
>+  var proxyURL = document.getElementById("networkProxyAutoconfigURL");
>+  var proxyURLValue = proxyURL.value;
Hardly seems worth it for a single use...

>+  try {
>+    var URIFixup = Components.classes["@mozilla.org/docshell/urifixup;1"].getService(nsIURIFixup);
>+    var url = URIFixup.createFixupURI(proxyURLValue, nsIURIFixup.FIXUP_FLAGS_MAKE_ALTERNATE_URI).spec;
[Spotted the pluralization inconsistency yet? Sigh...]
caillon says this is the right fixup flag, although personally I would have
picked FIXUP_FLAG_NONE, which doesn't fix localhost/proxy.pac to
http://www.localhost.com/proxy.pac, but I'll let jag decide.

>         <textbox id="networkProxyAutoconfigURL" flex="1" 
>-                 preftype="string" prefstring="network.proxy.autoconfig_url" class="uri-element"/>
>+                 preftype="string" prefstring="network.proxy.autoconfig_url" class="uri-element" onchange="FixProxyURL();"/>
That line is long enough as it is... perhaps you could add your attribute onto
the previous line, or rearrange that tag?

Comment 11

[jag (Peter Annema)]

Neil's "localhost" example is a good reason to not force the www. and .com
prefix and suffix, www.localhost.com is probably not what you meant when you
typed localhost/proxy.pac.

Hmm, and there might be a security/privacy issue too. If you force "proxy" to
become "www.proxy.com", the owner of that domain could, for any .pac file
requested at his domain, return a .pac file which sends all URI requests to a
proxy of his choosing, allowing him to do pretty much anything he wants with the
content you've requested. Not typing the full "proxy.company.net" is an easy
enough mistake to make and would probably go undetected if www.proxy.com was set
up as described above. Of course, "proxy" is just an example, it could be any
XXX for XXX.company.net which happens to have a www.XXX.com counterpart. Just
guess a good XXX. Unlikely to actually happen, sure, but why risk it?

_NONE it is, I say.

Comment 12

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch v4

Comment 13

[[e-mail]]

Comment on attachment 157501 [details] [diff] [review]
Patch v4

Nits: you didn't have to get rid of the const nsIURIFixup; also it looks as if
your prefstring indent is a space too short.

Comment 14

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch v5

Fixes nits

Comment 15

[jag (Peter Annema)]

Comment on attachment 157506 [details] [diff] [review]
Patch v5

+    var url = URIFixup.createFixupURI(proxyURL.value,
nsIURIFixup.FIXUP_FLAG_NONE).spec;
+    proxyURL.value = url;


Just a nit: get rid of the temporary variable url there and make that one line.

It'll actually all fit within 80 character lines if you format it like this:

    var URIFixup = Components.classes["@mozilla.org/docshell/urifixup;1"]
			     .getService(nsIURIFixup);
    proxyURL.value = URIFixup.createFixupURI(proxyURL.value,
					     
nsIURIFixup.FIXUP_FLAG_NONE).spec;

And you could even move that .spec to the next line and align the dots.

+		  prefstring="network.proxy.autoconfig_url"
onchange="FixProxyURL();"/>
	 <button id="autoReload" label="&reload.label;"
accesskey="&reload.accesskey;"
		 oncommand="ReloadPAC();"
-		 prefstring="pref.advanced.proxies.disable_button.reload"/>
+		Components.interfaces
prefstring="pref.advanced.proxies.disable_button.reload"/>


It looks like there's some c&p error there.

Address these nits and the above c&p error, submit one final patch, and I think
we're ready to go :-)

Comment 16

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch v6

Comment 17

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 157615 [details] [diff] [review]
Patch v6

(In reply to comment #15)
> And you could even move that .spec to the next line and align the dots.
That leaves some strange-looking whitespace (in my opinion).

Comment 18

[[e-mail]]

Comment on attachment 157615 [details] [diff] [review]
Patch v6

This is great, it even fixes up C:\proxy.pac :-)

Comment 19

[jag (Peter Annema)]

Comment on attachment 157615 [details] [diff] [review]
Patch v6

Marking Neil's previous r= (which I'm pretty sure still applies), adding my
sr=.

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in xpfe/components/prefwindow/resources/content/pref-proxies.js;
/cvsroot/mozilla/xpfe/components/prefwindow/resources/content/pref-proxies.js,v
 <--  pref-proxies.js
new revision: 1.16; previous revision: 1.15
done
Checking in xpfe/components/prefwindow/resources/content/pref-proxies.xul;
/cvsroot/mozilla/xpfe/components/prefwindow/resources/content/pref-proxies.xul,v
 <--  pref-proxies.xul
new revision: 1.57; previous revision: 1.56
done

Comment 21

[Giacomo Magnini]

Isn't bug 30387 similar? Could it get some loving too? It has been open for the
last 5 long years... ;)

Comment 22

[Darin Fisher]

Is someone going to port this over to firefox and thunderbird?

Comment 23

[chris hofmann]

keeping on the radar, might be good to pick this up for firefox 1.0

Comment 24

[Mike Connor [:mconnor]]

bug 243397 is the firefox version of this bug, and is fixed on the aviary branch
already.  This was filed as the seamonkey equivalent to that bug.

Maybe we need a fix for Thunderbird though, but it'd be better to file a
separate bug since there'd be a different fix for Tbird.

Comment 25

[benc]

ugh. Did anyone read bug 82992?

Does fixup change the URL and save it into the pref, or simply read the pref
string, and do fixup before calling necko to load the PAC file?

Comment 26

[[e-mail]]

(In reply to comment #25)
>ugh. Did anyone read bug 82992?
Obviously not... I suppose you'll have to dup it against this one now.

>Does fixup change the URL and save it into the pref, or simply read the
>pref string, and do fixup before calling necko to load the PAC file?
Fixup changes the URL saved into the pref, because necko can't use fixup.

Comment 27

[Scott MacGregor]

I fixed this for Thunderbird too under Bug #243397 which was the one used for
firefox. Thanks for the heads up Darin!