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

Reviewer: URLs starting with "//" do not load #6102

Closed
david-allison opened this issue May 4, 2020 · 9 comments · Fixed by #7666
Closed

Reviewer: URLs starting with "//" do not load #6102

david-allison opened this issue May 4, 2020 · 9 comments · Fixed by #7666
Assignees
Labels
Accepted Maintainers welcome a PR implementing this feature Good First Issue! Help Wanted Requesting Pull Requests from volunteers Keep Open avoids the stale bot Reviewer
Milestone

Comments

@david-allison
Copy link
Member

Anki loads URLs beginning with //, AnkiDroid fails

My presumption: In AnkiDroid, // refers to the root of the filesystem, rather than a selection between HTTP or HTTPS

Card source

<img alt="6.1 Russian road sign.svg" data-file-height="2250" data-file-width="1500" decoding="async" height="75" src="//upload.wikimedia.org/wikipedia/commons/thumb/5/50/6.1_Russian_road_sign.svg/50px-6.1_Russian_road_sign.svg.png" srcset="//upload.wikimedia.org/wikipedia/commons/thumb/5/50/6.1_Russian_road_sign.svg/75px-6.1_Russian_road_sign.svg.png 1.5x, //upload.wikimedia.org/wikipedia/commons/thumb/5/50/6.1_Russian_road_sign.svg/100px-6.1_Russian_road_sign.svg.png 2x" width="50"/>

Chrome error

75px-6.1_Russian_road_sign.svg.png:1 Failed to load resource: net::ERR_INVALID_URL

Card works if the sources are changed from // to https://

Originally posted by @david-allison-1 in #6101 (comment)

@david-allison
Copy link
Member Author

david-allison commented May 4, 2020

shouldInterceptRequest fires, but shouldOverrideUrlLoading doesn't

On a sample card:

request.getUrl().toString():
file://upload.wikimedia.org/wikipedia/commons/thumb/e/eb/6.4_Russian_road_sign.svg/75px-6.4_Russian_road_sign.svg.png

The above is a shame as it doesn't contain the // prefix, I've had a shot at the overloads, and none seem to provide it.

We can fix this by mapping file:// -> http(s), and file:/// to file:///.

Relative URLs come in as file:///:

file:///storage/emulated/0/AnkiDroidTEST/collection.media/paste-33779ea505e78e7dd39c14eadb3741ab51cb52f9.png


This leaves the questions:

  • Does Anki use HTTP, or HTTPS?
  • Do we want to restrict file:/// URLS to be inside collection.media (if not done already)? I really want to do this from a security perspective.

@david-allison david-allison changed the title Reviewer: URLs starting with // do not load Reviewer: URLs starting with "//" do not load May 4, 2020
@mikehardy
Copy link
Member

Those aren't valid URLs are they? Why would we accept URLs without a scheme? Seems Anki upstream should reject them too? Or am I missing some chunk of URL RFC that says "in the absence of a scheme this is the default..." ?

@david-allison
Copy link
Member Author

Those aren't valid URLs are they? Why would we accept URLs without a scheme? Seems Anki upstream should reject them too? Or am I missing some chunk of URL RFC that says "in the absence of a scheme this is the default..." ?

// as a prefix is valid as it's a Protocol Relative URL

I believe this is: https://tools.ietf.org/html/rfc3986#section-4.2

@mikehardy
Copy link
Member

I do love reading RFCs 🤓 - almost always learn something. HTTPS is my strong preference, and agreed on restricting file:/// to be in collection - willing to reconsider if there is a revolt after attempting it but would try first

@mikehardy mikehardy added Help Wanted Requesting Pull Requests from volunteers Accepted Maintainers welcome a PR implementing this feature labels May 18, 2020
@github-actions
Copy link
Contributor

Hello 👋, this issue has been opened for more than 2 months with no activity on it. If the issue is still here, please keep in mind that we need community support and help to fix it! Just comment something like still searching for solutions and if you found one, please open a pull request! You have 7 days until this gets closed automatically

@github-actions
Copy link
Contributor

Hello 👋, this issue has been opened for more than 2 months with no activity on it. If the issue is still here, please keep in mind that we need community support and help to fix it! Just comment something like still searching for solutions and if you found one, please open a pull request! You have 7 days until this gets closed automatically

@github-actions github-actions bot added the Stale label Sep 15, 2020
@david-allison david-allison added Keep Open avoids the stale bot and removed Stale labels Sep 15, 2020
@david-allison
Copy link
Member Author

Much easier now, we're HTTPS only

@Sofrosyn
Copy link

Sounds cool

@david-allison
Copy link
Member Author

david-allison commented Nov 13, 2020

I've tried to fix this, it works on my Android 9, but it's not feasible, and there were issues with timeouts on lower APIs

The problem is that shouldInterceptRequest won't allow you to respond with a HTTP 3xx.

This means a synchronous call to OkHTTP to obtain the image.

A synchronous blocking call isn't feasible - it can theoretically block until a HTTP timeout.

So I'll add a snackabar and a message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Maintainers welcome a PR implementing this feature Good First Issue! Help Wanted Requesting Pull Requests from volunteers Keep Open avoids the stale bot Reviewer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants