Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[LiveComponent] Update CSRF token after component request #2022

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

tijnema
Copy link
Contributor

@tijnema tijnema commented Aug 1, 2024

Q A
Bug fix? yes
New feature? yes
Issues
License MIT

When CSRF is enabled on live components, each request will received a new CSRF token in the response. Currently all subsequent requests made by live components will use the initial CSRF token received. This PR solves this, by updating the CSRF token with the one received in the last response.

@carsonbot carsonbot added Bug Bug Fix Feature New Feature LiveComponent Status: Needs Review Needs to be reviewed labels Aug 1, 2024
@tijnema tijnema force-pushed the feature/update-csrf-token branch 2 times, most recently from 8fdf7ae to 8c02735 Compare August 1, 2024 12:53
@smnandre
Copy link
Member

smnandre commented Aug 1, 2024

Hi @tijnema ! Do you thing you could show me a live example of the problem, or a small reproducer?

I've never had problems with csrf tokens but i often use components without so 😅 ..

@tijnema
Copy link
Contributor Author

tijnema commented Aug 2, 2024

We see this in various ways, most common however is that CSRF tokens will reset on login (like issue #1150). After login we rerender live components to refresh the tokens. With this PR applied, the actions work normally.

To reproduce, just use a simple live component with action, set session life time short, wait for session to expire and press action. It will fail with "CSRF token invalid". You could use the following stimulus controller to reload all components every 3 seconds:

import {Controller} from '@hotwired/stimulus';
import {getComponent} from '@symfony/ux-live-component';

export default class extends Controller {
    connect() {
        setInterval(this.reloadLiveComponents, 3000);
    }

    async reloadLiveComponents() {
        const componentElements = document.querySelectorAll('[data-controller~="live"]');
        for (const componentElement of componentElements) {
            await getComponent(componentElement).then(component => {
                component.render();
            });
        }
    }
}

This makes sure the component gets new CSRF token.
With this PR, action still works after session expired

@smnandre
Copy link
Member

smnandre commented Aug 2, 2024

We see this in various ways, most common however is that CSRF tokens will reset on login (like issue #1150). After login we rerender live components to refresh the tokens. With this PR applied, the actions work normally.

If you have two form on a page "Foo" currently, with csrf tokens. You open another page and log in. You come back on the page "Foo" and submit one of the form. What happens ?

just use a simple live component with action, set session life time short, wait for session to expire and press action

Is this not the same for a classic Form... or am i missing something here ?

With this PR, action still works after session expired

Do we want this ?

@tijnema
Copy link
Contributor Author

tijnema commented Aug 5, 2024

We see this in various ways, most common however is that CSRF tokens will reset on login (like issue #1150). After login we rerender live components to refresh the tokens. With this PR applied, the actions work normally.

If you have two form on a page "Foo" currently, with csrf tokens. You open another page and log in. You come back on the page "Foo" and submit one of the form. What happens ?

It depends. If there's a mechanism which reloads the live component (Like the stimulus controller I posted above), it works. Otherwise it will fail with an invalid CSRF.

just use a simple live component with action, set session life time short, wait for session to expire and press action

Is this not the same for a classic Form... or am i missing something here ?

Yes and no. With a classic form, CSRF is usually stored in a hidden input _csrf_token. Updating CSRF after login with JS is as simple as updating this input field, or rerender the whole form server side and it should work.

With this PR, action still works after session expired

Do we want this ?

Yes. It's still safe because the CSRF will be bound to the users' session. It will be the new session which has been started, instead of it's previous expired one.

@smnandre
Copy link
Member

It depends. If there's a mechanism which reloads [..]. Otherwise it will fail with an invalid CSRF.

So currently there is no difference with the behaviour of standard Form, right ?

Updating CSRF after login with JS is as simple as updating this input field, or rerender the whole form server side and it should work.

Is there something here you cannot do with LiveComponent ? You can re-render the whole component too right ?

Yes. It's still safe because the CSRF will be bound to the users' session. It will be the new session which has been started, instead of it's previous expired one.

But the form contains checksum and data from another user.

I'm not convinced for now, so let's wait other opinions on this.

@smnandre
Copy link
Member

Hello @tijnema!

So it seems the team agrees with you on the concept "If a component send another CSRF token, it should be updated". So waiting was not a bad idea :)

As you see it, is your PR ready or does it need some changes/updates ?

@smnandre
Copy link
Member

Friendly @tijnema ? Is this PR ready to merge for you ? :)

@tijnema
Copy link
Contributor Author

tijnema commented Sep 2, 2024

Sorry, I was enjoying holiday. Good to hear the team agrees with me :). As far as I'm concerned, this PR is ready to merge.

@smnandre
Copy link
Member

smnandre commented Sep 3, 2024

I'm just waiting for someone else to check/validate and let's merge it !

.... because we may need soon to adapt things here 😅 cc PR 58095 on symfony/symfony

Thank you @tijnema !

Copy link
Member

@smnandre smnandre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Good for me, i'd like someone else to check)

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Sep 3, 2024
Copy link

@italodeveloper italodeveloper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works to fix the bug in active components!

@smnandre smnandre removed the Bug Bug Fix label Sep 12, 2024
@smnandre
Copy link
Member

It took time, but here we go, this is in now. Thank you very much @tijnema.

@smnandre smnandre merged commit f2deb0f into symfony:2.x Sep 12, 2024
14 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Feature New Feature LiveComponent Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants