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

PrivateRoute auth vulnerable to user spoofing localStorage? #152

Closed
rbracco opened this issue Feb 8, 2021 · 4 comments
Closed

PrivateRoute auth vulnerable to user spoofing localStorage? #152

rbracco opened this issue Feb 8, 2021 · 4 comments

Comments

@rbracco
Copy link

rbracco commented Feb 8, 2021

I'm very new to auth so sorry if this is all wrong. The code for isAuthenticated which is used to avoid rendering PrivateRoutes to unauthenticated users just checks local storage for a plain string permissions. Wouldn't this make it possible for a user to set 'permissions' to 'user' or 'admin' manually on local storage and then be able to possibly access static information that they shouldn't be able to? I'm guessing this wouldn't let them access any data via the API, but shouldn't isAuthenticated call the server and validate the token? Thank you.

export const isAuthenticated = () => {
  const permissions = localStorage.getItem('permissions');
  if (!permissions) {
    return false;
  }
  return permissions === 'user' || permissions === 'admin' ? true : false;
};
@Buuntu
Copy link
Owner

Buuntu commented Feb 9, 2021

This just checks if the user is authenticated for UI purposes. The backend should always check if the given user is then authorized to access a given resource. Frontend code in and of itself is not secure so I think this is fine, the user can always spoof local storage but it's the backend's responsibility to then make sure that the user has the appropriate permissions.

@rbracco
Copy link
Author

rbracco commented Feb 9, 2021

Thank you for the quick reply and all your work on this library.

That makes sense about the backend handling authentication, but is there any reason we wouldn't just have isAuthenticated send the token off to our Users router on the background for validation? Then if someone ends up with static frontend content that only users should see, it doesn't get shown to non-users. The only drawback I can see is we then have to send an additional request on the first render.

Again I'm new to auth so I might be off here, would love to hear people's thoughts and feedback. Cheers.

@Buuntu
Copy link
Owner

Buuntu commented Feb 9, 2021

Thank you for the quick reply and all your work on this library.

That makes sense about the backend handling authentication, but is there any reason we wouldn't just have isAuthenticated send the token off to our Users router on the background for validation? Then if someone ends up with static frontend content that only users should see, it doesn't get shown to non-users. The only drawback I can see is we then have to send an additional request on the first render.

Again I'm new to auth so I might be off here, would love to hear people's thoughts and feedback. Cheers.

Yeah, there are many ways to handle this. I agree that if this were a production application I would take a look at authentication again. For simple apps I don't really see the harm in simple permissions check like this on the frontend, as long as the backend is doing the real authorization. The worst that could happen if someone took the liberty to change their local storage manually is they would see the admin page, but no admin data. This is a single page application so you can technically already see the admin page once anything loads.

That's my two cents, if you want to elaborate on this or submit a PR I'd take a look but this doesn't seem like a glaring security issue to me.

@rbracco
Copy link
Author

rbracco commented Feb 9, 2021

Thank you, that seems reasonable. If I come up with a potentially better solution I'll reopen the issue and/or submit a PR. Cheers.

@rbracco rbracco closed this as completed Feb 9, 2021
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

No branches or pull requests

2 participants