This default seems backwards to me. In routes.py we check if CORS is allowed by using not config.get_option("server.enableCORS"). Left it as is for now 🤷‍♀️

def allow_cross_origin_requests():
    """True if cross-origin requests are allowed.

    We only allow cross-origin requests when using the Node server. This is
    only needed when using the Node server anyway, since in that case we
    have a dev port and the prod port, which count as two origins.

    """
    return not config.get_option("server.enableCORS") or config.get_option(
        "global.useNode"
    )

Good point re the login in allow_cross_origin_requests(). For the sake of this file - would it be possible to minimize code change by retaining the location of server.enableCORS and moving server.enableCSRF alongside it?

I can do that. I was being silly and slowly alphabetizing them.

Good point re the login in allow_cross_origin_requests(). For the sake of this file - would it be possible to minimize code change by retaining the location of server.enableCORS and moving server.enableCSRF alongside it?

Is it sufficient to just issue a warning here? For example, in Server.py, we access self.xsrf_token when server.enableCSRF is enabled. Also in UploadFileRequestHandler.py it looks like server.enableCSRF takes precedence over allow_cross_origin_requests(). If both CSRF and CORS are enabled, we should either fail loudly, or preferably we should choose prefer one of the two along with a clear warning message?

updated the warning messaging indicating server.enableCSRF will take precedence. Let me know if it's not clear!

The warning message LGTM

Actually, I think this warning could be reworded (given the long internal conversation about how confusing server.enableCORS can be) as follows:

"Currently, CSRF protection is incompatible with Cross-Origin Resource Sharing (CORS). If you are enabling `server.enableCSRF` and disabling `server.enableCORS` options at the same time, we will prioritize `server.enableCSRF`. If cross origin POST, PUT, PATCH or DELETE requests are required, please disable 'server.enableCSRF'."

I don't know the ins and outs of CSRF, so this might be nonsense, but: I think this might be the wrong approach. Currently, if I'm understanding correctly:

• There are circumstances where a CSRF token needs to be updated
• When this happens, we need to manually call updateCsrfToken() for all HTTPClient instances. (It looks like we only have one, right now, but HTTPClient is a base class, not a singleton, so this won't necessarily always hold.)

This means we're introducing new state that we need to remember to manually update. Since this is just for setting headers on requests, do we actually need this state - that is, can we just set the CSRF header reactively, instead? For example, we could have an HTTPClient.request method that just wraps axios.request, and adds this header as appropriate.

public request(params: AxiosRequestConfig) {
  // If we have a CSRF cookie, add it to every outgoing request
  const cookie = getCookie("_xsrf")
  if (cookie != null) {
    params.headers["X-Xsrftoken"] = cookie
    params.withCredentials = true
  }
  return axios.request(params)
}

Then, FileUploadClient and any other HTTPClient instances would do this.request(...) instead of axios.request(...), and nobody has to remember to manually update a token anywhere?

I debated about this for quite a bit and ended up going with this approach to provide more flexibility.

On the server side we have to update the allowable headers for each request handler so its possible we could have CSRF enabled on some and not others (currently the case though none of the existing has a corresponding http client). I'm assuming we'll end up mapping each new HTTPClient to a different tornado.RequestHandler. In that regards, if we create a new RequestHandler without specifying the necessary allowable headers, the HTTPClient will not cry (assuming we wouldn't have called updateCsrfToken for the new HTTPClient as well). This could be a completely moot use case I'm accommodating for. Let me know what you think and happy to change!

I don't quite follow, but I still think that being stateless is accomplishable and desirable. (Though again, I might be misunderstanding something!)

If the CSRF header is a per-client concern, then we could have "useCSRFToken" as a parameter that gets passed to the constructor, and the code above becomes:

// (in class HTTPClient):
public request(params: AxiosRequestConfig) {
  if (this.useCSRFToken) {
      const cookie = getCookie("_xsrf")
      if (cookie != null) {
        params.headers["X-Xsrftoken"] = cookie
        params.withCredentials = true
      }
  }
  return axios.request(params)
}

(It's the stateful updateCsrfToken() function that I'm objecting to.)

I didn't think about passing it at the constructor level. 🤦‍♀️ I only thought about passing it at the request level and was too saddened by that. I'll update!

I'm not sure I understand this code. Can you add a comment explaining why it's needed? (Also, it looks like each iteration through this loop completely rewrites document.cookie, which possibly means that only the last iteration is needed?)

The warning message LGTM

What does "requires CORS to be disabled" mean?

If cross origin POST, PUT or DELETE requests are required, please disable 'server.enableCSRFProtection'

What does this mean? IIUC this is not a helpful message as the developer has no idea if what HTTP verbs we're using...