-
-
Notifications
You must be signed in to change notification settings - Fork 343
Adding an option to serve files from a different server #1492
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
Conversation
Good idea. I think most services call this "external URL" or "public URL" or something like that. What do you think? |
Yeah I wanted to stress out that this would only be applicable to the links to the files, maybe calling it --file-external-url then? |
Hm how does this interact with |
I like |
It's a fairly niche use-case but I'm ok having this in. Could you try to add some more description to the help text as to why a user would even want to use this? |
Also please fix CI :) |
Sounds good, will do. I'm a bit rusty in Rust. :-P |
Indeed interesting point. The wget command would still point to the page on the "original" server and fetch the index pages from there, the links would point to the external server, and thanks to the "-H" option that is there already, be fetched from there. I think this behaviour makes sense? |
Makes sense. Did you test that behavior in practice? Would be good to validate assumptions. |
…et + cargo fmt did some stuff to tests/api.rs
I'd also love a test showing this actually does anything. Should be pretty easy. :) |
Sure, you were totally right to ask for a test. It didn't work. :-) Changed the wget command to explicitely include the -H, as this is obviously needed. Changed the tests as well then and I had cargo fmt complain about something so changed that as well. You can test on http://45.59.100.15:5555/ which is the indexing server and http://45.59.100.15:55555/ which is the "external" server. |
… time. Adding a unit test.
Need to do some more tweaking and reading Summarizing the options to make sure I have them and the intent correctly, before I started messing around there was
|
Co-authored-by: Sven-Hendrik Haase <sven@svenstaro.org>
Are you happy with this? I'll then give this another review. |
Yeah it does what it needs to for my usecase, and I'm happy that there are no more linters and tests complaining. :-D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have some traversal attack tests already. I reckon we need to add some traversal attack tests for this new flag as well because the logic that builds the path isn't covered by the old set of tests.
Should this flag conflict with |
I'll have to read up on those a bit then first, I'm not entirely sure I understand the problem. The code added here doesn't change anything to the actual file serving logic, only to the hyperlinks on a given page. Any traversal attack should be covered already by test covering the serving or files or dirs? |
I suppose 99% of the usecases of webdav for miniserve would be to mount as a filesystem, right? I'm trying hard to imagine why someone would want to use webdav in the scenario I'm proposing this change for, and I'm not even sure webdav (fs mounts) would support different hosts at all. Would you be fine with just a note in both the --file-external-url and --enable-webdav args? |
Adding an option to specify an absolute hostname to serve the files from somewhere else.
In my usecase, I wanted to have an authenticated instance for browsing and an unauthenticated one to serve files (with indexing disabled). Instead of having to rewrite the URL when sharing a link, this option would allow this without manual intervention.