Skip to content

Conversation

jankeymeulen
Copy link
Contributor

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.

@svenstaro
Copy link
Owner

Good idea. I think most services call this "external URL" or "public URL" or something like that. What do you think?

@jankeymeulen
Copy link
Contributor Author

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?

@svenstaro
Copy link
Owner

Hm how does this interact with --show-wget-footer? Should that change it?

@svenstaro
Copy link
Owner

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?

I like --file-external-url.

@svenstaro
Copy link
Owner

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?

@svenstaro
Copy link
Owner

Also please fix CI :)

@jankeymeulen
Copy link
Contributor Author

Sounds good, will do. I'm a bit rusty in Rust. :-P

@jankeymeulen
Copy link
Contributor Author

Hm how does this interact with --show-wget-footer? Should that change it?

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?

@svenstaro
Copy link
Owner

Hm how does this interact with --show-wget-footer? Should that change it?

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.

Jan Keymeulen added 2 commits April 24, 2025 12:02
@svenstaro
Copy link
Owner

I'd also love a test showing this actually does anything. Should be pretty easy. :)

@jankeymeulen
Copy link
Contributor Author

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.

@jankeymeulen
Copy link
Contributor Author

Need to do some more tweaking and reading wget man page.

Summarizing the options to make sure I have them and the intent correctly, before I started messing around there was wget -rcnHp

  • -r for recursing
  • -c to continue failed downloads
  • -nH to disable those pesky host directories
  • -p to download the page prerequisites? I'm not sure I'm understanding why that would be needed. -np might make sense to ensure it doesn't go up in the directory tree?

@svenstaro svenstaro changed the title Adding an option to serve files from a different server. Adding an option to serve files from a different server Apr 25, 2025
Jan Keymeulen and others added 2 commits April 28, 2025 16:53
@svenstaro
Copy link
Owner

Are you happy with this? I'll then give this another review.

@jankeymeulen
Copy link
Contributor Author

Yeah it does what it needs to for my usecase, and I'm happy that there are no more linters and tests complaining. :-D

Copy link
Owner

@svenstaro svenstaro left a 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.

@svenstaro
Copy link
Owner

Should this flag conflict with --enable-webdav? Does it make sense to have both?

@jankeymeulen
Copy link
Contributor Author

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.

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?

@jankeymeulen
Copy link
Contributor Author

Should this flag conflict with --enable-webdav? Does it make sense to have both?

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?

@jankeymeulen jankeymeulen requested a review from svenstaro May 22, 2025 12:21
@svenstaro svenstaro merged commit 0ffc8c3 into svenstaro:master Jun 26, 2025
17 checks passed
svenstaro added a commit that referenced this pull request Jun 26, 2025
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

Successfully merging this pull request may close these issues.

2 participants