Skip to content

fix(node): Compression headers leak between requests #5203

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

Merged
merged 1 commit into from
Jun 3, 2022

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Jun 3, 2022

#5139 introduced a subtle bug where options.headers was modified which causes headers to leak between requests. This means requests after a compressed request will be incorrectly marked with content-encoding: gzip.

Copy link
Contributor

@lforst lforst left a comment

Choose a reason for hiding this comment

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

Nice thanks!

@timfish
Copy link
Collaborator Author

timfish commented Jun 3, 2022

This took me so long to work out what was going on and I found it after telling Abhi the PR was good to go...

I'm trying to think how we can make bugs like this less likely. Making the options ReadOnly would have stopped this as options should be immutable?

@lforst
Copy link
Contributor

lforst commented Jun 3, 2022

Yup. Thought exactly the same thing. readonly would have caught this. I'll bounce this off the rest of the team and create an issue for it if we think it's a good idea.

@lforst lforst merged commit f755bb3 into getsentry:7.x Jun 3, 2022
@timfish
Copy link
Collaborator Author

timfish commented Jun 3, 2022

At a minimum we could make the parameters in transports Readonly<T>.

export function makeNodeTransport(options: Readonly<NodeTransportOptions>): Transport {

We can't make transportOptions on the client options readonly because of this:

// Until node supports global TextEncoder in all versions we support, we are forced to pass it from util
options.transportOptions = {
textEncoder: new TextEncoder(),
...options.transportOptions,
};

@lforst
Copy link
Contributor

lforst commented Jun 3, 2022

I think we could rewrite the constructor of the clients so that it passes a new options object to super instead of a modified one. But yeah bare minimum we could make the node transport options readonly. I believe it's generally good practice to make as much readonly as possible.

lobsterkatie added a commit that referenced this pull request Jun 7, 2022
Note: This is an exact duplicate of #5139 and #5203, both originally by @timfish, which accidentally got merged into the 7.x branch rather than master.

#5139:

This is mostly stolen straight from the [Electron transport](https://github.com/getsentry/sentry-electron/blob/master/src/main/transports/electron-net.ts).

This will help with attachments!

#5203:

#5139 introduced a subtle bug where `options.headers` was modified which causes headers to leak between requests. This means requests after a compressed request will be incorrectly marked with `content-encoding: gzip`.
@timfish timfish deleted the fix/compression-headers branch October 5, 2022 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants