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

luci-app-filemanager: error opening or downloading files #7490

Open
1 task done
rdragonrydr opened this issue Dec 21, 2024 · 21 comments
Open
1 task done

luci-app-filemanager: error opening or downloading files #7490

rdragonrydr opened this issue Dec 21, 2024 · 21 comments

Comments

@rdragonrydr
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

screenshots or captures

No response

Actual behaviour

I installed luci-app-filemanager in the latest stable OS release, but I can't seem to access or download certain file types, or maybe it's by size? I should have plenty of free memory:
image

I don't have many files at hand to test with, but a text file worked (3K) while images and videos (binary files between 4 and 100M) did not. I SUSPECT it's because these are binary files, and perhaps it's sensitive to one of the bytes, perhaps a null?

Screenshot from 2024-12-20 22-14-25

I was looking at the code, but LuCI's app-as-JavaScript setup confuses me (where's the backend?) so I'm still not sure why it wouldn't be working. Otherwise I would have raised a PR, but I don't feel confident in fixing the issue.

However, I did see that the download is being stored as a blob in the browser side before it downloads. I've had issues with this in the past with my own web apps, so it's going to likely cause compatibility issues with low end PCs or with mobile devices.

Expected behaviour

I would expect downloading files of any size, format, and type to work, and ideally clicking an image or video would render it in browser.

Steps to reproduce

git clone https://git.openwrt.org/openwrt/openwrt.git
cd openwrt/
git tag
git checkout v24.10.0-rc2
./scripts/feeds update -a
./scripts/feeds install -a
make menuconfig
(set the device here and enable luci-app-filemanager)
make defconfig
cat diffconfig MV1000.txt >> .config
//NOTE: I also retried this entire process afresh after removing the filebrowser package in case of conflicts. It didn't help.
make diffconfig
make

I uploaded the image, not keeping settings, to the router using the webUI.

I then logged into the webUI, enabled a SD card mountpoint for storage (4 GB ext4), browsed to the files, and attempted to access them, but clicking them (or the download button) did nothing but raise an error.

Renaming the files did not help, if it's checking by extension and not a memory issue.

Additional Information

NAME="OpenWrt"
VERSION="24.10.0-rc2"
ID="openwrt"
ID_LIKE="lede openwrt"
PRETTY_NAME="OpenWrt 24.10.0-rc2"
VERSION_ID="24.10.0-rc2"
HOME_URL="https://openwrt.org/"
BUG_URL="https://bugs.openwrt.org/"
SUPPORT_URL="https://forum.openwrt.org/"
BUILD_ID="r28161-ea17e958b9"
OPENWRT_BOARD="mvebu/cortexa53"
OPENWRT_ARCH="aarch64_cortex-a53"
OPENWRT_TAINTS="no-all"
OPENWRT_DEVICE_MANUFACTURER="OpenWrt"
OPENWRT_DEVICE_MANUFACTURER_URL="https://openwrt.org/"
OPENWRT_DEVICE_PRODUCT="Generic"
OPENWRT_DEVICE_REVISION="v0"
OPENWRT_RELEASE="OpenWrt 24.10.0-rc2 r28161-ea17e958b9"
OPENWRT_BUILD_DATE="1733226068"

What browsers do you see the problem on?

Firefox

Relevant log output

This page uses the non standard property “zoom”. Consider using calc() in the relevant property values, or using “transform” along with “transform-origin: 0 0”. filemanager
Failed to load config: Resource not found filemanager.js:4:1186
    loadConfig http://192.168.7.1/luci-static/resources/view/system/filemanager.js?v=24.337.27339~b1968d9:4
    load http://192.168.7.1/luci-static/resources/view/system/filemanager.js?v=24.337.27339~b1968d9:332
    __init__ http://192.168.7.1/luci-static/resources/luci.js?v=24.337.27339~b1968d9:147
    super http://192.168.7.1/luci-static/resources/luci.js?v=24.337.27339~b1968d9:22
    ClassConstructor http://192.168.7.1/luci-static/resources/luci.js?v=24.337.27339~b1968d9:12
    compileClass http://192.168.7.1/luci-static/resources/luci.js?v=24.337.27339~b1968d9:182
    (Async: promise callback)
    compileClass http://192.168.7.1/luci-static/resources/luci.js?v=24.337.27339~b1968d9:178
    (Async: promise callback)
    require http://192.168.7.1/luci-static/resources/luci.js?v=24.337.27339~b1968d9:184
    instantiateView http://192.168.7.1/luci-static/resources/ui.js?v=24.337.27339~b1968d9:363
    <anonymous> http://192.168.7.1/cgi-bin/luci/admin/system/filemanager:56
@systemcrash
Copy link
Contributor

@rdmitry0911

@stokito
Copy link
Contributor

stokito commented Dec 22, 2024

Looks like we hit some limit of the JSON-RPC response.

The whole download should be rebuilt:

// Execute 'cat' to read the file content
fs.exec('cat', [filePath]).then(function(res) {

This should be replaced with:

handleDownload(path, fileStat, ev) {

@systemcrash
Copy link
Contributor

Seems a reasonable suggestion. @rdmitry0911

@jow-
Copy link
Contributor

jow- commented Dec 24, 2024

That's what luci.fs.read_direct() is for. Even better is constructing a POST request to /cgi-bin/cgi-download with the appropriate params, this way a direct download can be triggered without having to deal with intermediate blobs.

See https://github.com/openwrt/luci/blob/master/modules/luci-mod-system/htdocs/luci-static/resources/view/system/flash.js#L172-L190 for an existing example

@rdmitry0911
Copy link
Contributor

Seems a reasonable suggestion. @rdmitry0911

Yes, I'll fix this.

@rdmitry0911
Copy link
Contributor

Created a PR to fix this issue

@systemcrash
Copy link
Contributor

@rdragonrydr are you able to test that these changes fix your problem? ( just download the file and replace the copy on your router )

https://github.com/openwrt/luci/pull/7501/files

@rdragonrydr
Copy link
Author

rdragonrydr commented Dec 24, 2024

@systemcrash

@rdragonrydr are you able to test that these changes fix your problem? ( just download the file and replace the copy on your router )

https://github.com/openwrt/luci/pull/7501/files

I think I've done something incorrectly there, the page is not working and I get the error,

HTTP error 403 while loading class file "/luci-static/resources/view/system/filemanager.js?v=24.337.27339~b1968d9"
  at compileClass (http://192.168.7.1/luci-static/resources/luci.js?v=24.337.27339~b1968d9:168:16)

I copied the file to /www/luci-static/resources/view/system/. All other LuCI pages still work.

Do I need to set permissions?

@stokito
Copy link
Contributor

stokito commented Dec 24, 2024

try logout then login, also open the JS file in a separate tab and hard refresh it with Ctrl+F5

@rdragonrydr
Copy link
Author

Thanks, I tried that... turned out it was readable only by root after all, but knowing I had the correct file helped.

Downloading now works. Clicking the file still gives the same (prior) error. I'm going to hazard a guess the server-side code wasn't shared between the two features, but both were faulty.

Editing a binary file probably wouldn't work too well, but that's what the hex editor is for, so I'd still like that to function too.

That said, I'd love to see a (plugin?) method to actually view the files if supported by the browser.

@rdmitry0911
Copy link
Contributor

Thanks, I tried that... turned out it was readable only by root after all, but knowing I had the correct file helped.

Downloading now works. Clicking the file still gives the same (prior) error. I'm going to hazard a guess the server-side code wasn't shared between the two features, but both were faulty.

Editing a binary file probably wouldn't work too well, but that's what the hex editor is for, so I'd still like that to function too.

That said, I'd love to see a (plugin?) method to actually view the files if supported by the browser.

I updated the code in PR to use fs.read_direct() for file reading and now it can edit large binary files

@rdmitry0911
Copy link
Contributor

Thanks, I tried that... turned out it was readable only by root after all, but knowing I had the correct file helped.
Downloading now works. Clicking the file still gives the same (prior) error. I'm going to hazard a guess the server-side code wasn't shared between the two features, but both were faulty.
Editing a binary file probably wouldn't work too well, but that's what the hex editor is for, so I'd still like that to function too.
That said, I'd love to see a (plugin?) method to actually view the files if supported by the browser.

I updated the code in PR to use fs.read_direct() for file reading and now it can edit large binary files

It's now in PR here Download works as well as hexedit (alt click) of binary files.

@systemcrash
Copy link
Contributor

@rdragonrydr can you test #7505 to see whether it resolves your use-case?

@rdragonrydr
Copy link
Author

rdragonrydr commented Dec 27, 2024

I've tested the new PR. Files appear to download fine still.
However, clicking does not load the editor OR the hex editor. I got the error, "Failed to open file: The file does not contain valid text data." when clicking.

I was hoping the hex editor would work, but blocking use of both works too. (As one would expect, clicking the editor tab manually just shows "Loading..." or the content of the last editable file. That latter may be a minor issue if it still saves that old file over the new one if you edit it anyway.)

I would like if the hex editor worked on binary files, though.

@rdmitry0911
Copy link
Contributor

rdmitry0911 commented Dec 27, 2024

I've tested the new PR. Files appear to download fine still. However, clicking does not load the editor OR the hex editor. I got the error, "Failed to open file: The file does not contain valid text data." when clicking.

I was hoping the hex editor would work, but blocking use of both works too. (As one would expect, clicking the editor tab manually just shows "Loading..." or the content of the last editable file. That latter may be a minor issue if it still saves that old file over the new one if you edit it anyway.)

I would like if the hex editor worked on binary files, though.

Hex editor IS working. To invoke it you should use 'ALT' + click on file name. I wrote this in my message and it is written in help tab.

@systemcrash
Copy link
Contributor

Evidently it’s not obvious to a user unless they already know it. This discoverability must be improved. Using button captions which change depending on selection is a good Luci paradigm.

@rdmitry0911
Copy link
Contributor

I agree that this can be improved. I would change the message from "Failed to open file: The file does not contain valid text data." to something like this: "Failed to open file: The file does not contain valid text data. Use 'ALT' +click to open this file in Hex editor mode"

@rdragonrydr
Copy link
Author

rdragonrydr commented Dec 29, 2024

@rdmitry0911 I like the suggestion, but that's going to be annoying if someone's on mobile or a Raspberry Pi with Touchscreen. Not sure who'd do hex editing on mobile, but in a pinch there's probably someone who has.

If it detects the file is invalid, is there a way to automatically target the hex editor to open instead?

I have checked now, and alt-clicking does work though. Thanks!

@rdmitry0911
Copy link
Contributor

@rdmitry0911 I like the suggestion, but that's going to be annoying if someone's on mobile or a Raspberry Pi with Touchscreen. Not sure who'd do hex editing on mobile, but in a pinch there's probably someone who has.

If it detects the file is invalid, is there a way to automatically target the hex editor to open instead?

I have checked now, and alt-clicking does work though. Thanks!

I made some changes in logic. Now it opens non-text files in hex Editor by default.

@rdragonrydr
Copy link
Author

Sorry to have taken a while to test it. I have tried your updated filemanager and the hex editor opens automatically.
Great work, and thank you very much!

@rdmitry0911
Copy link
Contributor

@rdragonrydr I removed 'Toggle to ASCII mode' button if the editing file is non text. Please, check if it works for you

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

No branches or pull requests

5 participants