Icons on local filesystem about gntp HOT 7 CLOSED

I can try that or I could also try http://docs.python.org/2/library/urlparse.html since it should also take care of file paths.

from gntp.

Good thought; urlparse seems like a smart approach.

from gntp.

I just remembered this issue after upgrading to the latest version and having local icons broken.

Having read the GNTP protocol docs, and reviewing more of the code, it looks like the gntp.notifier._checkIcon method needs a change. I'd like to get a fix in for the next version and will be happy to do a pull request.

The GNTP protocol doesn't have any explicit restrictions on the URL scheme (http/https/file, etc.) for icons. It does allow use of embedded binary resources, which look like a pain to implement, and based on the docstring, it looks like the _checkIcon method is intended to determine whether the icon is a URL or an embedded resource, so it can ignore embedded icons entirely.

According to the GNTP docs, an embedded resource URL must be in the following format: x-growl-resource://<uniquevalue>. I'd propose one of the following options to detect/reject embedded icons:

• Option 1: reject any icon URL that starts with or has a scheme of 'x-growl-resource' and pass any other schemes, allowing the growl client to handle anything else
• Option 2: allow only icon URLs with 'http' or 'file' as schemes, rejecting any other scheme

Sub-options are whether to perform any logging on a rejected icon scheme, and whether to use the urlparse library to extract the scheme or just use 'startswith'. The urlparse method can break out the scheme (http, https, file, etc.) from a provided url, and supply a default (http) if one isn't provided, but doesn't know if a particular scheme is 'valid,' so if the only schemes you want to allow are http/https/file, startswith is the easier option.

Let me know your preferences, and I'll add a pull request.

Preview suggested change

from gntp.

Wow, 2013 and I still haven't fixed this. How embarrassing >_<
I believe my initial idea was to use urlparse because that seemed like it would be the most flexible since I assumed people passing in an icon would either be a path (to a file or url) or it would be the data for the icon itself.

        if icon:
            if self._checkIcon(icon):
                notice.add_header('Notification-Icon', icon)
            else:
                resource = notice.add_resource(icon)
                notice.add_header('Notification-Icon', resource)

I don't know why I named the function _checkIcon, but since it's an internal function, we could also rename it to make it more descriptive. I wonder if we could do something like this

try:
   return urlparse.urlparse(icon).scheme in ['http', 'https', 'file']

I'm usually a bit busy during the week so I don't have the time right now to test it. Sorry again for leaving this open for so long :/

from gntp.

I think I also need to do a better job of handling unicode vs bytes for the python 2 & 3 compatibility to make some of the code less ugly to look at. Right now I think I have too many instances of gntp.shim.u(data) and gntp.shim.b(data) floating around

from gntp.

No worries. It was such a simple change I'd done it to my install a long time ago. Just recently did some migration and it reminded me when the icons stopped working. I use the library all the time.

Given what _checkIcon actually does, you're right that it probably should be renamed, or maybe just return the formatted URL or resource for the Notification-Icon header directly, depending on the type:

if icon:
    notice.add_header('Notification-Icon', self._format_icon(icon))

Let me know if you want any help or other testing.

from gntp.

I like the idea of formatting it that way, but I think I may need to format it slightly differently. I'll see if I can get this sorted out this weekend since it's a simple change and I should have done this a LONG time ago ^^;

from gntp.