Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: add fireurl #5574

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from
Draft

feature: add fireurl #5574

wants to merge 13 commits into from

Conversation

glitsj16
Copy link
Collaborator

@glitsj16 glitsj16 commented Jan 6, 2023

[WIP] [POC] This PR is part of an attempt to integrate @rusty-snake's fireurl into Firejail and finally fix opening hyperlinks between sandboxes properly and elegantly.

IMO this offers a much needed (and much overdue) improvement to Firejail's (default) handling of inter-sandbox URL transport.
This PR takes care of cleaning up the 'level 1 hacks' from the affected profiles. Obviously there's more work to be done to bring in fireurl, but all in all surprisingly little.

Relates to:

@glitsj16 glitsj16 marked this pull request as draft January 6, 2023 01:44
Copy link
Collaborator

@kmk3 kmk3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR seems to contain changes from #5571, which makes it harder to review.

@glitsj16 commented on Jan 6:

This PR takes care of cleaning up the 'level 1 hacks' from the affected
profiles.

Instead of removing the existing methods for opening URLs and potentially
breaking user setups (especially this close to the release), I'd suggest only
whitelisting the socket and adding fireurl to private-bin for now.

Obviously there's more work to be done to bring in fireurl

With just the changes mentioned above, maybe fireurl can work as an external
tool for now for whoever wants to try it, but I have concerns with regards to
adding it as is to this repository (I'll expand on this later on #5364, #5566
or maybe on a new discussion).

@glitsj16
Copy link
Collaborator Author

glitsj16 commented Jan 6, 2023

Instead of removing the existing methods for opening URLs and potentially
breaking user setups (especially this close to the release), I'd suggest only
whitelisting the socket and adding fireurl to private-bin for now.

That's the thing though. User setups probably are already broken due to incomplete implementations of what was deemed necessary for allowing hyperlinks to be handled by a sandboxed Firefox. The whole point of migrating to a socket-driven approach like fireurl is to fix this and have a proper base from which to expand upon in the future. Like supporting more web browsers than Firefox, access control and path sanitization. Basically what is touched in #5364 for now. The draft status of the PR guarantees nothing will be changed as far as existing methods for URL handling is concerned.

@kmk3
Copy link
Collaborator

kmk3 commented Jan 7, 2023

@glitsj16 commented on Jan 6:

Instead of removing the existing methods for opening URLs and potentially
breaking user setups (especially this close to the release), I'd suggest
only whitelisting the socket and adding fireurl to private-bin for now.

That's the thing though. User setups probably are already broken due to
incomplete implementations of what was deemed necessary for allowing
hyperlinks to be handled by a sandboxed Firefox.

But was it already broken as of 0.9.70 or was it broken afterwards?

Also, by "broken" do you mean "not working at all" or half-working (such as by
the program opening firefox but it not having access to the desired profile)?

I do remember seeing multiple issues about this over time, but I assumed that
firefox would at least open the URL on the most common scenario.

The whole point of migrating to a socket-driven approach like fireurl is to
fix this and have a proper base from which to expand upon in the future. Like
supporting more web browsers than Firefox, access control and path
sanitization. Basically what is touched in #5364 for now.

To be clear, I'd personally also prefer a socket-based approach compared to
depending on dbus and/or making the profiles less restrictive to allow opening
firefox; my concerns here are both with the specific implementation and also
that having one in this project might not be necessary (compared to leveraging
existing custom URL handlers). Again, I'll expand on this on a new discussion.

The draft status of the PR guarantees nothing will be changed as far as
existing methods for URL handling is concerned.

I don't understand what you mean. Is this not intended to be merged?

The draft status can mean WIP (and that the PR is intended to be merged
eventually) or that it's just a POC (and that the PR is not intended to be
merged); it's hard to be 100% sure by just the status itself. Unless
explicitly stated otherwise, I'd usually assume it's the former.

Also, this comment is rather ambiguous:

So what changes exactly (if any) are intended for 0.9.72?

@glitsj16
Copy link
Collaborator Author

glitsj16 commented Jan 7, 2023

But was it already broken as of 0.9.70 or was it broken afterwards?

Not sure. Will need more time to track things down in our git history.

Also, by "broken" do you mean "not working at all" or half-working (such as by
the program opening firefox but it not having access to the desired profile)?
I do remember seeing multiple issues about this over time, but I assumed that
firefox would at least open the URL on the most common scenario.

Based on the string of issues we've been getting on problems with opening hyperlinks (with Firefox), I'm at a point that I'm not sure about what's what and when I re-read some of those I get conflicting impressions. That's a big part of what motivates me to try to find a reasonably secure way of ending this mess.

So what changes exactly (if any) are intended for 0.9.72?

I'll try to answer your questions about the status of this PR. We might have different working definitions here. For me it's not as clear-cut as either WIP or POC. I didn't ponder on this fine distinction when I opened this to be honest. But I guess one can look at it as an ongoing attempt at a smallish POC :-)
Again, I do not intend to be set anything in stone here. None of it is slated for 0.9.72 (which is why I didn't attach it to any milestone).

To be clear, I'd personally also prefer a socket-based approach compared to
depending on dbus and/or making the profiles less restrictive to allow opening
firefox; my concerns here are both with the specific implementation and also
that having one in this project might not be necessary (compared to leveraging
existing custom URL handlers). Again, I'll expand on this on a new discussion.

I understand, no worries. Looking forward to reading your take on the topic.

Have a nice weekend!

@kmk3
Copy link
Collaborator

kmk3 commented Jan 8, 2023

@glitsj16 commented on Jan 7:

But was it already broken as of 0.9.70 or was it broken afterwards?

Not sure. Will need more time to track things down in our git history.

To clarify, I asked because if it was indeed broken after 0.9.70, then it's a
bit more urgent, as it would mean that the functionality is currently working
for users and that releasing 0.9.72 as is would break it.

Even with something like fireurl, I'm not sure how easy it would be to convince
all applicable programs to use it as the browser (though that probably warrants
its own discussion).

If it's hard to find out when it broke and why, and if it's deemed important
enough, maybe it would make sense to delay the release for a bit to have more
time to discuss and fix this.

Based on the string of issues we've been getting on problems with opening
hyperlinks (with Firefox), I'm at a point that I'm not sure about what's what
and when I re-read some of those I get conflicting impressions. That's a big
part of what motivates me to try to find a reasonably secure way of ending
this mess.

It's indeed confusing and I also remember reading conflicting accounts on this.

So what changes exactly (if any) are intended for 0.9.72?

I'll try to answer your questions about the status of this PR. We might have
different working definitions here. For me it's not as clear-cut as either
WIP or POC.

Yeah, the terms are not necessarily clear-cut / an "either or"; what I meant
was more about the intent/expectations of the author and where a given PR falls
in the spectrum of how "ready" it is. Examples:

  • "This is ready; please review"
  • "This is almost ready and likely won't change much; feel free to review"
  • "This works except for X; how to fix X?"
  • "I'm trying to implement big feature X and this is where I'm at; is this
    correct approach?"
  • "I had an idea and I made a POC; don't bother reviewing it too much at this
    stage as it's mostly a code dump and I intend to change it a lot"
  • "This is just a POC to demonstrate X (not intended for merging)"

Note: Some of these examples might make more sense in bigger projects (such as
in the LKML).

I didn't ponder on this fine distinction when I opened this to be honest. But
I guess one can look at it as an ongoing attempt at a smallish POC :-) Again,
I do not intend to be set anything in stone here. None of it is slated for
0.9.72 (which is why I didn't attach it to any milestone).

Just to clarify, I don't understand the connection between "The draft status of
the PR" and "guarantees nothing will be changed as far as [...]". If you
consider the changed code itself to be in a draft status (which might not be
immediately obvious to anyone else), from the perspective of a reviewer, that
could be changed at any time with a single push and also the PR "Draft" status
can be easily toggled at will.

So I assumed that you meant that "draft status" == "not intended to be merged
soon" (which is not necessarily the case).

I understand, no worries. Looking forward to reading your take on the topic.

Have a nice weekend!

Thanks; you too!

@kmk3
Copy link
Collaborator

kmk3 commented Jan 11, 2023

I'll expand on this on a new discussion.

Here it is:

@vargn
Copy link

vargn commented Aug 24, 2024

When will this be added to master branch?

@kmk3 kmk3 changed the title fireurl introduction feature: add fireurl Aug 25, 2024
@kmk3 kmk3 added the enhancement New feature request label Aug 25, 2024
@kmk3 kmk3 added the sandbox-ipc Opening links and talking to programs outside of the sandbox (see #6462) label Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature request sandbox-ipc Opening links and talking to programs outside of the sandbox (see #6462)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants