telnet: isn't working, I get an unrecognized protocol handler error.

and?...

I can think of some really nice things here, like something similar to XMLterm, but with telnet as the network protocol in the background. In the meantime I think we should have another error beside NS_ERROR_UNKNOWN_PROTOCOL. We should also have NS_ERROR_UNSUPPORTED_PROTOCOL, which means we recognize this protocol, but we currently do not support it. This involes a stub protocolhandler that always returns NS_ERROR_UNSUPPORTED_PROTOCOL and a change in nsWebShell that pops up a related dialog box just like we do with NS_ERROR_UNKNOWN_PROTOCOL.

ugh! I can't believe we haven't fixed this yet! I ran into this today and had to run 4.x to get the helper app to launch. Add a bunch of keywords and change component to XP Apps since I'd think that helper apps should be handling this if the Networking protocol isn't supported. What I saw today was that when I clicked on a telnet:// link, I got no feedback (dialog) and nothing happened. If I type the telnet:// url in the urlbar then I got a dialog telling me that telnet is not a registered protocol.

Scott, is this a bug for you or for gagan and co.? Maybe rpotts? Who does the protocol handlers these days? 'Cause I'm pretty sure it has nothing to do with Navigator front end. If you don't know, just re-assign back to me. (Didn't realize I still had this. Ooops ...)

Sorry, this is not core, or central for browsing and reading email. marking dogfood-minus If there is extensive usage of this feature on the web, then please describe so that I can better understand why this is a dogfood blocker. (I didn't even know this existed).

triaging to necko.

hm, since telnet was handled as a helper app in 4.x (at least on Mac and unix), could it not be handled as such here? if not, okay --am just curious what the [future] implementation would be...

jar: we used this a lot for the 4.x xheads unix page. This is very useful for telnetting to a unix machine from any browser, you don't need to go hunt for the stupid little mac telnet whatever app.

helper apps in 6.0 are all dispatched based on content type. necko would need a dummy protocol handler for telnet which returns a content type. Is there a real content type for telnet? Then we'd look up in your OS registry for that content type in the hopes of finding some application for telnet. this seems like a definte rtm minus candidate to me. We are way passed the point of adding features for this release. IMHO

I came across this problem in my surfing... lots of libraries rely on telnet urls to connect to their databases. mscott--what were helper apps dispatched based on in 4.x? My 4.x Mac helper app setting has a Mimetype of Netscape/Telnet. Was this hard-coded in 4.x? I see that gopher protocol/helper app is similarly broken (there is a separate bug on this). What other protocol/helper apps are broken? We need to get these things in the release notes.

Content types are for data, not data transmission methods. Should delegating to other apps based on the URL scheme (found *before* opening a connection) and delegating to other apps based on the content type (found *after* opening a connection) really be considered the same problem (or, fed the same solution)?

I have a dummy telnet protocol handler in my tree, but it does not return a content type. Just the scheme and the port, everything else is not implemented.

rtm-, this is a feature that wasn't implemented in time.

On a recent build with XMLTerm enabled, clicking (or typing) a telnet link brings up an XMLTerm window (unfortunately, the XMLTerm does not then try to actually telnet to the desired host). Any suggestions how to either: a) tell XMLTerm to actually try to make the telnet connection, or b) set up a different helper application. BTW - NS4.7 seems to store these helper application settings like this (not that this is how you would necessarily want to do it in moz): liprefs.js:user_pref("applications.telnet", "rxvt -e telnet %h %p"); preferences.js:user_pref("applications.telnet", "rxvt -e telnet %h %p");

Placing on the Mozilla1.0 radar as something we need to be regarded as "Feature complete".

Although many people might prefer to use an external application to view telnet URLs, in my opinion it would make more sense from a UI point of view to open the telnet session inside the browser. Granted, that's not particularly easy. Perhaps this should be a separate RFE.

yes, I think it should be a separate RFE since the work required will probably be quite different than what is intended for this bug. Removing "Future" target milestone in the hopes that it will get retargeted to mozilla 1.0 or sooner.

*** Bug 61705 has been marked as a duplicate of this bug. ***

This bug (like Bug 11459 ) could be resolved by integrating Protozilla ( http://protozilla.mozdev.org/ ) into the Mozilla codebase, couldn't it? Including Protozilla would add a generalized interface for adding protocol handlers equivalent to the helper apps interface for adding MIME type handlers.

Bug 2110 leads me to believe that there is some code in Mozilla for registering external apps to handle URI schemes. Can anyone explain? For another take, see http://www.w3.org/TR/2001/NOTE-cuap-20010206#cp-uri-schemes or http://www.w3.org/Addressing/schemes

Since this worked in Communicator 4.x, whoever drops this feature from a schedule should justify it.

This feature of 4.x is used extensively here at Penn State. Anyone who needs to telnet to a specific Unix Lab computer can do so by clicking on a link on a page that lists the available workstations. Without this working, the CompSci department has to keep NS4.x on all of its machines, and must recommend to students that they NOT use Netscape6/Mozilla to telnet in. All of the major operating systems have a built in telnet app, why can't we hard code something in (like chrome://) that defines a type for telnet:// then assign that type to the telnet app. This definately needs to be fixed for Mozilla1.0

putting on nsbeta1 radar, unix people will expect this to work

Hey Penn State, how about a contribution here? A chance to work on some industrial-grade code, get some experience, and hey maybe have some fun :)

I plan on it. I am currently trying to get a group of interested students together to start an "Independent/Alternative Studies" Course involved in Mozilla. Right now, however, the beast is just too big. I heard that another university has already started a class with the Mozilla code, and was planning on seeing how that was implemented to get ideas for ours. Right now, its just testing/bug reporting for us. But back to the topic. If I read this week's status update correctly, it sounds as if dougt is working on a possible solution to this bug. Like steven-ehrbar@home.com recommended, Protozilla is a perfect fix to this problem (and several others.) Apparently Doug is working with Protozilla to integrate it with Mozilla. If this happens, I think Mozilla will have a much better method at handling protocols than anything out there. I installed protozilla here, and it works beautifully. I can telnet easily again!

Adding dougt. Mike, great! Any help your group can give mozilla is welcome, I hope Mozilla can be a good experience for your class. Also glad that dougt's work might address this bug, didn't know about that work.

Telnet Works! What happened here? I just saw one of my friends click a telnet link in mozilla .8, and he got a telnet window. I couldn't believe it and ran back to my computer to check it out. Build 2001021808 without Protozilla installed yet works! When was this fixed? What was the checkin? What is going on? ... Im confused. This is the second bug I've seen today that works without having had a patch/fix implemented. (The other was bug 57455 ) What is happening in Mozillaland? Is there a magical bug-fix fairy we should be thanking? Anyway, I can say that this appears fixed on Win98se and WinME. I'll check Linux tomorrow when I get to my machine. I'm not marking this fixed yet, because I don't know what happened, but it certainly appears fixed.

I fixed this. On windows if we attempt to load a protocol scheme that mozilla can't handle we kick it out to the OS. So your favorite telnet app is launched. This won't work for linux yet where there is no OS support to do something like this.

that would explain why someone mentioned that gopher:// spawned nc4, cool.

This is still broken in linux. Any progress ?? Cisco's http access to networking devices have a telnet:// link on the page. This will cause many networking professionals to not use mozilla. Thanks,

This is off the main radar, probably because it's linux-only. How ironic is that! Linux not having telnet: We need some mozilla help here.

qa to me.

+verifyme. VERIFYING: Mozilla 0.9 Win32+Win98. I'll hit the other OS's when I get the chance. mscott, if you fixed this, can you take ownership and mark it RESOLVED/FIXED?

windows build hands this off to the OS, so it's not really "fixed" in the mozilla sense, as I understand it. e.g. linux still doesn't do telnet://

UPDATED: This is a linux only bug, so I'm cleaning it up. -> plat/os to pc/linux - 4xp, comm 4 doesn't work either in Linux for me. +nsdogfood. This gets in my way when I use linux, and I can't stand it. This works in Mac now.

telnet:// URLs don't do anything on Mac OS X (2001072005) either, or maybe I haven't set it up right. It would be nice if a telnet://URL opened a telnet session in Terminal.app or the user's preferred telnet client.

I think it's time to put protozilla in the tree and enable it by default as an extension. Watch it open a telnet window when you click on a telnet link or put it into the urlbar on linux ...

+relnote for NS61. RTM Train: RELNOTE: telnet:// URLs do not work in Linux (and possibly other UNIX systems). telnet:// URL's work for Win32 and MacOS.

Relnote: Workaround: install protozilla < http://protozilla.mozdev.org/ >

For the release note, I think you can safely pull out the "possibly" from "(and possibly other UNIX systems)" - it doesn't work on Solaris and from browsing the code, I don't see how it would work on any *nix.

RELNOTE: Please put the protozilla reference only in mozilla and not netscape 6.

CONFIRMED: not working in Mac OS X, Mozilla 0.9.4.

UPDATE: telnet:// URLs work in MacOS X on Mozilla 0.9.5 running in classic mode. (now I'm confused why.)

Just to confirm it doesn't work on solaris/sparc, so shouldn't be marked pc/linux.

If a bug only affects unix platforms, it's usually left as PC/Linux, because there are more bugs that affect all unix platforms (but not other platforms) than bugs that affect only PC/Linux.

Shouldn't this get a 4xp keyword for better searchability?

4xp.

I'm not a Unix person so let me tell you what I did. There are already preferences in the .js files that have the syntax for telnet. For example: pref("applications.telnet", "xterm -e telnet %h %p"); I've added code that detects this pref is present, and then properly substitutes host and port and then it has a string to launch. I don't know how to actually execute the code on unix - this is probably one line of code.

Argh. For anyone testing this patch - application. should be applications. I did not do any testing of the patch - I have a similar patch for OS/2 that does everything the same except it gets the launch info from the OS and it uses an nsIFile to launch the file.

bz: I really want this to work for Mozilla 1.0. Can you work some of your bz magic?

I can try... I mailed Michael some stuff on how to launch things on Unix. I can definitely test, and after May 10 I can usefully code... till then, I cannot.

I'm making the OS/2 patch much more like Linux, so when I am done, the shell executing should be the only thing left. We have a Linux box so we will try it there. Hope to have something by Friday.

It looks like the preferences need some enhancing. Here's an example. What if your telnet using a syntax like this?: pref("applications.telnet", "xterm -e telnet %h -port %p -user %u"); We need to be able to remove the -port if no port is specified or the -user if the user is not specified. So here is the proposal: pref("applications.telnet", "xterm -e telnet"); pref("applications.telnet.host", "%h"); pref("applications.telnet.port", "-port %p"); pref("applications.telnet.user", "-user %u"); And then we concatenate them into the string. What do people think of this? Thanks

what happens if an app needs to care about order? pref("applications.telnet", "xterm -e telnet"); pref("applications.telnet.host", "xterm -e telnet %h"); pref("applications.telnet.host_port", "xterm -e telnet %h -port %p"); pref("applications.telnet.host_port_user", "xterm -e telnet %h -user %u -port %p"); pref("applications.telnet.host_user", "xterm -e telnet -user %u %h"); (this requires one very drunken telnet app, but the general idea is that there might be some app that implements some protocol that cares ...)

This uses the new syntax I specified. I haven't had a change to test on Linux yet, but it should build. Feel free to critique my string handling - I'm sure I've done something wrong.

Comment on attachment 81165 [details] [diff] [review] New patch >+ nsCOMPtr<nsIURI> uri = do_CreateInstance(kStandardURLCID, &rv); >+ if (uri) >+ { >+ nsCAutoString urlSpec; >+ aURL->GetSpec(urlSpec); >+ uri->SetSpec(urlSpec); This part looks a little odd... is aURI an nsSimpleURI or something? >+ uri->GetScheme(uProtocol); >+ uri->GetHost(uHost); >+ uri->GetPort(&port); >+ if (port != -1) { >+ uPort.AppendInt(port); >+ } >+ uri->GetUsername(uUsername); Would it not be better to lazily get these once we know we need them? >+ nsCString prefName; Make that an nsAutoCString >+ prefName.Append("."); At this point you want to get an nsIPrefBranch for that prefName (getPrefBranch on the prefService). That lets you avoid tempPrefName and all that gunk. >+ parameters.Append(" "); >+ parameters.Append(prefString); >+ PRInt32 pos; >+ pos = parameters.Find("%h"); >+ if (pos != -1) >+ parameters.Replace(pos, 2, uHost); >+ } Um... would it not be better to replace _before_ appending? Likewise below. >+ // if we were given an application to use then use it....otherwise >+ // make the registry call to launch the app This comment is bogus... :) >+ parameters.Insert("-c \", 0); >+ parameters.Append(\"); This is also bogus. You want to create an array containing two strings: const char foo[2] = {"-c", parameters.get()}; Then you want to pass "foo" as the second argument to nsIProcess::Run. Right now, you'd be launching /bin/sh with a single command-line argument of '-c "telnet foo.bar"' which is not at all what you want. >+/* End OS/2 specific */ This seems out of place too.

I think that is getting a little extreme, especially considering technically there are four things that get passed - host, protocol, user, pass: You would end up with six different preferences which is silly. What I am also implementing is a generic application.telnet.parameters which uses the old way for these situations. It has the interesting side effect of being able to create any arbitrary protocal to do anything, so you could have: edit:filename and it would edit a file or calc: and it would open a calculator. Cool stuff

> >+ aURL->GetSpec(urlSpec); > >+ uri->SetSpec(urlSpec); > This part looks a little odd... is aURI an nsSimpleURI or something? Yep, aURI is a Simple URI. Originally I thought we needed to convert helpers to use Standard (bugzilla 135550) but then I discovered I could just set the SimpleURI spec into a Standard and use it. > >+ uri->GetUsername(uUsername); > Would it not be better to lazily get these once we know we need them? Will do > >+ nsCString prefName; > Make that an nsAutoCString Will do > >+ prefName.Append("."); > At this point you want to get an nsIPrefBranch for that prefName Any good doc on this? > You want to create an array containing two strings: > const char foo[2] = {"-c", parameters.get()}; I'll look at this

> >+ uri->GetScheme(uProtocol); > >+ uri->GetHost(uHost); > >+ uri->GetPort(&port); > >+ if (port != -1) { > >+ uPort.AppendInt(port); > >+ } > >+ uri->GetUsername(uUsername); > Would it not be better to lazily get these once we know we need them? Actually, I do need to get these earlier. I do not even want to query the .port pref from the preferences if a port was not specified on the URL. If I go the other way, I'll always be appending the -p %p even if a port wasn't specified, and then the substituion will leave a dangling -p in the launchString. >>+ parameters.Append(" "); >>+ parameters.Append(prefString); >>+ PRInt32 pos; >>+ pos = parameters.Find("%h"); >>+ if (pos != -1) >>+ parameters.Replace(pos, 2, uHost); >>+ } > Um... would it not be better to replace _before_ appending? Likewise below I'd love to, but unfortunately nsXPIDLCString does not have a .Find function. So I have to put it into an nsCString before I can use .Find.

> I'd love to, but unfortunately nsXPIDLCString does not have a .Find function. So > I have to put it into an nsCString before I can use .Find. Oh, right... In that case, could you call the version of find that takes a starting offset? string.Find(str_to_find, PR_FALSE, offset); that way you won't accidentally replace a %p that happens to be in the command name with the port number (and will do a lot less searching, in any case).

I don't know enough about how nsIApplication works on Unix, so please bear with me. can't I create an nsIApplication object with a fully qualified path to telnet and just invoke telnet directly? Why do I have to pass it to the shell?

Well... if you have a fully qualified path, you can. But all you have is "telnet". And please do not make this depend on the GetFileTokenForPath() function in that file -- I will be removing that as soon as I can....

On OS/2 I am going to mandate that the application name be fully qualified. Since that is not the norm on Linux, I won't. We'll just use the /bin/sh trick. Patch coming up soon.

Syntax has changed a bit since I learned about network.protocol-handler.external. To use this for telnet do: pref("network.protocol-handler.external.telnet", true); pref("applications.telnet", "xterm -e telnet"); pref("applications.port", "%port"); pref("applications.host", "%host"); If you want to create something that handles mailto (assuming the application can take a mail address as an input) pref("network.protocol-handler.external.mailto", true); pref("applications.telnet", "xterm -e pine"); pref("applications.parameters", "%url"); I really need some help on the Linux side for someone to try and build and test this. It is working great on OS/2. Thanks

The previous patch doesn't compile for me. I'm fixing one obvious problem (a missing semicolon) and attaching it here. It still doesn't compile for me on Sparc Solaris 7 with gcc 2.95.3. The errors I get: ./unix/nsOSHelperAppService.cpp: In method `nsresult nsOSHelperAppService::LoadUrl(nsIURI *)': ./unix/nsOSHelperAppService.cpp:1139: warning: unknown escape sequence `\c' ./unix/nsOSHelperAppService.cpp:1146: no matching function for call to `nsCString::append (nsXPIDLCString &)' ./unix/nsOSHelperAppService.cpp:1157: `paramURL' undeclared (first use this function) ./unix/nsOSHelperAppService.cpp:1157: (Each undeclared identifier is reported only once ./unix/nsOSHelperAppService.cpp:1157: for each function it appears in.) ./unix/nsOSHelperAppService.cpp:1224: cannot allocate an object of type `nsACString' ./unix/nsOSHelperAppService.cpp:1224: since the following virtual functions are abstract: ../../dist/include/string/nsAString.h:634: char * nsACString::GetWritableFragment(nsWritableFragment<char> &, nsFragmentRequest, unsigned int = 0) ../../dist/include/string/nsAString.h:633: const char * nsACString::GetReadableFragment(nsReadableFragment<char> &, nsFragmentRequest, unsigned int = 0) const ../../dist/include/string/nsAString.h:414: PRUint32 nsACString::Length() const ../../dist/include/xpcom/nsILocalFile.h:361: in passing argument 1 of `NS_NewLocalFile(const nsACString &, int, nsILocalFile **)' make[1]: *** [nsOSHelperAppService.o] Error 1

I don't know what I was thinking. There is nothing Unix specific about that patch except for the /bin/sh. I changed it to CMD.EXE for OS/2 and built and test. This patch should work.

Comment on attachment 81541 [details] [diff] [review] Patch that builds and runs I assume that comment 64 was wrong in that you'd want to set "applications.mailto" to "xterm -e pine", and set "applications.telnet.host" to "%host"... I'll try to build and test this later this week.... comments from just reading: >+#include "nsLocalFile.h" Should this not be nsILocalFile.h? >+ nsCAutoString prefName("network.protocol-handler.external."); >+ prefName += aProtocolScheme; This is wrong, imo... If you look at (nsIOService::GetProtocolHandler) http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsIOService.cpp#402 it does something like: If (!external_handler_pref_set) { rv = TryToGetInternalHandler(); } if (external_handler_pref_set || NS_FAILED(rv)) { rv = TryToGetHandlerFromOS(); if (NS_FAILED(rv)) { return error_that_causes_unknown_protocol_alert; } } So we could have (!external_handler_pref_set) be true, fail to handle it internally, call down into this code... and fail again because you're checking this pref down here. >+ PRInt32 port; >+ uri->GetPort(&port); >+ if (port != -1) >+ uPort.AppendInt(port); Wouldn't it be better to initialize |port| and maybe even check the return value of GetPort() ? In fact, a lot of these GetFoo() things on the uri could use return-value checking... >+ nsCString paramHost; >+ nsCString paramPort; >+ nsCString paramUsername; >+ nsCString paramPassword; These are only needed in little tiny pieces of code below, no? Could you just define a single |nsAutoCString tempStr;| in the smallest scope that will work for all the places you need it... >+ const char *params[4]; Should that not be |const char *params[2]|? I don't see how you can end up with more params than that... >+ PRInt32 currentParam = 0; This becomes unneeded given the above... there are always two params -- "-c" and the full command string. >+ params[0] = "/c"; "-c" >+ rv = thePrefsService->GetBranch(prefName.get(), getter_AddRefs(prefBranch)); Check the return value? Or the prefBranch before dereferencing it? >+ prefBranch->GetCharPref("parameters", getter_Copies(prefString)); >+ /* If parameters have been specified, use them */ >+ if (NS_SUCCEEDED(rv) && !prefString.IsEmpty()) { You forgot |rv = | before the GetCharPref call? :) >+ parameters.Append(prefString); >+ pos = parameters.Find("%url"); Tou may want to get parameters.Length() before the Append(), and start your Find() at that point.... Right now, if "%url" is already in the string (as part of appname) this is wrong. >+ uURL.Replace(0, uProtocol.Length()+1, NS_LITERAL_CSTRING("")); You can use .Cut() here... >+ parameters.Replace(pos, strlen("%url"), uURL); use |sizeof("%url") - 1| instead of the strlen (then it's computed at compile-time. >+ paramPort.Replace(pos, strlen("%port"), uPort); Same (and other places below). In fact, you could define NS_NAMED_LITERAL_CSTRING() vars for those strings.... that would speed up the Find() calls (since those have to wrap into an nsDependentCString as you have it) and you could just use .Length() in the Replace() calls... >+ rv = NS_NewLocalFile(nsDependentCString("/bin/sh"), PR_FALSE, getter_AddRefs(application)); NS_LITERAL_CSTRING("/bin/sh") is faster -- it computes length at compile-time.... >+ if (NS_FAILED(rv = process->Run(PR_FALSE, params, currentParam, &pid))) >+ return rv; Does entering a nonexistent app get caught here? Or does the exitValue on the process need to be checked? Is it even possible to usefully check that in an async situation? :( >+ return NS_ERROR_FAILURE; Do callers actually do something useful with this error (like put up a dialog saying "could not start application")?

I've included a patch fixing the '/c' to '-c' problem. I've also added the relevant lines from unix.js, since we'll want those checked in as well so that telnet works by default. I've left the mailto: lines commented out, since we'll want using an external mailer to be optional. The telnet:// fix seems to be working fine. It even works with using ssh instead of telnet. Mailto: don't quite work yet - at least with the prefs that I've attached. I can get an external mailer to open up, but I can't feed it the To: address for the mail. I think the that the nsOSHelperAppService.cpp, might still need some tweaking, but I haven't looked too closely yet.

> Mailto: don't quite work yet - at least with the prefs that I've attached. I > can get an external mailer to open up, but I can't feed it the To: address I'm surprised it works at all. That branch of the code never adds the app to |params| and never increments |currentParam|. I did not bother to comment on that, since the current setup there is so broken anyway.... (see comment 67)

I'm learning while I am doing this, so I hope people don't mind all the patches. This is yet another overhaul. I think this is near final. I have taken all of bzbarsky's comments into this one. Incidentally, the reason it looked kind of weird in the last patch is because it was a port of the OS/2 one which does have the ability to pass multiple parameters to the nsiApplications. This one is totally Unix specific. I have cleaned up the error checking significantly which makes the code look a lot cleaner. I changed the substitution char to be %string% so it would be more unique and more consistent with other stuff I have seen in the tree. I moved the substitution to the very end. This will allow you to use the substitution in the parameters statement as well as in host,port, etc. I've cleaned up the string code quite a bit. I've changed the checking for the existence of a handler back to checking applications.protocol. I believe that the network.protocol-handler.external.protocol is really only intended to override existing protocols. So here are working examples from the OS/2 code that show what this can do: Here is a normal telnet application: pref("applications.telnet", "telnetpm.exe"); pref("applications.telnet.host", "%host%"); pref("applications.telnet.port", "-p %port%"); Here is code that allows you to type epm:filename in the url bar and it will open an editor. pref("applications.epm", "c:\\os2\\apps\\epm.exe"); pref("applications.epm.parameters", "%url%"); Here is code that given any fully qualified URL on the urlbar like netscape:www.mozilla.org/ports/os2, it will open netscape.exe with just the host. pref("network.protocol-handler.external.netscape", true); pref("applications.netscape", "netscape.exe"); pref("applications.netscape.parameters", "%host%");

Can we move the mailto: fix into a new bug? For one thing, I have doubts that all systems have PINE. Also, that bug would be verified by a different QA person. I care a ton about having telnet work in Linux/UNIX. I don't care nearly as much about mailto: b/c mozilla has a UA.

+ nsCString parameters; Make this a nsCAutoString? + PRInt32 iPort; + uri->GetPort(&iPort); + if (iPort != -1) Either initialize iPort to start with or error-check the GetPort(), preferably the latter. I'm not sure about attempting to replace %url even when we never appended the "parameters" to the command.... Should we perhaps have a bool that keeps track of which of the two branches we followed there and replace conditionally based on that bool? Minor nit, sorry about not saying it last time: use kNotFound instead of "-1"? Other than that, this is looking pretty good. Thanks for doing it, Michael!

The current patch (even when switching to "nsCAutoString parameters;") doesn't compile for me: ./unix/nsOSHelperAppService.cpp: In method `nsresult nsOSHelperAppService::LoadUrl(nsIURI *)': ./unix/nsOSHelperAppService.cpp:1198: no matching function for call to `nsCAutoString::Find (nsDependentCString &)' ../../dist/include/string/nsString.h:288: candidates are: PRInt32 nsCString::Find(const nsCString &, int = 0, int = 0, int = -1) const ../../dist/include/string/nsString.h:289: PRInt32 nsCString::Find(const char *, int = 0, int = 0, int = -1) const ./unix/nsOSHelperAppService.cpp:1205: no matching function for call to `nsCAutoString::Find (nsDependentCString &)' ../../dist/include/string/nsString.h:288: candidates are: PRInt32 nsCString::Find(const nsCString &, int = 0, int = 0, int = -1) const ../../dist/include/string/nsString.h:289: PRInt32 nsCString::Find(const char *, int = 0, int = 0, int = -1) const ./unix/nsOSHelperAppService.cpp:1209: no matching function for call to `nsCAutoString::Find (nsDependentCString &)' ../../dist/include/string/nsString.h:288: candidates are: PRInt32 nsCString::Find(const nsCString &, int = 0, int = 0, int = -1) const ../../dist/include/string/nsString.h:289: PRInt32 nsCString::Find(const char *, int = 0, int = 0, int = -1) const ./unix/nsOSHelperAppService.cpp:1213: no matching function for call to `nsCAutoString::Find (nsDependentCString &)' ../../dist/include/string/nsString.h:288: candidates are: PRInt32 nsCString::Find(const nsCString &, int = 0, int = 0, int = -1) const ../../dist/include/string/nsString.h:289: PRInt32 nsCString::Find(const char *, int = 0, int = 0, int = -1) const ./unix/nsOSHelperAppService.cpp:1217: no matching function for call to `nsCAutoString::Find (nsDependentCString &)' ../../dist/include/string/nsString.h:288: candidates are: PRInt32 nsCString::Find(const nsCString &, int = 0, int = 0, int = -1) const ../../dist/include/string/nsString.h:289: PRInt32 nsCString::Find(const char *, int = 0, int = 0, int = -1) const make[1]: *** [nsOSHelperAppService.o] Error 1 The correct bug for the mailto: portions of this patch is probably bug 33282.

> The correct bug for the mailto: portions of this patch is probably bug 33282. Ak - I meant bug 11459.

ARGH. Alecg removed find yesterday and my code is based on the day before. I guess I have to rewrite some more

Comment on attachment 81686 [details] [diff] [review] Playing the patch game I only removed Find() for nsString's, and its been gone for quite some time :) You can still use nsCString& or const char*. for the trunk I have an upgraded Find() that works on nsASingleFragmentSubstring so that your NS_NAMED_LITERAL_CSTRINGs will work. my suggestion is const char*, so you can say parameters.Find(url.get()) that said, "parameters" and "urlSpec" should be nsCAutoString here.

> + PRInt32 iPort; > + uri->GetPort(&iPort); > + if (iPort != -1) > Either initialize iPort to start with or error-check the GetPort(), preferably > the latter. See: http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsStandardURL.cpp#911 GetPort doesn't return an error. The only notion that there was no port specified is if it returns a -1, and it always returns a value.

Alright, I'm shooting for final patch here. -1 changed to kNotFound. Per alecf, I changed the .Find to use char* by using .get - I'm assuming he said this because it makes the Find more efficient. Fixed a bug in ExternalProtocolExists. Moved the %url% substitution back up into parameters like bzbarsky suggested. The one thing I don't have that bz wants is starting the Find for %url% at the point where the URL string is appended. If you feel strongly about that, I will do it. I also changed the two nsCStrings to nsCAutoStrings. The current patch is compiling for me on OS/2 current trunk, so it should compile elsewhere.

> + pos = parameters.Find(username); You want a .get() there, no? :) Also, your parameter order is broken... The standard telnet order is: telnet [options] [host [port]] You generate telnet port options host so telnetting to a non-default port fails. Please append the port last?

> Also, your parameter order is broken... The standard telnet order is: > telnet [options] [host [port]] I think I am going to cry. OS/2 requires the host name last (even though we use -p for parameter) Stupid broke OS/2 telnet. I had pondered having some sort of architecture for specifying the order of the parameters in the prefs.js, but I could never get my head around how to do it. I'll just have different orders of params for OS/2 and Unix since telnet is the primary consumer of this.

