Entering a OpenID identifier that includes a comma turns the input into an array causing an error

[tbys]

For instance:

NoMethodError in Devise::SessionsController#create
undefined method `match' for ["http://foo.openid.ne", "jp"]:Array

Entering just a comma for the identifier also causes an error.

It would be nice to render a flash error (Invalid OpenId identifier etc.) for these two instances.

I don't have a regular devise installation (with password login) to test on, so I can't tell if this is actually a issue with devise itself.

[nbudin]

Do you know why the input is ending up as a comma? I don't think this is normal Rails behavior; is it ruby-openid doing it?

[tbys]

Sorry for the late answer.

The user inputs the comma as part of the OpenID identifier, by mistake. For example, the user wanted to type http://john.openid.net but typed http://john.openid,org instead.

I am not sure what part of the OpenID stack (devise_openid_authenticatable, devise, ruby-openid etc.) should be responsible for handling invalid identifiers, but it seems to me that an invalid input should result in a flash message and not an 500 internal server error.

[nbudin]

Ah, makes sense.

I suspect that ruby-openid is throwing that error and we're not catching it correctly. Can you paste a traceback, if you have it, to indicate which file the error is happening in?

[tbys]

Here you go:

NoMethodError in Devise::SessionsController#create

undefined method `match' for ["foo", "bar"]:Array

ruby-openid (2.1.8) lib/openid/yadis/xri.rb:15:in `identifier_scheme'
ruby-openid (2.1.8) lib/openid/consumer/discovery.rb:491:in `discover'
ruby-openid (2.1.8) lib/openid/consumer.rb:333:in `discover'
ruby-openid (2.1.8) lib/openid/consumer/discovery_manager.rb:51:in `get_next_service'
ruby-openid (2.1.8) lib/openid/consumer.rb:222:in `begin'
rack-openid (1.3.1) lib/rack/openid.rb:123:in `begin_authentication'
rack-openid (1.3.1) lib/rack/openid.rb:102:in `call'
actionpack (3.1.4) lib/action_dispatch/middleware/best_standards_support.rb:17:in `call'
rack (1.3.6) lib/rack/etag.rb:23:in `call'
rack (1.3.6) lib/rack/conditionalget.rb:35:in `call'
actionpack (3.1.4) lib/action_dispatch/middleware/head.rb:14:in `call'
actionpack (3.1.4) lib/action_dispatch/middleware/params_parser.rb:21:in `call'
actionpack (3.1.4) lib/action_dispatch/middleware/flash.rb:247:in `call'
rack (1.3.6) lib/rack/session/abstract/id.rb:195:in `context'
rack (1.3.6) lib/rack/session/abstract/id.rb:190:in `call'
actionpack (3.1.4) lib/action_dispatch/middleware/cookies.rb:331:in `call'
activerecord (3.1.4) lib/active_record/query_cache.rb:64:in `call'
activerecord (3.1.4) lib/active_record/connection_adapters/abstract/connection_pool.rb:477:in `call'
actionpack (3.1.4) lib/action_dispatch/middleware/callbacks.rb:29:in `block in call'
activesupport (3.1.4) lib/active_support/callbacks.rb:392:in `_run_call_callbacks'
activesupport (3.1.4) lib/active_support/callbacks.rb:81:in `run_callbacks'
actionpack (3.1.4) lib/action_dispatch/middleware/callbacks.rb:28:in `call'
actionpack (3.1.4) lib/action_dispatch/middleware/reloader.rb:68:in `call'
rack (1.3.6) lib/rack/sendfile.rb:101:in `call'
actionpack (3.1.4) lib/action_dispatch/middleware/remote_ip.rb:48:in `call'
actionpack (3.1.4) lib/action_dispatch/middleware/show_exceptions.rb:47:in `call'
railties (3.1.4) lib/rails/rack/logger.rb:13:in `call'
rack (1.3.6) lib/rack/methodoverride.rb:24:in `call'
rack (1.3.6) lib/rack/runtime.rb:17:in `call'
activesupport (3.1.4) lib/active_support/cache/strategy/local_cache.rb:72:in `call'
rack (1.3.6) lib/rack/lock.rb:15:in `call'
actionpack (3.1.4) lib/action_dispatch/middleware/static.rb:61:in `call'
railties (3.1.4) lib/rails/engine.rb:456:in `call'
railties (3.1.4) lib/rails/application.rb:143:in `call'
rack (1.3.6) lib/rack/content_length.rb:14:in `call'
railties (3.1.4) lib/rails/rack/log_tailer.rb:14:in `call'
rack (1.3.6) lib/rack/handler/webrick.rb:59:in `service'
/home/foo/.rvm/rubies/ruby-1.9.2-p290/lib/ruby/1.9.1/webrick/httpserver.rb:111:in `service'
/home/foo/.rvm/rubies/ruby-1.9.2-p290/lib/ruby/1.9.1/webrick/httpserver.rb:70:in `run'
/home/foo/.rvm/rubies/ruby-1.9.2-p290/lib/ruby/1.9.1/webrick/server.rb:183:in `block in start_thread'

[nbudin]

Thanks for the traceback!

Unfortunately, this confirms what I was afraid of: the error is originating inside a call from rack-openid. By this point in the request, devise_openid_authenticatable is out of the picture (notice that devise_openid_authenticatable doesn't appear anywhere in the call stack). That means we have two options, at least that I can think of:

1. Submit a patch to ruby-openid or rack-openid that will handle this condition better. (Possibly both projects.) This is almost certainly the "right way" to do it, but it might take awhile to get patches accepted.
2. Try to work around it in devise_openid_authenticatable by detecting OpenID URLs that might cause this error before submitting it to rack-openid for authentication. I'm worried about false positives here; what's invalid for one OpenID implementation might be perfectly OK for another. But if you can come up with a safe way to do that that won't break existing implementations, I'm open to that.

[tbys]

You're welcome :)

I don't have any spare time at the moment, so I won't be able to look into it, at least not at the moment. I am relatively inexperienced, so I it would take me a while to wrap my head around it. But I'll start by submitting a bug report to rack-openid or ruby-openid when I have some spare time.