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

Setting the `api.csrf` option prevents the user from logging in #1944

Closed
hdodov opened this issue Jul 24, 2019 · 8 comments

Comments

@hdodov
Copy link

commented Jul 24, 2019

Describe the bug
I wanted to disable API authentication for development purposes. After looking at the source, I found out I can kind of disable it by using the undocumented api.csrf option. If I set it to null obviously nothing happens, but if I set it to '', I can log in and make requests to the API (from the browser where I have logged in) without a CSRF token.

This works as expected if I specify the option in config.php like this:

'api' => [
    'csrf' => ''
]

But it works weirdly if I specify it like this:

'api.csrf' => ''

By "weirdly" I mean that I can still request the API with no CSRF, but the tab where I've logged in the panel starts refreshing endlessly, redirecting me from /panel/login to /panel/site and back, with the "Unauthenticated" error on both of them. If I delete my session cookie, I can no longer request the API (I get the "Unauthenticated" error there as well), the panel redirection loop stops, I land on the "/panel/login" page, and I can no longer log in, even with the correct credentials.

To Reproduce
Add this in config.php:

return [
    'api.csrf' => ''
];

And do as I've said above.

Expected behavior
Specifying options both ways should work identically.

Kirby Version
3.2.0

Edit: I think this happens due to this line of code. It expects options['api'] to be an array. If that's the case, other options used in the same manner would likely result in unexpected behavior.

Edit 2: Yep, I can confirm. Changing the following in Panel.php from:

'csrf' => $kirby->option('api')['csrf'] ?? csrf(),

to:

'csrf' => $kirby->option('api.csrf') ?? csrf(),

Solves the problem.

@lukasbestle

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

Hm, interesting that we even have that option. I think we should remove it entirely actually as it could be a security risk. Or have I missed anything here @distantnative @bastianallgeier? What was the reason for the option when it was implemented?

@distantnative

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

I think we have it for our development setup. If I recall the FSRF token would not work if the site and API are reached via kirby.test while the Panel runs at localhost:8080.

@hdodov

This comment has been minimized.

Copy link
Author

commented Jul 24, 2019

@lukasbestle I think it's needed. Otherwise how would you make requests to the API from plugins' front-end? Actually, the panel itself uses the API too, so it also needs to provide the token?

Edit: I'm talking about the presence of the CSRF token in window.panel.csrf, which might be a different thing, now that I look at it.

@lukasbestle

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

@distantnative Ah, that makes sense. Then the fix proposed by @hdodov is probably the way to go.

@hdodov Yes, I was only talking about the option in the Kirby config. Right now it defaults to the dynamic CSRF from the session if not set.

@hdodov

This comment has been minimized.

Copy link
Author

commented Jul 24, 2019

@lukasbestle if my fix is the way to go, you should search the codebase for similar issues. $kirby->option('panel')['search']['limit'] is used in that same file.

@bastianallgeier

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

@hdodov's fix is the way to go. We probably have similar issues in other places.

@distantnative

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

I searched all files with \$kirby->option\(.*\)\[ regex, can only find those 2 occurrences. Will fix them.

@distantnative distantnative self-assigned this Aug 23, 2019
distantnative added a commit that referenced this issue Aug 23, 2019
@distantnative distantnative added this to the 3.2.5 milestone Aug 23, 2019
@afbora afbora referenced this issue Aug 28, 2019
1 of 4 tasks complete
bastianallgeier added a commit that referenced this issue Sep 3, 2019
@bastianallgeier

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

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