set Content-Length header in mapApiGatewayEventToHttpRequest #147

HotelCalifornia · 2018-05-30T20:48:11Z

Issue #, if available:
#106
Description of changes:

without content-length specified, it seems the body of DELETE requests get ignored
per RFC 2616, section 4.3:

The presence of a message-body in a request is signaled by the inclusion of a Content-Length or Transfer-Encoding header field in the request's message-headers. A message-body MUST NOT be included in a request if the specification of the request method (section 5.1.1) does not allow sending an entity-body in requests. A server SHOULD read and forward a message-body on any request; if the request method does not include defined semantics for an entity-body, then the message-body SHOULD be ignored when handling the request.
- in other words, whether or not it is recommended to have a body in a DELETE request, if the content-length header (or the transfer-encoding header) is set, the message body should be forwarded
concerns:
- I'm not sure whether it is necessary to perform more type checking on event.body. for instance, if it is an object, it should be passed through a call to JSON.stringify before getting the byte length not necessary
- ~~the header should probably be set only if not already specified~~ 1ef34b8

I would love feedback on this 🙇

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

- without content-length specified, it seems the body of DELETE requests get ignored - per RFC 2616, section 4.3: The presence of a message-body in a request is signaled by the inclusion of a Content-Length or Transfer-Encoding header field in the request's message-headers. A message-body MUST NOT be included in a request if the specification of the request method (section 5.1.1) does not allow sending an entity-body in requests. A server SHOULD read and forward a message-body on any request; if the request method does not include defined semantics for an entity-body, then the message-body SHOULD be ignored when handling the request. - in other words, whether or not it is recommended to have a body in a DELETE request, if the content-length header (or the transfer-encoding header) is set, the message body should be forwarded - concerns: - I'm not sure whether it is necessary to perform more type checking on `event.body`. for instance, if it is an object, it should be passed through a call to `JSON.stringify` before getting the byte length - the header should probably be set only if not already specified This commit is one possible solution to CodeGenieApp#106

HotelCalifornia · 2018-05-30T21:01:57Z

I recognize now that this may be the same change as in #130, though I can now confirm that this issue is also the cause of #106

will now only set the content-length header if the request has a body and the header is not already set

brettstack · 2018-06-20T22:07:57Z

"if it is an object, it should be passed through a call to JSON.stringify before getting the byte length" - event.body will always be a string, so no concern here.

Could you fix the failing tests and I'll get this merged in soon.

brettstack · 2018-06-20T22:09:46Z

Could you also add a comment: // NOTE: API Gateway is not setting content-length header on DELETE requests even when they have a body

HotelCalifornia · 2018-06-20T22:39:53Z

Yeah, I can get those addressed tomorrow

HotelCalifornia · 2018-06-21T14:33:28Z

note that by the section of RFC 2616 I quoted above, the bit about content-length applies to other normally bodiless requests, like GET (i.e. if the request has a body, content-length should be set, regardless of whether it is good practice). I bring this up because the failing test cases send a GET request with a body:

https://github.com/awslabs/aws-serverless-express/blob/42a5501f2ef393461e6d53c4ab28ab343d83044b/__tests__/index.js#L46-L62

the header is not set when the dummy event is cloned since my changes add the content-length inside this call

https://github.com/awslabs/aws-serverless-express/blob/42a5501f2ef393461e6d53c4ab28ab343d83044b/__tests__/index.js#L59

which means failure at the later call

https://github.com/awslabs/aws-serverless-express/blob/42a5501f2ef393461e6d53c4ab28ab343d83044b/__tests__/index.js#L86

I'm not really sure what the best way to go about remedying this would be. Maybe something like

            'x-apigateway-event': encodeURIComponent(JSON.stringify({
                'Content-Length': Buffer.byteLength('Hello serverless'),
                ...r.eventClone
            })),

(or the [functionally] equivalent Object.assign call)

brettstack · 2018-06-21T15:05:46Z

Okay. Your PR is method-agnostic so should work for GET also. Maybe make the comment a little more generic

- fix failing test cases - add comment about motivation for change in index.js

brettstack · 2018-06-21T16:55:06Z

index.js

@@ -36,6 +36,10 @@ function mapApiGatewayEventToHttpRequest(event, context, socketPath) {
    const eventWithoutBody = Object.assign({}, event)
    delete eventWithoutBody.body

+    // NOTE: API Gateway is not setting Content-Length header on any requests even when they have a body  


I don't think this is true. It correctly sets it for POST and PUT, right? Otherwise we'd be seeing this error on every method. Confirmed methods where it doesn't appear are GET and DELETE and I wouldn't be surprised if OPTIONS also didn't have the header.

brettstack · 2018-06-21T16:55:29Z

index.js

@@ -60,7 +64,7 @@ function forwardResponseToApiGateway(server, response, context) {
            const bodyBuffer = Buffer.concat(buf)
            const statusCode = response.statusCode
            const headers = response.headers
-
+            


Could you remove this address whitespace?

- only if body is present and there are no headers - seems hacky, but ensures that the `x-apigateway-event` header is set properly(?) later (i.e. including the content-length field)

HotelCalifornia · 2018-06-21T20:23:15Z

re: does it correctly set for POST/PUT?

@ajstocchetti set up a little test for this. running the following lambda as an API gateway integration:

const awsServerlessExpress = require('aws-serverless-express');
const app = require('./app');
const server = awsServerlessExpress.createServer(app);

exports.handler = (event, context) => {
    const headers = event.headers;
    const len = headers && headers['Content-Length'] ? headers['Content-Length'] : null;
    const cl = {
        method: event.httpMethod,
        headers,
        contentlength: len,
        body: !!event.body,
    };
    console.log(cl);
    return awsServerlessExpress.proxy(server, event, context);
}

produces the following output in cloudwatch:

2018-06-21T20:14:39.195Z	b94aa582-758f-11e8-8c38-4dde4dcb5d6c	{ method: 'POST',
headers: 
{ Accept: '*/*',
'accept-encoding': 'gzip, deflate',
'cache-control': 'no-cache',
'Content-Type': 'application/json',
Host: 'xxxxxxxxxx.execute-api.us-east-1.amazonaws.com',
'Postman-Token': '6293826b-1004-403c-a954-9b4da0747930',
'User-Agent': 'PostmanRuntime/7.1.5',
'X-Amzn-Trace-Id': 'Root=1-5b2c072e-375932d0e0bc47d2658c8028',
'X-Forwarded-For': 'XX.XX.XX.XX',
'X-Forwarded-Port': '443',
'X-Forwarded-Proto': 'https' },
contentlength: null,
body: true }

HotelCalifornia · 2018-06-21T21:11:48Z

also, I'm not sure why that last test is failing, but I don't think it has to do specifically with the changes I've made...

brettstack · 2018-06-21T21:35:09Z

It's failing in latest version of Node due to Buffer deprecation. I'll remove it from Travis config.

I'll open a ticket internally to investigate the content-length header not being sent.

Thanks! I'll have my integ tests out for PR soon. I'll merge this in immediately after that gets merged in.

brettstack · 2018-06-22T22:09:57Z

Could you rebase? The travis config has changed to support the new tests. You may also need to fix some tests. Alternatively if you want to push this to develop branch I can merge and fix from there.

HotelCalifornia · 2018-06-25T15:07:41Z

@brettstack I've changed the base branch to develop

brettstack · 2018-06-25T15:08:13Z

I've implemented this in a separate branch with some changes. The integ tests are already paying off as they found an issue with doing this with base64Encoded requests. Thanks for all your help and patience.

* Certain requests (e.g. GET and DELETE) would fail when they included a body due to missing content-length header. See #147, #106, #130

<a name="3.3.5"></a> ## [3.3.5](v3.3.4...v3.3.5) (2018-08-20) ### Bug Fixes * apply Content-Length header when missing ([b0927b8](b0927b8)), closes [#147](#147) [#106](#106) [#130](#130) * apply Content-Length header when missing ([#175](#175)) ([c2f416b](c2f416b))

<a name="3.3.5"></a> ## [3.3.5](CodeGenieApp/serverless-express@v3.3.4...v3.3.5) (2018-08-20) ### Bug Fixes * apply Content-Length header when missing ([b0927b8](CodeGenieApp/serverless-express@b0927b8)), closes [#147](CodeGenieApp/serverless-express#147) [#106](CodeGenieApp/serverless-express#106) [#130](CodeGenieApp/serverless-express#130) * apply Content-Length header when missing ([#175](CodeGenieApp/serverless-express#175)) ([c2f416b](CodeGenieApp/serverless-express@c2f416b))

only specify content-length in certain cases

1ef34b8

will now only set the content-length header if the request has a body and the header is not already set

update tests

9745be8

- fix failing test cases - add comment about motivation for change in index.js

HotelCalifornia force-pushed the patch-1 branch from 9c8541e to 9745be8 Compare June 21, 2018 15:36

brettstack requested changes Jun 21, 2018

View reviewed changes

forcibly inject Content-Length into event.headers

462bfdf

- only if body is present and there are no headers - seems hacky, but ensures that the `x-apigateway-event` header is set properly(?) later (i.e. including the content-length field)

remove extraneous whitespace

c86713d

brettstack approved these changes Jun 21, 2018

View reviewed changes

brettstack mentioned this pull request Jun 22, 2018

Fix: content-length was not matching the body after an array to string conversion #130

Closed

HotelCalifornia changed the base branch from master to develop June 25, 2018 15:06

brettstack closed this Jun 25, 2018

brettstack added a commit that referenced this pull request Jun 25, 2018

fix: apply Content-Length header when missing

d39a047

* Certain requests (e.g. GET and DELETE) would fail when they included a body due to missing content-length header. See #147, #106, #130

HotelCalifornia deleted the patch-1 branch July 11, 2018 19:27

brettstack added a commit that referenced this pull request Aug 16, 2018

fix: apply Content-Length header when missing

b0927b8

* Certain requests (e.g. GET and DELETE) would fail when they included a body due to missing content-length header. See #147, #106, #130

brettstack mentioned this pull request Aug 16, 2018

fix: apply Content-Length header when missing #175

Merged

snyk-bot mentioned this pull request Jan 24, 2020

[Snyk] Upgrade aws-serverless-express from 3.3.3 to 3.3.6 qsays/aws-serverless-express#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

set Content-Length header in mapApiGatewayEventToHttpRequest #147

set Content-Length header in mapApiGatewayEventToHttpRequest #147

HotelCalifornia commented May 30, 2018 •

edited

Loading

HotelCalifornia commented May 30, 2018

brettstack commented Jun 20, 2018

brettstack commented Jun 20, 2018

HotelCalifornia commented Jun 20, 2018

HotelCalifornia commented Jun 21, 2018 •

edited

Loading

brettstack commented Jun 21, 2018

brettstack Jun 21, 2018

brettstack Jun 21, 2018

HotelCalifornia commented Jun 21, 2018

HotelCalifornia commented Jun 21, 2018

brettstack commented Jun 21, 2018

brettstack commented Jun 22, 2018

HotelCalifornia commented Jun 25, 2018

brettstack commented Jun 25, 2018

set Content-Length header in mapApiGatewayEventToHttpRequest #147

set Content-Length header in mapApiGatewayEventToHttpRequest #147

Conversation

HotelCalifornia commented May 30, 2018 • edited Loading

HotelCalifornia commented May 30, 2018

brettstack commented Jun 20, 2018

brettstack commented Jun 20, 2018

HotelCalifornia commented Jun 20, 2018

HotelCalifornia commented Jun 21, 2018 • edited Loading

brettstack commented Jun 21, 2018

brettstack Jun 21, 2018

Choose a reason for hiding this comment

brettstack Jun 21, 2018

Choose a reason for hiding this comment

HotelCalifornia commented Jun 21, 2018

HotelCalifornia commented Jun 21, 2018

brettstack commented Jun 21, 2018

brettstack commented Jun 22, 2018

HotelCalifornia commented Jun 25, 2018

brettstack commented Jun 25, 2018

HotelCalifornia commented May 30, 2018 •

edited

Loading

HotelCalifornia commented Jun 21, 2018 •

edited

Loading