Skip to content
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.

Sign up for GitHub

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

fix: ui path validation #297

Merged
merged 2 commits into from
Nov 23, 2023
Merged

fix: ui path validation #297

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d979ab1
fix: ui path validation
m-ildefons Nov 23, 2023
2217c95
config: disable UI path validation
m-ildefons Nov 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
# limitations under the License.

.PHONY: check-ui-backend-env
.DEFAULT_GOAL:=image-build-ui

########################################################################
# Testing cluster
Expand Down Expand Up @@ -103,5 +104,3 @@ check-ui-backend-env:
ifndef S3GW_SERVICE_URL
$(error S3GW_SERVICE_URL must be set.)
endif


2 changes: 1 addition & 1 deletion src/backend/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

this still doesn't look right, because it will validate to true something like /-///. What about something like /?\w[\w-]+(?:/[\w-]+/?)$ ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Is /-/// not a valid path in this context?
  2. 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)

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then, please, leave a note on the code saying that needs to be validated, and file an issue for it.

if match is None:
logger.error(
f"The value of the environment variable {key} is malformed: {value}" # noqa: E501
Expand Down
5 changes: 5 additions & 0 deletions src/backend/tests/unit/test_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,11 @@ def test_good_ui_path_3() -> None:
assert "/foo-bar/baz/" == get_ui_path()


def test_good_ui_path_4() -> None:
os.environ["S3GW_UI_PATH"] = "/foo-bar/foo-bar/"
assert "/foo-bar/foo-bar/" == get_ui_path()


def test_no_ui_path() -> None:
os.environ.pop("S3GW_UI_PATH")
try:
Expand Down