New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Github change hook incorrectly treats payload with bytes #3452

Closed
maleadt opened this Issue Jul 18, 2017 · 4 comments

Comments

Projects
None yet
4 participants
@maleadt

maleadt commented Jul 18, 2017

Using the Github change hook, it seems like the decoded payload contains bytes instead of strings:

2017-07-18 13:37:22+0200 [-] {b'payload': [b'{"ref":"refs/heads/master","before":"3c41557e1a77291ca8b109d'
2017-07-18 13:37:22+0200 [-]               b'd267cbd80dfa647ae","after":"2d28d01c433ea620d4808ea0e7fb5ba3'
...

So indexing with a regular string "payload" throws a KeyError:

        Traceback (most recent call last):
          File "/opt/buildbot/lib/python3.5/site-packages/twisted/web/server.py", line 235, in render
            body = resrc.render(self)
          File "/opt/buildbot/lib/python3.5/site-packages/twisted/web/resource.py", line 250, in render
            return m(request)
          File "/opt/buildbot/lib/python3.5/site-packages/buildbot/www/change_hook.py", line 81, in render_POST
            d = self.getAndSubmitChanges(request)
          File "/opt/buildbot/lib/python3.5/site-packages/twisted/internet/defer.py", line 1532, in unwindGenerator
            return _inlineCallbacks(None, gen, Deferred())
        --- <exception caught here> ---
          File "/opt/buildbot/lib/python3.5/site-packages/twisted/internet/defer.py", line 1384, in _inlineCallbacks
            result = result.throwExceptionIntoGenerator(g)
          File "/opt/buildbot/lib/python3.5/site-packages/twisted/python/failure.py", line 393, in throwExceptionIntoGenerator
            return g.throw(self.type, self.value, self.tb)
          File "/opt/buildbot/lib/python3.5/site-packages/buildbot/www/change_hook.py", line 107, in getAndSubmitChanges
            changes, src = yield self.getChanges(request)
        builtins.KeyError: 'payload'

Changing the source here to index with bytes and decode the results (ie. json.loads(bytes2NativeString(request.args[b'payload'][0]))) makes the change hook work again, but I lack the python experience to know how good of a fix that is.

buildbot==0.9.9.post2
buildbot-console-view==0.9.9.post2
buildbot-waterfall-view==0.9.9.post2
buildbot-worker==0.9.9.post2
buildbot-www==0.9.9.post2

Python 3.5.3

Debian Jessie
@tardyp

This comment has been minimized.

Show comment
Hide comment
@tardyp

tardyp Jul 18, 2017

Member

Hi @maleadt that change looks good. can you submit a patch?

You may have to fix the unit tests:
http://docs.buildbot.net/latest/developer/tests.html

Member

tardyp commented Jul 18, 2017

Hi @maleadt that change looks good. can you submit a patch?

You may have to fix the unit tests:
http://docs.buildbot.net/latest/developer/tests.html

@rodrigc

This comment has been minimized.

Show comment
Hide comment
@rodrigc

rodrigc Sep 8, 2017

Collaborator

@maleadt I was able to reproduce your problem. The problem occurs if
your GitHub webhook is configured with application/x-www-form-urlencoded.
If you change your webhook to be configured with application/json, then the problem
does not occur and everything works.

I'll see if I can fix this.

Collaborator

rodrigc commented Sep 8, 2017

@maleadt I was able to reproduce your problem. The problem occurs if
your GitHub webhook is configured with application/x-www-form-urlencoded.
If you change your webhook to be configured with application/json, then the problem
does not occur and everything works.

I'll see if I can fix this.

@rodrigc

This comment has been minimized.

Show comment
Hide comment
@rodrigc

rodrigc Sep 8, 2017

Collaborator

One other thing to note.
In Python 3.5, this:

json.loads(b'{}')

gives an error:

TypeError: the JSON object must be str, not 'bytes'

On Python 3.6, json.loads(b'{}') does not give an error, i.e. it accepts an argument of bytes.

Collaborator

rodrigc commented Sep 8, 2017

One other thing to note.
In Python 3.5, this:

json.loads(b'{}')

gives an error:

TypeError: the JSON object must be str, not 'bytes'

On Python 3.6, json.loads(b'{}') does not give an error, i.e. it accepts an argument of bytes.

@maleadt

This comment has been minimized.

Show comment
Hide comment
@maleadt

maleadt Sep 8, 2017

@tardyp Thanks for the fix; sorry for not looking into it myself, I hadn't found the time to set up a dev env.

maleadt commented Sep 8, 2017

@tardyp Thanks for the fix; sorry for not looking into it myself, I hadn't found the time to set up a dev env.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment