tomichj / invitation

A Rails gem that can send 'scoped' invitations
MIT License
77 stars 28 forks source link

Security concerns #5

Closed vincentwoo closed 7 years ago

vincentwoo commented 7 years ago

The default approach in the readme suggests

new_invite_path(invite: { invitable_id: account.id, invitable_type: 'Account' } ) %>

However, this relies on the invitable id and type being set by the browser. A malicious user could easily edit the form to point at another invitable id. This library should probably set secure options like this only on the server side.

tomichj commented 7 years ago

This is an excellent point. The current code is attempting to provide a simple, easily implemented system that can be extender to provide security checks. Then I barely documented this process.

My current intent is that, if issuing invitations to your resources require authorization, you extend InvitesController and add a before_action to authorize access to the resource or resources. In our hypothetical Account-based example, that might mean loading the account to check current_user's authorization to issue invitations to that account, e.g. account.can_invite?(current_user).

I mentioned authorization checks very briefly in the controller documentation, and not at all in the README.md. This is a huge error on my part.

For the near-term, I will:

I'm not necessarily opposed to setting options server-side; let me consider further. If you feel like providing any examples of how this might be implemented in a highly generalized but simple way, I'd appreciate hearing anything more you'd like to contribute.

Thanks very much for the feedback!

vincentwoo commented 7 years ago

Thank you, I appreciate your thoughtful response. I basically did as suggested and hacked in the params in a route in the controller.

tomichj commented 7 years ago

Updated the README.md with some notes on security and a code sample. I might add additional code samples to the wiki, including something based on your current_org example, to the wiki in a future update. Closing this ticket for now, as the basics are met. Thanks again!