search

apache / cordova-ios

Apache Cordova iOS
https://cordova.apache.org/
Apache License 2.0
2.15k stars 987 forks source link

feat: Add LimitsNavigationsToAppBoundDomains configuration key #1249

Closed gazben closed 1 year ago

gazben commented 1 year ago

Platforms affected

IOS

Motivation and Context

Currently there is no option to set the limitsNavigationsToAppBoundDomains attribute from the config.xml. If you want to use cookie authentication or browser APIs you want to set this to YES.

Official documentation: https://webkit.org/blog/10882/app-bound-domains/

Description

This changeset is copied from this tutorial: https://www.ryseapp.io/blog/engineering/step-by-step-guide-to-setup-and-configure-cordova-for-ios-and-android-to-load-a-hosted-react-app

Testing

Tested on multiple simulator instances and and physical hardware. Without it the cookie authentication for a single domain will not work. After I add this change the authentication works.

Checklist

gazben commented 1 year ago

I don't know how can I write a test for this honestly.

dpogue commented 1 year ago

Thanks for the pull request! You probably want to target this to the master branch though.

I think app-bound domains are definitely something we want to support in cordova-ios, particularly because it will enable easier access to a bunch of APIs, as you mentioned.

However, I think for this to work, it will also need to populate the WKAppBoundDomains array in the application's Info.plist file with a list of domains. The blog post you linked is using config-file directives in config.xml to inject the domains they want into the WKAppBoundDomains list, but it probably makes sense to try to automatically populate that from a combination of the hostname value, access tags, and allow-navigation tags. I believe any communication with domains not listed will be blocked.

codecov-commenter commented 1 year ago

Codecov Report

Merging #1249 (aac88bc) into master (4efd400) will increase coverage by 3.73%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1249      +/-   ##
==========================================
+ Coverage   74.88%   78.62%   +3.73%     
==========================================
  Files          13       15       +2     
  Lines        1724     1773      +49     
==========================================
+ Hits         1291     1394     +103     
+ Misses        433      379      -54

see 28 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

gazben commented 1 year ago

You can only list 10 WKAppBoundDomains in your config file. I think if we automatically merge the hostname, allow-navigation etc... We can hit this limit pretty quickly, and I don't know what should we do if we hot this limit.

You are right navigating to a not set domain will trigger an error:

Setting this flag indicates to WebKit that a WKWebView will only navigate to app-bound domains. Once set, any attempt to navigate away from an app-bound domain will fail with the error: “App-bound domain failure.”

I can create a separete merge request for the master but I think it would be better to just provide this option to users so they don't have to resort to manual patching or forking the project. This flag is an optional thing even for native projects.

bryan-hunter commented 1 year ago

Platforms affected

IOS

Motivation and Context

Currently there is no option to set the limitsNavigationsToAppBoundDomains attribute from the config.xml. If you want to use cookie authentication or browser APIs you want to set this to YES.

Official documentation: https://webkit.org/blog/10882/app-bound-domains/

Description

This changeset is copied from this tutorial: https://www.ryseapp.io/blog/engineering/step-by-step-guide-to-setup-and-configure-cordova-for-ios-and-android-to-load-a-hosted-react-app

Testing

Tested on multiple simulator instances and and physical hardware. Without it the cookie authentication for a single domain will not work. After I add this change the authentication works.

Checklist

  • [x] I've run the tests to see all new and existing tests pass
  • [ ] I added automated test coverage as appropriate for this change
  • [x] Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • [ ] If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • [ ] I've updated the documentation if necessary

Thanks for adding this @gazben ! I'm not sure why I didn't contribute this back here and just put it in the blog post, but you're being a better open source contributor than me :)

gazben commented 1 year ago

@ochakov Can you merge this, or who should I contact?

ochakov commented 1 year ago

@ochakov Can you merge this, or who should I contact?

I don't have merge permissions, I know @breautek can merge and some others. Would be really helpful if they do!

gazben commented 1 year ago

Sorry, reverted.

gazben commented 1 year ago

As an FYI I've been using this change in production for half a year now (~2k users) no problems reported yet. My comment still stands from earlier, that I don't think that the automatic population is feasible because of the heavy size limitation.

bryan-hunter commented 1 year ago

As an FYI I've been using this change in production for half a year now (~2k users) no problems reported yet. My comment still stands from earlier, that I don't think that the automatic population is feasible because of the heavy size limitation.

Same here 👍

dpogue commented 1 year ago

Thanks @gazben for the contribution! 🎉