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

Gracefully handle query string global state parameter with malformed rison content. #12376

Merged

Conversation

Projects
None yet
3 participants
@azasypkin
Copy link
Member

commented Jun 16, 2017

If the user tries to open Kibana page with the URL that contains global state parameter with malformed content, Kibana ends up in a half-initialized state (e.g. copy-paste issue). For example open https://localhost:5601/app/kibana#/visualize?_g=alert(1). We should display Unable to parse URL and fallback to the default state instead.

The issue is that we get Rison unhandled exception during initialization of the main chrome controller (inside onRouteChange) handler.

cc @epixa

return rison.decode(queryParam);
} catch(err) {
return null;
}

This comment has been minimized.

Copy link
@azasypkin

azasypkin Jun 16, 2017

Author Member

question: I didn't add try/catch for JSON.parse below as I'm not sure if we can have malformed content in the session storage. To me it's something that is hard to do coincidentally. But what do you think?

This comment has been minimized.

Copy link
@spalger

spalger Jun 19, 2017

Member

I would rather bubble the errors if the session storage is getting corrupted somehow

@@ -259,6 +259,22 @@ describe('State Management', function () {
expect(fatalStub.firstCall.args[0]).to.be.an(Error);
expect(fatalStub.firstCall.args[0].message).to.match(/github\.com/);
});

it('translateHashToRison should fallback to null if parameter can not be parsed', () => {

This comment has been minimized.

Copy link
@azasypkin

azasypkin Jun 16, 2017

Author Member

It's the method that's as much close to changed code and also involved into the issue described in PR.

@@ -205,7 +205,11 @@ export function StateProvider(Private, $rootScope, $location, config, kbnUrl) {
*/
State.prototype._parseQueryParamValue = function (queryParam) {
if (!isStateHash(queryParam)) {

This comment has been minimized.

Copy link
@azasypkin

azasypkin Jun 16, 2017

Author Member

question: There is probably another way to fix this issue: just remove this decode code from here entirely. I've noticed that it's used only in two places:

  1. https://github.com/elastic/kibana/blob/master/src/ui/public/state_management/state.js#L73-L75. Here isStateHash check is performed outside of this method, so decode part is not used;
  2. https://github.com/elastic/kibana/blob/master/src/ui/public/state_management/state.js#L227. And here for non-hash case we do decode->encode that seems unnecessary.

So if there is no any non-obvious value in decode->encode cycle, we can probably just add isStateHash check inside 2. and remove decode part from this method.

How does that sound?

This comment has been minimized.

Copy link
@spalger

spalger Jun 19, 2017

Member

I think I agree, I suggest moving to that and we can take another look at it then

@azasypkin

This comment has been minimized.

Copy link
Member Author

commented Jun 16, 2017

Jenkins, test this

@azasypkin azasypkin requested a review from spalger Jun 16, 2017

@epixa epixa self-requested a review Jun 16, 2017

@spalger
Copy link
Member

left a comment

I'd like to see the change suggested in https://github.com/elastic/kibana/pull/12376/files#r122457824

@azasypkin azasypkin force-pushed the azasypkin:issue-xxx-bad-query-string-state branch from 9490d0d to 78c99ae Jun 20, 2017

@azasypkin

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2017

Okay it's ready for review @spalger and @epixa

@epixa

This comment has been minimized.

Copy link
Member

commented Jun 20, 2017

jenkins, test this

@spalger
Copy link
Member

left a comment

LGTM

@epixa

This comment has been minimized.

Copy link
Member

commented Jun 27, 2017

Did either of you try this with X-Pack? I'm getting infinite security redirects just trying to access Kibana with this PR. Happens in incognito as well.

@epixa

This comment has been minimized.

Copy link
Member

commented Jun 27, 2017

Whoops, this one was my fault. No default passwords :)

@epixa

epixa approved these changes Jun 27, 2017

Copy link
Member

left a comment

LGTM

@azasypkin azasypkin force-pushed the azasypkin:issue-xxx-bad-query-string-state branch from 78c99ae to 7788d94 Jun 27, 2017

@azasypkin azasypkin merged commit 15c7453 into elastic:master Jun 27, 2017

1 check passed

kibana-ci Build finished.
Details

@azasypkin azasypkin deleted the azasypkin:issue-xxx-bad-query-string-state branch Jun 27, 2017

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.