-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core: add CSP header for Colab output frames #2390
Conversation
Summary: This is required to use the new `google.colab.kernel.proxyPort` Colab feature (see Google-internal <http://b/130310433>). Test Plan: The included tests verify that the mechanism is implemented as intended. To verify that the mechanism actually works in Colab, create a Colab notebook with the following cells: ```python import werkzeug @werkzeug.Request.application def app(request): frame_ancestors = request.args.get("frame_ancestors") response = werkzeug.Response("Hello, %s!\n" % (frame_ancestors,)) response.headers["Content-Type"] = "text/html" if frame_ancestors is not None: response.headers["Content-Security-Policy"] = ( "frame-ancestors %s" % frame_ancestors ) return response ``` ``` import threading import werkzeug.serving if "server" in locals(): server.shutdown() server = werkzeug.serving.ThreadedWSGIServer("localhost", 2345, app) threading.Thread(target=server.serve_forever).start() ``` ``` !curl -i localhost:2345?frame_ancestors=foo ``` ```javascript %%javascript google.colab.kernel.proxyPort(2345).then((base) => { const ancestors = "https://*.googleusercontent.com https://*.google.com"; const url = new URL( "?frame_ancestors=" + encodeURIComponent(ancestors), base, ); const iframe = document.createElement("iframe"); iframe.src = url.toString(); document.body.appendChild(iframe); }); ``` Run the notebook and verify that the final output frame renders properly: ![Screenshot of output frame with intended “Hello” message][1] Note that when changing the `iframe.src` to just `base`, the iframe instead renders a “sad page”, and that a console error indicates that the culprit is `X-Frame-Options: sameorigin`. [1]: https://user-images.githubusercontent.com/4317806/60227895-cf530280-9845-11e9-93f0-cc5159b88e31.png wchargin-branch: colab-csp
@@ -344,13 +373,17 @@ class are WSGI applications. | |||
parsed_url = urlparse.urlparse(request.path) | |||
clean_path = _clean_path(parsed_url.path, self._path_prefix) | |||
|
|||
@functools.wraps(start_response) | |||
def new_start_response(status, headers): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there a reason not to do this in http_util.Respond()
? That's where the other automatically populated headers are set:
tensorboard/tensorboard/backend/http_util.py
Lines 148 to 158 in 63ab35a
headers = [] | |
headers.append(('Content-Length', str(content_length))) | |
if content_encoding: | |
headers.append(('Content-Encoding', content_encoding)) | |
if expires > 0: | |
e = wsgiref.handlers.format_date_time(time.time() + float(expires)) | |
headers.append(('Expires', e)) | |
headers.append(('Cache-Control', 'private, max-age=%d' % expires)) | |
else: | |
headers.append(('Expires', '0')) | |
headers.append(('Cache-Control', 'no-cache, must-revalidate')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stephan and I talked about this briefly and thought that it was
reasonable to do in middleware. IMHO, we should consider moving most of
Respond
’s functionality into middleware—our plugin API centers around
WSGI apps, which aren’t tied to Werkzeug or its Request.application
wrapper at all, so it’s strange to have this additional integration
point. What if plugin developers prefer Flask?
The alternative would be to make http_util
a public module or move
Request
into plugin_util
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a fair point, but I guess it seems like then we should have a TODO/issue open to move that logic into the middleware, so there's a rationale available for why otherwise similar code is going in two different places.
Summary: This commit broke Jupyter notebook integration: a content security policy that whitelists Colab also blocks access by any Jupyter notebooks. I don’t see a simple fix for this other than permitting all frame ancestors: Jupyter origins are arbitrary, and we can’t use `'self'` because Jupyter and TensorBoard are not in fact served from the same origin. This reverts commit ff05d1a. Test Plan: Build the Pip package, then install it into a virtualenv with Jupyter. From a Jupyter notebook, run `%tensorboard --logdir logs`. Before this commit, that rendered an empty cell and printed a console error, > Refused to display 'http://localhost:6006/' in a frame because an > ancestor violates the following Content Security Policy directive: > "frame-ancestors https://*.googleusercontent.com > https://*.google.com". As of this commit, this renders a working TensorBoard. wchargin-branch: revert-colab-csp
Summary: This commit broke Jupyter notebook integration: a content security policy that whitelists Colab also blocks access by any Jupyter notebooks. I don’t see a simple fix for this other than permitting all frame ancestors: Jupyter origins are arbitrary, and we can’t use `'self'` because Jupyter and TensorBoard are not in fact served from the same origin. This reverts commit ff05d1a. Test Plan: Build the Pip package, then install it into a virtualenv with Jupyter. From a Jupyter notebook, run `%tensorboard --logdir logs`. Before this commit, that rendered an empty cell and printed a console error, > Refused to display 'http://localhost:6006/' in a frame because an > ancestor violates the following Content Security Policy directive: > "frame-ancestors https://\*.googleusercontent.com > https://\*.google.com". As of this commit, this renders a working TensorBoard. wchargin-branch: revert-colab-csp
Summary:
This is required to use the new
google.colab.kernel.proxyPort
Colabfeature (see Google-internal http://b/130310433).
Test Plan:
The included tests verify that the mechanism is implemented as intended.
To verify that the mechanism actually works in Colab, create a Colab
notebook with the following cells:
Run the notebook and verify that the final output frame renders
properly:
Note that when changing the
iframe.src
to justbase
, the iframeinstead renders a “sad page”, and that a console error indicates that
the culprit is
X-Frame-Options: sameorigin
.wchargin-branch: colab-csp