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

Enhance external image hosting in agent configuration #4565

Merged
merged 6 commits into from
Apr 4, 2024

Conversation

flvndvd
Copy link
Contributor

@flvndvd flvndvd commented Apr 4, 2024

Description

This PR fixes #4556.

This PR addresses a security issue by improving the way we handle image uploads. It ensures that uploaded files have an image* mimeType, are less than 3MB in size, and restricts the accepted file types in the file picker.

In the agent configuration creation, it verifies that the file is self-hosted on our GCS and has the proper mime type, or it will yield an error.

To make this solution more resilient in the future, this PR starts centralizing the DFS (Distributed File System) logic in one single place.

It also applies some refactoring to the existing logic of content fragment, to use the newly introduced DFS class.

Risk

Worst case it breaks avatar upload on agent configuration or the content fragment (uploading file, downloading file, ...), but it has been well tested locally.

Deploy Plan

@flvndvd flvndvd marked this pull request as ready for review April 4, 2024 11:42
Copy link
Contributor

@spolu spolu left a comment

Choose a reason for hiding this comment

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

LGTM

I thiiiiiink content fragment could be moved as well to your new interface. Maybe worth doing it right now to convince ourselves it's the right one (esp as this PR is not to big)

@@ -0,0 +1,52 @@
import type { Bucket } from "@google-cloud/storage";
Copy link
Contributor

Choose a reason for hiding this comment

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

gcs rather than dfs in name no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I stick with DFS for this file/class but all other occurrences will go with GCS.

@flvndvd
Copy link
Contributor Author

flvndvd commented Apr 4, 2024

LGTM

I thiiiiiink content fragment could be moved as well to your new interface. Maybe worth doing it right now to convince ourselves it's the right one (esp as this PR is not to big)

I was planning to do it in a follow up PR, but let me bake this into this PR.

@spolu
Copy link
Contributor

spolu commented Apr 4, 2024

Up to you 👍

@flvndvd flvndvd self-assigned this Apr 4, 2024
@flvndvd flvndvd merged commit 081d591 into main Apr 4, 2024
5 checks passed
@flvndvd flvndvd deleted the flav/improve-external-image-hosting branch April 4, 2024 15:18
@flvndvd flvndvd added the security Security Issues label Apr 4, 2024
flvndvd added a commit that referenced this pull request May 26, 2024
* Strengthen external image hosting in agent configuration

* ✂️

* ✂️

* ✂️

* Apply new DFS logic to content fragment + refactor

* Accept static hosted Dust Avatars
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Security Issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unauthorized Use of External Image Hosting
2 participants