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

Establish basic file browser and index fallback #2662

Merged
merged 54 commits into from
Feb 5, 2023
Merged

Establish basic file browser and index fallback #2662

merged 54 commits into from
Feb 5, 2023

Conversation

ahopkins
Copy link
Member

@ahopkins ahopkins commented Jan 24, 2023

Closes #2661

This adds a new opt in pattern when a static directory is being served.

app.static(..., autoindex=True)
app.static(..., index_name="index.html")

# also available within a handler...

@app.get("/somepath")
async def handler(_):
    return await file(..., autoindex=True, index_name="index.html")

These may be used together or separately.

  • autoindex = Whether to show a basic HTML file browser
  • index_name = The name of a fallback file in that directory to show, if any

The logic is as follows:

  • if serving a dir:
    • if index_name set and exists:
      • return index file
    • else if autoindex:
      • show file browser

image
image

@ahopkins
Copy link
Member Author

@Tronic This is the fallback browser solution that I told you I had. LMK is this is what you had in mind.

@codecov
Copy link

codecov bot commented Jan 24, 2023

Codecov Report

Base: 88.650% // Head: 88.642% // Decreases project coverage by -0.009% ⚠️

Coverage data is based on head (5b6a1de) compared to base (4ad8168).
Patch coverage: 92.775% of modified lines in pull request are covered.

❗ Current head 5b6a1de differs from pull request most recent head b69a2ea. Consider uploading reports for the commit b69a2ea to get more accurate results

Additional details and impacted files
@@              Coverage Diff              @@
##              main     #2662       +/-   ##
=============================================
- Coverage   88.650%   88.642%   -0.009%     
=============================================
  Files           82        87        +5     
  Lines         6767      6850       +83     
  Branches      1156      1171       +15     
=============================================
+ Hits          5999      6072       +73     
- Misses         531       538        +7     
- Partials       237       240        +3     
Impacted Files Coverage Δ
sanic/application/logo.py 100.000% <ø> (ø)
sanic/blueprints.py 91.133% <ø> (-0.044%) ⬇️
sanic/constants.py 100.000% <ø> (ø)
sanic/handlers/error.py 97.368% <ø> (ø)
sanic/http/tls/creators.py 95.614% <ø> (ø)
sanic/mixins/startup.py 91.196% <ø> (ø)
sanic/request.py 94.527% <ø> (ø)
sanic/server/websockets/frame.py 92.523% <ø> (ø)
sanic/server/websockets/impl.py 38.479% <ø> (ø)
sanic/touchup/meta.py 85.714% <ø> (ø)
... and 10 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Tronic
Copy link
Member

Tronic commented Jan 24, 2023

The file browser formats fairly lengthy HTML documents by string.format (if I read the code correctly). Maybe we at this time should consider a minimal templating engine, something like html5tagger (my own minimalistic module for creating HTML from Python code), or some more sensible way of handling these?

As a bare minimum, filenames and such strings require escaping.

I gather the error pages are another place where bare strings are becoming unsustainable, as Sanic doesn't create much other HTML output itself than those and now this browser.

@ahopkins
Copy link
Member Author

This is very minimal stuff we're doing. It does not warrant a dependency IMO.

@Tronic
Copy link
Member

Tronic commented Jan 25, 2023

The (fast) escape function in sanic.errorpages is only for HTML content, not for attributes, so it is not sufficient for file browser which needs to put filenames in href attributes (unless we wish to assume that those don't include " characters, which is going on very dangerous waters).

I took the HTML formatting discussion to sanic-repo on Discord as it is clearly off-topic regarding this PR.

@ahopkins
Copy link
Member Author

Let's keep this in hold until we resolve the Discord conversation. In short, we are talking about potentially including a minimal lib for html rendering.

@Tronic
Copy link
Member

Tronic commented Jan 25, 2023

Another somewhat off-topic comment (sorry about my wandering mind). With a file browser Sanic is getting very close to something like python -m http.server (but with support for TLS and everything). Not saying that Sanic CLI should have it but 💭

@ahopkins
Copy link
Member Author

ahopkins commented Jan 25, 2023

Another somewhat off-topic comment (sorry about my wandering mind). With a file browser Sanic is getting very close to something like python -m http.server (but with support for TLS and everything). Not saying that Sanic CLI should have it but thought_balloon

It already does have this:

sanic /path/to/some/dir --simple

What I do want to upgrade is making the argument portion choose between a Sanic instance, a factory method, and a simple server dir without having to specify --simple or --factory.

@Tronic
Copy link
Member

Tronic commented Jan 26, 2023

It's looking good, didn't have much to comment on code but I didn't try actually running it yet.

@Tronic
Copy link
Member

Tronic commented Jan 26, 2023

At the rate Sanic is evolving with your patches, I wouldn't be surprised if this soon also supported multiple return formats, and then offered a JS frontend that fetches file lists in JSON to avoid page reloads while streaming video files to browser for quick playback like Droppy can 😆

@ahopkins
Copy link
Member Author

ahopkins commented Jan 26, 2023

It's looking good, didn't have much to comment on code but I didn't try actually running it yet.

Yeah. I think we should take your idea a step further and make the styles for this and the error pages common. Probably it's own PR though. It's a nice touch.

I've put next to zero thought into file name encoding and how it may impact navigation. We also should think about what the normal config patterns will be and if we need to separate autoindex and index.html fallback. Right now they are both explicit opt in separately.

@Tronic
Copy link
Member

Tronic commented Jan 26, 2023

I think the file browser and autoindex serve very different functions, and I don't want anyone on the Internet browsing the files of my webapps (that simply have a static index.html at root to serve their frontend), so keep them separated.

Regarding filename encoding, that could get really tricky with Unicode (the mentioned Droppy allows uploads with Unicode NFD and saves them on disk, but can then no longer load/touch those files because filenames from URLs become NFC, but that might be some NodeJS internals and not what browsers do). I will look into this once I get to run this [an extremely busy day].

One way that it is [supposed to be] handled is take the filename bytes and %-encode them, not even assuming that they are Unicode at any point. I hope that we can avoid this and work with str as usual.

@ahopkins
Copy link
Member Author

ahopkins commented Jan 26, 2023

I did some refactoring to break apart the autoindexing and allow for a new common module for our growing list of handlers and pages.

@Tronic
Copy link
Member

Tronic commented Jan 26, 2023

I did some refactoring to break apart the autoindexing and allow for a new common module for our growing list of handlers and pages.

Great. I was also thinking of that. And it could also have a small Sanic header/footer with the S logo in inline SVG, if we want to push the branding a bit more.

sanic/mixins/static.py Outdated Show resolved Hide resolved
sanic/mixins/static.py Outdated Show resolved Hide resolved
@ahopkins ahopkins marked this pull request as ready for review February 5, 2023 11:09
sanic/pages/css.py Outdated Show resolved Hide resolved
Tronic
Tronic previously approved these changes Feb 5, 2023
Copy link
Member

@Tronic Tronic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ahopkins ahopkins marked this pull request as draft February 5, 2023 11:49
@ahopkins ahopkins marked this pull request as ready for review February 5, 2023 11:49
@ahopkins ahopkins merged commit 9cb9e88 into main Feb 5, 2023
@ahopkins ahopkins deleted the issue2661 branch February 5, 2023 13:09
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.

Optional file browser
2 participants