Closed Bug 33282 Opened 24 years ago Closed 21 years ago

enable external scheme handlers (like aim: and telnet:) in Linux

Categories

(Core :: Networking, defect, P3)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.6alpha

People

(Reporter: mcafee, Assigned: Biesinger)

References

(Depends on 1 open bug, )

Details

(Keywords: platform-parity, relnote, testcase)

Attachments

(3 files, 18 obsolete files)

3.23 KB, patch
Details | Diff | Splinter Review
6.99 KB, patch
Details | Diff | Splinter Review
6.06 KB, patch
bzbarsky
: review+
darin.moz
: superreview+
Details | Diff | Splinter Review
telnet: isn't working, I get an unrecognized protocol handler error.
and?...
Assignee: gagan → nobody
Keywords: helpwanted
Target Milestone: --- → M20
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.
Assignee: nobody → don
Component: Networking → XP Apps
Keywords: 4xp, dogfood, relnoteRTM, rtm
QA Contact: tever → sairuh
Target Milestone: M20 → ---
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 ...)
Assignee: don → mscott
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).
Whiteboard: [dogfood-]
triaging to necko. 
Assignee: mscott → gagan
Component: XP Apps → Networking
QA Contact: sairuh → tever
Target Milestone: --- → Future
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.
Whiteboard: [dogfood-] → [dogfood-] relnote-user
rtm-, this is a feature that wasn't implemented in time.
Whiteboard: [dogfood-] relnote-user → [dogfood-][rtm-] relnote-user
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".
Keywords: mozilla1.0
Summary: telnet: is an unrecognized protocol handler → implement telnet:// support/handling.
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.
Target Milestone: Future → ---
*** 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
Depends on: protozilla
putting on nsbeta1 radar, unix people will expect this to work
Keywords: nsbeta1
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.
Target Milestone: --- → Future
Keywords: nsdogfood-
Keywords: nsdogfood
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.
QA Contact: tever → benc
+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?
Keywords: verifyme
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.
Keywords: 4xp, nsdogfood-nsdogfood
OS: All → Linux
Hardware: All → PC
Summary: implement telnet:// support/handling. → URL: telnet:// unregistered in Linux
Whiteboard: [dogfood-][rtm-] relnote-user
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.
Keywords: relnoteRTMrelnote
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.
Summary: URL: telnet:// unregistered in Linux → URL: telnet:// unregistered in Linux, MacOS X
Blocks: 104166
UPDATE:
telnet:// URLs work in MacOS X on Mozilla 0.9.5 running in classic mode.
(now I'm confused why.)
Keywords: verifyme
Summary: URL: telnet:// unregistered in Linux, MacOS X → URL: telnet:// unregistered in Linux
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.
Keywords: 4xp
Attached patch Attempt at fix (obsolete) — — Splinter Review
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.
Blocks: 82776
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 ...)
Attached patch New patch (obsolete) — — Splinter Review
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.
Attachment #80642 - Attachment is obsolete: true
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.
Attached patch New patch (obsolete) — — Splinter Review
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
Attachment #81165 - Attachment is obsolete: true
Attached patch Partially fixed patch (obsolete) — — Splinter Review
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
Attachment #81523 - Attachment is obsolete: true
Attached patch Patch that builds and runs (obsolete) — — Splinter Review
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.
Attachment #81537 - Attachment is obsolete: true
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.
Attachment #81541 - Attachment is obsolete: true
> 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)
Attached patch Playing the patch game (obsolete) — — Splinter Review
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.
Attachment #81686 - Flags: needs-work+
> +  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.
Attached patch Hopefully an attempt at final (obsolete) — — Splinter Review
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.
Attachment #81686 - Attachment is obsolete: true
> +  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()
Attached patch Last two things bz mentioned (obsolete) — — Splinter Review
Changed order to username/password/host/port

Added .get
Attachment #81863 - Attachment is obsolete: true
Comment on attachment 82098 [details] [diff] [review]
Last two things bz mentioned