This version doesn't compile either: unix/nsOSHelperAppService.cpp: In member function `virtual nsresult nsOSHelperAppService::LoadUrl(nsIURI*)': unix/nsOSHelperAppService.cpp:1212: no matching function for call to `nsCAutoString::Find(nsDependentCString&)' ../../dist/include/string/nsString.h:288: candidates are: PRInt32 nsCString::Find(const nsCString&, int = 0, int = 0, int = -1) const ../../dist/include/string/nsString.h:289: PRInt32 nsCString::Find(const char*, int = 0, int = 0, int = -1) const

Jim crumley: Are you on the branch or trunk? How recent is your build?

My build is from the trunk pulled earlier today. I'll try again.

Jim is likely running into the first problem I pointed out in comment 79 -- the missing .get()

Changed order to username/password/host/port Added .get

Comment on attachment 82098 [details] [diff] [review] Last two things bz mentioned r=bzbarsky

Ok, the current patch to nsOSHelperAppService.cpp compiles and works great for me. This is a patch to unix.js to turn on external telnet (and tn3270 and rlogin) on by default. Also, I found a scheme that will work for external mailto: - I'll mention it in bug 11459.

please create a placeholder bug for every url scheme you add. Just reference this bug (no need to break up the patch or attach to each bug...

This patch turns on external applications to take care of several protocols referenced in related bugs, hopefully clearing up those bugs. For some of the protocols referenced in this bug I'm unsure if I have chosen the "best" application, so suggest better apps if you wish. Protocol Bug Comments telnet bug 33282 rlogin bug 142559 tn3270 bug 142557 ssh bug 142790 rtsp bug 82776 Handoff to realplay, but QT also uses this link type pnm bug 37637 Older protocal used by Real tel bug 37637 calls a speakerphone app, should call IP telephone app? fax bug 37637 modem bug 37637 sip bug 37637 dict bug 44317 ldap bug 37637 calls ldapsearch, should call an LDAP browser? mailto bug 11459 commented out because we want Mail/News to be default Any other protocols that ot would be useful to add?

Unfortunately, the nsOSHelperAppService patch no longer compiles: ./unix/nsOSHelperAppService.cpp: In method `nsresult nsOSHelperAppService::LoadUrl(nsIURI *)': ./unix/nsOSHelperAppService.cpp:1230: cannot allocate an object of type `nsAString' ./unix/nsOSHelperAppService.cpp:1230: since the following virtual functions are abstract: ../../dist/include/string/nsAString.h:339: PRUnichar * nsAString::GetWritableFragment(nsWritableFragment<short unsigned int> &, nsFragmentRequest, unsigned int = 0) ../../dist/include/string/nsAString.h:338: const PRUnichar * nsAString::GetReadableFragment(nsReadableFragment<short unsigned int> &, nsFragmentRequest, unsigned int = 0) const ../../dist/include/string/nsAString.h:139: PRUint32 nsAString::Length() const ../../dist/include/xpcom/nsILocalFile.h:375: in passing argument 1 of `NS_NewLocalFile(const nsAString &, int, nsILocalFile **)' make[1]: *** [nsOSHelperAppService.o] Error 1

Yeah, the NS_LITERAL_CSTRING needs to be changed to NS_LITERAL_STRING in the NS_NewLocalFile() call, since Darin smaked the nsIFile API again.

i promise not to smack nsIFile again... honest ;-)

Comment on attachment 82098 [details] [diff] [review] Last two things bz mentioned >Index: nsOSHelperAppService.cpp >+ nsCAutoString prefName("applications."); >+ prefName += aProtocolScheme; nit: nsCAutoString prefName; prefName = NS_LITERAL_CSTRING("applications.") + nsDependentCString(aProtocolScheme); using "||" seems dangerous... imagine NS_FAILED(rv) with garbage in prefString. >+ if (NS_SUCCEEDED(rv) || !prefString.IsEmpty()) { and (nit) how about this: *aHandlerExists = (NS_SUCCEEDED(rv) && !prefString.IsEmpty()); instead of the "if" conditional. >+ nsCOMPtr<nsIURI> uri = do_CreateInstance(kStandardURLCID, &rv); >+ if (NS_FAILED(rv) || !uri) { no need to be redundant here... just check |rv| or |uri| but not both. >+ nsCAutoString prefName; >+ prefName.Append("applications."); >+ prefName.Append(uProtocol); nit: prefName = NS_LITERAL_CSTRING("applications.") + uProtocol; >+ uri->GetPort(&iPort); >+ if (iPort != kNotFound) kNotFound?? what does that have to do with nsIURI?? don't mix literals like this. >+ nsCAutoString uHost; >+ uri->GetHost(uHost); i'm guessing that GetAsciiHost would be more appropriate here. most telnet apps probably won't understand UTF-8 internationalized domain names. speaking of non-ASCII chars... have you thought about what happens if nsIURI::GetSpec returns a non-ASCII string? or what if there are escape sequences? you might want to unescape the username and password, for example. username may include a ':' or a '@' or some other fun character. what happens if we return an error? will the user be notified? >+ return NS_OK;

All changes made. the reason for the stray kNotFound is I did a global change of -1 to kNotFound. GetPort returns -1 if there is no port. I have made this clearer. I did testing and username and password would be the only ones that would have escaped chars, so I unescape them. As far as return code, nothing is done with the return code at this point in time. We only return NS_OK if everything succeeded though.

*** Bug 145085 has been marked as a duplicate of this bug. ***

moving this to the person who is working on it so i can find it with normal searches

Jim Crumley's attachment in comment #89 needs work. pref("network.protocol-handler.external.rtsp", true); is added twice. The second one is a typo. It should read "pnm" instead of "rtsp".

Fixed redundancy.

Oops, found another mistake. Hopefully this one is OK.

mkaply, this patch uses the deprecated nsIPref interface, see http://lxr.mozilla.org/seamonkey/source/modules/libpref/public/nsIPref.idl#47 you should use nsIPrefService/nsIPrefBranch

It wouldn't have been if this fix got checked in when it was done.

Let's try to get this for 1.1, PLEASE - Is there anything else but nsIPrefService that needs changing? it's using nsIPrefBranch already. Do we need to worry about any shell-special characters when the external app is launched? &|><%$() etc. I'd love to be able to redirect them to a URL as well, but that's a different bug (protozilla probably).

Does this patch cover only Linux? Do we need a separate Mac OS X version of this bug?

The patch is generic enough that it should work anywhere. I used it on OS/2 just by changing the name of the shell and the parameter that is passed to start an application.

MacOSX builds the Mac version of nsOSHelperAppService. That means that it is configurable via Internet Config and will follow the settings there for protocols. In other words, it should not need this patch. Not sure how mach-o builds fit into this picture....

I disagree with Jim Crumley that these protocols should be enabled by default. I remember some problem with 4.x where users were advised to disable the telnet Helper App, but I unfortunately don't remember specifics. There is a real risk that any of the apps that you add to the list has some bug in the commandline argument handling, which causes local exploits. Often, the assumption is that the one specifying the commandline is the one owning the process, but this is only half true here. What, for example, if any of these apps accepts an argument to start a command? What if an attacker manages to get a space and "-" in one of the parameters? If both is true, he could, for example, use a hostname of "dummy -postrun rm -rf /" and have the local application "rm -rf /" started. What, if there is a buffer overflow in one of these apps with one of the passed parameters? For example, what happens, if you pass 346357457457457 as port? This is a huge array of applications and parameters to choose from for attackers. It is very easy to start certain URLs from websites (any link could be such a URL or it could be started via JS). Also, consider that we intentionally block certain ports to be accessed, to avoid DoS-attacks on intranet resources, using the browser as proxy. Of course, this is all made-up, but I hope you can see that there is a real danger. That's why I think that it is very lightheaded to add all those handler applications by default. If you want to do that, please make a thourough check of the Mozilla code, the added apps and let it be checked by a security person. (Yes, the same could apply to the content handlers, which are already in, but I am not comfortable with that either.) On a different note, without wanting to disminish mkaply's hard work, shouldn't we use a scheme similar to mailcap (/etc/mailcap and ~/.mailcap) for protocol handlers?

I thought about creating some sort of file registry for the info, but we would just be inventing yet another file format. mailcap and mimetypes already exist.

The telnet: URL format is pretty clearly defined, so I think we just have to trust telnet clien vendors to do this correctly.

It might be too late for this suggestion, but what if we allowed opening telnet connection with Chatzilla? This way, we handle all the MUDs, MUCKs, Talkers etc without any external app, and allowing some vt100 emulation and typing into the chat window of chatzilla, provide one major feature (MUD client) nearly for free, and one (a real telnet, as opposed to the odd and evil windows thing :) rather cheap (vt100/ANSI support)

If you read this bug, you'll note that the patch implements a generic mechanism for handling any protocol with a helper. There's no reason Chatzilla could not provide a telnet protocol handler. Then this patch would not even kick in. File a bug on Chatzilla to that effect.

updated summary to include Mac OS X. made plat/os == all, because there was no better choice. interestingly, this seems to work for Chimera in recent builds, see bug 167118.

Um... this patch has nothing to do with OS X and will not affect OS X in any way. Please file a separate bug for OS X if this is an issue on OS X. This bug is for Linux/Solaris/BSD/etc (and we typically put such bugs in the Linux bucket, so back it goes).

I'll move Mac OS X. This has been a testcase for some time, so it gets checked on all plats.

I tried this patch on mozilla 1.1, but i get lots of rejects. is there a version working on 1.1? Is there any chance that this will go into 1.2? The problem with the external mail applications is the most important missing feature of mozilla, i think.

bz: Mac OS X is in bug 96217, I had forgotten about this bug. My apologies.

mozilla 1.1 keyword cleanup

*** Bug 171869 has been marked as a duplicate of this bug. ***

*** Bug 173087 has been marked as a duplicate of this bug. ***

*** Bug 173194 has been marked as a duplicate of this bug. ***

telnet:// broken in 1.2b w/ linux. Protozilla does not work and no information available at http://protozilla.mozdev.org/index.html Please fix, lack of telnet:// support is preventing cisco from using mozilla on linux and solaris

*** Bug 179789 has been marked as a duplicate of this bug. ***

mkaply, what's the status here? Is the path ready for review? If so, did you ask for it already?

I've asked for review multiple times from multiple people. Nothing ever goes anywhere on this, so I pretty much gave up, since technically it is an SEP* as it relates to my job. There was a request to rewrite the way I do prefs, but since the rest of the tree does it my way, that seems kind of secondary to getting the patch in. Last time I checked, the patch worked great and did exactly as it is supposed to do and broke nothing. Review from anyone would be much appreciated. * = Somebody Else's Problem - see Hitchhiker's Guide to the Galaxy trilogy

Comment on attachment 82098 [details] [diff] [review] Last two things bz mentioned >Index: nsOSHelperAppService.cpp >=================================================================== > .[...] > >+static NS_DEFINE_CID(kStandardURLCID, NS_STANDARDURL_CID); >+ In general contractIDs are preferred to CIDs. >@@ -1070,14 +1074,173 @@ > > NS_IMETHODIMP nsOSHelperAppService::ExternalProtocolHandlerExists(const char * aProtocolScheme, PRBool * aHandlerExists) > { >- // look up the protocol scheme in the windows registry....if we find a match then we have a handler for it... >+ /* if applications.protocol is in prefs, then we have an external protocol handler */ >+ nsresult rv; >+ nsCAutoString prefName("applications."); >+ prefName += aProtocolScheme; nsCAutoString prefName = NS_LITERAL_CSTRING("applications") + aProtocolScheme is preferred; in some cases it will save allocations (probably not here, but still). >+ >+ nsCOMPtr<nsIPref> thePrefsService(do_GetService(NS_PREF_CONTRACTID)); >+ if (thePrefsService) { >+ nsXPIDLCString prefString; >+ rv = thePrefsService->CopyCharPref(prefName.get(), getter_Copies(prefString)); >+ if (NS_SUCCEEDED(rv) || !prefString.IsEmpty()) { >+ *aHandlerExists = PR_TRUE; >+ return NS_OK; >+ } >+ } > *aHandlerExists = PR_FALSE; > return NS_OK; > } nsIPref is deprecated; please use nsIPrefBranch. > NS_IMETHODIMP nsOSHelperAppService::LoadUrl(nsIURI * aURL) > { >- return NS_ERROR_NOT_IMPLEMENTED; >+ nsCOMPtr<nsIPref> thePrefsService(do_GetService(NS_PREF_CONTRACTID)); >+ if (!thePrefsService) { >+ return NS_ERROR_FAILURE; >+ } See above. >+ >+ /* Convert SimpleURI to StandardURL */ Is it known that you won't already have a standard URL? i.e. is it possible that QI could just work here? >+ nsresult rv; >+ nsCOMPtr<nsIURI> uri = do_CreateInstance(kStandardURLCID, &rv); Use contractID, please. >+ if (NS_FAILED(rv) || !uri) { >+ return NS_ERROR_FAILURE; >+ } >+ nsCAutoString urlSpec; >+ aURL->GetSpec(urlSpec); >+ uri->SetSpec(urlSpec); >+ >+ /* Get the protocol so we can look up the preferences */ >+ nsCAutoString uProtocol; >+ uri->GetScheme(uProtocol); >+ >+ nsCAutoString prefName; >+ prefName.Append("applications."); >+ prefName.Append(uProtocol); >+ nsXPIDLCString prefString; >+ >+ rv = thePrefsService->CopyCharPref(prefName.get(), getter_Copies(prefString)); >+ if (NS_FAILED(rv) || prefString.IsEmpty()) { >+ return NS_ERROR_FAILURE; >+ } >+ Same string and nsIPref issues as mentioned previously. >+ nsCAutoString parameters; >+ >+ /* Put application name in parameters */ >+ parameters.Append(prefString); >+ >+ nsCAutoString uPort; >+ PRInt32 iPort; >+ uri->GetPort(&iPort); >+ if (iPort != kNotFound) >+ uPort.AppendInt(iPort); >+ >+ nsCAutoString uUsername; >+ uri->GetUsername(uUsername); >+ >+ nsCAutoString uPassword; >+ uri->GetPassword(uPassword); >+ >+ nsCAutoString uHost; >+ uri->GetHost(uHost); >+ >+ prefName.Append("."); using prefName.Append('.') should be a bit faster, I think. >+ nsCOMPtr<nsIPrefBranch> prefBranch; >+ rv = thePrefsService->GetBranch(prefName.get(), getter_AddRefs(prefBranch)); >+ if (NS_SUCCEEDED(rv) && prefBranch) { >+ rv = prefBranch->GetCharPref("parameters", getter_Copies(prefString)); >+ /* If parameters have been specified, use them instead of the separate entities */ >+ if (NS_SUCCEEDED(rv) && !prefString.IsEmpty()) { >+ parameters.Append(" "); >+ parameters.Append(prefString); >+ >+ NS_NAMED_LITERAL_CSTRING(url, "%url%"); >+ >+ PRInt32 pos = parameters.Find(url.get()); >+ if (pos != kNotFound) { >+ nsCAutoString uURL; >+ aURL->GetSpec(uURL); >+ uURL.Cut(0, uProtocol.Length()+1); >+ parameters.Replace(pos, url.Length(), uURL); >+ } >+ } else { >+ /* username */ >+ if (!uUsername.IsEmpty()) { >+ rv = prefBranch->GetCharPref("username", getter_Copies(prefString)); >+ if (NS_SUCCEEDED(rv) && !prefString.IsEmpty()) { >+ parameters.Append(" "); >+ parameters.Append(prefString); >+ } >+ } >+ /* password */ >+ if (!uPassword.IsEmpty()) { >+ rv = prefBranch->GetCharPref("password", getter_Copies(prefString)); >+ if (NS_SUCCEEDED(rv) && !prefString.IsEmpty()) { >+ parameters.Append(" "); >+ parameters.Append(prefString); >+ } >+ } >+ /* host */ >+ if (!uHost.IsEmpty()) { >+ rv = prefBranch->GetCharPref("host", getter_Copies(prefString)); >+ if (NS_SUCCEEDED(rv) && !prefString.IsEmpty()) { >+ parameters.Append(" "); >+ parameters.Append(prefString); >+ } >+ } >+ /* port */ >+ if (!uPort.IsEmpty()) { >+ rv = prefBranch->GetCharPref("port", getter_Copies(prefString)); >+ if (NS_SUCCEEDED(rv) && !prefString.IsEmpty()) { >+ parameters.Append(" "); >+ parameters.Append(prefString); >+ } >+ } >+ } >+ } >+ >+ PRInt32 pos; >+ >+ NS_NAMED_LITERAL_CSTRING(username, "%username%"); >+ NS_NAMED_LITERAL_CSTRING(password, "%password%"); >+ NS_NAMED_LITERAL_CSTRING(host, "%host%"); >+ NS_NAMED_LITERAL_CSTRING(port, "%port%"); >+ >+ pos = parameters.Find(username.get()); >+ if (pos != kNotFound) { >+ parameters.Replace(pos, username.Length(), uUsername); >+ } >+ pos = parameters.Find(password.get()); >+ if (pos != kNotFound) { >+ parameters.Replace(pos, password.Length(), uPassword); >+ } >+ pos = parameters.Find(host.get()); >+ if (pos != kNotFound) { >+ parameters.Replace(pos, host.Length(), uHost); >+ } >+ pos = parameters.Find(port.get()); >+ if (pos != kNotFound) { >+ parameters.Replace(pos, port.Length(), uPort); >+ } >+ >+ const char *params[2]; >+ params[0] = "-c"; >+ params[1] = parameters.get(); >+ >+ nsCOMPtr<nsILocalFile> application; >+ rv = NS_NewLocalFile(NS_LITERAL_CSTRING("/bin/sh"), PR_FALSE, getter_AddRefs(application)); >+ if (NS_FAILED(rv)) >+ return rv; >+ >+ nsCOMPtr<nsIProcess> process = do_CreateInstance(NS_PROCESS_CONTRACTID); >+ >+ if (NS_FAILED(rv = process->Init(application))) >+ return rv; >+ >+ PRUint32 pid; >+ if (NS_FAILED(rv = process->Run(PR_FALSE, params, 2, &pid))) >+ return rv; >+ >+ return NS_OK; > } > > nsresult nsOSHelperAppService::GetFileTokenForPath(const PRUnichar * platformAppPath, nsIFile ** aFile)

Instead of: + nsCOMPtr<nsIPref> thePrefsService(do_GetService(NS_PREF_CONTRACTID)); + if (thePrefsService) { + nsXPIDLCString prefString; + rv = thePrefsService->CopyCharPref(prefName.get(), getter_Copies(prefString)); Try: + nsCOMPtr<nsIPrefService> thePrefsService(do_GetService(NS_PREFSERVICE_CONTRACTID)); + if (thePrefsService) { + nsCOMPtr<nsIPrefBranch> prefBranch; + thePrefService->GetBranch(nsnull, getter_AddRefs(prefBranch)); + if (prefBranch) { + nsXPIDLCString prefString; + rv = prefBranch->CopyCharPref(prefName.get(), getter_Copies(prefString)); (plus a closing brace of course) Ditto for the next; try this: + nsCOMPtr<nsIPrefService> thePrefsService(do_GetService(NS_PREFSERVICE_CONTRACTID)); + if (!thePrefsService) { + return NS_ERROR_FAILURE; + } + nsCOMPtr<nsIPrefBranch> prefBranch; + thePrefService->GetBranch(nsnull, getter_AddRefs(prefBranch)); + if (!prefBranch) { + return NS_ERROR_FAILURE; + } ... + rv = prefBranch->CopyCharPref(prefName.get(), getter_Copies(prefString)); ... + rv = prefBranch->GetBranch(prefName.get(), getter_AddRefs(prefBranch)); The LoadURL code needs to be wrapped in #if XP_UNIX or some such (if it isn't already). The hard-coded "/bin/sh" should be ok for any unix. Is there any quoting/escaping necessary for the parameters passed to "/bin/sh -c"?

Sorry, I meant + rv = thePrefsService->GetBranch(prefName.get(), getter_AddRefs(prefBranch)); for that last one.

OK, using prefbranch now. Fixed the other subtleties

+ nsCAutoString prefName = NS_LITERAL_CSTRING("applications.") + nsDependentCString(aProtocolScheme); As long as you're taking the trouble to use nsIPrefBranch, why not get the "applications." branch and let the prefs enging handle the string concatenation for you? > Is there any quoting/escaping necessary for the parameters passed to > "/bin/sh -c"? Yes. Anything we get out of the URL should get quoted in single quotes (and any single quotes inside escaped). Good catch.... (consider usernames like ";rm%20-rf%20~;" (which is even valid as a username, I must add). What's with the removal of the XXX comment? r/sr=bzbarsky with those three issues addressed.

Comment on attachment 85236 [details] Another fix sr=darin (looks okay to me)

I pointed the other security-group members at this to make sure of any security issues. We might want/need to default some of the entries in all.js to commented out for security reasons, or require some form of warning requester.

rjesup: good point. i was probably too quick to sr= the pref changes.

I'm not at all comfortable with the idea that any Web page can cause me to launch any number of SSH connections to an arbitrary host, or cause my phone to dial an arbitrary phone number. I think we should check in a solution with an empty application list, and then we can go to individual bugs to discuss whether each given app should be on the default list. I'd also like some more protection against attacks inside mkaply's code. We should probably filter out special characters from the string that might confuse us, or the shell, or the helper program. It would be great if we could restrict the text obtained from the URL to just alphanumerics and spaces. For example, I haven't tried this, but what prevents someone from trying "telnet://ocallahan.org&&reboot"? We should also limit the maximum command length to something small like 256 characters, to prevent buffer overruns in weak helper applications. It might be helpful if we had a per-protocol pref that allows the protocol to require strict same-origin checking (based on host only). That would help stop automatic DOS or other attacks carried out via auto-loading telnet, ssh, etc URLs.

OK, someone else is going to need to own this now then. I'm not a Linux guru at all. My goal was to get basic helpers working, and I have done this. The rest is up to someone else.

I'm willing to take this over

What about checking this in then, and disabling/commenting out in prefs by default, putting some kind of warning comment ("Security risk") in prefs and filing a new bug "application helper is insecure" ? This is a much-desired feature and many people would surely take the potential risk, just to have it and the rest won't be bothered.

That's more-or-less what I'm thinking about as a first step, combined with perhaps some checks on the parameter strings for safety.

That could be a first step... rjesup, going forward you may be interested in the patches in bug 125505 -- the idea there is to have a small launcher app that will allow not calling the shell if not needed by using execve... this launcher will also also lose file descriptors and such that helpers don't need to see. If you think that's a decent approach here, I could try to give tenthumbs' patch a thorough review....

bug 125505 looks very interesting, especially combined with this bug (which covers more the "what do we pass to the external program as args" issue).

Invoking /bin/sh -c "USERSUPLIEDSTUFF" is really dangerous. My tests from javascript (xpcom) and plain C (not mozilla) show that invoking /bin/sh -c "/usr/bin/yes|more" invokes both "yes" and "more" which is not the expected in this bug behaviour IMHO. The string in quotes was passed as one pointer in argv.

First off, the current patch to nsOSHelperAppService.cpp (106682), doesn't compile for me. I get: ./unix/nsOSHelperAppService.cpp: In method `nsresult nsOSHelperAppService::ExternalProtocolHandlerExists(const char *, PRBool *)': ./unix/nsOSHelperAppService.cpp:1235: conversion from `const nsDependentCConcatenation' to non-scalar type `nsCAutoString' requested The problem line seems to be: nsCAutoString prefName = NS_LITERAL_CSTRING("applications.") + nsDependentCString(aProtocolScheme); Second, I'm attaching a new version of the unix.js changes that turn on external helper programs for various URI types. I found a couple of mistakes in the previous one, and I made some other changes that I will outline below. After looking at bug 39714, it appears that bug that ssh:// isn't an offical standard, so even though it makes more sense to use than telnet://, I have commented ssh:// out. Also, I have commented out some of the more obscure types that we don't have actual feature requests for. Personally, I think whatever we decide about specific protocols, we should check in a full list with the URIs that we don't want to support at this time commented out by default. At the very least we should check in with telnet:// turned on and a commented out version of mailto:, since these are highly requested features. As for the other URI schemes, I really think that it would be useful if they were discussed at this point, since they're unlikely to get any attention individually. I think some of the security concerns in this bug have been overstated. For example, if "telnet://ocallahan.org&&reboot", reboots your box, your box has problems that have nothing to do with mozilla ;). Of course we should be careful about what is turned on by default in mozilla, but turning on telnet, at the very least, seems reasonable.

> conversion from `const > nsDependentCConcatenation' to non-scalar type `nsCAutoString' requested What compiler is this, and can we stop using it? :) nsDependentCConcatenaton is an nsACString and nsCAutoString has both a constructor and an operator= for const nsACString.... > I think some of the security concerns in this bug have been overstated. telnet://rm%20-rf%20~@ocallahan.org Try seeing what happens to that with the existing patch....

On some Linux distributions (Lindows) you always run as root. It's kinda dumb but it happens and it's no excuse for us to betray the user. Also, on some setups you can run 'reboot' if you're a normal user logged in at the console. Anyway, that's not the point at all. The point is that the current code gives lots of ways for bad guys to run arbitrary shell commands and that is NOT good.

tenthumb's patch in bug 125505 handles George's worry about sh -c "stuff" invoking unexpected things, since there's no shell. (Though that means the helper app has to parse & handle the path variable.) Jim's updated patch and comments about the different URI types is quite helpful, and basically it's the path I'm planning to take to getting this in without opening security holes; I'll leave other URI handlers to be decided on in a case-by-case basis, or by the user who decides to allow these helpers. I plan to hold this until I find a good way to deal with avoiding any attacks by passing wacky URI's to telnet/etc. If someone wants (hint hint) a prefs UI could be created for these sorts of helpers, especially the oft-requested ones like mailto:.

telnet://rm%20-rf%20~@ocallahan.org will generate /bin/sh -c "xterm -e telnet rm -rf ~@ocallahan.org" which won't do anything interesting, and won't. Getting ; in there would be more dangerous (in particular the %url% versions). A more dangerous version might be something like telnet://somwhere%3b%20rm%20-rf%20%2ftmp%2fdoesnt_exist% which would yield /bin/sh -c "xterm -e telnet somewhere; rm -rf /tmp/doesnt_exist" (I'm mildly paranoid and don't want to leave a "kill-your-system" link in bugzilla if someone is trying out this patch...) Single-quoting each argument would solve many of the problems, since that removes the significance of all characters except single-quote. You couldn't have a single-quote in one of the arguments, and we'd have to check for that. We also have to worry about how the application will process the arguments we feed it; i.e. in many cases we're allowing semi-arbitrary arguments to be passed to an application (telnet, mail, emacsclient, etc), and that application might itself be a wrapper-script. Actually there are some other problems too. To specify a user in telnet requires "-l user", which this patch doesn't do. We need to add pref("applications.telnet.username", "-l %username%"); to the js file.

