Respect external URL allow list in TSVB #114093

DianaDerevyankina · 2021-10-06T12:21:50Z

Summary

Add ExternalUrl service validation to Top N and Table url drilldowns.
In case of access denied modal with error message is shown.

externalUrl.policy:
  - allow: false
    host: danger.google.com

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials

For maintainers

This was checked for breaking API changes and was labeled appropriately

DianaDerevyankina · 2021-10-06T12:57:06Z

@KOTungseth could you please validate the modal message?
I guess it would be great to have some link to documentation in this text, but it seems there's no specific page in docs for ExternalUrl service. Here's the URL drilldown settings docs, but as it is planned to implement that validation in other services, I believe it would be helpful to have a general article about it, as well as mention about externalUrl.policy in kibana.yml docs. Wdyt?

elasticmachine · 2021-10-14T14:06:46Z

Pinging @elastic/kibana-vis-editors (Team:VisEditors)

stratoula · 2021-10-18T07:15:42Z

@elasticmachine merge upstream

src/plugins/vis_types/timeseries/public/application/components/vis_types/top_n/vis.js

stratoula · 2021-10-18T09:17:32Z

@elastic/kibana-docs can we have a review on the modal message? ❤️

stratoula

This seems to work fine but when I have set up an external url policy for an internal link this doesn't work.
For example

This is a very common use case, users are creating drilldowns to navigate to a dashboard. If I set up an external url policy this is not allowed. I think that it should be allowed as it is an internal url.

Another thing that it seems a bit odd to me is that if I have blackisted a url (let's say the google.com), when I click on TSVB table or topN the modal appears. This is correct. But when I right click to open in new tab is allowed. This should not be allowed in right click too.

gchaps · 2021-10-18T16:02:22Z

I'm wondering if we can write this without the word "denied" so it doesn't sound so scary and remove the reference to the ExternalURL service. It would be better to put the URL at the end as it can be long. Here is a suggestion:

Access to this URL is not yet enabled
Configure externalUrl.policy in your kibana.yml to allow access to xxxx.

or put "external" in the title:

Access to this external URL is not yet enabled
Configure externalUrl.policy in your kibana.yml to allow access to xxxx.

stratoula · 2021-10-19T05:59:05Z

@elasticmachine merge upstream

stratoula · 2021-10-19T07:39:22Z

@elastic/kibana-core we need your help! We are trying to use the externalUrlService to validate URLs against an explicit allow/deny list in kibana.yml. These urls are used as drilldowns in TSVB. One very common case for the users (it also exists in the documentation) is to add as a drilldown an internal url to navigate to a dashboard. For example: dashboards#/view/<dashboard_id>. The service validates it as an external url and not as an internal one, so it is denied by the policy while it shouldn't.

Note: This doesn't happen when we are testing it via the functional test server, so we guess that it has to do with the 3 letters prefix (localhost:5601/xyz).

Maybe it is fine as it is, and it happens only locally but can you check it and let us know?

flash1293 · 2021-10-19T07:47:52Z

Do we even have to call the api for these urls? Or could we detect relative urls and allow them all the time? Also, is this a problem for vega as well?

stratoula · 2021-10-19T07:59:32Z

@flash1293 yes it is also denied by vega

flash1293 · 2021-10-19T08:07:12Z

Got it, thanks. We definitely need some mechanism to check between internal and external urls - the question is should the core service handle this or something on the consumer side? I guess it would have to be something like this:

Parse url
If host is defined, this is an external url
If path is defined and it doesn't start with the configured base path, this is an external url
Otherwise it's an internal URL and always safe to use

Edit: Just checked the code and it seems like there is some logic in there already along those lines:

kibana/src/core/public/http/external_url_service.ts

Line 63 in 4584a8b

url.origin === base.origin &&

I guess it doesn't cover the cases with relative paths and just hashes from above? cc @legrego as it seems like you wrote that code originally.

pgayvallet · 2021-10-19T08:10:03Z

@elastic/kibana-core we need your help! We are trying to use the externalUrlService to validate URLs against an explicit allow/deny list in kibana.yml. These urls are used as drilldowns in TSVB

@legrego was actually the one that implemented this service, so he may be more suited to answer, but:

From a quick look at the code, when we're checking if the url is an internal one, we're checking both the url's origin and that it starts with the configured basePath. So when a basePath is configured, an uri like dashboards#/view/<dashboard_id> will definitely not match, as it should be {basePath}/dashboards#/view/<dashboard_id>

kibana/src/core/public/http/external_url_service.ts

Lines 58 to 68 in 4584a8b

    
           const base = new URL(location.origin + serverBasePath); 
        
           return function validateExternalUrl(next: string) { 
        
             const url = new URL(next, base); 
        
             const isInternalURL = 
        
               url.origin === base.origin && 
        
               (!serverBasePath || url.pathname.startsWith(`${serverBasePath}/`)); 
        
             if (isInternalURL) { 
        
               return url; 
        
             }

Could it be that you're missing a basePath.prepend() call somewhere before calling the url validation service?

stratoula · 2021-10-19T08:14:11Z

@pgayvallet yes I think that we are just passing the url as it is added by the user. We are not prepending anything (@dziyanadzeraviankina correct me if I am wrong)

So maybe for the validation to work correctly we could identify if this is an internal link and create the validation url as it is proposed by Pierre {basePath}/dashboards#/view/<dashboard_id>

@flash1293 wdyt?

flash1293 · 2021-10-19T08:15:33Z

So maybe for the validation to work correctly we could identify if this is an internal link and create the validation url as it is proposed by Pierre

This is not the case we are looking at here - dashboard#/view/... is not missing the base path, it's a relative path. The full path would be {basePath}/app/dashboards.

We can definitely resolve this case on the caller side, but it seems like the wrong place to do it as all consumers would have this issue and the core service is already checking for internal urls, it's just missing this case.

stratoula · 2021-10-19T08:17:16Z

Yes I totally agree with you. I just had in mind the vega PR that is going to be released in 7.16 with this behavior. Not sure if it is used a lot though, but still.

flash1293 · 2021-10-19T08:17:58Z

Yeah, we should definitely fix this bug in 7.16.

I'm not sure about the origin part, but this line

(!serverBasePath || url.pathname.startsWith(`${serverBasePath}/`));

Is wrongly assuming url.pathname is an absolute path.

Let's wait for Larry to chime in.

pgayvallet · 2021-10-19T08:23:41Z

This is not the case we are looking at here - dashboard#/view/... is not missing the base path, it's a relative path. The full path would be {basePath}/app/dashboards.

Technically that's not even a relative path. A relative path would be ./dashboard.... This is a 'full' URL following the href specification (that says that all non-specifed url parts fallback to the equivalent document's current location's part). EDIT: I'm, wrong, it is.

Now, the url validation service does not support them atm, at it would require to either have it access document.location (which may be a bad idea for a service as it introduces env coupling, not sure), or to add an additional (optional?) currentLocation parameter to the validation API.

Let's wait for Larry to chime in.

++

flash1293 · 2021-10-19T08:26:57Z

Looks like it's already coupled to window.location via

kibana/src/core/public/http/external_url_service.ts

Line 58 in 4584a8b

const base = new URL(location.origin + serverBasePath);

kibana/src/core/public/http/external_url_service.ts

Line 93 in 4584a8b

validateUrl: createExternalUrlValidation(policy, location, serverBasePath),

kibana/src/core/public/http/http_service.ts

Line 44 in 4584a8b

    
           externalUrl: new ExternalUrlService().setup({ injectedMetadata, location: window.location }),

pgayvallet · 2021-10-19T08:33:51Z

Looks like it's already coupled to window.location via

Damn, it's not even that early in the morning anymore, so I don't have any excuses.

You're right, we're already depending on window.location. I still don't really like the idea of validation relative links TBH, as the validation results depends on the user's current location, which makes it, in a way, stateful. But let's wait to see what @legrego have to say about it.

flash1293 · 2021-10-19T08:40:32Z

I still don't really like the idea of validation relative links TBH, as the validation results depends on the user's current location, which makes it, in a way, stateful

I'm with you here, but this is a real example, people use it and it worked before - we fixed bugs around this in other places doing URL handling.

pgayvallet · 2021-10-19T08:43:40Z

I'm with you here, but this is a real example, people use it and it worked before

Sorry, but before what? We did not touch the url validation service since its initial implementation

flash1293 · 2021-10-19T08:51:10Z

Sorry, but before what? We did not touch the url validation service since its initial implementation

Sorry, I meant these urls used to work before the url validation service was introduced to Vega and TSVB.

flash1293 · 2021-10-19T08:56:51Z

@stratoula made me aware we are actually using relative paths in the documentation:

https://www.elastic.co/guide/en/kibana/current/tsvb.html

stratoula

Changes LGTM. This works fine for topN and table.
The only thing missing is that if you have defined an external url policy, the relative paths do not work. This is a quite common case as it is also mentioned in our docs.

We discussed it offline and we think that this is something that should be fixed (as it will also fail in vega), but as it is not introduced by this PR and we think that is a case that should be handled from the external url service, I am approving it. cc @flash1293

kibanamachine · 2021-10-19T10:17:02Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`visTypeTimeseries`	303	304	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`visTypeTimeseries`	485.8KB	488.5KB	+2.7KB

History

💚 Build #162506 succeeded 2c8a202
💔 Build #162093 failed e57644e
💔 Build #161877 failed 31bf80a
💚 Build #158418 succeeded 201f44a

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @dziyanadzeraviankina

flash1293 · 2021-10-19T10:21:22Z

Sounds good - however we decide to run this, we will have to do the same thing for Vega and TSVB.

flash1293 · 2021-10-19T10:41:44Z

It seems like the “resolve” helper https://www.npmjs.com/package/url#urlresolvefrom-to is exactly what we need - resolve the provided url in the current context to get a fully qualified url, then check the allow list against that.

Take a base URL, and a href URL, and resolve them as a browser would for an anchor tag

legrego · 2021-10-19T11:18:47Z

From a quick look at the code, when we're checking if the url is an internal one, we're checking both the url's origin and that it starts with the configured basePath. So when a basePath is configured, an uri like dashboards#/view/<dashboard_id> will definitely not match, as it should be {basePath}/dashboards#/view/<dashboard_id>

The intent of the validation service is to work with either internal or external URLs. Internal URLs can be either fully or partially qualified (i.e. /foo/app/dashboards... or https://localhost:5601/foo/app/dashboards...). The test suite for the service does a decent job of outlining what we want to support from this service:
https://github.com/elastic/kibana/blob/4584a8b570402aa07832cf3e5b520e5d2cfa7166/src/core/public/http/external_url_service.test.ts

If it's a partially qualified location, then it must start with the configured server.basePath.

When this was first written, we didn't have a way to determine what the public facing URL for Kibana, so we resorted to using window.location. Now that we have a dedicated configuration setting for this property, we could update this validation service to take advantage of that.

alexwizp · 2021-10-19T11:41:29Z

src/plugins/vis_types/timeseries/public/application/components/vis_types/table/vis.js

+        <a
+          href={sanitizeUrl(url)}
+          onClick={handleDrilldownUrlClick}
+          onContextMenu={handleDrilldownUrlClick}


I am not a fan of this kind of code, especially when in a similar place for top n it is handled differently ... but ok ... I will not block this pr for this reason

alexwizp · 2021-10-19T11:43:11Z

src/plugins/vis_types/timeseries/public/application/components/vis_types/table/vis.js

-      rowDisplay = <a href={sanitizeUrl(url)}>{rowDisplay}</a>;
+      const handleDrilldownUrlClick = this.createDrilldownUrlClickHandler(url);
+      rowDisplay = (
+        <a


Just wondering if we should use EuiLink in such cases?

* Respect external URL allow list in TSVB * Remove showExternalUrlErrorModal and onContextMenu handler for table * Update modal message Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* Respect external URL allow list in TSVB * Remove showExternalUrlErrorModal and onContextMenu handler for table * Update modal message Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Respect external URL allow list in TSVB

201f44a

DianaDerevyankina added release_note:enhancement Feature:TSVB TSVB (Time Series Visual Builder) Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 v7.16.0 labels Oct 6, 2021

DianaDerevyankina requested review from VladLasitsa and alexwizp October 6, 2021 12:21

DianaDerevyankina self-assigned this Oct 6, 2021

alexwizp marked this pull request as ready for review October 14, 2021 14:06

alexwizp requested a review from a team as a code owner October 14, 2021 14:06

alexwizp requested a review from KOTungseth October 14, 2021 14:06

Merge branch 'master' into issues/113094

31bf80a

alexwizp reviewed Oct 18, 2021

View reviewed changes

src/plugins/vis_types/timeseries/public/application/components/vis_types/top_n/vis.js Show resolved Hide resolved

stratoula requested a review from a team October 18, 2021 09:17

stratoula reviewed Oct 18, 2021

View reviewed changes

DianaDerevyankina added 2 commits October 18, 2021 17:18

Remove showExternalUrlErrorModal and onContextMenu handler for table

c29f229

Merge branch 'master' into issues/113094

e57644e

Merge branch 'master' into issues/113094

2c8a202

Update modal message

4c41b52

stratoula approved these changes Oct 19, 2021

View reviewed changes

alexwizp reviewed Oct 19, 2021

View reviewed changes

alexwizp approved these changes Oct 19, 2021

View reviewed changes

DianaDerevyankina merged commit 0e5f252 into elastic:master Oct 19, 2021

DianaDerevyankina mentioned this pull request Oct 19, 2021

[7.x] Respect external URL allow list in TSVB (#114093) #115550

Merged

flash1293 mentioned this pull request Oct 19, 2021

External url service is always treating relative path URLs without protocol and host name as external #115561

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect external URL allow list in TSVB #114093

Respect external URL allow list in TSVB #114093

DianaDerevyankina commented Oct 6, 2021 •

edited

Loading

DianaDerevyankina commented Oct 6, 2021

elasticmachine commented Oct 14, 2021

stratoula commented Oct 18, 2021

stratoula commented Oct 18, 2021

stratoula left a comment •

edited

Loading

gchaps commented Oct 18, 2021 •

edited

Loading

stratoula commented Oct 19, 2021

stratoula commented Oct 19, 2021

flash1293 commented Oct 19, 2021

stratoula commented Oct 19, 2021

flash1293 commented Oct 19, 2021 •

edited

Loading

pgayvallet commented Oct 19, 2021 •

edited

Loading

stratoula commented Oct 19, 2021 •

edited

Loading

flash1293 commented Oct 19, 2021 •

edited

Loading

stratoula commented Oct 19, 2021

flash1293 commented Oct 19, 2021 •

edited

Loading

pgayvallet commented Oct 19, 2021 •

edited

Loading

flash1293 commented Oct 19, 2021

pgayvallet commented Oct 19, 2021

flash1293 commented Oct 19, 2021

pgayvallet commented Oct 19, 2021 •

edited

Loading

flash1293 commented Oct 19, 2021

flash1293 commented Oct 19, 2021

stratoula left a comment

kibanamachine commented Oct 19, 2021

flash1293 commented Oct 19, 2021

flash1293 commented Oct 19, 2021 •

edited

Loading

legrego commented Oct 19, 2021

alexwizp Oct 19, 2021

alexwizp Oct 19, 2021

Respect external URL allow list in TSVB #114093

Respect external URL allow list in TSVB #114093

Conversation

DianaDerevyankina commented Oct 6, 2021 • edited Loading

Summary

Checklist

For maintainers

DianaDerevyankina commented Oct 6, 2021

elasticmachine commented Oct 14, 2021

stratoula commented Oct 18, 2021

stratoula commented Oct 18, 2021

stratoula left a comment • edited Loading

Choose a reason for hiding this comment

gchaps commented Oct 18, 2021 • edited Loading

stratoula commented Oct 19, 2021

stratoula commented Oct 19, 2021

flash1293 commented Oct 19, 2021

stratoula commented Oct 19, 2021

flash1293 commented Oct 19, 2021 • edited Loading

pgayvallet commented Oct 19, 2021 • edited Loading

stratoula commented Oct 19, 2021 • edited Loading

flash1293 commented Oct 19, 2021 • edited Loading

stratoula commented Oct 19, 2021

flash1293 commented Oct 19, 2021 • edited Loading

pgayvallet commented Oct 19, 2021 • edited Loading

flash1293 commented Oct 19, 2021

pgayvallet commented Oct 19, 2021

flash1293 commented Oct 19, 2021

pgayvallet commented Oct 19, 2021 • edited Loading

flash1293 commented Oct 19, 2021

flash1293 commented Oct 19, 2021

stratoula left a comment

Choose a reason for hiding this comment

kibanamachine commented Oct 19, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

flash1293 commented Oct 19, 2021

flash1293 commented Oct 19, 2021 • edited Loading

legrego commented Oct 19, 2021

alexwizp Oct 19, 2021

Choose a reason for hiding this comment

alexwizp Oct 19, 2021

Choose a reason for hiding this comment

DianaDerevyankina commented Oct 6, 2021 •

edited

Loading

stratoula left a comment •

edited

Loading

gchaps commented Oct 18, 2021 •

edited

Loading

flash1293 commented Oct 19, 2021 •

edited

Loading

pgayvallet commented Oct 19, 2021 •

edited

Loading

stratoula commented Oct 19, 2021 •

edited

Loading

flash1293 commented Oct 19, 2021 •

edited

Loading

flash1293 commented Oct 19, 2021 •

edited

Loading

pgayvallet commented Oct 19, 2021 •

edited

Loading

pgayvallet commented Oct 19, 2021 •

edited

Loading

flash1293 commented Oct 19, 2021 •

edited

Loading