r=bzbarsky
Attachment #82098 - Flags: review+
Attached patch Turn telnet and friends on in prefs (obsolete) — — Splinter Review
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.
Attachment #81679 - Attachment is obsolete: true
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...
Blocks: 142557
Blocks: 142559
Blocks: 142790
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?
Attachment #82207 - Attachment is obsolete: true
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;
Attachment #82098 - Flags: needs-work+
Attached patch Addressing darin's comments (obsolete) — — Splinter Review
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
Assignee: gagan → mkaply
Target Milestone: Future → ---
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.
Attachment #82651 - Attachment is obsolete: true
Attached file Another fix (obsolete) —
Oops, found another mistake. Hopefully this one is OK.
Attachment #85235 - Attachment is obsolete: true
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).
Keywords: mozilla1.0mozilla1.1
Does this patch cover only Linux? Do we need a separate Mac OS X version of this
bug?
Keywords: pp
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....
Keywords: mozilla1.2
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.
OS: Linux → All
Hardware: PC → All
Summary: URL: telnet:// unregistered in Linux → URL: telnet:// unregistered in Linux and Mac OS X
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).
OS: All → Linux
Hardware: All → PC
Summary: URL: telnet:// unregistered in Linux and Mac OS X → URL: telnet:// unregistered in Linux
I'll move Mac OS X. This has been a testcase for some time, so it gets checked
on all plats.
Keywords: testcase
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
Keywords: mozilla1.1
*** 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
Attachment #85236 - Flags: superreview?(darin)
Attachment #85236 - Flags: review?(bzbarsky)
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.
Attached patch Patch addressing dmose and jesup comments (obsolete) — — Splinter Review
OK, using prefbranch now. Fixed the other subtleties
Attachment #83416 - Attachment is obsolete: true
Attachment #106677 - Attachment is obsolete: true
+  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)
Attachment #85236 - Flags: superreview?(darin) → superreview+
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
Assignee: mkaply → rjesup
Status: NEW → ASSIGNED
Keywords: helpwanted, review
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.
Target Milestone: --- → mozilla1.3alpha
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.
Attachment #85236 - Attachment is obsolete: true
> 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....
Depends on: 125505
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?

Attached patch Fixed compilation error — — Splinter Review
Explicit constructor fixes compilation error.
Attachment #82098 - Attachment is obsolete: true
Attachment #106682 - Attachment is obsolete: true
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.
> 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. ***
Attachment #85236 - Flags: review?(bzbarsky)
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.
Flags: blocking1.3b?
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...

Flags: blocking1.3b? → blocking1.3b-
*** 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.
Blocks: 194325
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".
No longer blocks: 194325
Summary: URL: telnet:// unregistered in Linux → enable external scheme handlers (like aim: and telnet:) in Linux
Blocks: 56478
Blocks: 194325
*** Bug 121779 has been marked as a duplicate of this bug. ***
Blocks: 200509
*** Bug 200637 has been marked as a duplicate of this bug. ***
Blocks: 11459
*** 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.
Blocks: 202715
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.
Attached patch patch as described in comment 195 (obsolete) — — Splinter Review
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.
Assignee: rjesup → cbiesinger
Status: ASSIGNED → NEW
Attached patch patch to address bz's comments (obsolete) — — Splinter Review
Attachment #130957 - Attachment is obsolete: true
Attachment #130960 - Flags: review?(bz-vacation)
Blocks: protozilla
Status: NEW → ASSIGNED
No longer depends on: protozilla, 125505
Target Milestone: mozilla1.3alpha → mozilla1.6alpha
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=...
Attachment #130960 - Flags: superreview+
Attachment #130960 - Flags: review?(rjesup)
Attachment #130960 - Flags: review?(bz-vacation)
>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).
Attachment #130960 - Flags: review?(rjesup) → review+
> 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.
Attachment #130960 - Attachment is obsolete: true
Comment on attachment 131173 [details] [diff] [review]
patch; check rv

