Support Windows-style path in RewriteFrame's default iteratee #2319
Conversation
The previous default iteratee does not work in Windows because it only detects
paths that begin with `/`. This change ensures paths that begin with `C:\\` are
also accounted for.
This implementation requires that the specified `root` uses Unix-style paths.
So this will not work:
new RewriteFrames({
root: 'C:\\Program Files\\Apache\\www'
})
But this will:
new RewriteFrames({
root: '/Program Files/Apache/www'
})
Should this behaviour change or be noted in the official docs?
|
This implementation requires that the specified new RewriteFrames({
root: 'C:\\Program Files\\Apache\\www'
})But this will: new RewriteFrames({
root: '/Program Files/Apache/www'
})Should this behaviour change or be noted in the official docs? |
| @@ -24,8 +24,12 @@ export class RewriteFrames implements Integration { | |||
| * @inheritDoc | |||
| */ | |||
| private readonly _iteratee: StackFrameIteratee = (frame: StackFrame) => { | |||
| if (frame.filename && frame.filename.startsWith('/')) { | |||
| const base = this._root ? relative(this._root, frame.filename) : basename(frame.filename); | |||
| // Check if the frame filename begins with `/` or a Windows-style prefix such as `C:\\` | |||
There was a problem hiding this comment.
Windows-style prefix is C:\, not C:\\ but the regexp is correct, so just the comment needs a small fix
|
Yes, the docs update would be awesome! It has to be changed here https://github.com/getsentry/sentry-docs/blob/7d2b34cde1b7035d3ef1053cd3f746944e01981c/src/collections/_documentation/platforms/node/pluggable-integrations.md#rewriteframes and here https://github.com/getsentry/sentry-docs/blob/f8e6df79e9f9f0151d59e22c870ee8b123e34918/src/collections/_documentation/platforms/javascript/index.md If you want to send a PR there it'd be great. And at the same time, if you could copy 1:1 the text from Node to JavaScript it'd be even even better (as this one got updated :)). Thanks! |
| if (frame.filename && /^(\/|[A-Z]:\\)/.test(frame.filename)) { | ||
| const filename = frame.filename | ||
| .replace(/^[A-Z]:/, '') // remove Windows-style prefix | ||
| .replace(/\\/g, '/'); // replace all `\\` instances with `/` |
There was a problem hiding this comment.
It looks like this has a subtle bug, because \ is a valid character in a unix path.
You can have, for example, /tmp/a\b, where a\b is the filename, or the name of a directory on the path above our code. If you do that, this will rewrite it to /tmp/a/b, which probably doesn't exist, and so that'll break things later.
Probably rare, but easy to fix - I think you just want to do these replacements only in the Windows case (where backslashes are not valid in paths), rather than all the time.
|
I just hit this issue as well, and it's causing me lots of extra noise from error reports in Windows where my path rewriting is broken. I've left one small comment, but regardless this would be super useful for me, it'd be great to get this merged 👍. Let me know if there's anything I can do to help. |
|
Also docs getsentry/sentry-docs#1399 |
The previous default iteratee does not work in Windows because it only detects paths that begin with
/. This change ensures paths that begin withC:\\are also accounted for.