single-quoting will be tricky to do with %url% variations, BTW

Using this (note the single-quotes) would generally work better (for telnet:) +pref("network.protocol-handler.external.telnet", true); +pref("applications.telnet", "xterm -e telnet"); +pref("applications.telnet.port", "'%port%'"); +pref("applications.telnet.host", "'%host%'"); +pref("applications.telnet.username", "-l '%username%'"); Again, this solution will NOT work for %url% applications (like dict:) unless they are willing to take the whole URI as a single parameter (which it may be if it's a wrapper). I think this bug needs to be dependant on the helper app (bug 125505). Also note that things like mailto: will require mailer-specific-parsing to deal with the mailto: URI (see http://ftp.ics.uci.edu/pub/ietf/uri/rfc2368.txt) before the actual mailer is invoked. For example, mailto:foo@bar?bcc=xyz@qwe would need to be changed (for mutt) into "mutt -b xyz@qwe foo@bar". So you can't just use +pref("applications.mailto", "xterm -e mutt"); +pref("applications.mailto.parameters", "%url%"); You'd have to use something like +pref("applications.mailto", "xterm -e mutt_mailto"); +pref("applications.mailto.parameters", "'%url%'"); Oh, this is _so_ much fun....

I'm replying to several people here. > What compiler is this, and can we stop using it? :) nsDependentCConcatenaton > is an nsACString and nsCAutoString has both a constructor and an operator= for > const nsACString.... This is with gcc 2.95.4. This is a new box and I didn't realize that I had such an old gcc. I'll upgrade, but I think gcc 2.95.2+ is still is still supported, right? >telnet://somwhere%3b%20rm%20-rf%20%2ftmp%2fdoesnt_exist% The only way telnet links are dangerous is if machines are left open with accounts (or ports) that allow access with trivial passwords. In that case it is not a bug in mozilla, but a security hole in the system in question. Regardless, I guess we can just disallow "%" in the host name. As far as usernames, I propose Mozilla (like Netscape 4.7x) doesn't support them. I've never come across a telnet link in the wild that includes username.

Jim, the point of these funny URLs is that they lead to the execution of shell commands on the LOCAL machine. E.g., "telnet somewhere; rm -rf /tmp/doesnt_exist" telnets to 'somewhere' AND THEN removes a file on the local machine.

Yeah, I just forgot the ';' in my example. Jim, your arguments all fall down if I can sneak anything that terminates the friggin' telnet command (;, &&, ||, etc) into the command string; everything after that is executed on the _local_ host. In any case, the real usefulness of all this stuff is not "in the wild" but "on the intranet", where the system probably knows who you are because you authenticated and hence can easily contain links with your username in them (I know _I_'d be using this feature if it existed...). The point is, it's a security hole waiting to happen, and we don't want that. I think we all agree on that part (whether we accomplish this by using a helper launcher, eliminating certain chars, not supporting usernames, or whatever). Yes, gcc 2.95.x is supported. Does it build if you replace the '=' with the explicit constructor version? jesup, do you think there is any way we can make nsIProcess not suck? We could just use that, except that it requires an nsIFile; perhaps a custom (tiny) nsIFile impl is in order which would basically work for nothing useful other than being launchable (and hence would not need to know the full pathname but only the leaf)? This is by way of alternatives to tenthumbs' proposed helper launcher, though I'd be perfectly fine with using that too if we have no other alternatives.

Yep, Robert you're right. I didn't think it through. We should definitely be using single quotes.

having username for telnet makes no difference to the safety here; my example nuked things with just a hostname. As for nsIProcess: maybe; I haven't looked. Since the helper app solution will work, I'm open to using either it or modifications to nsIProcess, or even single quotes. Adding single quotes will work with /bin/sh -c (for telnet, and for others if there's an external program to process the args) so long as we give strip single quotes with arguments. Can anyone think of a significant security flaw with single quotes (plus stripping single quotes from parameters)? Any way in which the "xterm -e program args" trick bites us here?

Explicit constructor fixes compilation error.

I don't fully understand the workings of Mozilla in this matter, but from the point of view of generating shell commands to pass to /bin/sh -c, the usual thing to do is to to escape (with backslashes) all single quotes and backslashes, rather than just stripping single quotes. This will preserve the strings while still protecting the shell from any special characters. HTH.

This may be useful for mailto stuff with gnus: http://my.gnus.org/Lisp/1023105502 and http://www.gnus.org/list-archives/ding/200009/msg00070.html

> I think some of the security concerns in this bug have been overstated. For > example, if "telnet://ocallahan.org&&reboot", reboots your box, your box has > problems that have nothing to do with mozilla ;). root vs. normal user: For almost all users, the browser runs under the Unix-account with which they work normally, and that same user will have access to all their private files, which is the most (only?) valueable part of the computer. Which means that the difference between root and the current user is not all that relevant. shell+single quotes: Is /bin/sh garanteed to be a certain shell implementation or to follow the assumed rules wrt ' and \? helper app: Having no shell might not be enough protection. See comment 106 about the case where an app might call external apps itself, specified via an additional argument. defaults: I'd really suggest to keep the default list minimal, meaning 0-2, carefully checked schemes only. (This includes checking the invoked app.) I'd be most comfortable with 0 and a big warning in the prefs file.

> Is /bin/sh garanteed Yes. Or rather, any Unix system on which it does not work that way would pretty much fail to work out in the real world.

Using /bin/sh -c "'userstuff'" is a security hazard, please don't use it. In the past there were numerous ways exploiting /bin/sh -c in applications, some recent examples are gaim and konqueror and IIRC in most cases the fix was a migration to execve(). /bin/sh is a programming language which is not sandboxed. I have seen enough troubles with pure javascript to mess with /bin/sh. Please use execve() or an wrapper to it directly instead of dealing with /bin/sh. Even with execve() you should filter bad characters to avoid passing additional parameters to telnet or ssh (windoze suffered from this). A good idea is allowing only [A-Za-z0-9]. Single quotes are not solution at all, try the following: $ ls /tmp/tralala ls: /tmp/tralala: No such file or directory $ /bin/sh -c "xterm -e telnet 'somewhere $(touch /tmp/tralala)'" $ ls /tmp/tralala /tmp/tralala Another attack is to close the single quote with another single quote. Suggest the default for this option is everything off and it is the user's responsibility to turn it on - I definitely will turn everything off :)

Actually George's exact attack won't work without the second level of sh evaluation that comes from typing it at the command line. Still, it's highly advisable to avoid /bin/sh -c wherever possible with user-supplied (or worse, network-supplied) args. Searching for security or exploit and /bin/sh -c shows a number of ways this can happen, and though the majority of them are solved by single-quotes not all of them are. This is why I've made this bug dependant on bug 125505 (helper execve program), and it makes sense to use that or provide this functionality within mozilla itself.

grep()ing the mozilla source for '/bin/sh' shows it is used only in xmlterm, so it has been avoided for now. Strongly suggest the option for starting external applications be turned off by default. Very few users need it IMHO and those who need it should turn it on. If there is a bug in telnet or ssh, the users may get exploited and the exploit vector is mozilla, so users may blame mozilla. btw, my name is georgI (I , *not* E)

Georgi, you may also want to grep for uses of system(); I know that there is at least one such in the Unix helper app code already (dealing with mailcap without it is hard, since the mailcap entries are supposed to be run in a shell and often fail if they are not). I agree that when we roll this feature in it should start off by default.

Georgi; sorry about that. After consideration I will probably agree that all of them (including telnet) should either be off, or perhaps invoked only after a warning including the command and arguments to be executed (and the only one I'd enable for that would be telnet I think). Note that telnet is probably far less vulnerable than say xpdf or other large external helper apps that many people invoke - find an exploit in xpdf kicked off by a specific pdf file, for example. I certainly won't commit anything turning them on without support and agreement of the other security experts. We can't restrict mailto: arguments as severely as you indicated; at the minimum things like @, &, ?, . and + and even % need to be passed through. The mailto: protocol includes things like a body parameter that may require fairly arbitrary (modulo URL encoding) data. Basically, we'll need to pass through any valid URI and make it safe. That means at minimum using execve in one manner or another. The mailers will also have to be safe, but that will be the responsibility of the people who make the wrappers for those. Opera (see link above) allows a user to specify things like "mailto:mutt [-b %b] $t" (etc, I didn't bother to make sure I have the details right) to avoid needing wrappers around mail programs. This is much more specific to mailto (more special-purpose code in mozilla would be needed), and unless there's something I'm missing might be a potential security issue in Opera depending on how they invoke the mailer. Since we won't be enabling external mailto: by default under any case, it'll be the wrapper-writer (and person who enables it) who'll be making the assertion it's safe. We should heavily comment the prototype (commented-out) prefs for mailto in the JS file to warn people, and if UI is created for this it should include security warnings.

I've followed the recent conversation as best I can, so if I'm off base, let me know. Should we have a URL parsing function that handles the stripping of dangerous characters? Is it possible to find a set that needs to be handled in generic situations? This is what I was thinking when I filed Bug 166374.

Yes, when identified dangerous characters should be stripped. Though I am for the approach for allowing only "good" characters if possible. Once again I think that /bin/sh should not be used. execve() is more secure and the only drawback is a little more code. If /bin/sh goes away, the list of dangerous characters (if any) will be much smaller. Some tests show that Communicator 4.x strips some characters from telnet:// and rlogin://.

<i>Yes, when identified dangerous characters should be stripped.</i> Stripping dangerous characters seems like it would just break many of the 'commands' with potentially ungraceful results. I wonder if this couldn't be handled a little more gracefully. By default, if dangerous characters are present then a dialog box should open warning the user that dangerous characters are present and that this link has been disabled. As a user preference you could then allow an option to have the 'command' displayed and the option to grant permission to run this command. This would allow users who are aware of the concequences the opportunity to permit these 'commands' while protecting average users from any chance of danger.

we would have more security if we allow external protcols only via a whitelist (as I suggested in another recent bug)

We should probably see what Classic did in the way of "securing" telnet access. http://lxr.mozilla.org/classic/source/cmd/xfe/commands.c#1637 from what I can tell, it allows only alphanumeric input plus -_. and maybe one or 2 others. Also, I'm not sure how external apps were invoked in 4.x, but I'd figure that they might have had some good ideas. Why not take advantage of the source we have and see what they did?

we need both, but the character masking has uses beyond telnet. Okay, this bug has gotten too long. randell is owner. Do we have a patch for linux that works, for better or for worse? If so, then I'd like to propose moving each specific security aspect to another bug for a specific discussion.

1) How is it done in "open with..." by the download dialog? 2) Have you considered piping (|) all suspicious content (like email body) to the program that could be some user-supplied wrapper etc? I personally don't know any mailer that would accept whole message body without size limitations from command line, as opposed to ones fed through the pipe. When concerning telnet:// allow only characters legal in hostnames (alnum, dot and dash I think) as the hostname parameter. Ignore username and password, the host may reply to telnet in so many ways that hunting the "password" prompt misses its purpose. BTW, you could try to resolve the remote address first, hostname like host loaclhost%3Brm%20-rf has no IP address...

> 1) How is it done in "open with..." by the download dialog? In a really broken way that does not pass arguments and has other issues too. See comment 62 second paragraph, bug 125505, and the various bugs on not passing arguments. Piping (and even the helper from bug 125505, really) is kinda complicated if we want to do this on Unix _and_ OS/2 (which we do).

Just a comment that there are two different issues here: 1) processing the URL and 2) communicating with the OS. The first straightforward but I don't think you can do the second in a way that will work for multiple platforms simultaneously, e.g. I don't see how you can do the right thing for Linux and OS/2 in one module.

