Issue644

Title 'Random Issue' Button isn't working
Priority bug Status resolved
Superseder Nosy List berker.peksag, csabella, ezio.melotti, ncoghlan, rouilj
Assigned To Topics

Created on 2017-10-23.23:58:33 by csabella, last changed 2018-07-08.15:29:10 by berker.peksag.

Messages
msg3407 (view) Author: csabella Date: 2017-10-23.23:58:32
The 'Random Issue' button used to show a new issue every time it was clicked, but for the past few weeks, the issue returned only changes once a day.
msg3414 (view) Author: berker.peksag Date: 2017-11-05.15:57:28
From http://www.roundup-tracker.org/docs/customizing.html#extensions-adding-capabilities-to-your-tracker

    All files from this dir are loaded when tracker instance is created, [...]

It's not clear to me when this was added, but it seems like this is the cause of the bug.

I added a global counter in pydevutils.py to test this and it didn't work.

Ezio, is there a way to solve this bug?
msg3415 (view) Author: berker.peksag Date: 2017-11-09.19:54:47
Hi John, do you know a workaround to make the random issue extension work again?
msg3417 (view) Author: rouilj Date: 2017-11-11.02:59:39
Hi Berker:

In message <1510257288.61.0.213398074469.issue644@psf.upfronthosting.co.za>,
Berker Peksag writes:
>Hi John, do you know a workaround to make the random issue
>extension work again?

I pulled the source for the bugs.python.org tracker and took a look.

The function I got to work better in my demo tracker was:

import random
from roundup.cgi.actions import Action
from roundup.cgi.exceptions import Redirect

class RandomIssueAction(Action):
    def handle(self):
        """Redirect to a random open issue."""
        issue = self.db.getclass('issue')
        issue_ids = issue.filter(None, {'status': 1})
        random.seed()
	url = self.db.config.TRACKER_WEB + 'issue' + random.choice(issue_ids)
        raise Redirect(url)

def init(instance):
    instance.registerAction('random', RandomIssueAction)

I changed:

   the way I got the handle to issues from the db
   the way the list of id's in my tracker instance

I was getting tracebacks otherwise. Since you do get redirected to a
page on bugs.python.org you obviously aren't getting traceback so this
new function may not work.

Originally triggering the random action I saw a lot of repeats. My
thought was that the random function wasn't so random. The new 1.5.1+
(what will be 1.6) roundup uses more random data than 1.4.20. Addition
of nonces to protect against csrf etc. consumes random data.

So I added random.seed(). This seemed to increase the randomness of the
function. You may want to try adding random.seed() to your existing
function and see if that helps.

Even after seeding, I still saw duplicates, but there were only 20
issues to choose from. In your case you have 6000+ issues to select
from duplicates by chance are reduced.
msg3418 (view) Author: berker.peksag Date: 2017-11-11.05:05:04
Thank you for your quick response and for your detailed analysis, John.

I will attach a patch to implement your suggestions.

> My thought was that the random function wasn't so random. The new 1.5.1+
> (what will be 1.6) roundup uses more random data than 1.4.20. Addition
> of nonces to protect against csrf etc. consumes random data.

Should I open an upstream issue to document this at http://roundup.sourceforge.net/docs/upgrading.html ?
msg3419 (view) Author: rouilj Date: 2017-11-11.15:32:03
Hello Berker:

In message <1510376704.3.0.213398074469.issue644@psf.upfronthosting.co.za>,
Berker Peksag writes:
>Thank you for your quick response and for your detailed analysis, John.
>
>I will attach a patch to implement your suggestions.
>
>> My thought was that the random function wasn't so random. The new 1.5.1+
>> (what will be 1.6) roundup uses more random data than 1.4.20. Addition
>> of nonces to protect against csrf etc. consumes random data.
>
>Should I open an upstream issue to document this at
> http://roundup.sourceforge.net/docs/upgrading.html ?

Can you try my patch with and without the random.seed() call and see
if it makes a difference on bugs.python.org.

If the patch without random.seed() works then there is something
different happening on the b.p.o tracker and my test tracker.

As I said the code I got from your tracker wouldn't run at all in my
installation (context was always None, _klass wasn't a valid property
etc.). Given that I had to do surgery to even make it not crash I
wonder if there is something else in the code base that I am missing.

How the b.p.o tracker executed?  I run roundup as a stand alone
daemon with a web server front end proxying to the roundup
instance. How is b.p.o configured?  CGI, stand alone daemon, wsgi,
mod_python, zope?

I wonder if that makes a difference.
msg3420 (view) Author: rouilj Date: 2017-11-11.21:03:42
Hi all:

In message <20171111153201.827854C03FF@itserver6.localdomain>,
John Rouillard writes:
>In message <1510376704.3.0.213398074469.issue644@psf.upfronthosting.co.za>,
>Berker Peksag writes:
>>Thank you for your quick response and for your detailed analysis, John.

Well it gets more wierd.

>>I will attach a patch to implement your suggestions.
>>
>>> My thought was that the random function wasn't so random. The new 1.5.1+
>>> (what will be 1.6) roundup uses more random data than 1.4.20. Addition
>>> of nonces to protect against csrf etc. consumes random data.
>>
>>Should I open an upstream issue to document this at
>> http://roundup.sourceforge.net/docs/upgrading.html ?

Hmm, seems roundup is borking something in the random module.

I created a default tracker using ./demo.py in a current checkout of
the roundup tree.

I added 30 issues and my implementation of your random action.
I also added: 

   print random.random()
   print random.random()

inside the handler. I get the same sequence every time:

  0.8774733181
  0.652775856602

I access the http://...?@action=randomurl. That's not right

Every import of random is supposed to be seeded with the current time
or some os specific random data every time. Then once it's seeded the
same instance of random should be updating the random state so the
next call to random produces the next random number.

AFAICT, what I see should be happening only if something in the code
is calling random.seed() with the same value every time the url is
accessed.

However even when I comment out all of the seed() calls in the roundup
code (all located in roundup/cgi/client.py) I get the same results as
above. So there are no calls to random.seed in the roundup code base.

Restarting the server by rerunning ./demo.py generates a new
sequence with different values.

Shoving a random.seed() in the handler function is merely covering up
something that is broke.

I'll keep poking at it to try to figure out why random seems to be
losing state, but I am stumped here.
msg3451 (view) Author: rouilj Date: 2018-06-01.00:41:56
Hi all:

I took another look at this. I suggest replacing the current class
with:
class RandomIssueAction2(Action):
    def handle(self):
        """Redirect to a random open issue."""
        issue = self.context['context']
        # use issue._klass to get a list of ids, and not a list of instances    
        issue_ids = issue._klass.filter(None, {'status': '1'})
        random.seed()
        url = self.db.config.TRACKER_WEB + 'issue' + random.choice(issue_ids)
        raise Redirect(url)

two changes, the addition of the random.seed() call and change {'status': 1}
to {'status': '1'}. This works for my trackers. If I use the original
{'status': 1} value I get no id's back. The addition of the seed makes the
randomness return.

Also note that this function will throw a traceback if there are no issues
matching the reference criteria. Adding a try/catch or checking the size of
the issue_ids list is left as an exercise for the reader 8-).

-- rouilj
msg3453 (view) Author: berker.peksag Date: 2018-06-01.04:11:43
Thank you for taking a look at this again, John. I will incorporate your suggestions and prepare a patch this weekend.
msg3454 (view) Author: berker.peksag Date: 2018-06-02.05:53:42
I've just committed John's suggested approach in https://hg.python.org/tracker/python-dev/rev/9e1d65b4927c but it needs to be deployed by bugs.p.o maintainers. Thank you, again!

> I'll keep poking at it to try to figure out why random seems to be
> losing state, but I am stumped here.

Did you get a chance to look at what was wrong on Roundup's side?
msg3455 (view) Author: berker.peksag Date: 2018-06-02.07:50:16
Ezio has just deployed 9e1d65b4927c and I've confirmed that it works on bugs.python.org. Closing as 'resolved'.

Thanks for the report, Cheryl.
msg3456 (view) Author: rouilj Date: 2018-06-04.00:03:40
Hi Berker:

In message <1527918822.93.0.81473610881.issue644@psf.upfronthosting.co.za>,
Berker Peksag writes:
>Berker Peksag <berker.peksag@gmail.com> added the comment:
>
>I've just committed John's suggested approach in
> https://hg.python.org/tracker/python-dev/rev/9e1d65b4927c but it
>needs to be deployed by bugs.p.o maintainers. Thank you, again!
>
>> I'll keep poking at it to try to figure out why random seems to be
>> losing state, but I am stumped here.
>
>Did you get a chance to look at what was wrong on Roundup's side?

I spent about 4 hours on it but didn't come up with any cause. I
couldn't reproduce in a test case. Every theory I came up with
didn't predict the symptoms I was seeing.

The major change in the newest release is that roundup's core now uses
SystemRandom/os.urandom when available. This is used for various one
time keys/nonces to prevent CSRF and other nastyness.

The Random Issue extension uses the pseudo-random generator since
calling seed() has an effect. When using SystemRandom/os.urandom
seed() is a no-op. My only guess is that somehow those two random's
are interacting badly with the result that the pseudorandom generator
is not being properly initialized/reinitialized/state preserved. As a
result on every random call the same number is returned.

Proving that theory would require looking at the C code and seeing
what the relationship is between these two implementations. I was
going down that path and just ran out of steam. (Frankly I think this
theory is junk but it's the only one I have that explains the
symptoms.)

The solution I proposed does have the slight risk that the return
value of the pseudo random generator elsewhere in the roundup code
could be predicted. By hitting the random article the attacker can
seed the pseudo-random generator with one of a few known values. As a
result values returned by other uses of random may be predictable.

This shouldn't affect the core roundup use on any system that has
urandom.SystemRandom so there is no security issue AFAICT.

Hopefully this email will open and reresolve the issue. If it leaves
it open, sorry about that.
msg3463 (view) Author: ncoghlan Date: 2018-06-06.14:00:28
Note that a potential variation of the new approach is that rather than reseeding the default instance, the Action could create and seed a new random.Random() instance every time. It would be slightly slower, but I don't think this gets hit often enough for that to be a major concern.
msg3464 (view) Author: berker.peksag Date: 2018-06-06.14:11:36
Thank you again for taking a look at this, John.

I didn't think about using random.Random() to be honest, thank you Nick! Is there a reason why random.Random() is undocumented in the random module documentation?
msg3465 (view) Author: ncoghlan Date: 2018-06-06.14:23:19
Huh, I'd never noticed how easy that documentation is to miss: it's in the preamble text at the start of https://docs.python.org/3/library/random.html

I filed https://bugs.python.org/issue33783 to suggest giving it some proper Sphinx markup.
msg3466 (view) Author: berker.peksag Date: 2018-06-06.14:28:13
Ah, I've definitely missed the "You can instantiate your own instances of Random to get generators that don’t share state." part. Thanks!
msg3467 (view) Author: berker.peksag Date: 2018-06-06.14:42:28
Implemented Nick's suggestion in https://hg.python.org/tracker/python-dev/rev/ad6158d8414f
msg3517 (view) Author: csabella Date: 2018-06-19.22:34:24
Thanks Berker!
msg3520 (view) Author: rouilj Date: 2018-07-08.15:15:16
I found this same issue in other places in roundup.

Thanks to Joseph Myers (jsm) for explaining what I was seeing on the roundup-devel mailing list.

There is a fix for it now in roundup that will be released as version 1.6.

You don't have to make any changes to your Random Issue implementation,
but I thought it would be good to document that there was a fundamental
oversight that is fixed.
msg3521 (view) Author: berker.peksag Date: 2018-07-08.15:29:10
Thanks for the update, John.

For future readers, here's a link to the discussion on the roundup-devel list: https://sourceforge.net/p/roundup/mailman/roundup-devel/thread/20180708014103.75EE34C0A5C%40itserver6.localdomain/
History
Date User Action Args
2018-07-08 15:29:10berker.peksagsetmessages: + msg3521
2018-07-08 15:15:16rouiljsetmessages: + msg3520
2018-06-19 22:34:25csabellasetmessages: + msg3517
2018-06-06 14:42:28berker.peksagsetmessages: + msg3467
2018-06-06 14:28:13berker.peksagsetmessages: + msg3466
2018-06-06 14:23:19ncoghlansetmessages: + msg3465
2018-06-06 14:11:36berker.peksagsetmessages: + msg3464
2018-06-06 14:00:29ncoghlansetnosy: + ncoghlan
messages: + msg3463
2018-06-04 00:03:42rouiljsetmessages: + msg3456
2018-06-02 07:50:16berker.peksagsetstatus: testing -> resolved
messages: + msg3455
2018-06-02 05:53:42berker.peksagsetstatus: chatting -> testing
messages: + msg3454
2018-06-01 04:11:43berker.peksagsetmessages: + msg3453
2018-06-01 00:41:57rouiljsetmessages: + msg3451
2018-03-27 12:04:37berker.peksaglinkissue650 superseder
2017-11-11 21:03:43rouiljsetmessages: + msg3420
2017-11-11 15:32:03rouiljsetmessages: + msg3419
2017-11-11 05:05:04berker.peksagsetmessages: + msg3418
2017-11-11 02:59:40rouiljsetmessages: + msg3417
2017-11-09 19:54:48berker.peksagsetnosy: + rouilj
messages: + msg3415
2017-11-05 15:57:29berker.peksagsetstatus: unread -> chatting
nosy: + berker.peksag, ezio.melotti
messages: + msg3414
2017-10-23 23:58:33csabellacreate