Validate absolute pathnames in remotes' URLs #1480

lubosmj · 2021-07-12T17:16:30Z

Before this change, it was not possible to determine why did the synchronization fail when a user provided a seemingly valid URL. This commit also adds more relevant information to the error message.

Having set ALLOWED_EXPORT_PATHS to ["/tmp", "/home/vagrant/test"], the following error messages are shown:

$ pulp file remote create --name test --url file://error/vagrant/test/centos-7/PULP_MANIFEST
Error: {"url":["The path 'error/vagrant/test/centos-7/PULP_MANIFEST' needs to be an absolute pathname."]}

$ pulp file remote create --name test --url file:///error/vagrant/test/centos-7/PULP_MANIFEST
Error: {"url":["The path '/error/vagrant/test/centos-7/PULP_MANIFEST' is not in allowed import paths"]}

closes #9080

pulpbot · 2021-07-12T17:36:51Z

WARNING!!! This PR is not attached to an issue. In most cases this is not advisable. Please see our PR docs for more information about how to attach this PR to an issue.

dralley · 2021-07-12T18:37:08Z

pulpcore/app/serializers/repository.py

+        if any(
+            user_provided_realpath.startswith(allowed_path)
+            for allowed_path in settings.ALLOWED_IMPORT_PATHS
+        ):


To be honest the for loop felt a little cleaner.

You can even read it as a valid sentence: "If any user-provided real path starts with (an) allowed path for allowed path in Allowed Import Paths, return URL"! 👍

No strong feelings, it's just awkward because of the indentation

dralley · 2021-07-13T19:28:35Z

OK

$ pulp file repository sync --name $REPO_NAME --remote $REMOTE_NAME
Started background task /pulp/api/v3/tasks/b4748fc7-cec9-425d-b39e-46fcb9097153/
Error: Task /pulp/api/v3/tasks/b4748fc7-cec9-425d-b39e-46fcb9097153/ failed: '[ErrorDetail(string="The url 'file://home/vagrant/test/centos-7/PULP_MANIFEST' is not in an allowed import path. The path is interpreted as '/var/lib/pulp/tmp/73438@pulp3-source-fedora33.localhost.example.com/tmp2ft_wsar/home/vagrant/test/centos-7/PULP_MANIFEST'", code='invalid')]'

So I understand why the first remote failed to be created (because file://error/vagrant/test/centos-7/PULP_MANIFEST doesn't match any of the allowed paths) but I don't understand why the second one was allowed in the first place.

If I understand correctly, the problem is that file:// is part of the URL, so the actual path is interpreted as relative (home/vagrant/...) instead of absolute (/home/vagrant/...). That feels like something that we could be catching up front?

I think maybe clearest way we could improve the error message would be to strip the file:// prefix and present the user with the raw path that was extracted from their URL.

dralley · 2021-07-13T19:33:08Z

Edit: There is an actual bug with the path matching that you've uncovered here :) So we should create an issue for this and backport it.

dralley · 2021-07-13T19:48:39Z

I think maybe clearest way we could improve the error message would be to strip the file:// prefix and present the user with the raw path that was extracted from their URL.

Discussion on Matrix:

We should probably automatically reject all relative paths with a specific error message, and we can provide the extracted path to make it clear to the user.

The matching should have rejected home/vagrant/test/... since it is not the same as /home/vagrant/test/... -- hopefully the rejection of relative paths should handle this case automatically.

dralley · 2021-07-14T15:01:13Z

pulpcore/app/serializers/repository.py

+        if not os.path.isabs(user_path):
+            raise serializers.ValidationError(
+                _("The path '{}' needs to be an absolute pathname.").format(user_path)
+            )


dralley · 2021-07-14T15:03:52Z

pulpcore/app/serializers/repository.py

+                return url
+
+        raise serializers.ValidationError(
+            _("The path '{}' is not in allowed import paths").format(user_path)


What do you think about "The path '{}` is not a subdirectory of one of the allowed import paths"

Actually, it is not a subdirectory. It is a full path to the resource.

The check is only that the path startswith one of the allowed paths. For example if /tmp is allowed then any subdirectory of /tmp can be used.

e.g. https://github.com/pulp/pulp_rpm/pull/2034/files#diff-5a20483ca2f367cb948f26046e71ca30e3aac3e3622a02e4d10b0471966af4fdR150

How can /tmp/test/centos-7/PULP_MANIFEST be a subdirectory of /tmp/?

Fair point that an error message the describes "..../PULP_MANIFEST" as a subdirectory would be a little strange. The updated wording is probably OK.

dralley · 2021-07-14T15:04:42Z

pulpcore/tests/functional/api/using_plugin/test_crud_repos.py

+            "name": utils.uuid4(),
+            "url": "file://error/path/name",
+        }
+        self.raise_for_invalid_request(remote_attrs)


dralley · 2021-07-14T15:05:22Z

pulpcore/tests/functional/api/using_plugin/test_crud_repos.py

+        remote_attrs = {
+            "name": utils.uuid4(),
+            "url": "file:///tmp/good",
+        }


Perfect, thanks!

dralley · 2021-07-14T16:33:36Z

@lubosmj Please squash the functional tests into the same commit, it makes cherry-picking easier.

lubosmj · 2021-07-14T16:42:29Z

@lubosmj Please squash the functional tests into the same commit, it makes cherry-picking easier.

I squashed the commits, thanks for pointing it out.

Before this change, it was not possible to determine why did the synchronization fail when a user provided a seemingly valid URL. This commit also adds more relevant information to the error message. Having set `ALLOWED_EXPORT_PATHS` to `["/tmp", "/home/vagrant/test"]`, the following error messages are shown: ``` $ pulp file remote create --name test --url file://error/vagrant/test/centos-7/PULP_MANIFEST Error: {"url":["The path 'error/vagrant/test/centos-7/PULP_MANIFEST' needs to be an absolute pathname."]} $ pulp file remote create --name test --url file:///error/vagrant/test/centos-7/PULP_MANIFEST Error: {"url":["The path '/error/vagrant/test/centos-7/PULP_MANIFEST' does not start with any of the allowed import paths"]} ``` closes #9080

lubosmj changed the title ~~Provide more detailed error message~~ Provide a more detailed error message Jul 12, 2021

lubosmj force-pushed the improve-error-message-allimpp branch from e28f68c to 24669d9 Compare July 12, 2021 17:17

dralley reviewed Jul 12, 2021

View reviewed changes

daviddavis self-requested a review July 13, 2021 13:49

lubosmj marked this pull request as draft July 13, 2021 19:47

lubosmj force-pushed the improve-error-message-allimpp branch from 24669d9 to 89ded77 Compare July 14, 2021 10:41

lubosmj changed the title ~~Provide a more detailed error message~~ Validate absolute pathnames in remotes' URLs Jul 14, 2021

lubosmj force-pushed the improve-error-message-allimpp branch 2 times, most recently from 96cb2e5 to 1429277 Compare July 14, 2021 11:09

lubosmj marked this pull request as ready for review July 14, 2021 11:36

lubosmj requested a review from dralley July 14, 2021 11:36

dralley reviewed Jul 14, 2021

View reviewed changes

dralley approved these changes Jul 14, 2021

View reviewed changes

lubosmj force-pushed the improve-error-message-allimpp branch from 1429277 to ba6e75c Compare July 14, 2021 16:30

lubosmj force-pushed the improve-error-message-allimpp branch 2 times, most recently from 3085fba to 4fefc29 Compare July 14, 2021 16:42

lubosmj force-pushed the improve-error-message-allimpp branch from 4fefc29 to 9980d25 Compare July 14, 2021 16:45

daviddavis approved these changes Jul 14, 2021

View reviewed changes

dralley merged commit d8cba5a into pulp:master Jul 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate absolute pathnames in remotes' URLs #1480

Validate absolute pathnames in remotes' URLs #1480

lubosmj commented Jul 12, 2021 •

edited

Loading

pulpbot commented Jul 12, 2021

dralley Jul 12, 2021

lubosmj Jul 12, 2021

dralley Jul 12, 2021

dralley commented Jul 13, 2021 •

edited

Loading

dralley commented Jul 13, 2021 •

edited

Loading

dralley commented Jul 13, 2021 •

edited

Loading

dralley Jul 14, 2021

dralley Jul 14, 2021

lubosmj Jul 14, 2021

dralley Jul 14, 2021

dralley Jul 14, 2021

lubosmj Jul 14, 2021

dralley Jul 14, 2021

dralley Jul 14, 2021

dralley Jul 14, 2021

dralley commented Jul 14, 2021

lubosmj commented Jul 14, 2021

Validate absolute pathnames in remotes' URLs #1480

Validate absolute pathnames in remotes' URLs #1480

Conversation

lubosmj commented Jul 12, 2021 • edited Loading

pulpbot commented Jul 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dralley commented Jul 13, 2021 • edited Loading

dralley commented Jul 13, 2021 • edited Loading

dralley commented Jul 13, 2021 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dralley commented Jul 14, 2021

lubosmj commented Jul 14, 2021

lubosmj commented Jul 12, 2021 •

edited

Loading

dralley commented Jul 13, 2021 •

edited

Loading

dralley commented Jul 13, 2021 •

edited

Loading

dralley commented Jul 13, 2021 •

edited

Loading