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

Headers cased incorrectly #274

Closed
harshithkashyap opened this issue Dec 10, 2019 · 12 comments
Closed

Headers cased incorrectly #274

harshithkashyap opened this issue Dec 10, 2019 · 12 comments
Labels
breaking bug docs P3 v4 model 🚀 Related to the new V4 programming model
Milestone

Comments

@harshithkashyap
Copy link

Investigative information

Please provide the following:

  • Core Tools: Azure Functions Core Tools (2.7.1948 Commit hash: 29a0626ded3ae99c4111f66763f27bb9fb564103)
  • Function Runtime Version: 2.0.12888.0
  • Timestamp: NA
  • Function App name: NA
  • Function name(s) (as appropriate): NA
  • Invocation ID: NA
  • Region: NA

Repro steps

Provide the steps required to reproduce the problem:

  1. Create a function with httpTrigger.
  2. Trigger the function using the endpoint and include a header with an empty value.
    Screenshot from 2019-12-10 23-19-08

Expected behavior

Headers with empty values should be parsed as empty string.
The following screenshots shows the headers with empty values being correctly parsed by ngrok.
Screenshot from 2019-12-10 23-33-56
Screenshot from 2019-12-10 23-34-14

Actual behavior

Instead, the other subsequent headers are parsed for the empty header.
Screenshot from 2019-12-10 23-24-12

Known workarounds

There are no known workarounds as of now as it's difficult to predict the empty header without access to raw http request.

Related information

  • Programming language used: TypeScript.
@mhoeger
Copy link
Contributor

mhoeger commented Dec 18, 2019

Hi @harshithkashyap, the reason why we don't pass this information is because of a serializing issue that we discovered with using proto3.

Here's the original issue: #142

And this is the fix needed: Azure/azure-functions-language-worker-protobuf#21

While this is unavailable, our recommended workaround is to access header data through context.bindingData.headers, where the same information is available)

module.exports = async function (context, req) {
  return {
    status: 200,
    body: {...context.bindingData.headers},
    headers: {
        'content-type': 'application/json',
    },
  };
}

@mhoeger mhoeger self-assigned this Dec 18, 2019
@mhoeger mhoeger modified the milestones: triag, Triaged Dec 18, 2019
@mhoeger mhoeger added the bug label Dec 18, 2019
@harshithkashyap
Copy link
Author

@mhoeger I can now see the header under context.bindingData.headers object but it looks like the first character in the header is lowercased and the original casing is maintained for others. Eg, X-CUSTOM-HEADER becomes x-CUSTOM-HEADER. The headers should not be case sensitive and ideally be lower cased even in bindingData object.

@mhoeger mhoeger changed the title Empty Headers are parsed incorrectly Headers cased incorrectly Mar 5, 2020
@mhoeger
Copy link
Contributor

mhoeger commented Mar 5, 2020

Adding to this issue - in bindingData, it's "x-CUSTOM-HEADER". In req.headers, it's 'x-custom-header', and what we expect is "X-CUSTOM-HEADER"

@vicancy
Copy link
Member

vicancy commented Mar 9, 2020

Any ETA to fix this?

@mhoeger
Copy link
Contributor

mhoeger commented Mar 9, 2020

@vicancy - Don't have a good ETA on this item. Either we patch headers to have both combinations of casing (which is a bit strange but not a breaking change) or we fix the first letter to not become lower cased (which is a breaking change for people who are relying on that).

One thing that may be helpful is that the request object by default has a "get" method that takes a header name and gets the property, regardless of casing. I think using req.get("X-CUSTOM-HEADER"); is currently the best way forward..!

@vicancy
Copy link
Member

vicancy commented Mar 10, 2020

TBH, fixing the req.headers parser sounds the most important one to me. It is indeed unexpected behavior and is a bug.

@mhoeger
Copy link
Contributor

mhoeger commented Mar 10, 2020

@vicancy - sorry what do you mean? HTTP headers are expected to be case insensitive. The reason why it's case sensitive is because we expose headers as a properties of a JavaScript object (in addition to exposing it through the req.get method, which is case insensitive).

If we pushed out a fix today to preserve a header's casing to the best of our ability, current users who are looking for something like "x-CUSTOM-HEADER" would immediately be broken. I know that the casing is unexpected but I don't know what you mean by "fixing the parser sounds the most important." What is your suggestion there?

@vicancy
Copy link
Member

vicancy commented Mar 12, 2020

I mean the original issue: "Empty Headers are parsed incorrectly" in req.headers

@mhoeger
Copy link
Contributor

mhoeger commented Mar 30, 2021

Update: "Empty Headers are parsed incorrectly" issue has been fixed but not headers cased incorrectly issue.

@glasser
Copy link

glasser commented Apr 1, 2021

Just to clarify: when writing a function that uses @azure/functions and reads headers, is the correct way to read the header to use req.headers['lowercase-version-of-the-header-you-care-about']?

@mhoeger refers to req.get but I don't see that in the TypeScript definitions file.

@ejizba
Copy link
Contributor

ejizba commented Jan 28, 2022

I can confirm this is still happening because of this logic. I used this to test:

curl -H "X-CUSTOM-HEADER: test" http://localhost:7071/api/HttpTrigger1

We should at least decide on best practice and make sure it's added to the TypeScript definitions. Longer term we should consider removing that extra camel case logic because it may be more trouble than it's worth.

@ejizba ejizba added this to the Backlog Candidates milestone Jan 28, 2022
@ejizba ejizba added v4 model 🚀 Related to the new V4 programming model and removed v4 model 🚀 Related to the new V4 programming model labels Mar 21, 2022
@lilyjma lilyjma added the v4 model 🚀 Related to the new V4 programming model label Jul 18, 2022
@ejizba ejizba removed this from the Backlog Candidates milestone Aug 25, 2022
@ejizba ejizba added this to the August 2022 milestone Aug 25, 2022
ejizba added a commit to Azure/azure-functions-nodejs-library that referenced this issue Aug 25, 2022
Main points:
- We don't want `toCamelCase` to call `fromRpcTypedData` because it traverses an entire object recursively and only the top level objects should be RpcTypedData
- We will remove triggerMetadata from http/timer triggers. That information is best found on the request/timer objects instead

Related to all of these bugs:
Azure/azure-functions-nodejs-worker#607
Azure/azure-functions-nodejs-worker#274
Azure/azure-functions-nodejs-worker#204
Azure/azure-functions-nodejs-worker#388
@ejizba
Copy link
Contributor

ejizba commented Aug 25, 2022

For the existing programming model, we recommend using request.get('header-name') because it is case insensitive. I have added the type for that in Azure/azure-functions-nodejs-library#10

For the new programming model, this was addressed in Azure/azure-functions-nodejs-library@efa5fb3. I've started a doc here with more information: https://aka.ms/AzFuncNodeV4

@ejizba ejizba closed this as completed Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking bug docs P3 v4 model 🚀 Related to the new V4 programming model
Projects
None yet
Development

No branches or pull requests

6 participants