Bug #16812

Token appears in the download URL, being shared by accident

Added by Peter Amstutz 4 months ago. Updated about 1 month ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Workbench2
Target version:
Start date:
10/07/2020
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Description

Users are sharing download URLs with embedded user tokens. Workbench2 should hand off to keep-web in a way that does not expose the token to the user.

I believe the way Workbench 1 does it is by linking to a special workbench path, which returns a redirect which includes ?api_token in the query, when keep-web gets the request it returns a cookie and another redirect to the final URL with the ?api_token stripped, this is the one the user sees, with the token safely stashed in a cookie.

The different methods of doing token hand-off are described here:

https://dev.arvados.org/projects/arvados/repository/revisions/master/entry/services/keep-web/doc.go

// If a token is provided in a query string or in a POST request, the
// response is an HTTP 303 redirect to an equivalent GET request, with
// the token stripped from the query string and added to a cookie
// instead.

Workbench 2 collection should do something like:

  1. Provide "copy link to clipboard" in the context menu. The copied link must not have the token.
    1. This should probably be a special workbench2 link which will verify the user is logged in (or go through the login dance) and then redirect to keep-web as described next
  2. The "open file" and "open in new tab" behaviors should navigate to the download location with ?api_token in the query (it must not include the token in the path with "/t=.../")
  3. Keep-web will respond with a redirect which strips ?api_token from the URL and puts the token in a cookie.
Captura de Pantalla 2020-11-11 a la(s) 18.38.49.png (148 KB) Captura de Pantalla 2020-11-11 a la(s) 18.38.49.png Missing image preview Lucas Di Pentima, 11/11/2020 09:41 PM
img preview failed on ce8i5.png (93.3 KB) img preview failed on ce8i5.png Lucas Di Pentima, 12/08/2020 02:38 PM

Subtasks

Task #16882: Review 16812-token-appears-in-the-download-URLResolvedPeter Amstutz

Task #17200: Review: 16812-images-issue-fixIn ProgressDaniel Kutyła


Related issues

Related to Arvados - Bug #17202: [keep-web] Don't use 303-with-cookie when serving inline preview content as a third-party siteNew12/09/2020

Associated revisions

Revision 8127b5d5 (diff)
Added by Peter Amstutz 3 months ago

Separate inline/download keep-web ports refs #16812

This makes inline viewing of private files work as expected with
arvbox, which is helpful for development and testing.

It relies on "TrustAllContent: true", so it is also XSS insecure.

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

Revision 35ce0164
Added by Daniel Kutyła 3 months ago

Merge branch '16812-token-appears-in-the-download-URL'
Closes #16812

Arvados-DCO-1.1-Signed-off-by: Daniel Kutyła <>

Revision 0823c236
Added by Daniel Kutyła about 1 month ago

Merge branch '16812-images-issue-fix'
Closes #16812

Arvados-DCO-1.1-Signed-off-by: Daniel Kutyła <>

History

#1 Updated by Peter Amstutz 4 months ago

  • Subject changed from Token appears in the URL, being shared by accident to Token appears in the download URL, being shared by accident

#2 Updated by Peter Amstutz 4 months ago

  • Description updated (diff)

#3 Updated by Peter Amstutz 4 months ago

  • Description updated (diff)

#4 Updated by Peter Amstutz 4 months ago

  • Target version set to 2020-09-23 Sprint

#5 Updated by Peter Amstutz 4 months ago

  • Assigned To set to Daniel Kutyła
  • Category set to Workbench2

#6 Updated by Peter Amstutz 4 months ago

  • Description updated (diff)

#7 Updated by Peter Amstutz 4 months ago

  • Target version changed from 2020-09-23 Sprint to 2020-10-07 Sprint

#8 Updated by Peter Amstutz 4 months ago

  • Status changed from New to In Progress

#11 Updated by Peter Amstutz 3 months ago

  • Target version changed from 2020-10-07 Sprint to 2020-10-21 Sprint

#12 Updated by Peter Amstutz 3 months ago

The "Download" and "Open in new tab" menu items need to use different links. Neither link should use the /t=.../ form for tokens.

The "Download" menu item should have "download" anchor attribute set, and open a download dialog in the browser. The download link should use the base URL from Services.WebDAVDownload.ExternalURL.

The "Open in new tab" menu item should not have the "download" anchor attribute. The link should use the base URL from Services.WebDAV.ExternalURL.

The "Copy to clipboard" should not use an open redirect. The redirect could be used to easily steal someone's token. The link should be something like:

https://workbench2.example.com/collections/x2b8c-4zz18-9i4shmy31jxxfbe/download/hello.txt

Visiting this link should redirect the user to the file using the same link as "Open in new tab (but opening in the current tab, not a new one).

If the user arrives at Workbench2 without a token, the path should be stored in localstorage, and then restored after successful login.

#14 Updated by Peter Amstutz 3 months ago

So I started writing comments but I realized I didn't know what to tell you so I just went and started hacking on it. I discovered some other infrastructure issues in the process (you can see the conversation in the development gitter).

Take a look at these changes. I didn't run tests so I don't know if anything else needs to be updated.

16812-token-appears-in-the-download-URL @ arvados-workbench2|ae94f4d8463ff6350329e802cb902c8dad96a710

16812: Handoff token using query param

Need to pass the token to keep-web without it being "sticky" in the
URL bar. Using a query param accomplishes this, because keep-web
knows to strip the api_token query parameter and respond with redirect
and a cookie which the browser can use to fetch the file safely.

Also distinguish between KeepWebService (now the download service) and
KeepWebInlineService (the one that will serve content that can be
displayed inline in the browser if it is safe to do so).

#15 Updated by Peter Amstutz 3 months ago

Here's another way to try to pass the token without exposing it in the URL:

<form action="https://keep_web_url/c=.../file.txt" method="get" target="_blank">
  <input type="hidden" id="api_token" name="api_token" value="xyz" />
  <input type="submit" class="link-button">Open in new tab</input>
</form>

#17 Updated by Peter Amstutz 3 months ago

Let me try to clarify a few things.

The API that we are talking to is the WebDAV API, which is documented here:

https://doc.arvados.org/v2.1/api/keep-webdav.html

This documentation page is new (added on Friday) so I couldn't give it to you earlier.

There are two endpoints. The "Services.WebDAV" endpoint (keepWebInlineServiceUrl) and "Services.WebDAVDownload" (keepWebServiceUrl) endpoint.

The "inline" endpoint may respond such that a browser can render the file inline.

The "download" endpoint responds so that the browser will always invokes the download behavior.

When using "open in new tab" we want the "inline" endpoint and using "download" we want the "download" endpoint.

When sending the user to the download endpoint, the ?disposition=attachement is redundant. (That parameter does change the behavior if you send the user to the "inline" endpoint).

Finally, although the way the react context menu behaves makes it difficult or impossible to right-click copy links, the link (with the token) still shows up in the browser. Instead of using an anchor tag, it should be possible to use a form to provide the token as a hidden parameter, as in my example in #note-15. Then the token won't be visible to the user.

Also, I noticed in src/services/collection-service.ts:extendFileURL it is splitting the API token and only taking the secret portion. This is wrong. It should be using the complete token, but it should be URL-encoded.

I know we have already gone back and forth on this a couple of times, I don't want you to get frustrated so if you would prefer I can also take over the branch.

#19 Updated by Peter Amstutz 3 months ago

I think you committed "open-in-new-tab.actions.ts" by accident.

Otherwise LGTM.

#20 Updated by Daniel Kutyła 3 months ago

  • Status changed from In Progress to Resolved

#21 Updated by Lucas Di Pentima 2 months ago

I've seen that from arvados-workbench2|35ce0164 the preview on image files on the collection UI is not working properly (see attached capture).

The error is 404 on a URL like: https://10.1.1.7:9002/c=xupkx-4zz18-hziaxysm1b6mkvb/t=v2/xupkx-gj3su-695z0z7iv5xv04b/4zb24asu1eodzpqtsou3boo3wa28ox9zyymqu9k5rfjtq94let/Sculptor-150sec-2.jpg (from my local arvbox instance)

#22 Updated by Peter Amstutz 2 months ago

Make sure the image link is using the "inline" webdav URL, not the "download" one.

#23 Updated by Peter Amstutz 2 months ago

  • Release changed from 31 to 36

#24 Updated by Peter Amstutz 2 months ago

  • Release changed from 36 to 37

#25 Updated by Peter Amstutz 2 months ago

  • Target version changed from 2020-11-18 to 2020-12-02 Sprint

#26 Updated by Peter Amstutz about 2 months ago

  • Status changed from In Progress to Feedback

#27 Updated by Peter Amstutz about 2 months ago

  • Target version changed from 2020-12-02 Sprint to 2020-12-16 Sprint

#29 Updated by Lucas Di Pentima about 1 month ago

Reviewing arvados-workbench2|55c77cf - branch 16812-images-issue-fix

#30 Updated by Lucas Di Pentima about 1 month ago

I've also tried pointing this branch to ce8i5 and made the collection ce8i5-4zz18-uzsmr4zmgs1rkuz with a couple images in it (shared with you). It also failed (screenshot attached)

#32 Updated by Lucas Di Pentima about 1 month ago

  • Could you please replace the “test.com” domain usage with “example.com” on the tests? Example.com is meant to be used on tests/examples (https://tools.ietf.org/html/rfc2606#section-3), and “test.com” is a real existing domain.
  • Continuing with the tests: I think for readability purposes the mocked file URL should have more correct (format wise) data. Example:
    • Instead of c=test-hash for the collection’s uuid, it should use c=zzzzz-4zz18-0123456789abcde (https://doc.arvados.org/v2.1/api/methods/collections.html)
    • Instead of t=test-token/test-token2/test-token3 for the token, it should use something like v2/zzzzz-gj3su-0123456789abcde/xxxxxxtokenxxxxx
    • The tests will work the same, but for code maintainability I think is convenient.
  • I tried it and it worked ok on Firefox but not Chrome nor Safari.
    • I think Chrome is behaving more strictly that others and recently enabled a cross site protection for set-cookies headers. (https://blog.chromium.org/2019/10/developers-get-ready-for-new.html)
    • Couldn’t confirm if Safari has the same issue, but it’s probably not that important
    • On the other hand, if I test the commit just before the one that broke this feature, it works fine (tested it against ce8i5) on Chrome too. This is the last commit before the preview stopped working: 970a9e9dcd2a444d02181c4df3f205f7e0a8ebeb — it was using the v1 token on the file path. I think this got caught by the original fix on this story, to avoid sharing URLs with embedded tokens, but the URL used for the preview doesn’t get shared so we may want to keep using it in the url's path.
  • I manually tried to get the image passing the /t=v2%Fce8i5-gj3su-0123456789abcde%2Fxxxxxxtokenxxxxx (with slashes encoded as %2F) path part on the URL and it didn’t work, we should confirm with Peter or Tom if keep-web supports it.

#33 Updated by Lucas Di Pentima about 1 month ago

As a reference, it seems that the SameSite=Lax attribute will be eventually enabled by all browsers: https://web.dev/samesite-cookie-recipes/

#34 Updated by Lucas Di Pentima about 1 month ago

Tom fixed keep-web so that it adds SameSite=None; Secure attributes to the set-cookie header. With that, I can confirm that Chrome is happy.

So, after making the test cosmetic improvements, it will LGTM.

#35 Updated by Tom Clegg about 1 month ago

  • Related to Bug #17202: [keep-web] Don't use 303-with-cookie when serving inline preview content as a third-party site added

#36 Updated by Daniel Kutyła about 1 month ago

  • % Done changed from 50 to 100
  • Status changed from Feedback to Resolved

Also available in: Atom PDF