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

[FR]: Mobile sandboxing #4885

Closed
pazos opened this issue Apr 8, 2019 · 10 comments
Closed

[FR]: Mobile sandboxing #4885

pazos opened this issue Apr 8, 2019 · 10 comments

Comments

@pazos
Copy link
Member

pazos commented Apr 8, 2019

Currently for Android(5.0+), but should be the same for iOS(11.0+).

You cannot explore the root filesystem on mobile OSes. This is enforced on iOS since the beggining. In Android the implementation suffered some stupid modifications which ended in the same situation.

In both OS they're ways to open or import stuff.

Import stuff is the classic behaviour on iOS (by storing a copy per app) and open stuff is the classic behaviour on Android (by requesting a view action and providing a file to open)

Things are different now, where apple implemented an "open in place" stub and android killed the possibility of sharing a file between two (random) activities on Oreo.

So, sandboxing, in the context of KOReader, would mean:

  • have the primary storage path (/sdcard on android) always available on the UI.
  • have a button next to home button on fs explorer to import stuff using platform sdk
  • don't allow ../ on primary storage path
  • allow "open in place" in supported versions.

Probably a proper "Jailed" iOS app would require a few more steps but this is just the basic idea.

Related:

#4441
#4872
koreader/android-luajit-launcher#138 (comment)

@Frenzie
Copy link
Member

Frenzie commented Apr 21, 2019

Incidentally, based on a little chat on Telegram this would also be a good idea on UBports.

@houqp
Copy link
Member

houqp commented Apr 21, 2019

With "open in place" in iOS, can you still create sdr folder next to the file? If so, wouldn't it mess up the directory structure for the original app?

@pazos
Copy link
Member Author

pazos commented Apr 21, 2019

Incidentally, based on a little chat on Telegram this would also be a good idea on UBports.

👍

With "open in place" in iOS, can you still create sdr folder next to the file? If so, wouldn't it mess up the directory structure for the original app?

I don't think so, AFAIK it is something like ParcelFileDescriptor on Android. ie. you can change the original document without acessing to 3rd party sandbox.

Anyways, "open in place" in iOS context just means that documents inside KOReader sandbox are available from the Files app (and from the computer using itunes or libimobiledevice). We still need to add an import function to open documents stored on other applications. The same for android 4.4+ and Storage Acess Framework

@pazos
Copy link
Member Author

pazos commented May 20, 2019

To start I would need some help @Frenzie @poire-z. Probably easy but I didn't dig in the code yet.

I would need:

  1. prevent the file manager to access the parent directory if current directory is the sandbox root ("/sdcard")
  2. in reader mode, when user press the file manager icon, return to current directory if it is inside the sandbox, otherwise return to sandbox root.

a runtime check to see if the device is sandboxed:

+local function hasStorageAccessRestrictions()
+    -- storage access framework was introduced on Kitkat 4.4 (api 19)
+    -- see https://developer.android.com/guide/topics/providers/document-provider
+    return (android.app.activity.sdkVersion >= 19)
+end
+
 local Device = Generic:new{
     isAndroid = yes,
+    isSandboxed = hasStorageAccessRestrictions,
     model = android.prop.product,
     hasKeys = yes,
     hasDPad = no,
diff --git a/frontend/device/generic/device.lua b/frontend/device/generic/device.lua
index f253ef9b..dea117d8 100644
--- a/frontend/device/generic/device.lua
+++ b/frontend/device/generic/device.lua
@@ -26,6 +26,7 @@ local Device = {
     hasWifiToggle = yes,
     hasWifiManager = no,
     isTouchDevice = no,
+    isSandboxed = no,
     hasFrontlight = no,
     hasNaturalLight = no, -- FL warmth implementation specific to NTX boards (Kobo, Cervantes)
     hasNaturalLightMixer = no, -- Same, but only found on newer boards

to be able to build with target api > 23 we will need to request storage permission at runtime and display a message If the user denies that permission instead of crashing.

Once our fake file sandboxing is in place, we can repurpose most of koreader/android-luajit-launcher#138 to add an import function which allows to use modern providers, like external storage(usb,sd) or network services (dropbox,gdrive,etc)

@Frenzie
Copy link
Member

Frenzie commented May 21, 2019

prevent the file manager to access the parent directory if current directory is the sandbox root ("/sdcard")

I think the bases are probably mostly covered in between these two snippets.

function filemanagerutil.abbreviate(path)
if not path then return "" end
local home_dir_name = G_reader_settings:readSetting("home_dir_display_name")
if home_dir_name ~= nil then
local home_dir = G_reader_settings:readSetting("home_dir") or filemanagerutil.getDefaultDir()
local len = home_dir:len()
local start = path:sub(1, len)
if start == home_dir then
return home_dir_name .. path:sub(len+1)
end
end
return path
end

if dir.name == ".." then
text = up_folder_arrow
elseif dir.name == "." then -- possible with show_current_dir_for_hold
text = _("Long-press to select current directory")
else
text = dir.name.."/"
end

But that's just some stuff I remembered otoh. :-)

@poire-z
Copy link
Contributor

poire-z commented May 21, 2019

^ These snippets are more about the display of the current path, than going (or not) into the path, which must happen in these other snippets:

function FileManager:showFiles(path, focused_file)
path = path or G_reader_settings:readSetting("lastdir") or filemanagerutil.getDefaultDir()
G_reader_settings:saveSetting("lastdir", path)
restoreScreenMode()
local file_manager = FileManager:new{
dimen = Screen:getSize(),
covers_fullscreen = true, -- hint for UIManager:_repaint()
root_path = path,

function FileManager:reinit(path, focused_file)
self.dimen = Screen:getSize()
-- backup the root path and path items
self.root_path = path or self.file_chooser.path
local path_items_backup = {}
for k, v in pairs(self.file_chooser.path_items) do
path_items_backup[k] = v
end
-- reinit filemanager
self.focused_file = focused_file
self:init()

function FileChooser:changeToPath(path, focused_path)
path = ffiUtil.realpath(path)
self.path = path
if focused_path then
self.focused_path = focused_path
end
self:refreshPath()
self:onPathChanged(path)
end

So, may be in these snippets, just do some generic readability checks, or use some callback funcs of the Device object, used like if Device.check_dir_readable then if not Device.check_dir_readable(path) then return end end

But:

prevent the file manager to access the parent directory if current directory is the sandbox root ("/sdcard")

instead of crashing

I wonder if we should really add this sandbox directory thing, which would be android specific.
And if we shouldn't better just ensure we're not crashing when reaching an unreadable directory, and then just take an appropriate decision:

  • stay where we were, showing a warning that dir is unreadable
  • go down, just showing an empty list, or a fake item "directory unreadable", but still the "go up" item

Cause it's just about unreadable directories, because of android permissions, right?
Cause we're fine with browsable and readable directories that are just not writable (the metadata.lua is saved into koreader/history/ then, right?).

I remember that on my android, there are intermediate paths that are not readable, but going further down (by entering the full path in the file explorer) is readable - don't remember exactly which right now.

in reader mode, when user press the file manager icon, return to current directory if it is inside the sandbox, otherwise return to sandbox root.

If that requirement is because the current book is some imported content that has been saved with an ugly name in some koreader cache directory, why not just, if the current directory is that cache directory, just return to the home directory? Or just not add that cache directory to the FileManager paths stack, so we get back to the previous valid path, and if none, the home directory?

@Frenzie
Copy link
Member

Frenzie commented May 21, 2019

I wonder if we should really add this sandbox directory thing, which would be android specific.
And if we shouldn't better just ensure we're not crashing when reaching an unreadable directory, and then just take an appropriate decision:

I've never noticed any crashes due to unreadable directories. I think the program either displays an "empty" directory or the directory its run from instead. (Arguably it should revert to something else like the home directory but it's a niche edge case.) If there is any such crash it should indeed be fixed in platform-independent manner. But presumably this is just about dealing with asking for Android permissions?

@pazos
Copy link
Member Author

pazos commented May 21, 2019

Thanks for the hints!

I wonder if we should really add this sandbox directory thing, which would be android specific.

For now android specific, it also applies to iOS. Also Android < 4.4 can read dirs outside /sdcard. Newer android cannot (or some versions/vendors can read some dirs, which is the same :/)

Cause it's just about unreadable directories, because of android permissions, right?

It is, but there is no permission to read-only or read&write outside /sdcard, so we can't change this.

If that requirement is because the current book is some imported content that has been saved with an ugly name in some koreader cache directory, why not just, if the current directory is that cache directory, just return to the home directory?

Between android 4.3 and android 7.1 your phone can open files outside /sdcard because we can guess its absolute path. In that case there is no cache dir involved, just some random path and we cannot guarantee that once in that path we can return to /sdcard.

For content schemes I would suggest do not open them (and import them each time to a cache dir), but skip valid mimetypes entirely and implement an import function using a file picker, like this one:

upload_2016-8-29_13-51-54

I've never noticed any crashes due to unreadable directories. I think the program either displays an "empty" directory or the directory its run from instead

That's exactly what I saw (unreadable dirs). If I go from /storage/emulated/0 to /storage/emulated I cannot return back to last folder unless I have a shortcut already available.

But presumably this is just about dealing with asking for Android permissions?

The only storage permission available is granted at install time. We might need to request it at runtime if we want to build with target api 23+, but even then, we are only able to grant read(&write) permissions on /sdcard. Permissions outside sdcard are inexistent (that's why we need to add support for file providers, to be able to import contents from them).

@pazos
Copy link
Member Author

pazos commented May 21, 2019

intents

Imagen PNG

fm sandbox

Imagen PNG 2

@pazos
Copy link
Member Author

pazos commented Aug 7, 2019

The main issue (being unable to return home if we're outside /sdcard and home folder is not set) was fixed in #5163.

Closing. We can re-open for other mobile OSes. For android things work fine now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants