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

Csrf fix #2858

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Csrf fix #2858

wants to merge 6 commits into from

Conversation

muralibasani
Copy link
Contributor

Linked issue

Resolves: #xxxxx

  • In WebSecurity, create a csrf token
  • After login, add the token in header
  • Update frontend Angular with csrf token
  • Update React with csrf token (tbd)

What kind of change does this PR introduce?

  • Bug fix
  • New feature
  • Refactor
  • Docs update
  • CI update

What is the current behavior?

Describe the state of the application before this PR. Illustrations appreciated (videos, gifs, screenshots).

What is the new behavior?

Describe the state of the application after this PR. Illustrations appreciated (videos, gifs, screenshots).

Other information

Additional changes, explanations of the approach taken, unresolved issues, necessary follow ups, etc.

Requirements (all must be checked before review)

  • The pull request title follows our guidelines
  • Tests for the changes have been added (if relevant)
  • The latest changes from the main branch have been pulled
  • pnpm lint has been run successfully

Sorry, something went wrong.

Signed-off-by: Muralidhar Basani <muralidhar.basani@aiven.io>
Signed-off-by: Muralidhar Basani <muralidhar.basani@aiven.io>
Signed-off-by: Mirjam Aulbach <mirjam.aulbach@aiven.io>

const headers = {
...options.headers,
...(csrfToken ? { "X-CSRF-TOKEN": csrfToken } : {}),
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I understand it, usually X-XSRF-TOKEN is enough, but it seems BE expects X-XSRF-TOKEN to be sent, too. There may be a legacy/backwards-compatibility config in BE for this. Angular sends both, so I added it to be consistent (and well, because we need to to work :D)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good one. I missed to update it back.
Now I updated BE to use only X-XSRF-TOKEN. It works in Angular.
Can you pls try removing it and see if it works in react too ?

Copy link
Contributor

Choose a reason for hiding this comment

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

this is my only outstanding question as well aside from that i think it looks good.

@muralibasani muralibasani marked this pull request as ready for review March 28, 2025 15:42
Signed-off-by: Muralidhar Basani <muralidhar.basani@aiven.io>
roope-kar
roope-kar previously approved these changes Mar 31, 2025
Copy link
Contributor

@roope-kar roope-kar left a comment

Choose a reason for hiding this comment

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

coral side looks good to me 👍🏼

Signed-off-by: Muralidhar Basani <muralidhar.basani@aiven.io>
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.

None yet

4 participants