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

Use externalUrl service to protect against unsafe URLs #85006

Open
pgayvallet opened this issue Dec 4, 2020 · 2 comments
Open

Use externalUrl service to protect against unsafe URLs #85006

pgayvallet opened this issue Dec 4, 2020 · 2 comments
Labels
discuss impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@pgayvallet
Copy link
Contributor

Once #81234 is merged, we should leverage it in core where and when it is possible. This issue is here to discuss the possible implementations.

From #81234, it was mentioned that we should just add a global link handler to perform this check automatically on every existing links in the DOM.

why I think a global handler is not good enough:

  • It would be executed after the link's potential onClick handler, or any handler in the link's parent tree (as our handler would be attached to the document element, so executed last). For instance, most of our link are either <a href="url" onClick={(e) => { e.preventDefault(); navigateToApp(url)} }/> or <RedirectAppLink><a href="url"/> </RedirectAppLink> . In both these cases, the global handler would be of no use because a deeper handler would be preventing default + triggering the navigation via javascript. Note that we could add a check within navigateToUrl leveraging this new service to address this (which is not done in the PR)

  • More importantly, ctrl-click and right-click -> open in the tab are not executing onClick handlers at all, and just opening the url into a new tab. Meaning that in that case, any javascript based check would just be totally bypassed, which means that we just can't put any user-inputed url in the DOM at all. We could introduce a new redirect 'app', that would basically be /redirect-if-safe?url={url} and internally performing the url validity check before redirecting to the eternal link, but in that case, applications displaying links would still need to replace all < a href="{url}"/> to <a href={basePath.prepend(/redirect?url=${url})"/>

In short: to address this globally, I think we would need to:

  • add an url validity check to application.navigateToUrl to block navigation to urls considered unsafe (redirect to a new 'you are trying to access an unsafe url' page instead)
  • expose a new core API (probably from this new externalUrl service) for plugins to use to 'sanitize' their urls. A link with a user-inputted url would then looks like: <a href={core.externalUrl.sanitize(potentiallyUnsafeUrl)} /> . externalUrl.sanitize could either rewrite the url to {basePath}/redirect-if-safe?url={url} or even leave the url unchanged if considered safe, or change it to redirect to the new you are trying to access an unsafe url page if considered unsafe.
  • we would still need all applications that are displaying user-inputted links to use this new externalUrl.sanitize API anywhere they are displaying potentially unsafe links.
@pgayvallet pgayvallet added discuss Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! labels Dec 4, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

@exalate-issue-sync exalate-issue-sync bot added impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort labels Aug 5, 2021
@legrego legrego removed the Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! label May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
Development

No branches or pull requests

3 participants