symfony / ux

Symfony UX initiative: a JavaScript ecosystem for Symfony
https://ux.symfony.com/
MIT License
835 stars 307 forks source link

[TURBO] turbo streams role management and exposing sensitive data #1331

Closed n3o77 closed 3 months ago

n3o77 commented 10 months ago

We recently started to use ux-turbo and now run into the problem that the stream updates are always rendered for the user who triggers the change (which in hindsight makes total sense). But this means that role checks in templates don't make sense for other users. In our case we display senstive and more detailed data only to admin users.

To reproduce this create 2 users one with ROLE_ADMIN. In the turbo stream template:

{% block create %}
{% endblock %}

{% block update %}
    <turbo-stream action="replace" target="identifier">
        <template>
            <div id="identifier">
                {% if is_granted('ROLE_ADMIN') %}
                    <p>Sensitive Data</p>
                {% endif %}
                <p>General Data</p>
            </div>
        </template>
    </turbo-stream>
{% endblock %}

{% block remove %}
{% endblock %}

Now trigger an update with the ROLE_ADMIN user. As this block will be rendered from the admin users session both elements will be streamed to all connected users.

In our case this happened in some nested template, much less obvious as in the example above. Of course we could separate the update streams / listeners by users etc. or build the template in a different way to have that data better separated. But that would be something to always keep in mind and every dev who works on the codebase needs to be aware of that circumstance and needs to check that the template part being worked on might be used in some stream update. Which i think is only a matter of time until it goes wrong when the codebase is getting bigger or more people working on it.

If i'm not missing something i don't think it's currently possible to handle this in the current implementation but it would be nice to have a warning in the docs for awareness.

smnandre commented 10 months ago

You use asynchronous streams, right ?

n3o77 commented 10 months ago

Yes, basically what's described here: https://symfony.com/bundles/ux-turbo/current/index.html#coming-alive-with-turbo-streams

smnandre commented 10 months ago

Did you try using Mercure private updates ?

https://symfony.com/doc/current/mercure.html#authorization

https://symfony.com/bundles/ux-turbo/current/index.html#broadcast-options

n3o77 commented 10 months ago

Yes. But, if i'm not missing something, that only checks if the user is authenticated / has rights to receive updates on that stream. It doesn't affect which data is sent over the stream or how that data is rendered.

n3o77 commented 10 months ago

For anybody coming across this issue here's a quick and dirty workaround. But so far this seems to solve all of our issues with just minor adjustments to our codebase.

https://gist.github.com/n3o77/0c9bdd9f566eb23435a39953ea980f04

Basically we only send some key for the update over the event stream. Each client then sends a fetch request to render all the necessary turbo-streams which are passed to turbo to render the streams. This adds some delay and of course more load, but it's better than nothing till a better solution exists.

carsonbot commented 4 months ago

Thank you for this issue. There has not been a lot of activity here for a while. Has this been resolved?

carsonbot commented 3 months ago

Friendly reminder that this issue exists. If I don't hear anything I'll close this.

carsonbot commented 3 months ago

Hey,

I didn't hear anything so I'm going to close it. Feel free to comment if this is still relevant, I can always reopen!