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/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) 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 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`) 우회 차단.