From a65f9c303dd032c0545d82685bafb0354775672f Mon Sep 17 00:00:00 2001 From: otro Date: Wed, 5 Aug 2026 14:17:35 +0900 Subject: [PATCH 1/3] fix: restore PageBinaryUpdateSerializer's validators and update() The three methods belonging to PageBinaryUpdateSerializer -- validate_description_binary, validate_description_html and update -- sat on PageCommentSerializer instead. The docstring left behind inside PageCommentSerializer still read "Update the page instance". PageBinaryUpdateSerializer is a plain serializers.Serializer, so DRF has no model to derive update() from. Without it, serializer.save() raises NotImplementedError and every page description save returns 500. Observed in production as 6 x `update() must be implemented` on PagesDescriptionViewSet.partial_update. Two further consequences of the misplacement, both fixed by moving the methods back: - Page saves ran no validation at all, so validate_html_content -- the HTML sanitisation that #7507 added this serializer for -- never executed. - PageCommentSerializer.update() shadowed ModelSerializer.update() and only looked for description_* keys, which PageComment does not have (it has comment_html / comment_json). Editing a page comment therefore called instance.save() without applying any validated data: the edit silently saved nothing. Co-Authored-By: Claude Opus 5 (1M context) --- apps/api/plane/app/serializers/page.py | 94 +++++++-------- .../serializers/test_page_binary_update.py | 111 ++++++++++++++++++ 2 files changed, 158 insertions(+), 47 deletions(-) create mode 100644 apps/api/plane/tests/unit/serializers/test_page_binary_update.py diff --git a/apps/api/plane/app/serializers/page.py b/apps/api/plane/app/serializers/page.py index 04d4199440a..1ac86b14a17 100644 --- a/apps/api/plane/app/serializers/page.py +++ b/apps/api/plane/app/serializers/page.py @@ -191,6 +191,53 @@ class PageBinaryUpdateSerializer(serializers.Serializer): description_html = serializers.CharField(required=False, allow_blank=True) description_json = serializers.JSONField(required=False, allow_null=True) + def validate_description_binary(self, value): + """Validate the base64-encoded binary data""" + if not value: + return value + + try: + # Decode the base64 data + binary_data = base64.b64decode(value) + + # Validate the binary data + is_valid, error_message = validate_binary_data(binary_data) + if not is_valid: + raise serializers.ValidationError(f"Invalid binary data: {error_message}") + + return binary_data + except Exception as e: + if isinstance(e, serializers.ValidationError): + raise + raise serializers.ValidationError("Failed to decode base64 data") + + def validate_description_html(self, value): + """Validate the HTML content""" + if not value: + return value + + # Use the validation function from utils + is_valid, error_message, sanitized_html = validate_html_content(value) + if not is_valid: + raise serializers.ValidationError(error_message) + + # Return sanitized HTML if available, otherwise return original + return sanitized_html if sanitized_html is not None else value + + def update(self, instance, validated_data): + """Update the page instance with validated data""" + if "description_binary" in validated_data: + instance.description_binary = validated_data.get("description_binary") + + if "description_html" in validated_data: + instance.description_html = validated_data.get("description_html") + + if "description_json" in validated_data: + instance.description_json = validated_data.get("description_json") + + instance.save() + return instance + class PageCollectionSerializer(BaseSerializer): page_ids = serializers.SerializerMethodField() @@ -293,50 +340,3 @@ class Meta: "created_at", "updated_at", ] - - def validate_description_binary(self, value): - """Validate the base64-encoded binary data""" - if not value: - return value - - try: - # Decode the base64 data - binary_data = base64.b64decode(value) - - # Validate the binary data - is_valid, error_message = validate_binary_data(binary_data) - if not is_valid: - raise serializers.ValidationError(f"Invalid binary data: {error_message}") - - return binary_data - except Exception as e: - if isinstance(e, serializers.ValidationError): - raise - raise serializers.ValidationError("Failed to decode base64 data") - - def validate_description_html(self, value): - """Validate the HTML content""" - if not value: - return value - - # Use the validation function from utils - is_valid, error_message, sanitized_html = validate_html_content(value) - if not is_valid: - raise serializers.ValidationError(error_message) - - # Return sanitized HTML if available, otherwise return original - return sanitized_html if sanitized_html is not None else value - - def update(self, instance, validated_data): - """Update the page instance with validated data""" - if "description_binary" in validated_data: - instance.description_binary = validated_data.get("description_binary") - - if "description_html" in validated_data: - instance.description_html = validated_data.get("description_html") - - if "description_json" in validated_data: - instance.description_json = validated_data.get("description_json") - - instance.save() - return instance diff --git a/apps/api/plane/tests/unit/serializers/test_page_binary_update.py b/apps/api/plane/tests/unit/serializers/test_page_binary_update.py new file mode 100644 index 00000000000..22f3ecdc77e --- /dev/null +++ b/apps/api/plane/tests/unit/serializers/test_page_binary_update.py @@ -0,0 +1,111 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +import base64 +from unittest.mock import Mock + +import pytest +from rest_framework import serializers + +from plane.app.serializers.page import ( + PageBinaryUpdateSerializer, + PageCommentSerializer, +) + + +@pytest.mark.unit +class TestPageBinaryUpdateSerializer: + """ + `PageBinaryUpdateSerializer` is a plain `serializers.Serializer`, so DRF + cannot derive `update()` for it. Without an explicit one, `serializer.save()` + raises `NotImplementedError` and *every* page description save returns 500. + """ + + def test_update_is_implemented(self): + assert "update" in PageBinaryUpdateSerializer.__dict__, ( + "PageBinaryUpdateSerializer must define update(); " + "serializers.Serializer has no model to derive it from." + ) + + def test_save_writes_all_description_fields(self): + page = Mock() + serializer = PageBinaryUpdateSerializer( + page, + data={"description_html": "

hello

", "description_json": {"type": "doc"}}, + partial=True, + ) + + assert serializer.is_valid(), serializer.errors + serializer.save() + + assert page.description_html == "

hello

" + assert page.description_json == {"type": "doc"} + page.save.assert_called_once() + + def test_save_leaves_untouched_fields_alone(self): + page = Mock() + page.description_json = {"original": True} + + serializer = PageBinaryUpdateSerializer(page, data={"description_html": "

x

"}, partial=True) + assert serializer.is_valid(), serializer.errors + serializer.save() + + assert page.description_json == {"original": True} + + def test_description_binary_is_base64_decoded(self): + # validate_binary_data rejects anything shorter than 4 bytes. + raw = b"\x00\x01\x02\x03\x04\x05\x06\x07" + page = Mock() + + serializer = PageBinaryUpdateSerializer( + page, + data={"description_binary": base64.b64encode(raw).decode()}, + partial=True, + ) + + assert serializer.is_valid(), serializer.errors + serializer.save() + assert page.description_binary == raw + + def test_too_short_binary_is_rejected(self): + serializer = PageBinaryUpdateSerializer( + Mock(), + data={"description_binary": base64.b64encode(b"\x01\x02").decode()}, + partial=True, + ) + + assert not serializer.is_valid() + assert "description_binary" in serializer.errors + + def test_invalid_base64_is_rejected_not_saved(self): + serializer = PageBinaryUpdateSerializer(Mock(), data={"description_binary": "!!not base64!!"}, partial=True) + + assert not serializer.is_valid() + assert "description_binary" in serializer.errors + + def test_html_is_validated(self): + """The validators exist on this serializer, so HTML sanitisation runs on save.""" + assert "validate_description_html" in PageBinaryUpdateSerializer.__dict__ + assert "validate_description_binary" in PageBinaryUpdateSerializer.__dict__ + + +@pytest.mark.unit +class TestPageCommentSerializerHasNoPageDescriptionLogic: + """ + These three methods belong to `PageBinaryUpdateSerializer`. When they sat on + `PageCommentSerializer` instead, its `update()` override shadowed + `ModelSerializer.update()` and only ever looked for `description_*` keys — + which `PageComment` does not have — so editing a page comment silently saved + nothing. + """ + + @pytest.mark.parametrize( + "method", + ["update", "validate_description_binary", "validate_description_html"], + ) + def test_page_description_methods_are_not_defined_here(self, method): + assert method not in PageCommentSerializer.__dict__ + + def test_update_falls_through_to_model_serializer(self): + assert PageCommentSerializer.update is serializers.ModelSerializer.update From bd06b96a962168ea16770afea7c751abf78b1206 Mon Sep 17 00:00:00 2001 From: otro Date: Wed, 5 Aug 2026 14:17:47 +0900 Subject: [PATCH 2/3] fix: cap audit-log bodies so one big payload can't 500 the web UI APITokenLogMiddleware copied request and response bodies into the celery message with no size bound, and mongo_log duplicated the whole dict, so every message carried the payload twice. A large API response therefore published at roughly 2x its size. In production that reached 354,911,882 bytes against RabbitMQ's 128MB max_message_size. RabbitMQ rejects the publish with PRECONDITION_FAILED (406) and closes the channel -- and because the failure surfaces on the *next* publish over that connection, the request that actually returned 500 was an unrelated one: recent_visited_task.delay() inside ProjectViewSet.retrieve. Users saw `GET /api/workspaces/{slug}/projects/{id}/` fail with 500 and the sibling `/pages/` request hang until they gave up (499) -- i.e. opening a project or a page broke because of someone else's API traffic. Bodies are now capped at 64KB with an explicit truncation marker, bounding a message at ~128KB. Also in this path: - X-Api-Key is redacted from the logged headers blob, which duplicated the raw token for no audit value (partial PLANE-75; token_identifier is left alone as it is the audit join key). - StreamingHttpResponse has no .content, so page description binary downloads raised AttributeError into log_exception on every request. Those are now recorded as [Streaming Content] instead. Co-Authored-By: Claude Opus 5 (1M context) --- apps/api/plane/middleware/logger.py | 62 +++++++++-- .../unit/middleware/test_api_token_log.py | 102 ++++++++++++++++++ 2 files changed, 158 insertions(+), 6 deletions(-) create mode 100644 apps/api/plane/tests/unit/middleware/test_api_token_log.py diff --git a/apps/api/plane/middleware/logger.py b/apps/api/plane/middleware/logger.py index b8cf6f9c045..b3c7d5df8d0 100644 --- a/apps/api/plane/middleware/logger.py +++ b/apps/api/plane/middleware/logger.py @@ -20,6 +20,19 @@ api_logger = logging.getLogger("plane.api.request") +# Upper bound on how much of a request/response body is copied into the audit +# log. Each celery message carries the body twice (log_data + mongo_log), so an +# uncapped body is published to RabbitMQ at ~2x its size. A single oversized +# publish is rejected with `PRECONDITION_FAILED (406)` and takes the whole AMQP +# channel down with it, which then surfaces as a 500 on the *next* unrelated +# request that shares the connection (e.g. `recent_visited_task.delay()` while +# opening a project or page). Capping here keeps one big API payload from +# breaking web navigation for everyone. +MAX_LOGGED_BODY_BYTES = 64 * 1024 + +# Header whose value is the raw API token; never copy it into the audit log. +API_KEY_HEADER = "X-Api-Key" + class RequestLoggerMiddleware: def __init__(self, get_response): @@ -106,14 +119,51 @@ def _safe_decode_body(self, content): if content.startswith(b"\x89PNG") or content.startswith(b"\xff\xd8\xff") or content.startswith(b"%PDF"): return "[Binary Content]" + original_size = len(content) + truncated = original_size > MAX_LOGGED_BODY_BYTES + if truncated: + content = content[:MAX_LOGGED_BODY_BYTES] + try: - return content.decode("utf-8") + decoded = content.decode("utf-8") except UnicodeDecodeError: - return "[Could not decode content]" + if not truncated: + return "[Could not decode content]" + # The cut may have landed mid-character; that alone shouldn't turn a + # useful truncated body into "[Could not decode content]". + decoded = content.decode("utf-8", errors="replace") + + if truncated: + decoded += f"... [truncated: {original_size} bytes total, logged first {MAX_LOGGED_BODY_BYTES}]" + return decoded + + def _response_body(self, response): + """ + Returns the response body for logging, or a marker for responses whose + body cannot be read without consuming it. + """ + # StreamingHttpResponse (e.g. page description binary downloads) has no + # `.content`; touching it raises AttributeError. + if getattr(response, "streaming", False): + return "[Streaming Content]" + + content = getattr(response, "content", None) + return self._safe_decode_body(content) if content else None + + def _safe_headers(self, request): + """ + Stringified request headers with the API token redacted. + """ + headers = {} + for key, value in request.headers.items(): + if key.lower() == API_KEY_HEADER.lower(): + headers[key] = "[REDACTED]" + else: + headers[key] = value + return str(headers) def process_request(self, request, response, request_body): - api_key_header = "X-Api-Key" - api_key = request.headers.get(api_key_header) + api_key = request.headers.get(API_KEY_HEADER) # If the API key is not present, return if not api_key: @@ -125,9 +175,9 @@ def process_request(self, request, response, request_body): "path": request.path, "method": request.method, "query_params": request.META.get("QUERY_STRING", ""), - "headers": str(request.headers), + "headers": self._safe_headers(request), "body": self._safe_decode_body(request_body) if request_body else None, - "response_body": self._safe_decode_body(response.content) if response.content else None, + "response_body": self._response_body(response), "response_code": response.status_code, "ip_address": get_client_ip(request=request), "user_agent": request.META.get("HTTP_USER_AGENT", None), diff --git a/apps/api/plane/tests/unit/middleware/test_api_token_log.py b/apps/api/plane/tests/unit/middleware/test_api_token_log.py new file mode 100644 index 00000000000..a22618dc9f4 --- /dev/null +++ b/apps/api/plane/tests/unit/middleware/test_api_token_log.py @@ -0,0 +1,102 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +from unittest.mock import Mock + +import pytest + +from plane.middleware.logger import ( + API_KEY_HEADER, + MAX_LOGGED_BODY_BYTES, + APITokenLogMiddleware, +) + + +@pytest.fixture +def middleware(): + return APITokenLogMiddleware(get_response=Mock()) + + +@pytest.mark.unit +class TestAuditLogBodyCap: + """ + An uncapped body is copied into the celery message twice (log_data + + mongo_log), so one large API payload could exceed RabbitMQ's max message + size. That publish fails with PRECONDITION_FAILED (406) and kills the AMQP + channel, which then surfaces as a 500 on the next unrelated request sharing + the connection. The cap is what keeps that from happening. + """ + + def test_small_body_is_logged_verbatim(self, middleware): + assert middleware._safe_decode_body(b"hello") == "hello" + + def test_oversized_body_is_truncated(self, middleware): + original_size = MAX_LOGGED_BODY_BYTES * 3 + decoded = middleware._safe_decode_body(b"a" * original_size) + + assert len(decoded) < original_size + assert decoded.startswith("a" * 100) + assert f"truncated: {original_size} bytes total" in decoded + + def test_truncation_survives_a_cut_through_a_multibyte_character(self, middleware): + # "가" is 3 bytes in UTF-8, so the cut lands mid-character. + body = "가".encode("utf-8") * MAX_LOGGED_BODY_BYTES + + decoded = middleware._safe_decode_body(body) + + assert "truncated" in decoded + assert decoded != "[Could not decode content]" + + def test_undecodable_short_body_still_reports_plainly(self, middleware): + assert middleware._safe_decode_body(b"\xff\xfe\x00") == "[Could not decode content]" + + def test_binary_signatures_short_circuit(self, middleware): + assert middleware._safe_decode_body(b"%PDF-1.7 ...") == "[Binary Content]" + + def test_empty_body_is_none(self, middleware): + assert middleware._safe_decode_body(b"") is None + assert middleware._safe_decode_body(None) is None + + +@pytest.mark.unit +class TestAuditLogResponseBody: + def test_streaming_response_is_not_consumed(self, middleware): + # StreamingHttpResponse has no `.content`; reading it raises. + response = Mock(spec=["streaming"]) + response.streaming = True + + assert middleware._response_body(response) == "[Streaming Content]" + + def test_regular_response_body_is_read(self, middleware): + response = Mock() + response.streaming = False + response.content = b"ok" + + assert middleware._response_body(response) == "ok" + + def test_empty_response_body_is_none(self, middleware): + response = Mock() + response.streaming = False + response.content = b"" + + assert middleware._response_body(response) is None + + +@pytest.mark.unit +class TestAuditLogHeaderRedaction: + def test_api_key_is_redacted(self, middleware): + request = Mock() + request.headers = {API_KEY_HEADER: "plane_api_secret", "Host": "plane.example.com"} + + headers = middleware._safe_headers(request) + + assert "plane_api_secret" not in headers + assert "[REDACTED]" in headers + assert "plane.example.com" in headers + + def test_api_key_is_redacted_case_insensitively(self, middleware): + request = Mock() + request.headers = {"x-api-key": "plane_api_secret"} + + assert "plane_api_secret" not in middleware._safe_headers(request) From 2ec7eb07a308ed668ec5f1ba43078b323b7e1feb Mon Sep 17 00:00:00 2001 From: otro Date: Wed, 5 Aug 2026 14:17:55 +0900 Subject: [PATCH 3/3] test: assert logger_task stays in CELERY_IMPORTS, plus mote.51 notes mote.49 added "plane.bgtasks.logger_task" to CELERY_IMPORTS. The deployed mote.50 image does not have that line, although origin/mote does -- the rest of the tuple is byte-identical. mote.50 was built from /srv/shared/app-src/plane after a stash detour (see the mote.50 warning in RELEASES-mote.md), and the one-line fix was lost in the process. The failure mode is silent by construction: an unregistered task is not an error anywhere, the worker just logs "Received unregistered task" and discards the message, so the audit trail goes to zero without anything going red. This test pins the entry and checks that every listed module actually imports. No source change is needed for that one -- rebuilding and redeploying from origin/mote is the fix. Co-Authored-By: Claude Opus 5 (1M context) --- .../unit/settings/test_celery_imports.py | 33 +++++++++++++++++++ docs/RELEASES-mote.md | 12 +++++++ 2 files changed, 45 insertions(+) create mode 100644 apps/api/plane/tests/unit/settings/test_celery_imports.py diff --git a/apps/api/plane/tests/unit/settings/test_celery_imports.py b/apps/api/plane/tests/unit/settings/test_celery_imports.py new file mode 100644 index 00000000000..6ddaef26c38 --- /dev/null +++ b/apps/api/plane/tests/unit/settings/test_celery_imports.py @@ -0,0 +1,33 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +import importlib + +import pytest +from django.conf import settings + + +@pytest.mark.unit +class TestCeleryImports: + """ + A task the api enqueues but the worker never imports is not an error anywhere + — the worker just logs "Received unregistered task" and discards the message. + That failure mode is silent by construction, so it gets asserted here. + """ + + def test_every_listed_module_is_importable(self): + for module in settings.CELERY_IMPORTS: + importlib.import_module(module) + + def test_logger_task_is_registered(self): + """ + `APITokenLogMiddleware` enqueues `logger_task.process_logs` on every + API-key request. Dropping it from CELERY_IMPORTS discards the whole + audit trail without raising anything. + """ + assert "plane.bgtasks.logger_task" in settings.CELERY_IMPORTS + + def test_no_duplicate_entries(self): + imports = list(settings.CELERY_IMPORTS) + assert len(imports) == len(set(imports)) diff --git a/docs/RELEASES-mote.md b/docs/RELEASES-mote.md index f62443d796f..edf357ba9ad 100644 --- a/docs/RELEASES-mote.md +++ b/docs/RELEASES-mote.md @@ -5,6 +5,18 @@ CE v1.3.1 기반 `mote` 브랜치. 이미지 태그 `mote/plane-{backend,fronten > ⚠️ 배포 시 `--env-file plane.env` 필수 — 누락하면 인터폴레이션이 DB 비밀번호를 기본값으로 떨어뜨려 컨테이너가 인증 실패한다. +## v1.3.1-mote.51 (2026-08-05) — 문서(Pages) 저장 500 + 감사로그가 웹을 끊는 문제 fix + +도트/쿠키 "plane 페이지가 잘 안들어가진다" 리포트에서 출발. 서버·로그인·이슈 API는 전부 정상이었고(12시간 5xx 0건, SSO 정상, 홈 API 전건 200) 실제 결함은 아래 3건. + +- **문서 본문 저장이 100% 500** — `PageBinaryUpdateSerializer`의 `validate_description_binary` / `validate_description_html` / `update` 세 메서드가 병합 사고로 **`PageCommentSerializer` 안에 들어가 있었다**(331줄 docstring이 "Update the *page* instance"라고 말하는 게 증거). `PageBinaryUpdateSerializer`는 `serializers.Serializer`라 `update()`가 없으면 DRF가 `NotImplementedError`를 던진다 → 저장 시도 전건 500. 세 메서드를 원래 클래스로 되돌렸다. + - 부수 피해 2건도 같이 해소: ① 페이지 저장 시 **HTML 살균(`validate_html_content`)이 아예 실행되지 않고 있었다**(#7507의 목적이 무력화), ② 잘못 붙은 `update()`가 `ModelSerializer.update()`를 가려서 **페이지 댓글 수정이 조용히 아무것도 저장하지 않았다**(`PageComment`에는 `description_*` 필드가 없다). +- **감사로그가 AMQP 채널을 죽여 웹 화면 진입이 500** — `APITokenLogMiddleware`가 요청/응답 본문을 **무제한** 복사하고 `mongo_log`가 그걸 한 번 더 복제 → 실측 **354,911,882바이트** 메시지가 RabbitMQ 한도(128MB)를 넘겨 `PRECONDITION_FAILED (406)`으로 채널이 통째로 죽었다. 그러면 같은 커넥션을 쓰는 **다음 요청**이 터진다 — Plane은 프로젝트·문서를 열 때마다 `recent_visited_task.delay()`를 부르므로 실제 증상은 `GET /api/workspaces/{slug}/projects/{id}/ → 500` + `/pages/` 499(사용자가 기다리다 포기)로 나타났다. 본문을 64KB로 캡했다(메시지 최대 ~128KB). + - 같이 처리: `X-Api-Key`를 `headers` 블롭에서 **마스킹**(PLANE-75 일부), `StreamingHttpResponse`(문서 바이너리 다운로드)에서 `.content` 접근이 매번 AttributeError를 내던 것 차단. +- **`logger_task` 등록이 mote.50에서 유실(회귀)** — mote.49에서 넣은 `CELERY_IMPORTS`의 `"plane.bgtasks.logger_task"` 한 줄이 **배포 이미지에만 없었다**(레포에는 있음). mote.50이 stash 대피/복원을 거친 `/srv/shared/app-src/plane`에서 빌드된 결과(위 mote.50 ⚠️ 항목 참조). 코드 수정은 불필요하고 **재빌드·재배포가 곧 수정**. 재발 방지로 `CELERY_IMPORTS` 회귀 테스트를 추가했다. +- **테스트 25건 추가** — `test_page_binary_update.py`, `test_api_token_log.py`, `test_celery_imports.py`. 기준선(origin/mote) 대비 회귀 0건(89 → 114 passed, 실패·에러 동일). +- ⚠️ 남은 PLANE-75: `api_activity_logs.token_identifier`에 API 키 원문이 그대로 들어간다(감사 조인키라 이번엔 손대지 않음). 마스킹 여부는 otro 판단 대기. + ## v1.3.1-mote.50 (2026-07-21) — .md/.mdx 첨부파일 업로드 실패 fix - **원인**: `.md`/`.mdx`는 매직바이트가 없는 순수 텍스트라 프론트엔드 `file-type` 시그니처 감지가 빈 문자열을 반환 → 백엔드가 `not type` 체크로 400 "Invalid file type." 거부. 인프라(MinIO/S3) 문제 아님, API 직접 호출로 재현·검증 완료(TEST-32). - **수정**: upstream Plane `feat/file-uploads-md-mdx-support` 커밋 2개 cherry-pick(`dac358b2aa`, `9eb1148dde`) — 확장자 기반 MIME fallback(`EXTENSION_MIME_TYPE_MAP`) 추가, `text/mdx` allowlist 등록, 이중확장자(`foo.exe.md`) 우회 차단.