improve security + handle image preview

windmill-labs · Jan 3, 2025 · 60efe2d · 60efe2d
1 parent a13527d
commit 60efe2d
Show file tree

Hide file tree

Showing 3 changed files with 114 additions and 21 deletions.
diff --git a/backend/.sqlx/query-df0454f75e819d7d3d03fef0a7d5606940a089f6179e5443090afa9aba7c5b24.json b/backend/.sqlx/query-df0454f75e819d7d3d03fef0a7d5606940a089f6179e5443090afa9aba7c5b24.json
diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs
@@ -10,7 +10,6 @@ use std::{collections::HashMap, sync::Arc};
 
 use crate::{
     db::{ApiAuthed, DB},
-    job_helpers_ee::{download_s3_file_internal, DownloadFileQuery},
     resources::get_resource_value_interpolated_internal,
     users::{require_owner_of_path, OptAuthed},
     utils::WithStarredInfoQuery,
@@ -21,14 +20,17 @@ use crate::{
 #[cfg(feature = "parquet")]
 use crate::{
     job_helpers_ee::{
-        get_random_file_name, get_s3_resource, get_workspace_s3_resource, upload_file_from_req,
-        UploadFileResponse,
+        download_s3_file_internal, get_random_file_name, get_s3_resource,
+        get_workspace_s3_resource, load_image_preview_internal, upload_file_from_req,
+        DownloadFileQuery, LoadImagePreviewQuery, UploadFileResponse,
     },
     users::fetch_api_authed_from_permissioned_as,
 };
+#[cfg(feature = "parquet")]
+use axum::response::Response;
 use axum::{
     extract::{Extension, Json, Path, Query},
-    response::{IntoResponse, Response},
+    response::IntoResponse,
     routing::{delete, get, post},
     Router,
 };
@@ -94,6 +96,10 @@ pub fn unauthed_service() -> Router {
         .route("/execute_component/*path", post(execute_component))
         .route("/upload_s3_file/*path", post(upload_s3_file_from_app))
         .route("/download_s3_file/*path", get(download_s3_file_from_app))
+        .route(
+            "/load_image_preview/*path",
+            get(load_s3_file_image_preview_from_app),
+        )
         .route("/public_app/:secret", get(get_public_app_by_secret))
         .route("/public_resource/*path", get(get_public_resource))
 }
@@ -1688,20 +1694,18 @@ async fn download_s3_file_from_app() -> Result<()> {
 }
 
 #[cfg(feature = "parquet")]
-async fn download_s3_file_from_app(
-    OptAuthed(opt_authed): OptAuthed,
-    Extension(db): Extension<DB>,
-    Path((w_id, path)): Path<(String, StripPath)>,
-    Query(query): Query<DownloadFileQuery>,
-) -> Result<Response> {
-    let path = path.to_path();
-
+async fn get_on_behalf_authed_from_app(
+    db: &DB,
+    path: &str,
+    w_id: &str,
+    opt_authed: &Option<ApiAuthed>,
+) -> Result<ApiAuthed> {
     let policy_o = sqlx::query_scalar!(
         "SELECT policy from app WHERE path = $1 AND workspace_id = $2",
         path,
         w_id
     )
-    .fetch_optional(&db)
+    .fetch_optional(db)
     .await?;
 
     let policy = policy_o
@@ -1723,22 +1727,87 @@ async fn download_s3_file_from_app(
         fetch_api_authed_from_permissioned_as(permissioned_as, email, &w_id, &db, Some(username))
             .await?;
 
-    let allowed = sqlx::query_scalar!(
-            r#"SELECT EXISTS (SELECT 1 FROM completed_job WHERE result @> ('{"s3":"' || $1 ||  '"}')::jsonb AND workspace_id = $2 AND script_path LIKE $3 || '/%' AND (job_kind = 'appscript' OR job_kind = 'preview'))"#,
-            query.file_key,
+    Ok(on_behalf_authed)
+}
+
+#[cfg(feature = "parquet")]
+async fn check_if_allowed_to_access_s3_file_from_app(
+    db: &DB,
+    opt_authed: &Option<ApiAuthed>,
+    file_key: &str,
+    w_id: &str,
+    path: &str,
+) -> Result<()> {
+    // if anonymous, check that the file was the result of an app script ran by an anonymous user in the last 3 hours
+    // otherwise, if logged in, allow any file (TODO: change that when we implement better s3 policy)
+
+    let allowed = opt_authed.is_some()
+        || sqlx::query_scalar!(
+            r#"SELECT EXISTS (
+                SELECT 1 FROM completed_job 
+                WHERE workspace_id = $2 
+                    AND (job_kind = 'appscript' OR job_kind = 'preview')
+                    AND created_by = 'anonymous' 
+                    AND started_at > now() - interval '3 hours'
+                    AND script_path LIKE $3 || '/%' 
+                    AND result @> ('{"s3":"' || $1 ||  '"}')::jsonb 
+            )"#,
+            file_key,
             w_id,
             path,
-        ).fetch_one(&db)
+        )
+        .fetch_one(db)
         .await?
         .unwrap_or(false);
 
     if !allowed {
-        return Err(Error::BadRequest("File restricted".to_string()));
+        Err(Error::BadRequest("File restricted".to_string()))
+    } else {
+        Ok(())
     }
+}
+
+#[cfg(feature = "parquet")]
+async fn download_s3_file_from_app(
+    OptAuthed(opt_authed): OptAuthed,
+    Extension(db): Extension<DB>,
+    Path((w_id, path)): Path<(String, StripPath)>,
+    Query(query): Query<DownloadFileQuery>,
+) -> Result<Response> {
+    let path = path.to_path();
+
+    let on_behalf_authed = get_on_behalf_authed_from_app(&db, &path, &w_id, &opt_authed).await?;
+
+    check_if_allowed_to_access_s3_file_from_app(&db, &opt_authed, &query.file_key, &w_id, &path)
+        .await?;
 
     download_s3_file_internal(on_behalf_authed, &db, None, "", &w_id, query).await
 }
 
+#[cfg(not(feature = "parquet"))]
+async fn download_s3_file_from_app() -> Result<()> {
+    return Err(Error::BadRequest(
+        "This endpoint requires the parquet feature to be enabled".to_string(),
+    ));
+}
+
+#[cfg(feature = "parquet")]
+async fn load_s3_file_image_preview_from_app(
+    OptAuthed(opt_authed): OptAuthed,
+    Extension(db): Extension<DB>,
+    Path((w_id, path)): Path<(String, StripPath)>,
+    Query(query): Query<LoadImagePreviewQuery>,
+) -> Result<Response> {
+    let path = path.to_path();
+
+    let on_behalf_authed = get_on_behalf_authed_from_app(&db, &path, &w_id, &opt_authed).await?;
+
+    check_if_allowed_to_access_s3_file_from_app(&db, &opt_authed, &query.file_key, &w_id, &path)
+        .await?;
+
+    load_image_preview_internal(on_behalf_authed, &db, "", &w_id, query).await
+}
+
 fn get_on_behalf_of(policy: &Policy) -> Result<(String, String)> {
     let permissioned_as = policy
         .on_behalf_of

diff --git a/frontend/src/lib/components/DisplayResult.svelte b/frontend/src/lib/components/DisplayResult.svelte
@@ -663,7 +663,7 @@
 							{/if}
 						</div>
 						{#if typeof result?.s3 === 'string'}
-							{#if result?.s3?.endsWith('.parquet') || result?.s3?.endsWith('.csv')}
+							{#if !appPath && (result?.s3?.endsWith('.parquet') || result?.s3?.endsWith('.csv'))}
 								{#key result.s3}
 									<ParqetTableRenderer
 										disable_download={result?.disable_download}
@@ -677,15 +677,15 @@
 									<img
 										alt="preview rendered"
 										class="w-auto h-full"
-										src={`/api/w/${workspaceId}/job_helpers/load_image_preview?file_key=${result.s3}` +
+										src={`/api/w/${workspaceId}/${appPath ? 'apps_u/load_image_preview/' + appPath : 'job_helpers/load_image_preview'}?file_key=${result.s3}` +
 											(result.storage ? `&storage=${result.storage}` : '')}
 									/>
 								</div>
 							{:else if result?.s3?.endsWith('.pdf')}
 								<div class="h-96 mt-2 border">
 									<PdfViewer
 										allowFullscreen
-										source={`/api/w/${workspaceId}/job_helpers/load_image_preview?file_key=${result.s3}` +
+										source={`/api/w/${workspaceId}/${appPath ? 'apps_u/load_image_preview/' + appPath : 'job_helpers/load_image_preview'}?file_key=${result.s3}` +
 											(result.storage ? `&storage=${result.storage}` : '')}
 									/>
 								</div>