diff --git a/backend/app/api/routes/collections.py b/backend/app/api/routes/collections.py index fcfeeea53..828db79e5 100644 --- a/backend/app/api/routes/collections.py +++ b/backend/app/api/routes/collections.py @@ -249,6 +249,13 @@ def collection_info( include_url: bool = Query( False, description="Include a signed URL to access the document" ), + download: bool = Query( + False, + description=( + "When true the signed URL forces a download under the document's " + "original filename; when false it opens inline in the browser" + ), + ), limit: int | None = Query( None, @@ -275,7 +282,10 @@ def collection_info( ) collection_with_docs.documents = build_document_schemas( - documents=documents, storage=storage, include_url=include_url + documents=documents, + storage=storage, + include_url=include_url, + download=download, ) return APIResponse[CollectionWithDocsPublic].success_response(collection_with_docs) diff --git a/backend/app/api/routes/doc_transformation_job.py b/backend/app/api/routes/doc_transformation_job.py index 0522b47a9..367a2745e 100644 --- a/backend/app/api/routes/doc_transformation_job.py +++ b/backend/app/api/routes/doc_transformation_job.py @@ -32,6 +32,13 @@ def get_transformation_job( include_url: bool = Query( False, description="Include a signed URL for the transformed document" ), + download: bool = Query( + False, + description=( + "When true the signed URL forces a download under the document's " + "original filename; when false it opens inline in the browser" + ), + ), ): job_crud = DocTransformationJobCrud(session, current_user.project_.id) doc_crud = DocumentCrud(session, current_user.project_.id) @@ -48,6 +55,7 @@ def get_transformation_job( doc_crud=doc_crud, include_url=include_url, storage=storage, + download=download, ) return APIResponse.success_response(job_schema) @@ -70,6 +78,13 @@ def get_multiple_transformation_jobs( include_url: bool = Query( False, description="Include a signed URL for each transformed document" ), + download: bool = Query( + False, + description=( + "When true the signed URL forces a download under the document's " + "original filename; when false it opens inline in the browser" + ), + ), ): job_crud = DocTransformationJobCrud(session, project_id=current_user.project_.id) doc_crud = DocumentCrud(session, project_id=current_user.project_.id) @@ -89,6 +104,7 @@ def get_multiple_transformation_jobs( doc_crud=doc_crud, include_url=include_url, storage=storage, + download=download, ) return APIResponse.success_response( diff --git a/backend/app/api/routes/documents.py b/backend/app/api/routes/documents.py index f2e3f2d6b..43e5e378c 100644 --- a/backend/app/api/routes/documents.py +++ b/backend/app/api/routes/documents.py @@ -82,6 +82,13 @@ def list_docs( include_url: bool = Query( False, description="Include a signed URL to access each document" ), + download: bool = Query( + False, + description=( + "When true the signed URL forces a download under the document's " + "original filename; when false it opens inline in the browser" + ), + ), ) -> APIResponse[list[Union[DocumentPublic, TransformedDocumentPublic]]]: crud = DocumentCrud(session, current_user.project_.id) documents, has_more = crud.read_many(skip, limit) @@ -96,6 +103,7 @@ def list_docs( documents=documents, include_url=include_url, storage=storage, + download=download, ) return APIResponse[ list[Union[DocumentPublic, TransformedDocumentPublic]] @@ -268,6 +276,13 @@ def doc_info( include_url: bool = Query( False, description="Include a signed URL to access the document" ), + download: bool = Query( + False, + description=( + "When true the signed URL forces a download under the document's " + "original filename; when false it opens inline in the browser" + ), + ), ) -> APIResponse[Union[DocumentPublic, TransformedDocumentPublic]]: crud = DocumentCrud(session, current_user.project_.id) document = crud.read_one(doc_id) @@ -282,6 +297,7 @@ def doc_info( document=document, include_url=include_url, storage=storage, + download=download, ) return APIResponse[ diff --git a/backend/app/core/cloud/storage.py b/backend/app/core/cloud/storage.py index a0b451edd..a65d43fc5 100644 --- a/backend/app/core/cloud/storage.py +++ b/backend/app/core/cloud/storage.py @@ -7,7 +7,7 @@ import functools as ft from pathlib import Path from dataclasses import dataclass, asdict -from urllib.parse import ParseResult, urlparse, urlunparse +from urllib.parse import ParseResult, quote, urlparse, urlunparse from abc import ABC, abstractmethod from typing import Any @@ -32,6 +32,18 @@ def _mask(value: str | None) -> str: logger = logging.getLogger(__name__) +def _attachment_disposition(filename: str) -> str: + """ + Build a Content-Disposition header that forces a download. + """ + ascii_fallback = ( + filename.encode("ascii", "replace").decode("ascii").replace('"', "") + ) + return ( + f"attachment; filename=\"{ascii_fallback}\"; filename*=UTF-8''{quote(filename)}" + ) + + class CloudStorageError(Exception): pass @@ -151,7 +163,9 @@ def get_file_size_kb(self, url: str) -> float: pass @abstractmethod - def get_signed_url(self, url: str, expires_in: int = 3600) -> str: + def get_signed_url( + self, url: str, expires_in: int = 3600, filename: str | None = None + ) -> str: """Generate a signed URL with an optional expiry""" pass @@ -255,11 +269,14 @@ def get_file_size_kb(self, url: str) -> float: # Maximum allowed expiry for signed URLs (24 hours) MAX_SIGNED_URL_EXPIRY = 86400 - def get_signed_url(self, url: str, expires_in: int = 3600) -> str: + def get_signed_url( + self, url: str, expires_in: int = 3600, filename: str | None = None + ) -> str: """ Generate a signed S3 URL for the given file. :param url: S3 url (e.g., s3://bucket/key) :param expires_in: Expiry time in seconds (default: 1 hour, max: 24 hours) + :param filename: When set, the URL forces a download under this name :return: Signed URL as string """ # Cap expiry at maximum allowed value to prevent excessively long-lived URLs @@ -267,9 +284,12 @@ def get_signed_url(self, url: str, expires_in: int = 3600) -> str: name = SimpleStorageName.from_url(url) try: + params: dict[str, str] = {"Bucket": name.Bucket, "Key": name.Key} + if filename: + params["ResponseContentDisposition"] = _attachment_disposition(filename) signed_url = self.aws.client.generate_presigned_url( "get_object", - Params={"Bucket": name.Bucket, "Key": name.Key}, + Params=params, ExpiresIn=expires_in, ) logger.info( diff --git a/backend/app/services/documents/helpers.py b/backend/app/services/documents/helpers.py index ffbb1096b..6b18f1af4 100644 --- a/backend/app/services/documents/helpers.py +++ b/backend/app/services/documents/helpers.py @@ -179,15 +179,26 @@ def _to_public_schema(doc: Document) -> PublicDoc: return TransformedDocumentPublic.model_validate(doc, from_attributes=True) +def _signed_url(document: Document, storage: CloudStorage, download: bool) -> str: + """ + One URL, two behaviours: with ``download`` the link carries a + Content-Disposition that forces a save under the original filename, without + it the link is bare so browsers render it inline (iframe previews). + """ + filename = document.fname if download else None + return storage.get_signed_url(document.object_store_url, filename=filename) + + def build_document_schema( *, document: Document, include_url: bool, storage: CloudStorage | None, + download: bool = False, ) -> PublicDoc: schema = _to_public_schema(document) if include_url and storage: - schema.signed_url = storage.get_signed_url(document.object_store_url) + schema.signed_url = _signed_url(document, storage, download) return schema @@ -196,12 +207,13 @@ def build_document_schemas( documents: Iterable[Document], include_url: bool, storage: CloudStorage | None, + download: bool = False, ) -> list[PublicDoc]: out: list[PublicDoc] = [] for doc in documents: schema = _to_public_schema(doc) if include_url and storage: - schema.signed_url = storage.get_signed_url(doc.object_store_url) + schema.signed_url = _signed_url(doc, storage, download) out.append(schema) return out @@ -212,6 +224,7 @@ def build_job_schema( doc_crud: DocumentCrud, include_url: bool, storage: CloudStorage | None, + download: bool = False, ) -> DocTransformationJobPublic: """Build a single job schema, optionally attaching a signed URL.""" transformed_doc_schema: TransformedDocumentPublic | None = None @@ -225,7 +238,7 @@ def build_job_schema( object_url = doc.object_store_url if include_url and storage and object_url: - transformed_doc_schema.signed_url = storage.get_signed_url(object_url) + transformed_doc_schema.signed_url = _signed_url(doc, storage, download) job_schema = DocTransformationJobPublic.model_validate(job, from_attributes=True) return job_schema.model_copy( @@ -239,6 +252,7 @@ def build_job_schemas( doc_crud: DocumentCrud, include_url: bool, storage: CloudStorage | None, + download: bool = False, ) -> list[DocTransformationJobPublic]: """Build many job schemas efficiently.""" out: list[DocTransformationJobPublic] = [] @@ -249,6 +263,7 @@ def build_job_schemas( doc_crud=doc_crud, include_url=include_url, storage=storage, + download=download, ) ) return out diff --git a/backend/app/tests/api/routes/documents/test_route_document_info.py b/backend/app/tests/api/routes/documents/test_route_document_info.py index 26198348f..e8a1d5a45 100644 --- a/backend/app/tests/api/routes/documents/test_route_document_info.py +++ b/backend/app/tests/api/routes/documents/test_route_document_info.py @@ -1,6 +1,9 @@ +from unittest.mock import patch + import pytest from sqlmodel import Session +from app.models import Document from app.tests.utils.document import ( DocumentComparator, DocumentMaker, @@ -50,3 +53,41 @@ def test_cannot_info_unknown_document( response = crawler.get(route.append(next(maker))) assert response.is_error + + +class TestDocumentRouteInfoSignedUrl: + """The `download` flag decides whether the signed URL forces a save.""" + + @staticmethod + def _sign_and_capture( + db: Session, route: Route, crawler: WebCrawler + ) -> tuple[Document, list[str | None]]: + store = DocumentStore(db=db, project_id=crawler.user_api_key.project_id) + document = store.put() + with patch("app.api.routes.documents.get_cloud_storage") as mock_storage: + mock_storage.return_value.get_signed_url.return_value = "https://signed" + response = crawler.get(route.append(document)) + assert response.is_success + filenames = [ + call.kwargs.get("filename") + for call in mock_storage.return_value.get_signed_url.call_args_list + ] + return document, filenames + + def test_download_true_signs_with_filename( + self, db: Session, crawler: WebCrawler + ) -> None: + route = Route("", include_url="true", download="true") + + document, filenames = self._sign_and_capture(db, route, crawler) + + assert filenames == [document.fname] + + def test_download_omitted_signs_without_filename( + self, db: Session, crawler: WebCrawler + ) -> None: + route = Route("", include_url="true") + + _, filenames = self._sign_and_capture(db, route, crawler) + + assert filenames == [None] diff --git a/backend/app/tests/core/cloud/test_storage.py b/backend/app/tests/core/cloud/test_storage.py index 95e9ff7ff..e7ffb8cab 100644 --- a/backend/app/tests/core/cloud/test_storage.py +++ b/backend/app/tests/core/cloud/test_storage.py @@ -1,8 +1,14 @@ """Tests for app.core.cloud.storage helpers.""" -from unittest.mock import patch +from types import SimpleNamespace +from unittest.mock import MagicMock, patch +from uuid import uuid4 -from app.core.cloud.storage import GCS_SCOPES, build_gcp_sa_credentials +from app.core.cloud.storage import ( + GCS_SCOPES, + AmazonCloudStorage, + build_gcp_sa_credentials, +) def test_build_gcp_sa_credentials_passes_key_and_scopes(): @@ -14,3 +20,47 @@ def test_build_gcp_sa_credentials_passes_key_and_scopes(): mock_from_info.assert_called_once_with(sa_key, scopes=list(GCS_SCOPES)) assert creds is mock_from_info.return_value + + +def _amazon_storage_with_mock_client(mock_client): + storage = AmazonCloudStorage(project_id=1, storage_path=uuid4()) + storage.aws = SimpleNamespace(client=mock_client) + return storage + + +def test_get_signed_url_without_filename_omits_content_disposition(): + mock_client = MagicMock() + storage = _amazon_storage_with_mock_client(mock_client) + + storage.get_signed_url("s3://bucket/key.pdf") + + params = mock_client.generate_presigned_url.call_args.kwargs["Params"] + assert "ResponseContentDisposition" not in params + + +def test_get_signed_url_with_filename_forces_attachment(): + mock_client = MagicMock() + storage = _amazon_storage_with_mock_client(mock_client) + + storage.get_signed_url("s3://bucket/key.pdf", filename="report.pdf") + + disposition = mock_client.generate_presigned_url.call_args.kwargs["Params"][ + "ResponseContentDisposition" + ] + assert ( + disposition + == "attachment; filename=\"report.pdf\"; filename*=UTF-8''report.pdf" + ) + + +def test_get_signed_url_encodes_non_ascii_filename(): + mock_client = MagicMock() + storage = _amazon_storage_with_mock_client(mock_client) + + storage.get_signed_url("s3://bucket/key.pdf", filename="रिपोर्ट.pdf") + + disposition = mock_client.generate_presigned_url.call_args.kwargs["Params"][ + "ResponseContentDisposition" + ] + assert disposition.startswith('attachment; filename="') + assert "filename*=UTF-8''%E0%A4%B0" in disposition diff --git a/backend/app/tests/services/documents/test_helpers.py b/backend/app/tests/services/documents/test_helpers.py index 3da9ba30c..0e5f9d766 100644 --- a/backend/app/tests/services/documents/test_helpers.py +++ b/backend/app/tests/services/documents/test_helpers.py @@ -1,10 +1,14 @@ import csv from io import BytesIO +from uuid import uuid4 import pytest from fastapi import HTTPException, UploadFile +from app.models import Document from app.services.documents.helpers import ( + build_document_schema, + build_document_schemas, calculate_file_size, validate_upload, ) @@ -338,3 +342,77 @@ def test_missing_filename_raises_400(self) -> None: validate_upload(src=src, target_format=None, transformer=None) assert exc_info.value.status_code == 400 assert exc_info.value.detail == "Uploaded file has no filename" + + +def make_document(fname: str = "policy.pdf") -> Document: + return Document( + id=uuid4(), + fname=fname, + object_store_url="s3://bucket/key", + project_id=1, + ) + + +class FakeStorage: + """Records how each signed URL was requested.""" + + def __init__(self) -> None: + self.calls: list[dict] = [] + + def get_signed_url( + self, url: str, expires_in: int = 3600, filename: str | None = None + ) -> str: + self.calls.append({"url": url, "filename": filename}) + return f"https://signed/{'download' if filename else 'preview'}" + + +class TestDocumentUrls: + def test_download_true_signs_with_filename(self) -> None: + storage = FakeStorage() + + schema = build_document_schema( + document=make_document(), + include_url=True, + storage=storage, + download=True, + ) + + assert schema.signed_url == "https://signed/download" + assert [c["filename"] for c in storage.calls] == ["policy.pdf"] + + def test_download_defaults_to_false_and_signs_without_filename(self) -> None: + storage = FakeStorage() + + schema = build_document_schema( + document=make_document(), include_url=True, storage=storage + ) + + assert schema.signed_url == "https://signed/preview" + assert [c["filename"] for c in storage.calls] == [None] + + def test_no_url_when_include_url_is_false(self) -> None: + storage = FakeStorage() + + schema = build_document_schema( + document=make_document(), + include_url=False, + storage=storage, + download=True, + ) + + assert schema.signed_url is None + assert storage.calls == [] + + def test_list_applies_download_flag_to_every_document(self) -> None: + storage = FakeStorage() + documents = [make_document("a.pdf"), make_document("b.pdf")] + + schemas = build_document_schemas( + documents=documents, + include_url=True, + storage=storage, + download=True, + ) + + assert all(s.signed_url == "https://signed/download" for s in schemas) + assert [c["filename"] for c in storage.calls] == ["a.pdf", "b.pdf"] diff --git a/docs/wiki/modules/knowledge-base.md b/docs/wiki/modules/knowledge-base.md index c6b8462df..4cefd5719 100644 --- a/docs/wiki/modules/knowledge-base.md +++ b/docs/wiki/modules/knowledge-base.md @@ -33,6 +33,7 @@ All paths relative to `backend/app/`. - OpenAI vector stores / file uploads, object storage (`core/cloud/storage.py`), Zerox/OCR for transforms. ## Gotchas +- `signed_url` behaviour is chosen per request by the `download` query param (default false = inline, as before). `put()` stores the upload's `Content-Type`, so a bare presigned URL for a PDF opens in a tab (CSV/XLSX datasets happen to download, which is why only KB docs looked broken). `get_signed_url(..., filename=...)` adds `ResponseContentDisposition: attachment`; `_signed_url()` in `services/documents/helpers.py` passes `fname` only when `download` is set, and the flag threads through `build_document_schema(s)` / `build_job_schema(s)` from `api/routes/documents.py`, `doc_transformation_job.py` and `collections.py`. An attachment URL will not render in an `