Skip to content
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

[Taildrop] - Unable to retrieve files that have "%XX" sequences #2288

Closed
sheran opened this issue Jun 30, 2021 · 4 comments · Fixed by #2421
Closed

[Taildrop] - Unable to retrieve files that have "%XX" sequences #2288

sheran opened this issue Jun 30, 2021 · 4 comments · Fixed by #2421
Labels

Comments

@sheran
Copy link

sheran commented Jun 30, 2021

Sending file from: iOS 14.6 and Tailscale v 1.10.0
Receiving file on: Ubuntu Linux 20.04 and Tailscale v 1.10.0
Tailscale Account Type: Personal

I sent a file that had a space character ("QR code.png.png") to my Linux box. The file shows up on Linux as follows:

root@penguin:/var/lib/tailscale/files/[profile name]# ls -al
total 632
drwx------ 2 root root   4096 Jun 30 17:14 .
drwx------ 3 root root   4096 Jun 29 16:52 ..
-rw-r--r-- 1 root root 637548 Jun 30 17:14 QR%20code.png.png

When an attempt to retrieve it is made, there is an error:

sheran@penguin:~/code/tailscale$ sudo ./tailscale file get .
opening inbox file "QR%20code.png.png": HTTP 500 Internal Server Error: open redacted: no such file or directory

I traced the error to ./client/tailscale/tailscale.go and the GetWaitingFile() function. I believe the error occurs when the request is sent and the filename is url.PathEscape()'d. I added a Printf to check and this is the output of the request:

sheran@penguin:~/code/tailscale$ sudo ./tailscale file get .
http://local-tailscaled.sock/localapi/v0/files/QR%2520code.png.png
opening inbox file "QR%20code.png.png": HTTP 500 Internal Server Error: open redacted: no such file or directory

This shows a request for the filename QR%2520code.png.png instead of QR%20code.png.png

Happy to help fixing it, but I'm not entirely sure how a DCO works.

@sheran sheran changed the title [Taildrop] - Unable to retrieve file's that have "%XX" sequences [Taildrop] - Unable to retrieve files that have "%XX" sequences Jun 30, 2021
@sheran
Copy link
Author

sheran commented Jun 30, 2021

I think I've narrowed it down to the diskPath() function in ./ipn/ipnlocal/peerapi.go. I believe I have fixed it on my local source, but want to do further testing before I submit a PR.

@sheran
Copy link
Author

sheran commented Jul 2, 2021

Ok, so I think I may have been too hasty to call out an error in the code. I did some tests among a group of my devices and found that in summary, sending from either MacOS or iOS results in either 1) files copied with escape sequences or 2) files not being found as per my original comment. You can take a look at the full test results here: https://docs.google.com/spreadsheets/d/1nK_KWE-iwFSF8qldXn3tVIlv68Xofg1jt7QkbdB5oCQ/edit?usp=sharing

If you don't want to look at the results, here are the devices I tested:

OS      Version         Tailscale Version
=========================================
Windows 10 Pro 20H2     1.10.0
Mac OS  11.4            1.10.0
Linux   Debian 10       1.10.0
FreeBSD 11.4-RELEASE    1.11.29
Android 10              v1.10.0-405ea978f8b
iOS     14.6            1.10.0

I tested sending a file with a space from each of the devices to each of the other devices. I have not tested platform to same platform.

Success == means the file was received as sent (with the space)
Success - Escape sequence found in file == means the file was received with contents intact, but had an escape seq "%20"
HTTP 500 - File not found == means I did not receive the file and received the error in my first comment above

These tests lead me to suspect that the "Share" feature on Mac OS and iOS somehow either send the file pre-escaped to Tailscale or that the Tailscale front-end ands an additional layer of escaping. I don't actually have a way of verifying as the source to the Mac OS and iOS clients is closed.

@bradfitz
Copy link
Member

bradfitz commented Jul 8, 2021

@sheran, wow, thanks for doing so much debugging for us!

@bradfitz
Copy link
Member

bradfitz commented Jul 9, 2021

Yeah, the macOS/iOS clients (which send the file through a localapi helper handler that then does the peerapi PUT) ultimately send the request double-escaped:

RequestURI=`/v0/put/Screen%2520Shot%25202021-06-28%2520at%25208.27.53%2520PM.png`

It's also a bug that such requests get stuck in tailscaled without being able to retrieved out of the staging area.

I'll fix that latter bug first, and then look at the macOS/iOS double escaping.

bradfitz added a commit that referenced this issue Jul 13, 2021
The localapi was double-unescaping: once by net/http populating
the URL, and once by ourselves later. We need to start with the raw
escaped URL if we're doing it ourselves.

Started to write a test but it got invasive. Will have to add those
tests later in a commit that's not being cherry-picked to a release
branch.

Fixes #2288

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue Jul 13, 2021
The localapi was double-unescaping: once by net/http populating
the URL, and once by ourselves later. We need to start with the raw
escaped URL if we're doing it ourselves.

Started to write a test but it got invasive. Will have to add those
tests later in a commit that's not being cherry-picked to a release
branch.

Fixes #2288

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue Jul 13, 2021
The localapi was double-unescaping: once by net/http populating
the URL, and once by ourselves later. We need to start with the raw
escaped URL if we're doing it ourselves.

Started to write a test but it got invasive. Will have to add those
tests later in a commit that's not being cherry-picked to a release
branch.

Fixes #2288

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue Jul 13, 2021
The localapi was double-unescaping: once by net/http populating
the URL, and once by ourselves later. We need to start with the raw
escaped URL if we're doing it ourselves.

Started to write a test but it got invasive. Will have to add those
tests later in a commit that's not being cherry-picked to a release
branch.

Fixes #2288

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
(cherry picked from commit 98ad7f2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants