Created attachment 52314 [details]
patch to do this

The patch might cause problems as the code in appopen.cxx also is used for
hyperlinks etc. 

So hyperlinks to files that OOo can't load or programs now will fail.

So I hesitate to accept it.

I'm not sure that the situation wrt. anything opened through the existing
whitelisted protocols significantly changes ? Unless I'm missing something or
taking too local a look at the code.

In the current situation the whitelisted http|https and ftp urls get passed to
the first xSystemShellExecute->execute branch and on failure of that (e.g. a
failure of the eventually called gnome-open-url or cde-open-url) you just get an
error dialog of some kind.

In the new situation those urls still get passed to what was the second
xSystemShellExecute->execute that was previously only used for file://, where
the difference is that if that fails and there is a possible OOo filter for them
it continues on to get opened by OOo itself. So the current situation should
remain basically unchanged for the existing whitelisted url patterns, except
that OOo will now make an extra attempt to open them if FOO-open-url fails.

And in either case this is supposed to open happen for "external" formats. What
might break is the case of urls like smb://|webdav:// which previously would
only have been handled successfully by OOo itself now getting passed to
FOO-open-url first which might e.g. fail and not return a failure or do
something else unexpected.

"What might break is the case of urls ... now getting passed to
FOO-open-url first which might e.g. fail and not return a failure or do
something else unexpected."

Bah, and right now passing the help urls would cause just this to happen,
gnome-open passes them to firefox (for some reason) which just pops up a dialog
complaining, i.e. "something unexpected".

Caolan, I will have a closer look on the patch next week. Currently I'm too busy
with other things and the code freeze for beta has passed anyway. So we're not
in a hurry, I assume.

I've set the target to 3.0.

Oh no hurry at all

Created attachment 52751 [details]
slightly reworked patch

I wonder whether the more frequent use of "IsSecureURL" now might lead to
problems, by default obviously not.

The test matrix won't be small - do you agree with target 3.1 to get more time?

That's fine, I don't mind a delay as long as the principle at least is
considered desirable, even if the first cut at implementation may turn out to
throw up some unexpected nasties.

Yes, your idea is right in general. The "design" of that function was made
before we got the Gnome VFS extensions and should have been revised already. My
problem is just that I have to rethink the current design (fortunately we have a
spec for it ;-)) before I can review changes made to it.

The reason why we used white listing is that the list of "internal" protocols
that only OOo can handle is extendable. vnd.sun.star.help is one example, others
could be brought in by extensions. 

If the OS hands over unknown protocols to the web browser other protocols can
cause the same error message, as you discovered for the help protocol.

Would extending the white list to "smb:" and probably others be acceptable?
Surely the number of "external" protocols is extendable also, but IMHO it's
comprehensible that an application first tries to get its own stuff running well
before it looks for better OS integration.

If we handed over more protocols to the system we had to decide whether they
should go through a security check ("IsSecureURL") or not. Currently we have the
situation that in case of http(s) and ftp we assume that the browser will do
that and that OOo must do it for "file:". IMHO any other protocol perhaps should
be treated like the "file:" protocol, means: IsSecureURL() should be called. 

We shouldn't do this for http(s) and ftp as then they wouldn't become executed
then in case they are part of a document from the internet (what surely is
expected for "file:" or "smb:" URLs!), so we still need the white listing for
this difference.

added it to CWS mba31issues01

So now back to work. :-)

To make the changes smaller I would like to propose the changed patch. Please
comment.

Created attachment 58473 [details]
smaller patch from mba

Start Help and click on something. Given just this patch then for me help links
would be passed out to a system handler and not get handled by ourselves.

Created attachment 58500 [details]
Something like this would restore help links

Oh yes, but that can be added easily. The question is: does that solve your
problem? It would remove my concern wrt. the security checks.

Yeah, otherwise works fine on the type of urls I care about anyway

Thinking a bit further: "vnd.sun.star.help" isn't the only internal protocol we
have. And more over, the list of internal protocols can be extended. 
All of these protocols (once used in a hyperlink) will create the same error
message as we get for help links in my tentative patch.

So I start to think that an unqualified "else" part isn't the right solution.
I'm afraid that we must extend our "white list" for externally handable
protocols. Very unfortunate. The best thing would be if we could get a list of
protocols the system can handle. But I assume that this is not easy, right?

It seems that checking the ProtocolHandler service for registered protocols
instead of checking for "vnd.sun.star.help" only should do the trick. Assuming
that all registered protocols are "OOo internal" only is the best guess we can make.

Here's a patch that implements the usage of the protocol handler.
Caolan, would that be OK for you?

Created attachment 59531 [details]
patch using protocol handler configuration

That's fine by me, should give consistent behaviour. In my own case it should
mean that e.g. file:///tmp/demo.png and smb:///localhost/share/demo.png (seeing
as we use the gio and/or gnome-vfs ucb handler) would both end up getting opened
by OOo as they can be handled "internally"

The code doesn not check for UCPs, it just checks protocol handlers. So "file:"
should be handled externally as well as "smb:".

fixed in CWS mba31issues01

verified in CWS build

seen in OOO310_m2 and DEV300_m42