Skip to content

[HTTP] Safer client calls and new browser buildPath utility#257230

Merged
jloleysens merged 32 commits into
elastic:mainfrom
jloleysens:feature/http-browser-build-path-utility
Apr 3, 2026
Merged

[HTTP] Safer client calls and new browser buildPath utility#257230
jloleysens merged 32 commits into
elastic:mainfrom
jloleysens:feature/http-browser-build-path-utility

Conversation

@jloleysens
Copy link
Copy Markdown
Contributor

@jloleysens jloleysens commented Mar 11, 2026

  • Create a new best-effort ESLint rule that checks if http<method> calls are used dangerously: direct path injection
  • Adds a new buildPath utility that can be used with server-side routes /api/myapi/{id} to safely build and encode path parameters (bonus: server-side path consts can be reused by the client directly, no need to build these separately by hand)
  • Updates existing usages

No unsafe http path usage

Will flag usages of http like:

core.http.delete(`/api/myapi/${id}`, {...});

With a message to use buildPath or encodeURIComponent in order to safely encode parameters.

buildPath

Not strictly needed in this PR, this utility allows for using server side paths like /api/myapi/{id} in a parameterised fashion like:

import { buildPath } from '@kbn/core-http-browser';

buildPath('/api/dashboard/{id}' /* same as { path: ... } server side */, { id })
// => /api/dashboard/encoded-id

Happy to exclude this utility if it simplifies things.

(Made with cursor y'all)

@jloleysens jloleysens requested review from a team as code owners March 11, 2026 15:48
@jloleysens jloleysens added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting labels Mar 11, 2026
@elasticmachine
Copy link
Copy Markdown
Contributor

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

@jloleysens jloleysens marked this pull request as draft March 11, 2026 15:48
@elasticmachine
Copy link
Copy Markdown
Contributor

🤖 Jobs for this PR can be triggered through checkboxes. 🚧

ℹ️ To trigger the CI, please tick the checkbox below 👇

  • Click to trigger kibana-pull-request for this PR!
  • Click to trigger kibana-deploy-project-from-pr for this PR!
  • Click to trigger kibana-deploy-cloud-from-pr for this PR!
  • Click to trigger kibana-entity-store-performance-from-pr for this PR!
  • Click to trigger kibana-storybooks-from-pr for this PR!

@jloleysens
Copy link
Copy Markdown
Contributor Author

/ci

@jloleysens jloleysens changed the title [HTTP] Browser buildPath utility [HTTP] Safe delete calls and new browser buildPath utility Mar 12, 2026
@jloleysens
Copy link
Copy Markdown
Contributor Author

/ci

2 similar comments
@jloleysens
Copy link
Copy Markdown
Contributor Author

/ci

@jloleysens
Copy link
Copy Markdown
Contributor Author

/ci

@jloleysens jloleysens changed the title [HTTP] Safe delete calls and new browser buildPath utility [HTTP] Safer client calls and new browser buildPath utility Mar 12, 2026
@jloleysens
Copy link
Copy Markdown
Contributor Author

/ci

1 similar comment
@jloleysens
Copy link
Copy Markdown
Contributor Author

/ci

jloleysens and others added 14 commits March 31, 2026 10:37
Block encoded path traversal segments at the HTTP onRequest layer so Kibana rejects traversal attempts before routing or URL rewriting can redirect requests to unintended endpoints.

Made-with: Cursor
Cover the new browser-side path builder and verify dashboard client routes encode path params instead of interpolating raw ids into request URLs.

Made-with: Cursor
},
{
code: dedent`
const path = \`/api/dashboards/${'${id}'}\`;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me a bit to grasp what this is :) Was it your choice to 'escape' the variable symbols like this, or was this suggested by cursor?
If I had to chose, I'd pick the \ way:

Suggested change
const path = \`/api/dashboards/${'${id}'}\`;
const path = \`/api/dashboards/\${id}\`;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason this format was not working, at least not as intended. I'll refactor for better readability. I agree the cruft introduced by ${'${... is pretty noisey

Copy link
Copy Markdown
Member

@delanni delanni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outside the escaping nitpick, it looks good!

@jloleysens jloleysens enabled auto-merge (squash) April 2, 2026 14:19
Copy link
Copy Markdown
Contributor

@mashhurs mashhurs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logstash changes LGTM!

Copy link
Copy Markdown
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

datavis code review only LGTM.

@jloleysens jloleysens merged commit 2d72284 into elastic:main Apr 3, 2026
19 checks passed
@jloleysens jloleysens deleted the feature/http-browser-build-path-utility branch April 23, 2026 09:07
@jloleysens jloleysens added v9.3.4 v8.19.15 backport:all-open Backport to all branches that could still receive a release and removed backport:skip This PR does not require backporting labels Apr 23, 2026
@kibanamachine
Copy link
Copy Markdown
Contributor

Starting backport for target branches: 8.19, 9.2, 9.3, 9.4

https://github.com/elastic/kibana/actions/runs/24826911443

@kibanamachine
Copy link
Copy Markdown
Contributor

💔 All backports failed

Status Branch Result
8.19 Backport failed because of merge conflicts
9.2 Backport failed because of merge conflicts
9.3 Backport failed because of merge conflicts
9.4 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 257230

Questions ?

Please refer to the Backport tool documentation

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 24, 2026
@kibanamachine
Copy link
Copy Markdown
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.
cc: @jloleysens

1 similar comment
@kibanamachine
Copy link
Copy Markdown
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.
cc: @jloleysens

jloleysens added a commit that referenced this pull request Apr 28, 2026
…265249)

## Summary
Backport #257230 to `8.19`.

Resolved branch-specific conflicts while keeping the older `8.19`
dashboard and lens implementations intact.

## Validation
Could not run `node scripts/check_changes.ts` or Jest in the temporary
backport repo because dependencies are not bootstrapped there.

(Made with cursor y'all)

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Gerard Soldevila <gerard.soldevila@elastic.co>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Jeramy Soucy <jeramy.soucy@elastic.co>
@kibanamachine
Copy link
Copy Markdown
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.
cc: @jloleysens

jloleysens added a commit that referenced this pull request Apr 28, 2026
…65250)

## Summary
Backport #257230 to `9.3`.

Resolved branch-specific conflicts for the older branch layout while
preserving the intended path-safety changes.

## Validation
Could not run `node scripts/check_changes.ts` or Jest in the temporary
backport repo because dependencies are not bootstrapped there.

(Made with cursor y'all)

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Gerard Soldevila <gerard.soldevila@elastic.co>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 29, 2026
@Ikuni17 Ikuni17 added v9.3.5 and removed v9.3.4 labels May 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:all-open Backport to all branches that could still receive a release release_note:skip Skip the PR/issue when compiling release notes Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v8.19.15 v9.3.5 v9.4.0 v9.5.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.