From e46a97ba4299f8b37fcdb2b3608f5ab5056f0996 Mon Sep 17 00:00:00 2001 From: Steven Date: Fri, 8 Apr 2022 12:04:27 -0400 Subject: [PATCH 1/5] Fix leaking internal config to user-defined `loader` prop in `next/image` --- packages/next/client/image.tsx | 46 +++++++++++++++---- .../basic/pages/loader-prop.js | 23 ++++++++++ .../image-component/basic/test/index.test.js | 11 +++++ 3 files changed, 70 insertions(+), 10 deletions(-) create mode 100644 test/integration/image-component/basic/pages/loader-prop.js diff --git a/packages/next/client/image.tsx b/packages/next/client/image.tsx index be335edc63213..6a13948a0a91b 100644 --- a/packages/next/client/image.tsx +++ b/packages/next/client/image.tsx @@ -40,13 +40,23 @@ type ImageConfig = ImageConfigComplete & { allSizes: number[] } export type ImageLoader = (resolverProps: ImageLoaderProps) => string export type ImageLoaderProps = { - config: Readonly + config?: Readonly src: string width: number quality?: number } -const loaders = new Map string>([ +// Do not export - this is an internal type only +// because `next.config.js` is only meant for the +// built-in loaders, not for a custom loader() prop. +type ImageLoaderPropsWithConfig = ImageLoaderProps & { + config: Readonly +} + +const loaders = new Map< + LoaderValue, + (props: ImageLoaderPropsWithConfig) => string +>([ ['default', defaultLoader], ['imgix', imgixLoader], ['cloudinary', cloudinaryLoader], @@ -269,7 +279,7 @@ function getInt(x: unknown): number | undefined { return undefined } -function defaultImageLoader(loaderProps: ImageLoaderProps) { +function defaultImageLoader(loaderProps: ImageLoaderPropsWithConfig) { const loaderKey = loaderProps.config?.loader || 'default' const load = loaders.get(loaderKey) if (load) { @@ -357,7 +367,6 @@ export default function Image({ objectPosition, onLoadingComplete, onError, - loader = defaultImageLoader, placeholder = 'empty', blurDataURL, ...all @@ -376,8 +385,21 @@ export default function Image({ // Override default layout if the user specified one: if (rest.layout) layout = rest.layout - // Remove property so it's not spread into image: - delete rest['layout'] + // Remove property so it's not spread on : + delete rest.layout + } + + let loader = defaultImageLoader as ImageLoader + if ('loader' in rest && rest.loader) { + const customImageLoader = rest.loader + loader = (obj) => { + // The config object is internal only so we must + // delete before invoking the user-defined loader() + delete obj.config + return customImageLoader(obj) + } + // Remove property so it's not spread on + delete rest.loader } let staticSrc = '' @@ -957,7 +979,7 @@ function imgixLoader({ src, width, quality, -}: ImageLoaderProps): string { +}: ImageLoaderPropsWithConfig): string { // Demo: https://static.imgix.net/daisy.png?auto=format&fit=max&w=300 const url = new URL(`${config.path}${normalizeSrc(src)}`) const params = url.searchParams @@ -973,7 +995,11 @@ function imgixLoader({ return url.href } -function akamaiLoader({ config, src, width }: ImageLoaderProps): string { +function akamaiLoader({ + config, + src, + width, +}: ImageLoaderPropsWithConfig): string { return `${config.path}${normalizeSrc(src)}?imwidth=${width}` } @@ -982,7 +1008,7 @@ function cloudinaryLoader({ src, width, quality, -}: ImageLoaderProps): string { +}: ImageLoaderPropsWithConfig): string { // Demo: https://res.cloudinary.com/demo/image/upload/w_300,c_limit,q_auto/turtles.jpg const params = ['f_auto', 'c_limit', 'w_' + width, 'q_' + (quality || 'auto')] const paramsString = params.join(',') + '/' @@ -1001,7 +1027,7 @@ function defaultLoader({ src, width, quality, -}: ImageLoaderProps): string { +}: ImageLoaderPropsWithConfig): string { if (process.env.NODE_ENV !== 'production') { const missingValues = [] diff --git a/test/integration/image-component/basic/pages/loader-prop.js b/test/integration/image-component/basic/pages/loader-prop.js new file mode 100644 index 0000000000000..111b1a3621110 --- /dev/null +++ b/test/integration/image-component/basic/pages/loader-prop.js @@ -0,0 +1,23 @@ +import Image from 'next/image' + +const LoaderExample = () => { + return ( +
+

Custom loader in both next.config.js and loader prop

+ { + if (config) { + return 'https://example.vercel.sh/error-unexpected-config' + } + return `https://example.vercel.sh/success/${src}?width=${width}` + }} + > +
+ ) +} + +export default LoaderExample diff --git a/test/integration/image-component/basic/test/index.test.js b/test/integration/image-component/basic/test/index.test.js index 413b4932af33b..fe5534ec072f5 100644 --- a/test/integration/image-component/basic/test/index.test.js +++ b/test/integration/image-component/basic/test/index.test.js @@ -317,6 +317,17 @@ describe('Image Component Tests', () => { await browser.elementById('basic-image').getAttribute('data-nimg') ).toBe('intrinsic') }) + it('should automatically load images if observer does not exist', async () => { + browser = await webdriver(appPort, '/loader-prop') + expect( + await browser.elementById('loader-prop-img').getAttribute('src') + ).toBe('https://example.vercel.sh/success/foo.jpg?width=300') + expect( + await browser.elementById('lazy-no-observer').getAttribute('srcset') + ).toBe( + 'https://example.vercel.sh/success/foo.jpg?width=1024 1x, https://example.vercel.sh/success/foo.jpg?width=2000 2x' + ) + }) }) describe('Client-side Image Component Tests', () => { beforeAll(async () => { From 37f8f178567ebf237cc14de24fcc6fbec3b6a7f5 Mon Sep 17 00:00:00 2001 From: Steven Date: Fri, 8 Apr 2022 12:41:11 -0400 Subject: [PATCH 2/5] Fix test --- test/integration/image-component/basic/pages/loader-prop.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/image-component/basic/pages/loader-prop.js b/test/integration/image-component/basic/pages/loader-prop.js index 111b1a3621110..17747c211ca03 100644 --- a/test/integration/image-component/basic/pages/loader-prop.js +++ b/test/integration/image-component/basic/pages/loader-prop.js @@ -9,13 +9,13 @@ const LoaderExample = () => { src="foo.jpg" width={300} height={400} - loader={(config, src, width) => { + loader={({ config, src, width }) => { if (config) { return 'https://example.vercel.sh/error-unexpected-config' } return `https://example.vercel.sh/success/${src}?width=${width}` }} - > + /> ) } From 998b3af49d72813425fe3036f046cac7ceab8338 Mon Sep 17 00:00:00 2001 From: Steven Date: Fri, 8 Apr 2022 12:59:58 -0400 Subject: [PATCH 3/5] Fix when loader = undefined --- packages/next/client/image.tsx | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/next/client/image.tsx b/packages/next/client/image.tsx index 6a13948a0a91b..950d5660e4132 100644 --- a/packages/next/client/image.tsx +++ b/packages/next/client/image.tsx @@ -390,13 +390,15 @@ export default function Image({ } let loader = defaultImageLoader as ImageLoader - if ('loader' in rest && rest.loader) { - const customImageLoader = rest.loader - loader = (obj) => { - // The config object is internal only so we must - // delete before invoking the user-defined loader() - delete obj.config - return customImageLoader(obj) + if ('loader' in rest) { + if (rest.loader) { + const customImageLoader = rest.loader + loader = (obj) => { + // The config object is internal only so we must + // delete before invoking the user-defined loader() + delete obj.config + return customImageLoader(obj) + } } // Remove property so it's not spread on delete rest.loader From 9ecf9e1a27612cf88567d2690de396724aa1c789 Mon Sep 17 00:00:00 2001 From: Steven Date: Fri, 8 Apr 2022 17:37:53 -0400 Subject: [PATCH 4/5] Fix test --- test/integration/image-component/basic/test/index.test.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/integration/image-component/basic/test/index.test.js b/test/integration/image-component/basic/test/index.test.js index fe5534ec072f5..3a44542c2a89c 100644 --- a/test/integration/image-component/basic/test/index.test.js +++ b/test/integration/image-component/basic/test/index.test.js @@ -317,15 +317,15 @@ describe('Image Component Tests', () => { await browser.elementById('basic-image').getAttribute('data-nimg') ).toBe('intrinsic') }) - it('should automatically load images if observer does not exist', async () => { + it('should not pass config to custom loader prop', async () => { browser = await webdriver(appPort, '/loader-prop') expect( await browser.elementById('loader-prop-img').getAttribute('src') - ).toBe('https://example.vercel.sh/success/foo.jpg?width=300') + ).toBe('https://example.vercel.sh/success/foo.jpg?width=1024') expect( - await browser.elementById('lazy-no-observer').getAttribute('srcset') + await browser.elementById('loader-prop-img').getAttribute('srcset') ).toBe( - 'https://example.vercel.sh/success/foo.jpg?width=1024 1x, https://example.vercel.sh/success/foo.jpg?width=2000 2x' + 'https://example.vercel.sh/success/foo.jpg?width=480 1x, https://example.vercel.sh/success/foo.jpg?width=1024 2x' ) }) }) From b41388d0662d772575768648d602ab73725f1f7b Mon Sep 17 00:00:00 2001 From: Steven Date: Fri, 8 Apr 2022 18:01:42 -0400 Subject: [PATCH 5/5] Add new internal type `ImageLoaderWithConfig` --- packages/next/client/image.tsx | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/packages/next/client/image.tsx b/packages/next/client/image.tsx index 950d5660e4132..5eea24447eab3 100644 --- a/packages/next/client/image.tsx +++ b/packages/next/client/image.tsx @@ -40,7 +40,6 @@ type ImageConfig = ImageConfigComplete & { allSizes: number[] } export type ImageLoader = (resolverProps: ImageLoaderProps) => string export type ImageLoaderProps = { - config?: Readonly src: string width: number quality?: number @@ -49,6 +48,9 @@ export type ImageLoaderProps = { // Do not export - this is an internal type only // because `next.config.js` is only meant for the // built-in loaders, not for a custom loader() prop. +type ImageLoaderWithConfig = ( + resolverProps: ImageLoaderPropsWithConfig +) => string type ImageLoaderPropsWithConfig = ImageLoaderProps & { config: Readonly } @@ -142,7 +144,7 @@ export type ImageProps = Omit< onLoadingComplete?: OnLoadingComplete } -type ImageElementProps = Omit & { +type ImageElementProps = Omit & { srcString: string imgAttributes: GenImgAttrsResult heightInt: number | undefined @@ -155,7 +157,7 @@ type ImageElementProps = Omit & { loading: LoadingValue config: ImageConfig unoptimized: boolean - loader: ImageLoader + loader: ImageLoaderWithConfig placeholder: PlaceholderValue onLoadingCompleteRef: React.MutableRefObject setBlurComplete: (b: boolean) => void @@ -219,7 +221,7 @@ type GenImgAttrsData = { src: string unoptimized: boolean layout: LayoutValue - loader: ImageLoader + loader: ImageLoaderWithConfig width?: number quality?: number sizes?: string @@ -389,15 +391,15 @@ export default function Image({ delete rest.layout } - let loader = defaultImageLoader as ImageLoader + let loader: ImageLoaderWithConfig = defaultImageLoader if ('loader' in rest) { if (rest.loader) { const customImageLoader = rest.loader loader = (obj) => { + const { config: _, ...opts } = obj // The config object is internal only so we must - // delete before invoking the user-defined loader() - delete obj.config - return customImageLoader(obj) + // not pass it to the user-defined loader() + return customImageLoader(opts) } } // Remove property so it's not spread on