(PUP-9135) Treat task metadata referring to missing module as invalid #7083
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, we were raising a ModuleNotFound error in the case where a task depended on a file from another module that didn't exist. Raising that error caused a failure that was difficult to debug (the task was treated as simply not existing). In reality, this scenario is generally a problem with the metadata just like any other metadata error. Because of that, we now raise InvalidMetadata, which causes the actual error message to propagate through puppet-server and orchestrator, to the client. Additionally, this error message was vague and fairly unhelpful as it didn't explain *why* it was trying to load that module in the first place. That message has been improved.
nicklewis
force-pushed
the
PUP-9135-wrong-error-when-module-not-found
branch
from
b13429d to
07637c3
Compare
MikaelSmith
suggested changes
Sep 12, 2018
| @@ -63,11 +63,6 @@ def self.is_tasks_filename?(path) | |||
| end | |||
|
|
|||
| def self.get_file_details(path, mod) | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Previously, we were checking whether the referred file existed *before* checking whether it was a path traversal. This allowed a potential disclosure of file existence, through the roundabout method of deploying some task metadata using a path traversal to refer to a path of interest and then attempting to run the task and checking the error message. We now check for path traversal *first*, which also avoids checking it for every separate file in a directory.
nicklewis
force-pushed
the
PUP-9135-wrong-error-when-module-not-found
branch
from
07637c3 to
e9fed0a
Compare
MikaelSmith
approved these changes
Sep 12, 2018
|
Merge when CI is green. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, we were raising a ModuleNotFound error in the case where a task
depended on a file from another module that didn't exist. Raising that
error caused a failure that was difficult to debug (the task was treated as
simply not existing). In reality, this scenario is generally a problem with
the metadata just like any other metadata error. Because of that, we now
raise InvalidMetadata, which causes the actual error message to propagate
through puppet-server and orchestrator, to the client.
Additionally, this error message was vague and fairly unhelpful as it
didn't explain why it was trying to load that module in the first place.
That message has been improved.
(PUP-9135) Check for path traversal before checking file existence
Previously, we were checking whether the referred file existed before
checking whether it was a path traversal. This allowed a potential
disclosure of file existence, through the roundabout method of deploying
some task metadata using a path traversal to refer to a path of interest
and then attempting to run the task and checking the error message. We now
check for path traversal first, which also avoids checking it for every
separate file in a directory.