fix: update trusted images policies#2622
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
📝 WalkthroughWalkthroughUpdates README image CSS to preserve aspect ratio for images with a width attribute. Revises server image-proxy trusted domains: removes GitHub-related domains, adds images.opencollective.com, and changes domain check to exact hostname membership only. Changes
🚥 Pre-merge checks | ✅ 4✅ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
server/utils/image-proxy.ts (1)
85-92:⚠️ Potential issue | 🔴 CriticalUpdate
test/unit/server/utils/image-proxy.spec.ts— three assertions encode the previous behaviour and will now fail.The implementation at lines 85–92 uses exact-hostname matching only (
.includes(hostname)), andgithub.comanduser-images.githubusercontent.comhave been removed fromTRUSTED_IMAGE_DOMAINS. Three assertions in the spec now encode stale expectations:
it('trusts GitHub user images', …)at ~line 22: Assertsuser-images.githubusercontent.comis trusted; it is no longer in the domain list.it('trusts subdomain of trusted domains', …)at ~line 40: Assertssub.gitlab.comis trusted; the new exact-match-only logic means onlygitlab.comitself is trusted, not its subdomains.it('does not proxy GitHub blob URLs', …)at ~line 268: Assertshttps://github.com/owner/repo/blob/main/assets/logo.pngis returned unchanged;github.comis no longer trusted, so it will now be rewritten as a proxied URL.Update those tests to reflect the new exact-match semantics and domain-list changes (consider inverting assertion
#2to verify thatsub.gitlab.comis not trusted).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/utils/image-proxy.ts` around lines 85 - 92, The tests in test/unit/server/utils/image-proxy.spec.ts must be updated to match the new exact-hostname matching in isTrustedImageDomain and the trimmed TRUSTED_IMAGE_DOMAINS list: change the assertion in the "trusts GitHub user images" test to no longer expect user-images.githubusercontent.com to be trusted (adjust to expect false or remove the test), invert the "trusts subdomain of trusted domains" test so it asserts sub.gitlab.com is NOT trusted (since only exact gitlab.com is allowed), and update the "does not proxy GitHub blob URLs" test to expect that https://github.com/.../blob/... is proxied/rewritten (or otherwise handled) because github.com was removed from the trusted list; ensure all assertions reference the isTrustedImageDomain and proxy-rewrite helpers used in the spec.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@server/utils/image-proxy.ts`:
- Around line 85-92: The tests in test/unit/server/utils/image-proxy.spec.ts
must be updated to match the new exact-hostname matching in isTrustedImageDomain
and the trimmed TRUSTED_IMAGE_DOMAINS list: change the assertion in the "trusts
GitHub user images" test to no longer expect user-images.githubusercontent.com
to be trusted (adjust to expect false or remove the test), invert the "trusts
subdomain of trusted domains" test so it asserts sub.gitlab.com is NOT trusted
(since only exact gitlab.com is allowed), and update the "does not proxy GitHub
blob URLs" test to expect that https://github.com/.../blob/... is
proxied/rewritten (or otherwise handled) because github.com was removed from the
trusted list; ensure all assertions reference the isTrustedImageDomain and
proxy-rewrite helpers used in the spec.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 04cc0fba-745d-441a-8e34-1f103fc7e6df
📒 Files selected for processing (2)
app/components/Readme.vueserver/utils/image-proxy.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/unit/server/utils/image-proxy.spec.ts (1)
33-35: Minor coverage regression / gap.Per the AI summary, the previous explicit negative case for
user-images.githubusercontent.comwas removed together with the subdomain-trust change. Since that host is a concrete, real-world example of a GitHub-related subdomain that is deliberately not trusted (it redirects to S3), it's worth keeping a dedicated test for it so a future regression that re-adds subdomain matching — or re-adds the domain — is caught.Additionally, the PR adds
images.opencollective.comtoTRUSTED_IMAGE_DOMAINS, but there is no positive test asserting it is trusted. Consider adding one alongside the existing trusted-domain cases.💡 Suggested additions
it('does not trust subdomain of trusted domains', () => { expect(isTrustedImageDomain('https://sub.gitlab.com/image.png')).toBe(false) + // user-images.githubusercontent.com redirects to S3 and must stay untrusted. + expect( + isTrustedImageDomain('https://user-images.githubusercontent.com/123/abc.png'), + ).toBe(false) }) + + it('trusts images.opencollective.com URLs', () => { + expect( + isTrustedImageDomain('https://images.opencollective.com/foo/avatar.png'), + ).toBe(true) + })Based on learnings that well-known badge services and trusted image hosts (including
opencollective.com) should be covered inTRUSTED_IMAGE_DOMAINSfor npmx.dev.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/server/utils/image-proxy.spec.ts` around lines 33 - 35, Add two test cases for isTrustedImageDomain: one negative case asserting 'https://user-images.githubusercontent.com/owner/repo/blob/master/image.png' (or similar user-images.githubusercontent.com URL) returns false to preserve the explicit non-trust for that GitHub subdomain, and one positive case asserting 'https://images.opencollective.com/logo.png' returns true to cover the newly added images.opencollective.com entry; place these alongside the existing trusted/untrusted domain tests in image-proxy.spec.ts so regressions in isTrustedImageDomain or TRUSTED_IMAGE_DOMAINS are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/unit/server/utils/image-proxy.spec.ts`:
- Around line 261-264: Update the test description or the tested URL so they
match: either rename the spec title string in the it(...) from "does not proxy
GitHub blob URLs" to "does not proxy GitHub-hosted asset URLs" to reflect the
URL used, or replace the URL with an actual GitHub blob URL (e.g., one
containing "/blob/") when calling toProxiedImageUrl(url, TEST_SECRET); ensure
the test still asserts the original behavior (expect(...).toBe(url)).
---
Nitpick comments:
In `@test/unit/server/utils/image-proxy.spec.ts`:
- Around line 33-35: Add two test cases for isTrustedImageDomain: one negative
case asserting
'https://user-images.githubusercontent.com/owner/repo/blob/master/image.png' (or
similar user-images.githubusercontent.com URL) returns false to preserve the
explicit non-trust for that GitHub subdomain, and one positive case asserting
'https://images.opencollective.com/logo.png' returns true to cover the newly
added images.opencollective.com entry; place these alongside the existing
trusted/untrusted domain tests in image-proxy.spec.ts so regressions in
isTrustedImageDomain or TRUSTED_IMAGE_DOMAINS are caught.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c827af88-ebf9-4328-b5b6-702276851459
📒 Files selected for processing (1)
test/unit/server/utils/image-proxy.spec.ts
2 participants
🔗 Linked issue
Resolves #2611
🧭 Context
I accidentally noticed that our images weren't loading. This was due to different checks: in the client, we were checking for subdomains, but the CSP was set to exact match. I made our parser also check for exact match (https://npmxdev-git-fork-alexdln-fix-images-proxy-npmx.vercel.app/package/eslint-visitor-keys)
I also saw that there's a related open bug - github redirects to S3, causing CSP errors for these images. I removed the redirected domains from the allow list. The other GitHub subdomains seem to be working correctly (https://npmxdev-git-fork-alexdln-fix-images-proxy-npmx.vercel.app/package/effect-web-midi/v/0.2.1)
And another small one from the same issue above: if an image has a specified width and height, we change the width to 100%, but the height remains the user-specified value, and the image stretches. Adjusted this so that if the height is specified, the height will change to auto, and the browser will automatically set the desired height since it knows the image's aspect ratio from the attributes