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

feat(sveltekit): Add options to configure fetch instrumentation script for CSP #9969

Merged
merged 1 commit into from
Dec 22, 2023

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Dec 22, 2023

This PR adds options to the sentryHandle request handler that allows control over CSP-relevant aspects for the <script> that the request handler injects into the page. Previously, the injected script was blocked by browsers if CSP was enabled, due to inline <script>s not being allowed without a nonce or hash.

Users who configured CSP have now two options to deal with this script:

1. fetchProxyScriptNonce

Users can pass a nonce to sentryHandle which will be added to the injected script: <script nonce="...">:

// src/hooks.server.js
export const handle = sentryHandle({ fetchProxyScriptNonce: "2726c7f26c" })

This requires additional CSP configuration. For example:

// svelte.config.js
 kit: {
    csp: {
      directives: {
        "script-src": ["nonce-2726c7f26c"],
      },
    },
  },
};

2. Disable the fetch script with injectFetchProxyScript

Users can opt out of sentryHandle injecting the fetch proxy script:

// src/hooks.server.js
export const handle = sentryHandle({ injectFetchProxyScript: false })

For SvelteKit <1.26.0 this will disable fetch instrumentation in load function. For more recent versions, fetch instrumentation should continue to work because sveltejs/kit#10009 was merged in 1.26.0.

In the future we could look for a way to decide if we should at all inject the script based on the Kit version. However, for now, I think these manual methods are fine.

fixes #8925

Copy link
Contributor

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay, Feedback) - Webpack (gzipped) 75.89 KB (+0.1% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 67.25 KB (+0.12% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped) 60.87 KB (+0.12% 🔺)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 31.83 KB (+0.22% 🔺)
@sentry/browser (incl. Feedback) - Webpack (gzipped) 30.39 KB (+0.37% 🔺)
@sentry/browser - Webpack (gzipped) 22.09 KB (+0.6% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback) - ES6 CDN Bundle (gzipped) 73.32 KB (+0.11% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 64.99 KB (+0.13% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 31.13 KB (+0.17% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped) 23.13 KB (+0.53% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 203.79 KB (+0.11% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 93.67 KB (+0.23% 🔺)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 68.6 KB (+0.6% 🔺)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 34.08 KB (+0.14% 🔺)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 67.66 KB (+0.11% 🔺)
@sentry/react - Webpack (gzipped) 22.12 KB (+0.6% 🔺)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 84.31 KB (+0.09% 🔺)
@sentry/nextjs Client - Webpack (gzipped) 48.95 KB (+0.15% 🔺)
@sentry-internal/feedback - Webpack (gzipped) 16.6 KB (0%)

@Lms24 Lms24 marked this pull request as ready for review December 22, 2023 09:24
@Lms24 Lms24 requested review from mydea and lforst December 22, 2023 09:24
return html;
};
return ({ html }) => {
const transaction = getActiveTransaction();
Copy link
Member

Choose a reason for hiding this comment

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

l: unrelated to this PR, but we should look to rewrite this to use getActiveSpan() instead (as transaction will go away). Nothing to do here, just something I noticed :)

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah I had the same thought. Wanted to keep the PR atomic but I'll go over SvelteKit soon to get rid of startTxn and stuff like this.

@Lms24 Lms24 merged commit 948e7d3 into develop Dec 22, 2023
53 checks passed
@Lms24 Lms24 deleted the lms/feat-sveltekit-fetch-proxy-csp-options branch December 22, 2023 10:04
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.

CSP inline script violation when loading Sentry SvelteKit
2 participants