SNI support using Python requests for .url

[anarcat]

without this, SNI-enabled sites, which are becoming more and more
popular, are not displayed by the URL plugin

a good site to test with is: https://sni.velox.ch/

the requests API is similar enough to the web.get API to replace it,
but that is left to another pull request, as other plugins may not
require SNI support because they probably don't encounter the same
variety of sites as .url

[anarcat]

a sample run: peoplesmic is using web.get, wildcat is using requests.

18:10:05 <@anarcat> https://sni.velox.ch/
18:10:07 <wildcat> [ TLS SNI Test Site: *.sni.velox.ch ] - sni.velox.ch
18:10:13 <@anarcat> https://koumbit.org/
18:10:14 <wildcat> [ Koumbit.org | Pour un internet libre et solidaire ] - koumbit.org
18:10:17 <@anarcat> https://anarc.at/
18:10:17 <peoplesmic> [ À propos de moi ] - anarc.at
18:10:18 <wildcat> [ À propos de moi ] - anarc.at
18:10:19 <peoplesmic> [ Koumbit.org | Pour un internet libre et solidaire ] - koumbit.org
18:10:22 <@anarcat> https://groente.puscii.nl/test.html
18:10:23 <wildcat> [ '); drop tables;-- ] - groente.puscii.nl

notice how the older code is a solid (and consistent, during tests) 5 seconds slower than the requests equivalent, for https://koumbit.org. the discrepency is different for https://anarc.at/ because it's actually on the locall network.

[maxpowa]

What about editing web.py to use requests instead of replacing web.py? Does this use the CA certs defined in the config file or system CA certs?

Edit: This also appears to remove the functionality that limited the amount of bytes to download, so does this now download as much as the page serves? This could be very problematic, especially when linking large files/pages.

Edit2: nevermind my edit. What if the url is not text?

[anarcat]

1. i thought i had less of a chance to make web.py use requests than to change just this one plugin, but i'd be happy to oblige
2. i believe it would use the system CA certs, but that can be changed with verify=ca_certs i think. maybe an oversight on my part...
3. i assumed the URL is text, and i believe it is an assumption shared with the current code

[anarcat]

as for the max_bytes part, i believe this is actually faster than the previous version, because it will stop after a few lines instead of reading the whole 640kb of the page...

[anarcat]

re 2. i don't quite understand why the IRC bot needs to define his own CA list. why not use the system ones?

... yet if that would be necessary, i believe the following patch would be sufficient:

diff --git a/sopel/modules/url.py b/sopel/modules/url.py
index 0d7aac5..41fd1c2 100644
--- a/sopel/modules/url.py
+++ b/sopel/modules/url.py
@@ -181,7 +181,7 @@ def check_callbacks(bot, trigger, url, run=True):

 def find_title(url):
     """Return the title for the given URL."""
-    response = requests.get(url, stream=True)
+    response = requests.get(url, stream=True, verify=ca_certs)
     try:
         content = ''
         for line in response.iter_lines(decode_unicode=True):

as an aside, ca_certs seems like a weird global to me... it's not in any imports in web.py?

[embolalia]

I'll take a more detailed look at the PR later, but I just wanted to answer a few points real quick:

• Rather than using requests in sopel.web, we're going to deprecate it and suggest using requests instead. The module is kind of messy anyway, and there's no need to have a wrapper around an already pretty good API. (This has already been talked about here and there, and is a longer term plan, but just hasn't been acted on because it's somewhat low priority.)
• The ca_certs config setting is there because there's no reliable standardization on the location. It gets set somewhere in the bot's startup (when it's reading the config). Yes, this is hideous. But at least at the time, there was no cross-platform solution that didn't involve a hard dependency for core functionality (which we've been trying to avoid).
• The current code does not assume the content is text for size limiting. It reads howevermany bytes (not characters), stops, and then assumes the bytes are text. If I'm reading it correctly, what your code is doing will require there to be an occasional 0x0A byte, or it will never hit the limit. What if it's running on a Raspberry Pi, and someone posts a link to a 2GB video file?

[anarcat]

On 2016-01-06 08:08:52, Elsie Powell wrote:

I'll take a more detailed look at the PR later, but I just wanted to answer a few points real quick:

• Rather than using requests in sopel.web, we're going to deprecate
  it and suggest using requests instead. The module is kind of messy
  anyway, and there's no need to have a wrapper around an already pretty
  good API. (This has already been talked about here and there, and is a
  longer term plan, but just hasn't been acted on because it's somewhat
  low priority.)

Cool. This is what I was hoping would happen. I was also hoping this
change would show a good way forward on how to proceed with this.

In my opinion, there's no good reason why an IRC bot should have its own
HTTP library when requests is around in Python. :) At least now that
requests has significantly stablised.

• The ca_certs config setting is there because there's no reliable
  standardization on the location. It gets set somewhere in the bot's
  startup (when it's reading the config). Yes, this is hideous. But at
  least at the time, there was no cross-platform solution that didn't
  involve a hard dependency for core functionality (which we've been
  trying to avoid).

requests had exactly the same problem. they made the certifi package for
that exact purpose, after ripping it out of requests:

http://docs.python-requests.org/en/latest/user/advanced/#ca-certificates

basically, with requests, this is abstracted away to the distribution
(debian package of requests in my case or pypi dependencies in the
general case).

so basically, the deprecation of sopel.web would also deprecate
ca_certs.

• The current code does not assume the content is text for size
  limiting. It reads howevermany bytes (not characters), stops, and
  then assumes the bytes are text. If I'm reading it correctly, what
  your code is doing will require there to be an occasional 0x0A byte,
  or it will never hit the limit. What if it's running on a Raspberry
  Pi, and someone posts a link to a 2GB video file?

iter_lines has its own chunk size, unspecified here, but that defaults
to 512 bytes. in other words, i believe iter_lines reads the minimum
number of bytes between 512 bytes and "whatever i need to read up to a
newline". so basically, we read 512 bytes chunks, looking for a </title>
tag, but stop after 640kbytes.

so yes, this will use up to 640kbytes of ram. it seemed rather high when
i was reading the code, especially considering the reference to all
the memory of original PCs. ;) i'd be happy to reduce that to a more
reasonable number, say like 128KB or 64KB (you guys have a quote from
Wozniak on that one? ;)

see:

http://docs.python-requests.org/en/latest/api/#requests.Response.iter_lines
http://docs.python-requests.org/en/latest/user/advanced/#streaming-requests
https://en.wikipedia.org/wiki/Conventional_memory
https://en.wikiquote.org/wiki/Talk:Bill_Gates
http://www.computerworld.com/article/2534312/operating-systems/the--640k--quote-won-t-go-away----but-did-gates-really-say-it-.html

... and so on.

a.

During times of universal deceit, telling the truth becomes a
revolutionary act. - Georges Orwell

[maxpowa]

I don't know enough about requests, but iter_lines attempts to decode to a string right? When we are looking at a 2GB video file, it wouldn't be able to decode anything. You'd never hit your limiting and you'd always download the entire file, from what I understand at least.

[anarcat]

that is not what i read from the documentation.

but, only the source code would tell you that, right? in fact, reading the source code, iter_lines uses iter_content which in turn calls urllib3 (response.raw is a urllib3.response object):

https://github.com/kennethreitz/requests/blob/master/requests/models.py#L692
https://github.com/kennethreitz/requests/blob/master/requests/models.py#L645
https://github.com/kennethreitz/requests/blob/046a0f215de3fac60326ea1c2c9dd30beb329f61/requests/packages/urllib3/response.py#L263

then that reads at most the bytes specified and decodes the result. it won't read the whole 2GB file just to see if it can decode it better.

so i don't think those concerns are warranted. but then you can make a unit test to see if that's really a problem. in my tests, as i said, the requests version is faster than the native one, which can only be explained by the fact that it really does read only 512 bytes at a time, because otherwise the requests overhead would make it slower, not faster.

[maxpowa]

No I'm not saying it wouldn't do the 512 chunks, I'm saying it would do every 512 chunk. You'd still end up downloading 2GB on your network, rather than just 640kb.

[anarcat]

wouldn't this catch such a case? wouldn't len(content) > max_bytes?

[anarcat]

i guess i don't see the code path you are refering to. i mean if you are worried that iter_lines() just never returns until it read beyond 640KB, that is one case. i don't think it will happen, from reading the requests code. now if you think my code will loop beyond 640KB, that's a completely different case, yet i still don't see how it could happen. would len(content) be zero?

could you clarify the code path or provide a test case?

[maxpowa]

Just tested, I was thinking that something like http://mirror.internode.on.net/pub/test/1meg.test would cause it to download the entire thing (because len(content) would be 0), but it just fails out with the UnicodeDecodeError.

[elad661]

If we use requests we can limit the amount of data downloaded, according to this stackoverflow link https://stackoverflow.com/questions/22346158/python-requests-how-to-limit-received-size-transfer-rate-and-or-total-time

[anarcat]

On 2016-01-11 08:47:18, Elad Alfassa wrote:

If we use requests we can limit the amount of data downloaded, according to this stackoverflow link https://stackoverflow.com/questions/22346158/python-requests-how-to-limit-received-size-transfer-rate-and-or-total-time

This is essentially what the my code is proposing, except the example
above uses iter_content() instead of iter_lines() and also a timeout.

a.

Freedom is being able to make decisions that affect mainly you. Power
is being able to make decisions that affect others more than you. If
we confuse power with freedom, we will fail to uphold real freedom.
- Richard Stallman

[anarcat]

from the 6.2 release notes:

sopel.web is now deprecated in favor of the third-party requests library

so what's up here? is there anything i can do here to push this ahead?

[elad661]

So my hunch is that iter_lines will look for \n, if you happen to download binary content, you might not see any \n in the file at all, and it will just keep downloading. This is why I think iter_content is safer.

I still want this pull request merged, because we need to port away from web.py to requests, but I'll have to test it to see if my hunch is correct, and think of a way to fix this if it's not.

[anarcat]

i thought i made it pretty clear in #988 (comment) that iter_lines calls iter_contents so it's all the same:

    def iter_lines(self, chunk_size=ITER_CHUNK_SIZE, decode_unicode=None, delimiter=None):
        # [...]
        for chunk in self.iter_content(chunk_size=chunk_size, decode_unicode=decode_unicode):
             # [...] line splitting...
            for line in lines:
                yield line

so if you have a large binary file, the code path will be:

1. call iter_content(ITER_CHUNK_SIZE), that is, download 512 bytes
2. split the result on newlines into a list of lines
3. yield the list of lines

so if you call iter_lines() in an iterator context (which I am doing), it will not download the whole file. it would if you would do list(iter_lines()) but that would be silly. furthermore, i explicitely check the size of the chunks accumulated and abort after our own limit:

            if '</title>' in content or len(content) > max_bytes:

so you're welcome to follow that code path again, but i'm pretty confident it's doing the right thing.

[elad661]

Okay, got it. I'll run a few tests just to make sure, and if it works well I'll merge it.

[anarcat]

great, thanks! testing would be easier, btw, if the url module would talk when it doesn't find a title. :) just by the speed of the response you could tell if it's downloading the whole file or not... users in the channel often complain that they feel the bot is dead when it doesn't reply when they post a URL... most often it's because of SNI, but sometimes it's PDFs and people are surprised the bot is silent...

[elad661]

in the past, it did, but it annoyed people when they posted links for things that can't be handled by this module, so we silenced it.

So now when working on the url module the best thing to do is to add debug prints while you're working, and remove them before committing.

[elad661]

So far my main concern with this is that requests doesn't seem to support non-unicode stuff when the content-type is only defined within the html file itself. It makes sense for them, because they're an http library, not an html one. It tries to decode everything as unicode even if the Content-Type header doesn't specify a charset.

I'll try to find a solution.

[elad661]

Oh. Looks like we didn't support this specific case with web.py either. I just wasted an hour researching it before realizing. Oops.

Anyway, the iter_lines solution is not good enough. On a binary file with not a lot of newline the result is that the thread hangs and the bot uses 100% CPU. We clearly don't want that.

This probably happens because iter_lines keeps looking for newlines in iter_content.

If we want to iterate over lines, we have two options: either implement iter_lines ourselves with the limit built in instead of using the existing requests code, or monkey patch iter_content to stop after max_bytes read. I'm not sure which is cleaner, but whatever we do we probably can't copy code from requests because of licensing issues.

Alternatively, we can just read the content until max_bytes is reached, and then split that by lines and process them. This is obviously easier, and I don't think the performance penalty will be too bad.

@embolalia what do you think?

[elad661]

Merged with a bugfix (not using iter_lines), squashed two commits into one, and reworded the commit message (since SNI is no longer the reason we wanted this in).

Unfortunately because I squashed the commits and reworded them, github doesn't know I just merged it, but I did.

4cceef6

Thanks!