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

chore: enable csrf token to be read from request headers #2586

Merged

Conversation

muellerdemos
Copy link
Contributor

@muellerdemos muellerdemos commented Jan 17, 2024

This PR introduces to provide a csrf token via request headers. The main reason for this is that currently we are only checking csrf token on rpc form requests by adding a hidden input field to the form which contains the token.

From my perspective this is not optimal. We also load our addons via a rpc route but we do not submit a form therefore the loading of addons results in purple error messages saying the csrf token is missing. Also I find this a more convenient way to handle the csrf token.

In a coming PR all occurrences of hidden input fields can be removed since the changes of this PR will parse the headers for a X-Csrf-Token field which contains the token and can be validated within the CsrfSubscriber class.

How to review/test

  • Navigate e.g. to ewm's overview page of a procedure
  • Make sure the purple error message saying that a csrf token is missing

Linked PRs (optional)

demos-europe/demosplan-ui#714

Important

Before removing the hidden input field throughout dplan the demosplan-ui PR needs to be merged first (however merging this PR will not break things since both cases can be handled now when receiving rpc requests)

@muellerdemos muellerdemos marked this pull request as ready for review January 17, 2024 16:51
@muellerdemos muellerdemos self-assigned this Jan 17, 2024
@muellerdemos muellerdemos added the review:backend PRs that are missing reviews from a BE perspective label Jan 17, 2024
mussbach
mussbach previously approved these changes Jan 22, 2024
Copy link
Contributor

@mussbach mussbach left a comment

Choose a reason for hiding this comment

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

lgtm, thx! Code review only

@mussbach
Copy link
Contributor

@muellerdemos branch needs to be adjusted before merge

@muellerdemos muellerdemos changed the base branch from release to main January 22, 2024 09:53
@muellerdemos muellerdemos dismissed mussbach’s stale review January 22, 2024 09:53

The base branch was changed.

@muellerdemos muellerdemos merged commit 99b2bcf into main Jan 22, 2024
5 of 7 checks passed
@muellerdemos muellerdemos deleted the chore_enable_csrf_token_to_be_read_from_request_headers branch January 22, 2024 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review:backend PRs that are missing reviews from a BE perspective
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants