Skip to content

🔒️Unsafe attachments #663

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

Merged
merged 9 commits into from
Mar 3, 2025
Merged

🔒️Unsafe attachments #663

merged 9 commits into from
Mar 3, 2025

Conversation

AntoLC
Copy link
Collaborator

@AntoLC AntoLC commented Feb 25, 2025

Purpose

We improve the security of unsafe files.

Proposal

We decided to sanitize markup files.
We will prevent users to open files from a new tab, the files will be directly downloaded.
We alert the users when they are about to download unsafe files with a confirmation modal.
We could implement other security layer in future development inside this confirmation modal, like proposing jecliqueoupas.

  • 🚸(frontend) block click on unsafe image
  • 🛂(frontend) secure download button
  • 🔒️(back) set ContentDisposition on media upload
  • 🔒️(nginx) manage Content-Security-Policy in nginx config
  • 🛂(backend) remove svg from unsafe

Demo

image

@AntoLC AntoLC self-assigned this Feb 25, 2025
@AntoLC AntoLC force-pushed the security/unsafe-attachments branch 4 times, most recently from f6530be to 2c9132b Compare February 28, 2025 14:47
@AntoLC AntoLC marked this pull request as ready for review February 28, 2025 14:52
@AntoLC AntoLC force-pushed the security/unsafe-attachments branch from 2c9132b to b6696fa Compare March 3, 2025 10:18
@AntoLC AntoLC force-pushed the security/unsafe-attachments branch from b6696fa to a5a47cb Compare March 3, 2025 11:56
AntoLC and others added 9 commits March 3, 2025 13:07
The frontend cannot access custom headers of a file,
so we need to add a flag in the filename.
We add the `unsafe` flag in the filename to
indicate that the file is unsafe.

Previous filename: "/{UUID4}.{extension}"
New filename: "/{UUID4}-unsafe.{extension}"
Since the new design implementation,
the alert error was not looking good.
This commit improves the design of the alert error.
It can happen that the user is authenticated
then the token is expired. The authenticated
state should be updated to false in this case.
We want to prevent the user to open unsafe images
in the browser. We blocked the click on the images.
To download them, the user will have to use the
download button.
Blocknote download button opens the file in a new
tab, which could be not secure because of XSS attacks.
We replace the download button with a new one that
downloads the file instead of opening it in a new tab.
Some files are flags as unsafe (SVG / js / exe),
for these files we add a confirmation modal before
downloading the file to prevent the user from
downloading a file that could be harmful.
In the future, we could add other security layers
from this model, to analyze the file before
downloading it by example.
We moved the toolbar components in BlockNoteToolBar
folder.
On the media upload endpoint, we want to set the content-disposition
header. Its value is based on the uploaded file mime-type and if flagged
as unsafe. If the file is not an image or is unsafe then the
contentDisposition is set to attachment to force its download.
Otherwise, we set it to inline.
The media route is managed by nginx. On this route we want to add the
Content-Security-Header to forbid fetching any resources.
See : https://content-security-policy.com/
We added content-security-policy on nginx.
It should be safe to allow svg files now.
We remove the svg file from the unsafe
attachments list. We adapt the tests accordingly.
@AntoLC AntoLC force-pushed the security/unsafe-attachments branch from a5a47cb to 62d5c8d Compare March 3, 2025 12:07
@AntoLC AntoLC merged commit 7b1ddc0 into main Mar 3, 2025
19 of 20 checks passed
@AntoLC AntoLC deleted the security/unsafe-attachments branch March 3, 2025 12:18
@AntoLC AntoLC mentioned this pull request Mar 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

White font for alert messages against light-gray modal background are hard to read
2 participants