fix(core): spread storage options

Baroshem / nuxt-security

🛡 Automatically configure your app to follow OWASP security patterns and principles by using HTTP Headers and Middleware

https://nuxt-security.vercel.app/

MIT License

737 stars 56 forks source link

fix(core): spread storage options #452

Closed vejja closed 1 month ago

vejja commented 1 month ago

Closes #433

Types of changes

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

Description

The Rate Limiter options need to be spread in order to support all the unstorage drivers.

Previously, only the options property could be provided to the underlying driver, because this driver has only one property with the name options.

With this PR, any property name can be provided under the options property.

Checklist:

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

vercel[bot] commented 1 month ago

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	May 21, 2024 10:39am

vejja commented 1 month ago

I'm not entirely sure it's a breaking change, it could actually be a bug fix. It's very difficult to understand how nitro sets the unstorage options, and then how unstorage forwards these options to the underlying drivers. I'll do some local tests and will let you know.

Baroshem commented 1 month ago

I think it will be a breaking change purely because the interface for how certain feature will change. Wwith this change, users that uset the previous version of storage options, would have it not working anymore.

But no worries about that. This is not a huge breaking change as long as we provide release notes and migration guide.

vejja commented 1 month ago

I think it will be a breaking change purely because the interface for how certain feature will change. Wwith this change, users that uset the previous version of storage options, would have it not working anymore.

Yes that's right

vejja commented 1 month ago

@baroshem : Update after further review The bug @numselli found out for the redis driver was also a problem for all other drivers. The options were actually never passed to the underlying driver. I'm fixing the TypeScript definitions to clarify how the options must be passed and what values are available for each driver.

Marking as ready now

Baroshem commented 1 month ago

Thanks for finding it and fixing it @vejja 💚!

I can't wait for this release. it will be 🔥