search

OpenNTI / nti.contentrendering

Other
0 stars 0 forks source link

Make sure we use a encoded safe file name based on (possibly) user input #42

Closed jzuech3 closed 6 years ago

jzuech3 commented 6 years ago

We did not handle unicode titles here. Also, talking with Jason, we should take precautions against using user-created text in file-handling.

jzuech3 commented 6 years ago

@jamadden Thanks for the feedback. Are my latest changes good enough for the release today?

jamadden commented 6 years ago

@jzuech3 If I had my druthers, I would like to see some unittest cases for _get_safe_icon_filename because there are several branches. And if it were me, I would probably roll the "default" handling into that function so that it can also be tested independently of the rest of the code. Then the only change to the main body of the code would be calling that function and the changes to os.path, all of which don't involve any branches and so don't really need new test coverage.

def  _get_safe_icon_filename(filename, default):
   assert default and isinstance(default, bytes)
   if filename and isinstance(filename, bytes):
      return filename
   ...
   if result:
     return base64.urlsafe_b64encode(result)
   return default
...
filename = _get_safe_icon_filename(title, b'book')

But I understand if that's not in the cards right now.

jzuech3 commented 6 years ago

Good points. I'm a bit hard-pressed for time today.

There was another change I made in nti.contentlibrary_rendering (r125027) to support non-ascii characters, similar to these changes in contentrendering. r125027 causes a downstream error in plasTeX when urlquoting this unicode field. I need to track that down.

jamadden commented 6 years ago

It would not surprise me at all if plasTeX can't handle non-ascii characters in identifiers. That's a fairly recent feature in TeX and you need to use a special distribution (LuaTeX or XeTeX) to use them. This may be a place for the 'replace' error handler.