fix: resolve cover_image relative URL validation error for self-hoste…#9294
fix: resolve cover_image relative URL validation error for self-hoste…#9294karthiksuki wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthrough
ChangesMinIO Public Endpoint Routing
User Asset URL Exposure and Upload Guard
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/plane/tests/unit/settings/test_storage.py (1)
67-78: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a companion test for the
is_server=Truebranch.The new test verifies only the public-endpoint path. Please add a second assertion/test that
S3Storage(request=request, is_server=True)keepsendpoint_urlon the internal endpoint, so the new contract is fully pinned.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/plane/tests/unit/settings/test_storage.py` around lines 67 - 78, Add a companion test method that verifies the behavior when S3Storage is initialized with is_server=True parameter. The new test should follow the same setup pattern as test_minio_uses_public_endpoint_for_request_signed_urls but pass is_server=True to the S3Storage constructor, and assert that the endpoint_url in the boto3 client call_kwargs uses the internal endpoint (not the public localhost endpoint), ensuring the internal server branch of the endpoint selection logic is fully covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/plane/settings/storage.py`:
- Around line 47-50: The issue is that browser-facing presigned URL generation
in asset.py is incorrectly instantiating S3Storage with is_server=True, which
causes presigned URLs to point to the internal MinIO endpoint instead of the
public endpoint that browsers need. Find the S3Storage instantiation in the
presigned upload flow (around line 337-349 in asset.py) and change
is_server=True to is_server=False so that the public endpoint URL is used for
presigned URLs returned to clients.
---
Nitpick comments:
In `@apps/api/plane/tests/unit/settings/test_storage.py`:
- Around line 67-78: Add a companion test method that verifies the behavior when
S3Storage is initialized with is_server=True parameter. The new test should
follow the same setup pattern as
test_minio_uses_public_endpoint_for_request_signed_urls but pass is_server=True
to the S3Storage constructor, and assert that the endpoint_url in the boto3
client call_kwargs uses the internal endpoint (not the public localhost
endpoint), ensuring the internal server branch of the endpoint selection logic
is fully covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dbca2950-a059-4bda-bd8b-3774052424dc
📒 Files selected for processing (4)
apps/api/plane/app/serializers/user.pyapps/api/plane/settings/storage.pyapps/api/plane/tests/unit/settings/test_storage.pyapps/web/helpers/cover-image.helper.ts
…d MinIO #9283
Description
Fixes a misleading
400 Bad Requesterror when a user updates their profile cover image or avatarDepending on the preferred architecture design of the maintainers, this PR addresses the issue via:
cover_imageto handle relative paths gracefully (similar toavatar_urlimplementation) instead of utilizing standard strict DjangoURLFieldvalidation.PATCH /api/users/me/request provides a consistent schema that aligns with backend expectations.Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
New Features
Bug Fixes