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

deprecation of safe_conversion in iri_to_uri breaks itms-services: URIs #2691

Closed
AndyQ opened this issue May 3, 2023 · 12 comments
Closed

deprecation of safe_conversion in iri_to_uri breaks itms-services: URIs #2691

AndyQ opened this issue May 3, 2023 · 12 comments
Milestone

Comments

@AndyQ
Copy link

AndyQ commented May 3, 2023

itms-services whilst undocumented is a validly used scheme and is used to install Enterprise or Ad-Hoc iOS Apps.

The fix for issue #2609 breaks this.

  • Python version: 3.9.16
  • Werkzeug version: 2.3.3
@davidism
Copy link
Member

davidism commented May 3, 2023

Please provide official documentation about the format of the itms-services: URI scheme. Additionally, in my searches that did not turn up anything, it seemed like Apple may have switched to other URI schemes that did not have the same issue; please verify that itms-services is not superseded by another scheme.

@davidism
Copy link
Member

davidism commented May 3, 2023

You can also use direct links instead of redirects:

<a href="itms-services:...">Click here to install the app.</a>

Or a native UI button, etc. This does not require a redirect to navigate to.

@davidism
Copy link
Member

davidism commented May 3, 2023

And as the linked PR mentions, please report this as a bug in urlunsplit to Python.

@AndyQ
Copy link
Author

AndyQ commented May 3, 2023

Here is a link to the Apple documentation (look for the "Use a website to distribute the app" section).
https://support.apple.com/en-gb/guide/deployment/depce7cefc4d/web

The main issue is that I generate a unique link with a short lived token in order to allow the download. This is generated by the backend and a redirect is done over to the itms-services link.

@davidism
Copy link
Member

davidism commented May 3, 2023

At the end of the day though, if you're using the same value for the itms-services URI each time, then someone could copy the redirect location instead and not have to go through that redirect next time. Why not make the unique URL part of the original link:

<a href="itms-services://?action=download-manifest&url=https://example.com/manifest.plist&token=abcde">Download</a>
token = request.args["token"]

if signer.loads(token, max_age=600):
    return send_file("manifest.plist")
else:
    abort(401)

Now the direct link is guarded by a token, rather than only protecting the initial link but not the real URI.

@AndyQ
Copy link
Author

AndyQ commented May 3, 2023

Not quite - I generate a random token which expires after say 10 seconds - just enough to redirect and validate it.
Something like
itms-services://?action=download-manifest&url=https://theacmeinc.com/abcdefeg

Thats currently generated when you click an install button, and its currently a simple redirect over to that link which that installs the app associated with the token.

Any attempt to reuse that token or use it after its expired returns invalid.

I can of course re-write it and do it totally differently but this has been working nicely since about 2010.

@davidism
Copy link
Member

davidism commented May 3, 2023

Did you report this to Python?

@davidism
Copy link
Member

davidism commented May 3, 2023

Basically, this one (invalid?) URI scheme that's unlikely to be used in most applications requires extra code on every single response, which is not great. It requires a call to encode (which will require exception handling overhead if it actually is an IRI with non-ASCII chars), and a call to str.split to look for unlikely spaces. This is unecessary work in the vast, vast majority of cases, and part of the motivation of refactoring our URL parsing was improving speed.

I'm open to fixing this, but it could either be fixed in Python for everyone, or we could figure out a lower impact solution. I don't plan to undeprecate the original solution.

@AndyQ
Copy link
Author

AndyQ commented May 3, 2023

Would it be possible to implement a flag on the response to just pass the redirect url through unchanged - would be more than happy to default that to no and just have an exception path which may well be slower for that as as you mentioned, its only a very tiny number of urls that would be affected.

@davidism
Copy link
Member

davidism commented May 3, 2023

That's the other idea I mention in the linked issue. It would be nice if autocorrect_location_header could be refactored rather than adding a new flag, but it's not clear how to do that.

@davidism davidism changed the title deprecate safe_conversion parameter of iri_to_uri #2609. Breaks itms-services: links deprecation of safe_conversion in iri_to_uri breaks itms-services: URIs May 3, 2023
@davidism
Copy link
Member

davidism commented May 3, 2023

It looks like you haven't reported this to Python yet. Please create an issue https://github.com/python/cpython/issues/ and link it here so I can see that it's been reported upstream and track that.

@davidism
Copy link
Member

davidism commented May 4, 2023

For now, I will add back in the special case for the location header, but keep the parameter itself deprecated. In 2.4 I will add in an attribute like response.normalize_location, and either get rid of or rename response.autocorrect_location_header (absolute_location?).

@davidism davidism added this to the 2.3.4 milestone May 4, 2023
@davidism davidism closed this as completed May 6, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants