silvermine / lambda-express Goto Github PK

We should see what features Express is shipping in 5.x and see if we need to / should support any of them, and what, if any, of the changes in 5.x are incompatible with the interfaces of Express that we've implemented.

Reference: expressjs/express#2237

Builds that previously passed (master branch) are now failing on the latest node version. We need to see if this is a problem with the tests, or an actual break in the code, and fix.

We need to figure out which types, classes, interfaces, etc, we export and what that interface looks like.

The current implementation for building a JSONP callback function uses the exact string which was passed to the API. This can create invalid JS if the API was requested with something like: https://example.com/endpoint?callback=%20not%20valid.

Looking at the implementation in express, all but \[\][a-zA-Z0-9_] will be filtered from the callback name. Should lambda-express be doing something similar?

For posterity, other express items that might be of use:

• expressjs/express@0ae18bc#commitcomment-465075
• https://github.com/expressjs/express/pull/502/files
• expressjs/express@df2584c
• expressjs/express@89c5aff

See

With the introduction of APIGW HTTP APIs, a new version of the payload format between APIGW and Lambda was also introduced - version 2.0 [1]. The serverless framework by default uses version 1.0 when an HTTP API is created. This preserves backward compatibility if switching over from using a REST API. The new format has changes to multi-value headers, the request context object and some others noted here [2]. We might want to consider supporting payload version 2.0. At this point, the need is not urgent as we can continue using v1.0 even with HTTP APIs.

[1] https://docs.aws.amazon.com/apigateway/latest/developerguide/http-api-develop-integrations-lambda.html
[2] https://medium.com/@lancers/amazon-api-gateway-explaining-lambda-payload-version-2-0-in-http-api-24b0b4db5d36

Probably something similar to https://github.com/jeremydaly/lambda-api#logging

The Request class in express has an accepts function for determining the preferred content type. This functionality along these lines is needed to address this TODO.

This issue is to track the planning and addition of Accept header parsing to the Request class.

Need to write usage documentation in the README. It should include:

• any differences from the API of Express (notably, mounting sub-routers)
• examples of middleware, request handlers, error handlers
• hello world

Add an easy way to automatically log requests, in the style of: https://github.com/jeremydaly/lambda-api#access-logs

Since our org typically prefers promises over callbacks, I was excited to see that 5.x versions of Express will support promises. Please evaluate what they support and whether we can support it while still supporting the callback variant as well.

References:

• expressjs/express#2259 (comment)
• https://github.com/pillarjs/router/tree/v2.0.0-alpha.1#middleware
• pillarjs/router@d2bce0c
• expressjs/express#2237

See

And

We should be building documentation from our JSDocs. Since this is our first public TypeScript project, we'll need to figure out the right method of exposing all the right information. For example: https://github.com/silvermine/lambda-express/blob/master/src/Response.ts#L243

See

lambda-express automatically sets the charset to utf-8 for JSON responses, but does not do so for text responses. Looking at behavior of send in express, the charset will be set for string responses (added in expressjs/express#f14e39d4).

This issue is to investigate making the charset behavior of lambda-express to be more inline with that of express.

See:

• Response.ts#L544-L549
• Response.ts#L515
• Response.ts#L527-L528

Express supports internal request re-routing by allowing route handlers to change the request's url property. Lambda-express should support the same to maintain compatibility.

We should consider adding support for sampling request logs, in the style of: https://github.com/jeremydaly/lambda-api#sampling

To fix expressjs/express#1132, express started escaping \u2028 and \u2029 as these were valid in JSON, but not JS (was changed in this proposal). Should lambda-express also perform a similar escaping?

See also:

• http://timelessrepo.com/json-isnt-a-javascript-subset
• https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON#JavaScript_and_JSON_differences

Microsoft's API Extractor tool can analyze the types and interfaces that are exported in your main type definition file and warn you about types and interfaces that are used in the public API but that are not exported.

Also, the tool outputs a reviewable file that makes it clear how each change affects the public API.

Since v1.0.0 (original commit), express will return the target URL in the body of a redirection response. Their current implementation is slightly different, but the concept still exists. This might be helpful functionality to add to lambda-express.

Regarding Request.hostname, the lambda-express code states:

When the trust proxy app setting is truthy, [the hostname] property will instead have the value of the X-Forwarded-Host header field.

Unfortunately, the hostname code doesn't seem to actually do that. Tests that are listed below fail due to this. Seems like either the docs or the code needs to change.

Unit Tests

describe('hostname property', () => {

   const testCases = [
      {
         host: 'b5gee6dacf.execute-api.us-east-1.amazonaws.com',
         expectedWithTrustProxy: 'b5gee6dacf.execute-api.us-east-1.amazonaws.com',
         expectedWithoutTrustProxy: 'b5gee6dacf.execute-api.us-east-1.amazonaws.com',
      },
      {
         host: 'b5gee6dacf.execute-api.us-east-1.amazonaws.com:443',
         expectedWithTrustProxy: 'b5gee6dacf.execute-api.us-east-1.amazonaws.com',
         expectedWithoutTrustProxy: 'b5gee6dacf.execute-api.us-east-1.amazonaws.com',
      },
      {
         host: 'b5gee6dacf.execute-api.us-east-1.amazonaws.com',
         xForwardedHost: 'api.example.com',
         expectedWithTrustProxy: 'api.example.com',
         expectedWithoutTrustProxy: 'b5gee6dacf.execute-api.us-east-1.amazonaws.com',
      },
      {
         host: 'b5gee6dacf.execute-api.us-east-1.amazonaws.com:443',
         xForwardedHost: 'api.example.com',
         expectedWithTrustProxy: 'api.example.com',
         expectedWithoutTrustProxy: 'b5gee6dacf.execute-api.us-east-1.amazonaws.com',
      },
      {
         host: 'b5gee6dacf.execute-api.us-east-1.amazonaws.com',
         xForwardedHost: 'api.example.com:433',
         expectedWithTrustProxy: 'api.example.com',
         expectedWithoutTrustProxy: 'b5gee6dacf.execute-api.us-east-1.amazonaws.com',
      },
      {
         host: 'b5gee6dacf.execute-api.us-east-1.amazonaws.com:443',
         xForwardedHost: 'api.example.com:443',
         expectedWithTrustProxy: 'api.example.com',
         expectedWithoutTrustProxy: 'b5gee6dacf.execute-api.us-east-1.amazonaws.com',
      },
   ];

   it('parses proper values - APIGW', () => {
      _.each(testCases, (testCase) => {
         let evt: RequestEvent = apiGatewayRequest(),
               req;

         evt.headers.Host = testCase.host;

         if (testCase.xForwardedHost) {
            evt.headers['X-Forwarded-Host'] = testCase.xForwardedHost;
            evt.multiValueHeaders['X-Forwarded-Host'] = [ testCase.xForwardedHost ];
         } else {
            delete evt.headers['X-Forwarded-Host'];
            delete evt.multiValueHeaders['X-Forwarded-Host'];
         }

         app.enable('trust proxy');
         req = new Request(app, evt, handlerContext());
         expect(req.hostname).to.eql(testCase.expectedWithTrustProxy);

         app.disable('trust proxy');
         req = new Request(app, evt, handlerContext());
         expect(req.hostname).to.eql(testCase.expectedWithoutTrustProxy);
      });
   });

   it('parses proper values - ALB', () => {
      _.each(testCases, (testCase) => {
         let req;

         _.each([ albRequest(), albMultiValHeadersRequest() ], (evt) => {
            if (evt.headers) {
               evt.headers.host = testCase.host;
               if (testCase.xForwardedHost) {
                  evt.headers['X-Forwarded-Host'] = testCase.xForwardedHost;
               } else {
                  delete evt.headers['X-Forwarded-Host'];
               }
            }
            if (evt.multiValueHeaders) {
               evt.multiValueHeaders.host = [ testCase.host ];
               if (testCase.xForwardedHost) {
                  evt.multiValueHeaders['X-Forwarded-Host'] = [ testCase.xForwardedHost ];
               } else {
                  delete evt.multiValueHeaders['X-Forwarded-Host'];
               }
            }

            app.enable('trust proxy');
            req = new Request(app, evt, handlerContext());
            expect(req.hostname).to.eql(testCase.expectedWithTrustProxy);

            app.disable('trust proxy');
            req = new Request(app, evt, handlerContext());
            expect(req.hostname).to.eql(testCase.expectedWithoutTrustProxy);
         });
      });
   });

});

When using Application's run() function, the entire RequestEvent, HandlerContext, and response are logged. While logging that information can be helpful at times, this seems like debug code. Should the console.log be removed or at least be allowed to be disabled?

With lambda-express in early development, features and fixes are getting added and need to be tested faster than new versions are being cut. As such, it would be handy to be able to
install lambda-express directly from the git repo. However, due to the build process and needing the dist files, this isn't currently possible using npm i <repo>. Fortunately, a prepare script was added in npm 5. This script is ran when installing from a git repo and can be used to build the needed dist files.

More info: https://blog.jim-nielsen.com/2018/installing-and-building-an-npm-package-from-github/#installing-and-building-packages-with-npm-from-github

As part of preventing Rosetta Flash, express sets the X-Content-Type-Options header to nosniff. lambda-express already has the /**/ mitigation, however should nosniff also be added?

See also: https://helmetjs.github.io/docs/dont-sniff-mimetype/

For all GET routes, express will add support for HEAD requests automatically if a HEAD route is not already defined. lambda-express does not do this. At this time you must manually add the HEAD route.

Express docs: https://expressjs.com/en/api.html#router.METHOD

The router.get() function is automatically called for the HTTP HEAD method in addition to the GET method if router.head() was not called for the path before router.get().

Express sample:

$ cat index.js
const express = require('express')
const app = express()
const port = 3000

app.get('/', (req, res) => res.send('Hello World!'))

app.listen(port, () => console.log(`Example app listening at http://localhost:${port}`))

$ curl --head http://localhost:3000
HTTP/1.1 200 OK
X-Powered-By: Express
Content-Type: text/html; charset=utf-8
Content-Length: 12
ETag: W/"c-Lve95gjOVATpfV8EL5X4nxwjKHE"
Date: Sat, 11 Jul 2020 01:40:25 GMT
Connection: keep-alive