[ARVADOS] created: b59ac41bf409566da73e43ecf4927b40729fc72c

git at public.curoverse.com git at public.curoverse.com
Thu Dec 17 01:08:15 EST 2015


        at  b59ac41bf409566da73e43ecf4927b40729fc72c (commit)


commit b59ac41bf409566da73e43ecf4927b40729fc72c
Author: Tom Clegg <tom at curoverse.com>
Date:   Thu Dec 17 01:07:32 2015 -0500

    7884: Detect when a "download log" response has a JSON-encoded redirect
    to keep-web, and convert it to POST to avoid a second redirect that
    would be forbidden by SOP.

diff --git a/apps/workbench/app/views/jobs/_show_log.html.erb b/apps/workbench/app/views/jobs/_show_log.html.erb
index 1802134..7d67b74 100644
--- a/apps/workbench/app/views/jobs/_show_log.html.erb
+++ b/apps/workbench/app/views/jobs/_show_log.html.erb
@@ -62,38 +62,62 @@ var makeFilter = function() {
 <% if @object.log and !@object.log.empty? %>
   <% logcollection = Collection.find @object.log %>
   <% if logcollection %>
-    log_size = <%= logcollection.files[0][2] %>
-    log_maxbytes = <%= Rails.configuration.log_viewer_max_bytes %>;
-    logcollection_url = '<%=j url_for logcollection %>/<%=j logcollection.files[0][1] %>';
+    var log_size = <%= logcollection.files[0][2] %>
+    var log_maxbytes = <%= Rails.configuration.log_viewer_max_bytes %>;
+    var logcollection_url = '<%=j url_for logcollection %>/<%=j logcollection.files[0][1] %>';
     $("#log-viewer-download-url").attr('href', logcollection_url);
     $("#log-viewer-download-pane").show();
+    var headers = {};
     if (log_size > log_maxbytes) {
-      range_header = { 'Range': 'bytes=0-' + log_maxbytes };
-    } else {
-      range_header = null;
+      headers['Range'] = 'bytes=0-' + log_maxbytes;
     }
-    $.ajax(logcollection_url, { headers: range_header }).
-        done(function(data, status, jqxhr) {
-            logViewer.filter();
-            addToLogViewer(logViewer, data.split("\n"), taskState);
-            logViewer.filter(makeFilter());
-            content_range_hdr = jqxhr.getResponseHeader('Content-Range');
-            var v = content_range_hdr && content_range_hdr.match(/bytes \d+-(\d+)\/(.+)/);
-            short_log = v && (v[2] == '*' || parseInt(v[1]) + 1 < v[2]);
-            if (jqxhr.status == 206 && short_log) {
-              $("#log-viewer-overview").html(
-                '<p>Showing only ' + data.length + ' bytes of this log.' +
-                ' Timing information is unavailable since' +
-                ' the full log was not retrieved.</p>'
-              );
-            } else {
-              generateJobOverview("#log-viewer-overview", logViewer, taskState);
+    var ajax_opts = { dataType: 'text', headers: headers };
+    load_log();
+
+    function load_log() {
+        $.ajax(logcollection_url, ajax_opts).done(done).fail(fail);
+    }
+    function done(data, status, jqxhr) {
+        if (jqxhr.getResponseHeader('Content-Type').indexOf('application/json') === 0) {
+            // The browser won't allow a redirect-with-cookie response
+            // because keep-web isn't same-origin with us. Instead, we
+            // assure keep-web it's OK to respond with the content
+            // immediately by setting the token in the request body
+            // instead and adding disposition=attachment.
+            logcollection_url = JSON.parse(data).href;
+            var queryAt = logcollection_url.indexOf('?api_token=');
+            if (queryAt >= 0) {
+                ajax_opts.method = 'POST';
+                ajax_opts.data = {
+                    api_token: logcollection_url.slice(queryAt+11),
+                    disposition: 'attachment',
+                };
+                logcollection_url = logcollection_url.slice(0, queryAt);
             }
-            $("#log-viewer .spinner").detach();
-        }).
-        fail(function(jqxhr, status, error) {
-            $("#log-viewer .spinner").detach();
-        });
+            return load_log();
+        }
+        logViewer.filter();
+        addToLogViewer(logViewer, data.split("\n"), taskState);
+        logViewer.filter(makeFilter());
+        content_range_hdr = jqxhr.getResponseHeader('Content-Range');
+        var v = content_range_hdr && content_range_hdr.match(/bytes \d+-(\d+)\/(.+)/);
+        short_log = v && (v[2] == '*' || parseInt(v[1]) + 1 < v[2]);
+        if (jqxhr.status == 206 && short_log) {
+            $("#log-viewer-overview").html(
+                '<p>Showing only ' + data.length + ' bytes of this log.' +
+                    ' Timing information is unavailable since' +
+                    ' the full log was not retrieved.</p>'
+            );
+        } else {
+            generateJobOverview("#log-viewer-overview", logViewer, taskState);
+        }
+        $("#log-viewer .spinner").detach();
+    }
+    function fail(jqxhr, status, error) {
+        // TODO: tell the user about the error
+        console.log('load_log failed: status='+status+' error='+error);
+        $("#log-viewer .spinner").detach();
+    }
   <% end %>
 <% else %>
   <%# Live log loading not implemented yet. %>
diff --git a/apps/workbench/test/integration/download_test.rb b/apps/workbench/test/integration/download_test.rb
index ed91ae0..8a16fb8 100644
--- a/apps/workbench/test/integration/download_test.rb
+++ b/apps/workbench/test/integration/download_test.rb
@@ -2,16 +2,10 @@ require 'integration_helper'
 require 'helpers/download_helper'
 
 class DownloadTest < ActionDispatch::IntegrationTest
-  def getport service
-    File.read(File.expand_path("../../../../../tmp/#{service}.port", __FILE__))
-  end
+  include KeepWebConfig
 
   setup do
-    @kwport = getport 'keep-web-ssl'
-    @kwdport = getport 'keep-web-dl-ssl'
-    Rails.configuration.keep_web_url = "https://localhost:#{@kwport}/c=%{uuid_or_pdh}"
-    Rails.configuration.keep_web_download_url = "https://localhost:#{@kwdport}/c=%{uuid_or_pdh}"
-    CollectionsController.any_instance.expects(:file_enumerator).never
+    use_keep_web_config
 
     # Make sure Capybara can download files.
     need_selenium 'for downloading', :selenium_with_download
diff --git a/apps/workbench/test/integration/jobs_test.rb b/apps/workbench/test/integration/jobs_test.rb
index b8d3890..0c407b3 100644
--- a/apps/workbench/test/integration/jobs_test.rb
+++ b/apps/workbench/test/integration/jobs_test.rb
@@ -4,6 +4,8 @@ require 'tmpdir'
 require 'integration_helper'
 
 class JobsTest < ActionDispatch::IntegrationTest
+  include KeepWebConfig
+
   setup do
       need_javascript
   end
@@ -66,6 +68,26 @@ class JobsTest < ActionDispatch::IntegrationTest
     assert page.has_text? 'Showing only 100 bytes of this log'
   end
 
+  test 'view log via keep-web redirect' do
+    use_keep_web_config
+
+    token = api_fixture('api_client_authorizations')['active']['api_token']
+    logdata = fakepipe_with_log_data.read
+    logblock = `echo -n #{logdata.shellescape} | ARVADOS_API_TOKEN=#{token.shellescape} arv-put --no-progress --raw -`.strip
+    assert $?.success?, $?
+
+    job = nil
+    use_token 'active' do
+      job = Job.find api_fixture('jobs')['running']['uuid']
+      mtxt = ". #{logblock} 0:#{logdata.length}:#{job.uuid}.log.txt\n"
+      logcollection = Collection.create(manifest_text: mtxt)
+      job.update_attributes log: logcollection.portable_data_hash
+    end
+    visit page_with_token 'active', '/jobs/'+job.uuid
+    find('a[href="#Log"]').click
+    assert_text 'log message 1'
+  end
+
   [
     ['foobar', false, false],
     ['job_with_latest_version', true, false],
diff --git a/apps/workbench/test/integration_helper.rb b/apps/workbench/test/integration_helper.rb
index 48d2cb5..785912d 100644
--- a/apps/workbench/test/integration_helper.rb
+++ b/apps/workbench/test/integration_helper.rb
@@ -126,6 +126,20 @@ module HeadlessHelper
   end
 end
 
+module KeepWebConfig
+  def getport service
+    File.read(File.expand_path("../../../../tmp/#{service}.port", __FILE__))
+  end
+
+  def use_keep_web_config
+    @kwport = getport 'keep-web-ssl'
+    @kwdport = getport 'keep-web-dl-ssl'
+    Rails.configuration.keep_web_url = "https://localhost:#{@kwport}/c=%{uuid_or_pdh}"
+    Rails.configuration.keep_web_download_url = "https://localhost:#{@kwdport}/c=%{uuid_or_pdh}"
+    CollectionsController.any_instance.expects(:file_enumerator).never
+  end
+end
+
 class ActionDispatch::IntegrationTest
   # Make the Capybara DSL available in all integration tests
   include Capybara::DSL

commit fa1bb089c55056fc25cf888b11b43b3a2ac2feca
Author: Tom Clegg <tom at curoverse.com>
Date:   Thu Dec 17 01:03:37 2015 -0500

    7884: Serve simple cross-origin AJAX POST requests without redirecting.

diff --git a/services/keep-web/handler.go b/services/keep-web/handler.go
index 847329f..3601003 100644
--- a/services/keep-web/handler.go
+++ b/services/keep-web/handler.go
@@ -100,6 +100,12 @@ func (h *handler) ServeHTTP(wOrig http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	if r.Header.Get("Origin") != "" {
+		// Allow simple cross-origin requests, without
+		// credentials.
+		w.Header().Set("Access-Control-Allow-Origin", "*")
+	}
+
 	arv := clientPool.Get()
 	if arv == nil {
 		statusCode, statusText = http.StatusInternalServerError, "Pool failed: "+clientPool.Err().Error()
@@ -149,7 +155,23 @@ func (h *handler) ServeHTTP(wOrig http.ResponseWriter, r *http.Request) {
 		statusCode = http.StatusNotFound
 		return
 	}
-	if t := r.FormValue("api_token"); t != "" {
+	formToken := r.FormValue("api_token")
+	if formToken != "" && r.Header.Get("Origin") != "" && attachment && r.URL.Query().Get("api_token") == "" {
+		// The client provided an explicit token in the POST
+		// body. The Origin header indicates this *might* be
+		// an AJAX request, in which case redirect-with-cookie
+		// won't work: we should just serve the content in the
+		// POST response. This is safe because:
+		//
+		// * We're supplying an attachment, not inline
+		//   content, so we don't need to convert the POST to
+		//   a GET and avoid the "really resubmit form?"
+		//   problem.
+		//
+		// * The token isn't embedded in the URL, so we don't
+		//   need to worry about bookmarks and copy/paste.
+		tokens = append(tokens, formToken)
+	} else if formToken != "" {
 		// The client provided an explicit token in the query
 		// string, or a form in POST body. We must put the
 		// token in an HttpOnly cookie, and redirect to the
@@ -179,7 +201,7 @@ func (h *handler) ServeHTTP(wOrig http.ResponseWriter, r *http.Request) {
 
 		http.SetCookie(w, &http.Cookie{
 			Name:     "arvados_api_token",
-			Value:    auth.EncodeTokenCookie([]byte(t)),
+			Value:    auth.EncodeTokenCookie([]byte(formToken)),
 			Path:     "/",
 			HttpOnly: true,
 		})
diff --git a/services/keep-web/handler_test.go b/services/keep-web/handler_test.go
index 94a1cf7..8929aa0 100644
--- a/services/keep-web/handler_test.go
+++ b/services/keep-web/handler_test.go
@@ -96,6 +96,21 @@ func authzViaPOST(r *http.Request, tok string) int {
 	return http.StatusUnauthorized
 }
 
+func (s *IntegrationSuite) TestVhostViaXHRPOST(c *check.C) {
+	doVhostRequests(c, authzViaPOST)
+}
+func authzViaXHRPOST(r *http.Request, tok string) int {
+	r.Method = "POST"
+	r.Header.Add("Content-Type", "application/x-www-form-urlencoded")
+	r.Header.Add("Origin", "https://origin.example")
+	r.Body = ioutil.NopCloser(strings.NewReader(
+		url.Values{
+			"api_token":   {tok},
+			"disposition": {"attachment"},
+		}.Encode()))
+	return http.StatusUnauthorized
+}
+
 // Try some combinations of {url, token} using the given authorization
 // mechanism, and verify the result is correct.
 func doVhostRequests(c *check.C, authz authorizer) {
@@ -129,11 +144,15 @@ func doVhostRequestsWithHostPath(c *check.C, authz authorizer, hostPath string)
 			Header:     http.Header{},
 		}
 		failCode := authz(req, tok)
-		resp := doReq(req)
+		req, resp := doReq(req)
 		code, body := resp.Code, resp.Body.String()
 		if tok == arvadostest.ActiveToken {
 			c.Check(code, check.Equals, http.StatusOK)
 			c.Check(body, check.Equals, "foo")
+			// Any request with the token showing in the
+			// URL must either fail or get redirected to
+			// an URL that hides it.
+			c.Check(req.URL.String(), check.Not(check.Matches), `.*api_token.*`)
 		} else {
 			c.Check(code >= 400, check.Equals, true)
 			c.Check(code < 500, check.Equals, true)
@@ -151,11 +170,11 @@ func doVhostRequestsWithHostPath(c *check.C, authz authorizer, hostPath string)
 	}
 }
 
-func doReq(req *http.Request) *httptest.ResponseRecorder {
+func doReq(req *http.Request) (*http.Request, *httptest.ResponseRecorder) {
 	resp := httptest.NewRecorder()
 	(&handler{}).ServeHTTP(resp, req)
 	if resp.Code != http.StatusSeeOther {
-		return resp
+		return req, resp
 	}
 	cookies := (&http.Response{Header: resp.Header()}).Cookies()
 	u, _ := req.URL.Parse(resp.Header().Get("Location"))
@@ -376,6 +395,34 @@ func (s *IntegrationSuite) TestRange(c *check.C) {
 	}
 }
 
+// XHRs can't follow redirect-with-cookie so they rely on method=POST
+// and disposition=attachment (telling us it's acceptable to respond
+// with content instead of a redirect) and an Origin header that gets
+// added automatically by the browser (telling us it's desirable to do
+// so).
+func (s *IntegrationSuite) TestXHRNoRedirect(c *check.C) {
+	u, _ := url.Parse("http://example.com/c=" + arvadostest.FooCollection + "/foo")
+	req := &http.Request{
+		Method:     "POST",
+		Host:       u.Host,
+		URL:        u,
+		RequestURI: u.RequestURI(),
+		Header:     http.Header{
+			"Origin": {"https://origin.example"},
+			"Content-Type": {"application/x-www-form-urlencoded"},
+		},
+		Body: ioutil.NopCloser(strings.NewReader(url.Values{
+			"api_token":   {arvadostest.ActiveToken},
+			"disposition": {"attachment"},
+		}.Encode())),
+	}
+	resp := httptest.NewRecorder()
+	(&handler{}).ServeHTTP(resp, req)
+	c.Check(resp.Code, check.Equals, http.StatusOK)
+	c.Check(resp.Body.String(), check.Equals, "foo")
+	c.Check(resp.Header().Get("Access-Control-Allow-Origin"), check.Equals, "*")
+}
+
 func (s *IntegrationSuite) testVhostRedirectTokenToCookie(c *check.C, method, hostPath, queryString, contentType, reqBody string, expectStatus int, expectRespBody string) *httptest.ResponseRecorder {
 	u, _ := url.Parse(`http://` + hostPath + queryString)
 	req := &http.Request{

-----------------------------------------------------------------------


hooks/post-receive
-- 




More information about the arvados-commits mailing list