Made Server Less Error Prone #107
Labels
No Label
client
server
user made
L
M
S
XL
bug
bugfix
discussion
documentation
feature
maintenance
postponed
refactoring
wontfix
No Milestone
No Assignees
2 Participants
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: zdm/envoy#107
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "f/secure-server"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Made server far less error proneto Made Server Far Less Error ProneMade Server Far Less Error Proneto Made Server Less Error ProneAs already discussed, the
AuthenticatedRequest
is an unnecessary overhead. The server can compare user ID and socket ID using theConnectionManager
.The actual problem is, that an authenticated client might send objects to the server, that might not be supposed to be sent by that client. For example, a message could be sent, whose sender ID is not the user ID bound to the socket on which that message is received by the server.
As far as I can see, this is not prevented by these changes, but could be, easily, inside the individual processors, which have access to the
ConnectionManager
and are supposed to handle request-specific logic, like comparing two user IDs for a received message.I do not know if you have noticed, @kske, but the AuthenticatedRequest only sends the id of the user who is supposed to be the originator, and the real request. The server compares this id to the id bound to the socket and hence can verify whether the correct client sent this message.
However, I do agree that it is still possible to send a message with wrong sender/ recipient, meaning that the same procedure should also be performed in the (
Group
-)MessageProcessor
. For most other cases, it prevents unauthorized access. Only those cases, where the userID has been sent should be checked again.The question however should be: Is this bug worth fixing before it might/ will be scrapped by the new data model?
I have noticed and that's why I mention it. There is no need send the user ID, as the server has already authenticated the user and can infer the user ID from the socket ID. There is no way it could be "incorrect". Also, a malicious client could just choose the correct user ID while still sending invalid data, making the mechanism even more unnecessary.
I thought this was the entire point of this pull request, until I actually looked at the changes. The comparison between the registered user ID and the sender ID of messages sent by that client is an essential part of server security.
I do agree that this should be postponed until the new data model is in place, as it will cause structural changes to the different processors on the server, in which aforementioned security checks should be implemented.
Ok, so the PR does what it should do now, namely verifying a user's ID against the IDs user in objects sent by that user.
What I don't understand, however, is why
AuthenticatedRequest
is still here. I have explained in great detail why I deem it unnecessary and don't see why it is still used.Done
Very nice 👍