Arvados

Updates at 4bb449eb5 - branch 14718-api-login-salted-token
Test run: https://ci.curoverse.com/job/developer-run-tests/1041/

Took advantage of return_to being passed around between API server and SSO to propagate the remote parameter.

In user_sessions_controller.rb → login, it looks like params[:remote] is missing a CGI.escape() in case it's an unexpectedly non-alphanum/malicious string -- it gets appended to another string provided by the same client so I don't see how it would be exploitable, but I'm inclined to be defensive about it anyway.

        remote_param += "remote=#{params[:remote]}" 

• it's sneaky/non-obvious enough that it should be explained in comments
• it should be removed from return_to before create() redirects there. If I'm following correctly, when Workbench sends the user to https://api/login?return_to=https://wb/home&remote=bbbbb, the user will eventually land on either https://wb/home?remote=bbbbb or https://wb/home depending on whether they needed to go through the joshid→sso→sessions#create flow or took the "already logged in" short cut. It would be better if we could ensure they always land on https://wb/home as requested. (Am I following correctly?)

I wonder whether it might even be less confusing to use a format like return_to="bbbbb,https://wb/home" (or ",https://wb/home" for the unsalted case) to avoid all the corner cases involved in stashing the remote id in the client-provided URL. For example, I'd rather not even try to figure out whether we're doing the right thing here when the client-provided return_to is something like "https://wb/home?foo=bar#foo/bar?baz".

Updates at 0d1836a8d
Test run: https://ci.curoverse.com/job/developer-run-tests/1043/

Addressed above suggestions.

• ignore the remote param if it contains a comma (or perhaps even if it doesn't match /^[0-9a-z]{5}$/)
• escape the whole combined param including the comma (I notice CGI.escape(',') == '%2c' -- and we will unescape the whole thing before splitting, so we might as well escape the whole thing after joining)

the rest LGTM, thanks!

Tom Clegg wrote:

• ignore the remote param if it contains a comma (or perhaps even if it doesn't match /^[0-9a-z]{5}$/)

...only on the "login" handler, not the "callback" handler.

On second thought, I think we should error out (rather than ignoring the remote param) in both callback and login handlers if the "remote" value doesn't look like a cluster ID.

Updates at 245e4cafd
Test run: https://ci.curoverse.com/job/developer-run-tests/1044/

• Validates the remote cluster id parameter both on login endpoint & omniauth callback.
• Added tests.
• Updated lib/controller handler test to support the new format.

LGTM, thanks!