Cannot download title with colon in name via OPDS

[zephyros-dev]

• KOReader version: v2021.05
• Device: Google Pixel 4A (sunfish)

Issue

Same as issue #2433

Steps to reproduce

1. Download epub from any OPDS source (Project Gutenberg, Feedbooks)
2. Title with colon in the name failed with error "Could not save file to [local directory]"
3. Other title without colon can be downloaded normally

crash.log (if applicable)

crash.log is a file that is automatically created when KOReader crashes. It can normally be found in the KOReader directory:

• /mnt/private/koreader for Cervantes
• koreader/ directory for Kindle
• .adds/koreader/ directory for Kobo
• applications/koreader/ directory for Pocketbook

Android logs are kept in memory. Please go to Help → Bug Report to save these logs to a file.

Please try to include the relevant sections in your issue description. You can upload the whole crash.log file on GitHub by dragging and dropping it onto this textbox.

If you instead opt to inline it, please do so behind a spoiler tag:

crash.log

``` ```

[NiLuJe]

Right, we're probably missing fs sanitization there (as colons are a no-no on some FS).

[Frenzie]

We have a util function for that.

[NiLuJe]

Yep, which makes me wonder why it's not being used here ;o).

[Frenzie]

The issue claims it's the same as #2433 but having glanced at it now, that was fixed by its very introduction. ;-) https://github.com/koreader/koreader/pull/2464

[NiLuJe]

Possibly the filesystem check in util.getSafeFilename needs an update?

@phongngthanh: What's the target filesystem? (And is /proc/mounts actually readable by a plain app, ping @pazos?).

[Galunid]

https://github.com/koreader/koreader/blob/6b31b160a2964aacfb2fb8b44ffb75d7efa98666/frontend/util.lua#L823-L827 seems to miss ;

[Frenzie]

It's not in there because it's perfectly fine.

[zephyros-dev]

Possibly the filesystem check in util.getSafeFilename needs an update?

@phongngthanh: What's the target filesystem? (And is /proc/mounts actually readable by a plain app, ping @pazos?).

How do I check for that? I'm using Android 11 atm. I just tested on another device running Android 10 and the OPDS download normally even with colon. Maybe it had something to do with Android 11. I'll try to find some other device to test with if possible.

[pazos]

is /proc/mounts actually readable by a plain app?

No idea. Reading stuff from /proc isn't part of the api, so it can be implemented at vendors will. If we're relying on that maybe we should change it to another android API call :tm:

@phongngthanh: I would need a sample of a book that fails to download in Android11 (name, repo) to check if I can reproduce on my samsung tablet.

[zephyros-dev]

is /proc/mounts actually readable by a plain app?

No idea. Reading stuff from /proc isn't part of the api, so it can be implemented at vendors will. If we're relying on that maybe we should change it to another android API call ™️

@phongngthanh: I would need a sample of a book that fails to download in Android11 (name, repo) to check if I can reproduce on my samsung tablet.

You can try

• Project Gutenberg - "Jane Eyre: An Autobiography", "Beowulf: An Anglo-Saxon Epic Poem"
• Internet Archive - "Children, work and women: a managerial perspective" Most other books I tested with characters like ( [ ] . - ' ) works fine, but I haven't tested that thoroughly so I might be missing some edge case.

[pazos]

I can reproduce the issue here too. Nothing weird in the logs. I'm building a debug APK just to be sure I'm not missing something.

@NiLuJe: found the requisites for the kernel interface: /proc/mounts is in there (a symbolic link to /proc/self/mounts). That just mean than the android system relies on it for some stuff, but access for unprivilegied apps can be restricted by selinux

[pazos]

It turns out that : is invalid in android: https://cs.android.com/android/platform/superproject/+/master:frameworks/base/core/java/android/os/FileUtils.java;l=972?q=isValidFatFilenameChar

nothing jumps out in logs, except for this line:

06-14 19:54:29.459  3144  3614 E MediaProvider: File name contains invalid characters

[pazos]

downgrading targetSdk doesn't work either.

[Frenzie]

But the function in question errs on the side of caution. If a colon's being inserted something's going wrong somewhere.

[pazos]

@Frenzie: no idea. Maybe util.getSafeFilename fails on some android versions and that triggers a different replaceFunc in https://github.com/koreader/koreader/blob/master/frontend/util.lua#L858-L863 ?

The infomessage shows the full path with a colon, but I'm not sure if it should display the safe filename there.

[NiLuJe]

Well, if colons are invalid on Android, period, then just stick an android specific branch that doesn't even check the fs and just enforce the full sanitization?

[Frenzie]

Yes, early exit here if isAndroid() or something like that.

https://github.com/koreader/koreader/blob/6b31b160a2964aacfb2fb8b44ffb75d7efa98666/frontend/util.lua#L849-L851