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

Chore/1.0.0 #317

Merged
merged 23 commits into from
Dec 13, 2023
Merged

Chore/1.0.0 #317

merged 23 commits into from
Dec 13, 2023

Conversation

Baroshem
Copy link
Owner

@Baroshem Baroshem commented Dec 7, 2023

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

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)

Copy link

vercel bot commented Dec 7, 2023

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 Dec 13, 2023 9:17am

feat(doc): extend FAQ with Prismic
@Baroshem
Copy link
Owner Author

Baroshem commented Dec 7, 2023

@vejja

Do you have any objections regarding releasing 1.0.0 tomorrow? If you give a 🟢 I think we could proceed with releasing and giving users a stable 1.0.0 to work with :)

@vejja
Copy link
Collaborator

vejja commented Dec 7, 2023

@Baroshem We have 2 reported regressions, let me fix these first as they were introduced by me on rc5

@vejja
Copy link
Collaborator

vejja commented Dec 7, 2023

It's ok now
PR #318 and PR #320 fix the regressions

@Baroshem
Copy link
Owner Author

Baroshem commented Dec 8, 2023

@vejja

Let me know if there is a 🟢 light from you :)

@vejja
Copy link
Collaborator

vejja commented Dec 8, 2023

@vejja

Let me know if there is a 🟢 light from you :)

@Baroshem
Sorry, let me get my head over the implications of #321 first.
It's an easy change and I want to be sure we don't break existing functionalities

@vejja
Copy link
Collaborator

vejja commented Dec 8, 2023

@vejja
Let me know if there is a 🟢 light from you :)

@Baroshem Sorry, let me get my head over the implications of #321 first. It's an easy change and I want to be sure we don't break existing functionalities

#321 fixed by #322

fix: csp false in rc5 removes custom csp header
@Baroshem
Copy link
Owner Author

Baroshem commented Dec 8, 2023

@vejja

I have tested all steps. Everything is fine from my side.

Do you think we should release a version today or should we wait until the next week? :)

@vejja
Copy link
Collaborator

vejja commented Dec 8, 2023

Hi @Baroshem
I am proposing an improved implementation in PR #323, to solve a difficult edge case that I discovered:

Suppose someone has defined headers using traditional Nuxt route rules syntax :

routeRules: {
  'some-route': {
    headers: {
      'Strict-Transport-Security': 'max-age=123456'
    }
  }
}

Presumably the user wants STS applied to all assets.

But the fix I implemented in PR #322 was deleting this rule from the nitro config object. This worked well for HTML pages but not for other assets.

I did not like the idea of modifying the route rules directly in the nitro config, so instead here I'm not deleting the rules anymore. I think it is cleaner, and it covers the edge case above.

To give you some background on why I am making this change:

  1. In rc.5 the logic was simple:
  • I was looking at the security object. If the STS header was set to false I removed it
  • If the asset was not HTML, the hook was not called anyways so the STS header was kept intact

However as evidenced in #321, the removal was too intrusive.

  1. With the fix in fix: csp false in rc5 removes custom csp header #322, I changed the logic:
  • I still look at the security object. If the STS header is set to false I do not remove it anymore. But if it had been kept in the native routeRules object, it would still apply. So I needed to delete it from the nitro config
  • Now if the asset is not HTML, the nitro config object has been modified so the STS header is not there anymore

It's a bit of an unlikely case, because it means that somebody has simultaneously defined the same header in 3 different places, with 3 different settings: in the native routeRules object (with string value), in the Nuxt-Security custom security property (with false value), and also in a server middleware (via defineEventHandler).

With #323 I'm reverting to the simpler logic of rc.5 which I think is cleaner because it is based on not touching the native routeRules in the nitro config. Instead, I'm making the check from within the Nitro plugin.

I think we can release now, let me know

@Baroshem
Copy link
Owner Author

Baroshem commented Dec 8, 2023

@vejja

Thanks for this additional research from you side and solving the edge case!

Let's merge this PR and test it next week. I think we can postpone the release to next week (for example wednesday) to see if there will be more bugfixes reported over the weekend.

improve implementation and add tests
eyopa21 and others added 2 commits December 11, 2023 00:42
Documentation typo change from route roules to route rules
@Baroshem
Copy link
Owner Author

@vejja

I have tested all my paths and this seems to be working well.

There is one small bug that I will open that we can fix for the 1.0.1 version which is about frame-ancestors in SSG does is ignored so we could programatically remove it from the meta tag. So it is some sort of a feature/bugfix.

Let me know if we have a 🟢 light from you and if so I will proceed with the release :)

@vejja
Copy link
Collaborator

vejja commented Dec 13, 2023

@vejja

I have tested all my paths and this seems to be working well.

There is one small bug that I will open that we can fix for the 1.0.1 version which is about frame-ancestors in SSG does is ignored so we could programatically remove it from the meta tag. So it is some sort of a feature/bugfix.

Let me know if we have a 🟢 light from you and if so I will proceed with the release :)

Yes, let’s release !!!
And more importantly : Congratulations 🎊🎈🍾🎉

@Baroshem Baroshem linked an issue Dec 13, 2023 that may be closed by this pull request
@Baroshem
Copy link
Owner Author

Huge kudos to you @vejja 🚀

You are doing an amazing work making this module more secure than ever! So happy to have you as a maintainer and core part of the module 💚

@Baroshem Baroshem merged commit a6b9c02 into main Dec 13, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

what config to make google auth works ? Releases section does not work
4 participants