nibtime / next-safe-middleware

Strict CSP (Content-Security-Policy) for Next.js hybrid apps https://web.dev/strict-csp/
https://next-safe-middleware.vercel.app
MIT License
78 stars 20 forks source link

How to handle Emotion and Google Fonts inlined styles #34

Closed alexturpin closed 2 years ago

alexturpin commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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!

alexturpin commented 2 years ago

@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 commented 2 years ago

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?

alexturpin commented 2 years ago

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 commented 2 years ago

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: https://github.com/vercel/next.js/issues/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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@alexturpin Are you using also emotionCache in your application?

alexturpin commented 2 years ago

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

nibtime commented 2 years ago

@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[bot] commented 2 years ago

@nibtime

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

nibtime commented 2 years ago

@alexturpin

the shallow routing bug you described seems not gone yet, still persists in 12.2.3 released 3 days ago: https://github.com/vercel/next.js/issues/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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

nibtime commented 2 years ago

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 (https://github.com/nibtime/next-safe-middleware/discussions/60#discussioncomment-3259782) 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 commented 2 years ago

@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.

alexturpin commented 2 years ago

@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!