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

[$2000] Deep links are broken on iOS #15158

Closed
1 of 6 tasks
marcaaron opened this issue Feb 15, 2023 · 59 comments
Closed
1 of 6 tasks

[$2000] Deep links are broken on iOS #15158

marcaaron opened this issue Feb 15, 2023 · 59 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff

Comments

@marcaaron
Copy link
Contributor

marcaaron commented Feb 15, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Install the New Expensify app
  2. Sign in and close app
  3. In a browser, follow a deep link like https://new.expensify.com/settings

Expected Result:

App should open to the settings page

Actual Result:

Safari or other default web browser opens the page

Workaround:

Open the app directly

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: v1.2.71-1
iOS Version: 16.3.1
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:
Issue reported by: @shawnborton
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1676413158858859

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01d64bfcd13e8d5909
  • Upwork Job ID: 1627724629399916544
  • Last Price Increase: 2023-02-27
@marcaaron marcaaron added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Feb 15, 2023
@MelvinBot
Copy link

Triggered auto assignment to @arielgreen (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot melvin-bot bot locked and limited conversation to collaborators Feb 15, 2023
@MelvinBot
Copy link

MelvinBot commented Feb 15, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@mvtglobally
Copy link

@marcaaron we had an old Issue on this which was closed. There was a bigger discussion. I'll try to link it in a bit

@mvtglobally
Copy link

@marcaaron
Copy link
Contributor Author

Thanks @mvtglobally (great memory)!

I'll leave a comment over there. They seem like the same issue so if anything we will keep this one open and leave that one closed.

@melvin-bot melvin-bot bot added the Overdue label Feb 17, 2023
@arielgreen arielgreen added the External Added to denote the issue can be worked on by a contributor label Feb 20, 2023
@melvin-bot melvin-bot bot unlocked this conversation Feb 20, 2023
@melvin-bot melvin-bot bot changed the title Deep links are broken on iOS [$1000] Deep links are broken on iOS Feb 20, 2023
@MelvinBot
Copy link

Job added to Upwork: https://www.upwork.com/jobs/~01d64bfcd13e8d5909

@MelvinBot
Copy link

Current assignee @arielgreen is eligible for the External assigner, not assigning anyone new.

@MelvinBot
Copy link

Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (External)

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 20, 2023
@MelvinBot
Copy link

Triggered auto assignment to @NikkiWines (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@arielgreen
Copy link
Contributor

Added external

@melvin-bot melvin-bot bot removed the Overdue label Feb 20, 2023
@bradjin8
Copy link

Is it still open for new ones?

@allroundexperts
Copy link
Contributor

@NikkiWines @parasharrajat This seems to be a feature request. Did it ever worked on iOS? As far as I can see, DeepLinkWrapper was never implemented for native platforms. It is just done for Desktop.

@allroundexperts
Copy link
Contributor

Ref: #10893

@parasharrajat
Copy link
Member

Ok, @allroundexperts. We are happy to take proposals here.

@bjin9 Yes it is open.

@parasharrajat
Copy link
Member

Next time, please post complete proposals. Also, do explain why

we're restricting the app to open the links on mobile here src/components/DeeplinkWrapper/index.website.js.

@cubuspl42
Copy link
Contributor

cubuspl42 commented Feb 20, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

We're trying to make universal links work between the web app and the mobile app on iOS.

What is the root cause of that problem?

  1. The url event from the Linking API is handled, but only report URLs seem to be supported (ReportUtils.openReportFromDeepLink).
  2. Like @s77rt mentioned, apple-app-site-association have a generic binary file MIME type binary/octet-stream. I haven't verified that it's actually a blocker, though.

What changes do you think we should make in order to solve the problem?

  1. I propose to extend the handling of the url event for the other deep link types, via a new helper.
  2. apple-app-site-association should have a MIME type application/json. Specifics would depend on how the app is hosted in production. If it's hosted on Firebase Hosting (as firebase.json is checked into source control), it's possible to cofigure it there.

@s77rt
Copy link
Contributor

s77rt commented Feb 21, 2023

This may be due to https://new.expensify.com/.well-known/apple-app-site-association not setting the correct content-type header value, it should be application/json.

@marcochavezf
Copy link
Contributor

Oops, the PR to change the content-type for the AASA file was merged, but the deep links are not opening the installed iOS app instead of the browser.

@marcochavezf
Copy link
Contributor

marcochavezf commented Mar 11, 2023

The universal link diagnostics and the AASA validator indicate that everything is fine:

Upload.from.GitHub.for.iOS.MOV

But links to production and staging are still opening the browser

e.g.

https://new.expensify.com/r/3504895439653267

https://staging.new.expensify.com/r/3504895439653267

@s77rt
Copy link
Contributor

s77rt commented Mar 11, 2023

It may be worth to check if this is related to cache, I think apple check those urls once a week but not really sure

@s77rt
Copy link
Contributor

s77rt commented Mar 14, 2023

@marcochavezf Can you please remove the "HOLD" from the title, this is not fixed yet.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Mar 16, 2023
@marcochavezf marcochavezf changed the title [HOLD for payment 2023-03-17] [$2000] Deep links are broken on iOS [$2000] Deep links are broken on iOS Mar 16, 2023
@marcochavezf
Copy link
Contributor

It may be worth to check if this is related to cache, I think apple check those urls once a week but not really sure

Yeah, that was my first thought, but it has already passed a week since the apple-app-site-association file was updated. Probably we'd need to wait for a few days more.

@melvin-bot melvin-bot bot added the Overdue label Mar 20, 2023
@MelvinBot
Copy link

@marcochavezf, @parasharrajat, @arielgreen Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@marcochavezf
Copy link
Contributor

Update here. It seems that we'd need to wait until the other change is deployed to production and test again.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Mar 21, 2023
@arielgreen
Copy link
Contributor

@marcochavezf is this still held?

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Mar 23, 2023
@marcochavezf
Copy link
Contributor

The other files were already updated on production and staging, but the deep links are not still working.

Screenshot 2023-03-27 at 10 26 51

Screenshot 2023-03-27 at 10 26 28

@melvin-bot melvin-bot bot removed the Overdue label Mar 27, 2023
@marcochavezf
Copy link
Contributor

Looking for more ideas of what could be the issue here.

@mczernek
Copy link
Contributor

mczernek commented Mar 28, 2023

I did some research on that and found some interesting things.

It was extremely difficult for me to get any one app to handle universal links. However I discovered that most domains uses different formats of AASA file than we do. Great example (and working one!) is YouTube:

{
    "applinks": {
        "apps": [],
        "details": [
            {
                "appID": "EQHXZ8M8AV.com.google.ios.youtube.dev",
                "paths": [
                    "NOT /yt/*",
					// Whole bunch of NOTs here,
                    "*"
                ]
            },
            // tonns of other appIDs
		]
	}
}

The differences to our file are:

  • instead of components they have paths which is array of strings
  • they don't have webcredentials key within applinks

There are other domains that follow similar format (pretty much every one I checked) but with webcredentials in it.

Interestingly youtube.com is the only one that Diagnostics in Settings finds correct (apart from us, but unlike us YouTube actually works).

@mczernek
Copy link
Contributor

After some more investigation I have one more thought to be tested.

In fact, iOS does not query our associated file directly. Instead, it uses apple-cdn by getting from url such like https://app-site-association.cdn-apple.com/a/v1/new.expensify.com

For some reason it does return 404 for our domain, while it works for pretty much every other I tested. My guess would be that there is something wrong with our response when serving associated-domains file. In fact, there are quite some headers and two cookies being returned. Can we try simplify that as much as possible and leave only relevant headers and no cookies. For reference, this is set of headers returned by branch for one of apps:

content-type: application/json; charset=utf-8
content-length: 1362
vary: Accept-Encoding
server: openresty
date: Tue, 28 Mar 2023 14:34:49 GMT
strict-transport-security: max-age=31536000; includeSubDomains
x-cache: Miss from cloudfront
via: 1.1 2e4b77c76f89825e36f12179cf1b33ea.cloudfront.net (CloudFront)
x-amz-cf-pop: WAW51-P1
x-amz-cf-id: 8lgANq-rSQNzbOs4GuRIniAxHrXVjNrQCw9dVy48ZLPKPkdzc6OM-g==

@parasharrajat
Copy link
Member

I think it is definitely that 404 error issue.

@marcochavezf
Copy link
Contributor

Thanks for the insights @mczernek! We're discussing with the infra team the headers issue.

@marcochavezf
Copy link
Contributor

After more investigation seems that our firewall is blocking Apple's crawler from pulling the AASA file from our servers. Infra already updated the rule. So I think we should only need to wait a few days to corroborate if the apple-cdn is updated.

@marcochavezf
Copy link
Contributor

It's working now!

RPReplay_Final1680179383.MP4

@marcochavezf
Copy link
Contributor

For future reference, if someone has a similar problem in the future, the problem was the firewall.

We were getting this error from the headers of the apple cdn Apple-Failure-Reason: SWCERR00101 Bad HTTP Response: 403 Forbidden:

Screenshot 2023-03-29 at 15 04 05

And someone else that had the same error, posted in the apple developer forum that the problem was their firewall configuration: https://developer.apple.com/forums/thread/675324?answerId=668081022#668081022

@marcochavezf
Copy link
Contributor

I will close this one since it's working on staging (app from TestFlight) and production. Thank you all so much for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff
Projects
None yet
Development

No branches or pull requests