feat(gatsby-plugin-fastify): add customizable headers with sensible defaults for security #392

tsdexter · 2023-03-08T14:51:19Z

Description

@moonmeister I think I've got this working as expected now. Please take a look and let me know.

options.headers plugin configuration added for applying custom headers to specific files/paths
default security headers added

Documentation

Headers section at the bottom of the README

Related Issues

closes #98
fixes #59

@moonmeister test Gatsby Node API › Should Build Config is failing - I'm not sure why. Can you take a look?

- add initial work from original POC in #98\n- slightly modifications from original POC for missing vars/typos/etc

changeset-bot · 2023-03-08T14:51:22Z

🦋 Changeset detected

Latest commit: 266d421

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
gatsby-plugin-fastify	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

tsdexter · 2023-03-08T15:10:09Z

@moonmeister what's the best way to test this out in a real site pre-release? For example, can we do an experimental release?

moonmeister

Good start! Thanks for all your work on this. I'm sure a lot of my comments are against the code you copied and pasted from me, I'm aware. None of this is a comment on you, just me wanting to get us to a good point.

So the major question in the back of my head is if this can/should be implemented as a plugin using hooks or Decorators. I myself have been refactoring features as I go to use these features. It helps segment code logic into a single file so I don't have to reimplement things across ssr/static, etc.

Looks like your tests have focussed on static/ssr/dsg. Have you tested client-only routes or thought about how headers interact with client-only routes?

I'm sure I've forgotten some things but hopefully this gets you started?

packages/gatsby-plugin-fastify/package.json

.changeset/neat-pans-serve.md

packages/gatsby-plugin-fastify/src/gatsby-node.ts

packages/gatsby-plugin-fastify/src/plugins/static.ts

packages/gatsby-plugin-fastify/src/utils/constants.ts

packages/gatsby-plugin-fastify/src/utils/headers.ts

packages/gatsby-plugin-fastify/src/plugins/server-routes.ts

moonmeister · 2023-03-08T20:27:59Z

@moonmeister what's the best way to test this out in a real site pre-release? For example, can we do an experimental release?

Great question, It is possible, but I've never done it with Changesets, I'll try to make time to do some reading...though if you want to look into the docs and see what's possible that'd be helpful.

moonmeister · 2023-03-08T22:08:24Z

@moonmeister what's the best way to test this out in a real site pre-release? For example, can we do an experimental release?

Great question, It is possible, but I've never done it with Changesets, I'll try to make time to do some reading...though if you want to look into the docs and see what's possible that'd be helpful.

I think I'll need to manually do a pre-release. LEt's get this a little further along then I can do that.

tsdexter · 2023-03-09T14:35:25Z

Thanks @moonmeister - getting back to it today.

FYI:

@moonmeister test Gatsby Node API › Should Build Config is failing - I'm not sure why. Can you take a look?

I think I figured out why this is failing, looking at a fix. Having to do with trying to read the webpack.stats.json from the __tests__ files which is now used in onPostBuild but wasn't previously

moonmeister · 2023-03-09T18:00:12Z

Thanks @moonmeister - getting back to it today.

FYI:

@moonmeister test Gatsby Node API › Should Build Config is failing - I'm not sure why. Can you take a look?

I think I figured out why this is failing, looking at a fix. Having to do with trying to read the webpack.stats.json from the __tests__ files which is now used in onPostBuild but wasn't previously

Yeah. Maybe need to mock this.

- refactor into headers plugin that handles add all necessary headers across all paths/modules - move headers related tests into new headers test suite - add additional tests for rev proxy, functions, client routes - add test coverage for `header-builder.ts` - rewrite documentation Resolves: - resolves #392 - resolves #392 - resolves #392 - resolves #392 - resolves #392 - resolves #392 - resolves #392 - resolves #392 - resolves #392

tsdexter · 2023-03-17T21:01:00Z

@moonmeister ok, I think it's in a much better place now! Let me know what we need to do next.

Benchmarks

I ran the benchmarks and they most definitely called for a refactor.

On the runtime version, the benchmark was pretty much always the lowest and by a lot in some cases, even when it was middle and top those results were essentially the same across all three.
On the buildtime version, in some of the most used scenarios (ie: static) it actually outperformed main by a decent margin. On the two that it lost, the three were essentially equals, and on the few that it was middle it wasn't too far off main.

_{Benchmarks for main, feat/default-custom-headers (pre-refactor), feat/default-custom-headers (post-refactor)}

Refactor

I refactored it to add all of the actual page paths to the headers object in the config during onPostBuild and then just look them up in the headers fastify plugin.

refactor header-builder.ts
- in onPostBuild I send all clientSideRoutes, serverSideRoutes, redirects, proxies, functions, and pages to buildHeadersProgram where it loops through them, along with all static files in /public, and some generated paths such as page-data and without trailing slashes for SSR/DSG and adds their headers to the headers object with their serverUrl as a key
refactor runtime headers.ts fastify plugin
- perform a simple look up of headers from the config json and add them
- I've moved default security headers to here otherwise it would write the same 6 lines to every path entry in the JSON config file, this saves thousands/tens of thousands of lines to limit file size/memory consumption
- routes with server handlers that can set their own headers are written last to ensure they take precedence
add tests to support your feature table (see below)

@moonmeister says:

This generally applies to all route paterns /example/[post_slug].ts. The major exception being if the header applies to /example/test/. We don't know that that exists but we will assume it does and save the header in the lookup table. That may take some special logic. lmk if you have any questions.

For this, I added reply.mode and reply.path decorators which are added in the modules that support dynamic paths. In the headers plugin we check if the route is handled by one of these dynamic modules and if it is we look up the headers with the the dynamic path that was matched rather than the actual request.url

Lastly, while refactoring/testing, I came across a bug in the reverse proxy implementation which I fixed in 0883b8f

Tests

I have written test cases to check all configurations in the feature table. I've updated/flipped some of the values you initially configured either because that's how Gatsby Cloud supports it or I think it's how we should support it - I've added explanations for them all in the footnotes. I also added Client-only Routes in the last column.

	SSR HTML ¹	SSR JSON ²	DSG HTML ³	DSG JSON ⁴	Static HTML ⁵	Static JSON ⁶	App Data ⁷	sw.js ⁸	Function Route ⁹	Generated Static Assets (Images, CSS, JS) /w Hash ¹⁰	Generated Static Assets (Images, CSS, JS) /wo Hash ¹¹	Static Folder Assets ¹²	Gatsby Image CDN ¹³	Client Only Routes ¹⁴
getServerData headers	Y	Y	N	N	N	N	N	N	N	N	N	N	N	N
Gatsby Functions setHeaders	N	N	N	N	N	N	N	N	Y	N	N	N	N	N
Default Immutable Cache Headers	N	N	N	N	N	N	N	N	N	Y	N	N	?? N ¹⁵	N
Default Revalidate Cache Headers	Y	Y	Y	Y	Y	Y	Y	Y	N Y ¹⁶	N	Y	Y	?? N ¹⁵	Y
Default Security Headers ¹⁶ ¹⁷	Y ?	Y ?	Y ?	Y ?	Y ?	Y ?	Y ?	Y ?	N Y ¹⁷	Y ?	Y ?	Y ?	Y ?	Y
gatsby-plugin-fastify Custom Headers	Y	Y	Y	Y	Y	Y	Y	Y	N Y ¹⁸	?? Y ¹⁸	?? Y ¹⁸	Y	?? ¹⁸	Y

See test case: it('should support SSR HTML headers as gatsby cloud does') ↩
See test case: it('should support SSR JSON headers as gatsby cloud does') ↩
See test case: it('should support DSG HTML headers as gatsby cloud does') ↩
See test case: it('should support DSG JSON headers as gatsby cloud does') ↩
See test case: it('should support Static HTML (SSG) headers as gatsby cloud does') ↩
See test case: it('should support Static JSON (SSG) headers as gatsby cloud does') ↩
See test case: it('should support app-data.json headers as gatsby cloud does') ↩
See test case: it('should support sw.js headers as gatsby cloud does') ↩
See test case: it('should support function headers as gatsby cloud does') ↩
See test case: it('should support hashed static asset headers as gatsby cloud does') ↩
See test case: it('should support unchunked root asset headers as gatsby cloud does') ↩
See test case: it('should support non-hashed static folder asset headers copied as is from /static to /public as gatsby cloud does') ↩
See test case: it('should support gatsby image CDN headers as gatsby cloud does') ↩
See test case: it('should support client-only route headers as gatsby cloud does') ↩
cache-control header on gatsby cloud is unset, using the same ↩ ↩²
Matching support to Gatsby Cloud hosting ↩ ↩²
I think default security should apply to all routes if configured, they can still be overwritten via customHeaders ↩ ↩²
I think customHeaders should apply to all modules routes as long as modules that can set their own headers (functions, SSR, Proxy) have those headers take precedence, this allows the user to define "global" custom headers such a /** that apply to all routes without also having to make sure they add that header to every handler they write while still being able to overwrite/unset it in specific handlers if required ↩ ↩² ↩³ ↩⁴

tsdexter · 2023-03-17T21:05:24Z

@moonmeister the test site with the updated version should build on gatsby cloud from the latest commit and I'll also get the plugin-fastify test site built and pushed to WPE Atlas platform running fastify on Monday so we can compare the two.

- refactor `header-builder.ts` to loop through all possible pages/files and add their matching headers during `onPostBuild` - refactor `headers.ts` fastify plugin to look up headers for the path and add them, alongside default security, respecting overwrite precedence - add `reply.mode` and `reply.path` decorators to support adding headers to dynamic routes via the matched path rather than `request.url` - add tests to support feature table in #392, specifically: #392 (comment)

tsdexter · 2023-03-17T21:13:00Z

TODO: update docs to match refactor

- refactor `header-builder.ts` to loop through all possible pages/files and add their matching headers during `onPostBuild` - refactor `headers.ts` fastify plugin to look up headers for the path and add them, alongside default security, respecting overwrite precedence - add `reply.mode` and `reply.path` decorators to support adding headers to dynamic routes via the matched path rather than `request.url` - add tests to support feature table in #392, specifically: #392 (comment)

tsdexter · 2023-03-20T18:12:39Z

@moonmeister didn't get much time to work on this today...thoughts on #392 (comment) before I get back to it tomorrow?

moonmeister · 2023-03-20T18:31:08Z

@moonmeister didn't get much time to work on this today...thoughts on #392 (comment) before I get back to it tomorrow?

Hey sorry, been a busy weekend and haven't had much time. I read it briefly. Haven't been able to look at code. I should have time tonight.

moonmeister · 2023-03-20T18:31:30Z

Definitely seems like we're on the right track.

moonmeister · 2023-03-21T03:43:48Z

RE: Benchmarks - Glad we did some testing to validate this is the correct path forward.

RE: Table - Thanks for validating and giving feedback. I agree with your assessments. Only one question, re: Functions and Security headers, What does Gatsby Cloud do?

RE: Refactor - Thanks, I'll do a code review and provide feedback there

moonmeister

I don't think I have the literal bandwidth to make a release tonight...I'll try in the next couple of days when I have time and a decent cell conection.

moonmeister · 2023-03-21T03:48:13Z

integration-tests/plugin-fastify/benchmark.js

+} = require("gatsby-plugin-fastify/dist/utils/config");
+const { serveGatsby } = require("gatsby-plugin-fastify/dist/plugins/gatsby");


Why is this change needed?

I was getting gatsby-plugin-fastify module not found (iirc on the exact error)...I tried a few things and only this worked. I'll try again while I'm making changes and put in the exact error, maybe you have another fix.

weird...it kinda makes sense...but why has it been working is just as strange.

ya know what...this isn't automated...it probably hasn't been run since I made this change.

moonmeister · 2023-03-21T03:51:00Z

integration-tests/plugin-fastify/package.json

@@ -29,6 +30,7 @@
    "gatsby-source-faker": "^5.7.0",
    "gatsby-source-filesystem": "^5.7.0",
    "gatsby-transformer-sharp": "^5.7.0",
+    "pino-pretty": "^10.0.0",


Not seeing this used anywhere... also http-status-codes is being used now but it hasn't been added to deps.

packages/gatsby-plugin-fastify/README.md

moonmeister · 2023-03-21T03:59:47Z

packages/gatsby-plugin-fastify/src/__tests__/plugins/reverse-proxy.js

I think you open a PR for this, let's just get it out of this one now.

packages/gatsby-plugin-fastify/src/plugins/decorators.ts

moonmeister · 2023-03-21T04:15:05Z

packages/gatsby-plugin-fastify/src/plugins/functions.ts

@@ -49,6 +49,8 @@ export const handleFunctions: FastifyPluginAsync<{
            handler: async function (request, reply) {
              try {
                reply.appendModuleHeader("Functions");
+                reply.mode = "FUNCTION";
+                reply.path = `${FUNCTIONS_PREFIX}${path}`;


Should we stay consistent with our use of path.join, or the POSIX version, which ever is correct?

we can switch it to POSIX - just path.join does \\api\\test (on Windows).

moonmeister · 2023-03-21T04:18:02Z

packages/gatsby-plugin-fastify/src/plugins/headers.ts

+    // add configured headers
+    reply.headers(pageHeaders);
+
+    // check pageHeaders for "undefined" values and remove those headers


I agree we shouldn't be sending undefined headers...this seems like a type checking issue somewhere else and should not need to be cleaned at runtime.

This isn't a type checking issue.

Perhaps you can add some input on a better way to handle this but in order to support being able to remove headers I made it so if you pass the string value "undefined" to the config object (or any of the default constants) it will properly remove the header with reply.removeHeader.

I originally needed this functionality to match gatsby hosting as for some routes they do not return a cache-control header at all, whereas Fastify was defaulting to public, max-age: 0 if we didn't explicitly set or remove it. Afterwards, I realized it could be helpful for customHeaders as well to "unset" headers on specific paths and documented it in the README.

Ah...okay. hmmm. I don't love the "undefined" but maybe there is no other good option. what about just using js undefined? or false? or "unset"?

Also, I think we can do this in the onPostBuild...The only exception would be default security. but that could be cleaned up to be faster probably. Thoughts?

Ah...okay. hmmm. I don't love the "undefined" but maybe there is no other good option. what about just using js undefined? or false? or "unset"?

IIRC, setting the object like { "cache-control": undefined } with the js undefined makes the object not have the cache-control property (it is undefined) so we can't check for it and remove the headers. I went back and forth with "undefined" and "unset" and ended on using undefined because the end result is that the header is not defined (vs the default or previously matched path headers). I think unset works too, it's just a more specific/made up term for our use-case. If you want to change it, I would lean back toward unset.

Also, I think we can do this in the onPostBuild...The only exception would be default security. but that could be cleaned up to be faster probably. Thoughts?

onPostBuild doesn't actually set or remove the headers, just puts the entry into the headers object, we could check previously configured defaults/customs to remove them here instead of adding the unset/undefined value - but we still have to check and removeHeader at runtime to cover cases where it's getting set by fastify outside of our own code, such as the cache-control default or other headers the user may want to remove. These lines are only looking at the specific headers object for the matched path, which is only 5-10 entries, generally, so I don't think it will make a performance difference to do some in onPostBuild if we still have to check/remove at runtime anyway. Thoughts?

So, I'm guessing that @fastify/static is adding this header, see: https://github.com/fastify/fastify-static#send-options. If we disable that then we should not have to deal with any headers set by Fastify. There are a couple of others like Etag and LastModified but I don't see any reason these would need to be overridden. If someone brings one to light, we can address the issue then. This means the only possible things we'd need to undefine would be things the user is setting. or gatsby defaults...which is kinda why we have those as selectable options. Unless you have an immediate use-case for this I think we should shelve this as a feature. Wait till there's a clear need.

moonmeister · 2023-03-21T04:29:30Z

packages/gatsby-plugin-fastify/src/plugins/headers.ts

+  useDefaultSecurity: boolean;
+}> = fp(async (fastify, { headers, useDefaultSecurity }) => {
+  fastify.addHook("onSend", async function (request, reply: FastifyReply) {
+    let pageHeaders = {};


Instead of mutating this, I think Object.assign can clean this up.

packages/gatsby-plugin-fastify/src/plugins/server-routes.ts

Co-authored-by: Alex Moon <[email protected]>

packages/gatsby-plugin-fastify/src/plugins/headers.ts

moonmeister · 2023-03-21T17:54:00Z

packages/gatsby-plugin-fastify/src/plugins/headers.ts

+export const handleHeaders: FastifyPluginAsync<{
+  headers: Headers;
+  useDefaultSecurity: boolean;
+}> = fp(async (fastify, { headers, useDefaultSecurity }) => {


Also thought about logging...not a rush but before we merge we'll want a couple of debug logs noting the use of default security headers, default caching headers, and custom headers. custom headers can probably just say hey there are custom headers or aren't. see https://github.com/gatsby-uc/plugins/blob/main/packages/gatsby-plugin-fastify/src/plugins/client-routes.ts#L16 for an example.

moonmeister · 2023-04-04T12:25:38Z

@tsdexter You're not waiting on me are you?

tsdexter · 2023-04-10T15:47:13Z

@moonmeister haven't forgot about this, just been having some other production issues and deadlines. Will hopefully get back to it later this week or next.

tsdexter · 2023-04-10T15:48:13Z

@tsdexter You're not waiting on me are you?

Nope, no worries. Just had to get some other stuff done in the meantime. See above.

moonmeister · 2023-06-14T13:59:56Z

Gatsby officially has an Adapter spec on the way 🥳 . It specifically notes the inclusion of headers in the spec to eliminate our very need to do this crazy implementation per adapter. 🥳

gatsbyjs/gatsby#38231

tsdexter · 2023-06-15T18:59:31Z

Gatsby officially has an Adapter spec on the way 🥳 . It specifically notes the inclusion of headers in the spec to eliminate our very need to do this crazy implementation per adapter. 🥳

gatsbyjs/gatsby#38231

And I had it on the schedule to finally get back to this after my vacation in July. I wonder how long until it's stable - looks great so far, been playing around with the demo repo.

moonmeister · 2023-06-16T02:11:17Z

Gatsby officially has an Adapter spec on the way 🥳 . It specifically notes the inclusion of headers in the spec to eliminate our very need to do this crazy implementation per adapter. 🥳

gatsbyjs/gatsby#38231

And I had it on the schedule to finally get back to this after my vacation in July. I wonder how long until it's stable - looks great so far, been playing around with the demo repo.

Yeah but also this is going to make life so easy. We can close every issue ever and it'll just work...hopefully.

moonmeister · 2023-06-30T13:07:24Z

With the announcement of the Gatsby Adapters spec, we'll be discontinuing any work on this plugin. Should the need for a performant node server still be needed, this work can continue under the official spec as gatsby-adapter-fastify.

tsdexter · 2023-06-30T19:32:01Z

@moonmeister this makes sense. Just one comment. I think there is still a bit of uncertainty around adapters, but to me it seems they are intended to be more platform specific.

I think there is probably still a path for something generic like gatsby-adapter-fastify or gatsby-adapter-express etc but from the initial post in the RFC I think it may be intended to be less generic and more specific to the platform.

For example, gatsby-adapter-atlas or gatsby-adapter-google-cloud either of which may still use fastify or express under the hood, but also takes care of the specific requirements for deployment on their respective platforms.

If my thinking is correct, perhaps the more generic gatsby-plugin-fastify still exists but changes to accept the output an adapter receives and then can be used within the platform-specific adapters.

Thoughts?

moonmeister · 2023-06-30T21:30:00Z

@tsdexter I just opened #440 , let's have this discussion there...thanks for the feedback, I'll respond there once you repost this. Just want to keep us organized. Thanks

tsdexter added 9 commits March 8, 2023 08:19

add initial work from PR #98

74b9009

- add initial work from original POC in #98\n- slightly modifications from original POC for missing vars/typos/etc

work on headers for static files

f82a85f

add custom headers via options config

8053ccf

fix static test

f50252f

Update yarn.lock

21b785f

add changeset

4797817

add headers documentation

4099220

undo husky change

41ceb84

add SSR specifics to documentation

2e99ba7

tsdexter requested a review from moonmeister as a code owner March 8, 2023 14:51

Update .all-contributorsrc

3e84682

tsdexter requested a review from a team as a code owner March 8, 2023 18:22

manually run lint-staged scripts

233b2ae

tsdexter force-pushed the feat/fastify/default-custom-headers branch from d246da3 to 233b2ae Compare March 8, 2023 18:25

fix linting issues

dfe706f

tsdexter force-pushed the feat/fastify/default-custom-headers branch from aa5be3b to 5643eba Compare March 8, 2023 18:52

fix validate types issues

568e8ad

tsdexter force-pushed the feat/fastify/default-custom-headers branch from 5643eba to 568e8ad Compare March 8, 2023 19:03

moonmeister requested changes Mar 8, 2023

View reviewed changes

tsdexter force-pushed the feat/fastify/default-custom-headers branch from 734259a to ce7d202 Compare March 13, 2023 22:44

tsdexter force-pushed the feat/fastify/default-custom-headers branch from ce7d202 to 70e5443 Compare March 13, 2023 22:53

tsdexter force-pushed the feat/fastify/default-custom-headers branch from ab6ff63 to 3be7da1 Compare March 17, 2023 21:07

tsdexter force-pushed the feat/fastify/default-custom-headers branch from 3be7da1 to 35bafd2 Compare March 17, 2023 21:18

tsdexter force-pushed the feat/fastify/default-custom-headers branch from 35bafd2 to 56511ae Compare March 17, 2023 21:27

tsdexter force-pushed the feat/fastify/default-custom-headers branch from 56511ae to 897e7d5 Compare March 17, 2023 21:29

moonmeister requested changes Mar 21, 2023

View reviewed changes

tsdexter and others added 2 commits March 21, 2023 13:10

Update packages/gatsby-plugin-fastify/README.md

9bb9be1

Co-authored-by: Alex Moon <[email protected]>

Update packages/gatsby-plugin-fastify/README.md

266d421

Co-authored-by: Alex Moon <[email protected]>

moonmeister reviewed Mar 21, 2023

View reviewed changes

packages/gatsby-plugin-fastify/src/plugins/headers.ts Show resolved Hide resolved

moonmeister reviewed Mar 21, 2023

View reviewed changes

moonmeister closed this Jun 30, 2023

		} = require("gatsby-plugin-fastify/dist/utils/config");
		const { serveGatsby } = require("gatsby-plugin-fastify/dist/plugins/gatsby");

feat(gatsby-plugin-fastify): add customizable headers with sensible defaults for security #392

feat(gatsby-plugin-fastify): add customizable headers with sensible defaults for security #392

Conversation

tsdexter commented Mar 8, 2023

Description

Documentation

Related Issues

changeset-bot bot commented Mar 8, 2023 • edited Loading

🦋 Changeset detected

tsdexter commented Mar 8, 2023

moonmeister left a comment

Choose a reason for hiding this comment

moonmeister commented Mar 8, 2023

moonmeister commented Mar 8, 2023

tsdexter commented Mar 9, 2023

moonmeister commented Mar 9, 2023

tsdexter commented Mar 17, 2023 • edited Loading

Benchmarks

Refactor

Tests

Footnotes

tsdexter commented Mar 17, 2023

tsdexter commented Mar 17, 2023

tsdexter commented Mar 20, 2023

moonmeister commented Mar 20, 2023

moonmeister commented Mar 20, 2023

moonmeister commented Mar 21, 2023

moonmeister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moonmeister commented Apr 4, 2023

tsdexter commented Apr 10, 2023

tsdexter commented Apr 10, 2023

moonmeister commented Jun 14, 2023

tsdexter commented Jun 15, 2023

moonmeister commented Jun 16, 2023

moonmeister commented Jun 30, 2023

tsdexter commented Jun 30, 2023 • edited Loading

moonmeister commented Jun 30, 2023 • edited Loading

changeset-bot bot commented Mar 8, 2023 •

edited

Loading

tsdexter commented Mar 17, 2023 •

edited

Loading

tsdexter commented Jun 30, 2023 •

edited

Loading

moonmeister commented Jun 30, 2023 •

edited

Loading