-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Avoids URIError in @osd/std url methods
#10915
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
base: main
Are you sure you want to change the base?
Conversation
…arse Signed-off-by: Paul Sebastian <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10915 +/- ##
=======================================
Coverage 60.75% 60.75%
=======================================
Files 4533 4533
Lines 122211 122217 +6
Branches 20483 20483
=======================================
+ Hits 74250 74258 +8
+ Misses 42720 42719 -1
+ Partials 5241 5240 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
packages/osd-std/src/url.ts
Outdated
| } | ||
| return `${obj.protocol}//${obj.hostname}${obj.port ? `:${obj.port}` : ''}`; | ||
| } catch (error) { | ||
| return url; // Safe fallback to original url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as commented in L149 - "Returns the origin (protocol + host + port) from given url if url is a valid absolute url, or null otherwise" it should return null when error is caught since it's likely a malformed url
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @ZilongX, should better to return null if the url failed to parse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, we should better handle here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does sound better - I'll make the change.
|
generally looking good with some comments, thanks @paulstn for the quick fix. Meanwhile there are a bunch of checking test failing for this PR (6+) which doesn't seem to be related the changes introduced in, is it common all PRs ? Then we should fix all those failed test cases |
They seemed to be unrelated, a test rerun has them all passing now. Edit: They seem to be related to something else that is causing every PR to fail. |
Signed-off-by: Paul Sebastian <[email protected]>
Description
Urls, when being parsed through any functionality within OSD that uses the @osd/std package’s methods for
modifyUrl,isRelativeUrl, orgetUrlOrigin, eventually go on to use theparsemethod from thenode:urlpackage.When the URL is malformed, the
parsemethod will throw aURIError, which is not handled by the current code. This means that malformed URLs can crash the page.An exmaple service that uses modifyUrl that causes the application to crash:
https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/core/server/http/base_path_service.ts#L87
I've added tests that would've previously passed in user input that can cause crashes, and changed the
urlfile's methods to catch errors and return values that make sense to default to.The example malicious URL that I've written into tests has a malformed
passin theauthsection of the url, since that is where thenode:url'sparsemethod would explicitly throw a URIError.Changelog
Check List
yarn test:jestyarn test:jest_integration