Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): unified router context #429

Merged
merged 22 commits into from
Apr 26, 2024
Merged

Conversation

vejja
Copy link
Collaborator

@vejja vejja commented Apr 20, 2024

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

This PR is a significant rewrite of the core engine of Nuxt Security, motivated primarily by the introduction of runtime hooks in PR #298 by @huang-julien and comments thereon by @harlan-zw.

Background

The background for this PR is further detailed in discussion #346 (Refactoring Headers) and in RFC #300 (Move out everything from readonly runtimeConfig).

Currently, Nuxt-Security accepts route-level security definitions via the routeRules option. Also, Nuxt-Security accepts runtime modification of security definitions via runtime hooks.

This has resulted in two parallel code paths, with sometimes different behaviours (e.g. in whether definitions are merged or overwritten). The historic code path used the native getRouteRules function, while the hooks code path used a custom event.context object.

This PR unifies these two code paths by moving all security definitions to the new event.context object introduced by the runtime hooks, making it the unique source of truth. Instead of using the getRouteRules function, we now use a resolveSecurityRules function everywhere.

Benefits

With this PR, we introduce several benefits to Nuxt-Security

  1. All security options can now be modified via runtime hooks
    It is now possible to modify any of the Nuxt Security options, and not solely the headers : any other option such as hidePoweredBy, rateLimiter, is now taken into consideration and applied at route level.

  2. Route rules are now consistently merged
    The router merging strategy is now the same irrespective of the way the security options are set (inline, global, routeRules, and runtime hooks). Previously, it was a mix of defu, defuReplaceArray, and plain overwriting - leading to confusion on how nested rules would apply (see CSP not working for specific routes? (Google Maps) #430 for instance). We now apply the defuReplaceArray strategy across the board.

  3. Clear scoping of security headers to HTML pages, SWR support
    We now make a clearer distinction between the scope of Nitro plugins (modifying HTML pages and their headers) and the scope of Server middlewares (functions that apply to all routes). This avoids to overwrite headers of non-HTML assets with irrelevant options, and as a result we are able to support SWR natively.

  4. Route-level support of RateLimiter
    Thanks to the ability to resolveSecurityRoutes at runtime, we are now able to support route-based definitions for the Rate Limiter. This solves the issue of getting 429 denials for routes where we want to have a higher rate limit. We also take this opportunity to solve the issue of getting 429s when pre-rendering.

New runtime hook

This PR introduces a new runtime hook : nuxt-security:routeRules, that allows to modify any security rule on any route. With this hook, the user is now able to apply any strategy for the rule (merge, overwrite, append, etc.).

The syntax for this new hook is simpler than the nuxt-security:headers syntax.

We can write

nitroApp.hooks.hook('nuxt-security:routeRules', async routeRules => {
  // any kind of modification of routeRules here, such as :
  routeRules['/my-route'] = ...
 })

Instead of

nitroApp.hooks.hook(`nuxt-security:ready, async() => {
 // define headers for one route
 // cannot modify other security options
 // modification is always by overwrite, no merging is possible
 nitroApp.hooks.callHook('nuxt-security:headers', {
   route: '/my-route'
   headers: ...
 }
})

The former nuxt-security:ready & nuxt-security:headers hooks are still supported but we are soft-depecrating them by removing them from the documentation.

Issues closed by this PR

Other notes

This PR also soft-deprecates the substitution merging via string syntax feature. This is now rendered unnecessary because the defuReplaceArray strategy is applied consistently everywhere.

We are removing corresponding mentions in the documentation, which were confusing (it only applied to headers, and it only applied in the router merging step but not in the definition step). The feature still exists to maintain backwards compatibility.

Please note that some security options can only be applied globally (removeLoggers, csrf and basicAuth) because they depend on third-party modules. The TypeScript definitions have been updated to remove these 3 options from the properties that can be set at route-level.

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes (if not applicable, please state why)

Added tests for

  • new runtime hook
  • rate limiter by route

vejja added 17 commits April 9, 2024 11:02
This now works by:
- fetching rules in context from useRuntimeConfig()
- ensuring event context is read at the right level by hooking in render:response rather than beforeResponse
- only HTML renderer writes security headers
- runtime hook is able to fetch dynamic values from API
make resolveSecurityRules synchronous
Copy link

vercel bot commented Apr 20, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nuxt-security ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 25, 2024 4:01pm

@vejja vejja self-assigned this Apr 20, 2024
@vejja vejja marked this pull request as ready for review April 21, 2024 22:59
@vejja vejja requested a review from Baroshem April 21, 2024 23:03
@vejja vejja added the enhancement New feature or request label Apr 21, 2024
@vejja
Copy link
Collaborator Author

vejja commented Apr 22, 2024

@Baroshem @huang-julien @harlan-zw happy to get your feedback on this

Copy link
Owner

@Baroshem Baroshem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few comments but overall this is a great piece of work you did there @vejja

I have tested it and it seems that it is working correctly. After you answer or fix the comments, I think we could merge it to main and test once again there and release it as a new version :)

src/defaultConfig.ts Outdated Show resolved Hide resolved
src/defaultConfig.ts Show resolved Hide resolved
src/types/index.ts Outdated Show resolved Hide resolved
@vejja
Copy link
Collaborator Author

vejja commented Apr 25, 2024

@Baroshem : do you want me to squash commits for clarity ?

@Baroshem
Copy link
Owner

Baroshem commented Apr 26, 2024

No need @vejja

We can keep them as history :)

I will merge this PR in a few hours do that we can tesg once again on main and release a new version 🚀

@Baroshem Baroshem merged commit 15142c0 into main Apr 26, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment