Apparent buffer overrun affecting the Request object passed to the function

[daniel-rocha]

Suppose a function requires a query string parameter, like “productId”. Previously, if either the key wasn’t present in the query string, or if the value was null, the corresponding req. in node would be null, and we could do validation on it to decide how to proceed with the function.

Investigative information

Please provide the following:

• Timestamp: 2018-11-01T01:48:59.948
• Function App name: noderequestbugtest
• Function name(s) (as appropriate): https://noderequestbugtest.azurewebsites.net/api/NodeRequestBugFunction
• Invocation ID: Id=abf562a9-9b5e-4803-b565-994985b0e880
• Region: Central US

Repro steps

Provide the steps required to reproduce the problem:

1. Write a function in Node that requires a parameter from the query string, e.g. "productId"
2. Call the function passing "productId=something". Verify it works.
3. Call the function passing either "http://functionapp/api/FunctionName?productId" or "http://functionapp/api/FunctionName?productId="

Expected behavior

Provide a description of the expected behavior.

req.query.productId should be null or undefined.

Actual behavior

req.query.productId is full of garbage, including the entire header of the HTTP request:

`ser-agent�rMozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.77 Safari/537.36��

�upgrade-insecure-requests��1�>

�x-waws-unencoded-url�&/api/NodeRequestBugFunction?productId=�!

client-ip��100.72.212.131:51978��

�is-service-tunneled��0�4

x-arr-log-id�$37c75121-e24c-4afb-8eb2-8d0af7bc3fdb�6

�disguised-host�$noderequestbugtest.azurewebsites.net�*

�x-site-deployment-id��NodeRequestBugTest�<

�was-default-hostname�$noderequestbugtest.azurewebsites.net�8

�x-original-url�&/api/NodeRequestBugFunction?productId=�&

�x-forwarded-for��167.220.255.10:2412���

x-arr-ssl���2048|256|C=US, S=Washington, L=Redmond, O=Microsoft Corporation, OU=Microsoft IT, CN=Microsoft IT TLS CA 4|CN=*.azurewebsites.net��

�x-forwarded-proto��httpsz�`

Known workarounds

No known workarounds, which is important because prevents validation of query string parameters.

Related information

Provide any related information

• Programming language used: Node
• Links to source
• Bindings used

module.exports = async function (context, req) {
  context.log('GetProductDescription function processed a request.');
  context.log('Requested product id is ' + req.query.productId);
 
  const productId = req.query.productId || "";
  
  if (productId || productId.length > 0) {
    context.res = {
      // status: 200, /* Defaults to 200 */
      headers: {
        "Content-Type": "text/plain"
      },
      body: `The product name for your product id ${productId} is Starfruit Explosion`
    };
  }
  else {
    context.res = {
      status: 400,
      body: "Please pass a productId on the query string or in the request body"
    };
  }
};

-->

[mhoeger]

Thanks @daniel-rocha, @dahatake has been investigating this issue recently too. It comes from a bug in code generated by protobuf.js. We're currently working on the fix, but in the meantime could you try checking context.bindingData.query.productId? This will work as expected for you.

Do you also know which version this was working for you? Is this a recent regression?

[fabiocav]

@mhoeger are we targeting Sprint 37 for this? If so, can we assign to the sprint?

[mhoeger]

The root cause of this issue is that we use a "map" in our protobuf file.

From the protocol buffers docs:

If you provide a key but no value for a map field, the behavior when the field is serialized is language-dependent. In C++, Java, and Python the default value for the type is serialized, while in other languages nothing is serialized.

In combination with protobuf.js, this results in generated code that overflows.

                    case 15:
                        reader.skip().pos++;
                        if (message.query === $util.emptyObject)
                            message.query = {};
                        key = reader.string();
                        reader.pos++;
                        message.query[key] = reader.string();
                        break;

[mhoeger]

This specific concern will be addressed in Azure/azure-functions-host#3789. A longer-term fix for valid empty values in a mapping will be addressed here: Azure/azure-functions-language-worker-protobuf#21