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.

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

Filter out headers with empty values #5866

Closed
wants to merge 3 commits into from
Closed

Conversation

mhoeger
Copy link
Contributor

@mhoeger mhoeger commented Apr 7, 2020

No description provided.

@mhoeger
Copy link
Contributor Author

mhoeger commented Apr 10, 2020

Note - we probably don't want to filter out for everything because this behavior is only buggy for Node.js. Correct approach is through capabilities and either explicitly opting out of those or introducing a new headers property that is typed correctly.

@mathewc
Copy link
Member

mathewc commented Apr 23, 2020

Why do we have to do this?

@mhoeger
Copy link
Contributor Author

mhoeger commented Apr 23, 2020

@mathewc - because of the way we implemented headers in our protobuf schema (as a map<string, string>) and the "map" type doesn't accept empty values for runtimes like node. Causes buffer overrun and corrupts other parts of the message :(

Azure/azure-functions-nodejs-worker#142 and Azure/azure-functions-language-worker-protobuf#21

@fabiocav
Copy link
Member

fabiocav commented May 4, 2020

@mhoeger this was originally opened against dev, which is now 3.0. Do you need to retarget?

@mhoeger
Copy link
Contributor Author

mhoeger commented May 4, 2020

@fabiocav - yes I need to! I think that I'll do this as through a capability flag too so that there's no chance of a break on languages that don't experience this bug. Closing PR and giving it a proper bug issue to track: #5975

@mhoeger mhoeger closed this May 4, 2020
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.

None yet

3 participants