craftcms / cms

Build bespoke content experiences with Craft.
https://craftcms.com
Other
3.25k stars 630 forks source link

FR: Hashed or Encrypted Parameters for Front-End Forms #2671

Closed theskyfloor closed 6 years ago

theskyfloor commented 6 years ago

Description

It would be rad if hashing or encrypting parameters on front-end forms was a universal option much like the added ability to hash redirect parameters:

All redirect params must now be hashed using the |hash filter (e.g. <input type="hidden" name="redirect" value="{{ 'foo'|hash }}">). A new redirectInput() global function makes this a little easier (e.g. {{ redirectInput('foo') }}).

Use case is adding a very basic way of protecting front-end forms from manipulation without the need to resort to a custom plugin or controller.

We have built our own version of this a few times when bootstrapping Craft in an external PHP script usually relying on AES-256-CBC and OpenSSL. Why not have Craft look for a hashed value and the decrypt it as an option for front end form parameters like entry id, user id, and/or any other sensitive form parameters?

Additional info

chasegiunta commented 6 years ago

Was about to submit an FR for the same. Echoing my need for this as well.

brandonkelly commented 6 years ago

Why not have Craft look for a hashed value and the decrypt it as an option for front end form parameters like entry id, user id, and/or any other sensitive form parameters?

If it’s optional, then getting around this would be as simple as replacing the hashed value with a tampered, non-hashed value.

theskyfloor commented 6 years ago

Good point. Not optional then : ) I guess my thought was it would be a shot in the dark even more so and someone would have to guess that just a number could work. So security through obscurity but I agree that having it optional is a dumb idea and I admit to being dumb for thinking it, haha.

chasegiunta commented 6 years ago

@theskyfloor I’m right there with ya. Didn’t think of that. Ha.

I was needing this specifically for hashing a section ID in a frontend entry form. Attempted to do the decrypting in the beforesave event, but apparently the model validating happens before the event, and it flags the hashed value.

Custom controller for now seems the way to go.

theskyfloor commented 6 years ago

For sure. I mean I see this as an essential and basic level of security for a CMS that allows for front-end entries. IMO we shouldn't have to create custom controllers to do it but obviously P&T is free to disagree : )

chasegiunta commented 6 years ago

Well, considering the requirement being a fairly large breaking change for current sites, it doesn’t make sense for Craft 3. We’re a bit too late.

theskyfloor commented 6 years ago

That to. I'm retiring, haha, thanks @chasegiunta I couldn't stand the embarrassment if BK pointing out my lack of intelligence again.

I guess the optional part would be a config setting or something?

brandonkelly commented 6 years ago

I’m not sold on why this is so essential. There’s already a permission system in place that ensures that users can’t post things that they’re not authorized to do.

theskyfloor commented 6 years ago

For me, it's "essential" when I am building a system on the front end where users have permissions for the whole section but are only intended to edit or create their own content and no one else's. I don't want shifty users to be able to look at a form and force in different parameters to see what happens. 90% of the time they would prob butt up against the wrong section or permissions but there is a chance that they would correctly guess an ID and at that point, they would be compromising the way it was designed to work (not to mention other people's data).

Unless I am mistaken the only other ways to deal with this would be writing a custom controller to either hash the parameters per site, to create a user group for every user, or to check for custom conditions in a custom controller. None of these feel like practical solutions when I have had this issue come up on several projects.... I understand that I could write something that might give me this ability on more than one of my projects but I am not very good at PHP 7/Craft 3 coding and some people who might need this basic and added level of security might have even less experience with custom solutions than me.

At the same time, I understand the obvious constraints of picking what is a core function of the CMS and what is not. I won't be offended if you want to reject and close the issue... I just like suggesting things I feel would be good for the platform : ) but it ain't up to me!

brandonkelly commented 6 years ago

As long as you don’t give them the “Edit other authors’ entries” permission, they will get a 403 error if they try editing an entry that doesn’t belong to them.

theskyfloor commented 6 years ago

Hmmm. That’s an excellent point. Still there have been a few builds where the users are allowed to edit other peoples entries but shouldn’t be able to edit all of the entries. Obviously we hide the other entries from them but the scenarios described above would still apply or at least I think so. Could be wrong again. Certainly it’s getting more fringe by the minute. @chasegiunta what was your use case?

chasegiunta commented 6 years ago

In my case, I run a college sports league site on Craft. An team (entry) doesn’t belong to just one person (or author) it belongs to many coaches. Editing their roster, submitting upcoming scheduled events, submitting results, athlete records, etc is all done through frontend forms.

Technically a coach, or team president (user) could edit any athlete’s (user) record on any team if they knew the ID (or just guessed by happy accidents). They could edit any team’s info.

The “permissions checking” I have in place is done through relationship fields. “Does this user belong to this team?”

There’s ton of frontend interactions like this where I have to either use events or a custom controller to do this relationship checking, but it would be a great convenience in the future building out further interactions to not have to worry about if some college kid messed with the html.

Admittedly, this is more of an application, not your traditional content editing website.

theskyfloor commented 6 years ago

That’s basically the perfect description of my use case too. Different taxonomies same idea.

brandonkelly commented 6 years ago

So I’d say that a better approach to this is to write a plugin/module that adds an extra authorization layer to the back-end. Basically listen to the Entry::EVENT_BEFORE_SAVE event, and add whatever checks you need from there. If it looks like they’re trying to edit something they shouldn’t, throw a ForbiddenHttpException (403 error).

chasegiunta commented 6 years ago

If this FR is still viable, seems that it would have to be a config setting that would require any of those normally hidden required values (section ID, user ID, etc) be hashed. Though, it wouldn't surprise me there's other unforeseen issues with this.

Otherwise, I'd say close this issue.

The Entry::EVENT_BEFORE_SAVE is what I'm currently doing - and while it seems cumbersome to do for every authenticated frontend interaction, it may be the only option.

brandonkelly commented 6 years ago

Yeah idk, I’m not sold on it. Permissions should always be enforced in the back end.

The reason that redirect params get hashed is because their values get executed as Twig code. So it’s much more dangerous to leave those open for users to tamper with.