upleveled / security-vulnerability-examples-next-js-postgres

https://vuln-examples-next-postgres-jose.vercel.app/
11 stars 6 forks source link

Add DTO for users table for example 5 #29

Open dschwarz91 opened 1 year ago

dschwarz91 commented 1 year ago

at the moment, different SQL-Queries are used to either include or exclude the user pwd hash. Best practice would be to use a DTO to make unintentional mistakes less common

karlhorky commented 1 year ago

Interesting idea! Do you think you could edit the description above to also include 2 code blocks for "Before" and "After"? I would be interested to see what you mean from a code perspective.

dschwarz91 commented 1 year ago

similar to this: https://khalilstemmler.com/articles/enterprise-typescript-nodejs/use-dtos-to-enforce-a-layer-of-indirection/

dschwarz91 commented 1 year ago

so we could do something like this for the user object/class in example 5:

interface userDTO {
  id: Number;
  name: String;
}

class userMap {
  public static toDTO(user: User) {
    return {
      id: user.id,
      name: user.username
    } as userDTO
  }

}

I'm just having some trouble with the handling of the multiple rows....

karlhorky commented 1 year ago

Looks complex... what's the benefit? (and how would you explain this benefit to beginners?)

Usually we try to avoid any layers of indirection unless they are absolutely necessary.

dschwarz91 commented 1 year ago

The idea is, that the DTO more or less defines the structure of your API (like an API contract). Advantages:

but I also think about skipping this part.

karlhorky commented 1 year ago

Hm... not sure how a DTO by itself would prevent exposure of an API key like this:

https://github.com/upleveled/security-vulnerability-examples-next-js-postgres/blob/ed88e44e5304bf700235ea3f2c8e85ef13c5aa13/pages/example-5-secrets-exposure/vulnerable.tsx#L31-L41

In this case our Props type in this file acts as our "DTO", if I'm understanding correctly. And in this case, apiKey is part of the Props (meaning that it's part of the DTO).

A DTO by itself probably doesn't inherently increase security, if you were to have the secret data defined in that DTO accidentally (like in this Props example).


I'm also similar skeptical about the value of a DTO in keeping public-facing APIs stable over time (if you change your internal data structure, then you may similarly mess up exposing it properly via the DTO). I've gone this route of abstraction before, but in the last years, I've been writing my code simpler and with stronger guarantees like full-stack type safety, and it seems to work pretty well to avoid these problems.

dschwarz91 commented 1 year ago

Didn't talk about the API key, but about the password-hash contained in the user-objects in line 38. If you define which attributes are exposed via the API (with the DTO), it doesn't matter if the database-query returns additional data fields or not, you can be sure that only the intended data gets exposed.

DTOs don't help you with the whole API-key problem, because this is actually a generic design issue (in my opinion) if you need an API key for a backend-service in the frontend.

But I absolutely get your point regarding the additional complexity...

Let's skip this whole DTO thing for now... This is maybe a little bit too complex/controversial for such a short beginners course.

karlhorky commented 1 year ago

Ok, I guess my point is similar with the users with password hash though: if the DTO (Props can be seen like this here), define a permissive data structure like User (allowing a password hash), then you would still have the problem. The DTO itself doesn't solve the problem, it just makes it more clear to see the error and centralizes the place where problems should be looked for (although really, you probably want to look for problems on all layers anyway).

Instead of adding these extra abstractions, another (simpler) solution would be on the type level in the database file:

export type User = {
  id: number;
  username: string;
+ passwordHash: never;
};

-export type UserWithPasswordHash = User & {
+export type UserWithPasswordHash = Omit<User, 'passwordHash'> & {
  passwordHash: string;
};

Then your code doesn't even compile if a user object has a passwordHash property and there's no way that you make this mistake, all without extra abstraction layers.

That could be added as part of one of the existing solutions, or added as a new solution too.