Skip to content

Fix CSRF token leakage in SAML POST binding template#241

Merged
Zogoo merged 2 commits intosaml-idp:masterfrom
tyage:fix/csrf-token-leak
Feb 2, 2026
Merged

Fix CSRF token leakage in SAML POST binding template#241
Zogoo merged 2 commits intosaml-idp:masterfrom
tyage:fix/csrf-token-leak

Conversation

@tyage
Copy link
Contributor

@tyage tyage commented Dec 26, 2025

This PR replaces form_tag with a plain HTML <form> tag in spec/rails_app/app/views/saml_idp/idp/saml_post.html.erb.

While this template is part of the spec Rails app, it is often used as a reference for actual implementation. Using form_tag automatically includes an authenticity token, which is unnecessary for SAML POST binding and results in leaking the IdP's CSRF token to Service Providers.

This change ensures that only the required SAML parameters are sent to the SP.

@tyage
Copy link
Contributor Author

tyage commented Dec 26, 2025

I noticed that the code example in the wiki contains the same issue.
I also recommend updating the wiki example:

https://github.com/saml-idp/saml_idp/wiki/Rails_Integration#create-view-for-post-request-saml-response

@Zogoo
Copy link
Collaborator

Zogoo commented Jan 5, 2026

The Saml IdP gem allows an IdP application to use its own template by overwriting the default one.
I think you can do this in your app.

@tyage
Copy link
Contributor Author

tyage commented Jan 6, 2026

While this should be handled at the application level, the wiki and this template are often copied as-is. Using form_tag implicitly includes an authenticity token, which then gets posted to the SP. Since this is unnecessary for SAML POST binding, adjusting the examples would provide a safer reference.

@Zogoo Zogoo self-assigned this Feb 2, 2026
Copy link
Collaborator

@Zogoo Zogoo left a comment

Choose a reason for hiding this comment

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

LGMT

@Zogoo Zogoo merged commit 4dde1a3 into saml-idp:master Feb 2, 2026
42 checks passed
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