Avoids URIError in @osd/std url methods
#10915
Conversation
…arse Signed-off-by: Paul Sebastian <[email protected]>
paulstn
requested review from
AMoo-Miki,
BionIT,
Flyingliuhub,
LDrago27,
Maosaic,
SuZhou-Joe,
ZilongX,
abbyhu2000,
ananzh,
angle943,
ashwin-pc,
bandinib-amzn,
curq,
huyaboo,
joshuali925,
joshuarrrr,
kavilla,
manasvinibs,
mengweieric,
ruanyl,
sejli,
virajsanghvi,
wanglam,
xinruiba,
zengyan-amazon,
zhongnansu and
zhyuanqi
as code owners
paulstn
requested review from
TackAdam,
d-buckner,
ps48,
raintygao,
ruchidh and
sumukhswamy
as code owners
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.
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.
Agree with @ZilongX, should better to return null if the url failed to parse
There was a problem hiding this comment.
agree, we should better handle here
There was a problem hiding this comment.
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]>
paulstn
added
infra
OSD
4 participants
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