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

PR - Issue 50634 - Clean up CLI errors output #3713

Closed
389-ds-bot opened this issue Sep 13, 2020 · 12 comments
Closed

PR - Issue 50634 - Clean up CLI errors output #3713

389-ds-bot opened this issue Sep 13, 2020 · 12 comments
Labels
merged Migration flag - PR pr Migration flag - PR

Comments

@389-ds-bot
Copy link

389-ds-bot commented Sep 13, 2020

Cloned from Pagure Pull-Request: https://pagure.io/389-ds-base/pull-request/50658

  • Created at 2019-10-17 13:49:39 by spichugi (@droideck)
  • Merged at 2019-10-18 08:07:25

Description: CLI tools should print human easy readable messages
if something went wrong.
As discussed here: https://pagure.io/389-ds-base/pull-request/50624

Change the CLI error processing so the dict type is always transformed.

Resolves: #3689

Reviewed by: ?

@389-ds-bot 389-ds-bot added merged Migration flag - PR pr Migration flag - PR labels Sep 13, 2020
@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2019-10-17 14:05:08

LGTM

@389-ds-bot
Copy link
Author

Comment from mhonek (@kenoh) at 2019-10-17 14:11:56

Couldn't we abstract that sequence of several lines that are literally the same into a procedure?

@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2019-10-17 14:44:56

1 new commit added

  • Encupsulate code

@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2019-10-17 14:45:12

Good catch, thanks!

@389-ds-bot
Copy link
Author

Comment from mhonek (@kenoh) at 2019-10-17 15:15:41

Looks good, thanks!

@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2019-10-18 07:59:15

rebased onto b74ddc0d1656ca2d487141c14d7c3a967a47f8ae

@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2019-10-18 08:00:41

rebased onto a2e3c02

@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2019-10-18 08:07:26

Pull-Request has been merged by droideck

@389-ds-bot
Copy link
Author

Comment from firstyear (@Firstyear) at 2019-10-21 02:12:06

This is a security risk, because if an attacker can control any input that becomes put into an error message, it will be run here. This is not safe.

@389-ds-bot
Copy link
Author

Comment from spichugi (@droideck) at 2019-10-21 11:31:15

This is a security risk, because if an attacker can control any input that becomes put into an error message, it will be run here. This is not safe.

Could you please provide some examples of a possible attack?..
Looking at the code and at the docstring, it looks safe - https://hg.python.org/cpython/file/tip/Lib/ast.py#l40

@389-ds-bot
Copy link
Author

Comment from firstyear (@Firstyear) at 2019-10-22 00:31:33

My apologies - eval normal leads to horrible things, but indeed you chose the safe one! Okay, could you comment (with the oneline rule maybe?) about this discussion above the use of literal_eval incase this discussion point comes up in a security review?

@389-ds-bot
Copy link
Author

Patch
50658.patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged Migration flag - PR pr Migration flag - PR
Projects
None yet
Development

No branches or pull requests

1 participant