Expands the secure_rel utility to handle elastic domains#1565
Expands the secure_rel utility to handle elastic domains#1565joelgriffith merged 4 commits intomasterfrom
Conversation
| @@ -2,31 +2,37 @@ | |||
| * Secures outbound links. For more info: | |||
| * https://www.jitbit.com/alexblog/256-targetblank---the-most-underestimated-vulnerability-ever/ | |||
| */ | |||
There was a problem hiding this comment.
Interface is expanded here. Uses a single object argument versus trying to add another argument since that felt weird given the optionality of those args. I also tried to consolidate the algo a bit to make it readable, but let me know if that's not the case!
|
I'd love to review, but have no clue what I'm looking at ;) Tagged @chandlerprall in |
src/services/url.ts
Outdated
| @@ -0,0 +1,16 @@ | |||
| const isElasticDomain = /(https?:\/\/(.+?\.)?elastic\.co(\/[A-Za-z0-9\-\._~:\/\?#\[\]@!$&'\(\)\*\+,;\=]*)?)/g; | |||
There was a problem hiding this comment.
I confirmed that this positively matches against all URLs in the elastic.co sitemaps. A bit hesitant that it doesn't match against extended character sets (unicode), but we don't use those in our urls at this time.
chandlerprall
left a comment
There was a problem hiding this comment.
Needs an update to CHANGELOG.md describing the modification.
src/services/url.ts
Outdated
| @@ -0,0 +1,16 @@ | |||
| const isElasticDomain = /(https?:\/\/(.+?\.)?elastic\.co(\/[A-Za-z0-9\-\._~:\/\?#\[\]@!$&'\(\)\*\+,;\=]*)?)/g; | |||
There was a problem hiding this comment.
This does not match if there is a querystring with no path, e.g. https://elastic.co?a=b
There was a problem hiding this comment.
Good catch, addressed!
chandlerprall
left a comment
There was a problem hiding this comment.
Changes LGTM, tested the new function locally
* Expands the getSecureRelForTarget utility to let referrer's pass-through to elastic.co domains.
This PR changes the behavior of the EUILink component to apply rel="noreferrer" universally for all external links, as it's considered a best practice. Prior to this PR, there was an exception for elastic.co domains, added in #1565. As this behavior is no longer required, we're opting to remove the exception.
* [EuiToolTip] Add `repositionOnScroll` prop (#6515) * Add `repositionOnScroll` prop and event listener * [Docs] Add fixed tooltip example + add auto-focus behavior for keyboard users * changelog * Updated changelog. * 72.2.0 * Updated documentation. * [EuiDataGrid] Fix nested interactive controls axe error (#6517) * Fix nested interactive controls error in column visibility control * Restore mouse experience by making switch "label" draggable - this (almost) restores previous draggable functionality - the column name and handle are both mouse draggable, while the switch is not as it is its own interactive thing * Fix nested interactive controls error in column sorting control - the X button and sort button group should not be draggable as they are interactive * Improve mouse experience by making column name draggable - The x button and sort button group are interactive and cannot be draggable, but the column name/token can be + tweak padding to belong to the column name, for extra grabbable area * [opinionated] Fix multiple CSS styles to actually work correctly - whether or not these style changes are wanted will have to be approved by a designer * changelog * Improve Kibana upgrade docs (#6518) - add table of contents - order steps more generally in chronological order so it's easier to follow vs jumping around the page - update recommendations and general context * [Docs] Update `EuiConfirmModal` examples (#6519) * [Docs] Update `EuiConfirmModal` examples * Adding a width * Update src-docs/src/views/modal/confirm_modal.tsx Co-authored-by: florent-leborgne <florent.leborgne@elastic.co> Co-authored-by: florent-leborgne <florent.leborgne@elastic.co> * [EuiToolTip] Enforce only one visible tooltip at a time (#6520) * [misc cleanup] Group relative imports by general concept - keep parent services together, keep tooltip-specific imports together * Add tooltip manager that hides all other tooltips when a new tooltip is shown * Write Cypress E2E tests for multiple tooltip behavior * Changelog * Unit tests for control of internal tooltip visibility state via `ref` (#6522) * [Docs] Fix code snippet (#6526) * Added a11y specs for EuiNotificationEvent, EuiPageHeader, EuiPortal (#6524) * Added a11y specs for EuiNotificationEvent, EuiPageHeader, EuiPortal * Added a11y and keyboard specs for EuiNotificationEvent. * Added page header a11y tests. * Added a11y and keyboard specs for EuiPortal. * Simplifying Portal test, adding breadcrumb and tabs to Page Header. * Simplified two components, added a state check to EuiNotification. * Further reducing the EuiPortal specs to test just that one component. * update i18ntokens * Updated changelog. * 73.0.0 * Updated documentation. * Check that the `upstream` remote is correctly set before proceeding in release script (#6528) * [EuiModalHeaderTitle] Remove automatic detection of title contents in favor of always wrapping a `h1`, configurable via new `component` prop (#6530) * Use a `component` prop for EuiModalHeaderTitle tag wrapper instead of trying to detect child types - this is because Kibana's `<FormattedMessage>` component (which only outputs a string) is incorrectly not getting styles applied to it * Add prop unit tests + convert tests to RTL while we're here * Allow `EuiConfirmModal`s to override title component tag as well if necessary * Changelog * tweak changelog copy a bit more * [Docs EuiTable] Replace all the demos using `data_store` with `faker-js` (#6521) * Replacing `data_store` with `faker-js` * Converted `actions.js` to `tsx` * Converted `auto.js` to `.tsx` * Converted `expanding_rows.js` to `*.tsx` * Converted `footer.js` to `*.tsx` * Converted `mobile.js` to `*.tsx` * Converted `selection.js` to `*.tsx` * Converted `sorting.js` to `*.tsx` * Fix typing/`any` usages - provide correct EuiBasicTable typescript usages/definitions * Clean up divs and Fragments * Simplify & clean up delete selected items code + remove it entirely from `footer` and `expanding_rows` examples - those demos really don't need that behavior at all * Remove unnecessary `renderStatus` abstraction - not being used in multiple places, so no need to DRY it out * Fix incorrectly rendering mobile names + DRY out `renderStatus` - not totally clear to me why those examples have such complex mobile examples + add a `mobileOptions.only` example in the `mobile` demo to make up for removed examples * Remove unused `sortable` - there's no sorting being passed to the table, so it does nothing * Clean up table layout example - remove need for randomly generated group IDs - simplify var names - use value passed directly from `onChange` instead of using a `.find()` - use flex group for button toggles instead of ` `s * Clean up emoji flag logic - move `getEmojiFlag` util to bottom of the file - it's not relevant to anything table-related and shouldn't be the first thing a dev has to read/parse - remove unnecessary extra flag/countryCode() call * Remove unnecessary data length abstraction/logic - if `userLength` isn't being used anywhere else, just hard code it in - no need for a var - remove unnecessary `filteredUsers` logic + remove unnecessary `RIGHT_ALIGNMENT` import * Organize table/component logic by concept - move `columns` out to static init where logical - add headings to sort out manual pagination/sorting logic where it isn't necessarily relevant to the demo (but aids in visual output) - add comment to `findUsers` to more clearly explain what it's doing - remove unnecessary abstractions for longer files * Removed `getEmojiFlag` * Remove lodash `uniqBy` dependency - replace one instance with a `Set` example for uniqueness * Tweak new location column to truncate - this column is now fairly long compared to before - let's restore the previous table visual apperances by truncating * Standardize inconsistent link behavior - it doesn't really make sense to have a last name be a link as opposed to a username - some demos were doing this, some weren't, so standardize the rendering - remove actual external links to github - most of the newly random usernames aren't valid in any case Co-authored-by: Constance Chen <constance.chen.3@gmail.com> * Removed domain check from EuiLink (#6535) This PR changes the behavior of the EUILink component to apply rel="noreferrer" universally for all external links, as it's considered a best practice. Prior to this PR, there was an exception for elastic.co domains, added in #1565. As this behavior is no longer required, we're opting to remove the exception. * [EuiBasicTable] Fix row heights jumping when actions are disabled (#6538) * [EuiBasicTable] Fix row heights jumping when actions are disabled * Update snapshots * changelog * Adding EuiModal callout for H1 rendering. (#6497) * Adding EuiModal callout for H1 rendering. * Updating text to be more concise and clear. * Updating documentation for default H1 wrapper on EuiModalHeaderTitle. * Removing example callout, added explanation to component type definition. * Removed extra H1 tags from Docs examples. * Removed H1 from EUIModal component and a11y specs. * Update checklist template to recommend `@default` jsdoc usage (#6541) * [Emotion] Convert EuiBasicTable (#6539) * [tech debt] convert `useEuiTheme` tests to RTL `renderHook` - which is generally a nicer API than the one I yolo'd * [tech debt] Add more missing unit tests for `useEuiTheme` * [tech debt] write basic unit test for `withEuiTheme` * Add new `RenderWithEuiTheme` render prop util * Convert `tbody` loading styles to Emotion - I opted not to create a top-level component for this due to the very limited styles being applied, and due to HOC/theme access shenanigans * Fix error/empty states not rendering loading styles - by only rendering one `<tbody>`, not multiple * Write basic `loading` test + switch `render` to RTL * [extra] Massive clean up of EuiBasicTable unit tests - switch to RTL totally (shallow was not handling the new render prop well) - DRY out various repeated props - stop use snapshots for every single test - use specific assertions instead. For visual rendering for various prop combos, we should use Storybook - leave snapshots in for two specific render tests - barebones & kitchen sink props * Delete scss files * Add `shouldRenderCustomStyles` test * changelog * Add affordance for reduced motion media query - this matches how EuiProgress behaves + clean up animation shorthand * Add CSS workaround/fix for visual Safari bug - apparently `position: relative` on the parent and not on the `tbody` was a cross-browser fix :( * [EuiResizablePanel] Added tabindex prop to EuiResizablePanel for keyboard accessibility. (#6534) * Added tabindex prop to EuiResizablePanel for keyboard accessibility. * Added unit test and corrected tabIndex type. * Renaming tabindex to tabIndex for consistency in docs. * Renaming test description. Removing changelog. Co-authored-by: Jason Stoltzfus <jastoltz24@gmail.com> Co-authored-by: Elizabet Oliveira <elizabet.oliveira@elastic.co> Co-authored-by: florent-leborgne <florent.leborgne@elastic.co> Co-authored-by: Trevor Pierce <1Copenut@users.noreply.github.com> Co-authored-by: Bree Hall <briannajdhall@gmail.com>
Summary
Hey friends 👋, this is a fun one to fulfill the requirements of elastic/kibana#30846. In short, we want to see where we're getting traffic from in Elastic products so we can better cater to those users. To that end, I've altered
get_secure_rel_for_targetto allow it to be passed anhrefparam. It will check if the domain (elastic.co) is part of the href and omit thenoreferrerattribute.I searched endlessly for a util to get just the parent domain name to no avail. They either didn't work, were clunky, or had some interesting side-effects. Thus, I've added a small utility in the services are for doing checks. It has a good amount of tests, including edgecases, for seeing if a string is a URL with the domain of
elastic.co, including multiple subs.Please let me know if I'm missing anything, this is my first EUI PR!
Checklist
[ ] This was checked in mobile[ ] This was checked in dark mode[ ] Any props added have proper autodocs[ ] Documentation examples were added[ ] This was checked against keyboard-only and screenreader scenarios[ ] This required updates to Framer X components