-
Notifications
You must be signed in to change notification settings - Fork 113
fix(security): proxy external markdown images via Cloudinary #3542
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(security): proxy external markdown images via Cloudinary #3542
Conversation
When users view posts containing external images in markdown, their browsers make direct requests to those external servers, exposing IP addresses and user-agent information. This change implements a server-side image proxy using Cloudinary's fetch capability to cache and serve external images through media.daily.dev. Key features: - Create imageProxy utility with signed Cloudinary fetch URLs - Validate URLs: only HTTP/HTTPS protocols, block private IPs (SSRF prevention) - Skip proxying for already-allowed domains (media.daily.dev, res.cloudinary.com) - Hook into markdown-it's image renderer to transform external image URLs - Block invalid/malicious URLs by setting empty src attribute Security considerations: - Signed URLs prevent CDN quota abuse (only backend can generate valid URLs) - SSRF protection via private IP blocking (127.x, 10.x, 192.168.x, etc.) - Protocol validation prevents file:// and other dangerous protocols - URL length limit prevents abuse Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add explicit cloudinary SDK configuration in test beforeEach hooks - Fix test expectations for file:// protocol (markdown-it doesn't render them as images) - Fix test expectations for proxied URLs (original URL is included in fetch URL) - Add documentation comment explaining protocol handling in isExternalImageUrl Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
🍹 The Update (preview) for dailydotdev/api/prod (at eb097ff) was successful. Resource Changes Name Type Operation
+ vpc-native-api-clickhouse-migration-024dab72 kubernetes:batch/v1:Job create
~ vpc-native-update-tag-recommendations-cron kubernetes:batch/v1:CronJob update
~ vpc-native-generic-referral-reminder-cron kubernetes:batch/v1:CronJob update
~ vpc-native-private-deployment kubernetes:apps/v1:Deployment update
+ vpc-native-api-db-migration-024dab72 kubernetes:batch/v1:Job create
~ vpc-native-hourly-notification-cron kubernetes:batch/v1:CronJob update
~ vpc-native-user-posts-analytics-refresh-cron kubernetes:batch/v1:CronJob update
~ vpc-native-check-analytics-report-cron kubernetes:batch/v1:CronJob update
~ vpc-native-post-analytics-history-day-clickhouse-cron kubernetes:batch/v1:CronJob update
~ vpc-native-clean-zombie-opportunities-cron kubernetes:batch/v1:CronJob update
~ vpc-native-post-analytics-clickhouse-cron kubernetes:batch/v1:CronJob update
~ vpc-native-user-profile-updated-sync-cron kubernetes:batch/v1:CronJob update
- vpc-native-api-clickhouse-migration-a7439cef kubernetes:batch/v1:Job delete
- vpc-native-api-db-migration-a7439cef kubernetes:batch/v1:Job delete
~ vpc-native-clean-zombie-images-cron kubernetes:batch/v1:CronJob update
~ vpc-native-user-profile-analytics-history-clickhouse-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-trending-cron kubernetes:batch/v1:CronJob update
~ vpc-native-bg-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-update-source-public-threshold-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-tags-str-cron kubernetes:batch/v1:CronJob update
~ vpc-native-user-profile-analytics-clickhouse-cron kubernetes:batch/v1:CronJob update
~ vpc-native-clean-stale-user-transactions-cron kubernetes:batch/v1:CronJob update
~ vpc-native-clean-zombie-users-cron kubernetes:batch/v1:CronJob update
~ vpc-native-validate-active-users-cron kubernetes:batch/v1:CronJob update
~ vpc-native-temporal-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-clean-zombie-user-companies-cron kubernetes:batch/v1:CronJob update
~ vpc-native-ws-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-update-highlighted-views-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-views-cron kubernetes:batch/v1:CronJob update
~ vpc-native-personalized-digest-cron kubernetes:batch/v1:CronJob update
~ vpc-native-expire-super-agent-trial-cron kubernetes:batch/v1:CronJob update
~ vpc-native-personalized-digest-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-clean-gifted-plus-cron kubernetes:batch/v1:CronJob update
~ vpc-native-daily-digest-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-source-tag-view-cron kubernetes:batch/v1:CronJob update
~ vpc-native-calculate-top-readers-cron kubernetes:batch/v1:CronJob update
~ vpc-native-sync-subscription-with-cio-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-current-streak-cron kubernetes:batch/v1:CronJob update
~ vpc-native-generate-search-invites-cron kubernetes:batch/v1:CronJob update
... and 1 other changes |
| } | ||
|
|
||
| // Only consider http/https URLs as external (relative URLs and other protocols are skipped) | ||
| if (!url.startsWith('http://') && !url.startsWith('https://')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see we checked for this condition also in validateImageUrl. One of them is redundant
src/common/imageProxy.ts
Outdated
| * @param html The HTML content containing images | ||
| * @returns The HTML with external image URLs replaced with proxied URLs | ||
| */ | ||
| export function proxyImagesInHtml(html: string): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see that we use it anywhere. we can consider removing it
|
@claude review my PR comments and address them. Make sure you update the relevant AGENTS.md if needed to avoid making the same mistakes. Verify lint and relevant tests |
|
Claude finished @idoshamun's task in 2m 54s —— View job Addressing PR Review Comments ✓
Changes Made
SummaryAll PR review comments have been addressed. The changes remove ~110 lines of redundant/unused code and add documentation to prevent similar issues. CI will verify lint and tests pass. --- | Branch: eng-589-bug-user-ip-addresses-exposed-via-remote-images | Latest commit: 89d126e |
- Remove redundant protocol check in validateImageUrl (already filtered by isExternalImageUrl) - Remove unused proxyImagesInHtml function and its tests - Update AGENTS.md with best practices for avoiding dead code and redundant validation Co-authored-by: Ido Shamun <idoshamun@users.noreply.github.com>
Summary
Security measures
Closes ENG-589
Created by Huginn 🐦⬛