fix(bundler-plugins): Preserve directive prologues during bundle injection

[Andarist]

Bundler plugins inject Sentry code into generated bundles. In some outputs the injection could be placed before directive prologues such as "use strict", causing JavaScript to stop recognizing them as directives.

I found this issue to affect some production cases - I don't have access to their sources so I can't fully say how the original code was authored and what exactly made it lose the strict mode, but the generated output looked like this:

try{!function(){var e="undefined"!=typeof window?window:"undefined"!=typeof global?global:"undefined"!=typeof globalThis?
  globalThis:"undefined"!=typeof self?self:{},t=(new e.Error).stack;t&&(e._sentryDebugIds=e._sentryDebugIds||{},e._sentryDebugIds[t]="612bd636-5fcc-473a-bdd0-20460245872c",e._sentryDebugIdIdentifier="sentry-dbid-612bd636-5fcc-473a-bdd0-
  20460245872c")}()}catch(e){}"use strict";(self.webpackChunk_N_E=self.webpackChunk_N_E||[]).push([[120],{67232:function(e,t,n){var r,l=n(41498),a=n(90413),o={usingClientEntryPoint:!1,Events:null,Dispatcher:{current:null}};function i(e){var
  t="https://react.dev/errors/"+e;if(1<arguments.length){t+="?args[]="+encodeURIComponent(arguments[1]);

Given the sentry code was injected before the strict mode directive, that changed the meaning of arguments[1] at this position in the app code:

function aW(e, t) {
  if (null !== (t = null !== (t = t.updateQueue) ? t.lastEffect : null)) {
    var n = (t = t.next);
    do {
      if ((n.tag & e) === e) {
        var r = n.create,
          l = n.inst;
        /* arguments[1] no longer refer to the original argument */
        r = r();

        l.destroy = r;
      }
      n = n.next;
    } while (n !== t);
  }
}

That's because in the sloppy mode the assignment to t before the arguments[1] reference changes the arguments content too 🫠 . You can test it out using this isolated sample:

function test(foo) {
  foo = 2;
  console.log(arguments[0]); // 2
}

test(1);

function testStrict(foo) {
  "use strict";
  foo = 2;
  console.log(arguments[0]); // 1
}

testStrict(1);

This PR:

• adds a bunch of tests for edge cases and for source mapping behavior (the latter was already working OK but didn't quite have the coverage)
• replaces simple regex with a more spec-compliant tiny scanner so the proper injection point can be found
• in Webpack BannePlugin can only prepend/append text, as far as I know, it can't just inject into an arbitrary position. So it was replaced with a compilation hook and ReplaceSource plugin. That allows for a fine-grained control at the asset level
• in the case of Rollup, this PR only slightly changes the insertion point calculation - but it doesn't replace the overall mechanism/hooks used
• esbuild has not required any fixes because inject API handles this for us

---

AI disclosure: I have steered it a bunch myself and I understand each line of code added. I ensured (using my own judgement) that all of this matches the project's style and goal but ofc I have much less context on that than the maintainers here. That said, I can address any PR feedback thrown my way.

[timfish]

Seems sensible to me!

[Andarist]

@timfish could u re-approve the CI run? I fixed the test failures (hopefully the last ones). I'd test it out properly locally... but I'm facing a stupid issue of this project having such a big dep graph that I'm currently running out of disk space 🫠 I need to clean it up but didn't have time for that right now. FWIW, I was totally testing this... but through proxy isolated pnpm-based projects. So I, from the start, tested he behavioral changes of this PR - but then I faced some outdated snapshots here 😢

[Andarist]

@timfish I believe I fixed the remaining issues in the test harness. I also tested it through your github actions on my fork (see this run). Some e2e tests are failing but that's because I don't have the required secrets on my fork.

Could you now retrigger the CI here?

[Andarist]

I had to sync this with develop given the merge conflicts, this this will require now a CI re-run.

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.

^{Reviewed by Cursor Bugbot for commit 5f98e4c. Configure here.}

[timfish]

Thanks, will review this week!

[timfish]

Thanks, the scanner and the move to processAssets both look right to me. A few follow-ups inline, plus one that has no diff line to hang on: COMMENT_USE_STRICT_REGEX in packages/bundler-plugins/src/core/index.ts has no users left after this change. Can you delete it (and its CodeQL note)?

[timfish]

We already have a scanner for this in packages/nextjs/src/config/loaders/valueInjectionLoader.ts (findInjectionIndexAfterDirectives), and @sentry/nextjs already imports from @sentry/bundler-plugins/core. Could you export this one from core and switch the Next.js loaders to it, so the edge cases you fixed here (CR-only, U+2028/9, ++/--, in/instanceof) apply there too? Happy for that to be a follow-up PR if you'd rather keep this one focused.

[Andarist]

addressed this in 1679219

[timfish]

Nit: this copies the whole remainder of the chunk to test at most a dozen characters. A bounded slice (position + 'instanceof'.length + 1) gives the same result.

[Andarist]

addressed in 1679219#diff-ee8efc138ed4ece111e3e545a52b287db0855ed22d471cf72dc329ed380f9978

[sentry]

Bug: The regex in canContinueStringExpression handles ++ and -- but misses single unary + and - operators, causing incorrect parsing of directive prologues.
_{Severity: MEDIUM}

Suggested Fix

Update the regular expression in canContinueStringExpression to also treat single unary + and - operators as tokens that cannot continue a string expression. This will ensure they are handled consistently with ++ and -- for directive parsing.

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: packages/bundler-plugins/src/core/get-code-injection-position.ts#L73

Potential issue: The function `canContinueStringExpression` uses a regular expression to
detect non-continuation tokens after a string expression. This regex explicitly checks
for prefix increment (`++`) and decrement (`--`) operators but fails to account for
single unary plus (`+`) and minus (`-`) operators. As a result, if code like `"use
strict"\n+something` is encountered, the unary plus is not correctly identified as a new
expression, leading to incorrect directive prologue handling. This represents an
incomplete implementation of unary operator detection.

[timfish]

The bot claims "use strict"\n+something should end the prologue. It should not.