search

gothinkster / aspnetcore-realworld-example-app

ASP.NET Core backend implementation for RealWorld
https://realworld.io
MIT License
1.92k stars 546 forks source link

Security Risk - User Hijacking? #52

Open keithn opened 5 years ago

keithn commented 5 years ago

Not sure, just glancing through the code, but in user Edit it doesn't seem like user names are required to be unique when editing? all the code to deal with uniqueness is on create.

A current user could change their username to another users user name, Then they could edit again and then the firstordefault will likely give them access to the original user.

keithn commented 5 years ago

also, seems like there is code between create and edit is duplicated... This doesn't seem like a good idea.

adamhathcock commented 5 years ago

Pull requests are welcome to fix possible issues.

Without looking at the code, you might be right about duplicattion. However, a little duplication can be better than added complexity to remove the little duped code. As always: it depends

keithn commented 5 years ago

I'm just having a look more than anything else, I was just curious how security was being handled. I think because of how the user concerns are spread out and are quite noisy it leads to the security problem , the duplication was the hint that should of pointed to more cohesive solution to the rules. I don't think I'd ever endorse having two locations in the code where you set things like passwords.

adamhathcock commented 5 years ago

Cross cutting concerns can be put in MediatR pipelines if there are any.

Things are organized vertically to illustrate how functionality is usually simple before “concerns” are added in. I’ll admit things aren’t perfect nor fully audited but I see no reason for further abstraction as there aren’t more rules to implement yet.