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

How to handle Emotion and Google Fonts inlined styles #34

Closed
alexturpin opened this issue Jun 30, 2022 · 20 comments
Closed

How to handle Emotion and Google Fonts inlined styles #34

alexturpin opened this issue Jun 30, 2022 · 20 comments
Labels
bug Something isn't working enhancement New feature or request question Further information is requested

Comments

@alexturpin
Copy link
Contributor

alexturpin commented Jun 30, 2022

Hey all,

I'm just integrating the middleware into my Next.js website and I'm running into a few issues. You can see the issues live here.

I am using Google Fonts and it seems like Google or Next.js does some inlining into a style tag, and those tags don't have a hash.

Here's how I include the Google Fonts:

  render() {
    const { Head, NextScript } = provideComponents(this.props)

    return (
      <Html>
        <Head>
          <link rel="preconnect" href="https://fonts.googleapis.com" />
          <link rel="preconnect" href="https://fonts.gstatic.com" crossOrigin="" />
          <link // eslint-disable-line @next/next/no-page-custom-font
            href="https://fonts.googleapis.com/css2?family=Open+Sans:wght@400;700;800&family=Playfair+Display:wght@500&display=swap"
            rel="stylesheet"
          />
        </Head>
        <body>
          <Main />
          <NextScript />
        </body>
      </Html>
    )
  }

The other problem is that I am using Mantine which leverages Emotion under the hood. It outputs an inline style tag at the top of my that also doesn't seem to have a hash. I do have access to the nonce option of Emotion in my _app.ts but not sure if that's even helpful.

Anyone know if there there are any workarounds to these problems? Cheers!

@nibtime
Copy link
Owner

nibtime commented Jul 3, 2022

Hi @alexturpin

that no style tag at all is covered is odd. Your 404 page (https://can-you-unlock-edbupijva-alexturpin.vercel.app/doesntexist) also has inline styles blocked (The default Next error component uses inline style attributes). This should work with a proper setup, see here: https://next-safe-middleware.vercel.app/doesntexist

Can you try this setup and see how it goes?

// pages/_document.js

import Document, {
	provideComponents,
} from '@next-safe/middleware/dist/document';
import { Html, Main } from 'next/document';
import React from 'react';

export default class MyDocument extends Document {
	static async getInitialProps(ctx: any) {
		return Document.getInitialProps(ctx);
	}

	render() {
		const { Head, NextScript } = provideComponents(this.props);
		return (
			<Html>
				<Head>
					<link
						href="https://fonts.googleapis.com/css2?family=Inter:wght@100..900&display=swap"
						rel="stylesheet"
					/>
				</Head>
				<body>
					<Main />
					<NextScript />
				</body>
			</Html>
		);
	}
}
// pages/_middleware.js
import type { Middleware } from '@next-safe/middleware';
import {
	chain,
	nextSafe,
	strictDynamic,
	strictInlineStyles,
} from '@next-safe/middleware';

const isDev = process.env.NODE_ENV === 'development';

export default chain(
	nextSafe({ isDev }),
	strictDynamic(),
	strictInlineStyles({
		extendStyleSrc: false,
	}),
)

I experimented a lot with what's possible to avoid style-src: 'unsafe-inline' in the CSP. I think I haven't put this in docs yet at all though, I intend do update it soon though, I will tackle #30 the next few weeks

I am currently working on some stuff for a version 0.7.0. I will set up Mantine in the e2e app, see how it goes and report problems/limitations I found back here.

I realized though that it might not always be possible to avoid style-src: 'unsafe-inline' and is dependent on how the CSS-in-JS framework works and if what your 3rd party libs do with inline styles (insert/change dynamically etc.)

@alexturpin
Copy link
Contributor Author

alexturpin commented Jul 3, 2022

@nibtime thanks for taking the time to reply! Here's a deployment with the additional strictInlineStyles config. Still doesn't appear to work: https://can-you-unlock-f1n8qxa6z-alexturpin.vercel.app/

Here's my full _document.tsx (Mantine also requires styles from getInitialProps)

import Document, { provideComponents } from "@next-safe/middleware/dist/document"
import { DocumentContext, Html, Main } from "next/document"
import { createStylesServer, ServerStyles } from "@mantine/next"

const stylesServer = createStylesServer()

export default class MyDocument extends Document {
  static async getInitialProps(ctx: DocumentContext) {
    // This needs to handle both @next-safe and Mantine
    const initialProps = await Document.getInitialProps(ctx)

    return {
      ...initialProps,
      styles: [
        initialProps.styles,
        <ServerStyles html={initialProps.html} server={stylesServer} key="styles" />,
      ],
    }
  }

  render() {
    const { Head, NextScript } = provideComponents(this.props)

    return (
      <Html>
        <Head>
          <link rel="preconnect" href="https://fonts.googleapis.com" />
          <link rel="preconnect" href="https://fonts.gstatic.com" crossOrigin="" />
          <link // eslint-disable-line @next/next/no-page-custom-font
            href="https://fonts.googleapis.com/css2?family=Open+Sans:wght@400;700;800&family=Playfair+Display:wght@500&display=swap"
            rel="stylesheet"
          />
        </Head>
        <body>
          <Main />
          <NextScript />
        </body>
      </Html>
    )
  }
}

And here's my _middleware.tsx:

import { nextSafe, chain, strictDynamic, strictInlineStyles } from "@next-safe/middleware"
import { isDev } from "~/config"

const devCSP = {
  "connect-src": ["'self'", "o1263600.ingest.sentry.io", "webpack://*"],
  "font-src": ["'self'", "fonts.gstatic.com"],
  "script-src": ["'self'", "'unsafe-eval'"],
  "style-src": ["'self'", "fonts.googleapis.com", "'unsafe-inline'"],
}

const prodCSP = {
  "connect-src": ["'self'", "o1263600.ingest.sentry.io"],
  "font-src": ["'self'", "fonts.gstatic.com"],
  "style-src": ["'self'", "fonts.googleapis.com"],
}

export default chain(
  nextSafe({
    isDev,
    contentSecurityPolicy: isDev ? devCSP : prodCSP,
    referrerPolicy: "strict-origin-when-cross-origin",
  }),
  strictDynamic(),
  strictInlineStyles({
    extendStyleSrc: false,
  })
)

nibtime added a commit that referenced this issue Jul 6, 2022
as requested in #34

good opportunity to test the lib with another great UI framework
and CSS-in-JS lib
nibtime added a commit that referenced this issue Jul 6, 2022
as requested in #34

good opportunity to test the lib with another great UI framework
and CSS-in-JS lib
nibtime added a commit that referenced this issue Jul 6, 2022
as requested in #34

good opportunity to test the lib with another great UI framework
and CSS-in-JS lib
@nibtime
Copy link
Owner

nibtime commented Jul 6, 2022

Hi @alexturpin

I just released 0.8.0 0.7.0 to NPM and as your issues with Mantine/emotion intersected with other issues I had in already mind, I chose it as a test scenario for the e2e app:

https://next-safe-middleware.vercel.app/
https://next-safe-middleware.vercel.app/mantine
https://next-safe-middleware.vercel.app/mantine/gsp
https://next-safe-middleware.vercel.app/mantine/gssp

To make it work, I needed to extend the interface for the getInitialProps in _document, now called getCspInitialProps.

There is a section in the README now for this. You'll find links to files of the e2e app, which basically now has exactly the solution you need.

So can you install 0.8.0 0.7.0 and see if it works now for you?

Cheers!

@nibtime nibtime added enhancement New feature or request question Further information is requested labels Jul 6, 2022
@nibtime nibtime pinned this issue Jul 9, 2022
@nibtime nibtime closed this as completed Jul 9, 2022
@alexturpin
Copy link
Contributor Author

alexturpin commented Jul 11, 2022

@nibtime wow that's fantastic, great job on the new release(s). Great DX and love the Sentry reporting stuff. Sorry for the late feedback, I just got back on this project.

Unfortunately, I haven't been able to get it to work 100%. The CSP directives get set and I have no CSP errors, but oddly enough, on Vercel, whenever the page seems to get hydrated, all of the content gets removed from the DOM.

I'm still trying to figure out why, but I've narrowed it down to the strictDynamic() middleware. If I comment that out, it doesn't happen. It also doesn't happen in dev 🤔 Any idea what could cause this?

"next": "^12.2.2",
"@next-safe/middleware": "^0.8.0",
import { getCspInitialProps, provideComponents } from "@next-safe/middleware/dist/document"
import Document, { DocumentContext, Html, Main } from "next/document"

import { createStylesServer, ServerStyles } from "@mantine/next"

const stylesServer = createStylesServer()

export default class MyDocument extends Document {
  static async getInitialProps(ctx: DocumentContext) {
    // This needs to handle both @next-safe and Mantine
    const initialProps = await getCspInitialProps({
      ctx,
      trustifyStyles: true,
      hashRawCss: [
        (initialProps) => [
          stylesServer.extractCritical(initialProps.html).css,
          ...stylesServer.extractCriticalToChunks(initialProps.html).styles.map((s) => s.css),
        ],
      ],
    })

    // Required for Mantine CSS-in-JS SSR (Emotion) since we're also using @next-safe
    initialProps.styles = (
      <>
        {initialProps.styles}
        {/* Mantine CSS-in-JS SSR (Emotion) */}
        <ServerStyles html={initialProps.html} server={stylesServer} />
      </>
    )

    return initialProps
  }

  render() {
    const { Head, NextScript } = provideComponents(this.props)

    return (
      <Html>
        <Head />
        <body>
          <Main />
          <NextScript />
        </body>
      </Html>
    )
  }
}

@nibtime nibtime reopened this Jul 11, 2022
@nibtime
Copy link
Owner

nibtime commented Jul 11, 2022

Hi @alexturpin

I can see on your deployment that you are using an optional catch-all route for your landing page ([[...landingPage]]-d7ac30ad2f212bb9.js).

And it's not happing on the 404 (https://can-you-unlock-qob3a8y00-alexturpin.vercel.app/notfound). I suppose you haven't a custom pages/404.js yet, so it's per request and with Nonce-based strict CSP.

Is there a specific reason why you are doing that, instead of using pages/index.js for your landing page?

From my experience, Next.js treats the index page somewhat special, (e.g. with i18n routing), so I can imagine it has something to do with that.

Can you try it without the catch-all route and report back how that goes?

@nibtime nibtime added the bug Something isn't working label Jul 11, 2022
@alexturpin
Copy link
Contributor Author

alexturpin commented Jul 12, 2022

I was using the catch all route because I need to generate the pages from CMS data and was just grabbing index the same way, but I can handle index as its own case. This does seem to work now 🎉

Thank you so much for your work on this library and the help getting this up and running!

@nibtime
Copy link
Owner

nibtime commented Jul 12, 2022

Awesome! I thought this was very strange, as this lib only does server-side stuff and nothing past hydration.

Your're welcome, I am glad the lib is useful to you.

The problem might be also related to this: vercel/next.js#38267

This seems a 12.2 issue and shows up as a console error Error: Invariant: attempted to hard navigate to the same URL /how-it-works. Happens on your app and the e2e app of the lib, but not on my apps < 12.2.

@alexturpin
Copy link
Contributor Author

Yeahh I noticed that as well. Doesn't seem to be causing major issues but will keep an eye out for a Next.js patch for it.

@alexturpin
Copy link
Contributor Author

Interestingly, that bug might be more consequential than I initially thought. I realized that now some of my dynamic pages that use shallow routing were no longer navigating properly. The URL in the address bar changes but nothing else, the app doesn't render the new page. I figure it might be related to the Next.js issue linked.

I've disabled the middleware once again and will monitor.

@oauramos
Copy link

@alexturpin Are you using also emotionCache in your application?

@alexturpin
Copy link
Contributor Author

@middlebaws hmm not directly, perhaps Mantine does under the hood. Why?

@nibtime
Copy link
Owner

nibtime commented Jul 24, 2022

@all-contributors please add @alexturpin for bug, doc, ideas

regarding the navigate bug: I released 0.9.0 and I think it seems mostly gone with the new matching logic of isPageRequest, that now excludes requests with the x-nextjs-data header. That also excludes prefetch requests to other pages that load when next/link comes into view - doesn't make sense to apply a CSP there .

In the e2e app it only happens whenever I hard-navigate to https://e2e-next-safe-middleware-nibtime.vercel.app/isr/gsp (an ISR page) from the home page (the links there are regular <a>'s, so CSP refreshes), and if I did once, the error keeps recurring.

However If I directly enter https://e2e-next-safe-middleware-nibtime.vercel.app/isr/gsp into the browser it loads without error the also for any subsequent navigation.

Also when you go to https://e2e-next-safe-middleware-nibtime.vercel.app/gsp and then use the internal navigation (next/link) to https://e2e-next-safe-middleware-nibtime.vercel.app/isr/gsp, the error doesn't happen.

So it seems to happen, whenever the first navigation was a hard navigation with an <a>. Really odd.

@allcontributors
Copy link
Contributor

@nibtime

I've put up a pull request to add @alexturpin! 🎉

@nibtime
Copy link
Owner

nibtime commented Jul 25, 2022

@alexturpin

the shallow routing bug you described seems not gone yet, still persists in 12.2.3 released 3 days ago: vercel/next.js#38595

Since middleware is stable, it also kind of totally breaks routing :D. I hope this gets fixed soon ...

Everything related to this I could find:

A minimal Codesandbox repro from someone: https://codesandbox.io/s/magical-allen-ibzr6l
That shows that the shallow routing bug is happening even with empty middleware. That is a really bad bug. It makes all 12.2 versions until a fix basically unusable.

@nibtime
Copy link
Owner

nibtime commented Jul 27, 2022

closing this, I consider the style problem with emotion and Mantine solved. I will keep an eye on the routing bug(s) with middleware, as soon as they are solved it will release a patch version with updated peer deps that only allow Next 12.2 versions since the fix.

@nibtime nibtime closed this as completed Jul 27, 2022
@nibtime
Copy link
Owner

nibtime commented Jul 29, 2022

I just tested the e2e app with 12.2.4-canary.7. The hard-navigate bug is gone. The shallow routing bug won't get fixed completely as it seems, it has been closed with a caveat note in docs.

I will then use >=12.2.4 as peer dep for this lib once it is out.

@alexturpin
Copy link
Contributor Author

Ah so shallow routing won't be compatible with the middleware at all then?

@nibtime
Copy link
Owner

nibtime commented Jul 29, 2022

Seems like it, at least certain patterns. Apart from the note, I couldn't find a PR with a fix in code. I don't like that middleware breaks perfectly good working existing setups. But I also understand Next maintainers, as middleware is a great feature that enables so many possibilities and those things can just fundamentally disagree.

I made to decision to split up the lib (#60 (comment)) in multiple packages, so that more configuration options than middleware are available, with different peer dep requirements, so the CSP features don't depend on latest Next.

You might decide now not to go for 12.2 beacuse your app requires the shallow routing, but then also can't use this lib because middleware is the only option, etc.

@nibtime
Copy link
Owner

nibtime commented Jul 29, 2022

@alexturpin

correction: https://github.com/vercel/next.js/releases/tag/v12.2.4-canary.8 also includes a code fix alongside the caveat note.

I forked the minimal repro with 12.2.4-canary.8: https://codesandbox.io/s/restless-dream-0u45e8. The shallow routing works there.

@nibtime nibtime mentioned this issue Aug 2, 2022
nibtime added a commit that referenced this issue Aug 5, 2022
has important routing bug fixes related to middleware, see #34 (comment)
nibtime added a commit that referenced this issue Aug 5, 2022
has important routing bug fixes related to middleware, see #34 (comment)
nibtime added a commit that referenced this issue Aug 5, 2022
has important routing bug fixes related to middleware, see #34 (comment)
nibtime added a commit that referenced this issue Aug 5, 2022
has important routing bug fixes related to middleware, see #34 (comment)
@alexturpin
Copy link
Contributor Author

@alexturpin

correction: https://github.com/vercel/next.js/releases/tag/v12.2.4-canary.8 also includes a code fix alongside the caveat note.

I forked the minimal repro with 12.2.4-canary.8: https://codesandbox.io/s/restless-dream-0u45e8. The shallow routing works there.

Woo! That's fantastic. Thanks for the update!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants