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 creating access tokens without passing a CSRF token #1925

Conversation

@clarkwinkelmann
Copy link
Contributor

clarkwinkelmann commented Oct 29, 2019

Also allows extension to register their own CSRF ignored paths

Fixes #1905

Changes proposed in this pull request:
Fixes the linked issue and makes it possible for extension to register their own ignored paths.

Reviewers should focus on:
Do we want to expose the middleware itself through a singleton or should we create a new separate class to contain the ignored paths ?

Confirmed

  • Backend changes: tests are green (run composer test).
Also allows extension to register their own CSRF ignored paths
@tobyzerner

This comment has been minimized.

Copy link
Contributor

tobyzerner commented Oct 29, 2019

My initial thought: Why is the CSRF token checked at all for any API route? REST APIs are meant to be stateless, CSRF token is a stateful concept.

(Also, hi 👋)

@clarkwinkelmann

This comment has been minimized.

Copy link
Contributor Author

clarkwinkelmann commented Oct 29, 2019

Hi Toby 👋

Well... we allow cookie auth for the API (that's our primary way of storing session), so we have to check CSRF.

But actually... if no cookie session is active (no cookie was passed), we could skip the CSRF check completely. Thoughts on that ? 🤔

@datitisev

This comment has been minimized.

Copy link
Member

datitisev commented Oct 29, 2019

That makes sense... CSRF check fails without a cookie anyway, which kinda makes the API a bit pointless if you still need an CSRF token.

Not sure if we discussed this before and decided to stick with CSRF tokens for all API calls, though.

@tobyzerner

This comment has been minimized.

Copy link
Contributor

tobyzerner commented Oct 29, 2019

But actually... if no cookie session is active (no cookie was passed), we could skip the CSRF check completely.

Exactly!

@flarum flarum deleted a comment from cmcjacob Oct 31, 2019
@flarum flarum deleted a comment from cmcjacob Oct 31, 2019
@franzliedke

This comment has been minimized.

Copy link
Member

franzliedke commented Oct 31, 2019

I tend to agree with Toby. So does this StackOverflow question / answer.

Technically speaking, even cookie-authenticated API access shouldn't be a problem, because normal HTTP forms (which would transfer cookie information) can not submit JSON payloads. AJAX requests could, but they wouldn't have the cookies in a cross-site scenario.

Am I missing something?

@clarkwinkelmann

This comment has been minimized.

Copy link
Contributor Author

clarkwinkelmann commented Oct 31, 2019

@franzliedke I think all our API endpoints accept classic POST data as well. But the only place we use that feature is when sending image data.

I actually tested the /token endpoint locally by sending POST data via curl -d, which worked fine.

So if we wanted to drop CSRF from those endpoints completely, we'd have to block the ability to pass classic POST data and only allow JSON. But we would still be left with the avatar endpoints and similar others that have to use classic POST.

I can start another PR where we only check CSRF when acting on cookie-based session as discussed above.

@luceos

This comment has been minimized.

Copy link
Member

luceos commented Nov 11, 2019

From what I can see, the CreateTokenController is now able to generate a token for any user, even those that haven't confirmed their email account. Opening this up as such would potentially increase the number of spam accounts.

I have no objection against dropping or keeping those csrf checks, but perhaps we can allow them on specific endpoints only for validated users if no csrf is available?

@clarkwinkelmann

This comment has been minimized.

Copy link
Contributor Author

clarkwinkelmann commented Nov 11, 2019

What would be the problem ? You're still limited by the same permissions. If a non-confirmed user creates a token, they will get permission denied on all actions until they validate their account via email.

EDIT: by the way this changes nothing from the present state. Having CSRF or not is not linked to account activation. Non activated users can already create tokens, they just need to send the CSRF token like other users.

I'm in favor of simply dropping CSRF checks for all non-cookie requests as discussed above, so we could close this and I'll create another PR.

@luceos

This comment has been minimized.

Copy link
Member

luceos commented Nov 15, 2019

Non activated users can already create tokens, they just need to send the CSRF token like other users.

Okay then I have no object against dropping those CSRF check either.

@luceos
luceos approved these changes Nov 15, 2019
Copy link
Member

luceos left a comment

Okay understood, see my comment for a minor optimization.

$this->app->singleton('flarum.api.csrf-middleware', function () {
$checkCsrf = $this->app->make(HttpMiddleware\CheckCsrfToken::class);

$checkCsrf->ignorePath('/token');

return $checkCsrf;
});
Comment on lines +50 to +56

This comment has been minimized.

Copy link
@luceos

luceos Nov 15, 2019

Member

Now that you are binding those middlewares explicitly per frontend, we could make your code shorter by setting the ignorePath method when the binding is resolved like so:

        $this->app->resolving('flarum.api.csrf-middleware', function (HttpMiddleware\CheckCsrfToken $check) {
            $checkCsrf->ignorePath('/token');
        });

Untested +)

@clarkwinkelmann

This comment has been minimized.

Copy link
Contributor Author

clarkwinkelmann commented Nov 19, 2019

I'm closing this PR, I'll create another PR dropping CSRF for non-cookie requests.

Now... do we still need a way for extensions to register excluded paths with CSRF ? I've seen a single person asking for that on the forum, and I suspect it wasn't the right solution for them. I can't think of any situation where it makes sense and doesn't create a security issue. So it's probably best to not even allow it. Which means completely abandoning this PR.

@andreasjacobsen93

This comment has been minimized.

Copy link

andreasjacobsen93 commented Nov 20, 2019

We actually depend on registering excluded paths from CSRF check, on a return URL from a custom sign-on provider. So I'm a little disappointed to see the PR closed without merge ;)

@clarkwinkelmann

This comment has been minimized.

Copy link
Contributor Author

clarkwinkelmann commented Nov 20, 2019

@andreasjacobsen93 isn't the return url a GET ?

@andreasjacobsen93

This comment has been minimized.

Copy link

andreasjacobsen93 commented Nov 21, 2019

No, it's a POST request and we don't have a CSRF token since we come from the provider. We have other means to verify the user based on metadata from the provider, so it should not be a problem to bypass the CSRF check on that specific route.

We tried registering a middleware that set the CSRF bypass value, but it seems it couldn't be registered before the CSRF check.

So it would be nice to be able to register a specific route to be ignored by CSRF check.

@luceos

This comment has been minimized.

Copy link
Member

luceos commented Nov 21, 2019

@andreasjacobsen93 can you please create a new issue and reference this PR. I have a feeling this might be lost otherwise and that's not something we (both) want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.