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

fix: Abort SSR rendering on redirect #438

Merged
merged 3 commits into from
Sep 23, 2023
Merged

Conversation

NielsJanssen
Copy link
Contributor

When the auth middleware is run, on the server using SSR, it redirects using the sendRedirect method from h3. This results in a redirect being sent along with the response, however the entire page is also executed on the server.

This fix prevents that by returning abortNavigation after the redirect is sent, ensuring the protected page will not render.

One downside I've noticed is that this may log a 404 error, due to false having been returned from the middleware, but it seems to be the only way to actually stop navigation. Feedback on this is appreciated.

Here is a small reproduction of the principle as a generic Nuxt page:

<template>
  <h1>test</h1>
</template>

<script setup lang="ts">
import {sendRedirect} from 'h3';
import {abortNavigation} from '#app';

definePageMeta({
  middleware: [
    function () {
      const nuxtApp = useNuxtApp();

      if (process.server) {
        if (nuxtApp.ssrContext?.event) {
          return nuxtApp.callHook('app:redirected').then(() => {
            // 1. Current implementation, comment next line to text new implemenation
            return sendRedirect(nuxtApp.ssrContext!.event, '/any-page', 302);

            // 2. New implementation
            sendRedirect(nuxtApp.ssrContext!.event, '/any-page', 302);
            return abortNavigation();
          });
        }
      }
    },
  ],
});

console.error('This should never be logged');
</script>

Checklist:

  • issue number linked above after pound (#)
    • replace "Closes " with "Contributes to" or other if this PR does not close the issue
  • manually checked my feature / checking not applicable
  • wrote tests / testing not applicable
  • attached screenshots / screenshot not applicable

@BracketJohn
Copy link
Contributor

Thanks so much for contributing! Will have a look at this soon (:

@BracketJohn
Copy link
Contributor

one thing you could already have a look at: ci is failing

@NielsJanssen
Copy link
Contributor Author

@BracketJohn Should be fixed now!

@zoey-kaiser
Copy link
Member

Hi @NielsJanssen

One downside I've noticed is that this may log a 404 error, due to false having been returned from the middleware, but it seems to be the only way to actually stop navigation. Feedback on this is appreciated.

Would it be possible to return a 403 code instead? This would also be more appropriate IMO

@zoey-kaiser zoey-kaiser self-requested a review July 26, 2023 14:25
@zoey-kaiser zoey-kaiser merged commit 6b9da1a into sidebase:main Sep 23, 2023
3 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.

3 participants