Skip to content

Commit

Permalink
[Security Solution] Fix missing hash in sync to url (elastic#166847)
Browse files Browse the repository at this point in the history
## Summary

This PR fixes the root cause for
elastic#166686 and
elastic#166774

@angorayc @machadoum

### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

(cherry picked from commit d52c5a1)
  • Loading branch information
lgestc committed Sep 22, 2023
1 parent 1810987 commit e35b279
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 10 deletions.
31 changes: 26 additions & 5 deletions packages/kbn-url-state/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ describe('useSyncToUrl', () => {
window.location = {
...originalLocation,
search: '',
hash: '',
};
window.history = {
...originalHistory,
Expand Down Expand Up @@ -65,9 +66,30 @@ describe('useSyncToUrl', () => {
);
});

it('should should not alter the location hash', () => {
const key = 'testKey';
const valueToSerialize = { test: 'value' };
window.location.hash = '#should_be_there';

const { result } = renderHook(() => useSyncToUrl(key, jest.fn()));

act(() => {
result.current(valueToSerialize);
});

expect(window.history.replaceState).toHaveBeenCalledWith(
{ path: expect.any(String) },
'',
'/#should_be_there?testKey=%28test%3Avalue%29'
);
});

it('should clear the value from the query string on unmount', () => {
const key = 'testKey';

// Location should have a key to clear
window.location.search = `?${key}=${encode({ test: 'value' })}`;

const { unmount } = renderHook(() => useSyncToUrl(key, jest.fn()));

act(() => {
Expand All @@ -85,17 +107,16 @@ describe('useSyncToUrl', () => {
const key = 'testKey';
const restore = jest.fn();

// Location should have a key to clear
window.location.search = `?${key}=${encode({ test: 'value' })}`;

renderHook(() => useSyncToUrl(key, restore, true));

act(() => {
window.dispatchEvent(new Event('popstate'));
});

expect(window.history.replaceState).toHaveBeenCalledTimes(1);
expect(window.history.replaceState).toHaveBeenCalledWith(
{ path: expect.any(String) },
'',
'/?'
);
expect(window.history.replaceState).toHaveBeenCalledWith({ path: expect.any(String) }, '', '/');
});
});
9 changes: 7 additions & 2 deletions packages/kbn-url-state/use_sync_to_url.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,15 @@ export const useSyncToUrl = <TValueToSerialize>(
searchParams.delete(key);
}

const newSearch = searchParams.toString();
const stringifiedSearchParams = searchParams.toString();
const newSearch = stringifiedSearchParams.length > 0 ? `?${stringifiedSearchParams}` : '';

if (window.location.search === newSearch) {
return;
}

// Update query string without unnecessary re-render
const newUrl = `${window.location.pathname}?${newSearch}`;
const newUrl = `${window.location.pathname}${window.location.hash}${newSearch}`;
window.history.replaceState({ path: newUrl }, '', newUrl);
},
[key]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,18 @@ type FlyoutState = Parameters<ExpandableFlyoutApi['openFlyout']>[0];
export const useSyncFlyoutStateWithUrl = () => {
const flyoutApi = useRef<ExpandableFlyoutApi>(null);

const syncStateToUrl = useSyncToUrl<FlyoutState>(FLYOUT_URL_PARAM, (data) => {
flyoutApi.current?.openFlyout(data);
});
const handleRestoreFlyout = useCallback(
(state?: FlyoutState) => {
if (!state) {
return;
}

flyoutApi.current?.openFlyout(state);
},
[flyoutApi]
);

const syncStateToUrl = useSyncToUrl<FlyoutState>(FLYOUT_URL_PARAM, handleRestoreFlyout);

// This should be bound to flyout changed and closed events.
// When flyout is closed, url state is cleared
Expand Down

0 comments on commit e35b279

Please sign in to comment.