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.

Sign up for GitHub

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

Support Windows-style path in RewriteFrame's default iteratee #2319

Closed
wants to merge 1 commit into from
Closed

Support Windows-style path in RewriteFrame's default iteratee #2319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions packages/integrations/src/rewriteframes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:\\`
Copy link
Contributor

Choose a reason for hiding this comment

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

Windows-style prefix is C:\, not C:\\ but the regexp is correct, so just the comment needs a small fix

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 `/`
Copy link

Choose a reason for hiding this comment

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

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.

const base = this._root ? relative(this._root, filename) : basename(filename);
frame.filename = `app:///${base}`;
}
return frame;
Expand Down
37 changes: 37 additions & 0 deletions packages/integrations/test/rewriteframes.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { RewriteFrames } from '../src/rewriteframes';
let rewriteFrames: RewriteFrames;
let messageEvent: Event;
let exceptionEvent: Event;
let windowsExceptionEvent: Event;

describe('RewriteFrames', () => {
beforeEach(() => {
Expand Down Expand Up @@ -38,6 +39,24 @@ describe('RewriteFrames', () => {
],
},
};
windowsExceptionEvent = {
exception: {
values: [
{
stacktrace: {
frames: [
{
filename: 'C:\\www\\src\\app\\file1.js',
},
{
filename: 'C:\\www\\src\\app\\file2.js',
},
],
},
},
],
},
};
});

describe('default iteratee appends basename to `app:///` if frame starts with `/`', () => {
Expand All @@ -58,6 +77,18 @@ describe('RewriteFrames', () => {
});
});

describe('default iteratee appends basename to `app:///` if frame starts with `C:\\`', () => {
beforeEach(() => {
rewriteFrames = new RewriteFrames();
});

it('trasforms windowsExceptionEvent frames', () => {
const event = rewriteFrames.process(windowsExceptionEvent);
expect(event.exception!.values![0].stacktrace!.frames![0].filename).toEqual('app:///file1.js');
expect(event.exception!.values![0].stacktrace!.frames![1].filename).toEqual('app:///file2.js');
});
});

describe('can use custom root to perform `relative` on filepaths', () => {
beforeEach(() => {
rewriteFrames = new RewriteFrames({
Expand All @@ -76,6 +107,12 @@ describe('RewriteFrames', () => {
expect(event.exception!.values![0].stacktrace!.frames![0].filename).toEqual('app:///src/app/file1.js');
expect(event.exception!.values![0].stacktrace!.frames![1].filename).toEqual('app:///src/app/file2.js');
});

it('trasforms windowsExceptionEvent frames', () => {
const event = rewriteFrames.process(windowsExceptionEvent);
expect(event.exception!.values![0].stacktrace!.frames![0].filename).toEqual('app:///src/app/file1.js');
expect(event.exception!.values![0].stacktrace!.frames![1].filename).toEqual('app:///src/app/file2.js');
});
});

describe('can use custom iteratee', () => {
Expand Down