)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"12ed5c1a2e9032b8291079ce3e1f4b1afd163559","unresolved":false,"context_lines":[{"line_number":9,"context_line":"Add two methods to common.py that will help build and sanitize commands"},{"line_number":10,"context_line":"being sent to the remote VIOS system to prevent shell injection."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"fixes bug 1192971"},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Change-Id: If5ac48c5c889034c7b0ba24d977e8f4a14137a12"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"AAAAP3%2F%2F9so%3D","line":12,"updated":"2013-07-08 13:58:45.000000000","message":"Seems to be the wrong bug ID, the link doesn\u0027t work.  Also, please make sure the bug is tagged with \u0027powervm\u0027 since I didn\u0027t see it in the powervm bug list via the tag query.","commit_id":"19076d9944ccce041c4be12c012047965ff657b4"}],"nova/tests/virt/powervm/test_powervm.py":[{"author":{"_account_id":6928,"name":"Ben Nemec","email":"openstack@nemebean.com","username":"bnemec"},"change_message_id":"15176860f7fe768a53317d5ff58e01389cdaed4e","unresolved":false,"context_lines":[{"line_number":880,"context_line":""},{"line_number":881,"context_line":"    def test_command_builder_good_path(self):"},{"line_number":882,"context_line":"        \"\"\""},{"line_number":883,"context_line":"        Test to ensure we santize commands being sent to the remote VIOS"},{"line_number":884,"context_line":"        system to protect against shell injections. This case tests with"},{"line_number":885,"context_line":"        unharmful paths."},{"line_number":886,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAP3%2F%2Fnbs%3D","line":883,"updated":"2013-07-15 20:19:28.000000000","message":"*sanitize","commit_id":"76f20adad87f108277221184b9a1bba35eea4d1f"},{"author":{"_account_id":6928,"name":"Ben Nemec","email":"openstack@nemebean.com","username":"bnemec"},"change_message_id":"cb01ff04a989372cf639594504f38803f96d7459","unresolved":false,"context_lines":[{"line_number":858,"context_line":""},{"line_number":859,"context_line":"    def test_command_builder_good_path(self):"},{"line_number":860,"context_line":"        \"\"\""},{"line_number":861,"context_line":"        Test to ensure we santize commands being sent to the remote VIOS"},{"line_number":862,"context_line":"        system to protect against shell injections. This case tests with"},{"line_number":863,"context_line":"        unharmful paths."},{"line_number":864,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAAQn%2F%2FPGY%3D","line":861,"updated":"2013-08-04 23:48:24.000000000","message":"*sanitize","commit_id":"1c7c0e810223ff5c67b8a424cf99656fde7ed1b0"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"ff75fb93702926b834f75d4010d29a6b7fab9a44","unresolved":false,"context_lines":[{"line_number":858,"context_line":""},{"line_number":859,"context_line":"    def test_command_builder_good_path(self):"},{"line_number":860,"context_line":"        \"\"\""},{"line_number":861,"context_line":"        Test to ensure we santize commands being sent to the remote VIOS"},{"line_number":862,"context_line":"        system to protect against shell injections. This case tests with"},{"line_number":863,"context_line":"        unharmful paths."},{"line_number":864,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAAQn%2F%2B%2FaA%3D","line":861,"in_reply_to":"AAAAQn%2F%2FPGY%3D","updated":"2013-08-08 20:01:00.000000000","message":"Done","commit_id":"1c7c0e810223ff5c67b8a424cf99656fde7ed1b0"},{"author":{"_account_id":4992,"name":"malini-k-bhandaru","email":"malini.k.bhandaru@intel.com","username":"malini-k-bhandaru"},"change_message_id":"d62432f4ffa7fd932049573ebaaf3ce2f1a0121f","unresolved":false,"context_lines":[{"line_number":1073,"context_line":"        remote_path \u003d \u0027/mnt/openstack/images\u0027"},{"line_number":1074,"context_line":"        exp_remote_path \u003d os.path.join(remote_path,"},{"line_number":1075,"context_line":"                                       os.path.basename(file_path))"},{"line_number":1076,"context_line":"        exp_remote_path \u003d \u0027\\\u0027\u0027 + exp_remote_path + \u0027\\\u0027\u0027"},{"line_number":1077,"context_line":"        exp_cmd \u003d \u0027 \u0027.join([\u0027/usr/bin/rm -f\u0027, exp_remote_path])"},{"line_number":1078,"context_line":""},{"line_number":1079,"context_line":"        def fake_md5sum_remote_file(remote_path):"}],"source_content_type":"text/x-python","patch_set":11,"id":"AAAARn%2F%2FwUc%3D","line":1076,"updated":"2013-08-22 08:29:41.000000000","message":"Why did you need/want to do this modification?","commit_id":"ddc65b392bceae9840b593db542c226407af0c22"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"9548ec0f8cd37a6584a73a70a2ebbfe3898196d4","unresolved":false,"context_lines":[{"line_number":1073,"context_line":"        remote_path \u003d \u0027/mnt/openstack/images\u0027"},{"line_number":1074,"context_line":"        exp_remote_path \u003d os.path.join(remote_path,"},{"line_number":1075,"context_line":"                                       os.path.basename(file_path))"},{"line_number":1076,"context_line":"        exp_remote_path \u003d \u0027\\\u0027\u0027 + exp_remote_path + \u0027\\\u0027\u0027"},{"line_number":1077,"context_line":"        exp_cmd \u003d \u0027 \u0027.join([\u0027/usr/bin/rm -f\u0027, exp_remote_path])"},{"line_number":1078,"context_line":""},{"line_number":1079,"context_line":"        def fake_md5sum_remote_file(remote_path):"}],"source_content_type":"text/x-python","patch_set":11,"id":"AAAARn%2F%2Fvdg%3D","line":1076,"in_reply_to":"AAAARn%2F%2FwUc%3D","updated":"2013-08-22 13:34:44.000000000","message":"_copy_image_file method uses common.command_builder, so we either need to stub out the method or modify the expected output to match what we get back from common.command_builder, wrapping the file path in single quotes.","commit_id":"ddc65b392bceae9840b593db542c226407af0c22"},{"author":{"_account_id":8560,"name":"Sriram Subramanian","email":"sriram@sriramhere.com","username":"sriramhere"},"change_message_id":"48f6f134f921bc1bd9212e694d3732fe4e117a43","unresolved":false,"context_lines":[{"line_number":959,"context_line":"        system to protect against shell injection attacks. This case tests"},{"line_number":960,"context_line":"        with a harmful injected command."},{"line_number":961,"context_line":"        \"\"\""},{"line_number":962,"context_line":"        cmd \u003d \u0027gzip %s %s\u0027"},{"line_number":963,"context_line":"        self.assertRaises(exception.PowerVMInvalidVIOSParameter,"},{"line_number":964,"context_line":"                          common.command_builder, \u0027some/file/path\u0027,"},{"line_number":965,"context_line":"                          \u0027;rm -rf /;\u0027)"}],"source_content_type":"text/x-python","patch_set":12,"id":"AAAASX%2F%2F7Cc%3D","line":962,"updated":"2013-08-23 17:57:22.000000000","message":"this variable looks like not used later, is this needed?","commit_id":"92002a744b6c3e05a4de84d1ca5cbe71ae7e4a62"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"04530a9bc56d8bb1205868db6e7f097ba25d1ade","unresolved":false,"context_lines":[{"line_number":959,"context_line":"        system to protect against shell injection attacks. This case tests"},{"line_number":960,"context_line":"        with a harmful injected command."},{"line_number":961,"context_line":"        \"\"\""},{"line_number":962,"context_line":"        cmd \u003d \u0027gzip %s %s\u0027"},{"line_number":963,"context_line":"        self.assertRaises(exception.PowerVMInvalidVIOSParameter,"},{"line_number":964,"context_line":"                          common.command_builder, \u0027some/file/path\u0027,"},{"line_number":965,"context_line":"                          \u0027;rm -rf /;\u0027)"}],"source_content_type":"text/x-python","patch_set":12,"id":"AAAASn%2F%2FWdM%3D","line":962,"in_reply_to":"AAAASX%2F%2F7Cc%3D","updated":"2013-09-05 21:36:06.000000000","message":"Good catch, thanks and done!","commit_id":"92002a744b6c3e05a4de84d1ca5cbe71ae7e4a62"}],"nova/virt/powervm/blockdev.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"eafc6ad3259bae1a27655b4d049f5e60db48fe4f","unresolved":false,"context_lines":[{"line_number":268,"context_line":"            file_name \u003d \u0027\u0027.join([lv_name, \u0027_rsz\u0027])"},{"line_number":269,"context_line":"        file_path \u003d os.path.join(image_path, file_name)"},{"line_number":270,"context_line":"        self._copy_device_to_file(lv_name, file_path)"},{"line_number":271,"context_line":"        cmds \u003d common.command_builder(\u0027gzip \"%s\"\u0027, file_path)"},{"line_number":272,"context_line":"        self.run_vios_command_as_root(cmds)"},{"line_number":273,"context_line":"        file_path \u003d file_path + \u0027.gz\u0027"},{"line_number":274,"context_line":"        # If destination is not same host"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAP3%2F%2F9qE%3D","line":271,"updated":"2013-07-08 14:00:21.000000000","message":"Shouldn\u0027t these be single-quotes? Otherwise you\u0027re still susceptible to some forms of injection, such as using variables and other substitutions.\n\nAlso, unless you escape the quote character you\u0027re using, you\u0027re not really improving anything, right? If I send a string like this, you\u0027re still toast:\n\n  \"; rm -Rf /; \"","commit_id":"19076d9944ccce041c4be12c012047965ff657b4"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"afb187ee6a29a1a71b7261ae803cac1886c02b63","unresolved":false,"context_lines":[{"line_number":268,"context_line":"            file_name \u003d \u0027\u0027.join([lv_name, \u0027_rsz\u0027])"},{"line_number":269,"context_line":"        file_path \u003d os.path.join(image_path, file_name)"},{"line_number":270,"context_line":"        self._copy_device_to_file(lv_name, file_path)"},{"line_number":271,"context_line":"        cmds \u003d common.command_builder(\u0027gzip \"%s\"\u0027, file_path)"},{"line_number":272,"context_line":"        self.run_vios_command_as_root(cmds)"},{"line_number":273,"context_line":"        file_path \u003d file_path + \u0027.gz\u0027"},{"line_number":274,"context_line":"        # If destination is not same host"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAP3%2F%2F9iI%3D","line":271,"in_reply_to":"AAAAP3%2F%2F9qE%3D","updated":"2013-07-08 14:41:06.000000000","message":"So basically I would just need to escape the \" around the %s? The reason behind having the substitution surrounded in \" \" is so that it will be treated as a single entity, such as a file or volume group, if the double quotes passed in the variable file_path are escaped or removed.","commit_id":"19076d9944ccce041c4be12c012047965ff657b4"}],"nova/virt/powervm/common.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"12ed5c1a2e9032b8291079ce3e1f4b1afd163559","unresolved":false,"context_lines":[{"line_number":257,"context_line":"        cleanup_key_on_source()"},{"line_number":258,"context_line":""},{"line_number":259,"context_line":""},{"line_number":260,"context_line":"def command_builder(cmd, *args, **kwargs):"},{"line_number":261,"context_line":"    \"\"\"Here we are building each command that gets sent to a remote system"},{"line_number":262,"context_line":"    to ensure security."},{"line_number":263,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAP3%2F%2F9rE%3D","line":260,"updated":"2013-07-08 13:58:45.000000000","message":"Do you need to define **kwargs?  If so, shouldn\u0027t you be handling it below?","commit_id":"19076d9944ccce041c4be12c012047965ff657b4"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"afb187ee6a29a1a71b7261ae803cac1886c02b63","unresolved":false,"context_lines":[{"line_number":257,"context_line":"        cleanup_key_on_source()"},{"line_number":258,"context_line":""},{"line_number":259,"context_line":""},{"line_number":260,"context_line":"def command_builder(cmd, *args, **kwargs):"},{"line_number":261,"context_line":"    \"\"\"Here we are building each command that gets sent to a remote system"},{"line_number":262,"context_line":"    to ensure security."},{"line_number":263,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAP3%2F%2F9kQ%3D","line":260,"in_reply_to":"AAAAP3%2F%2F9rE%3D","updated":"2013-07-08 14:41:06.000000000","message":"I don\u0027t believe we have a case were a keyword argument is passed in, I can remove this.","commit_id":"19076d9944ccce041c4be12c012047965ff657b4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"12ed5c1a2e9032b8291079ce3e1f4b1afd163559","unresolved":false,"context_lines":[{"line_number":262,"context_line":"    to ensure security."},{"line_number":263,"context_line":""},{"line_number":264,"context_line":"    :param cmd: cmd to be built"},{"line_number":265,"context_line":"    :param args: arguments to be built"},{"line_number":266,"context_line":"    :param kwargs: keyword arguments passed in"},{"line_number":267,"context_line":"    :returns: sanitized string"},{"line_number":268,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAP3%2F%2F9qY%3D","line":265,"updated":"2013-07-08 13:58:45.000000000","message":"Looks like each invocation of this method requires that the args values be wrapped in double-quotes so you can parse them correctly.  I have a feeling that someone is going to miss that when using this.  Can you at least mention something in the docstring about requiring the args to be wrapped in double-quotes for proper parsing?","commit_id":"19076d9944ccce041c4be12c012047965ff657b4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"12ed5c1a2e9032b8291079ce3e1f4b1afd163559","unresolved":false,"context_lines":[{"line_number":274,"context_line":"    # NOTE(ldbragst): Here lets ensure if there are any double quotes in"},{"line_number":275,"context_line":"    # the argument, they are properly escaped to avoid shell injections."},{"line_number":276,"context_line":"    for clean_arg in clean_args:"},{"line_number":277,"context_line":"        clean_arg \u003d clean_arg.replace(\u0027\"\u0027, \u0027\\\\\"\u0027)"},{"line_number":278,"context_line":""},{"line_number":279,"context_line":"    # NOTE(ldbragst): Build the original command wih the cleaned values"},{"line_number":280,"context_line":"    # in clean_args"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAP3%2F%2F9sI%3D","line":277,"updated":"2013-07-08 13:58:45.000000000","message":"Isn\u0027t line 293 replacing double-quotes?","commit_id":"19076d9944ccce041c4be12c012047965ff657b4"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"afb187ee6a29a1a71b7261ae803cac1886c02b63","unresolved":false,"context_lines":[{"line_number":274,"context_line":"    # NOTE(ldbragst): Here lets ensure if there are any double quotes in"},{"line_number":275,"context_line":"    # the argument, they are properly escaped to avoid shell injections."},{"line_number":276,"context_line":"    for clean_arg in clean_args:"},{"line_number":277,"context_line":"        clean_arg \u003d clean_arg.replace(\u0027\"\u0027, \u0027\\\\\"\u0027)"},{"line_number":278,"context_line":""},{"line_number":279,"context_line":"    # NOTE(ldbragst): Build the original command wih the cleaned values"},{"line_number":280,"context_line":"    # in clean_args"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAP3%2F%2F9kc%3D","line":277,"in_reply_to":"AAAAP3%2F%2F9sI%3D","updated":"2013-07-08 14:41:06.000000000","message":"Yes, I can remove this","commit_id":"19076d9944ccce041c4be12c012047965ff657b4"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"c172484a39521aa63062df236f5303c5aa4d1c88","unresolved":false,"context_lines":[{"line_number":284,"context_line":"    :param safe_string: string to sanitize"},{"line_number":285,"context_line":"    :returns: safe string"},{"line_number":286,"context_line":"    \"\"\""},{"line_number":287,"context_line":"    for char in \u0027\\\\;\" \u0027:"},{"line_number":288,"context_line":"        # NOTE(ldbragst): This will ensure \\, ;, \", or spaces are not allowed"},{"line_number":289,"context_line":"        # in values being passed to remote systems, preventing shell injection"},{"line_number":290,"context_line":"        # attacks."}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAP3%2F%2F0t8%3D","line":287,"updated":"2013-07-10 21:15:57.000000000","message":"I still think that you should be using single-quotes in your command lines, instead of doubles. Since you don\u0027t remove $ here, if I get something like $(rm /etc/passwd) into my file path, I could do some nasty things.","commit_id":"65590240b8d55875afa3c3353b331afd62f124d2"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"dedc06bca1733c0d7b436760d33a551f5a04507f","unresolved":false,"context_lines":[{"line_number":284,"context_line":"    :param safe_string: string to sanitize"},{"line_number":285,"context_line":"    :returns: safe string"},{"line_number":286,"context_line":"    \"\"\""},{"line_number":287,"context_line":"    for char in \u0027\\\\;\" \u0027:"},{"line_number":288,"context_line":"        # NOTE(ldbragst): This will ensure \\, ;, \", or spaces are not allowed"},{"line_number":289,"context_line":"        # in values being passed to remote systems, preventing shell injection"},{"line_number":290,"context_line":"        # attacks."}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAP3%2F%2FxSU%3D","line":287,"in_reply_to":"AAAAP3%2F%2F0t8%3D","updated":"2013-07-11 18:09:59.000000000","message":"Good point, for fixing this would you recommend reworking the current implementation with single quotes (like you suggested) or using something like what Ben Nemec suggested with pipes.quote? \n\nhttp://docs.python.org/2/library/pipes.html#pipes.quote","commit_id":"65590240b8d55875afa3c3353b331afd62f124d2"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"ae8357abf0fe9d8dfb52eccd0dfa50217d847459","unresolved":false,"context_lines":[{"line_number":284,"context_line":"    :param safe_string: string to sanitize"},{"line_number":285,"context_line":"    :returns: safe string"},{"line_number":286,"context_line":"    \"\"\""},{"line_number":287,"context_line":"    for char in \u0027\\\\;\" \u0027:"},{"line_number":288,"context_line":"        # NOTE(ldbragst): This will ensure \\, ;, \", or spaces are not allowed"},{"line_number":289,"context_line":"        # in values being passed to remote systems, preventing shell injection"},{"line_number":290,"context_line":"        # attacks."}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAP3%2F%2FwzM%3D","line":287,"in_reply_to":"AAAAP3%2F%2Fw0o%3D","updated":"2013-07-11 20:04:10.000000000","message":"nevermind... I got too excited and didn\u0027t read that it was \"new in 3.3\"...","commit_id":"65590240b8d55875afa3c3353b331afd62f124d2"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"2638f139cef9f1750789b4a1e6f0e4d45c9f2e81","unresolved":false,"context_lines":[{"line_number":284,"context_line":"    :param safe_string: string to sanitize"},{"line_number":285,"context_line":"    :returns: safe string"},{"line_number":286,"context_line":"    \"\"\""},{"line_number":287,"context_line":"    for char in \u0027\\\\;\" \u0027:"},{"line_number":288,"context_line":"        # NOTE(ldbragst): This will ensure \\, ;, \", or spaces are not allowed"},{"line_number":289,"context_line":"        # in values being passed to remote systems, preventing shell injection"},{"line_number":290,"context_line":"        # attacks."}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAP3%2F%2FxPc%3D","line":287,"in_reply_to":"AAAAP3%2F%2FxP4%3D","updated":"2013-07-11 18:19:37.000000000","message":"Agreed, if everyone is on board with a pipes.quote implementation I\u0027ll be happy to push up a new patch.","commit_id":"65590240b8d55875afa3c3353b331afd62f124d2"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"e656ff757ecad67b9a395f308dac93a585924da5","unresolved":false,"context_lines":[{"line_number":284,"context_line":"    :param safe_string: string to sanitize"},{"line_number":285,"context_line":"    :returns: safe string"},{"line_number":286,"context_line":"    \"\"\""},{"line_number":287,"context_line":"    for char in \u0027\\\\;\" \u0027:"},{"line_number":288,"context_line":"        # NOTE(ldbragst): This will ensure \\, ;, \", or spaces are not allowed"},{"line_number":289,"context_line":"        # in values being passed to remote systems, preventing shell injection"},{"line_number":290,"context_line":"        # attacks."}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAP3%2F%2Fw0o%3D","line":287,"in_reply_to":"AAAAP3%2F%2FxPc%3D","updated":"2013-07-11 19:59:57.000000000","message":"Actually, comstud just explaining why we *shouldn\u0027t* use pipes as it was probably a private interface before Python 3.3.... but, that doesn\u0027t mean I couldn\u0027t use http://docs.python.org/dev/library/shlex.html#shlex.quote","commit_id":"65590240b8d55875afa3c3353b331afd62f124d2"},{"author":{"_account_id":6928,"name":"Ben Nemec","email":"openstack@nemebean.com","username":"bnemec"},"change_message_id":"a8ecb1729255fa589cf4474208630bbcb30ce630","unresolved":false,"context_lines":[{"line_number":284,"context_line":"    :param safe_string: string to sanitize"},{"line_number":285,"context_line":"    :returns: safe string"},{"line_number":286,"context_line":"    \"\"\""},{"line_number":287,"context_line":"    for char in \u0027\\\\;\" \u0027:"},{"line_number":288,"context_line":"        # NOTE(ldbragst): This will ensure \\, ;, \", or spaces are not allowed"},{"line_number":289,"context_line":"        # in values being passed to remote systems, preventing shell injection"},{"line_number":290,"context_line":"        # attacks."}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAP3%2F%2FxP4%3D","line":287,"in_reply_to":"AAAAP3%2F%2FxSU%3D","updated":"2013-07-11 18:17:16.000000000","message":"FWIW, this is exactly why I suggest not rolling our own.  I believe this is the second attack vector Dan has pointed out that we weren\u0027t handling.  I realize the quote function\u0027s status is kind of weird, but at least it seems to be widely used and presumably has had a lot more eyes on it than we can manage ourselves.","commit_id":"65590240b8d55875afa3c3353b331afd62f124d2"},{"author":{"_account_id":6928,"name":"Ben Nemec","email":"openstack@nemebean.com","username":"bnemec"},"change_message_id":"29f0c062850ece6c6deed47aa6e689ee3a49c69c","unresolved":false,"context_lines":[{"line_number":284,"context_line":"    :param safe_string: string to sanitize"},{"line_number":285,"context_line":"    :returns: safe string"},{"line_number":286,"context_line":"    \"\"\""},{"line_number":287,"context_line":"    for char in \u0027()$\\\\;\" \u0027:"},{"line_number":288,"context_line":"        # NOTE(ldbragst): This will ensure (, ), $, \\, ;, \", or spaces are not"},{"line_number":289,"context_line":"        # allowed in values being passed to remote systems, preventing shell"},{"line_number":290,"context_line":"        # injection attacks."}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAP3%2F%2FoMQ%3D","line":287,"updated":"2013-07-15 17:28:18.000000000","message":"I was going to suggest that you add backticks to this list, but then I discovered the single quoting takes care of that.  I think that means you can remove some of the other characters here too, in particular the double quote and maybe others.\n\nAlso, would a string containing a single quote mess us up here?  I don\u0027t see that case covered in the tests, and since we\u0027re single quoting now it seems like a possible concern.","commit_id":"54f315cf19b1b7edb13523d8bdd4a8e0a3883560"},{"author":{"_account_id":6928,"name":"Ben Nemec","email":"openstack@nemebean.com","username":"bnemec"},"change_message_id":"3dc25b6be8bfd5bc97d57f4c935d2aa6850c14c8","unresolved":false,"context_lines":[{"line_number":284,"context_line":"    :param safe_string: string to sanitize"},{"line_number":285,"context_line":"    :returns: safe string"},{"line_number":286,"context_line":"    \"\"\""},{"line_number":287,"context_line":"    for char in \u0027()$\\\\;\" \u0027:"},{"line_number":288,"context_line":"        # NOTE(ldbragst): This will ensure (, ), $, \\, ;, \", or spaces are not"},{"line_number":289,"context_line":"        # allowed in values being passed to remote systems, preventing shell"},{"line_number":290,"context_line":"        # injection attacks."}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAP3%2F%2Fnw8%3D","line":287,"in_reply_to":"AAAAP3%2F%2FoDI%3D","updated":"2013-07-15 19:01:50.000000000","message":"I can do something like \"touch Ben\\\u0027sImage\" though, and now I have a filename with a single quote in it.  I wouldn\u0027t personally name my files that way, but it is a valid use case.  Even if it turns out to not be a problem, I would like to see a test case to verify it.","commit_id":"54f315cf19b1b7edb13523d8bdd4a8e0a3883560"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"9239fe21523227c28791be38fbc32b5915eef183","unresolved":false,"context_lines":[{"line_number":284,"context_line":"    :param safe_string: string to sanitize"},{"line_number":285,"context_line":"    :returns: safe string"},{"line_number":286,"context_line":"    \"\"\""},{"line_number":287,"context_line":"    for char in \u0027()$\\\\;\" \u0027:"},{"line_number":288,"context_line":"        # NOTE(ldbragst): This will ensure (, ), $, \\, ;, \", or spaces are not"},{"line_number":289,"context_line":"        # allowed in values being passed to remote systems, preventing shell"},{"line_number":290,"context_line":"        # injection attacks."}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAP3%2F%2FoDI%3D","line":287,"in_reply_to":"AAAAP3%2F%2FoMQ%3D","updated":"2013-07-15 17:48:50.000000000","message":"Hey Ben, \n\nI don\u0027t think that having a single quote in the string being cleaned would purpose a problem. As a file name it shouldn\u0027t have a single quote in it anyway and in this case it would just remove it. I can add a test case for it though if that needs to be explicitly tested.","commit_id":"54f315cf19b1b7edb13523d8bdd4a8e0a3883560"},{"author":{"_account_id":6928,"name":"Ben Nemec","email":"openstack@nemebean.com","username":"bnemec"},"change_message_id":"15176860f7fe768a53317d5ff58e01389cdaed4e","unresolved":false,"context_lines":[{"line_number":277,"context_line":"    return clean_cmd"},{"line_number":278,"context_line":""},{"line_number":279,"context_line":""},{"line_number":280,"context_line":"def _command_sanitizer(safe_string):"},{"line_number":281,"context_line":"    \"\"\"Here we are sanitizing a string to ensure it doesn\u0027t have characters"},{"line_number":282,"context_line":"    that might cause \u0027shell injection\u0027."},{"line_number":283,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAP3%2F%2FnaI%3D","line":280,"updated":"2013-07-15 20:19:28.000000000","message":"Isn\u0027t the parameter here technically the dirty string that may have bad characters?  If that\u0027s the case then I don\u0027t think we want to call it safe.","commit_id":"76f20adad87f108277221184b9a1bba35eea4d1f"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ebde541dbd1231e53e52ad1590437cdda7050c9a","unresolved":false,"context_lines":[{"line_number":285,"context_line":"    :returns: safe string"},{"line_number":286,"context_line":"    \"\"\""},{"line_number":287,"context_line":"    for char in \u0027\\\u0027()$\\\\; \u0027:"},{"line_number":288,"context_line":"        # NOTE(ldbragst): This will ensure \u0027, (, ), $, \\, ;, or spaces are"},{"line_number":289,"context_line":"        # not allowed in values being passed to remote systems, preventing"},{"line_number":290,"context_line":"        # shell injection attacks."},{"line_number":291,"context_line":"        safe_string \u003d safe_string.replace(char, \u0027\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAAQn%2F%2FlGs%3D","line":288,"updated":"2013-07-27 16:46:54.000000000","message":"The pipe character mentioned here doesn\u0027t seem to be in the list of banned characters that is the subject of the loop...","commit_id":"edf8f65f319a41f0db997dcd8a89e9b52d67af8a"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ebde541dbd1231e53e52ad1590437cdda7050c9a","unresolved":false,"context_lines":[{"line_number":288,"context_line":"        # NOTE(ldbragst): This will ensure \u0027, (, ), $, \\, ;, or spaces are"},{"line_number":289,"context_line":"        # not allowed in values being passed to remote systems, preventing"},{"line_number":290,"context_line":"        # shell injection attacks."},{"line_number":291,"context_line":"        safe_string \u003d safe_string.replace(char, \u0027\u0027)"},{"line_number":292,"context_line":""},{"line_number":293,"context_line":"    return \u0027\\\u0027\u0027 + safe_string + \u0027\\\u0027\u0027"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAAQn%2F%2FlGg%3D","line":291,"updated":"2013-07-27 16:46:54.000000000","message":"Does it really make sense to allow this to continue if any of the banned characters are present? If a string with one of these characters makes it this far, then the only failure the admin will see will be a strange one even deeper with a seemingly-mangled  version of the original command. Shouldn\u0027t we just fail here with some generic (but contextual) error and log the result for the admin?","commit_id":"edf8f65f319a41f0db997dcd8a89e9b52d67af8a"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"b751e6d0158c5d0d2a09a361b49b7e50f1c61569","unresolved":false,"context_lines":[{"line_number":288,"context_line":"        # NOTE(ldbragst): This will ensure \u0027, (, ), $, \\, ;, or spaces are"},{"line_number":289,"context_line":"        # not allowed in values being passed to remote systems, preventing"},{"line_number":290,"context_line":"        # shell injection attacks."},{"line_number":291,"context_line":"        safe_string \u003d safe_string.replace(char, \u0027\u0027)"},{"line_number":292,"context_line":""},{"line_number":293,"context_line":"    return \u0027\\\u0027\u0027 + safe_string + \u0027\\\u0027\u0027"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAAQn%2F%2FkUo%3D","line":291,"in_reply_to":"AAAAQn%2F%2FlGg%3D","updated":"2013-07-28 22:20:24.000000000","message":"I agree, I thought about that as well. I think redoing this to fit that case is easy enough and would be less confusing. I\u0027ll respin this using that approach. \n\nThanks Dan","commit_id":"edf8f65f319a41f0db997dcd8a89e9b52d67af8a"},{"author":{"_account_id":6928,"name":"Ben Nemec","email":"openstack@nemebean.com","username":"bnemec"},"change_message_id":"cb01ff04a989372cf639594504f38803f96d7459","unresolved":false,"context_lines":[{"line_number":278,"context_line":"    return clean_cmd"},{"line_number":279,"context_line":""},{"line_number":280,"context_line":""},{"line_number":281,"context_line":"def _parameter_check(safe_string):"},{"line_number":282,"context_line":"    \"\"\"Here we are sanitizing a string to ensure it doesn\u0027t have characters"},{"line_number":283,"context_line":"    that might cause \u0027shell injection\u0027."},{"line_number":284,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAAQn%2F%2FPGw%3D","line":281,"updated":"2013-08-04 23:48:24.000000000","message":"I still don\u0027t like safe_string as the name for this parameter.  It\u0027s a bit of a nit, but since it\u0027s security-related code I think it\u0027s good to be explicit that this is _not_ known to be a safe string.","commit_id":"1c7c0e810223ff5c67b8a424cf99656fde7ed1b0"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"ff75fb93702926b834f75d4010d29a6b7fab9a44","unresolved":false,"context_lines":[{"line_number":278,"context_line":"    return clean_cmd"},{"line_number":279,"context_line":""},{"line_number":280,"context_line":""},{"line_number":281,"context_line":"def _parameter_check(safe_string):"},{"line_number":282,"context_line":"    \"\"\"Here we are sanitizing a string to ensure it doesn\u0027t have characters"},{"line_number":283,"context_line":"    that might cause \u0027shell injection\u0027."},{"line_number":284,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAAQn%2F%2B%2FYw%3D","line":281,"in_reply_to":"AAAAQn%2F%2FPGw%3D","updated":"2013-08-08 20:01:00.000000000","message":"Done","commit_id":"1c7c0e810223ff5c67b8a424cf99656fde7ed1b0"},{"author":{"_account_id":6928,"name":"Ben Nemec","email":"openstack@nemebean.com","username":"bnemec"},"change_message_id":"cb01ff04a989372cf639594504f38803f96d7459","unresolved":false,"context_lines":[{"line_number":290,"context_line":"        # not allowed in values being passed to remote systems, preventing"},{"line_number":291,"context_line":"        # shell injection attacks."},{"line_number":292,"context_line":"        if char in safe_string:"},{"line_number":293,"context_line":"            LOG.exception(_(\"Invalid parameter: %(char)s in %(safe_string)s\"),"},{"line_number":294,"context_line":"                          {\u0027char\u0027: char, \u0027safe_string\u0027: safe_string})"},{"line_number":295,"context_line":"            raise exception.PowerVMInvalidVIOSParameter(safe_string)"},{"line_number":296,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAAQn%2F%2FPG0%3D","line":293,"updated":"2013-08-04 23:48:24.000000000","message":"I don\u0027t think LOG.exception is what you want if you\u0027re not in an exception handler.","commit_id":"1c7c0e810223ff5c67b8a424cf99656fde7ed1b0"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"ff75fb93702926b834f75d4010d29a6b7fab9a44","unresolved":false,"context_lines":[{"line_number":290,"context_line":"        # not allowed in values being passed to remote systems, preventing"},{"line_number":291,"context_line":"        # shell injection attacks."},{"line_number":292,"context_line":"        if char in safe_string:"},{"line_number":293,"context_line":"            LOG.exception(_(\"Invalid parameter: %(char)s in %(safe_string)s\"),"},{"line_number":294,"context_line":"                          {\u0027char\u0027: char, \u0027safe_string\u0027: safe_string})"},{"line_number":295,"context_line":"            raise exception.PowerVMInvalidVIOSParameter(safe_string)"},{"line_number":296,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAAQn%2F%2B%2FU0%3D","line":293,"in_reply_to":"AAAAQn%2F%2FPG0%3D","updated":"2013-08-08 20:01:00.000000000","message":"Done","commit_id":"1c7c0e810223ff5c67b8a424cf99656fde7ed1b0"},{"author":{"_account_id":6928,"name":"Ben Nemec","email":"openstack@nemebean.com","username":"bnemec"},"change_message_id":"a8f42df58bc3e2c1c35ba5b33cd06e87d4411f03","unresolved":false,"context_lines":[{"line_number":269,"context_line":"    # NOTE(ldbragst): args come in as an immutable tuple, so here we\u0027re"},{"line_number":270,"context_line":"    # converting it back to a tuple of clean arguments before"},{"line_number":271,"context_line":"    # assembling the command to be run on the remote system."},{"line_number":272,"context_line":"    clean_args \u003d ([_parameter_check(arg) for arg in args])"},{"line_number":273,"context_line":""},{"line_number":274,"context_line":"    # NOTE(ldbragst): Build the original command wih the checked values"},{"line_number":275,"context_line":"    # in clean_args"}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAAQn%2F%2B7Ao%3D","line":272,"updated":"2013-08-09 20:50:23.000000000","message":"Nit: I don\u0027t think the outer parens here or on line 276 are doing anything.","commit_id":"8c055fc2284fc778921ff17eea2f716da58fd0ca"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"79ddeafd81c713c4a45a366fcbbc335bd5644c3f","unresolved":false,"context_lines":[{"line_number":269,"context_line":"    # NOTE(ldbragst): args come in as an immutable tuple, so here we\u0027re"},{"line_number":270,"context_line":"    # converting it back to a tuple of clean arguments before"},{"line_number":271,"context_line":"    # assembling the command to be run on the remote system."},{"line_number":272,"context_line":"    clean_args \u003d ([_parameter_check(arg) for arg in args])"},{"line_number":273,"context_line":""},{"line_number":274,"context_line":"    # NOTE(ldbragst): Build the original command wih the checked values"},{"line_number":275,"context_line":"    # in clean_args"}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAAQn%2F%2B620%3D","line":272,"in_reply_to":"AAAAQn%2F%2B7Ao%3D","updated":"2013-08-09 21:28:01.000000000","message":"Done","commit_id":"8c055fc2284fc778921ff17eea2f716da58fd0ca"},{"author":{"_account_id":6928,"name":"Ben Nemec","email":"openstack@nemebean.com","username":"bnemec"},"change_message_id":"a8f42df58bc3e2c1c35ba5b33cd06e87d4411f03","unresolved":false,"context_lines":[{"line_number":279,"context_line":""},{"line_number":280,"context_line":""},{"line_number":281,"context_line":"def _parameter_check(string_to_clean):"},{"line_number":282,"context_line":"    \"\"\"Here we are sanitizing a string to ensure it doesn\u0027t have characters"},{"line_number":283,"context_line":"    that might cause \u0027shell injection\u0027."},{"line_number":284,"context_line":""},{"line_number":285,"context_line":"    :param string_to_clean: string to sanitize"}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAAQn%2F%2B7L4%3D","line":282,"updated":"2013-08-09 20:50:23.000000000","message":"This docstring isn\u0027t really accurate now.  We\u0027re not doing any sanitization of the string (other than wrapping it in \u0027, I guess).  This is just checking for dangerous characters and throwing an exception.","commit_id":"8c055fc2284fc778921ff17eea2f716da58fd0ca"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"79ddeafd81c713c4a45a366fcbbc335bd5644c3f","unresolved":false,"context_lines":[{"line_number":279,"context_line":""},{"line_number":280,"context_line":""},{"line_number":281,"context_line":"def _parameter_check(string_to_clean):"},{"line_number":282,"context_line":"    \"\"\"Here we are sanitizing a string to ensure it doesn\u0027t have characters"},{"line_number":283,"context_line":"    that might cause \u0027shell injection\u0027."},{"line_number":284,"context_line":""},{"line_number":285,"context_line":"    :param string_to_clean: string to sanitize"}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAAQn%2F%2B62s%3D","line":282,"in_reply_to":"AAAAQn%2F%2B7L4%3D","updated":"2013-08-09 21:28:01.000000000","message":"Done","commit_id":"8c055fc2284fc778921ff17eea2f716da58fd0ca"},{"author":{"_account_id":8560,"name":"Sriram Subramanian","email":"sriram@sriramhere.com","username":"sriramhere"},"change_message_id":"48f6f134f921bc1bd9212e694d3732fe4e117a43","unresolved":false,"context_lines":[{"line_number":286,"context_line":"    :raises: PowerVMInvalidVIOSParameter"},{"line_number":287,"context_line":"    :returns: safe string/argument"},{"line_number":288,"context_line":"    \"\"\""},{"line_number":289,"context_line":"    for char in \u0027\\\u0027()$\\\\; \u0027:"},{"line_number":290,"context_line":"        # NOTE(ldbragst): This will ensure \u0027, (, ), $, \\, ;, or spaces are"},{"line_number":291,"context_line":"        # not allowed in values being passed to remote systems, preventing"},{"line_number":292,"context_line":"        # shell injection attacks."}],"source_content_type":"text/x-python","patch_set":12,"id":"AAAASX%2F%2F7B4%3D","line":289,"updated":"2013-08-23 17:57:22.000000000","message":"isn\u0027t it better to store these characters in an array to improve readability?","commit_id":"92002a744b6c3e05a4de84d1ca5cbe71ae7e4a62"},{"author":{"_account_id":5652,"name":"David Ripton","email":"dripton@redhat.com","username":"dripton"},"change_message_id":"0f349fb7bfa9ca7de5d008969e56f40b26d73767","unresolved":false,"context_lines":[{"line_number":286,"context_line":"    :raises: PowerVMInvalidVIOSParameter"},{"line_number":287,"context_line":"    :returns: safe string/argument"},{"line_number":288,"context_line":"    \"\"\""},{"line_number":289,"context_line":"    for char in \u0027\\\u0027()$\\\\; \u0027:"},{"line_number":290,"context_line":"        # NOTE(ldbragst): This will ensure \u0027, (, ), $, \\, ;, or spaces are"},{"line_number":291,"context_line":"        # not allowed in values being passed to remote systems, preventing"},{"line_number":292,"context_line":"        # shell injection attacks."}],"source_content_type":"text/x-python","patch_set":12,"id":"AAAASn%2F%2FbQE%3D","line":289,"in_reply_to":"AAAASX%2F%2F7B4%3D","updated":"2013-09-04 21:36:24.000000000","message":"I think it\u0027s fine to iterate over characters in a string.  Changing it to a list would just add a lot more noise with the extra quotes and commas.","commit_id":"92002a744b6c3e05a4de84d1ca5cbe71ae7e4a62"},{"author":{"_account_id":5652,"name":"David Ripton","email":"dripton@redhat.com","username":"dripton"},"change_message_id":"0f349fb7bfa9ca7de5d008969e56f40b26d73767","unresolved":false,"context_lines":[{"line_number":295,"context_line":"                       {\u0027char\u0027: char, \u0027string_to_check\u0027: string_to_check})"},{"line_number":296,"context_line":"            raise exception.PowerVMInvalidVIOSParameter(string_to_check)"},{"line_number":297,"context_line":""},{"line_number":298,"context_line":"    return \u0027\\\u0027\u0027 + string_to_check + \u0027\\\u0027\u0027"}],"source_content_type":"text/x-python","patch_set":12,"id":"AAAASn%2F%2FbO0%3D","line":298,"updated":"2013-09-04 21:36:24.000000000","message":"I would make this line return \"\u0027\" + string_to_check + \"\u0027\" to avoid the backslashes.","commit_id":"92002a744b6c3e05a4de84d1ca5cbe71ae7e4a62"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"04530a9bc56d8bb1205868db6e7f097ba25d1ade","unresolved":false,"context_lines":[{"line_number":295,"context_line":"                       {\u0027char\u0027: char, \u0027string_to_check\u0027: string_to_check})"},{"line_number":296,"context_line":"            raise exception.PowerVMInvalidVIOSParameter(string_to_check)"},{"line_number":297,"context_line":""},{"line_number":298,"context_line":"    return \u0027\\\u0027\u0027 + string_to_check + \u0027\\\u0027\u0027"}],"source_content_type":"text/x-python","patch_set":12,"id":"AAAASn%2F%2FWcw%3D","line":298,"in_reply_to":"AAAASn%2F%2FbO0%3D","updated":"2013-09-05 21:36:06.000000000","message":"Done","commit_id":"92002a744b6c3e05a4de84d1ca5cbe71ae7e4a62"},{"author":{"_account_id":7996,"name":"Luis A. Garcia","email":"luis@linux.vnet.ibm.com","username":"luisg"},"change_message_id":"563c3a71ab110937f34c7ee3bacf0972b05dc6ab","unresolved":false,"context_lines":[{"line_number":258,"context_line":"        cleanup_key_on_source()"},{"line_number":259,"context_line":""},{"line_number":260,"context_line":""},{"line_number":261,"context_line":"def command_builder(cmd, *args):"},{"line_number":262,"context_line":"    \"\"\"Here we are building each command that gets sent to a remote system"},{"line_number":263,"context_line":"    to ensure security."},{"line_number":264,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"AAAAS3%2F%2FwJs%3D","line":261,"updated":"2013-09-19 22:38:58.000000000","message":"command_builder sounds more like a class name, than a method name. Should we call this build_command()","commit_id":"f7ea65ef3a33494e06c581d904debadf691e7b00"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"a7324099ed61e66cf8fee3bd52d19d1b5d8d8ae3","unresolved":false,"context_lines":[{"line_number":258,"context_line":"        cleanup_key_on_source()"},{"line_number":259,"context_line":""},{"line_number":260,"context_line":""},{"line_number":261,"context_line":"def command_builder(cmd, *args):"},{"line_number":262,"context_line":"    \"\"\"Here we are building each command that gets sent to a remote system"},{"line_number":263,"context_line":"    to ensure security."},{"line_number":264,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"AAAAS3%2F%2Fv%2Bg%3D","line":261,"in_reply_to":"AAAAS3%2F%2FwJs%3D","updated":"2013-09-20 00:14:10.000000000","message":"Done","commit_id":"f7ea65ef3a33494e06c581d904debadf691e7b00"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"90ca38e3637482a0d709b75cbb1458951a41c560","unresolved":false,"context_lines":[{"line_number":293,"context_line":"        if char in string_to_check:"},{"line_number":294,"context_line":"            LOG.error(_(\"Invalid parameter: %(char)s in %(string_to_check)s\"),"},{"line_number":295,"context_line":"                       {\u0027char\u0027: char, \u0027string_to_check\u0027: string_to_check})"},{"line_number":296,"context_line":"            raise exception.PowerVMInvalidVIOSParameter(string_to_check)"},{"line_number":297,"context_line":""},{"line_number":298,"context_line":"    return \"\u0027\" + string_to_check + \"\u0027\""}],"source_content_type":"text/x-python","patch_set":14,"id":"AAAAS3%2F%2FE1E%3D","line":296,"updated":"2013-10-04 20:00:06.000000000","message":"This is an error-prone approach that is all but guaranteed to bite you on the butt when you aren\u0027t looking.  What you want to do is filter for allowed characters, rather than filtering out disallowed characters.  That is obviously harder to do, but you could do something like:\n\n    ALLOWED_CHARS \u003d re.compile(r\u0027[\\s\\w…-]\u0027)\n\n    if not ALLOWED_CHARS.match(string_to_check):\n        raise exception…","commit_id":"08f0f90a6806b11d9517d3207487d39de2f4bed0"},{"author":{"_account_id":1561,"name":"Russell Bryant","email":"rbryant@redhat.com","username":"russellb"},"change_message_id":"4aea30e0228533d5e405af420f8a11d00aaffcfb","unresolved":false,"context_lines":[{"line_number":293,"context_line":"        if char in string_to_check:"},{"line_number":294,"context_line":"            LOG.error(_(\"Invalid parameter: %(char)s in %(string_to_check)s\"),"},{"line_number":295,"context_line":"                       {\u0027char\u0027: char, \u0027string_to_check\u0027: string_to_check})"},{"line_number":296,"context_line":"            raise exception.PowerVMInvalidVIOSParameter(string_to_check)"},{"line_number":297,"context_line":""},{"line_number":298,"context_line":"    return \"\u0027\" + string_to_check + \"\u0027\""}],"source_content_type":"text/x-python","patch_set":14,"id":"AAAATH%2F%2F3l4%3D","line":296,"in_reply_to":"AAAAS3%2F%2FE1E%3D","updated":"2013-10-08 18:04:01.000000000","message":"I actually think what you want is to ensure arguments are actually treated as an argument and not interpreted by the shell.  We should just call pipes.quote() on every argument.\n\nhttp://docs.python.org/2/library/pipes.html","commit_id":"08f0f90a6806b11d9517d3207487d39de2f4bed0"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"186c5f5a6150adcd66a6ccaa1e080f4401c83784","unresolved":false,"context_lines":[{"line_number":293,"context_line":"        if char in string_to_check:"},{"line_number":294,"context_line":"            LOG.error(_(\"Invalid parameter: %(char)s in %(string_to_check)s\"),"},{"line_number":295,"context_line":"                       {\u0027char\u0027: char, \u0027string_to_check\u0027: string_to_check})"},{"line_number":296,"context_line":"            raise exception.PowerVMInvalidVIOSParameter(string_to_check)"},{"line_number":297,"context_line":""},{"line_number":298,"context_line":"    return \"\u0027\" + string_to_check + \"\u0027\""}],"source_content_type":"text/x-python","patch_set":14,"id":"AAAATn%2F%2FFOs%3D","line":296,"in_reply_to":"AAAATH%2F%2F3l4%3D","updated":"2013-11-14 16:46:34.000000000","message":"Hey, thanks for the comments! I believe using pipes.quote was suggested, but we didn\u0027t go for it since it wasn\u0027t publicly documented for Python2.7.\n\nhttps://review.openstack.org/#/c/36078/2/nova/virt/powervm/common.py\n\nYou can also see Ben Nemec\u0027s comment from July 9th. I would be more than happy to switch the implementation to pipes.quote if that is the general consensus.","commit_id":"08f0f90a6806b11d9517d3207487d39de2f4bed0"}]}