search

petersolopov / carbonara

API for carbon
MIT License
125 stars 32 forks source link

Allow to upload custom font #23

Closed superruzafa closed 2 years ago

superruzafa commented 2 years ago

Hi.

With this PR the /api/cook endpoint also accepts multipart/form-data which allows to upload custom fonts for rendering code snippets.

petersolopov commented 2 years ago

Hi @superruzafa! This is an interesting feature, but I see some potential issues:

  1. Writing font files to the host machine file system. It is the default behavior of formidable. Sooner or later the space will end.
  2. Downloading potentially dangerous files. A customer of API can upload whatever he wants.

Because of the above, this PR cannot be merged in such an implementation

superruzafa commented 2 years ago

Ok, thanks for the explanation.

What about to limit the size of the contents the user can upload? For fonts let's say... 300 Kb?

And regarding the disk space, what about to delete the uploaded file once its contents have been loaded into the Carbon page?

petersolopov commented 2 years ago

It's still about uploading some files on the server. And files can be anything.

I think we can pass a link to a font in the body payload. Or even base64 encoded font.

superruzafa commented 2 years ago

I reverted the file upload thing and allowed the user to specify the font encoded in base64.

petersolopov commented 2 years ago

Great! Should we use multipart and formidable now? We can pass base64 in JSON.

Could you please make rebase for tests passing and add a test case for custom font?

superruzafa commented 2 years ago

Using multipart/form-data is just to have an easier way (for me) to construct the curl command in CLI without mess with the JSON string, but if you prefert to get rid of it please let me know and I'll remove it from the PR.

Additionally let me add a test case and fix the test.

petersolopov commented 2 years ago

Ok, good point about curl. I think multipart can be kept, but it requires some tests too.

superruzafa commented 2 years ago

@petersolopov, can you please review this PR?

I didn't notice there was a Dockerfile in the project to run the tests so I was using a custom one, now all the tests pass.

petersolopov commented 2 years ago

I didn't notice there was a Dockerfile in the project to run the tests so I was using a custom one, now all the tests pass.

There is Dockerfile 🙂

petersolopov commented 2 years ago

Merged. @superruzafa thanks a lot!