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.

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

Address security bug in yaml loading #175

Merged
merged 2 commits into from
Aug 28, 2020
Merged

Conversation

vmayoral
Copy link
Member

Though testing code, still worth curating the bug.

Similar to ros/actionlib#171 and connected
with aliasrobotics/RVD#2401.

Curate bug similar to ros/actionlib#171.
Connected with aliasrobotics/RVD#2401.

Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>
@@ -219,7 +219,7 @@ def test_launch_node_with_parameter_dict(self):
file_path, is_file = expanded_parameter_arguments[0]
assert is_file
with open(file_path, 'r') as h:
expanded_parameters_dict = yaml.load(h, Loader=yaml.FullLoader)
expanded_parameters_dict = yaml.safe_load(h, Loader=yaml.FullLoader)
Copy link
Member

Choose a reason for hiding this comment

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

This change currently fails CI:

TypeError: safe_load() got an unexpected keyword argument 'Loader'

Copy link
Member

Choose a reason for hiding this comment

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

Since CI (https://ci.ros2.org/view/All/job/test_ci_linux/42/pytest-warnings-with-location/) only marks the first case as a warning I went ahead and reverted the second change in ef54e1c.

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
@dirk-thomas
Copy link
Member

@vmayoral I will go ahead and merge the reduced patch for now since it resolves a warning shown in the CI output. If you still think the reverted case needs to be changed please create a follow up pull request. Thanks.

@dirk-thomas dirk-thomas merged commit 38407ba into ros2:master Aug 28, 2020
jacobperron pushed a commit that referenced this pull request Oct 26, 2021
* Address security bug in yaml loading

Curate bug similar to ros/actionlib#171.
Connected with aliasrobotics/RVD#2401.

Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>

* revert one of the changes

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

Co-authored-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
jacobperron pushed a commit that referenced this pull request Nov 16, 2021
* Address security bug in yaml loading

Curate bug similar to ros/actionlib#171.
Connected with aliasrobotics/RVD#2401.

Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>

* revert one of the changes

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

Co-authored-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants