-
Notifications
You must be signed in to change notification settings - Fork 50
fix(debugId): Add guards for injected code to avoid errors #783
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which issue does this close? The "closes" currently just links to a previous PR :)
Also, I thought the snippet was safe enough to avoid the size hit of the try/catch but chances are high I missed something. So I think we should proceed with this PR.
@@ -431,7 +431,7 @@ export function createComponentNameAnnotateHooks(ignoredComponents?: string[]): | |||
} | |||
|
|||
export function getDebugIdSnippet(debugId: string): string { | |||
return `;{try{(function(){var e="undefined"!=typeof window?window:"undefined"!=typeof global?global:"undefined"!=typeof globalThis?globalThis:"undefined"!=typeof self?self:{},n=(new e.Error).stack;n&&(e._sentryDebugIds=e._sentryDebugIds||{},e._sentryDebugIds[n]="${debugId}",e._sentryDebugIdIdentifier="sentry-dbid-${debugId}");})();}catch(e){}};`; | |||
return `;{try{(function(){var e=("undefined"!=typeof window&&window)||("undefined"!=typeof global&&global)||("undefined"!=typeof globalThis&&globalThis)||("undefined"!=typeof self&&self)||{},n=(new e.Error).stack;n&&(e._sentryDebugIds=e._sentryDebugIds||{},e._sentryDebugIds[n]="${debugId}",e._sentryDebugIdIdentifier="sentry-dbid-${debugId}");})();}catch(e){}};`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what was the concern with the ternary chain here? Isn't it smaller in bundle size?
77bf77d
to
06d6a99
Compare
return `!function(){try{var e="undefined"!=typeof window?window:"undefined"!=typeof global?global:"undefined"!=typeof globalThis?globalThis:"undefined"!=typeof self?self:{};e._sentryModuleMetadata=e._sentryModuleMetadata||{},e._sentryModuleMetadata[(new e.Error).stack]=function(e){for(var n=1;n<arguments.length;n++){var a=arguments[n];if(null!=a)for(var t in a)a.hasOwnProperty(t)&&(e[t]=a[t])}return e}({},e._sentryModuleMetadata[(new e.Error).stack],${JSON.stringify( | ||
metadata | ||
)})}();`; | ||
)})}catch(e){}}();`; |
Check warning
Code scanning / CodeQL
Improper code sanitization Medium
improperly sanitized value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 26 days ago
To fix the problem, we need to ensure that any stringified object injected into generated JavaScript code is properly sanitized to prevent code injection, especially if the code is ever embedded in an HTML <script>
tag. The best way to do this is to escape potentially dangerous characters in the output of JSON.stringify(metadata)
before inserting it into the code string. This can be achieved by defining an escapeUnsafeChars
function (as shown in the background) and applying it to the result of JSON.stringify(metadata)
. The changes should be made in packages/bundler-plugin-core/src/utils.ts
:
- Add the
escapeUnsafeChars
function and its supportingcharMap
near the top of the file. - In
generateModuleMetadataInjectorCode
, replaceJSON.stringify(metadata)
withescapeUnsafeChars(JSON.stringify(metadata))
.
-
Copy modified lines R9-R28 -
Copy modified lines R358-R359
@@ -8,2 +8,22 @@ | ||
|
||
// Escape potentially dangerous characters for safe code injection | ||
const charMap: Record<string, string> = { | ||
'<': '\\u003C', | ||
'>': '\\u003E', | ||
'/': '\\u002F', | ||
'\\': '\\\\', | ||
'\b': '\\b', | ||
'\f': '\\f', | ||
'\n': '\\n', | ||
'\r': '\\r', | ||
'\t': '\\t', | ||
'\0': '\\0', | ||
'\u2028': '\\u2028', | ||
'\u2029': '\\u2029' | ||
}; | ||
|
||
function escapeUnsafeChars(str: string): string { | ||
return str.replace(/[<>\b\f\n\r\t\0\u2028\u2029/\\]/g, x => charMap[x] || x); | ||
} | ||
|
||
/** | ||
@@ -337,4 +357,4 @@ | ||
// Use try-catch to avoid issues when bundlers rename global variables like 'window' to 'k' | ||
return `!function(){try{var e="undefined"!=typeof window?window:"undefined"!=typeof global?global:"undefined"!=typeof globalThis?globalThis:"undefined"!=typeof self?self:{};e._sentryModuleMetadata=e._sentryModuleMetadata||{},e._sentryModuleMetadata[(new e.Error).stack]=function(e){for(var n=1;n<arguments.length;n++){var a=arguments[n];if(null!=a)for(var t in a)a.hasOwnProperty(t)&&(e[t]=a[t])}return e}({},e._sentryModuleMetadata[(new e.Error).stack],${JSON.stringify( | ||
metadata | ||
return `!function(){try{var e="undefined"!=typeof window?window:"undefined"!=typeof global?global:"undefined"!=typeof globalThis?globalThis:"undefined"!=typeof self?self:{};e._sentryModuleMetadata=e._sentryModuleMetadata||{},e._sentryModuleMetadata[(new e.Error).stack]=function(e){for(var n=1;n<arguments.length;n++){var a=arguments[n];if(null!=a)for(var t in a)a.hasOwnProperty(t)&&(e[t]=a[t])}return e}({},e._sentryModuleMetadata[(new e.Error).stack],${escapeUnsafeChars( | ||
JSON.stringify(metadata) | ||
)})}catch(e){}}();`; |
I think it was fine to add the guards here but I don't understand how |
Yeah, I'm also not 100% sure about this specific case but adding a try/catch here is a good idea in general. |
| datasource | package | from | to | | ---------- | ------------------- | ----- | ----- | | npm | @sentry/vite-plugin | 3.6.1 | 4.1.0 | ## [v4.1.0](https://github.com/getsentry/sentry-javascript-bundler-plugins/blob/HEAD/CHANGELOG.md#410) - feat(deps): Bump [@sentry/cli](https://github.com/sentry/cli) to 2.51.0 [#786](getsentry/sentry-javascript-bundler-plugins#786) - feat(core): Add flag for disabling sourcemaps upload [#785](getsentry/sentry-javascript-bundler-plugins#785) - fix(debugId): Add guards for injected code to avoid errors [#783](getsentry/sentry-javascript-bundler-plugins#783) - docs(options): Improve JSDoc for options [#781](getsentry/sentry-javascript-bundler-plugins#781) - feat(core): Expose method for injecting debug Ids from plugin manager [#784](getsentry/sentry-javascript-bundler-plugins#784) ## [v4.0.2](https://github.com/getsentry/sentry-javascript-bundler-plugins/blob/HEAD/CHANGELOG.md#402) - fix(core): Make `moduleMetadata` injection snippet ES5-compliant ([#774](getsentry/sentry-javascript-bundler-plugins#774)) ## [v4.0.1](https://github.com/getsentry/sentry-javascript-bundler-plugins/blob/HEAD/CHANGELOG.md#401) - fix(core): Make plugin inject ES5-friendly code ([#770](getsentry/sentry-javascript-bundler-plugins#770)) - fix(core): Use `renderChunk` for release injection for Rollup/Rolldown/Vite ([#761](getsentry/sentry-javascript-bundler-plugins#761)) Work in this release was contributed by [@grushetsky](https://github.com/grushetsky). Thank you for your contribution! ## [v4.0.0](https://github.com/getsentry/sentry-javascript-bundler-plugins/blob/HEAD/CHANGELOG.md#400) ##### Breaking Changes - (Type change) Vite plugin now returns `VitePlugin` type instead of `any` - Deprecated function `getBuildInformation` has been removed ##### List of Changes - feat(core)!: Remove `getBuildInformation` export ([#765](getsentry/sentry-javascript-bundler-plugins#765)) - feat(vite)!: Update return type of vite plugin ([#728](getsentry/sentry-javascript-bundler-plugins#728))
| datasource | package | from | to | | ---------- | ------------------- | ----- | ----- | | npm | @sentry/vite-plugin | 3.6.1 | 4.1.1 | ## [v4.1.1](https://github.com/getsentry/sentry-javascript-bundler-plugins/blob/HEAD/CHANGELOG.md#411) - fix(react-native): Enhance fragment detection for indirect references ([#767](getsentry/sentry-javascript-bundler-plugins#767)) ## [v4.1.0](https://github.com/getsentry/sentry-javascript-bundler-plugins/blob/HEAD/CHANGELOG.md#410) - feat(deps): Bump [@sentry/cli](https://github.com/sentry/cli) to 2.51.0 [#786](getsentry/sentry-javascript-bundler-plugins#786) - feat(core): Add flag for disabling sourcemaps upload [#785](getsentry/sentry-javascript-bundler-plugins#785) - fix(debugId): Add guards for injected code to avoid errors [#783](getsentry/sentry-javascript-bundler-plugins#783) - docs(options): Improve JSDoc for options [#781](getsentry/sentry-javascript-bundler-plugins#781) - feat(core): Expose method for injecting debug Ids from plugin manager [#784](getsentry/sentry-javascript-bundler-plugins#784) ## [v4.0.2](https://github.com/getsentry/sentry-javascript-bundler-plugins/blob/HEAD/CHANGELOG.md#402) - fix(core): Make `moduleMetadata` injection snippet ES5-compliant ([#774](getsentry/sentry-javascript-bundler-plugins#774)) ## [v4.0.1](https://github.com/getsentry/sentry-javascript-bundler-plugins/blob/HEAD/CHANGELOG.md#401) - fix(core): Make plugin inject ES5-friendly code ([#770](getsentry/sentry-javascript-bundler-plugins#770)) - fix(core): Use `renderChunk` for release injection for Rollup/Rolldown/Vite ([#761](getsentry/sentry-javascript-bundler-plugins#761)) Work in this release was contributed by [@grushetsky](https://github.com/grushetsky). Thank you for your contribution! ## [v4.0.0](https://github.com/getsentry/sentry-javascript-bundler-plugins/blob/HEAD/CHANGELOG.md#400) ##### Breaking Changes - (Type change) Vite plugin now returns `VitePlugin` type instead of `any` - Deprecated function `getBuildInformation` has been removed ##### List of Changes - feat(core)!: Remove `getBuildInformation` export ([#765](getsentry/sentry-javascript-bundler-plugins#765)) - feat(vite)!: Update return type of vite plugin ([#728](getsentry/sentry-javascript-bundler-plugins#728))
The injected code snippets where changed in those PRs:
renderChunk
for release injection for Rollup/Rolldown/Vite #761This PR adds a try/catch guard around the snippet to avoid errors.
closes #782