We can wash hostnames and ports fairly easily for telnet. We could wash usernames to be "a-zA-Z0-9_@,." which should be safe. NOTE: (and this isn't handled correctly by this patch) - usernames can be separated by commas. To a certain extent, this is an inherently risky feature to use. On the other hand, it's also a very much wanted feature, especially mailto:. I'd be willing to dump everything else in order to keep external mailto: support. If this requires an external shim app/shell to avoid security issues by error-checking the parameters before invoking the real mailer, so be it. If we can do it (or do it partially) in mozilla, fine. From the mailto: RFC -- 4. Unsafe headers The user agent interpreting a mailto URL SHOULD choose not to create a message if any of the headers are considered dangerous; it may also choose to create a message with only a subset of the headers given in the URL. Only the Subject, Keywords, and Body headers are believed to be both safe and useful. The creator of a mailto URL cannot expect the resolver of a URL to understand more than the "subject" and "body" headers. Clients that resolve mailto URLs into mail messages should be able to correctly create RFC 822-compliant mail messages using the "subject" and "body" headers. This implies we need to support Subject, Body, and (implied) To at a minimum. The reality is that body data is usually not too critical, and is usually limited in length and scope, but it is most certainly used. (Cc it probably also useful, as is bcc.) IE supports subject, body, Cc, & Bcc (plus the implied To) -- http://msdn.microsoft.com/workshop/networking/predefined/mailto.asp Also see http://www.network23.com/hub/mailto/default.html and http://www.opera.com/support/search/supsearch.dml?index=472 The Opera syntax isn't bad, if you want to avoid using a wrapper program/script. It requires spawning with execve (as per bug 125505) for safety, and probably should still have some amount of washing on args.

Just a thought, maybe dumb... How is passing the message body to the mailer handled in Windows? I doubt a 500k long email could be passed in command line. What about saving it to a temp file and invoking the mailer with the filename as a parameter, eventually stream from a <file ? There would be a question about when the file could be deleted (if the mailer program is exitted? But what if Mozilla exits first?) and if all mailers can handle that. In Pine there's an option to send file as attachment and mostly everything could be achieved with initial keystroke list parameter. In Mutt, there's -H (draft file with headers) How does it look in other mailers?

As one of those links implies, IE (and/or NS4) had a limit of 256 characters for the mailto URL, which eliminates very long bodies from something to worry about. Bodies are very much used for things like mailing-list subscribe/unsubscribe commands, etc.

Waiting for telnet:// to be fixed for a long time. <snip> bug Opened: 2000-03-24 19:05 ------- Additional Comment #3 From brade@netscape.com 2000-10-18 08:28 ------- ugh! I can't believe we haven't fixed this yet! </snip> As of Mon Nov 25 16:18:17 PST 2002 still not fixed :-) Any chance we can decide on how to fix it soon? How did mailto: handling get mixed up in this ?

Probably a soon fix is using execve() instead of system. The path to xterm should be found and then building argv[] which is not very difficult.

*** Bug 183658 has been marked as a duplicate of this bug. ***

Konqueror messed up in the telnet invokation as well :-(. <http://www.debian.org/security/2002/dsa-204>

Another month as gone by. :-( telnet:// not fuctioning in daily builds. Protozilla workaround not functional since 1.1 Please give linux users that understand the security risks a way to enable telnet:// Please warn the user and recommend best practices but provide some way to enable this functionality in linux.

barnaby: here is an easy way: right click -> copy link location On KDE start menu select "run command" then middle click to paste then ENTER Or just fire telnet and paste the address.

I understand and have used dozens of workarounds. I don't use KDE. It's a bug, fix it or close it. :-)

My 2 cents: - The protocols should be user-defineable - Mozilla should ship with a few known-good ones (telnet, mailto) - They can be made safe by calling a Mozilla-provided wrapper script, generated per platform. - extra bonus points for protocols that do url rewriting, like in KDE: "gg:foo" is a shortcut for opening a Google search for the word "foo". - Protozilla can do all those things. So I propose that more effort is put in getting protozilla in the trunk...

Sorry for that bad formatting! My 2 cents: - The protocols should be user-defineable - Mozilla should ship with a few known-good ones (telnet, mailto) - They can be made safe by calling a Mozilla-provided wrapper script, generated per platform. - extra bonus points for protocols that do url rewriting, like in KDE: "gg:foo" is a shortcut for opening a Google search for the word "foo". - Protozilla can do all those things. So I propose that more effort is put in getting protozilla in the trunk...

*** Bug 188746 has been marked as a duplicate of this bug. ***

A gentle reminder that another month has gone by :-) Has a solution been decided upon? Is the code written? When will a daily build with the fix be available? Thanks again.

I've changed the summary to reflect the more general changes that are being made in this bug, and created bug 194325, "telnet for Linux".

*** Bug 121779 has been marked as a duplicate of this bug. ***

*** Bug 200637 has been marked as a duplicate of this bug. ***

*** Bug 161125 has been marked as a duplicate of this bug. ***

Is there an extension that enables telnet:// for linux on firebird? Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4b) Gecko/20030516 Mozilla Firebird/0.6

Is the only reason this bug is open for *three and a half years* because it only affects *nix users? Bugs do not die of old age, but Protozilla apparently did.

*** Bug 217850 has been marked as a duplicate of this bug. ***

ph-bz: I think everyone wants this to work, (telnet not workin on unix systems is some what ironic...), but there is no linux only hack, and having a generic protocol->external handler has security concerns nobody has worked out. If you look at protocol handler bugs for Windows, you find some really unpleasant problems because after creating this mechanism, Microsoft ships some defaul protocol handlers that do really bad things. btw, the real reason I'm here is that I noticed that three recent dupes are about rtsp:, so I'm going to make one dependent to this bug so it stays open and shows up in future searches.

Actually, from where I sit, I'm tracking this bug not because I want telnet to work, but because I want to be able to click on an email address in a web page and have it open evolution's mail composer window, instead of having to copy and paste (thank god that Linux has middle click paste or this would really be annoying). With Firebird coming with no email client this ability is going to be sorely missed. Or am I missing something?

The only thing missing here is some code that's actually acceptable to people. Speaking of which, I have somewhat of a suggestion... So far a lot of the discussion has centered on how best to launch, eg, the telnet client in light of the various things that may or may not be in the telnet:// url, how best to avoid security issues, etc. It seems to me that we should consider an approach similar to the one that mac/windows take (and what comment 183 suggests): 1) The helper app service will only be resposible for finding and launching the app that handles the given protocol scheme. It will determine this based on preferences, gconf, KDE settings, voodoo dolls, whatever. As a first step, it can use preferences. The URL in question will be passed off to the app. 2) All responsibility for parsing the URL, extracting useful info, making TCP connections, handling security issues, whatever, will be with the app. 3) If really desired, we can ship some default apps with Mozilla (eg a perl script that can handle telnet:// urls by launching the telnet client). Being written in a sane language (like sh or perl) these apps will be able to usefully query the environment, use PATH, easily extract things from the URL, shell things out to the system, etc. Since the app handling a protocol will be invoked asynchronously by Mozilla, we don't care whether it blocks, asks the user further questions, whatever. Note that since mail clients tend to already understand mailto: urls, this will in fact solve the mailto: issue for people. As already mentioned, that's far more pressing now that we are in the process of transferring to the standalone apps. On a separate note, something that has not been mentioned is that Thunderbird needs to be able to launch the defalt http: handler; the proposal also makes that easy (set said handler to "mozilla"). Finally, the external protocol handlers we ship with Mozilla, if any, will be more amenable to modification than helper app service code is (since they will be pretty localized pieces of code in widely known languages and hacking on them will not require recompiling mozilla). So we could ship somewhat skeleton implementations of those, possibly off by default, and if people need more functionality they will have an easy time hacking it in. Thoughts? mkaply? rjesup? biesi? If people are OK with this, I will implement the above scheme when I get back (unless someone beats me to it; this should not be very hard to code up).

Just to clarify, I want to get a general system in place for launching external protocol handlers on Unix. Once we have that, we can work on writing a telnet:// handler that has no security holes and all... but at least the mailto: and http:// handlers will be able to pretty much work immediately and without security holes, since apps handling those protocols typically just take the URL as a whole. A possible complication I am willing to accept is that we can have a pref for whether the external app wants to take the full url string or whether we should parse the URL as an nsStandardURL and pass it the various parts of the URL, unescaped, in argv. But this should be a secondary concern, imo, and only done if we foresee having handlers other than the telnet:// handler needing such information.

bz: That sounds like a cool idea. It might have security implications, though - e.g. you can't just do system("the_app url") - what if the url contains semicolons. nsIProcess might do the right thing, I haven't checked.

nsIProcess on Unix handles things like that correctly.

This implements bz's suggestion from comment 195, basically... though it's not ideal - if we do want to usefully ship with default handlers for telnet: and stuff like that, mozilla should look in $MOZILLA_FIVE_HOME for the executables too, instead of just trying to do NS_NewLocalFile with the path.

Comment on attachment 130957 [details] [diff] [review] patch as described in comment 195 Seems reasonable to me... We could use GetFileTokenForPath here (and modify it to look in MOZILLA_FIVE_HOME, maybe) if desired. We don't really want to read a complex nsIFile, since we want people to be able to set these prefs... Perhaps ExternalProtocolHandlerExists should check not only that the pref is set but also that it can be converted to an nsIFile and that said nsIFile exists() and isExecutable() ?

>Seems reasonable to me... We could use GetFileTokenForPath here (and modify it >to look in MOZILLA_FIVE_HOME, maybe) if desired. I don't think we want to look in MOZILLA_FIVE_HOME first for entries in mimeTypes.rdf, which would be the effect of that change... But I can use it for looking in $PATH, will attach a new patch for this... >Perhaps ExternalProtocolHandlerExists should check not only that the pref is >set but also that it can be converted to an nsIFile and that said nsIFile >exists() and isExecutable() ? ...and this.

taking.

Comment on attachment 130960 [details] [diff] [review] patch to address bz's comments >Index: unix/nsOSHelperAppService.cpp >+ if (!branch) // No protocol handlers set up -> can't load url >+ return NS_ERROR_UNEXPECTED; NS_ERROR_NOT_AVAILABLE is better, imo. >+ // First, try to treat |appPath| as absolute path, if it starts with '/' >+ NS_ConvertUTF8toUTF16 utf16AppPath(appPath); We may want to do the whole "convert from filesystem charset" thing here... in particular, one can create local files using "native" paths and append native paths.... but GetFileTokenForPath, unfortunately, needs a UTF-16 path (something we should fix, maybe). OK to leave it like this for now, I guess; all the Unix helper app stuff is pretty intl-broken anyway. >+ PRBool exists = PR_FALSE; >+ (*aApp)->Exists(&exists); >+ if (exists) >+ return NS_OK; Exists() can fail; you need to check the return value there too. >+ app->Exists(&exists); >+ app->IsExecutable(&isExecutable); >+ *aHandlerExists = (exists && isExecutable); Again, those can both fail; you need to check return values. sr=bzbarsky with the nits picked. I'd like to hear what rjesup has to say about this, though, so requesting his r=...

>We may want to do the whole "convert from filesystem charset" thing here... Well, can prefs be in a non-utf8 charset?

Oh, and for the "Exists can fail": I figured that if it fails, it leaves the out value untouched/sets it to false; and I meant to treat such failure as if the file wouldn't exist.

You can't depend on the out params being untouched in case of failure with the nsIFile API, really. And pref values, per the IDL, can only be ascii, so I guess what you're doing is ok.

>You can't depend on the out params being untouched in case of failure with the >nsIFile API, really. ok then... I'll add a check for the return value. do you want to see a new patch for that?

Yes, please.

After following a dozen links, I see that eventually on Unix this calls execv/execve(), and we do it in a non-blocking fashion. Those should be safe to pass arbitrary parameters to. However, if we're going to provide a default handler for this (such as for mail), that will have to be very careful with it's handling of the arguments (length, special characters, etc) and how it processes the arguments. In theory, passing a 1MB argument (say) to execv() and an external handler should be ok. You can't assume an email/telnet/etc program (or shell) will handle that properly however unless you've checked those very carefully. We also need to be sure there aren't pitfalls for other OS's in the spawn-process sequence. So long as we realize that the security issues are unchanged, just pushed out to the handler app(s) and anything they invoke, I'm ok with it. (and modulo BZ's nits).

> We also need to be sure there aren't pitfalls for other OS The only non-Unix OS using this code is VMS... Colin should know whether any of this will be a problem there, I hope. And yes, the point here is that a lot of the security issues need to be looked at per-protocol. Some (eg length checking) we could add to the core code if we see that all the handlers need them. The rest should be handled by the handlers themselves.

We'll probably want to set up different default prefs for OpenVMS, and have different shell scripts, but the basic plan should work.

Comment on attachment 131173 [details] [diff] [review] patch; check rv ok, |rv| checks added

Comment on attachment 131173 [details] [diff] [review] patch; check rv r=bzbarsky (or take this as an sr if you have another r= from someone).

ok, so what about bug 128668? that seems like the right way to enable external protocol handlers under linux/gnome. what do we do with this code when that code lands? do the gnome helper app associations take precidence over these pref settings? or is it the other way around? are these prefs only meant to help out advanced users of non-gnome linux systems?

> what do we do with this code when that code lands? The same thing we do with the mailcap/mime.types code and our mimeTypes.rdf code once that code lands -- leave the two in side-by-side. > do the gnome helper app associations take precidence over these pref settings? Mozilla's pref settings should always override the OS settings when the two come in conflict, imo. This is the way it works for helper apps, and it's the way it should work for protocols. And yes, these prefs will likely only be used by Linux users not using GNOME systems. Which is a lot of Linux users, as it happens. The prefs need not only help out advanced users -- distributors (eg distributions that use KDE by default or IS personnel at organizations that do not use GNOME) could set these prefs for commonly used protocols like mailto.

>ok, so what about bug 128668? that seems like the right way to enable external >protocol handlers under linux/gnome. Yeah... if Gnome is present. As bz pointed out, not all linux users have Gnome. > what do we do with this code when that > code lands? do the gnome helper app associations take precidence over these > pref settings? imo, preferences (if set) should take precedence over the gnome handlers, so that users can override those settings. >are these prefs only meant to >help out advanced users of non-gnome linux systems? see comment 195 point 3, we may want to ship some default protocol handlers, e.g. a telnet handler that does "xterm -e telnet host port", after parsing host and port out of the url If we don't do that, it still helps multi-user systems where the administrator can define protocol handlers, and it also helps advanced users (until bug 128668 lands, even on non-gnome systems. and the patch there must be updated for bitrot...)

Comment on attachment 131173 [details] [diff] [review] patch; check rv >Index: unix/nsOSHelperAppService.cpp >+/** Looks up the handler for a specific scheme from prefs and returns the >+ * file representing it in aApp. Note: This function doesn't guarantee the >+ * existance of *aApp. */ nit: this comment isn't consistent with any of the other comment styles. >+nsresult >+nsOSHelperAppService::GetHandlerAppFromPrefs(const char* aScheme, /*out*/ nsIFile** aApp) >+{ >+ nsresult rv; >+ nsCOMPtr<nsIPrefService> srv(do_GetService("@mozilla.org/preferences-service;1", &rv)); nit: use NS_PREFSERVICE_CONTRACTID :) >+ nsCAutoString spec; >+ rv = aURI->GetSpec(spec); hmm.. maybe use GetAsciiSpec here instead. that way any non-ASCII characters will be URL-escaped. btw, i agree that these two methods of handling unknown protocols can and should exist side-by-side and that prefs should take precidence over system settings. sr=darin

Comment on attachment 131173 [details] [diff] [review] patch; check rv >Index: unix/nsOSHelperAppService.cpp >+/** Looks up the handler for a specific scheme from prefs and returns the >+ * file representing it in aApp. Note: This function doesn't guarantee the >+ * existance of *aApp. */ nit: this comment isn't consistent with any of the other comment styles. >+nsresult >+nsOSHelperAppService::GetHandlerAppFromPrefs(const char* aScheme, /*out*/ nsIFile** aApp) >+{ >+ nsresult rv; >+ nsCOMPtr<nsIPrefService> srv(do_GetService("@mozilla.org/preferences-service;1", &rv)); nit: use NS_PREFSERVICE_CONTRACTID :) >+ nsCAutoString spec; >+ rv = aURI->GetSpec(spec); hmm.. maybe use GetAsciiSpec here instead. that way any non-ASCII characters will be URL-escaped. btw, i agree that these two methods of handling unknown protocols can and should exist side-by-side and that prefs should take precidence over system settings. sr=darin