Converted small middlewares into plugins #854
Conversation
|
Size Change: 0 B Total Size: 681 kB ℹ️ View Unchanged
|
| if (matchedDomain) { | ||
| res.set('X-Frame-Options', `ALLOW-FROM ${referer}`); | ||
| } | ||
| done(); |
There was a problem hiding this comment.
why do we call done here instead of next as previously done?
There was a problem hiding this comment.
this is a fastify naming convention (see https://www.fastify.io/docs/latest/Reference/Plugins/), of course we can call it next if we want, but it's better to follow fastify's naming convention to keep everything more fastify-like and make it easier to read and associate.
you'll also see that i have renamed req -> request and res -> reply, same idea
There was a problem hiding this comment.
yea, I just thought that next and done where different in Fastify, good to know
There was a problem hiding this comment.
in this case, there isn't an equivalent in express since a plugin does more things than an middleware and a middleware gets executed on a request, but yeah they're similar
| delete request.headers.correlation_id; | ||
| request.headers.unique_id = 'thunderstorms'; | ||
| ensureCorrelationId(fastify, null, jest.fn()); | ||
| expect(request.headers).toHaveProperty('correlation-id', 'thunderstorms'); | ||
| }); | ||
|
|
||
| it('adds a correlation-id header to a request without anything', () => { |
There was a problem hiding this comment.
| it('adds a correlation-id header to a request without anything', () => { | |
| it('adds a unique correlation-id to request header if correlation-id is null', () => { |
There was a problem hiding this comment.
I propose adds a unique correlation-id to the request object since tests are assertions, things like without and if shouldn't be used. Also, these are the original test cases, maybe we should spend some time refreshing them 🤔
| const csp = (fastify, _opts, done) => { | ||
| fastify.decorateRequest('scriptNonce', null); | ||
|
|
||
| fastify.addHook('onRequest', async (request, reply) => { |
There was a problem hiding this comment.
Same as above. Is async intentional?
| * @param {import('fastify').FastifyPluginCallback} done plugin callback | ||
| */ | ||
| const forwardedHeaderParser = (fastify, _opts, done) => { | ||
| fastify.addHook('onRequest', async (request) => { |
|
|
||
| fastify.addHook('onRequest', async (request, reply) => { | ||
| reply.header('Strict-Transport-Security', 'max-age=15552000; includeSubDomains'); | ||
| reply.header('x-dns-prefetch-control', 'off'); |
There was a problem hiding this comment.
Why does this include so many headers that were not there previously?
There was a problem hiding this comment.
these are headers required by the integration tests that are not widely present. For the case of x-dns-prefetch-control, it is added by Helmet (see https://github.com/helmetjs/helmet/blob/a2549bd4688d9e711aea64a7fdffb07f4136f6cf/middlewares/x-dns-prefetch-control/index.ts#L17). Helmet, in the Fastify implementation, is only used on routes that will render Html, it isn't needed for all urls. By doing so, we increase performance. Right now, there's an integration test that expects this header in a route that doesn't render html (see
one-app/__tests__/integration/one-app.spec.js
Line 1365 in 8229994
if we feel that we do not actually need them for all routes them we can delete them, they were added to support backwards compatibility
| // eslint-disable-next-line unicorn/better-regex -- conflicts with unsafe-regex | ||
| .replace(/\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}/g, '0.0.0.0'); | ||
|
|
||
| describe('csp', () => { |
There was a problem hiding this comment.
Please restore the describe block
|
|
||
| describe('conditionallyAllowCors', () => { |
There was a problem hiding this comment.
Restore the describe block
| @@ -14,39 +14,100 @@ | |||
| * permissions and limitations under the License. | |||
| */ | |||
|
|
|||
| import addSecurityHeaders from '../../../src/server/middleware/addSecurityHeaders'; | |||
|
|
|||
| describe('addSecurityHeaders', () => { | |||
There was a problem hiding this comment.
Restore the describe block
| }); | ||
| }); | ||
|
|
||
| test('adds frame options header', () => { |
There was a problem hiding this comment.
Our convention is to use it over test and for all tests to be contained in a described block
| const res = { set: jest.fn((key, value) => value) }; | ||
| const next = jest.fn(); | ||
| addCacheHeaders(req, res, next); | ||
| test('adds cache headers', () => { |
There was a problem hiding this comment.
Use it and restore the describe block
giulianok
requested review from
bishnubista,
code-forger and
10xLaCroixDrinker
giulianok
deleted the
refactor/express-middlewares-to-fastify-plugins
branch
Description
Converted small ExpressJS middlewares into Fastify plugins. These are considered small since have relatively small code and they are (almost) completely abstract
Motivation and Context
By converting ExpressJS middlewares into Fastify plugins, we can migrate routes from Express to Fastify to improve app's performance
How Has This Been Tested?
Unit testing
Types of Changes
Checklist:
What is the Impact to Developers Using One App?