fix(rollup): Prevent double-injection of debug ID #827
Conversation
| // Check if a debug ID has already been injected to avoid duplicate injection (e.g. by another plugin or Sentry CLI) | ||
| const chunkStartSnippet = code.slice(0, 2000); | ||
| const chunkEndSnippet = code.slice(-500); | ||
|
|
||
| if ( | ||
| chunkStartSnippet.includes("_sentryDebugIdIdentifier") || | ||
| chunkEndSnippet.includes("//# debugId=") | ||
| ) { | ||
| return null; // Debug ID already present, skip injection | ||
| } |
There was a problem hiding this comment.
l: Looking at the tests, I see that we slice the code here for performance reasons (TIL that String.slice is O(1) while Array.slice is O(n)) .
I think this is a good idea but I do have a concern here: A rollup banner plugin could inject a lengthy license banner into a file which could mean the debugId snippet only comes after the first 2000 characters (unlikely admittedly). We can say we're fine with this for the moment and remove the slicing if someone reports this. I'm just a bit worried that this would be hard for users and for us to find. Any thoughts on this?
There was a problem hiding this comment.
Yeah, this is the only concern I have as well. I just checked the starting snippet as it's most likely that the debug ID is in there and checking the whole file might create a performance problem (we don't know how large the file gets).
Or maybe we should just do String.includes() on the whole file as it's probably not as bad as it seems 🤔
It should be O(n+m) and as the snippet should be somewhere at the beginning, the search will be done quite fast. However, most files don't already have the snippet, so it would look through the whole file in most cases which increases search times drastically.
There are 3 options:
String.includes()on whole file: will always detect the snippet but increases search times a lotString.includes()only for the top snippet (like now) - we would potentially miss some casesString.includes()only for the top snippet (but a larger snippet) - we would probably catch more cases but still not all of them
If we change something, I would only increase the slicing limit but not include the whole file. It's still better than before as before we didn't check for the existence of the snippet at all
There was a problem hiding this comment.
yeah, let's do 3 -- sounds like a reasonable compromise :D thanks for writing up the options! I agree that the performance concern when checking the entire file is valid.
There was a problem hiding this comment.
I'll increase it to 6000 chars - this is approximately 150 LOC
| expect(result).not.toBeNull(); | ||
| expect(result?.code).toMatch(/^"use strict";.*;{try/); | ||
| }); |
There was a problem hiding this comment.
l: wdyt about using toMatchInlineSnapshot here instead (this applies to most added tests here, but this one in particular would show much nicer where the snippet is actually injected)
|
Also, thanks for adding the additional tests! |
The plugin just injects the debug ID, but doesn't check if it is already in the file. Injecting multiple debug IDs result in problems (like the linked issue below):
getsentry/sentry-javascript#18519
This PR checks the beginning and end of the file for debug ID content. The numbers for that are a best estimate, so we don't need to read the whole file (potentially resource-intense).