Skip to content
New issue

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

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

Already on GitHub? # to your account

oauth: boost::spirit::qi::expectation_failure #146

Closed
mmd-osm opened this issue May 20, 2018 · 3 comments
Closed

oauth: boost::spirit::qi::expectation_failure #146

mmd-osm opened this issue May 20, 2018 · 3 comments

Comments

@mmd-osm
Copy link
Collaborator

mmd-osm commented May 20, 2018

I was testing some use cases according to the osm website changeset_controller_test.rb file, and noticed an issue with OAuth header parsing. Somehow all test cases use an apostrophe for XML attributes (like in <node id='#{node.id}'), and parts of this gets included in the OAuth Auth header.

Unfortunately, using ' instead of " triggers an HTTP 500 Internal Server error along with an Exception: boost::spirit::qi::expectation_failure. I'm suspecting that this case isn't covered in the parser implementation in oauth.cpp.

(mod_fastcgi.c.3021) backend died; we'll disable it for 1 seconds and send the request to another backend instead: reconnects: 0 load: 1

curl statement to reproduce from command line:

curl -v 'http://localhost:31337/api/0.6/changeset/876/download' -H 'Accept: */*'  -H 'Content-Type: application/x-www-form-urlencoded' -H $'Authorization: OAuth %3Cdummy%20id="\'-1\'%2F%3E", oauth_consumer_key="U84xxVrHBewaYHehTpaV0Rk3nGhahzRj0zntCe1N", oauth_nonce="Xw1WlI", oauth_signature="32gRmihzmdV47jW2juAL7mIpXkA%3D", oauth_signature_method="HMAC-SHA1", oauth_timestamp="1526814151", oauth_token="FLfg7mbQlV6TAcBSY58YgQz39mpcWgj47J3PZPEx"'

BTW: the request was generated by https://github.com/osmlab/osm-auth, maybe there's some issue in that library as well. It shouldn't trigger a crash on cgimap side though.

@zerebubuth
Copy link
Owner

zerebubuth commented May 21, 2018

Yup, you're right. That should be a parse fail rather than a crash.

Digging into the cause of the failure, it seems to be that %3Cdummy%20id="'-1'%2F%3E" is causing it because ' (single quote) is not escaped, but ought to be because it is not one of the unreserved set of characters that are mandated by the OAuth spec. This would appear to be an issue with whatever generated that OAuth header - although it could have been unquoted by a browser or other tool trying to be helpful. How did you generate that string?

The string looks a bit weird to me: %3Cdummy%20id="'-1'%2F%3E" unquotes to <dummy id="'-1'/>", which looks an awful lot like:

s = "<dummy id='-1'/>"
k, v = s.split('=')
print "%s=\"%s\"" % (k, v)
# writes: <dummy id="'-1'/>"

I don't understand why there'd be some random XML in the OAuth header. But regardless - the parse fail should be only a fail, not a crash.

zerebubuth added a commit that referenced this issue May 21, 2018
Don't propagate exceptions during OAuth header parsing and validation. Instead, treat it as a validation failure.

Fixes #146.
@mmd-osm
Copy link
Collaborator Author

mmd-osm commented May 21, 2018

Thanks a lot for the quick fix.

I'm using the following test client https://github.com/mmd-osm/osm-auth/blob/debug_client/index.html, based on osm-auth, which is also used in iD editor. The relevant call is here (https://github.com/mmd-osm/osm-auth/blob/debug_client/index.html#L93-L96) - so I'm guessing this must be triggered by the library somehow.

Test payload:

<dummy id='1'/>

I think I will also open an issue for that library. It could be that I'm missing something how this library is supposed to be used. For the iD editor, this issue doesn't seem relevant, as they're using double quotes instead of single quotes when uploading changesets.

@mmd-osm
Copy link
Collaborator Author

mmd-osm commented May 21, 2018

Adding options: { header: { 'Content-Type': 'text/xml' } }, in my script seems to do the trick, the strange header is gone now. Good that we've also identified and fixed this parsing issue in cgimap along the way.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants