8177: Expand trust_all_content comments.

Skip whole check_uri block when trust_all_content.

Fix test name.

diff --git a/apps/workbench/app/controllers/collections_controller.rb b/apps/workbench/app/controllers/collections_controller.rb
index 63af8285b461d56a029c2e339fa4a71e7d250fe7..7a002427cfe97b0418b6f8cda2a671dd0148c10f 100644 (file)
--- a/apps/workbench/app/controllers/collections_controller.rb
+++ b/apps/workbench/app/controllers/collections_controller.rb
@@ -339,7 +339,7 @@ class CollectionsController < ApplicationController
       # Prefer the attachment-only-host when we want an attachment
       # (and when there is no preview link configured)
       tmpl = Rails.configuration.keep_web_download_url
-    else
+    elsif not Rails.configuration.trust_all_content
       check_uri = URI.parse(tmpl % fmt)
       if opts[:query_token] and
           not check_uri.host.start_with?(munged_id + "--") and
@@ -347,9 +347,7 @@ class CollectionsController < ApplicationController
         # We're about to pass a token in the query string, but
         # keep-web can't accept that safely at a single-origin URL
         # template (unless it's -attachment-only-host).
-        unless (Rails.configuration.trust_all_content and tmpl)
-          tmpl = Rails.configuration.keep_web_download_url
-        end
+        tmpl = Rails.configuration.keep_web_download_url
         if not tmpl
           raise ArgumentError, "Download precluded by site configuration"
         end

diff --git a/apps/workbench/config/application.default.yml b/apps/workbench/config/application.default.yml
index 7e8c3aa52e3e8f1e6c00640c75c1aed4dc50d908..239ffcd225da24a0c444851c16e2484c1293ba96 100644 (file)
--- a/apps/workbench/config/application.default.yml
+++ b/apps/workbench/config/application.default.yml
@@ -258,9 +258,17 @@ common:
   # keep_web_download_url: https://download.uuid_prefix.arvadosapi.com/c=%{uuid_or_pdh}
   keep_web_download_url: false
 
-  # In "trust all content" mode, Workbench will redirect users to
-  # keep-web even when that exposes XSS vulnerabilities.
+  # In "trust all content" mode, Workbench will redirect download
+  # requests to keep-web, even in the cases when keep-web would have
+  # to expose XSS vulnerabilities in order to handle the redirect.
   #
-  # When enabling this setting, the corresponding setting on the
-  # keep-web server must also be enabled.
+  # When enabling this setting, the -trust-all-content flag on the
+  # keep-web server must also be enabled.  For more detail, see
+  # https://godoc.org/github.com/curoverse/arvados/services/keep-web
+  #
+  # This setting has no effect in the recommended configuration, where
+  # the host part of keep_web_url begins with %{uuid_or_pdh}: in this
+  # case XSS protection is provided by browsers' same-origin policy.
+  #
+  # The default setting (false) is appropriate for a multi-user site.
   trust_all_content: false

diff --git a/apps/workbench/test/controllers/collections_controller_test.rb b/apps/workbench/test/controllers/collections_controller_test.rb
index 0cd747eb25c4dd01983625d056e4fb3d3d4910c7..45aab3c8575dd94f801536a25cfe6dbb18f2f43c 100644 (file)
--- a/apps/workbench/test/controllers/collections_controller_test.rb
+++ b/apps/workbench/test/controllers/collections_controller_test.rb
@@ -578,7 +578,7 @@ class CollectionsControllerTest < ActionController::TestCase
       assert_equal "https://download.example/c=#{id.sub '+', '-'}/_/w%20a%20z?api_token=#{tok}", @response.redirect_url
     end
 
-    test "Redirect to keep_web_download_url via #{id_type} when trust_all_content enabled" do
+    test "Redirect to keep_web_url via #{id_type} when trust_all_content enabled" do
       Rails.configuration.trust_all_content = true
       setup_for_keep_web('https://collections.example/c=%{uuid_or_pdh}',
                          'https://download.example/c=%{uuid_or_pdh}')

diff --git a/services/keep-web/doc.go b/services/keep-web/doc.go
index 5a66d8600d76bc2dbd57df4f120d9cc4b784ab91..9ca732f01af40e4fd4206677c0fcc7838db1ff10 100644 (file)
--- a/services/keep-web/doc.go
+++ b/services/keep-web/doc.go
@@ -217,7 +217,7 @@
 //
 // In "trust all content" mode, Keep-web will accept credentials (API
 // tokens) and serve any collection X at
-// "https://collections.example.com/collections/X/path/file.ext".
+// "https://collections.example.com/c=X/path/file.ext".
 // This is UNSAFE except in the special case where everyone who is
 // able write ANY data to Keep, and every JavaScript and HTML file
 // written to Keep, is also trusted to read ALL of the data in Keep.
@@ -233,6 +233,8 @@
 //   keep-web -listen :9999 -attachment-only-host domain.example:9999 -trust-all-content
 //
 // Depending on your site configuration, you might also want to enable
-// "trust all content" setting on Workbench, in which case Workbench will
-// redirect users to keep-web even when that exposes XSS vulnerabilities.
+// "trust all content" setting on Workbench. Normally, Workbench
+// avoids redirecting requests to keep-web if they depend on
+// -trust-all-content being set.
+//
 package main