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

Attachment support #56

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

ringtailsoftware
Copy link
Contributor

I've added support for attaching a single file.
It was working here (with my other changes), until I merged with the latest composer changes.

Raising a PR in case it's obvious to you what's gone wrong, else I'll continue later...

Only supports a single file. Doesn't generate a blurhash, so Mastodon seems to show the image as "Not available" for a few seconds.
index.js Outdated Show resolved Hide resolved
@benbrown
Copy link
Owner

This is super cool, thank you for this contribution.

One thing I am wondering is if there is a very strong benefit to wrapping the media as base64 in a json file? Does it make it easier to handle in express?

My concerns are that his will result in large files that can't be easily inspected. I would prefer to write them as normal files so that the owner of the instance can look at them without extra tooling. Also, this would allow them to be served out of a static media folder instead of processing them through a handler.

Using the hash of the content to create the filename is nice since it elegantly handles duplicate files.

Also, this will need a way to add the content description as well -- I am happy to work on that if you would prefer?

Copy link
Owner

@benbrown benbrown left a comment

Choose a reason for hiding this comment

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

Thanks again! See comments.

lib/account.js Show resolved Hide resolved
lib/account.js Outdated Show resolved Hide resolved
public/app.js Outdated Show resolved Hide resolved
@benbrown
Copy link
Owner

In my testing, the POST will fail for files that are larger than about 1 MB. Chrome debugger doesn't really show much - it says the request canceled.

I suspect this has to do with Express hitting the payload size, but I'm not sure. Base64 stuff gets big fast.

…ad, return immediately and handle the rest in .then()

This was broken on Firefox, seemed ok in Chrome.
@ringtailsoftware
Copy link
Contributor Author

I agree about base64 encoded files getting big. I only chose to do it that way so that each individual media file on disk would have the binary data and the metadata (mimetype, + description, blurhash etc in future).
If I just wrote the raw datafile to disk (named by hash), is there already a good place to stash the metadata?
Or, should I have a _meta.json file alongside?

@ringtailsoftware
Copy link
Contributor Author

I'm happy for you to do the content description, yes.
There's some code in cubiti to calculate image file dimensions and the blurhash too, which can be reused: https://github.com/ringtailsoftware/cubiti/blob/main/cubiti-server/routes/mastodon.js#L987

@ringtailsoftware
Copy link
Contributor Author

In my testing, the POST will fail for files that are larger than about 1 MB. Chrome debugger doesn't really show much - it says the request canceled.

I suspect this has to do with Express hitting the payload size, but I'm not sure. Base64 stuff gets big fast.

I've just tested ok with an 8MB PNG (Firefox). Took a little while but worked. Initially it was failing due to nginx (https://www.cyberciti.biz/faq/linux-unix-bsd-nginx-413-request-entity-too-large/)

@ringtailsoftware
Copy link
Contributor Author

@benbrown I'm done. Ready for you to try it.

@benbrown
Copy link
Owner

you are on fire @ringtailsoftware. I was just looking at your recent commits. I'll try it out when I can!!

@benbrown
Copy link
Owner

Why do we need to store the metadata separately from where it lives in the post itself?

I think we get mime-type for free if the file gets written out to /media/hash.png

@benbrown
Copy link
Owner

@ringtailsoftware this is awesome. I might put some work into this while you sleep -- if the power doesn't go out in texas!!

@ringtailsoftware
Copy link
Contributor Author

If we use the file extension then we'd still have to decode ".png" into "image/png" and so on.
In theory, so long as the browser sends a valid mimetype this scheme should work for video and other formats without a mapping table.

@benbrown
Copy link
Owner

benbrown commented Dec 22, 2022

The original filename must be available in the browser - it could be passed as part of the payload, and then can use that to apply the extension. Ultimately I think it is important to have human readable files in the /media folder (meaning I can look at them with my normal tools)

@ringtailsoftware
Copy link
Contributor Author

The original filename must be available in the browser - it could be passed as part of the payload, and then can use that to apply the extension. Ultimately I think it is important to have human readable files in the /media folder (meaning I can look at them with my normal tools)

Ok, in that case, I'd suggest taking the mimetype "image/png" and renaming the data file to ${hash}.${mimetype.split('/')[1]}. That way the original filename doesn't matter, but you can still view the files easily.

I'm concerned about someone uploading a file called "../../" or a PNG called "foo.jpg", etc.

@ringtailsoftware
Copy link
Contributor Author

That'll do it

…(eg. abc123.png). This means that the media directory could be served by a static webserver.

When a request comes in via existing /media/:id endpoint, drop the .ext and lookup the mime type (for Content-Type) via the metadata.
@ringtailsoftware
Copy link
Contributor Author

Minor tweak made to allow media directory to be statically served

@benbrown benbrown added the wont-merge indicates this PR will not be merged label Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wont-merge indicates this PR will not be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants