Skip to content
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

MPP-3835: Workaround next/image's inline style for SVG images #4820

Merged
merged 2 commits into from
Jun 27, 2024

Conversation

jwhitlock
Copy link
Member

Next.js's Image from next/image adds an inline style style="color:transparent" to some images, such as the logo image:

<img
  alt=""
  loading="lazy"
  width="42"
  height="44"
  decoding="async"
  data-nimg="1"
  style="color:transparent"
  src="/_next/static/media/relay-logo.cabccc81.svg"
/>

This is a known issue, with a good explanation by an issue writer at https://0xdbe.github.io/NextJS-CSP-NextImage/ .

Since we do not use image optimization, we can replace this with an <img /> element. This PR extracts the alt text and image parameters from the <Image> declaration, turns an inline style="color:transparent" into a className with an equivalent CSS rule, and returns the new <img /> element.

How to test:

To see the current behavior
In main or PR #4819, run cd frontend; npm run build. Look for <img or style="color:transparent" in that document. If desired, make a copy for diff later.

In Firefox, open the developer tools, console, and load / reload https://relay.firefox.com/. See many Content-Security-Policy violations.

To see the new behavior
In this branch, run cd frontend; npm run build. The string "color:transparent" should not be in that document. Some of the <img tags will have a new className entry, like Image_transparent__sZTo9. A visual diff with the old version should show that the alt= tag is still present, and the data-nimg="1" element is dropped.

When converting to django-csp 4.x, the 'unsafe-inline' rule was applied
to script-src instead of style-src.
Image from next/image adds an inline style="color:transparent" to many
SVG images. This causes CSP violations in production, and the style is ignored.

This PR replaces Image with a local version that turns the inline style
into a className with the color:transparent style. It only handles this
variant, which was tested to be the only inline style emitted by next.js.
@jwhitlock jwhitlock requested a review from bcolsson as a code owner June 27, 2024 14:21
@jwhitlock jwhitlock requested a review from rafeerahman June 27, 2024 14:22
@jwhitlock jwhitlock changed the title Workaround next/image's inline style for SVG images. MPP-3835: Workaround next/image's inline style for SVG images Jun 27, 2024
@groovecoder groovecoder removed the request for review from bcolsson June 27, 2024 16:25
Copy link
Contributor

@rafeerahman rafeerahman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes!

Not required before merging but I get this warning when running a production build,

⚠ For production Image Optimization with Next.js, the optional 'sharp' package is strongly recommended. Run 'npm i sharp', and Next.js will use it automatically for Image Optimization. Read more: https://nextjs.org/docs/messages/sharp-missing-in-production

Wondering if we should install it or try and remove the warning, as we're not using Image Optimization.

@@ -0,0 +1,3 @@
.transparent {
color: transparent;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

import { ComponentProps } from "react";
import styles from "./Image.module.scss";

export default function Image(props: ComponentProps<typeof NextImage>) {
Copy link
Contributor

@rafeerahman rafeerahman Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, tested it out and the build outputs html <img>'s with no inline styles.

// Work around next.js adding inline style color:transparent
// This breaks our CSP rules about inline styles.
// More context and some suggested solutions from:
// https://0xdbe.github.io/NextJS-CSP-NextImage/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was a good read, learned some more about CSP and the Image issue with Next.js 👍

@jwhitlock
Copy link
Member Author

Wondering if we should install sharp or try and remove the warning, as we're not using Image Optimization.

I read https://nextjs.org/docs/messages/sharp-missing-in-production, but I don't see a way to remove the warning, or an existing issue. This will need some more research. It may be worth installing sharp and seeing if it changes the SVGs at all, and if it is compatible with this change. I'll spend a little time on this before merging.

@jwhitlock
Copy link
Member Author

The output is identical with sharp installed, which is what we'd expect since we've disabled optimization. Let's ignore the warning for now, and maybe revisit disabling after the next.js 15 upgrade.

@jwhitlock jwhitlock added this pull request to the merge queue Jun 27, 2024
Merged via the queue into main with commit f36acc0 Jun 27, 2024
29 checks passed
@jwhitlock jwhitlock deleted the workaround-next-img-inline-style branch June 27, 2024 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants