airbnb / DeepLinkDispatch

A simple, annotation-based library for making deep link handling better on Android
http://nerds.airbnb.com/deeplinkdispatch/
4.37k stars 407 forks source link

Enable placeholders in custom annotation prefixes. #315

Closed rossbacher closed 3 years ago

rossbacher commented 3 years ago

This change enables the usage of placeholders in custom annotation prefixes.

Internally DLD is creating a separate deeplink entry for every deeplink multiplied by the number prefixes.

e.g. if you have 100 places which use your custom WebDeepLink which is defined like this:

@DeepLinkSpec(prefix = { "http://example.com", "https://example.com" })
@Retention(RetentionPolicy.RUNTIME)
public @interface WebDeepLink {  String[] value();}

it will crate 200 entries as there are 2 prefixes per usage. If you have many prefixes e.g. for country domain differences this can add up and create a lot of entires that are very similar.

DLD is capable of handling that many entires but it is still not very efficient.

With this change it will allow you to write something like this:

@DeepLinkSpec(prefix = { "http{scheme}://example.com" })
@Retention(RetentionPolicy.RUNTIME)
public @interface WebDeepLink {  String[] value();}

This will match http and https urls (but also basically any url scheme that start with http and it will only create one entry in the match index.

If you also have country prefixes for the hosts you can write something like this:

@DeepLinkSpec(prefix = { "http{scheme}://{host_prefix}.example.com" })
@Retention(RetentionPolicy.RUNTIME)
public @interface WebDeepLink {  String[] value();}

This will match hosts that are .*example.com without having to list them all and still with just one entry in the match index.

rossbacher commented 3 years ago

@elihart I was thinking about those, but they actually require some bigger changes to the index and I did want to avoid that. I thought about it and I don't think they are required from a security point of view.

I just have questions from the design side - could it potentially be a security issue that this would allow bad schemes like httpz or other? It seems like it would be safer for the design to be more explicit in matching, like prefix = { "[http]|[https]://airbnb.com", almost more like a regex. Or have a separate way to set valid options likeval validSchemes = "http,https" ; prefix = { "{validSchemes}://airbnb.com"`

Actually the supported schemas are listed in the manifest still, so if I only list http and https I would not even get anything else to handle. But even if I did, it does not really matter. It would just mean that we handle more links then we want but the links are not "vetted" e.g. someone can already create a link with http://www.example.com/maliciousContent which would match http://www.example.com/{placeholder} and we need to (right now) be able to handle that. No harm is created even if we would handle httpz://www.example.com/maliciousContent

not sure how much of an issue that really is, but it came to mind.

My other question is if there are plans or possibilities to handle the tld with a placeholder too (which would require an allow list of placeholder values I guess)

We can do that right now with those code. Just define it as http{scheme}://www.example.{tld} e.g. again we already have to handle all those URLs no harm can come from someone inventing a TLD and making us handle this.

Also again for TLDs we already have to filter via the manifest entry anyway.