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

Form action type is too restrictive? #6822

Closed
binajmen opened this issue Sep 15, 2022 · 1 comment · Fixed by #6869
Closed

Form action type is too restrictive? #6822

binajmen opened this issue Sep 15, 2022 · 1 comment · Fixed by #6869
Labels

Comments

@binajmen
Copy link

I have the following login page:

// +page.svelte

<script lang="ts">
  import { enhance } from '$app/forms';
  import type { ActionData } from './$types';

  export let form: ActionData;
</script>

<div class="sm:mx-auto sm:w-full sm:max-w-md">
  <h2 class="mt-6 text-center text-3xl font-bold tracking-tight text-gray-900">
    Sign in to your account
  </h2>
</div>

<div class="mt-8 sm:mx-auto sm:w-full sm:max-w-md">
  <div class="bg-white py-8 px-4 shadow sm:rounded-lg sm:px-10">
    <form method="POST" class="space-y-6" use:enhance>
      {#if form?.invalid}
        <p class="error">Username and password is required.</p>
      {/if}

      {#if form?.credentials}
        <p class="error">You have entered the wrong credentials.</p>
      {/if}

      ...
    </form>
  </div>
</div>

With the attached form action:

// +page.server.ts

import { SESSION_SECRET } from '$env/static/private';
import prisma from '$lib/prisma';
import type { User } from '$lib/session';
import { invalid, redirect } from '@sveltejs/kit';
import bcrypt from 'bcryptjs';
import jwt from 'jsonwebtoken';
import type { Actions, PageServerLoad } from './$types';

export const load: PageServerLoad = async ({ locals }) => {
  if (locals.user) {
    throw redirect(302, '/');
  }
};

export const actions: Actions = {
  default: async ({ request, cookies }) => {
    const data = await request.formData();

    const email = data.get('email');
    const password = data.get('password');

    if (
      typeof email !== 'string' ||
      typeof password !== 'string' ||
      !email ||
      !password
    ) {
      return invalid(400, { invalid: true });
    }

    const user = await prisma.user.findUnique({
      where: { email },
      include: { password: true },
    });

    if (!user) {
      return invalid(400, { credentials: true });
    }

    const matchingPassword = await bcrypt.compare(
      password,
      user.password?.hash ?? ''
    );

    if (!matchingPassword) {
      return invalid(400, { credentials: true });
    }

    const session: User = { id: user.id, email: user.email };
    const token = jwt.sign(session, SESSION_SECRET);

    cookies.set('session', token, {
      path: '/',
      httpOnly: true,
      sameSite: 'strict', // TODO: or lax?
      secure: process.env.NODE_ENV === 'production',
      maxAge: 60 * 60 * 24 * 30, // a month
    });

    throw redirect(302, '/admin');
  },
};

I have the feeling this is correctly done, however the typing of form is "wrong":

type ActionData = {
    invalid: boolean;
} | {
    credentials: boolean;
}

Wrong in a sense that I can't properly use form?.invalid in my svelte page without having a TS error:

Property 'invalid' does not exist on type '{ invalid: boolean; } | { credentials: boolean; }'.

Shouldn't the type rather be:

type ActionData = {
    invalid?: boolean;
    credentials?: boolean;
}

Don't get me wrong, I understand why the type is generated as-is, I'm just wondering if that's what we want as we can't use form?.invalid (which would be convenient) 🤔

Originally posted by @binajmen in #6820

@dummdidumm
Copy link
Member

The type is correct, because it can either be shape X or shape Y, the combined type you suggest doesn't exist like this, you can't have both set at the time which the type suggests. That said, it would be nice if we somehow could make this work by enhancing the shape so that all properties are accessible while leaving the type correctly. In this example it would be

type ActionData = {
    invalid: boolean;
    credentials?: never;
} | {
    credentials: boolean;
    invalid?: never;
}

I'm not sure if this is possible to achieve, but I'm giving it the enhancement label.

dummdidumm pushed a commit that referenced this issue Sep 19, 2022
…ata is missing (#6869)

Fixes #6823
Fixes #6822

Co-authored-by: William Viktorsson <wilvik05@edu.umea.se>
Co-authored-by: williamv <williamv@cs.umu.se>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants