TB2.4+ POST to scalar_multirun omits XSRF header, blocking use of TensorBoard under JupyterLab

cliffwoolley commented 3 years ago

Environment information (required)

Diagnostics

Diagnostics output

`````` --- check: autoidentify INFO: diagnose_tensorboard.py version e43767ef2b648d0d5d57c00f38ccbd38390e38da --- check: general INFO: sys.version_info: sys.version_info(major=3, minor=8, micro=5, releaselevel='final', serial=0) INFO: os.name: posix INFO: os.uname(): posix.uname_result(sysname='Linux', nodename='xpl-dvt-52', release='4.15.0-20-generic', version='#21-Ubuntu SMP Tue Apr 24 06:16:15 UTC 2018', machine='x86_64') INFO: sys.getwindowsversion(): N/A --- check: package_management INFO: has conda-meta: False INFO: $VIRTUAL_ENV: None --- check: installed_packages WARNING: no installation among: ['tb-nightly', 'tensorboard', 'tensorflow-tensorboard'] WARNING: no installation among: ['tensorflow', 'tensorflow-gpu', 'tf-nightly', 'tf-nightly-2.0-preview', 'tf-nightly-gpu', 'tf-nightly-gpu-2.0-preview'] INFO: installed: tensorflow-estimator==2.4.0 --- check: tensorboard_python_version INFO: tensorboard.version.VERSION: '2.4.1' --- check: tensorflow_python_version 2021-02-12 23:29:05.329875: I tensorflow/stream_executor/platform/default/dso_loader.cc:49] Successfully opened dynamic library libcudart.so.11.0 INFO: tensorflow.__version__: '2.4.0' INFO: tensorflow.__git_version__: 'unknown' --- check: tensorboard_data_server_version INFO: no data server installed --- check: tensorboard_binary_path INFO: which tensorboard: b'/usr/local/bin/tensorboard\n' --- check: addrinfos socket.has_ipv6 = True socket.AF_UNSPEC = socket.SOCK_STREAM = socket.AI_ADDRCONFIG = socket.AI_PASSIVE = Loopback flags: Loopback infos: [(, , 6, '', ('::1', 0, 0, 0)), (, , 6, '', ('127.0.0.1', 0)) ] Wildcard flags: Wildcard infos: [(, , 6, '', ('0.0.0.0', 0)), (, , 6, '', ('::', 0, 0, 0))] --- check: readable_fqdn INFO: socket.getfqdn(): '[redacted]' --- check: stat_tensorboardinfo INFO: directory: /tmp/.tensorboard-info INFO: .tensorboard-info directory does not exist --- check: source_trees_without_genfiles INFO: tensorboard_roots (1): ['/usr/local/lib/python3.8/dist-packages']; bad_roots (0): [] --- check: full_pip_freeze INFO: pip freeze --all: [redacted] ``````

Screenshot, if it’s a visual issue:

Issue description

