politics-rewired / Spoke

Politics Rewired's fork of Spoke
GNU General Public License v3.0
35 stars 16 forks source link

Media attachment error message false positives #1102

Closed sync-by-unito[bot] closed 1 year ago

sync-by-unito[bot] commented 2 years ago

It looks like browser-based checks will not work for this and we'll need to add a resolver for performing this validation server-side (see comment below). Open to suggestions, however.

Media attachment in question: https://s3.amazonaws.com/ak-newsom/images/GGN_P2P_Image_220131_v1.jpeg

We should also check the image's size while we're here.

sync-by-unito[bot] commented 2 years ago

➤ Benjamin Chrobot commented:

Derrick Liu shared:

some CDNs (Cloudflare, Fastly) and storage services (like S3) won't auto-set the MIME type and requires the uploader to do it manually. If people don't do it, it'll show up as application/octet-stream (generic binary blob) regardless of the type of data actually in the file. If you have an allowlist in Spoke of image MIME types then this will fail that check if you want to super accurately determine the file type you probably have to download the first few bytes of the file and check for magic numbers or something https://github.com/mscdex/mmmagic ( https://github.com/mscdex/mmmagic )

sync-by-unito[bot] commented 2 years ago

➤ Benjamin Chrobot commented:

mmmagic is a binding of native libmagic which complicates development and deployment.

https://github.com/npcz/magic ( https://github.com/npcz/magic ) is a web assembly port of libmagic and may be a better fit

bchrobot commented 2 years ago

A larger refactor of script composition might make sense. Other issues include:

hiemanshu commented 1 year ago
Screenshot 2022-10-05 at 1 49 16 PM

Spent a bit of time trying to figure out what's happening and why axios won't show the request header. Turns out it's not a content type problem, but a CORS configuration issue on the S3 bucket. The solution to work around this would be to add a GraphQL query to run this on the server. Thoughts on that @bchrobot ?

bchrobot commented 1 year ago

The solution to work around this would be to add a GraphQL query to run this on the server. Thoughts on that @bchrobot ?

That sounds great to me!

bchrobot commented 1 year ago

I also wonder whether now would be a good time to move media attachments from [] in the script to a dedicated field. That would help with accurate character counts and simplify validation.

We already have a lot going on visually in the script editor so maybe the field itself is hidden by default and there's an "Add media attachment" button to show the field.

hiemanshu commented 1 year ago

I also wonder whether now would be a good time to move media attachments from [] in the script to a dedicated field. That would help with accurate character counts and simplify validation.

We already have a lot going on visually in the script editor so maybe the field itself is hidden by default and there's an "Add media attachment" button to show the field.

I think we should create a new issue, with a design doc going with @politics-rewired/organizing for this. We have multiple script side refactors we want to do, and doing it in a design doc together is my suggested approach.

ninabaldwin commented 1 year ago

@hiemanshu that sounds good. You can create another issue and we can do a design doc for that!