Arvados

7884-ajax-log-redirect @ 384a517, tests at https://ci.curoverse.com/job/developer-test-job/73/

Reviewing 7884-ajax-log-redirect @ 384a517

I think I finally understand more or less what's going on. Comments:

I would prefer to avoid using POST in this way, since it breaks HTTP semantics.

Since the current solution already modifies keep-web, it would be straightforward to add support for the OPTIONS method and then do a proper GET request with an "Authorization" header.

1. Use the /t=xxx/ syntax which doesn't set a cookie
2. Add &no_cookie to the query to suppress the cookie redirect

In both cases, since it's an AJAX request it's not going to show up in browsing history.

Peter Amstutz wrote:

I would prefer to avoid using POST in this way, since it breaks HTTP semantics.

You mean by making the response non-cacheable when by all rights it should be cacheable?

Since the current solution already modifies keep-web, it would be straightforward to add support for the OPTIONS method and then do a proper GET request with an "Authorization" header.

Yes, as discussed on IRC, that's the real solution. But I'd like to merge this in the meantime, to unblock the work that's waiting for a bugfix.

Alternately, since the underlying problem doing a GET request with "?api_token=xxx" is that it responds with a redirect that sets the arvados_api_token cookie which gets rejected by the browser. Wouldn't it be better to do one of:

1. Use the /t=xxx/ syntax which doesn't set a cookie
2. Add &no_cookie to the query to suppress the cookie redirect

I did consider a no_cookie param, as an explicit way to ask for this; OTOH, the Origin header is automatically added by the browser in exactly the situations where it will reject the redirect-with-cookie, so it seems like we would still want to do the "no_cookie" thing if Origin is set and no_cookie isn't.

In both cases, since it's an AJAX request it's not going to show up in browsing history.

True, but I'd say it's also worth keeping tokens out of log files whenever we can.

Tom Clegg wrote:

Yes, as discussed on IRC, that's the real solution. But I'd like to merge this in the meantime, to unblock the work that's waiting for a bugfix.

Do we have a ticket for that?

True, but I'd say it's also worth keeping tokens out of log files whenever we can.

Okay, so the specific reasoning for using POST in this case instead of one of the alternatives is to avoid including the token in the URL so it doesn't get logged. That seems like a reasonable trade off.

This comment is misleading:

    if r.Header.Get("Origin") != "" {
        // Allow simple cross-origin requests, without
        // credentials.
        w.Header().Set("Access-Control-Allow-Origin", "*")
    }

What this is actually saying is this will permit CORS requests that don't use a custom Authorization header, but the credentials are supplied by some other mechanism.

Do we have a ticket for that?

I was thinking of just doing it here, but yes, probably better to move it to an issue and prioritize separately. Added #8064

Peter Amstutz wrote:

This comment is misleading:

[...]

What this is actually saying is this will permit CORS requests that don't use a custom Authorization header, but the credentials are supplied by some other mechanism.

Ah, yes. I meant "credentials" in the CORS sense, not the general sense. I'll clarify.

0.5 story points to write it and 1.5 story points to review it.

Looks good to me. Please merge.