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

Allow modules to override CSRF protection. #1296

Closed
wants to merge 1 commit into from

Conversation

tzvetkoff
Copy link
Contributor

Allow modules to override CSRF protection.
Useful for Web APIs and all other kinds of things.

API changes:
- Added public CHTTPSock::GetURI() method
- Added public CModule::ValidateWebRequestCSRFCheck() method
- Made CWebSock::GetCSRFCheck() method public so it can be accessed
from CModule
- Added public CWebSock::ValidateCSRFCheck() method

Other changes:
- Added a Sample Web API module (modules/samplewebapi.cpp) and a
simple web form with no CSRF check.

Implements feature request #1180.

Useful for Web APIs and all other kinds of things.

API changes:
	- Added public CHTTPSock::GetURI() method
	- Added public CModule::ValidateWebRequestCSRFCheck() method
	- Made CWebSock::GetCSRFCheck() method public so it can be accessed
	  from CModule
	- Added public CWebSock::ValidateCSRFCheck() method

Other changes:
	- Added a Sample Web API module (modules/samplewebapi.cpp) and a
	  simple web form with no CSRF check.

Implements feature request znc#1180.
@coveralls
Copy link

coveralls commented Jul 15, 2016

Coverage Status

Coverage decreased (-0.03%) to 40.058% when pulling eec19f6 on tzvetkoff:module_csrf_override into 56620af on znc:master.

@lol768
Copy link
Contributor

lol768 commented Jul 15, 2016

Cheers @tzvetkoff

@@ -655,8 +655,8 @@ CWebSock::EPageReqResult CWebSock::OnPageRequestInternal(const CString& sURI,
// 1. they obviously know the password,
Copy link
Member

Choose a reason for hiding this comment

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

This comment becomes outdated.

@DarthGandalf
Copy link
Member

DarthGandalf commented Jul 25, 2016

Please add some tests.

I don't quite understand why existing WebRequiresLogin() is not enough. Shouldn't that function be used to disable protection?
I know that currently that doesn't work. I'm talking about the proposed API changes.

@lol768
Copy link
Contributor

lol768 commented Jul 25, 2016

@DarthGandalf

I don't quite understand why existing WebRequiresLogin() is not enough. Shouldn't that function be used to disable protection?

I can still think of use-cases where I want to require cookie-based auth but not require a CSRF token. For example, a JSON-based API used with frontend JavaScript. CSRF isn't necessarily a worry if a Content-Type of application/x-www-form-urlencoded isn't accepted since a form is only going to send data using that content type - and without CORS, the same-origin policy will prevent other-origin requests to the same API.

In this circumstance, requiring a CSRF value to be submitted is just additional hassle.


Let's not conflate authentication and cross-site request forgery tokens. There are cases (e.g. a "contact us" form) where it's perfectly reasonable for a developer to want to apply a CSRF token to prevent other users from being made to submit spam via the contact form from another site. This is completely independent of any form of authentication mechanism.

@DarthGandalf
Copy link
Member

Yes, that's a good example, thanks.

@lol768
Copy link
Contributor

lol768 commented Sep 29, 2016

@DarthGandalf @tzvetkoff I've updated this PR against the latest master, clarified some of the source code comments and added an integration test.

https://github.com/lol768/znc/commits/module_csrf_override

What's the best way forward, can you merge my commits in @tzvetkoff?

@DarthGandalf
Copy link
Member

@lol768 you can create PR yourself if @tzvetkoff doesn't merge your commits to this PR.

@lol768
Copy link
Contributor

lol768 commented Oct 1, 2016

Cheers @DarthGandalf, will do so if the commits aren't added in a few days.

@tzvetkoff If I do end up adding my own PR, I'll split the bounty amount on work done and PayPal you the bigger share of it.

@lol768
Copy link
Contributor

lol768 commented Oct 4, 2016

PR added to #1327

@tzvetkoff tzvetkoff deleted the module_csrf_override branch July 3, 2024 23:07
@tzvetkoff tzvetkoff restored the module_csrf_override branch July 3, 2024 23:07
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.

4 participants