When loading TensorBoard through Jupyter-TensorBoard + JupyterLab-Tensorboard , we find that with TB2.4 and later (since https://github.com/tensorflow/tensorboard/pull/4050 ), the Scalar card doesn't ever finish loading. What's happening is that the new scalars_multirun POST requests are getting rejected by the JupyterLab web service before routing them to TB, because they are missing the correct Cross-Site Request Forgery (XSRF) header, which should have been set by JupyterLab and passed back to it by TensorBoard yet wasn't for some reason.

As a workaround, in a vaguely similar way to https://github.com/tensorflow/tensorboard/pull/4191, if I patch TensorBoard to force use of the older Get path for Scalars at https://github.com/tensorflow/tensorboard/blob/master/tensorboard/plugins/scalar/tf_scalar_dashboard/tf-scalar-card.ts#L298 , then TensorBoard 2.4 and JupyterLab can work well again together.

How can we get this header passed through the POST request for JupyterLab to accept scalars_multirun requests?

cliffwoolley commented 3 years ago

@wchargin , per our discussion earlier today. Thanks!

cliffwoolley commented 3 years ago

https://github.com/jupyterlab/jupyterlab/issues/1435#issuecomment-268771659 might be helpful?

the the xsrf token needs to be in the X-XSRFToken header or an _xsrf url param, or the request needs to be authenticated with an API token. See here for how the legacy js notebook does it.

...which refers to this snippet of code: https://github.com/jupyter/notebook/blob/4.x/notebook/static/base/js/utils.js#L699-L704

        if (!settings.headers.Authorization) {
            var xsrf_token = _get_cookie('_xsrf');
            if (xsrf_token) {
                settings.headers['X-XSRFToken'] = xsrf_token;
            }
        }

cliffwoolley commented 3 years ago

So there seem to be at least two things happening:

Jupyter is using Tornado, and TensorBoard is using Angular. Both have XSRF protection built in, but they use slightly different cookie names (_xsrf versus XSRF-TOKEN) as well as slightly different request header names (X-XSRFTOKEN versus X-XSRF-TOKEN). Angular allows overriding these, but Tornado doesn't appear to. That said, with a bit of monkey-patching in Jupyter-Tensorboard, I think I could translate one set to the other.
But I haven't yet gotten TensorBoard to actually issue its own XSRF Header in its POSTs, even when using the names Angular expects.

cliffwoolley commented 3 years ago

@stephanwlee FYI since you appear to be working on some related things recently.

cliffwoolley commented 3 years ago

But I haven't yet gotten TensorBoard to actually issue its own XSRF Header in its POSTs, even when using the names Angular expects.

I realized that the reason for this one is just that TB isn't using the Angular JS client for those particular POSTS; it's using calls to XHR in tensorboard/components/tf_backend/requestManager.ts. It seems we can insert something minimal here to mimic the Angular behavior; perhaps something like this?

diff --git a/tensorboard/components/tf_backend/requestManager.ts b/tensorboard/components/tf_backend/requestManager.ts
index 16416e165..d2762a6e9 100644
--- a/tensorboard/components/tf_backend/requestManager.ts
+++ b/tensorboard/components/tf_backend/requestManager.ts
@@ -270,6 +270,12 @@ namespace tf_backend {
     }
   }

+  function getCookie(name: string): string {
+    /* https://stackoverflow.com/a/21125098 */
+    var match = document.cookie.match(new RegExp('(^| )' + name + '=([^;]+)'));
+    if (match) return match[2];
+  }
+
   function buildXMLHttpRequest(
     methodType: HttpMethodType,
     url: string,
@@ -284,6 +290,12 @@ namespace tf_backend {
     if (contentType) {
       req.setRequestHeader('Content-Type', contentType);
     }
+    if (methodType == HttpMethodType.POST) {
+      var xsrf_token = getCookie('XSRF-TOKEN');
+      if (xsrf_token) {
+        req.setRequestHeader('X-XSRF-TOKEN', xsrf_token);
+      }
+    }
     return req;
   }

nfelt commented 3 years ago

Hi Cliff, thanks for the additional details! We've discussed this a bit and as I think you've determined, this isn't so much a specific omission / bug in TensorBoard as it just is that we don't do any XSRF token propagation at all, because TensorBoard itself doesn't have state-changing endpoints and therefore doesn't intrinsically require XSRF protection.

There isn't a single standard way to do XSRF token propagation (and as you can see we also don't use the Angular request manager), so it doesn't really make sense in our code to special-case specific cookie names and headers that are used by Angular and/or Tornado.

However I noticed you've already figured out that you can make the necessary changes within Jupyter TensorBoard's request handlers to conditionally disable Tornado's XSRF token checking: https://github.com/cliffwoolley/jupyter_tensorboard/commit/dfa2a17f938322602399ac098eb2b91fd6438a4b

Rather than the version-gating on TB <= 2.4, our recommendation is just to go with this as the solution, since it keeps the logic that's specific to the Jupyter/Tornado XSRF requirements contained to where it's necessary. (You shouldn't need the XSRF header translation logic, so it could look more like the original upstream PR.) If you'd like to be stricter about which POST requests you exempt from XSRF checking, you could always check the request path and limit the exemption to scalars_multirun (and the HParams plugin paths if you want those to work as well).

cliffwoolley commented 3 years ago

OK @nfelt , thanks for the feedback. Done in https://github.com/cliffwoolley/jupyter_tensorboard/commit/ffa7e26138b82549453306e06b535a9ac36db17a and closing.

tensorflow / tensorboard