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

Allow a nonce to be set on single fetch stream transfer inline scripts #9364

Merged

Conversation

haines
Copy link
Contributor

@haines haines commented May 3, 2024

Closes: #9359

  • Docs
  • Tests

Testing Strategy:

I added a test case to integration/single-fetch-test.ts, and I patched @remix-run/react in our application with these changes and saw that it fixed our CSP violations.

Copy link

changeset-bot bot commented May 3, 2024

🦋 Changeset detected

Latest commit: 1087c22

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
@remix-run/react Patch
@remix-run/testing Patch
@remix-run/dev Patch
create-remix Patch
remix Patch
@remix-run/architect Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/css-bundle Patch
@remix-run/deno Patch
@remix-run/eslint-config Patch
@remix-run/express Patch
@remix-run/node Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@haines haines force-pushed the add-nonce-to-stream-transfer-scripts branch from 81a318e to 38ca5a4 Compare May 3, 2024 09:13
@brophdawg11
Copy link
Contributor

Thanks!

Yeah this stuff is best tested through our E2E tests - want to try something like this in integration/single-fetch-test.ts?

You can run it with pnpm test:integration single-fetch --project chromium

  test("supports nonce on streaming script tags", async ({ page }) => {
    let fixture = await createFixture({
      config: {
        future: {
          unstable_singleFetch: true,
        },
      },
      files: {
        ...files,
        "app/entry.server.tsx": js`
          import { PassThrough } from "node:stream";

          import type { EntryContext } from "@remix-run/node";
          import { createReadableStreamFromReadable } from "@remix-run/node";
          import { RemixServer } from "@remix-run/react";
          import { renderToPipeableStream } from "react-dom/server";

          export default function handleRequest(
            request: Request,
            responseStatusCode: number,
            responseHeaders: Headers,
            remixContext: EntryContext
          ) {
            return new Promise((resolve, reject) => {
              const { pipe } = renderToPipeableStream(
                <RemixServer context={remixContext} url={request.url} nonce="the-nonce" />,
                {
                  onShellReady() {
                    const body = new PassThrough();
                    const stream = createReadableStreamFromReadable(body);
                    responseHeaders.set("Content-Type", "text/html");
                    resolve(
                      new Response(stream, {
                        headers: responseHeaders,
                        status: responseStatusCode,
                      })
                    );
                    pipe(body);
                  },
                  onShellError(error: unknown) {
                    reject(error);
                  },
                  onError(error: unknown) {
                    responseStatusCode = 500;
                  },
                }
              );
            });
          }
        `,
      },
    });
    let appFixture = await createAppFixture(fixture);
    let app = new PlaywrightFixture(appFixture, page);
    await app.goto("/data", true);
    let scripts = await page.$$("script");
    expect(scripts.length).toBe(6);
    for (let s of scripts) {
      // Do nonce expect() calls here for the scripts that should have it - which I think might just be
      // any with `window.__remixContext.streamController` in the innterHTML?
      console.log(await s.getAttribute("nonce"), await s.innerHTML());
    }
  });

@haines
Copy link
Contributor Author

haines commented May 6, 2024

@brophdawg11 thanks a lot for helping with the test case!

I found that <Scripts> also injects an inline script that references window.__remixContext.streamController. I opted to add the nonce to that as well, rather than trying to distinguish it from the scripts added by <StreamTransfer>, which I felt would probably make the test too brittle.

@brophdawg11 brophdawg11 linked an issue May 6, 2024 that may be closed by this pull request
Copy link
Contributor

@brophdawg11 brophdawg11 left a comment

Choose a reason for hiding this comment

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

Thanks!

@brophdawg11 brophdawg11 merged commit 142f47b into remix-run:dev May 6, 2024
5 checks passed
@haines haines deleted the add-nonce-to-stream-transfer-scripts branch May 6, 2024 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Single fetch: allow setting a nonce on StreamTransfer's script tags
2 participants