reenable sandbox on macos by hpfmn · Pull Request #7500 · qutebrowser/qutebrowser

hpfmn · 2022-11-28T23:12:01Z

This is my first attempt to reenable sandboxing on macos. This is based on the work by @rokm as described in #7278

I'm not sure if it is relevant but I tested it on an M1 MacBook Air and the script created a native arm build

How to test this:

❯ python3 -m venv venv
❯ source ./venv/bin/activate
❯ pip install -r requirements.txt
❯ pip install asciidoc tox pyqt6==6.3.1
❯ python scripts/dev/build_release.py --qt6

Now you should have an app bundle in dist/qutebrowser.app

Fixes: #7278

toofar

If this works then it looks pretty standalone which is nice. Apart from the change to __init__.py of course.

Can you provide some guidance to other mac users on how they might be able to help us test this? I know nothing about mac or creating installers :)

The linters on CI have some suggestions for you.

toofar · 2022-11-29T04:59:19Z

qutebrowser/__init__.py

 __version_info__ = tuple(int(part) for part in __version__.split('.'))
 __description__ = "A keyboard-driven, vim-like browser based on Python and Qt."

+if 'QTWEBENGINE_DISABLE_SANDBOX' in os.environ:


It's not clear to me 1) where this is being set in the first place that we need to unset it here (thats the kind of thing that I expect to be explained in commit messages for future devs who want to look why a change was added) 2) whether we should be unilaterally disabling this on all platforms or only for mac (when running from a pyinstaller packaged app?)

If it's pyinstaller doing this could we add a test that'll tell us when pyinstaller releases a new version that doesn't do this?

Also I think this should probably be done in earlyinit, not at import time (there the logging module should be set up too). If it could be done later that would be even better. I'm not sure what the actual step it needs to be done before is. Ideally it would be done after we conditionally launch an existing instance. Upstream says to set it before you import any Qt stuff. But is it really? Or is it before you import webengine, or even before you instantiate webengine stuff?

Yep, this is definitely the wrong place for it. QtWebEngine reads the variable when the WebEngineContext() is initialized, which is quite late - here is a quick try with gdb and --debug:

09:49:57 DEBUG init app:init:144 Starting init... [...] 09:50:22 DEBUG extensions loader:_load_component:126 Importing qutebrowser.components.adblockcommands 09:50:24 DEBUG extensions loader:_load_component:126 Importing qutebrowser.components.braveadblock 09:50:24 DEBUG extensions loader:_load_component:134 Running init hook 'init' 09:50:24 DEBUG extensions loader:_load_component:126 Importing qutebrowser.components.caretcommands 09:50:24 DEBUG extensions loader:_load_component:126 Importing qutebrowser.components.hostblock 09:50:24 DEBUG extensions loader:_load_component:134 Running init hook 'init' 09:50:24 DEBUG network hostblock:_should_be_used:83 Configured adblock method auto, adblock library usable: True 09:50:24 DEBUG extensions loader:_load_component:126 Importing qutebrowser.components.misccommands 09:50:24 DEBUG extensions loader:_load_component:126 Importing qutebrowser.components.readlinecommands 09:50:24 DEBUG extensions loader:_load_component:126 Importing qutebrowser.components.scrollcommands 09:50:24 DEBUG extensions loader:_load_component:126 Importing qutebrowser.components.utils.blockutils 09:50:24 DEBUG extensions loader:_load_component:126 Importing qutebrowser.components.zoomcommands 09:50:24 DEBUG init app:_init_modules:454 Initializing logging from config... 09:50:24 DEBUG init log:init_from_config:550 --debug flag overrides log configs 09:50:24 DEBUG init app:_init_modules:458 Initializing save manager... 09:50:24 DEBUG init app:_init_modules:464 Checking backend requirements... 09:50:24 DEBUG init app:_init_modules:467 Initializing prompts... 09:50:24 DEBUG init app:_init_modules:470 Initializing network... 09:50:24 DEBUG init app:_init_modules:473 Initializing proxy... 09:50:24 DEBUG init app:_init_modules:477 Initializing downloads... 09:50:24 DEBUG init app:_init_modules:483 Initializing web history... [...] 09:50:27 DEBUG init app:_init_modules:492 Initializing command history... 09:50:27 DEBUG init app:_init_modules:495 Initializing websettings... 09:50:27 DEBUG init webenginesettings:init:517 Initializing qute://* handler... 09:50:27 DEBUG init webenginesettings:init:521 Initializing request interceptor... 09:50:27 DEBUG init webenginesettings:init:526 Initializing QtWebEngine downloads... 09:50:27 DEBUG init webenginesettings:init:532 Initializing notification presenter... [...] 09:50:27 DEBUG init webenginesettings:init:535 Initializing global settings... 09:50:27 DEBUG init webenginesettings:init:539 Initializing profiles... Thread 1 "python3" hit Breakpoint 1, QtWebEngineCore::WebEngineContext::WebEngineContext (this=<optimized out>, this=<optimized out>) at release/../../../../qtwebengine/src/3rdparty/chromium/base/memory/ref_counted.h:32 32 release/../../../../qtwebengine/src/3rdparty/chromium/base/memory/ref_counted.h: No such file or directory.

I think the best place to do those kind of things is actually not earlyinit, but backendproblem.py. We already have some other backend-specific workarounds (like dealing with stale service worker data) there, and with an appropriate function name (say _reenable_pyinstaller_macos_sandboxing()), it'd be much clearer why this is done even without reading the commit message (but agreed, that should at minimum link to the appropriate qutebrowser and PyInstaller issue).

I also agree this should be done only when all of those conditions are true:

Running on Qt 6

Running on macOS

Running with PyInstaller (hasattr(sys, 'frozen'))

I don't think we can easily write a test for it (tests don't run with PyInstaller) - but if the three conditions above are true, we could actually do this without checking os.environ first. Then if it's not needed anymore after a PyInstaller upgrade, the del will raise a KeyError, and that will be caught by the smoke tests before the release (if not before).

scripts/dev/build_release.py

The-Compiler · 2022-11-29T09:02:20Z

qutebrowser/__init__.py

 __version_info__ = tuple(int(part) for part in __version__.split('.'))
 __description__ = "A keyboard-driven, vim-like browser based on Python and Qt."

+if 'QTWEBENGINE_DISABLE_SANDBOX' in os.environ:


Yep, this is definitely the wrong place for it. QtWebEngine reads the variable when the WebEngineContext() is initialized, which is quite late - here is a quick try with gdb and --debug:

09:49:57 DEBUG init app:init:144 Starting init... [...] 09:50:22 DEBUG extensions loader:_load_component:126 Importing qutebrowser.components.adblockcommands 09:50:24 DEBUG extensions loader:_load_component:126 Importing qutebrowser.components.braveadblock 09:50:24 DEBUG extensions loader:_load_component:134 Running init hook 'init' 09:50:24 DEBUG extensions loader:_load_component:126 Importing qutebrowser.components.caretcommands 09:50:24 DEBUG extensions loader:_load_component:126 Importing qutebrowser.components.hostblock 09:50:24 DEBUG extensions loader:_load_component:134 Running init hook 'init' 09:50:24 DEBUG network hostblock:_should_be_used:83 Configured adblock method auto, adblock library usable: True 09:50:24 DEBUG extensions loader:_load_component:126 Importing qutebrowser.components.misccommands 09:50:24 DEBUG extensions loader:_load_component:126 Importing qutebrowser.components.readlinecommands 09:50:24 DEBUG extensions loader:_load_component:126 Importing qutebrowser.components.scrollcommands 09:50:24 DEBUG extensions loader:_load_component:126 Importing qutebrowser.components.utils.blockutils 09:50:24 DEBUG extensions loader:_load_component:126 Importing qutebrowser.components.zoomcommands 09:50:24 DEBUG init app:_init_modules:454 Initializing logging from config... 09:50:24 DEBUG init log:init_from_config:550 --debug flag overrides log configs 09:50:24 DEBUG init app:_init_modules:458 Initializing save manager... 09:50:24 DEBUG init app:_init_modules:464 Checking backend requirements... 09:50:24 DEBUG init app:_init_modules:467 Initializing prompts... 09:50:24 DEBUG init app:_init_modules:470 Initializing network... 09:50:24 DEBUG init app:_init_modules:473 Initializing proxy... 09:50:24 DEBUG init app:_init_modules:477 Initializing downloads... 09:50:24 DEBUG init app:_init_modules:483 Initializing web history... [...] 09:50:27 DEBUG init app:_init_modules:492 Initializing command history... 09:50:27 DEBUG init app:_init_modules:495 Initializing websettings... 09:50:27 DEBUG init webenginesettings:init:517 Initializing qute://* handler... 09:50:27 DEBUG init webenginesettings:init:521 Initializing request interceptor... 09:50:27 DEBUG init webenginesettings:init:526 Initializing QtWebEngine downloads... 09:50:27 DEBUG init webenginesettings:init:532 Initializing notification presenter... [...] 09:50:27 DEBUG init webenginesettings:init:535 Initializing global settings... 09:50:27 DEBUG init webenginesettings:init:539 Initializing profiles... Thread 1 "python3" hit Breakpoint 1, QtWebEngineCore::WebEngineContext::WebEngineContext (this=<optimized out>, this=<optimized out>) at release/../../../../qtwebengine/src/3rdparty/chromium/base/memory/ref_counted.h:32 32 release/../../../../qtwebengine/src/3rdparty/chromium/base/memory/ref_counted.h: No such file or directory.

I think the best place to do those kind of things is actually not earlyinit, but backendproblem.py. We already have some other backend-specific workarounds (like dealing with stale service worker data) there, and with an appropriate function name (say _reenable_pyinstaller_macos_sandboxing()), it'd be much clearer why this is done even without reading the commit message (but agreed, that should at minimum link to the appropriate qutebrowser and PyInstaller issue).

I also agree this should be done only when all of those conditions are true:

Running on Qt 6

Running on macOS

Running with PyInstaller (hasattr(sys, 'frozen'))

I don't think we can easily write a test for it (tests don't run with PyInstaller) - but if the three conditions above are true, we could actually do this without checking os.environ first. Then if it's not needed anymore after a PyInstaller upgrade, the del will raise a KeyError, and that will be caught by the smoke tests before the release (if not before).

scripts/dev/build_release.py

hpfmn · 2022-12-01T18:27:24Z

If this works then it looks pretty standalone which is nice. Apart from the change to __init__.py of course.

Can you provide some guidance to other mac users on how they might be able to help us test this? I know nothing about mac or creating installers :)

The linters on CI have some suggestions for you.

I updated the description how to use this. And I'll keep an eye on the linters.

The-Compiler · 2022-12-01T18:57:35Z

Did you intend to close this? 🤔

hpfmn · 2022-12-01T19:04:34Z

Did you intend to close this? 🤔

Oh no.... I just wanted to resolve this particular comment

DrTobe · 2022-12-02T16:41:17Z

I do not know if you need further confirmation here but this can be built, installed and run with the steps in the description.

If I understand correctly, this would technically solve #6478?

And regarding the sandbox: With these changes, there should be no security concerns in using this, right?

The-Compiler · 2022-12-02T17:01:55Z

If I understand correctly, this would technically solve #6478?

I don't see how the two things would be related in any way?

hpfmn · 2022-12-02T18:32:27Z

If I understand correctly, this would technically solve #6478?

I don't see how the two things would be related in any way?

If you build it like I descirbed it above you're getting a native arm build out of it. So it kinda solves it - even though it is not related directly

The-Compiler

Thanks! I added some code-style comments, but this looks great otherwise! Haven't gotten around to testing it, though.

qutebrowser/misc/backendproblem.py

scripts/dev/build_release.py

hpfmn · 2022-12-14T14:25:42Z

Is there anything more needed here?

The-Compiler · 2022-12-14T14:27:35Z

I haven't had a chance to take another look so far. Seems fine from a glance at the diff, but I haven't tested it yet.

hpfmn · 2023-01-29T14:37:26Z

I rebased this to the current upstream - but seems linting is broken upstream

The-Compiler · 2023-01-29T14:43:33Z

I rebased this to the current upstream - but seems linting is broken upstream

Whoops, my bad, indeed - fixed in f4f5ca2.

hpfmn · 2023-02-28T10:41:59Z

Linting is still broken upstream, correct?

toofar · 2023-03-01T18:57:53Z

I just forked off of the qt6-v2 branch and everything looks green: https://github.com/toofar/qutebrowser/actions/runs/4306879197/jobs/7511253081

hpfmn · 2023-04-18T18:29:54Z

I rebased this against current master with some hacks (see hack commit) to make it work

toofar · 2023-08-14T03:47:20Z

Looks like this is resolved by PyInstaller upstream so we don't need this workaround anymore.
Thank you very much for the contribution anyhow @hpfmn! We don't have many contributors on Macs so I really appreciate that you made the effort to improve things there.

toofar reviewed Nov 29, 2022

View reviewed changes

The-Compiler requested changes Nov 29, 2022

View reviewed changes

hpfmn closed this Dec 1, 2022

hpfmn reopened this Dec 1, 2022

hpfmn force-pushed the qt6-v2 branch 5 times, most recently from a562c8d to e1f2f5f Compare December 1, 2022 19:51

hpfmn force-pushed the qt6-v2 branch from e1f2f5f to e56f748 Compare December 2, 2022 17:40

The-Compiler requested changes Dec 3, 2022

View reviewed changes

hpfmn force-pushed the qt6-v2 branch from 510c636 to 4c775dd Compare December 9, 2022 20:25

hpfmn force-pushed the qt6-v2 branch from 4c775dd to e82f62d Compare January 29, 2023 14:29

hpfmn changed the title ~~WIP: reenable sandbox on macos~~ reenable sandbox on macos Jan 29, 2023

hpfmn force-pushed the qt6-v2 branch from e82f62d to 29f4bec Compare January 30, 2023 14:13

hpfmn force-pushed the qt6-v2 branch from 29f4bec to a4ed98b Compare March 15, 2023 15:57

hpfmn added 2 commits April 18, 2023 20:25

reenable sandbox

daa6fd5

hacks to make it work

af38046

hpfmn changed the base branch from qt6-v2 to master April 18, 2023 18:26

hpfmn force-pushed the qt6-v2 branch from a4ed98b to af38046 Compare April 18, 2023 18:29

give args to asciidoc

4d7c68e

rokm mentioned this pull request Jun 13, 2023

Let there be symbolic links pyinstaller/pyinstaller#7619

Merged

toofar closed this Aug 14, 2023

Uh oh!

Conversation

hpfmn commented Nov 28, 2022 • edited by toofar Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

toofar left a comment

Choose a reason for hiding this comment

Uh oh!

toofar Nov 29, 2022

Choose a reason for hiding this comment

Uh oh!

toofar Nov 29, 2022

Choose a reason for hiding this comment

Uh oh!

The-Compiler Nov 29, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

The-Compiler Nov 29, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hpfmn commented Dec 1, 2022

Uh oh!

The-Compiler commented Dec 1, 2022

Uh oh!

hpfmn commented Dec 1, 2022

Uh oh!

DrTobe commented Dec 2, 2022

Uh oh!

The-Compiler commented Dec 2, 2022

Uh oh!

hpfmn commented Dec 2, 2022

Uh oh!

The-Compiler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hpfmn commented Dec 14, 2022

Uh oh!

The-Compiler commented Dec 14, 2022

Uh oh!

hpfmn commented Jan 29, 2023

Uh oh!

The-Compiler commented Jan 29, 2023

Uh oh!

hpfmn commented Feb 28, 2023

Uh oh!

toofar commented Mar 1, 2023

Uh oh!

hpfmn commented Apr 18, 2023

Uh oh!

toofar commented Aug 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hpfmn commented Nov 28, 2022 •

edited by toofar

Loading