develop salt.utils.sanitize_win_path_string breaks state.apply/highstate on windows

[damon-atkins]

Description of Issue/Question

Setup

salt minion on windows apply a state or highstate.

    Data failed to compile:
----------
    Traceback (most recent call last):
  File "c:\Salt-Dev\salt\salt\state.py", line 3644, in call_highstate
    top = self.get_top()
  File "c:\Salt-Dev\salt\salt\state.py", line 3101, in get_top
    tops = self.get_tops()
  File "c:\Salt-Dev\salt\salt\state.py", line 2799, in get_tops
    saltenv
  File "c:\Salt-Dev\salt\salt\fileclient.py", line 193, in cache_file
    return self.get_url(path, u'', True, saltenv, cachedir=cachedir)
  File "c:\Salt-Dev\salt\salt\fileclient.py", line 512, in get_url
    result = self.get_file(url, dest, makedirs, saltenv, cachedir=cachedir)
  File "c:\Salt-Dev\salt\salt\fileclient.py", line 1058, in get_file
    path, senv = salt.utils.url.split_env(path)
  File "c:\Salt-Dev\salt\salt\utils\url.py", line 136, in split_env
    path, senv = parse(url)
  File "c:\Salt-Dev\salt\salt\utils\url.py", line 43, in parse
    path = salt.utils.sanitize_win_path_string(path)
  File "c:\Salt-Dev\salt\salt\utils\__init__.py", line 1396, in sanitize_win_path_string
    winpath = winpath.translate(trantab)
TypeError: character mapping must return integer, None or unicode

My preference would be to see this function removed, or raise an error. Also note trantab ends up being a extremely long string, could be all of the unicode chars.

If it must be kept try

def sanitize_win_path_string(winpath): 
    return re.sub(u'[\/:*?"<>|]',u'_',winpath,flags=re.UNICODE)

I add code to print the winpath value and type. When value passed is Unicode it breaks.

2017-08-18 21:46:49,667 [salt.utils       .sanitize_win_path_string][ERROR   ] top.sls
2017-08-18 21:46:49,667 [salt.utils       .sanitize_win_path_string][ERROR   ] <type 'str'>
2017-08-18 21:47:01,229 [salt.utils       .sanitize_win_path_string][ERROR   ] top.sls
2017-08-18 21:47:01,229 [salt.utils       .sanitize_win_path_string][ERROR   ] <type 'unicode'>  <-Causes the issue

I have raise this just encase some else comes across it.

Also interest it top.sls starts as a PY2 string and then becomes PY2 unicode latter on. (I assume it will be fixed when all general strings have been prefixed with u across all python files)

Versions Report

develop on windows as of 2017-08-17

[gtmanfred]

Just to make sure I know where the error is, if you switch the if statement in that to check if isinstance str instead of six.string_types. I think the problem is where we changed str to six.string_types

I wonder if it is also broken on py3 and needs to be six.binary_types.

Thanks, Daniel

[damon-atkins]

translate does not like the contents of trantab when applied against a unicode string within winpath

>>> import string
>>> trantab=string.maketrans(u'<>:|?*', u'______')
>>> print len(trantab)
256
>>> trantab
'\x00\x01\x02\x03\x04\x05\x06\x07\x08\t\n\x0b\x0c\r\x0e\x0f\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f !"#$%&\'()_+,-./0123456789_;_=__@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{_}~\x7f\x80\x81\x82\x83\x84\x85\x86\x87\x88\x89\x8a\x8b\x8c\x8d\x8e\x8f\x90\x91\x92\x93\x94\x95\x96\x97\x98\x99\x9a\x9b\x9c\x9d\x9e\x9f\xa0\xa1\xa2\xa3\xa4\xa5\xa6\xa7\xa8\xa9\xaa\xab\xac\xad\xae\xaf\xb0\xb1\xb2\xb3\xb4\xb5\xb6\xb7\xb8\xb9\xba\xbb\xbc\xbd\xbe\xbf\xc0\xc1\xc2\xc3\xc4\xc5\xc6\xc7\xc8\xc9\xca\xcb\xcc\xcd\xce\xcf\xd0\xd1\xd2\xd3\xd4\xd5\xd6\xd7\xd8\xd9\xda\xdb\xdc\xdd\xde\xdf\xe0\xe1\xe2\xe3\xe4\xe5\xe6\xe7\xe8\xe9\xea\xeb\xec\xed\xee\xef\xf0\xf1\xf2\xf3\xf4\xf5\xf6\xf7\xf8\xf9\xfa\xfb\xfc\xfd\xfe\xff'
>>> print type(trantab)
<type 'str'>
>>>

The regex is a much cleaner solution. It seems this function is only used three times.

/salt/fileclient.py:            netloc = salt.utils.sanitize_win_path_string(url_data.netloc)
url.py as a fair bit of if windows coding here
/salt/utils/url.py:        path = salt.utils.sanitize_win_path_string(path)
/salt/utils/url.py:        path = salt.utils.sanitize_win_path_string(path)

[gtmanfred]

Great, would you mind submitting a PR for that?

Thanks, Daniel

[Ch3LL]

Is this good to be closed?

[damon-atkins]

salt.utils.sanitize_win_path_string()is not using the new safe_filepath() yet. I have not had a chance to check what the path separator will be in all cases, which I suspect it will be Unix/URL one all the time. safe_filepath() defaults to the OS path separator it supports any separator.

Unfortunately I do not have the time to look into this further, with the due care required in replacing or altering salt.utils.sanitize_win_path_string() with safe_filepath() which is basically check what the directory separator needs to be.

safe_filename_leaf() is already in use in other parts of salt code.

[twangboy]

@damon-atkins Are you still seeing this error? I am unable to replicate this. I just applied a state and did not see the stack trace.

Does this happen on all states or a state you're using? Does it only happen on highstates?

[damon-atkins]

@twangboy, I think you updated salt.utils.sanitize_win_path_string() to fix it.

safe_filename_leaf(file_basename) is now used in a critical bit of salt code pushed into 2017, changes character which any file system does not like to URL encoded. Idea being its safe on any file system, rather per filesytem per OS approach.

safe_filepath() splits the path and calls safe_filename_leaf() for each part of the path.

However I suggest you change it to safe_filepath() which has the same result on all platforms, you can remove the if windows salt.utils.sanitize_win_path_string code if there is any.

Found it #44792

[twangboy]

@damon-atkins If I understand this correctly, we would change all references to salt.utils.path.sanitize_win_path to salt.utils.files.safe_filepath but we'll also need to pass the path separator. Sometimes that would be \ in Windows but there are cases where it's passing a url so / would be appropriate. You can't just pass os.sep and be good.

I found references to salt.utils.path.sanitize_win_path in 3 files:

• salt\fileclient.py
• salt\utils\__init__.py
• salt\utils\url.py

I don't think we should try to refactor this for the Oxygen release.

[damon-atkins]

Two options label this for the next release, or create a new one.... What should we do?

[twangboy]

Let's create a new one as this issue is fixed.