fix: ui path validation #297
Conversation
m-ildefons
added
kind/bug
m-ildefons
force-pushed
the
fix/ui-path-validation
branch
from
578f798 to
2b87a0f
Compare
Fix validation of the UI application path. Signed-off-by: Moritz Röhrich <[email protected]>
m-ildefons
force-pushed
the
fix/ui-path-validation
branch
from
2b87a0f to
d979ab1
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #297 +/- ##
==========================================
- Coverage 98.86% 98.68% -0.19%
==========================================
Files 15 15
Lines 1061 1061
==========================================
- Hits 1049 1047 -2
- Misses 12 14 +2 ☔ View full report in Codecov by Sentry. |
src/backend/config.py
Outdated
| @@ -120,7 +120,7 @@ def get_ui_path() -> str: | |||
| def post_process(key: str, value: str | None) -> str: | |||
| if value is None or value == "/": | |||
| return "/" | |||
| match = re.fullmatch(r"/?[\w./-]+(?:[\w]+)/?", value) | |||
| match = re.fullmatch(r"[\w/-]+[\w/]+", value) | |||
There was a problem hiding this comment.
this still doesn't look right, because it will validate to true something like /-///. What about something like /?\w[\w-]+(?:/[\w-]+/?)$ ?
There was a problem hiding this comment.
- Is
/-///not a valid path in this context? - Your regex allows something like
/fooobar-//, which wouldn't be so great either.
I don't think we should be validating paths with regexes at all (ever tried to validate anything else remotely complex with regexes? Like an email address?).
But I couldn't find a library that can just tell me if something is a valid URL path.
What I'm trying to do with this commit is not excluding all possible invalid paths, but soften up the rules enough that valid paths don't fail validation anymore, as this will otherwise be a real problem with the Longhorn integration (see the error log in the linked issue)
There was a problem hiding this comment.
- Is
/-///not a valid path in this context?
From the test you provided, I'm assuming it's not. Generally speaking, it wouldn't be, but we also don't have a definition of what is expected. 🤷
2. Your regex allows something like `/fooobar-//`, which wouldn't be so great either.
True, it can certainly see some more thinking and a few more iterations.
I don't think we should be validating paths with regexes at all (ever tried to validate anything else remotely complex with regexes? Like an email address?).
Well, yes. Regular expressions may be annoying, but they are incredibly powerful.
What I'm trying to do with this commit is not excluding all possible invalid paths, but soften up the rules enough that valid paths don't fail validation anymore, as this will otherwise be a real problem with the Longhorn integration (see the error log in the linked issue)
What's the cutoff for how much we soften this validation? Because I'd be more comfortable removing it altogether, rather than have a validation that does a half-baked job. At least by not validating we know that we are not validating, rather than have the false sense of security from a validation check.
There was a problem hiding this comment.
Because I'd be more comfortable removing it altogether, rather than have a validation that does a half-baked job
That's fine with me, I'll take the axe to the code then.
There was a problem hiding this comment.
Then, please, leave a note on the code saying that needs to be validated, and file an issue for it.
Disable UI path validation completely. Signed-off-by: Moritz Röhrich <[email protected]>
|
codecov is being annoying and I haven't been able to figure out how to reduce the threshold to something more reasonable. Merging without having it passing. |
Fix validation of the UI application path by removing it.
This removes the buggy regex that tries in vain to validate that the user configured a correct path. This is a must-have for v0.23 as it otherwise breaks Longhorn integration.
Fixes: https://github.com/aquarist-labs/s3gw/issues/834