Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

doc: add notes about CSRF and Form requirements #98

Merged
merged 5 commits into from
Sep 27, 2024

Conversation

SpartakusMd
Copy link
Contributor

@SpartakusMd SpartakusMd commented Aug 6, 2024

humanize twig filter is used to display a human readable scheduler names. The filter uses FormRenderer which is available in symfony/form. Should the bundle drop the use of humanize? It seems too small benefit to add such a big dependency, especially for API or console projects.

Error:

Uncaught PHP Exception Twig\Error\SyntaxError: "Did you forget to run "composer require symfony/form"? Unknown filter "humanize" in "@ZenstruckMessengerMonitor/schedule.html.twig"." at UndefinedCallableHandler.php line 99

@kbond
Copy link
Member

kbond commented Aug 15, 2024

You're totally right. Out of curiosity, does item.name|humanize|default(item.name) work?

@SpartakusMd
Copy link
Contributor Author

SpartakusMd commented Aug 15, 2024

@kbond nope, item.name|humanize|default(item.name) does not work, same error that humanize is unknown filter.

Also while debugging, I discovered that there is also used the CSRF token from symfony/form data-token="{{ csrf_token(['trigger', task.id, t]|join('-')) }}" in order to retrigger a scheduled message. Would you prefer the symfony/form to be added as a dependency or CSRF validation removed from this specific URL?

image

@kbond
Copy link
Member

kbond commented Aug 20, 2024

We're kind of in a tough spot here. The csrf & form is only required when using the UI. This package can still be used without so I don't want to make these hard deps. I think, for now, just adding notes to the doc would be best.

In the future, the best way would be to break this package up into backend and frontend packages.

  • messenger-monitor-bundle (backend)
  • messenger-monitor-ui-bundle (frontend)

@SpartakusMd
Copy link
Contributor Author

SpartakusMd commented Aug 22, 2024

Got it. Didn't remove the CSRF check and added the suggestion to install symfony/form. Also added the dependency in or project even if it's an API :/ Still removed the humanize from the schedule name, so that it's easier understandable which schedule it is. Can revert it if needed.

README.md Outdated Show resolved Hide resolved
templates/schedule.html.twig Outdated Show resolved Hide resolved
@SpartakusMd SpartakusMd requested a review from kbond August 22, 2024 19:20
@SpartakusMd
Copy link
Contributor Author

After some more testing, I see there are more changes required to support CSRF.

CSRF protection is not enabled in your application. Enable it with the "csrf_protection" key in "config/packages/framework.yaml"
CSRF protection needs sessions to be enabled.

Should I add it to the Readme too? I would rewrite the note to:

Note

If using symfony/scheduler, you'll need symfony/form and symfony/security-csrf
(composer require symfony/form symfony/security-csrf) and enable CSRF protection and sessions in your
"config/packages/framework.yaml".

@kbond
Copy link
Member

kbond commented Sep 27, 2024

@SpartakusMd, sorry for the delay here. So if not using scheduler, csrf is not required? Maybe let's have two notes then:

Note

symfony/form (composer require symfony/form) is required for the UI.

Note

If using symfony/scheduler and the UI, symfony/security-csrf (composer require symfony/security-csrf) is required. CSRF protection and sessions must also be enabled in your
config/packages/framework.yaml.

@kbond
Copy link
Member

kbond commented Sep 27, 2024

Thanks for your patience @SpartakusMd!

@kbond kbond merged commit 79df644 into zenstruck:1.x Sep 27, 2024
11 of 12 checks passed
@kbond kbond changed the title Remove usage of humanize twig filter doc: add notes about CSRF and Form requirements Sep 27, 2024
@kbond kbond mentioned this pull request Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants