mirror of
https://github.com/tiennm99/DocsGPT.git
synced 2026-10-15 18:15:33 +00:00
fix(attachments): close the .txt gap in the parseability gate
Review follow-ups on the attachment gate. .txt was listed as parser-backed, but it has no parser — it *is* the plain-text fallthrough. That let it skip the content check, so renaming a video to notes.txt walked straight back into the bug the gate exists for (verified: 5132 chars of binary "extracted" and stored). The list is now exactly the file extractor's keys, .txt included in the content check like any other unparsed suffix, and the drift test asserts equality rather than containment. The sniff now recognises a UTF-16/32 BOM as text, so a Notepad "Unicode" .txt is not caught by the NUL-byte rule. The picker's accept filter listed parser-backed suffixes only, hiding .txt, .py and .log — files the gate reads happily — behind "All files". It now carries text/* as well, so it can never be narrower than what the upload accepts. A rejected batch carries one errors entry per file, but the non-200 branch applied the top-level message to every chip, so two files failing for two reasons both reported the first one. Reasons are now matched by upload_index, with the top-level message as fallback. _get_store_attachment_user_error no longer reads str(exc): the unsupported-type message is rebuilt from the filename, so no exception state can reach a response body (CodeQL py/stack-trace-exposure).
This commit is contained in:
1 parent
6d0319fb63
commit
f7cd94668f
9 files changed
+302
-49
No files matched your search
@@ -2369,6 +2369,20 @@ class TestGetStoreAttachmentUserError:
|
||||
msg = _get_store_attachment_user_error(RuntimeError("oops"))
|
||||
assert msg == "Failed to process file"
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_unsupported_type_message_is_rebuilt_from_the_filename(self):
|
||||
"""Nothing is read off the exception, so its text cannot reach a response."""
|
||||
from application.api.user.attachments.routes import (
|
||||
_get_store_attachment_user_error,
|
||||
)
|
||||
from application.upload_limits import UnsupportedUploadTypeError
|
||||
|
||||
err = UnsupportedUploadTypeError("internals: /srv/app/tmp/staged-42")
|
||||
msg = _get_store_attachment_user_error(err, "clip.mp4")
|
||||
|
||||
assert msg == "Unsupported file type: .mp4"
|
||||
assert "/srv/app" not in msg
|
||||
|
||||
|
||||
class TestRequireLiveSttRedisUnavailable:
|
||||
"""Cover lines 99-102: Redis unavailable returns 503."""
|
||||
@@ -2421,6 +2435,29 @@ class TestStoreAttachmentTypeGate:
|
||||
mock_storage.save_file.assert_not_called()
|
||||
mock_store_attachment.assert_not_called()
|
||||
|
||||
@patch("application.api.user.tasks.store_attachment.delay")
|
||||
def test_rejects_a_binary_renamed_to_a_text_suffix(
|
||||
self, mock_store_attachment, flask_app
|
||||
):
|
||||
""".txt has no parser, so it is judged on content like any other suffix."""
|
||||
from application.api.user.attachments.routes import StoreAttachment
|
||||
|
||||
app = Flask(__name__)
|
||||
mock_storage = MagicMock()
|
||||
with patch("application.api.user.base.storage", mock_storage), app.test_request_context(
|
||||
"/api/store_attachment",
|
||||
method="POST",
|
||||
data={"file": (io.BytesIO(MP4_BYTES), "notes.txt")},
|
||||
content_type="multipart/form-data",
|
||||
):
|
||||
request.decoded_token = {"sub": "test_user"}
|
||||
response = StoreAttachment().post()
|
||||
|
||||
assert _get_response_status(response) == 400
|
||||
assert _get_response_json(response)["message"] == "Unsupported file type: .txt"
|
||||
mock_storage.save_file.assert_not_called()
|
||||
mock_store_attachment.assert_not_called()
|
||||
|
||||
@patch("application.api.user.tasks.store_attachment.delay")
|
||||
def test_accepts_a_text_file_with_no_dedicated_parser(
|
||||
self, mock_store_attachment, flask_app
|
||||
|
||||
@@ -1,15 +1,16 @@
|
||||
"""Tests for the shared file-extension constants."""
|
||||
|
||||
import re
|
||||
from importlib.util import find_spec
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
from application.parser.file.bulk import get_default_file_extractor
|
||||
from application.parser.file.constants import (
|
||||
ATTACHMENT_PARSER_EXTENSIONS,
|
||||
attachment_extension,
|
||||
has_attachment_parser,
|
||||
SUPPORTED_ATTACHMENT_EXTENSIONS,
|
||||
SUPPORTED_SOURCE_EXTENSIONS,
|
||||
)
|
||||
|
||||
@@ -19,30 +20,38 @@ FRONTEND_CONSTANTS = (
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_every_parser_backed_suffix_is_an_allowed_attachment():
|
||||
"""The allow-list is a hand-kept literal; this is what keeps it honest.
|
||||
def test_parser_extensions_match_the_extractor():
|
||||
"""The list is a hand-kept literal; this is what keeps it honest.
|
||||
|
||||
``SUPPORTED_ATTACHMENT_EXTENSIONS`` cannot import the extractor (that
|
||||
would pull docling into the API process), so a parser added to
|
||||
``ATTACHMENT_PARSER_EXTENSIONS`` cannot import the extractor (that would
|
||||
pull docling into the API process), so a parser added to
|
||||
``get_default_file_extractor`` would otherwise stay refused as an
|
||||
attachment — which is how .webp, .tiff, .vtt and .xml were first missed.
|
||||
A suffix listed here but *without* a parser is the opposite failure: it
|
||||
skips the content check and reaches the plain-text fallthrough, which is
|
||||
how a binary named notes.txt got through.
|
||||
"""
|
||||
parser_suffixes = set(get_default_file_extractor())
|
||||
|
||||
assert parser_suffixes <= set(SUPPORTED_ATTACHMENT_EXTENSIONS), (
|
||||
assert parser_suffixes <= set(ATTACHMENT_PARSER_EXTENSIONS), (
|
||||
"parsers exist for suffixes the attachment gate refuses: "
|
||||
f"{sorted(parser_suffixes - set(SUPPORTED_ATTACHMENT_EXTENSIONS))}"
|
||||
f"{sorted(parser_suffixes - set(ATTACHMENT_PARSER_EXTENSIONS))}"
|
||||
)
|
||||
if find_spec("docling") is not None:
|
||||
assert parser_suffixes == set(ATTACHMENT_PARSER_EXTENSIONS), (
|
||||
"listed as parser-backed but nothing parses them: "
|
||||
f"{sorted(set(ATTACHMENT_PARSER_EXTENSIONS) - parser_suffixes)}"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_frontend_mirrors_the_attachment_allow_list():
|
||||
def test_frontend_mirrors_the_parser_extensions():
|
||||
"""The composer gates uploads client-side; a divergent list refuses valid files.
|
||||
|
||||
``ATTACHMENT_PARSER_EXTENSIONS`` in the frontend is a hand-kept copy of
|
||||
``SUPPORTED_ATTACHMENT_EXTENSIONS``. Nothing but this test connects them,
|
||||
and .mdx, .xhtml and .adoc were blocked in the UI while the API accepted
|
||||
them before it existed.
|
||||
the backend's. Nothing but this test connects them, and .mdx, .xhtml and
|
||||
.adoc were blocked in the UI while the API accepted them before it
|
||||
existed.
|
||||
"""
|
||||
if not FRONTEND_CONSTANTS.exists():
|
||||
pytest.skip("frontend sources not present in this checkout")
|
||||
@@ -56,16 +65,19 @@ def test_frontend_mirrors_the_attachment_allow_list():
|
||||
assert match, "ATTACHMENT_PARSER_EXTENSIONS not found in fileUpload.ts"
|
||||
frontend_extensions = set(re.findall(r"'(\.[^']+)'", match.group(1)))
|
||||
|
||||
assert frontend_extensions == set(SUPPORTED_ATTACHMENT_EXTENSIONS), (
|
||||
assert frontend_extensions == set(ATTACHMENT_PARSER_EXTENSIONS), (
|
||||
"frontend and backend attachment lists disagree — "
|
||||
f"backend only: {sorted(set(SUPPORTED_ATTACHMENT_EXTENSIONS) - frontend_extensions)}, "
|
||||
f"frontend only: {sorted(frontend_extensions - set(SUPPORTED_ATTACHMENT_EXTENSIONS))}"
|
||||
f"backend only: {sorted(set(ATTACHMENT_PARSER_EXTENSIONS) - frontend_extensions)}, "
|
||||
f"frontend only: {sorted(frontend_extensions - set(ATTACHMENT_PARSER_EXTENSIONS))}"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_attachments_accept_everything_source_ingestion_does():
|
||||
assert set(SUPPORTED_SOURCE_EXTENSIONS) <= set(SUPPORTED_ATTACHMENT_EXTENSIONS)
|
||||
def test_source_ingestion_types_are_parser_backed_apart_from_text():
|
||||
"""Source ingestion's own list must not smuggle a parserless suffix past the sniff."""
|
||||
assert set(SUPPORTED_SOURCE_EXTENSIONS) - set(ATTACHMENT_PARSER_EXTENSIONS) == {
|
||||
".txt"
|
||||
}
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
@@ -93,3 +105,5 @@ def test_has_attachment_parser_is_case_insensitive_and_excludes_zip():
|
||||
assert not has_attachment_parser("archive.zip")
|
||||
assert not has_attachment_parser("main.py")
|
||||
assert not has_attachment_parser("clip.mp4")
|
||||
# .txt is the plain-text fallthrough itself, not a parser.
|
||||
assert not has_attachment_parser("notes.txt")
|
||||
@@ -1,5 +1,6 @@
|
||||
"""Tests for bounded user-upload stream helpers."""
|
||||
|
||||
import codecs
|
||||
import io
|
||||
|
||||
import pytest
|
||||
@@ -80,10 +81,49 @@ def test_enforce_parseable_attachment_rejects_binary_without_a_parser(
|
||||
assert str(excinfo.value).startswith("Unsupported file type")
|
||||
|
||||
|
||||
def test_enforce_parseable_attachment_rejects_binary_named_as_text(tmp_path):
|
||||
""".txt has no parser — it *is* the fallthrough — so it is sniffed like any suffix.
|
||||
|
||||
Renaming a video to notes.txt would otherwise walk straight back into the
|
||||
bug this gate exists for.
|
||||
"""
|
||||
from application.upload_limits import (
|
||||
enforce_parseable_attachment,
|
||||
UnsupportedUploadTypeError,
|
||||
)
|
||||
|
||||
path = tmp_path / "notes.txt"
|
||||
path.write_bytes(MP4_HEADER + bytes(range(256)) * 4)
|
||||
|
||||
with pytest.raises(UnsupportedUploadTypeError) as excinfo:
|
||||
enforce_parseable_attachment(path, "notes.txt")
|
||||
assert str(excinfo.value) == "Unsupported file type: .txt"
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"content",
|
||||
[
|
||||
codecs.BOM_UTF8 + "hello — Unicode\n".encode(),
|
||||
codecs.BOM_UTF16_LE + "hello\n".encode("utf-16-le"),
|
||||
codecs.BOM_UTF16_BE + "hello\n".encode("utf-16-be"),
|
||||
codecs.BOM_UTF32_BE + "hello\n".encode("utf-32-be"),
|
||||
],
|
||||
)
|
||||
def test_enforce_parseable_attachment_accepts_bom_marked_unicode_text(
|
||||
content, tmp_path
|
||||
):
|
||||
"""A UTF-16 .txt is half NUL bytes and still ordinary text — the BOM says so."""
|
||||
from application.upload_limits import enforce_parseable_attachment
|
||||
|
||||
path = tmp_path / "notes.txt"
|
||||
path.write_bytes(content)
|
||||
|
||||
enforce_parseable_attachment(path, "notes.txt")
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"filename",
|
||||
[
|
||||
"notes.txt",
|
||||
"Report.PDF",
|
||||
"photo.JPG",
|
||||
"slides.pptx",
|
||||
@@ -108,7 +148,15 @@ def test_enforce_parseable_attachment_accepts_parser_backed_types(filename, tmp_
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"filename",
|
||||
["main.py", "server.log", "config.yaml", "query.sql", "Dockerfile", "notes.unknown"],
|
||||
[
|
||||
"notes.txt",
|
||||
"main.py",
|
||||
"server.log",
|
||||
"config.yaml",
|
||||
"query.sql",
|
||||
"Dockerfile",
|
||||
"notes.unknown",
|
||||
],
|
||||
)
|
||||
def test_enforce_parseable_attachment_accepts_text_without_a_parser(filename, tmp_path):
|
||||
"""The plain-text fallthrough reads these correctly, so they must stay allowed."""
|
||||
@@ -127,6 +175,8 @@ def test_enforce_parseable_attachment_accepts_text_without_a_parser(filename, tm
|
||||
(b"plain text\n", True),
|
||||
("café — em dash\n".encode(), True),
|
||||
(b"\x1b[31mred log line\x1b[0m\n", True),
|
||||
(codecs.BOM_UTF16_LE + "hi\n".encode("utf-16-le"), True),
|
||||
(codecs.BOM_UTF8 + b"hi\n", True),
|
||||
(b"text\x00with nul", False),
|
||||
(bytes(range(32)) * 4, False),
|
||||
(b"\x7f\x7f\x7f\x7f" + b"a" * 16, False),
|
||||
|
||||
Reference in new issue
Block a user