ok, |rv| checks added
Attachment #131173 - Flags: review?(bz-vacation)
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).
Attachment #131173 - Flags: review?(bz-vacation) → review+
Attachment #131173 - Flags: superreview?(darin)
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
Attachment #131173 - Flags: superreview?(darin) → superreview+
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
Thanks for the reviews. I'm going to mark this bug fixed and leave considering
shipping default protocol handlers to the bugs this one blocks.

I do wonder, though, why bug 194325 (for the telnet protocol) is WONTFIX...

Anyway, this is checked in
Checking in unix/nsOSHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/unix/nsOSHelperAppService.cpp,v  <-- 
nsOSHelperAppService.cpp
new revision: 1.38; previous revision: 1.37
done
Checking in unix/nsOSHelperAppService.h;
/cvsroot/mozilla/uriloader/exthandler/unix/nsOSHelperAppService.h,v  <-- 
nsOSHelperAppService.h
new revision: 1.10; previous revision: 1.9
done
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Oh, one more comment, for those on the CC list that didn't read the patch:
Setting a protocol handler works this way (c&p'ed from my checkin comment):

This allows setting external protocol handlers via a preference setting:
network.protocol-handler.app.<scheme>
This is supposed to be a string value.
Mozilla will first try to interpret this as an absolute path, then as a filename
relative to $MOZILLA_FIVE_HOME, then as a file in $PATH.
If an application is found in one of these places, it will be called and passed
the complete url as first (and only) argument.
Sigh.
This has been 'fixed' for years via Protozilla IPC.  Most (all?) of the
protozilla developers have given up on you guys ever getting round to Protozilla
(it's now more than 2 years!) and have just gone off and done their own thing
-not- using the Mozilla framework.

As this 'fix' doesn't seem to allow IPC, you can't even have the display handled
via the browser, just external applications. This could have been fixed in 2001,
but now it's frankly too late.
>Sigh.
>This has been 'fixed' for years via Protozilla IPC.  Most (all?) of the
>protozilla developers have given up on you guys ever getting round to Protozilla
>(it's now more than 2 years!) and have just gone off and done their own thing
>-not- using the Mozilla framework.

I am sorry about that, really.
Did protozilla ever get submitted to bugzilla.mozilla.org in patch form, and
were people asked to review it?

>As this 'fix' doesn't seem to allow IPC, you can't even have the display
>handled via the browser, just external applications.

That is correct.

If you want IPC, install protozilla.

> This could have been fixed in 2001,
> but now it's frankly too late.

not sure what your point is in this sentence?
> >This has been 'fixed' for years via Protozilla IPC.  Most (all?) of the
> I am sorry about that, really.
> Did protozilla ever get submitted to bugzilla.mozilla.org in patch form, and
> were people asked to review it?

Many times. Protozilla is bug #68702, and has had patches since __0.8.1__!

Example:
-------
2001-03-01, Gerv:  
Adding some keywords. Given that this bug blocks the implementation of 
Protozilla, and thereby about six 4xp and 3xp bugs relating to new URI schemes, 
upping severity.
neeti: as module owner of Networking, what's your plan for getting this 
reviewed?
-------
From Gagan  2001-03-11:
my apologies for not getting to this sooner. But I will review this soon.
--------
From Doug T  2001-10-05:
not enough time.  0.9.5 -> 0.9.6
--------
From Doug T 2001-10-08:
reassigning to brendan.  Waiting on his review.
--------

And on and on and on.

> > This could have been fixed in 2001, but now it's frankly too late.
> not sure what your point is in this sentence?

Anyone who was doing stuff with Protozilla has abandonned it and gone on to seek
other solutions which don't take now more than 2 1/2 years to resolve. 
Rob: 

we don't have a time machine, so I don't think anyone can go back and check that
into the 0.8.1 trunk.

Complaining about the past is not useful for doing what many of us want here,
which is to give us some code guts that would lead to fixing mailto: and telnet:
URLs.

If you think this is the wrong approach, file a new bug on why protozilla should
replace it. 

What I'm really interested in, is finding a way of verifying this bug. It sounds
like you could configure mozilla for a dummy protocol handler (test:) that is a
shell script that would verify the URL is passed correctly?
Keywords: nsdogfoodverifyme
benc: Indeed - I tested it with setting "gmessage" as the handler for the url,
which shows a message box with the passed parameter. I used tester as the
protocol and stuff like tester://foo as the url. (of course after setting the
preference as described in an earlier comment)
Hi,

I have this

user_pref("network.protocol-handler.app.telnet", "/sbin/xterm -e telnet %h %o");

and this

user_pref("network.protocol-handler.external.telnet", true);

what else do I need to get this to work on linux ?

google returns lots of info none of which is helping me.

Thanks,

Barnaby
command line arguments do not work.
so, anyone gotten this to work with a stock mozilla 1.6? supposedly its fixed,
but I still get "telnet is not a registered protocol" when opening up telnet://
links in mozilla 1.6 compiled from source on solaris 8.
yes, sorry. this bug has morphed. bug 194325 is now about providing telnet:
support in a stock mozilla distribution.
so from a stock mozilla-1.6 source code compile, to get telnet:// links working
(at least on solaris)
create a file moztelnet 

#!/bin/sh
IFS="/:"
set -- $*
'/path/to/your/xterm' -e telnet $2 $3


then in your prefs.js file in your profile, add the following:

user_pref("network.protocol-handler.app.telnet", "/path/to/moztelnet");

hope this helps people out.. Thank you Christian!
i couldn't make this work.
has someone examined the output of:
telnet://$(ls)
?
Re comment #232 : shouldn't this be $4 and $5 instead of $2 and $3, i.e. the
last line in the script should be:

/path/to/your/xterm -e telnet $4 $5

With this changes, it works for me. 

In order to handle security matters I am using a perl script that I have whipped
up quickly instead:

#!/usr/bin/perl -w
my $arg = shift;
my (undef,$uri) = split(/:\/\//,$arg) or exit;
my ($host,$port) = split(/:/,$uri) or exit;
exit if ($host =~ /[^a-zA-Z0-9\.-]/);
if ($port) {
  exit if ($port !~ /[0-9]+/);
  system("/usr/bin/X11/rxvt","-e","telnet",$host,$port);
} else {
  system("/usr/bin/X11/rxvt","-e","telnet",$host);
}

It checks that the hostname/ip does not contain anything that is not a letter,
digit, dot or hyphen and it checks that the port does not contain anything that
is not a digit.
Then it passes the arguments in a way that circumvents shell invocation.
Sorry, the perl script contains a call to rxvt not xterm, which might not be
installed. 

Here is a modified script that also handles usernames in the
telnet://user@host:port format. 
---snip---
#!/usr/bin/perl -w
our $pathtoxterm = "/usr/bin/X11/xterm";
my $arg = shift;
my $user = "";
my (undef,$uri) = split(/:\/\//,$arg) or exit;
my ($host,$port) = split(/:/,$uri) or exit;
exit if ($host =~ /[^a-zA-Z0-9\.-@]/);
if($host=~/@/) {
  ($user,$host) = split(/@/,$host);
}
if ($port) {
  exit if ($port !~ /[0-9]+/);
  if($user) {
    run("-l",$user,$host,$port);
  } else {
    run($host,$port);
  }
} else {
  if ($user) {
    run("-l",$user,$host);
  } else {
    run($host);
  }
}

sub run {
  exec($pathtoxterm,"-T","$arg","-e","telnet",@_);
}
---snap---

Adapt the first two lines to your configuration. 
Here is a new version that handles IPv6 literal addresses as well
as ssh.  Think of:

ssh://root@[::1]:22

#!/usr/bin/perl -w
use strict;

my $exec = "/usr/bin/xterm";
my $arg = shift;
defined $arg || die "undefined arg\n";
$arg =~
m#^(telnet|ssh)://(([A-Za-z0-9\.\-\_]+)(:([^@]+))?@)?(\[?([A-Za-z0-9\.:-]+?)\]?)(:([\d]+))?$#;

my $proto = $1;
my $user = $3;
my $password = $5;
my $host = $7;
my $port = $9;

# DEBUG:
# print "proto = $proto, user = $user, password = $password, host = $host, port
= $port\n";

defined $host || die "bad url\n";
my @args = ($exec, "-T", "$arg", "-e", $proto);
push (@args, "-l", $user) if ($user);
push (@args, $host);
push (@args, $port) if ($port);

exec @args;
Should we add UI for it.
(In reply to comment #236)

Ok, I hacked this up just a tiny bit more.  I needed it to allow a trailing
slash on the URL, and the extra open fds handed down by the browser were
causing my telnet client to gag.  I added tn3270: and rlogin: (yeah, I know...)
while I was at it, probably suboptimally, since I don't perl too often.

#! /usr/bin/perl

#
# from http://bugzilla.mozilla.org/show_bug.cgi?id=33282#c236 (bugs@mdarwin.ca)
# changes:
#
# 14Aug2004 rlhamil@mindwarp.smart.net
# * tolerate trailing slash on URLs
# * close unneeded file descriptors before starting xterm (caused problems)
# * add tn3270 and (admittedly evil) rlogin support (not quite official
#   schemes, but common enough)
#
# Note: I chose to use x3270 for tn3270 URLs, but some may wish to use tn3270
# in an xterm instead; that would somewhat simplify the code, although neither
# can handle user or password info.
#
# Note: the fingerprint parameter for ssh URLs (described in expired
# draft-ietf-secsh-scp-sftp-ssh-uri-01.txt) is NOT presently implemented.
#
#
# installation:
# by editing your $HOME/.mozilla/$LOGNAME/*/prefs.js or preferably via
# about:config, add the following preferences:
#     network.protocol-handler.app.telnet        /path/to/script
#     network.protocol-handler.external.telnet   true
# repeat above two as desired replacing telnet with ssh, tn3270, or rlogin
#


use strict;
use POSIX;

# change next two as needed; 1st tricky for me to allow preferred if available
my $exec = ( -x "/opt/sfw/bin/xterm" ) ? "/opt/sfw/bin/xterm" :
"/usr/openwin/bin/xterm";
my $x3270 = "/usr/local/bin/x3270";

my $arg = shift;
defined $arg || die "undefined arg\n";
# telnet://[<user>[:<password>]@]<host>[:<port>]/
$arg =~
m#^(telnet|ssh|rlogin|tn3270)://(([A-Za-z0-9\.\-\_]+)(:([^@]+))?@)?(\[?([A-Za-z0-9\.:-]+?)\]?)(:([\d]+))?/?$#;

my $proto = $1;
my $user = $3;
my $password = $5;
my $host = $7;
my $port = $9;

# DEBUG:
# print STDERR "proto = $proto, user = $user, password = $password, host =
$host, port = $port\n";
defined $host || die "bad url\n";
my @args;
if ( $proto eq "telnet" || $proto eq "ssh" || $proto eq "rlogin" ) {
    @args = ($exec, "-T", "$arg", "-e", $proto);
    push (@args, "-l", $user) if ($user);
    push (@args, $host);
    push (@args, $port) if ($port && $proto ne "rlogin");
}
elsif ( $proto eq "tn3270" ) {
# neither x3270 nor tn3270 programs can handle user or password, so just
# ignore them
    @args = ($x3270, $host);
    push (@args, $port) if ($port);
}

# DEBUG
# print STDERR "@args\n";
my $fd;
foreach $fd (3 .. POSIX::sysconf(&POSIX::_SC_OPEN_MAX) ) { POSIX::close($fd); }
exec @args;


Depends on: 748643
Blocks: 748643
No longer depends on: 748643
No longer blocks: 748643
Depends on: 748643
Depends on: 749492
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: