Replace next image optimization with drupal image styles #93

pookmish · 2023-12-05T15:51:29Z

READY FOR REVIEW

Summary

Provide a custom next image loader that uses drupal image styles.

Steps to Test

review the approach.

vercel · 2023-12-05T15:51:34Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
su-library	✅ Ready (Inspect)	Visit Preview	Dec 12, 2023 4:05pm

pookmish · 2023-12-05T19:39:05Z

Thinking about this, we loose webp support. Not sure if that's a concern, but it is something to note

sherakama · 2023-12-06T07:13:00Z

next.config.js

@@ -9,27 +9,32 @@ module.exports = {
        hostname: process.env.NEXT_IMAGE_DOMAIN,
      },
    ],
+    deviceSizes: [


Can you tell me why you chose these breakpoints?

Mostly arbitrary. I took the default values and commented out the ones I didn't want. I tried to get something close-ish to the breakpoints.

Why not align them with the breakpoints of the design system?

meh, it's not really necessary IMO. Images don't need to break down at the same breakpoint. Just as long as the image is larger than the area it's in. Almost all images use a css cover property anyways

This seems like a thing to ask what others on the team are doing and how they set up their responsive images. It is in the 'doesn't really matter' category but this seems like a good opportunity to engage and align with others.

sherakama · 2023-12-06T07:22:48Z

lib/libguides.tsx

@@ -21,6 +21,7 @@ const fetchLibGuides = async ({accountId, subjectId}: { accountId?: number, subj
    }

    const guidesConfig = {
+      next: {revalidate: 1},


What is this doing?

Tells next to only cache the fetch call for 1 second.

Aye, but why is it in this PR?

🤷 it was in my local code, so i just committed it with the stuff.

Ok, if you want it in then I guess it stays in.

It would be great if you could write out an RFC-style document that details how the ISR invalidation is intended to work so that I could have something to compare this to. That would also be helpful documentation for the SUP project.

sherakama · 2023-12-06T07:25:34Z

components/patterns/elements/image-loader.tsx

+    {w: 1200, s: 'breakpoint_xl_2x'},
+    {w: 3840, s: 'breakpoint_2xl_2x'},
+  ];
+  const style = imageStyles.find(s => s.w === width)?.s || 'breakpoint_xl_2x';


Writing this out in English so my brain is happy.

For all images, that are not local or relative, try and match an image style to an image width, if no matching image width to the array of imageStyles, default to the biggest image style.

Example: https://github.com/SU-SWS/sulgryphon-nextjs/blob/1.x/components/paragraph/stanford-card.tsx#L43

That wouldn't get all the src set values (128, 384, 750, 1200, 3840) because it doesn't set the sizes key? It would just render as the one size?

opposite of what you ask. If you don't specifiy the sizes property, it will use the default from next.config.js.

Gotcha thanks.

So the convention and recommendation would be to supply a sizes property wherever images are being used, otherwise the largest image will be used?

no no... It's recommended to use the default values. Maybe unless you are displaying a small image and don't need the large versions. If you don't pass a sizes property when using an <Image> it will make a responsive image using the sizes defined in the next.config.js which should be the most ideal in most situations I think.

Gotcha, ok, this info is something that @jenbreese will find helpful/useful when she gets going on the SUP project.

components/patterns/elements/image-loader.tsx

sherakama · 2023-12-06T07:37:27Z

next.config.js

+    imageSizes: [
+      // 16,
+      // 32,
+      // 48,
+      // 64,
+      // 96,
+      128,
+      // 256,
+      384
+    ],


What unit are these in? I'm not fully grokking the documentation

https://nextjs.org/docs/app/api-reference/components/image#imagesizes

https://nextjs.org/docs/app/api-reference/components/image#sizes

Do you have an example I can follow?

https://html.com/attributes/img-srcset/#Specifying_Image_Width

So it tells the browser the size of the image... which isn't really accurate, but it's something

<img alt="" loading="lazy" decoding="async" data-nimg="fill" class="su-object-cover su-object-center" style="position: absolute; height: 100%; width: 100%; inset: 0px; color: transparent;" sizes="100vw" srcset="https://example.com/breakpoint_lg_2x/image.jpg 750w, https://example.com/breakpoint_xl_2x/image.jpg 1200w, https://example.com/breakpoint_2xl_2x/image.jpg 3840w" src="https://example.com/breakpoint_2xl_2x/image.jpg">

next.config.js

sherakama · 2023-12-06T07:50:27Z

Do you have to do anything to support pixel density (1x and 2x)?

sherakama · 2023-12-06T08:03:16Z

Oh and one last thought, what about cache control? https://nextjs.org/docs/app/api-reference/components/image#caching-behavior

pookmish · 2023-12-06T16:57:22Z

Do you have to do anything to support pixel density (1x and 2x)?

Nothing provided to handle that

Oh and one last thought, what about cache control?

The image src urls are changed to target the Drupal site directly. So the cacheing would be on the Acquia side.

A problem I found recently though is this implementation requires the allow_insecure_derivatives enabled because the itok for each image style would be different.

sherakama · 2023-12-07T18:00:10Z

components/patterns/elements/image-loader.tsx

+  const style = imageStyles.find(s => s.w === width)?.s || 'breakpoint_xl_2x';
+  // Switch the image style for the desired width. Add the width parameter just to prevent console logs.
+  // https://nextjs.org/docs/messages/next-image-missing-loader-width
+  return src.toString().replace(/styles\/\w+\//, `styles/${style}/`) + `&w=${width}`;


However unlikely, in theory, the src string could already have a w parameter. It would be safer to put the URL through new URL and then set the searchParams instead of concatenating the string.

eg: https://nextjs.org/docs/app/api-reference/next-config-js/images#fastly

I thought about that too. It would be more bullet proof that way, but since we're only ever pulling images from Drupal, there shouldn't ever be that param.

pookmish · 2023-12-07T18:03:46Z

FYI, I'm still not 100% convinced myself of this approach since it requires insecure derivatives in the Drupal side. What are your thoughts on that

sherakama · 2023-12-11T19:25:41Z

FYI, I'm still not 100% convinced myself of this approach since it requires insecure derivatives in the Drupal side. What are your thoughts on that

By skipping the Next/Vercel optimizations and going straight to Drupal without secure derivatives we are removing the DoS flood protection on Drupal and limiting the styles/sizes that can be produced to the image styles available in the Drupal install. Overall, I don't think we've had many issues with DoS attacks on our image derivatives and we have better logging/reporting/blocking on Acquia than we do currently on Vercel so this seems like a valid trade.

However, if we are going to remove the secure token, why not just re-write the image paths, like you had before, to remove any and all URL parameters and force the image source to always be the original file? Wouldn't that get around the URL parameter 'source' issue on Vercel and allow this site to continue to use Next's image optimizations and Vercel's Edge network caching?

pookmish · 2023-12-12T16:27:36Z

However, if we are going to remove the secure token, why not just re-write the image paths, like you had before, to remove any and all URL parameters and force the image source to always be the original file? Wouldn't that get around the URL parameter 'source' issue on Vercel and allow this site to continue to use Next's image optimizations and Vercel's Edge network caching?

From what it seems, it doesn't matter if we rewrite the urls. Vercel still treats each request as a unique value. I tried to implement a middleware for the _next/image path and it didn't seem to work. I then tried to use vercel.json file to redirect paths instead of rewrite them. That also didn't seem to work as desired. I'm not sure if our Akamai service comes with an image optimizer as mentioned here, so the Drupal image styles seem to be the only option to fix this.

sherakama · 2023-12-12T20:00:17Z

However, if we are going to remove the secure token, why not just re-write the image paths, like you had before, to remove any and all URL parameters and force the image source to always be the original file? Wouldn't that get around the URL parameter 'source' issue on Vercel and allow this site to continue to use Next's image optimizations and Vercel's Edge network caching?

From what it seems, it doesn't matter if we rewrite the urls. Vercel still treats each request as a unique value. I tried to implement a middleware for the _next/image path and it didn't seem to work. I then tried to use vercel.json file to redirect paths instead of rewrite them. That also didn't seem to work as desired. I'm not sure if our Akamai service comes with an image optimizer as mentioned here, so the Drupal image styles seem to be the only option to fix this.

Remote image invalidation is supported by a cache query param: https://vercel.com/docs/image-optimization#remote-images

Remote image cache invalidation: Adding a query string to the image URL (e.g., ?new or ?v=2) will serve a new image to visitors. Alternatively, you can configure the cache to expire more frequently in order to serve new images.

But I am surprised you cannot re-write the URL to drop the extra params before the route is hit or provide the middleware to validate that the request came from a legit page render by using a CSRF token or something.

I submitted a support request with Vercel
https://vercel.com/teams/stanford-libraries/support/cases/00179738

One thing we may want to do is forego fetching from the image styles directory at all and always pull the original source image from sites/default/files. Optimize on Vercel for different sizes of the one source file instead of optimizing on the already optimized file on the image derivative.

sherakama · 2023-12-13T19:49:29Z

Reply from Vercel support

The best way to filter out these cache busting requests would be to implement Edge Middleware that filters out query parameters: Edge Middleware Overview

We have a pre-existing template on this which you might find helpful: Filtering Query Parameters

This middleware allows you to specify a list of allowed query parameters (allowedParams), and any query parameter not included in this list is removed from the URL, effectively filtering out unwanted or potentially harmful parameters.

If the offender is consistently making these requests from a specific IP address, you can also leverage our middleware template on IP Blocking with Upstash or IP Blocking with DataDome.

You can also introduce User-Agent Based Rendering or API Rate Limiting by IP and API Keys with Upstash depending on your application's needs.

Filtering Query Parameters
IP Blocking with Upstash
IP Blocking with DataDome.
User-Agent Based Rendering
API Rate Limiting by IP and API Keys with Upstash

pookmish · 2023-12-14T20:03:33Z

I kinda had a thought about the whole itok thing... what if we removed the use of the image styles like you mentioned. then in the components, it will be fetching the original image which doesn't need an itok at all. This would eliminate the sql injection getting appended to the parameter.

However, I don't think it solves the whole problem, because based on the documentation, a bot can just add it to the url to generate another image. /_next/image?url=http....jpg'inject=something'&w=1200.

sherakama · 2023-12-14T21:35:23Z

I kinda had a thought about the whole itok thing... what if we removed the use of the image styles like you mentioned. then in the components, it will be fetching the original image which doesn't need an itok at all. This would eliminate the sql injection getting appended to the parameter.

However, I don't think it solves the whole problem, because based on the documentation, a bot can just add it to the url to generate another image. /_next/image?url=http....jpg'inject=something'&w=1200.

That sounds like an improvement to me.

As far as parameters go, I think your rewrite is helping and can continue to help. If you can adjust it to allow only the w parameter and only contain a max of 4 integers, I think you'll reduce the attack surface substantially. Yes, bots could send arbitrary integers directly to that URL param and at that point, we'd need to block the bot.

-- Removed Monitoring Discussion and Links To Focus On This PR --

pookmish · 2024-06-24T19:34:55Z

Closing this since we seem to be doing good since the issue.

pookmish requested a review from sherakama December 5, 2023 15:51

github-actions bot added the size/xs label Dec 5, 2023

vercel bot deployed to Preview December 5, 2023 15:52 View deployment

vercel bot deployed to Preview December 5, 2023 16:01 View deployment

pookmish force-pushed the next-image branch from 193c9e6 to af74826 Compare December 5, 2023 16:03

vercel bot deployed to Preview December 5, 2023 16:04 View deployment

sherakama reviewed Dec 6, 2023

View reviewed changes

components/patterns/elements/image-loader.tsx Outdated Show resolved Hide resolved

sherakama reviewed Dec 6, 2023

View reviewed changes

next.config.js Outdated Show resolved Hide resolved

vercel bot deployed to Preview December 6, 2023 16:58 View deployment

pookmish force-pushed the next-image branch from 0589697 to c3f1954 Compare December 6, 2023 17:19

vercel bot deployed to Preview December 6, 2023 17:20 View deployment

sherakama reviewed Dec 7, 2023

View reviewed changes

pookmish added 2 commits December 12, 2023 07:51

Replace next image optimization with drupal image styles

7168175

Fixup styles

c96af7a

pookmish force-pushed the next-image branch from c3f1954 to c96af7a Compare December 12, 2023 16:03

vercel bot deployed to Preview December 12, 2023 16:05 View deployment

pookmish force-pushed the 1.x branch 2 times, most recently from 1da44a6 to c8b984d Compare April 19, 2024 17:40

pookmish closed this Jun 24, 2024

pookmish deleted the next-image branch June 24, 2024 19:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace next image optimization with drupal image styles #93

Replace next image optimization with drupal image styles #93

pookmish commented Dec 5, 2023 •

edited

Loading

vercel bot commented Dec 5, 2023 •

edited

Loading

pookmish commented Dec 5, 2023

sherakama Dec 6, 2023

pookmish Dec 6, 2023

sherakama Dec 6, 2023

pookmish Dec 6, 2023

sherakama Dec 7, 2023

sherakama Dec 6, 2023

pookmish Dec 6, 2023

sherakama Dec 7, 2023

pookmish Dec 7, 2023

sherakama Dec 11, 2023

sherakama Dec 6, 2023

sherakama Dec 6, 2023

pookmish Dec 6, 2023

sherakama Dec 7, 2023

pookmish Dec 7, 2023

sherakama Dec 11, 2023

sherakama Dec 6, 2023

pookmish Dec 6, 2023

sherakama commented Dec 6, 2023

sherakama commented Dec 6, 2023

pookmish commented Dec 6, 2023

sherakama Dec 7, 2023

pookmish Dec 7, 2023

pookmish commented Dec 7, 2023

sherakama commented Dec 11, 2023 •

edited

Loading

pookmish commented Dec 12, 2023

sherakama commented Dec 12, 2023 •

edited

Loading

sherakama commented Dec 13, 2023 •

edited

Loading

pookmish commented Dec 14, 2023

sherakama commented Dec 14, 2023 •

edited

Loading

pookmish commented Jun 24, 2024

Replace next image optimization with drupal image styles #93

Replace next image optimization with drupal image styles #93

Conversation

pookmish commented Dec 5, 2023 • edited Loading

READY FOR REVIEW

Summary

Steps to Test

vercel bot commented Dec 5, 2023 • edited Loading

pookmish commented Dec 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sherakama commented Dec 6, 2023

sherakama commented Dec 6, 2023

pookmish commented Dec 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pookmish commented Dec 7, 2023

sherakama commented Dec 11, 2023 • edited Loading

pookmish commented Dec 12, 2023

sherakama commented Dec 12, 2023 • edited Loading

sherakama commented Dec 13, 2023 • edited Loading

pookmish commented Dec 14, 2023

sherakama commented Dec 14, 2023 • edited Loading

pookmish commented Jun 24, 2024

pookmish commented Dec 5, 2023 •

edited

Loading

vercel bot commented Dec 5, 2023 •

edited

Loading

sherakama commented Dec 11, 2023 •

edited

Loading

sherakama commented Dec 12, 2023 •

edited

Loading

sherakama commented Dec 13, 2023 •

edited

Loading

sherakama commented Dec 14, 2023 •

edited

Loading