Skip to content

fix(api): stop stored SVG/XML files from executing as documents - #39834

Open
kah-ja wants to merge 1 commit into
langgenius:mainfrom
kah-ja:fix/inert-svg-file-responses
Open

fix(api): stop stored SVG/XML files from executing as documents#39834
kah-ja wants to merge 1 commit into
langgenius:mainfrom
kah-ja:fix/inert-svg-file-responses

Conversation

@kah-ja

@kah-ja kah-ja commented Jul 30, 2026

Copy link
Copy Markdown

Summary

Several endpoints stream a stored file back to the browser. Two of them — the deprecated GET /files/<file_id>/image-preview and the public GET /files/workspaces/<workspace_id>/webapp-logo — return the stored bytes with the uploader's own Content-Type and no download-forcing headers at all (api/controllers/files/image_preview.py:71,181):

return Response(generator, mimetype=mimetype)

svg is an accepted image extension (api/constants/__init__.py:10), uploads are not sanitised, and UPLOAD_FILE_EXTENSION_BLACKLIST is empty by default, so those bytes can be an SVG document that carries <script>. Chunk attachments and RAG retrieval results are still signed against image-preview (api/models/dataset.py:1106,1250, api/core/rag/retrieval/dataset_retrieval.py:546, api/core/rag/datasource/retrieval_service.py:935,963, api/core/tools/signature.py:40), so that URL is what the API hands to whoever views the chunk — and opening it renders the file as a document in the app origin, where csrf_token is readable from document.cookie and the session cookies are same-site. A member with dataset-edit rights (editor / dataset_operator) can therefore get script running as any user who views the chunk.

enforce_download_for_html already exists for exactly this problem, but its type set is HTML-only (api/controllers/common/file_response.py:7-8), so SVG and XML fall through — and image-preview / webapp-logo never call it. The provider and plugin icon endpoints (api/controllers/console/workspace/{model_providers,tool_providers,plugin}.py) serve plugin-controlled bytes the same way. file-preview is not affected: it already forces application/octet-stream.

Fix

One helper, used by every endpoint that streams a stored file:

  • X-Content-Type-Options: nosniff is always sent.
  • HTML keeps today's behaviour: attachment + application/octet-stream.
  • SVG/XML keep their Content-Type, so <img src="..."> thumbnails, provider icons and webapp logos render exactly as before, but they are marked Content-Disposition: attachment and get an inert Content-Security-Policy (default-src 'none'; style-src 'unsafe-inline'; sandbox). Both headers are ignored for <img> subresource loads, and each one independently prevents a top-level navigation from executing the file.

enforce_download_for_html is left untouched and still covers HTML only; the new harden_served_file composes it, so the four existing call sites keep their behaviour and additionally gain SVG/XML and nosniff coverage.

If you would rather not send Content-Disposition: attachment for SVG, dropping that single line still closes the hole via the CSP alone.

This was reported privately as GHSA-6w5x-w2p4-7837 (2026-05-13, draft still open), which is why there is no public issue to link here. Happy to move the discussion there or to split the icon endpoints into a separate PR if you prefer.

Screenshots

Measured on 1.16.1 with the stock docker/.env.example and a real Chromium, serving an uploaded SVG through image-preview:

Before After
Content-Type image/svg+xml; charset=utf-8 image/svg+xml; charset=utf-8
Content-Disposition (absent) attachment
X-Content-Type-Options (absent) nosniff
Content-Security-Policy (absent) default-src 'none'; style-src 'unsafe-inline'; sandbox
<img src="..."> renders, 320x90 renders, 320x90 (load, unchanged)
top-level navigation rendered as a document in the app origin, the script in it ran browser downloads image-preview.svg, nothing runs

Checklist

  • This change requires a documentation update, included: Dify Document
  • I understand that this PR may be closed in case there was no previous discussion or issues. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.
  • I ran make lint && make type-check (backend) and cd web && pnpm exec vp staged (frontend) to appease the lint gods

Backend gates, run with the versions pinned in api/uv.lock:

step tool result
ruff format --check ./api ruff 0.15.12 3199 files already formatted
ruff check ./api ruff 0.15.12 All checks passed
api-contract-lint api/dev/lint_response_contracts.py 472 valid, 0 mismatch. The two refactorable hits are pre-existing and in files this PR does not touch. Worth checking here because this PR rewrites three return send_file(...) statements that carry # response-contract:ignore comments.
lint-imports import-linter 2.11 1241 files, 9486 dependencies, 0 broken
dotenv-linter dotenv-linter clean
mypy --check-untyped-defs --disable-error-code=import-untyped mypy 1.20.2 no error added by this PR — A/B'd against the unpatched files, both runs produce the identical 11 pre-existing errors, the only delta being one line number shifting 976 -> 979
pyrefly check pyrefly 1.0.0 no finding attributable to this diff
pytest on the two touched test modules pytest 9 29 passed (9 new)

Caveat on how the type checks were run: I have no local Python toolchain, so mypy and pyrefly ran inside a langgenius/dify-api:1.16.1 container and were scoped to the eight changed source files rather than the whole tree (the type-check target excludes tests/ anyway), with the A/B baseline described above. ruff, the contract lint and lint-imports ran over the whole api/ tree. There is no frontend change, so pnpm exec vp staged has nothing staged to look at.

From Claude Code

@dosubot dosubot Bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jul 30, 2026
@kah-ja kah-ja closed this Jul 30, 2026
@kah-ja kah-ja reopened this Jul 30, 2026
Every endpoint that streams a stored file back to the browser now runs
through one helper:

- `X-Content-Type-Options: nosniff` is always sent.
- HTML keeps the existing behaviour (attachment + application/octet-stream).
- SVG/XML keep their Content-Type, so `<img src="...">` still renders them,
  but are marked `Content-Disposition: attachment` and get an inert
  `Content-Security-Policy`, so a top-level navigation can no longer run
  script they carry.

The deprecated `GET /files/<id>/image-preview` and the public
`GET /files/workspaces/<id>/webapp-logo` returned the stored bytes with the
uploader's own Content-Type and no download-forcing headers at all, so an
`image/svg+xml` file uploaded by a member with dataset-edit rights executed
in the app origin as soon as another user opened it — chunk attachments are
still signed against `image-preview` (models/dataset.py, dataset_retrieval,
retrieval_service), so the URL is handed out by the API. The provider/plugin
icon endpoints served plugin-controlled bytes the same way.

`enforce_download_for_html` is unchanged and still covers HTML only; the new
`harden_served_file` composes it, so existing callers keep their behaviour
and additionally gain the SVG/XML and nosniff handling.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@kah-ja
kah-ja force-pushed the fix/inert-svg-file-responses branch from 39bb358 to d5ac684 Compare July 30, 2026 19:04
@github-actions

Copy link
Copy Markdown
Contributor

Pyrefly Type Coverage

Metric Base PR Delta
Type coverage 56.84% 56.84% -0.00%
Strict coverage 56.37% 56.36% -0.00%
Typed symbols 37,153 37,164 +11
Untyped symbols 28,447 28,460 +13
Modules 3092 3092 0

@github-actions

Copy link
Copy Markdown
Contributor

Pyrefly Diff

base → PR
--- /tmp/pyrefly_base.txt	2026-07-30 20:29:37.102718170 +0000
+++ /tmp/pyrefly_pr.txt	2026-07-30 20:29:20.463587533 +0000
@@ -2121,17 +2121,19 @@
 ERROR Argument `list[FromClause]` is not assignable to parameter `tables` with type `Sequence[Table] | None` in function `sqlalchemy.sql.schema.MetaData.create_all` [bad-argument-type]
   --> tests/unit_tests/controllers/console/workspace/test_workspace.py:45:54
 ERROR `SimpleNamespace` is not assignable to attribute `db` with type `SQLAlchemy` [bad-assignment]
-  --> tests/unit_tests/controllers/files/test_image_preview.py:18:17
+  --> tests/unit_tests/controllers/files/test_image_preview.py:19:17
 ERROR `SimpleNamespace` is not assignable to attribute `request` with type `Request` [bad-assignment]
-  --> tests/unit_tests/controllers/files/test_image_preview.py:37:26
+  --> tests/unit_tests/controllers/files/test_image_preview.py:38:26
 ERROR `SimpleNamespace` is not assignable to attribute `request` with type `Request` [bad-assignment]
-  --> tests/unit_tests/controllers/files/test_image_preview.py:60:26
+  --> tests/unit_tests/controllers/files/test_image_preview.py:63:26
 ERROR `SimpleNamespace` is not assignable to attribute `request` with type `Request` [bad-assignment]
-  --> tests/unit_tests/controllers/files/test_image_preview.py:83:26
+  --> tests/unit_tests/controllers/files/test_image_preview.py:81:26
 ERROR `SimpleNamespace` is not assignable to attribute `request` with type `Request` [bad-assignment]
-   --> tests/unit_tests/controllers/files/test_image_preview.py:113:26
+   --> tests/unit_tests/controllers/files/test_image_preview.py:104:26
 ERROR `SimpleNamespace` is not assignable to attribute `request` with type `Request` [bad-assignment]
-   --> tests/unit_tests/controllers/files/test_image_preview.py:145:26
+   --> tests/unit_tests/controllers/files/test_image_preview.py:134:26
+ERROR `SimpleNamespace` is not assignable to attribute `request` with type `Request` [bad-assignment]
+   --> tests/unit_tests/controllers/files/test_image_preview.py:166:26
 ERROR No attribute `global_db` in module `controllers.files.tool_files` [missing-attribute]
   --> tests/unit_tests/controllers/files/test_tool_files.py:25:5
 ERROR `SimpleNamespace` is not assignable to attribute `request` with type `Request` [bad-assignment]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant