livewire / livewire

A full-stack framework for Laravel that takes the pain out of building dynamic UIs.
MIT License
21.94k stars 1.5k forks source link

Add global property lock #8586

Closed PhiloNL closed 4 days ago

PhiloNL commented 2 weeks ago

This PR adds support for a global property lock. The idea is similar to the Laravel default where models are guarded from mass assignment.

Especially newcomers to Livewire seem unaware that public properties can be modified using the client library and the security implications.

How it works To enable this feature, a user would add Livewire::lockProperties(); to their AppServiceProvider. When enabled, a CannotUpdateLockedPropertyException exception will be thrown whenever a public property is attempted to be updated from the client side.

class AppServiceProvider extends ServiceProvider
{
    /**
     * Register any application services.
     */
    public function register(): void
    {
        Livewire::lockProperties();
    }
}

A property can be unlocked on the property level:

use Livewire\Attributes\Unlocked;

class Counter extends Component
{
    #[Unlocked]
    public $count = 0;
}

It is also possible to unlock all the properties for an entire component by adding the attribute to the root level:

use Livewire\Attributes\Unlocked;

#[Unlocked]
class Counter extends Component
{
    public $count = 0;
}

Maybe this global lock could be enabled by default in Livewire 4 to provide the most secure setup by default.

I'll add some docs if this PR is good to go 😄

calebporzio commented 1 week ago

Hmmm yeah I can see that. Definitely something that's not obvious to new-comers.

With this enabled though, they wouldn't be able to make properties for a simple form with wire:model though right?

chillbram commented 1 week ago

This PR is pretty close to the idea I had about locking full components (#8327). Personally I'm not a big fan of setting this globally as it would require changes to all existing Livewire components, although I do understand your motivation with regards to new and unexperienced users and having a secure default in the future.

In any case, these ideas can be very compatible. What do guys think about extending this PR to allow for Locking and Unlocking at all three levels? Level Locked Unlocked
Application set in AppServiceProvider default
Component #[Locked] #[Unlocked]
Property #[Locked] #[Unlocked]
PhiloNL commented 1 week ago

With this enabled though, they wouldn't be able to make properties for a simple form with wire:model though right?

Correct. Interestingly, at Laravel Live UK I asked people if they use Model::unguard(), and turns out, in this case, it was just a small group, so most people set their $fillable attribute.

So from that perspective, I can see this as a valuable option to have the most secure setup with a simple ServiceProvider call. Maybe there are some additional things we can implement in the future and have Livewire::strict() similar to features Laravel provides :)

PhiloNL commented 1 week ago

This PR is pretty close to the idea I had about locking full components (#8327). Personally I'm not a big fan of setting this globally as it would require changes to all existing Livewire components, although I do understand your motivation with regards to new and unexperienced users and having a secure default in the future.

In any case, these ideas can be very compatible. What do guys think about extending this PR to allow for Locking and Unlocking at all three levels?

Level Locked Unlocked Application set in AppServiceProvider default Component #[Locked] #[Unlocked] Property #[Locked] #[Unlocked]

This PR should already allow for this 😄

chillbram commented 1 week ago

It's missing the Component level #[Locked], which should then also allow individual properties to be #[Unlocked] within it. I'd be willing to try my hand at adding it, if you're alright with that.

devajmeireles commented 6 days ago

Speaking from my experience with Livewire 3, I don't believe there is a scenario where large Livewire 3-based applications would use this. I'm thinking about big applications. Perhaps to resolve this "issue", it would be a good idea to offer the possibility of defining it globally - as the current way, as well as by specific components or by namespace, affecting all components of a given folder.

Livewire::lockProperties(); // all
Livewire::lockProperties('Payment'); // per namespace (Livewire/Payment/*)
Livewire::lockProperties([Livewire\Payment\Renewal::class]); // per component
calebporzio commented 4 days ago

Ok, here is my take: "no, for now"

I understand that it's surprising to people that public properties can be manipulated in the front-end, even with all the documentation messaging we have for educating them.

But I don't want to introduce an API that makes it feel like Livewire is insecure if you don't use it or something IDK. It also adds friction if it's the default. Having that friction makes sense for something like guarded model attributes, but I think public properties are more analogous to request input keys and don't need that level of security.

Anyhow, thanks for the PR Philo, and the input everyone, but I'm closing this for now.