Revert "feat: add org logo upload and branding support" - #1406
Merged
Conversation
- Add logo_url, logo_s3_key, logo_filename fields to Org model (migration 0164) - Add org logo service with upload from file, upload from URL, and delete logic - Add org logo API endpoints: GET, POST /upload/, POST /url/, DELETE - Return logo_filename in OrgSchema and OrgLogoResponse for frontend tab state - Fix SVG logo blurriness in PDF by converting to high-res PNG via canvas before capture
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds org logo support: environment variable, database fields and migration, Pydantic schemas, S3 upload/delete utilities with HTTP helpers and domain exceptions, OrgLogoService business logic, REST API endpoints at ChangesOrganization Logo Management
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
ddpui/utils/s3_utils.py (1)
61-61: ⚡ Quick winRemove unnecessary f-string prefix.
The string has no placeholders, so the
fprefix is not needed.♻️ Proposed fix
- raise ValueError(f"File size exceeds the 5MB limit") + raise ValueError("File size exceeds the 5MB limit")🤖 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 `@ddpui/utils/s3_utils.py` at line 61, The ValueError message uses an unnecessary f-string prefix; replace raise ValueError(f"File size exceeds the 5MB limit") with a plain string (remove the leading f) so it becomes raise ValueError("File size exceeds the 5MB limit") in the file size check (the raise ValueError(...) statement in ddpui/utils/s3_utils.py).ddpui/core/org_logo/org_logo_service.py (2)
95-100: ⚡ Quick winPreserve exception chain when re-raising.
Use
raise ... from eto maintain the full traceback for debugging.♻️ Proposed fix
except Exception as e: logger.error(f"S3 delete failed for {org.slug}: {e}") - raise OrgLogoS3Error("Failed to delete logo from S3") + raise OrgLogoS3Error("Failed to delete logo from S3") from e🤖 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 `@ddpui/core/org_logo/org_logo_service.py` around lines 95 - 100, The except block in org_logo_service.py catches Exception as e when calling delete_org_logo(org.logo_s3_key) and re-raises OrgLogoS3Error without preserving the original exception chain; update the re-raise to use "raise OrgLogoS3Error('Failed to delete logo from S3') from e" so the original traceback and context are preserved for debugging while keeping the existing log call (logger.error(f"S3 delete failed for {org.slug}: {e}")).
41-51: ⚡ Quick winPreserve exception chain when re-raising.
When converting exceptions to domain-specific errors, use
raise ... from eto maintain the full traceback for debugging.♻️ Proposed fix
except ValueError as e: - raise OrgLogoValidationError(str(e)) + raise OrgLogoValidationError(str(e)) from e except Exception as e: logger.error(f"S3 upload failed for {org.slug}: {e}") - raise OrgLogoS3Error("Failed to upload logo to S3") + raise OrgLogoS3Error("Failed to upload logo to S3") from e🤖 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 `@ddpui/core/org_logo/org_logo_service.py` around lines 41 - 51, The exception handlers in the upload block for upload_org_logo currently swallow the original exceptions; update the except clauses to preserve the exception chain by re-raising domain errors using "raise ... from e" (i.e., change the OrgLogoValidationError raise to "raise OrgLogoValidationError(str(e)) from e" and change the OrgLogoS3Error raise to "raise OrgLogoS3Error(\"Failed to upload logo to S3\") from e"), keeping the existing logger.error call for S3 failures so the original traceback is available for debugging.
🤖 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 `@ddpui/api/org_logo_api.py`:
- Around line 64-78: The upload_logo_from_url endpoint (function
upload_logo_from_url) currently calls OrgLogoService.upload_logo_from_url
without any error handling, so any network, validation, or DB exception becomes
a raw 500; wrap the call in a try/except that catches expected errors (e.g.,
network/requests exceptions, ValueError/validation errors, and DB errors like
SQLAlchemyError) and translate them into appropriate HTTP responses using
api_response (success=False with a clear message) or raise HTTPException with
proper status codes (400 for invalid URL/validation, 502 for upstream fetch
errors, 500 for save failures), ensuring OrgLogoResponse.from_model is only
called on success; reference OrgLogoService.upload_logo_from_url and the
api_response/OrgLogoResponse usage when implementing the handling.
In `@ddpui/core/org_logo/org_logo_service.py`:
- Line 1: Run Black to reformat the file ddpui/core/org_logo/org_logo_service.py
and commit the resulting changes; ensure the module-level docstring and the rest
of the file conform to Black's formatting (you can run `black
ddpui/core/org_logo/org_logo_service.py` locally) and include the formatted file
in your commit so CI passes.
In `@ddpui/core/reports/pdf_export_service.py`:
- Around line 114-120: svg detection currently relies on ".svg" in the URL
(inside the page.evaluate that builds svg_items) which misses
extensionless/querystring CDN URLs; instead, collect all image srcs in svg_items
(remove the .svg url filter in page.evaluate) and then, in the server-side code
after page.evaluate, fetch each img.src and determine if it's an SVG by checking
the HTTP Content-Type header for "image/svg+xml" and/or if the response body
starts with "<svg". Use that result to decide which images to send to the
rasterization pass so extensionless/CDN/querystring-served SVGs are correctly
rasterized; update references to svg_items and the rasterization logic
accordingly.
- Around line 123-131: The loop that fetches SVGs (iterate svg_items and
variable svg_url) currently calls http_requests.get(svg_url) unchecked; restrict
this by validating the URL before fetching: parse svg_url (e.g., via
urllib.parse) and allow only trusted schemes/hosts from a configured allowlist
(your S3/CDN/frontend origins) and reject or skip any URLs that resolve to
non-HTTP(S), missing netloc, or to private/RFC1918 IPs; if a URL is external,
avoid calling http_requests.get and instead require that external logos be
re-hosted/ingested earlier in the pipeline. Apply this check right before the
http_requests.get call (where svg_data_uri is created) so only approved origins
are fetched and converted.
- Around line 148-154: The current page.evaluate call in pdf_export_service.py
replaces every <img> with the same src (using svg_url), which breaks cases where
the SVG is reused at different sizes; instead, pass a unique element index or
selector from svg_items into the evaluate call and only replace that specific
node. Update the code that builds svg_items to capture an index or a unique
selector per item, include that index/selector in the args (alongside
svg_url/png_data_uri), and change the page.evaluate callback to target
document.querySelectorAll('img')[index] or document.querySelector(selector) so
only the intended element is swapped; keep references to svg_items, svg_url,
png_data_uri and the page.evaluate invocation to locate and change the logic.
In `@ddpui/schemas/org_schema.py`:
- Line 52: The OrgLogoResponse schema currently declares logo_url as a required
str but the Org model allows null; update the schema in
ddpui/schemas/org_schema.py to accept None by changing logo_url to an optional
type (e.g., Optional[str] with a default of None) so from_model() can pass
org.logo_url when it's null without Pydantic validation errors; locate the
OrgLogoResponse (and any related from_model/static method that maps Org ->
OrgLogoResponse) and make the logo_url type nullable to match the Django Org
model.
---
Nitpick comments:
In `@ddpui/core/org_logo/org_logo_service.py`:
- Around line 95-100: The except block in org_logo_service.py catches Exception
as e when calling delete_org_logo(org.logo_s3_key) and re-raises OrgLogoS3Error
without preserving the original exception chain; update the re-raise to use
"raise OrgLogoS3Error('Failed to delete logo from S3') from e" so the original
traceback and context are preserved for debugging while keeping the existing log
call (logger.error(f"S3 delete failed for {org.slug}: {e}")).
- Around line 41-51: The exception handlers in the upload block for
upload_org_logo currently swallow the original exceptions; update the except
clauses to preserve the exception chain by re-raising domain errors using "raise
... from e" (i.e., change the OrgLogoValidationError raise to "raise
OrgLogoValidationError(str(e)) from e" and change the OrgLogoS3Error raise to
"raise OrgLogoS3Error(\"Failed to upload logo to S3\") from e"), keeping the
existing logger.error call for S3 failures so the original traceback is
available for debugging.
In `@ddpui/utils/s3_utils.py`:
- Line 61: The ValueError message uses an unnecessary f-string prefix; replace
raise ValueError(f"File size exceeds the 5MB limit") with a plain string (remove
the leading f) so it becomes raise ValueError("File size exceeds the 5MB limit")
in the file size check (the raise ValueError(...) statement in
ddpui/utils/s3_utils.py).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d6c6fdb2-8c12-4434-9826-97ae0ec5a0a4
📒 Files selected for processing (12)
.env.templateddpui/api/org_logo_api.pyddpui/api/public_api.pyddpui/core/org_logo/__init__.pyddpui/core/org_logo/exceptions.pyddpui/core/org_logo/org_logo_service.pyddpui/core/reports/pdf_export_service.pyddpui/migrations/0164_org_logo.pyddpui/models/org.pyddpui/routes.pyddpui/schemas/org_schema.pyddpui/utils/s3_utils.py
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1406 +/- ##
==========================================
+ Coverage 60.26% 60.27% +0.01%
==========================================
Files 146 148 +2
Lines 17222 17449 +227
==========================================
+ Hits 10378 10517 +139
- Misses 6844 6932 +88 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ddpui/tests/core/org_logo/test_org_logo_service.py (1)
1-185:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRun Black and commit the reformatted file.
CI indicates this file is reformatted by Black during checks.
🤖 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 `@ddpui/tests/core/org_logo/test_org_logo_service.py` around lines 1 - 185, The test file ddpui/tests/core/org_logo/test_org_logo_service.py is not formatted to Black; run the project's Black formatter and commit the changes. Specifically, run the repository's formatting command (e.g., black . or the configured pre-commit/format script) to reformat this file, verify tests referencing OrgLogoService and its test functions still pass, stage the updated test_org_logo_service.py, and create a commit containing the Black-applied changes.Source: Pipeline failures
ddpui/tests/api_tests/test_org_logo_api.py (1)
1-145:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRun Black and commit the reformatted file.
CI reports this file is not Black-formatted and gets rewritten by pre-commit, which blocks checks.
🤖 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 `@ddpui/tests/api_tests/test_org_logo_api.py` around lines 1 - 145, This file fails Black formatting; run the Black formatter and commit the updated file so pre-commit stops rewriting it — e.g., format ddpui.tests.api_tests.test_org_logo_api (the file containing test_get_org_logo_not_found_raises_404, test_upload_logo_file_validation_error_raises_400, test_upload_logo_file_s3_error_raises_502, test_upload_logo_from_url_validation_error_raises_400, test_delete_logo_not_found_raises_404, test_delete_logo_s3_error_raises_502 and imports like get_org_logo/upload_logo_file/upload_logo_from_url/delete_logo) with black (or black .) and commit the reformatted file.Source: Pipeline failures
🧹 Nitpick comments (2)
ddpui/tests/api_tests/test_org_logo_api.py (1)
117-127: ⚡ Quick winAdd a test for the upload-from-URL fallback 500 path.
upload_logo_from_urlinddpui/api/org_logo_api.pyalso maps unexpected exceptions toHttpError(500), but this contract isn’t covered here. Add one test that patchesOrgLogoService.upload_logo_from_urlto raise a genericExceptionand assert status500.🤖 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 `@ddpui/tests/api_tests/test_org_logo_api.py` around lines 117 - 127, Add a new test in ddpui/tests/api_tests/test_org_logo_api.py that patches OrgLogoService.upload_logo_from_url to raise a generic Exception and then calls the API-level upload_logo_from_url (using mock_request(orguser) and OrgLogoUrlPayload) asserting that the raised HttpError has status_code == 500; reference the existing test_upload_logo_from_url_validation_error_raises_400 for structure and reuse mock_request, orguser fixture, HttpError, and OrgLogoUrlPayload names.ddpui/tests/core/org_logo/test_org_logo_service.py (1)
178-185: ⚡ Quick winStrengthen the S3-delete-failure test with state assertions.
After asserting
OrgLogoS3Error, also refresh and assertlogo_url/logo_s3_key/logo_filenameare unchanged. That protects the contract that failed S3 deletion must not clear DB fields.🤖 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 `@ddpui/tests/core/org_logo/test_org_logo_service.py` around lines 178 - 185, Update the test_delete_logo_s3_error_raises test to, after the pytest.raises(OrgLogoS3Error) block, refresh the org instance from the DB and assert that org_with_logo.logo_url, org_with_logo.logo_s3_key, and org_with_logo.logo_filename remain unchanged; specifically call org_with_logo.refresh_from_db() and compare those three attributes to their original values captured before calling OrgLogoService.delete_logo to guarantee a failed S3 delete does not clear DB fields.
🤖 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.
Outside diff comments:
In `@ddpui/tests/api_tests/test_org_logo_api.py`:
- Around line 1-145: This file fails Black formatting; run the Black formatter
and commit the updated file so pre-commit stops rewriting it — e.g., format
ddpui.tests.api_tests.test_org_logo_api (the file containing
test_get_org_logo_not_found_raises_404,
test_upload_logo_file_validation_error_raises_400,
test_upload_logo_file_s3_error_raises_502,
test_upload_logo_from_url_validation_error_raises_400,
test_delete_logo_not_found_raises_404, test_delete_logo_s3_error_raises_502 and
imports like get_org_logo/upload_logo_file/upload_logo_from_url/delete_logo)
with black (or black .) and commit the reformatted file.
In `@ddpui/tests/core/org_logo/test_org_logo_service.py`:
- Around line 1-185: The test file
ddpui/tests/core/org_logo/test_org_logo_service.py is not formatted to Black;
run the project's Black formatter and commit the changes. Specifically, run the
repository's formatting command (e.g., black . or the configured
pre-commit/format script) to reformat this file, verify tests referencing
OrgLogoService and its test functions still pass, stage the updated
test_org_logo_service.py, and create a commit containing the Black-applied
changes.
---
Nitpick comments:
In `@ddpui/tests/api_tests/test_org_logo_api.py`:
- Around line 117-127: Add a new test in
ddpui/tests/api_tests/test_org_logo_api.py that patches
OrgLogoService.upload_logo_from_url to raise a generic Exception and then calls
the API-level upload_logo_from_url (using mock_request(orguser) and
OrgLogoUrlPayload) asserting that the raised HttpError has status_code == 500;
reference the existing test_upload_logo_from_url_validation_error_raises_400 for
structure and reuse mock_request, orguser fixture, HttpError, and
OrgLogoUrlPayload names.
In `@ddpui/tests/core/org_logo/test_org_logo_service.py`:
- Around line 178-185: Update the test_delete_logo_s3_error_raises test to,
after the pytest.raises(OrgLogoS3Error) block, refresh the org instance from the
DB and assert that org_with_logo.logo_url, org_with_logo.logo_s3_key, and
org_with_logo.logo_filename remain unchanged; specifically call
org_with_logo.refresh_from_db() and compare those three attributes to their
original values captured before calling OrgLogoService.delete_logo to guarantee
a failed S3 delete does not clear DB fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 29bacfd5-079d-4fc5-a181-ab321ba8bdf8
📒 Files selected for processing (5)
ddpui/core/reports/pdf_export_service.pyddpui/tests/api_tests/test_org_logo_api.pyddpui/tests/core/org_logo/__init__.pyddpui/tests/core/org_logo/test_org_logo_service.pyddpui/tests/utils/test_s3_utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ddpui/core/reports/pdf_export_service.py
…ogo byte fetching logic
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ddpui/core/org_logo/org_logo_service.py (1)
72-94:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCritical SSRF vulnerability: validate and restrict logo URLs before saving.
upload_logo_from_urlaccepts anyimage_urlstring and writes it directly toorg.logo_urlwithout validation. An attacker can supply internal URLs (http://localhost,http://169.254.169.254/latest/meta-data/, private IP ranges) and later trigger server-side requests to those URLs via the newGET /proxy/endpoint (which callsget_logo_bytes), leaking internal metadata, services, or cloud credentials.🛡️ Proposed fix: validate URL scheme and destination
Add validation before line 89 to reject non-HTTP(S) schemes and private/internal destinations:
`@staticmethod` def upload_logo_from_url(image_url: str, org: Org) -> Org: """Store an external image URL directly in the DB — no S3 upload. Args: image_url: Public URL of the image to use as logo org: The organization Returns: Updated Org instance """ + # Validate URL to prevent SSRF + from urllib.parse import urlparse + import ipaddress + + parsed = urlparse(image_url) + if parsed.scheme not in ("http", "https"): + raise OrgLogoValidationError("Only HTTP and HTTPS URLs are allowed") + + # Block private/internal IPs + try: + import socket + host = parsed.hostname + if not host: + raise OrgLogoValidationError("Invalid URL: missing hostname") + + # Resolve hostname to IP + ip = socket.gethostbyname(host) + ip_obj = ipaddress.ip_address(ip) + + if ip_obj.is_private or ip_obj.is_loopback or ip_obj.is_link_local: + raise OrgLogoValidationError("URLs pointing to private or internal networks are not allowed") + except socket.gaierror: + raise OrgLogoValidationError("Unable to resolve hostname") + # Delete old S3 file if the previous logo was uploaded (not a URL) if org.logo_s3_key:🤖 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 `@ddpui/core/org_logo/org_logo_service.py` around lines 72 - 94, The upload_logo_from_url function currently saves any image_url directly to org.logo_url; validate and sanitize the URL before saving by (1) parsing image_url (e.g., via urllib.parse) and rejecting non-http/https schemes and suspicious schemes like file:, data:, ftp:, (2) resolve and validate the hostname to an IP (using DNS resolution + ipaddress) and reject private, loopback, link-local, multicast, or known cloud metadata IPs/ranges (e.g., 169.254.169.254/32) and localhost names, and (3) optionally restrict to a safe allowlist of hostnames or require a public IP. If validation fails, raise/return a validation error and do not modify org; only proceed to clear org.logo_s3_key/logo_filename, set org.logo_url and call org.save(update_fields=[...]) when the URL passes validation. Ensure the checks are applied inside upload_logo_from_url before changing org and before logging.
🧹 Nitpick comments (2)
ddpui/api/org_logo_api.py (2)
104-115: ⚡ Quick winAdd cache headers to reduce repeated logo fetches.
The
/proxy/endpoint fetches the logo from external storage on every request. If logos are immutable or change infrequently, addingCache-Controlheaders would reduce backend load and improve client performance.⚡ Proposed enhancement
try: image_bytes, content_type = OrgLogoService.get_logo_bytes(orguser.org) - return HttpResponse(image_bytes, content_type=content_type) + response = HttpResponse(image_bytes, content_type=content_type) + # Cache for 1 hour (adjust based on logo update frequency) + response["Cache-Control"] = "public, max-age=3600" + return response except OrgLogoNotFoundError as e:Alternatively, use
ETagorLast-Modifiedheaders if you track logo update timestamps.🤖 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 `@ddpui/api/org_logo_api.py` around lines 104 - 115, The proxy_org_logo_image endpoint currently returns raw bytes but lacks caching headers; update the HttpResponse returned in proxy_org_logo_image (inside the try block after OrgLogoService.get_logo_bytes) to include appropriate cache headers (e.g., set "Cache-Control: public, max-age=86400, immutable" or another suitable max-age) and optionally add ETag or Last-Modified headers if you can derive a version/timestamp from OrgLogoService or org (use OrgLogoService.get_logo_bytes return metadata or another method to compute an ETag); ensure headers are set on the HttpResponse before returning and keep existing error handling (OrgLogoNotFoundError, OrgLogoS3Error) intact.
82-83: 💤 Low valueRemove unreachable exception handler.
The service method
OrgLogoService.upload_logo_from_url(inddpui/core/org_logo/org_logo_service.pylines 72-94) performs only DB writes and S3 deletion; it never raisesOrgLogoValidationError. This catch block will never execute.♻️ Proposed cleanup
return api_response( success=True, data=OrgLogoResponse.from_model(org), message="Logo URL saved successfully", ) - except OrgLogoValidationError as e: - raise HttpError(400, str(e)) from e except Exception as e:Note: If you accept the SSRF fix proposed in
ddpui/core/org_logo/org_logo_service.py(lines 72-94), the service will raiseOrgLogoValidationErrorfor invalid URLs, so this handler should be kept.🤖 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 `@ddpui/api/org_logo_api.py` around lines 82 - 83, The except block catching OrgLogoValidationError in the upload endpoint should be removed because OrgLogoService.upload_logo_from_url does not raise OrgLogoValidationError; delete the except OrgLogoValidationError handler and its raise HttpError(400, ...) line from the upload handler (the code referencing OrgLogoValidationError in org_logo_api.py) so only relevant exceptions are handled or let higher-level error middleware handle unexpected errors; if you later apply the SSRF validation change inside OrgLogoService.upload_logo_from_url that begins raising OrgLogoValidationError, reintroduce this specific catch to convert it to HttpError(400).
🤖 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 `@ddpui/core/org_logo/org_logo_service.py`:
- Around line 31-35: Raise a new, non-S3-specific exception when HTTP fetching
fails: add OrgLogoFetchError(OrgLogoError) (message + code
"ORG_LOGO_FETCH_ERROR") in exceptions.py, then in the code that performs
http_requests.get(org.logo_url, ...) replace the raised OrgLogoS3Error calls
with OrgLogoFetchError for both the RequestException branch and the resp.ok
check so failures from external HTTP(S) endpoints are correctly classified;
finally update the API layer error handling (where OrgLogoS3Error is currently
caught) to also handle or map OrgLogoFetchError so callers receive the
appropriate error code.
- Line 31: Validate org.logo_url’s URL scheme before calling http_requests.get:
parse org.logo_url (e.g., using urllib.parse.urlparse or
requests.utils.urlparse) and ensure scheme is exactly "http" or "https"; if not,
log or handle the invalid URL and skip the fetch instead of calling
http_requests.get(org.logo_url, timeout=10). Update the code around the resp =
http_requests.get(...) call to perform this check and only call
http_requests.get when the scheme is allowed, returning an appropriate
error/None when rejected.
- Around line 25-37: The get_logo_bytes function currently reads resp.content
with no size limit; change the fetch to request a streamed response (use
http_requests.get(..., stream=True)) and read the body incrementally (e.g., via
resp.iter_content or chunked reads) up to a fixed maximum byte cap (e.g., a few
MB), closing the response and raising OrgLogoS3Error if the body exceeds that
cap or on read errors; keep using org.logo_url and the existing timeout and
preserve returning a (bytes, content_type) tuple when within limit.
---
Outside diff comments:
In `@ddpui/core/org_logo/org_logo_service.py`:
- Around line 72-94: The upload_logo_from_url function currently saves any
image_url directly to org.logo_url; validate and sanitize the URL before saving
by (1) parsing image_url (e.g., via urllib.parse) and rejecting non-http/https
schemes and suspicious schemes like file:, data:, ftp:, (2) resolve and validate
the hostname to an IP (using DNS resolution + ipaddress) and reject private,
loopback, link-local, multicast, or known cloud metadata IPs/ranges (e.g.,
169.254.169.254/32) and localhost names, and (3) optionally restrict to a safe
allowlist of hostnames or require a public IP. If validation fails, raise/return
a validation error and do not modify org; only proceed to clear
org.logo_s3_key/logo_filename, set org.logo_url and call
org.save(update_fields=[...]) when the URL passes validation. Ensure the checks
are applied inside upload_logo_from_url before changing org and before logging.
---
Nitpick comments:
In `@ddpui/api/org_logo_api.py`:
- Around line 104-115: The proxy_org_logo_image endpoint currently returns raw
bytes but lacks caching headers; update the HttpResponse returned in
proxy_org_logo_image (inside the try block after OrgLogoService.get_logo_bytes)
to include appropriate cache headers (e.g., set "Cache-Control: public,
max-age=86400, immutable" or another suitable max-age) and optionally add ETag
or Last-Modified headers if you can derive a version/timestamp from
OrgLogoService or org (use OrgLogoService.get_logo_bytes return metadata or
another method to compute an ETag); ensure headers are set on the HttpResponse
before returning and keep existing error handling (OrgLogoNotFoundError,
OrgLogoS3Error) intact.
- Around line 82-83: The except block catching OrgLogoValidationError in the
upload endpoint should be removed because OrgLogoService.upload_logo_from_url
does not raise OrgLogoValidationError; delete the except OrgLogoValidationError
handler and its raise HttpError(400, ...) line from the upload handler (the code
referencing OrgLogoValidationError in org_logo_api.py) so only relevant
exceptions are handled or let higher-level error middleware handle unexpected
errors; if you later apply the SSRF validation change inside
OrgLogoService.upload_logo_from_url that begins raising OrgLogoValidationError,
reintroduce this specific catch to convert it to HttpError(400).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 325d075f-bd31-4b6b-a19f-36d395340d5d
📒 Files selected for processing (4)
ddpui/api/org_logo_api.pyddpui/core/org_logo/org_logo_service.pyddpui/tests/api_tests/test_org_logo_api.pyddpui/tests/core/org_logo/test_org_logo_service.py
🚧 Files skipped from review as they are similar to previous changes (2)
- ddpui/tests/api_tests/test_org_logo_api.py
- ddpui/tests/core/org_logo/test_org_logo_service.py
| if not org.logo_url: | ||
| raise OrgLogoNotFoundError() | ||
| try: | ||
| resp = http_requests.get(org.logo_url, timeout=10) |
There was a problem hiding this comment.
Restrict URL schemes to HTTP(S) only.
requests.get(org.logo_url, ...) will follow any scheme supported by the requests library (including file://, ftp://, etc.). If org.logo_url contains a non-HTTP(S) scheme, this could expose local files or trigger unintended protocol handlers.
🔒 Proposed fix: validate scheme before fetch
+ from urllib.parse import urlparse
+
if not org.logo_url:
raise OrgLogoNotFoundError()
+
+ parsed = urlparse(org.logo_url)
+ if parsed.scheme not in ("http", "https"):
+ raise OrgLogoValidationError(f"Unsupported URL scheme: {parsed.scheme}")
+
try:
resp = http_requests.get(org.logo_url, timeout=10)🤖 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 `@ddpui/core/org_logo/org_logo_service.py` at line 31, Validate org.logo_url’s
URL scheme before calling http_requests.get: parse org.logo_url (e.g., using
urllib.parse.urlparse or requests.utils.urlparse) and ensure scheme is exactly
"http" or "https"; if not, log or handle the invalid URL and skip the fetch
instead of calling http_requests.get(org.logo_url, timeout=10). Update the code
around the resp = http_requests.get(...) call to perform this check and only
call http_requests.get when the scheme is allowed, returning an appropriate
error/None when rejected.
…to handle HTTP errors
himanshudube97
approved these changes
Jun 11, 2026
himanshudube97
self-requested a review
June 11, 2026 06:19
| except Exception as e: | ||
| logger.warning(f"Failed to delete old S3 logo for {org.slug}: {e}") | ||
|
|
||
| org.logo_url = image_url |
Contributor
There was a problem hiding this comment.
we need to add some validations to this url and make sure its an image.
people might set random urls. what if its a script ?
Contributor
Author
There was a problem hiding this comment.
Added URL scheme validation (HTTP/HTTPS only) and a HEAD request check to verify the Content-Type is a valid image type before saving.
Contributor
There was a problem hiding this comment.
Have you tested it out by adding some url that is not an imaeg ?
… and retrieval logic
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ddpui/api/org_logo_api.py (1)
35-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid unbounded in-memory file reads in upload endpoint.
Line 35 reads the entire payload before size validation. A large upload can consume worker memory unnecessarily before the 5MB cap is enforced.
Use a bounded read (or pre-check
file.size) and reject oversized payloads before fully materializing bytes.Suggested direction
- org = OrgLogoService.upload_logo_from_file( - file_bytes=file.read(), + max_bytes = 5 * 1024 * 1024 + if getattr(file, "size", None) and file.size > max_bytes: + raise HttpError(400, "File size exceeds the 5MB limit") + file_bytes = file.read(max_bytes + 1) + if len(file_bytes) > max_bytes: + raise HttpError(400, "File size exceeds the 5MB limit") + org = OrgLogoService.upload_logo_from_file( + file_bytes=file_bytes, content_type=file.content_type or "", filename=file.name or "", org=orguser.org, )🤖 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 `@ddpui/api/org_logo_api.py` at line 35, The file upload in the org logo endpoint reads the entire file content into memory with file.read() before validating the file size against the 5MB limit. Instead of reading the full file first, check the file size before materializing the bytes in memory by accessing the file.size attribute (or Content-Length header from the request). If the file size exceeds 5MB, reject it immediately and return an appropriate error response. Only proceed to call file.read() after confirming the file size is within the acceptable limit.
🧹 Nitpick comments (2)
ddpui/tests/api_tests/test_org_logo_api.py (1)
78-138: ⚡ Quick winAdd API mapping tests for the
/proxy/error path.Current tests do not cover
proxy_org_logo_imagemappingOrgLogoFetchError/OrgLogoS3Errorto HTTP 502 (implemented inddpui/api/org_logo_api.pyLine 99-100). Add these to lock the contract.🤖 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 `@ddpui/tests/api_tests/test_org_logo_api.py` around lines 78 - 138, Add two new test functions following the existing pattern: one that tests proxy_org_logo_image when OrgLogoFetchError is raised and expects HTTP 502, and another that tests proxy_org_logo_image when OrgLogoS3Error is raised and expects HTTP 502. Follow the same structure as the existing test functions like test_upload_logo_file_s3_error_raises_502 and test_delete_logo_s3_error_raises_502, patching the OrgLogoService.fetch_logo_stream method to raise each exception, then asserting the returned HttpError has status_code 502.ddpui/tests/core/org_logo/test_org_logo_service.py (1)
57-206: ⚡ Quick winAdd focused tests for
get_logo_bytesbehavior.
get_logo_byteshas critical logic (streaming read, 5MB cap, fetch/read error mapping), but this file has no assertions for it. Please add coverage for success, oversize response, and upstream failure branches.🤖 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 `@ddpui/tests/core/org_logo/test_org_logo_service.py` around lines 57 - 206, The test file is missing test coverage for the get_logo_bytes method which contains critical logic for streaming reads, file size validation, and error handling. Add three new test functions to this file: one that tests successful retrieval of logo bytes, one that tests the behavior when the response exceeds the 5MB size limit, and one that tests error handling when upstream fetch or read operations fail. Each test should use appropriate mocks for the underlying S3 or HTTP operations and verify both the returned data and any exceptions raised match the expected behavior of get_logo_bytes.
🤖 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 @.env.template:
- Line 69: The S3_IMAGES_BUCKET variable assignment in the .env.template file
contains excessive whitespace between the equals sign and the inline comment,
which can cause inconsistent parsing by dotenv tools. Remove the extra
whitespace after the equals sign so there is minimal spacing before the inline
comment character, ensuring the line follows a consistent and parseable format
that other environment variable assignments in the file use.
In `@ddpui/core/org_logo/org_logo_service.py`:
- Around line 46-47: The dalgo_get call at line 46 fetches org.logo_url
server-side without validating that the target resolves to a public IP address,
and the validation at lines 121-127 only checks the URL scheme, leaving the code
vulnerable to SSRF attacks via internal hosts and redirects. Add strict
validation before the dalgo_get call to ensure the resolved IP address is not in
private ranges (localhost, link-local, private CIDR blocks), and either disable
redirects in the dalgo_get call by setting allow_redirects=False or validate
each redirect target to ensure it also resolves to a public IP before following
it.
---
Outside diff comments:
In `@ddpui/api/org_logo_api.py`:
- Line 35: The file upload in the org logo endpoint reads the entire file
content into memory with file.read() before validating the file size against the
5MB limit. Instead of reading the full file first, check the file size before
materializing the bytes in memory by accessing the file.size attribute (or
Content-Length header from the request). If the file size exceeds 5MB, reject it
immediately and return an appropriate error response. Only proceed to call
file.read() after confirming the file size is within the acceptable limit.
---
Nitpick comments:
In `@ddpui/tests/api_tests/test_org_logo_api.py`:
- Around line 78-138: Add two new test functions following the existing pattern:
one that tests proxy_org_logo_image when OrgLogoFetchError is raised and expects
HTTP 502, and another that tests proxy_org_logo_image when OrgLogoS3Error is
raised and expects HTTP 502. Follow the same structure as the existing test
functions like test_upload_logo_file_s3_error_raises_502 and
test_delete_logo_s3_error_raises_502, patching the
OrgLogoService.fetch_logo_stream method to raise each exception, then asserting
the returned HttpError has status_code 502.
In `@ddpui/tests/core/org_logo/test_org_logo_service.py`:
- Around line 57-206: The test file is missing test coverage for the
get_logo_bytes method which contains critical logic for streaming reads, file
size validation, and error handling. Add three new test functions to this file:
one that tests successful retrieval of logo bytes, one that tests the behavior
when the response exceeds the 5MB size limit, and one that tests error handling
when upstream fetch or read operations fail. Each test should use appropriate
mocks for the underlying S3 or HTTP operations and verify both the returned data
and any exceptions raised match the expected behavior of get_logo_bytes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4aa948e-8f95-452c-9b38-1139e7b9c6d5
📒 Files selected for processing (10)
.env.templateddpui/api/org_logo_api.pyddpui/core/org_logo/exceptions.pyddpui/core/org_logo/org_logo_service.pyddpui/ddpdbt/elementary_service.pyddpui/tests/api_tests/test_org_logo_api.pyddpui/tests/core/org_logo/test_org_logo_service.pyddpui/tests/utils/test_s3_utils.pyddpui/utils/http.pyddpui/utils/s3_utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ddpui/core/org_logo/exceptions.py
| S3_AWS_SECRET_ACCESS_KEY= | ||
| ELEMENTARY_S3_BUCKET= | ||
| LATEST_ELEMENTARY_PACKAGE_VERSION= | ||
| S3_IMAGES_BUCKET= # shared bucket for all image uploads (logos, etc.) |
There was a problem hiding this comment.
Normalize S3_IMAGES_BUCKET assignment to avoid parser ambiguity.
Line 69 has significant whitespace after = before the inline comment, which can be interpreted inconsistently by dotenv parsers/tooling.
Suggested fix
-S3_IMAGES_BUCKET= # shared bucket for all image uploads (logos, etc.)
+# shared bucket for all image uploads (logos, etc.)
+S3_IMAGES_BUCKET=📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| S3_IMAGES_BUCKET= # shared bucket for all image uploads (logos, etc.) | |
| # shared bucket for all image uploads (logos, etc.) | |
| S3_IMAGES_BUCKET= |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 69-69: [SpaceCharacter] The line has spaces around equal sign
(SpaceCharacter)
[warning] 69-69: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
🤖 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 @.env.template at line 69, The S3_IMAGES_BUCKET variable assignment in the
.env.template file contains excessive whitespace between the equals sign and the
inline comment, which can cause inconsistent parsing by dotenv tools. Remove the
extra whitespace after the equals sign so there is minimal spacing before the
inline comment character, ensuring the line follows a consistent and parseable
format that other environment variable assignments in the file use.
Source: Linters/SAST tools
|
|
||
| @org_logo_router.get("/proxy/") | ||
| @has_permission(["can_view_orgusers"]) | ||
| def proxy_org_logo_image(request): |
Contributor
There was a problem hiding this comment.
why do we need this api ? aren't we storing the logo on s3 and the s3 url is being embedded on the frontend.
am i missing something ?
Contributor
Author
There was a problem hiding this comment.
The proxy exists for the chart download feature inside dashboards. When a user downloads a chart as PNG, the frontend calls /api/org/logo/proxy/ to fetch the logo and draws it onto an HTML Canvas to add branding. The browser enforces CORS when drawing an external-origin image (like an S3 URL) directly onto canvas — the canvas becomes "tainted" and canvas.toBlob() throws a security error, breaking the PNG download. The proxy sidesteps this by serving the image bytes from the same origin, so the canvas draw succeeds.
| org.logo_url = None | ||
| org.logo_s3_key = None | ||
| org.logo_filename = None | ||
| org.save(update_fields=["logo_url", "logo_s3_key", "logo_filename"]) |
Contributor
There was a problem hiding this comment.
what is difference between saving like this vs just doing org.save() ?
Contributor
Author
There was a problem hiding this comment.
update_fields only updates the specified columns in the DB, avoiding accidental overwrites of other fields changed concurrently. org.save() would update all fields.last
| } | ||
|
|
||
|
|
||
| @user_org_router.post("/org/logo/upload/", response=ApiResponse[OrgLogoResponse]) |
Contributor
There was a problem hiding this comment.
for the naming of routes we usually follow REST api practices. Can you read about it a bit ? and then rename the route accordingly.
as much as possible we should follow that.
| raise HttpError(500, "Failed to save logo URL") from e | ||
|
|
||
|
|
||
| @user_org_router.delete("/org/logo/", response=ApiResponse) |
Contributor
There was a problem hiding this comment.
here you have followed REST convention. nice !!
| raise HttpError(502, str(e)) from e | ||
|
|
||
|
|
||
| @user_org_router.get("/org/logo/proxy/") |
Contributor
There was a problem hiding this comment.
Same. Can you follow REST conventions in naming this route too?
We dont need the word proxy.
Contributor
Author
There was a problem hiding this comment.
updated , thanks
| os.environ["DJANGO_ALLOW_ASYNC_UNSAFE"] = "true" | ||
| django.setup() | ||
|
|
||
| from django.contrib.auth.models import User |
Contributor
There was a problem hiding this comment.
We should move these tests to user_org_api.py
We no longer have org_logo_api.py
…eature/org-logo-upload
…logo handling logic
Ishankoradia
approved these changes
Jun 24, 2026
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
S3_IMAGES_BUCKETplaceholder for shared image storage