Use spec-compliant URL parser to parse `next` URL parameter. #183521

azasypkin · 2024-05-15T13:57:07Z

Summary

The Node.js URL parser has been deprecated and we should start slowly switching to the spec-compliant URL parser to parse URLs in Kibana starting from the next query string parameter.

Flaky Test Runner: x65 for every added functional test

azasypkin · 2024-05-15T13:57:48Z

/ci

azasypkin · 2024-05-15T14:13:15Z

/ci

azasypkin · 2024-05-16T10:08:08Z

/ci

azasypkin · 2024-05-16T11:11:32Z

packages/kbn-std/src/is_internal_url.ts

+  // URL (//), or a scheme-relative URL with an empty host (///). Browsers may process such URLs unexpectedly due to
+  // backward compatibility reasons (e.g., a browser may treat `///abc.com` as just `abc.com`). For more details, refer
+  // to https://url.spec.whatwg.org/#concept-basic-url-parser and https://url.spec.whatwg.org/#url-representation.
+  let normalizedURL: URL;


note: hopefully, we can get rid of this trickery when/if this is accepted and implemented: whatwg/url#531

kibanamachine · 2024-05-16T12:38:17Z

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#6010

[✅] x-pack/test/security_functional/login_selector.config.ts: 65/65 tests passed.
[✅] x-pack/test/security_functional/oidc.config.ts: 65/65 tests passed.
[✅] x-pack/test/security_functional/saml.config.ts: 65/65 tests passed.

see run history

elasticmachine · 2024-05-16T13:14:32Z

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

elena-shostak · 2024-05-16T14:16:14Z

packages/kbn-std/src/is_internal_url.ts

+  // to https://url.spec.whatwg.org/#concept-basic-url-parser and https://url.spec.whatwg.org/#url-representation.
+  let normalizedURL: URL;
+  try {
+    for (const baseURL of ['http://example.org:5601', 'https://example.com']) {


QQ: why do we need the one with the port there? isn't https://example.com not enough for normalization?

Based on what we know today, it's redundant, agreed. The idea was to have two base URLs that differ in all three parts of the origin (protocol, host, and port) just to ensure there is no way to exploit any backward compatibility browser quirk that we're not aware of yet to make the browser treat a relative URL as an absolute one.

jeramysoucy

I just had one nit/question regarding the special characters handling/tests, though I may be misinterpreting how this is supposed to work.

jeramysoucy · 2024-05-16T17:28:08Z

packages/kbn-std/src/is_internal_url.test.ts

+    it('should return `false` if absolute URL contains tabs or new lines', () => {
+      for (const char of ['\t', '\n', '\r', '\t\n\r']) {
+        for (const url of [
+          `/${char}/${basePath}app/kibana`,
+          `/${char}//${basePath}app/kibana`,
+          `//${char}/${basePath}app/kibana`,
+          `htt${char}ps://example.com${basePath}`,
+          `htt${char}p://example.org:5601${basePath}`,
+          `file${char}://example.com${basePath}`,
+          `htt${char}p://example.org:5601${char}${basePath}`,
+          `https:/${char}/example.com${char}${basePath}`,
+        ]) {


Wouldn't these all return false anyway, even without the special character? Should we have at least one test case that would otherwise return true, if not for the tab/newline?

I've explained the idea in https://github.com/elastic/kibana/pull/183521/files#r1603774927, but in this specific case (with the base path) you're right - these all seem to return false even with the previous code, which is not what we want. Let me take a look.

jeramysoucy · 2024-05-16T17:34:45Z

packages/kbn-std/src/is_internal_url.test.ts

+    it('should return `false` if absolute URL contains tabs or new lines', () => {
+      for (const char of ['\t', '\n', '\r', '\t\n\r']) {
+        for (const url of [
+          `/${char}/app/kibana`,
+          `/${char}//app/kibana`,
+          `//${char}/app/kibana`,
+          `/${char}/example.com`,
+          `/${char}//example.com`,
+          `//${char}/example.com`,
+          `htt${char}ps://example.com`,
+          `/${char}/example.org`,
+          `/${char}//example.org`,
+          `//${char}/example.org`,
+          `htt${char}ps://example.org`,
+          `/${char}/example.org:5601`,
+          `/${char}//example.org:5601`,
+          `//${char}/example.org:5601`,
+          `htt${char}ps://example.org:5601`,
+          `java${char}script:alert(1)`,
+          `htt${char}p://example.org:5601`,
+          `https:/${char}/example.com`,
+          `file${char}://example.com`,
+          `https://example${char}.com/path`,
+        ]) {


Same here. These would all return false, even without the special character. Should we have at least one test case that would otherwise return true, if not for the tab/newline?

I tried this with using '/app/kibana#/discover/New-Saved-Search' (from the true test case) and adding the special char in various places, but it still returned true. Am I misinterpreting the test?

These would all return false, even without the special character.

Without the special character, they will behave as expected with both the current and previous code and return false, and we have existing tests for this behavior. The issue I'm trying to cover here is that with the previous code, the presence of the special character made this function mistakenly return true instead of false. Try reverting the changes in is_internal_url.ts but keep the tests.

Does that make sense?

Should we have at least one test case that would otherwise return true, if not for the tab/newline?

Yeah, I think it's a good idea. What about having a similar loop with a subset of the URLs tested here, but with a white space instead of \t\r\n that should cause the function to return true?

Okay, I've made an attempt to make the intention a bit clearer in unit tests in 96772c3. @jeramysoucy let me know if it helps.

Yes, this is much more clear to me now. Thanks, Oleg!

…bs/newlines, spaces, and other characters

jeramysoucy

LGTM!

kibana-ci · 2024-05-17T11:13:25Z

💚 Build Succeeded

Buildkite Build
Commit: 96772c3

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`kbnUiSharedDeps-srcJs`	3.1MB	3.1MB	-50.0B

History

💚 Build #210427 succeeded 8fea2be
💔 Build #210186 failed 2111045
💔 Build #210179 failed 72b5054

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

kibanamachine · 2024-05-17T11:44:29Z

💔 All backports failed

Status	Branch	Result
❌	7.17	Backport failed because of merge conflicts
❌	8.14	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 183521

Questions ?

Please refer to the Backport tool documentation

…#183521) ## Summary The Node.js URL parser [has been deprecated](https://nodejs.org/api/url.html#urlparseurlstring-parsequerystring-slashesdenotehost) and we should start slowly switching to the [spec-compliant URL parser](https://url.spec.whatwg.org/#url-parsing) to parse URLs in Kibana starting from the `next` query string parameter. __Flaky Test Runner:__ [x65 for every added functional test](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6010) (cherry picked from commit b306f00) # Conflicts: # .github/CODEOWNERS # x-pack/plugins/security/common/is_internal_url.ts

azasypkin · 2024-05-17T11:59:51Z

💚 All backports created successfully

Status	Branch	Result
✅	8.14
✅	7.17

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…#183521) ## Summary The Node.js URL parser [has been deprecated](https://nodejs.org/api/url.html#urlparseurlstring-parsequerystring-slashesdenotehost) and we should start slowly switching to the [spec-compliant URL parser](https://url.spec.whatwg.org/#url-parsing) to parse URLs in Kibana starting from the `next` query string parameter. __Flaky Test Runner:__ [x65 for every added functional test](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6010) (cherry picked from commit b306f00) # Conflicts: # .github/CODEOWNERS # x-pack/plugins/security/common/is_internal_url.ts

…183521) (#183726) # Backport This will backport the following commits from `main` to `8.14`: - [Use spec-compliant URL parser to parse `next` URL parameter. (#183521)](#183521)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)

…183521) (#183728) # Backport This will backport the following commits from `main` to `7.17`: - [Use spec-compliant URL parser to parse `next` URL parameter. (#183521)](#183521)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

azasypkin added chore Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! release_note:skip Skip the PR/issue when compiling release notes backport:all-open Backport to all branches that could still receive a release labels May 15, 2024

azasypkin force-pushed the issue-xxx-parse-next branch from 8234e28 to 2111045 Compare May 15, 2024 14:13

Use spec-compliant URL parser to parse next URL parameter.

7410e34

azasypkin force-pushed the issue-xxx-parse-next branch from 2111045 to d6ca29f Compare May 16, 2024 09:36

Migrate to WHATWG URL parser only.

8fea2be

azasypkin force-pushed the issue-xxx-parse-next branch from d6ca29f to 8fea2be Compare May 16, 2024 10:07

azasypkin commented May 16, 2024

View reviewed changes

jeramysoucy self-requested a review May 16, 2024 11:46

azasypkin marked this pull request as ready for review May 16, 2024 13:14

azasypkin requested review from a team as code owners May 16, 2024 13:14

pgayvallet approved these changes May 16, 2024

View reviewed changes

elena-shostak reviewed May 16, 2024

View reviewed changes

jeramysoucy reviewed May 16, 2024

View reviewed changes

azasypkin added 2 commits May 17, 2024 09:46

Merge branch 'main' into issue-xxx-parse-next

3e55db9

Review#1: extend unit tests and split them in 3 different buckets: ta…

96772c3

…bs/newlines, spaces, and other characters

azasypkin requested a review from jeramysoucy May 17, 2024 09:53

jeramysoucy approved these changes May 17, 2024

View reviewed changes

azasypkin merged commit b306f00 into elastic:main May 17, 2024
18 checks passed

azasypkin deleted the issue-xxx-parse-next branch May 17, 2024 11:39

kibanamachine added the v8.15.0 label May 17, 2024

azasypkin mentioned this pull request May 17, 2024

[8.14] Use spec-compliant URL parser to parse next URL parameter. (#183521) #183726

Merged

azasypkin mentioned this pull request May 17, 2024

[7.17] Use spec-compliant URL parser to parse next URL parameter. (#183521) #183728

Merged

kibanamachine added the v8.14.0 label May 17, 2024

kibanamachine added the v7.17.22 label May 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use spec-compliant URL parser to parse `next` URL parameter. #183521

Use spec-compliant URL parser to parse `next` URL parameter. #183521

azasypkin commented May 15, 2024 •

edited by kibanamachine

azasypkin commented May 15, 2024

azasypkin commented May 15, 2024

azasypkin commented May 16, 2024

azasypkin May 16, 2024

kibanamachine commented May 16, 2024

elasticmachine commented May 16, 2024

elena-shostak May 16, 2024

azasypkin May 16, 2024

jeramysoucy left a comment

jeramysoucy May 16, 2024

azasypkin May 17, 2024

jeramysoucy May 16, 2024

azasypkin May 17, 2024

azasypkin May 17, 2024

jeramysoucy May 17, 2024

jeramysoucy left a comment

kibana-ci commented May 17, 2024

kibanamachine commented May 17, 2024

azasypkin commented May 17, 2024

Use spec-compliant URL parser to parse next URL parameter. #183521

Use spec-compliant URL parser to parse next URL parameter. #183521

Conversation

azasypkin commented May 15, 2024 • edited by kibanamachine

Summary

azasypkin commented May 15, 2024

azasypkin commented May 15, 2024

azasypkin commented May 16, 2024

Choose a reason for hiding this comment

kibanamachine commented May 16, 2024

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#6010

elasticmachine commented May 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeramysoucy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeramysoucy left a comment

Choose a reason for hiding this comment

kibana-ci commented May 17, 2024

💚 Build Succeeded

Metrics [docs]

Page load bundle

History

kibanamachine commented May 17, 2024

💔 All backports failed

Manual backport

Questions ?

azasypkin commented May 17, 2024

💚 All backports created successfully

Questions ?

Use spec-compliant URL parser to parse `next` URL parameter. #183521

Use spec-compliant URL parser to parse `next` URL parameter. #183521

azasypkin commented May 15, 2024 •

edited by kibanamachine