Token appears in the download URL, being shared by accident
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:
// 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
Workbench 2 collection should do something like:
- Provide "copy link to clipboard" in the context menu. The copied link must not have the token.
- 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
- 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=.../")
- Keep-web will respond with a redirect which strips ?api_token from the URL and puts the token in a cookie.
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 <email@example.com>
#9 Updated by Daniel Kutyła 4 months ago
New version: https://dev.arvados.org/projects/arvados/repository/arvados-workbench2/revisions/055723f58e163f0b5e49c1e8b92fd1bebe81873e
Test run: https://ci.arvados.org/job/developer-tests-workbench2/99/
Added new way of downloading files
#10 Updated by Daniel Kutyła 4 months ago
#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
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:
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.
#13 Updated by Daniel Kutyła 3 months ago
New version: https://dev.arvados.org/projects/arvados/repository/arvados-workbench2/revisions/9525ed95bef2a8de63b48a0682c342465d29bae9
Test run: https://ci.arvados.org/job/developer-tests-workbench2/126/
Removed download attribute, fixed redirectTo
#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>
#16 Updated by Daniel Kutyła 3 months ago
New version first commit: https://dev.arvados.org/projects/arvados/repository/arvados-workbench2/revisions/ffc9666a04b0cef18a15571c1a0f4fdc41e87b75
Test run: https://ci.arvados.org/job/developer-tests-workbench2/128/
Fixed tests and redirect
#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:
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.
#18 Updated by Daniel Kutyła 3 months ago
New version first commit: https://dev.arvados.org/projects/arvados/repository/arvados-workbench2/revisions/67d77b206ebee7c776bd4d7dbc112ebb0c7ba800
Test run: https://ci.arvados.org/job/developer-tests-workbench2/129/
#20 Updated by Daniel Kutyła 3 months ago
- Status changed from In Progress to Resolved
Applied in changeset arvados-workbench2|35ce0164f3863e7117fade1319ed3c2789bc216a.
#21 Updated by Lucas Di Pentima 2 months ago
- Target version changed from 2020-10-21 Sprint to 2020-11-18
- Status changed from Resolved to In Progress
- File Captura de Pantalla 2020-11-11 a la(s) 18.38.49.png Captura de Pantalla 2020-11-11 a la(s) 18.38.49.png added
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)
#28 Updated by Daniel Kutyła about 1 month ago
New version first commit: https://dev.arvados.org/projects/arvados/repository/arvados-workbench2/revisions/55c77cf6945551bbb2500c213be48db84315b445
Test run: https://ci.arvados.org/job/developer-tests-workbench2/215/
Fixed img preview for collection items
#29 Updated by Lucas Di Pentima about 1 month ago
Reviewing arvados-workbench2|55c77cf - branch
- The test suite is named after a different component
- Instead of “test.com” as a fake domain please use “example.com” as we do in other tests.
- Testing this fix on arvbox doesn’t work, the request fails with status 401 (unauthorized)
- Example URL on “master” branch: https://10.1.1.7:9004/c=xupkx-4zz18-l4r1mbaajnbu1qz/t=v2/xupkx-gj3su-ru0tjgov29ijmyy/5otoq9c65yl6uef63ojpsd35bgj2gvmgzsmjowyz1ii85qz4ic/Sculptor-150sec-2.jpg (fails with 404).
- Same URL on this branch: https://10.1.1.7:9004/c=xupkx-4zz18-l4r1mbaajnbu1qz/Sculptor-150sec-2.jpg (fails with 401 — token is missing).
- What I think is happening here is that the v2 token needs to be url-encoded, so its ‘
/‘ chars don’t get interpreted as path separators.
#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)
#31 Updated by Daniel Kutyła about 1 month ago
New version first commit: https://dev.arvados.org/projects/arvados/repository/arvados-workbench2/revisions/a1abf5536a6d65f6ede59748cfbb9afceb249a83
Test run: https://ci.arvados.org/job/developer-tests-workbench2/216/
Added token as query param
#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-hashfor the collection’s uuid, it should use
- Instead of
t=test-token/test-token2/test-token3for the token, it should use something like
- The tests will work the same, but for code maintainability I think is convenient.
- Instead of
- 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.
#36 Updated by Daniel Kutyła about 1 month ago
- % Done changed from 50 to 100
- Status changed from Feedback to Resolved
Applied in changeset arvados-workbench2|0823c236e7a046ea77e60798c622602ac2a77f98.