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

[server/xsrf] use the current version as the xsrf header #5587

Merged
merged 7 commits into from
Dec 9, 2015

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Dec 7, 2015

Rather than a random value, send the current kibana version to the browser and require the browser send it back in the kbn-version header in every non-GET request. Otherwise, this is functionally identical to the way that XSRF used to work, except for two things:

  1. sending the wrong version in a GET request will cause that request to be rejected
  2. failures are now 400 errors, since this doesn't really have to do with authorization any more

Fixes #5574
Fixes #5594

@epixa
Copy link
Contributor

epixa commented Dec 7, 2015

Just so there isn't any confusion, can you update the content of the linked issue to reflect these changes? It's currently specifying refresh behaviors and such, which are understandably not a part of this first phase.

@spalger
Copy link
Contributor Author

spalger commented Dec 8, 2015

Good catch @epixa, done

@@ -114,7 +113,7 @@ describe('chrome xsrf apis', function () {
});

it('accepts alternate tokens to use', function () {
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 no longer a thing, correct? There's no custom token stuff going on behind the scenes since we're relying on version. I'm actually mildly concerned that this didn't fail outright...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corret... now why is this test passing :/

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I know why: this test was never really testing anything useful. It's just verifying that $http sends custom headers, which the actual tests for $http should verify for us. I think you can safely remove this whole test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, sorry, this is still a thing, but it probably shouldn't be. If the user sends a custom value it will be used as the kbn-version header. This I'll change this value to be boolean-y

@epixa
Copy link
Contributor

epixa commented Dec 9, 2015

Is it possible to make the csrf failures more friendly? At the moment, a version mismatch results in kibana crashing with a fatal error that simply says "Bad Request". Ideally, the error would instead instead just slide down from the top and would at least say something along the lines of "Kibana client version is out of date, refresh required."

@spalger
Copy link
Contributor Author

spalger commented Dec 9, 2015

I'll give it a shot

@epixa
Copy link
Contributor

epixa commented Dec 9, 2015

LGTM

@epixa epixa assigned Bargs and unassigned w33ble Dec 9, 2015
@Bargs
Copy link
Contributor

Bargs commented Dec 9, 2015

LGTM

Dunno if it was brought up before, but one thing to note about this solution (that applied to the previous random value as well) is that vanilla form posts won't work. It's fine for us since we're using angular for everything, but it could be frustrating for plugin developers in the future. Just something to think about.

@Bargs Bargs assigned spalger and unassigned Bargs Dec 9, 2015
@spalger spalger added the v4.5.0 label Dec 9, 2015
spalger added a commit that referenced this pull request Dec 9, 2015
[server/xsrf] use the current version as the xsrf header
@spalger spalger merged commit 9fe99e5 into elastic:master Dec 9, 2015
@epixa epixa removed the v4.5.0 label Dec 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants