search

norkator / poker-pocket-backend

Nitramite Poker Pocket light weight NodeJS poker game backend
https://pokerpocket.nitramite.com
MIT License
31 stars 20 forks source link

Handle invalid roomId in holdem.js #22

Closed shrpne closed 1 year ago

shrpne commented 1 year ago

Custom clients may send invalid roomId, so it will throw TypeError: rooms[roomId] is undefined and crash the server app.

Since there are also repetitive checks of players[connectionId].connection !== null and players[connectionId].socketKey === socketKey, I would like to implement the fix by introducing helper function that will check connection and also check roomId when it is needed.

To implement it correctly I would like to hear your feedback:

  1. There are places where you only checking players[connectionId].connection but not checking socketKey. L543, L560, L615 etc. Is it intended? Maybe we should check socketKey there too?
  2. There are else branches which always throw because there is no players[connectionId].connection.sendText when players[connectionId].connection === null. L745, L769. How should I handle these else branches?
shrpne commented 1 year ago

I've implemented checking roomId, so resilience against invalid input is improved and it is ready to merge.

I've left untouched places with unclear behavior, I can fix them later after your feedback or we can leave them as is

(BTW you may find it easier to review with disabled whitespaces, since there is a lot of updated padding https://github.com/norkator/poker-pocket-backend/pull/22/files?w=1 )

norkator commented 1 year ago

@shrpne do you want me add you as collaborator so you can freely maintain this backend as you feel right or change behaviour if something I have made makes no sense.

I was developing this long time ago and would not probably take same steps as I have been doing here.

So to answer your questions:

  1. so it looks like you implemented these. Looks good. I test this quickly soon. "Is it intended" no, I was just developing quickly and didn't think this trough.
  2. probably do nothing in this case so maybe remove these else branches.
norkator commented 1 year ago

@shrpne will you continue working in this branch or should title "Draft:" be removed and this pr merged?

shrpne commented 1 year ago

I've removed else branches and Draft title. It is ready to merge.

As for maintaining the repo, I'm not sure. I will develop a fork that will be highly opinionated to meet specific needs of my project. And I'm not sure how many features I will be able to backport to this repo, probably not many