Allow express 'trust proxy' to be set

[edclement]

Related to Rate limiting when behind a proxy or load balancer discussion.

I was able to test this change and verify manually while running Verdaccio locally.

I did attempt to add some unit tests for this to test/unit/modules/api/api.spec.ts as well. I ran into issues in that regard as it doesn't appear the app.set('trust proxy', config.server.trustProxy); has any effect when supertest is being leveraged. I'm not sure why this is the case. I've provided an example unit test below. No matter what I tried, the following issues were encountered:

1. When providing an X-Forwarded-For header, the req.ip field would still resolve to 127.0.0.1 from the express handler. I verified this when a break point was placed at src/api/endpoint/api/ping.ts:7. This is not the expected behavior or the behavior exhibited when actually running Verdaccio.
2. The res.req.ip property being checked in the test would always resolve to undefined. It seems that value is unavailable in the unit test as it gets cleared out after the response is sent. Even if it was available, based on bullet point #1, it would not be the expected value.

Attempted unit tests that wouldn't work:

test/unit/modules/api/api.spec.ts

@@ -39,6 +39,7 @@ const putVersion = (app, name, publishMetadata) => {
 describe('endpoint unit test', () => {
   let app;
   const mockServerPort = 55549;
+  const trustProxy = '127.0.0.1';
   let mockRegistry;

   beforeAll(function (done) {
@@ -71,6 +72,7 @@ describe('endpoint unit test', () => {
             },
           },
           logs: [{ type: 'stdout', format: 'pretty', level: 'warn' }],
+          server: { trustProxy }
         },
         'api.spec.yaml'
       );
@@ -90,7 +92,41 @@ describe('endpoint unit test', () => {
     nock.cleanAll();
   });

-  describe('Registry API Endpoints', () => {
+  describe.only('Registry API Endpoints', () => {
+
+    test('should derive the client IP from req.ipwhen the X-Forwarded-For header is not present', (done) => {
+      request(app)
+        .get('/-/ping')
+        .connect("127.0.0.1")
+        .expect(HEADER_TYPE.CONTENT_TYPE, HEADERS.JSON_CHARSET)
+        .expect(HTTP_STATUS.OK)
+        .end(function (err, res) {
+          if (err) {
+            return done(err);
+          }
+          expect(res.req.ip).toEqual('127.0.0.1') // this fails as `res.req.ip` is undefined
+          done();
+        });
+    });
+
+    test('should derive the client IP from the X-Forwarded-For header when present', (done) => {
+      const clientIP = '9.8.7.6';
+      request(app)
+        .get('/-/ping')
+        .connect("127.0.0.1")
+        .set('X-Forwarded-For', `${clientIP}, ${trustProxy}`)
+        .expect(HEADER_TYPE.CONTENT_TYPE, HEADERS.JSON_CHARSET)
+        .expect(HTTP_STATUS.OK)
+        .end(function (err, res) {
+          if (err) {
+            return done(err);
+          }
+          expect(res.req.ip).toEqual(clientIP) // this fails as `res.req.ip` is undefined
+          done();
+        });
+    });
+
+

[juanpicado]

LGTM so far, I will check the unit test examples over the weekend.

[edclement]

LGTM so far, I will check the unit test examples over the weekend.

Thanks. Let me know what you find there.

I could not for the life of me understand why the unit tests didn't behave the same as the actual running Verdaccio instance. I can't think of a way to test this using integrations tests either since the change in functionality does not effect the response.

[juanpicado]

@edclement it's very complicated setup those tests, I think won't invest this time since #3100 has already refactor all test making wayyy more simpler and easy to run one single block and 5.x is really a pain, so I'll trust your implementaiton because is really simple and this PR very well documented ( 🙃 as always).