Incorrect percent encoding under Sonoma #4861

low-batt · 2024-03-31T21:25:22Z

System and IINA version:

macOS 14.4
IINA 1.3.4

Expected behavior:
IINA correctly percent encodes URLs.

Actual behavior:
When running under macOS Sonoma IINA will perform percent encoding twice resulting in malformed URLs. @xfoxfu provided two examples:

Opening this URL in Firefox:

iina://weblink?url=https://example.com/%5Bfoo%20bar.mkv

IINA changes %5Bfoo%20bar.mkv to be %5Bfoo%2520bar.mkv:

13:54:42.969 [iina][d] Parsing URL iina://weblink?url=https://example.com/%5Bfoo%20bar.mkv
13:54:42.969 [player0][d] Open URL: https://example.com/%5Bfoo%2520bar.mkv
13:54:42.969 [player0][d] Opening https://example.com/%5Bfoo%2520bar.mkv in main window

Opening this URL in Firefox:

iina://weblink?url=https://example.com/%25foo%20bar.mkv

IINA changes %25foo%20bar.mkv to be %25foo%2520bar.mkv:

13:51:00.896 [iina][d] Parsing URL iina://weblink?url=https://example.com/%25foo%20bar.mkv
13:51:00.897 [player0][d] Open URL: https://example.com/%25foo%2520bar.mkv
13:51:00.897 [player0][d] Opening https://example.com/%25foo%2520bar.mkv in main window

This is a change from the behavior under macOS Ventura.

Steps to reproduce:

Enable logging in IINA
Open one of the URLs given above in Firefox
Look for the message shown above in IINA's log file
MPV does not have this problem.

This is an issue with IINA code. I have not checked if mpv also has this problem.

How often does this happen?
Every time.

The text was updated successfully, but these errors were encountered:

low-batt · 2024-03-31T21:34:18Z

Analysis

@xfoxfu Traced the change in behavior to an intentional change by Apple. From the Xcode 15 Release Notes:

Fixed: For apps linked on or after iOS 17 and aligned OS versions, URL parsing has updated from the obsolete RFC 1738/1808 parsing to the same RFC 3986 parsing as URLComponents. This unifies the parsing behaviors of the URL and URLComponents APIs. Now, URL automatically percent- or IDNA-encodes invalid characters to help create a valid URL.

To check if a urlString is strictly valid according to the RFC, use the new URL(string: urlString, encodingInvalidCharacters: false) initializer. This init leaves all characters as they are and will return nil if urlString is explicitly invalid. (93368104)

Many thanks to @xfoxfu for identifying the cause of the change in behavior.

Opening this URL in Firefox under macOS Ventura:

iina://weblink?url=https://example.com/%5Bfoo%20bar.mkv

IINA correctly forms the URL:

14:26:32.836 [iina][d] Parsing URL iina://weblink?url=https://example.com/%5Bfoo%20bar.mkv
14:26:32.837 [player0][d] Open URL: https://example.com/%5Bfoo%20bar.mkv
14:26:32.837 [player0][d] Opening https://example.com/%5Bfoo%20bar.mkv in main window

That means this problem is a regression and is therefore a high priority to fix.

Unfortunately…

Opening this URL in Firefox under macOS Ventura:

iina://weblink?url=https://example.com/%25foo%20bar.mkv

FAILS reporting the URL can not be percent encoded:

14:25:51.188 [iina][d] Parsing URL iina://weblink?url=https://example.com/%25foo%20bar.mkv
14:25:51.188 [player0][e] Cannot add percent encoding for https://example.com/%foo bar.mkv

There is a second, already existing, problem. Adding additional logging:

      Logger.log("Percent encoding \(str)")
      guard let pstr = str.addingPercentEncoding(withAllowedCharacters: .urlAllowed) else {
        Logger.log("Cannot add percent encoding for \(str)", level: .error, subsystem: subsystem)
        return
      }
      Logger.log("Percent encoded \(pstr)")
      guard let url = URL(string: pstr) else {
        Logger.log("Cannot parse url for \(pstr)", level: .error, subsystem: subsystem)
        return
      }

Shows the Cannot add percent encoding error is bogus:

14:29:51.383 [iina][d] Parsing URL iina://weblink?url=https://example.com/%25foo%20bar.mkv 14:30:03.043 [iina][d] Percent encoding https://example.com/%foo bar.mkv 14:30:03.048 [iina][d] Percent encoded https://example.com/%foo%20bar.mkv 14:30:03.048 [player0][e] Cannot parse url for https://example.com/%foo%20bar.mkv

The addingPercentEncoding method completed successfully. It was the URL initializer that failed.

Why did IINA not report this same error running under Sonoma?

Looking at the log message from the test under Sonoma shows %foo%20bar.mkv was changed to %5Bfoo%2520bar.mkv:

13:54:42.969 [player0][d] Opening https://example.com/%5Bfoo%2520bar.mkv in main window

The URL initializer did not fail because it applied percent encoding to the % characters. Why did that not happen under Ventura? Looking at the call to addingPercentEncoding:

guard let pstr = str.addingPercentEncoding(withAllowedCharacters: .urlAllowed), let url = URL(string: pstr) else {

It uses urlAllowed which does not require % to be encoded:

extension CharacterSet {
  static let urlAllowed: CharacterSet = {
    var set = CharacterSet.urlHostAllowed
      .union(.urlUserAllowed)
      .union(.urlPasswordAllowed)
      .union(.urlPathAllowed)
      .union(.urlQueryAllowed)
      .union(.urlFragmentAllowed)
    set.insert(charactersIn: "%")
    return set
  }()
}

Percent was added to the set a long time ago in commit 84927d7.

The reason why CharacterSet provides the the different sets is that each URL component has different rules regarding what is allowed. It is concerning that IINA is trying to apply percent encoding to a full string. Posts on the net recommend using URLComponents which has always handled percent encoding.

Fixing

Currently PR #4860 removes the call to addingPercentEncoding and directly passes the string to the URL initializer. Even when running under macOS Sonoma this might fail. This test program demonstrates the problem (.txt added to make GitHub happy):

URLTest.tar.gz.txt

When built with Xcode 15 the proposed fix works:

Proposed fix:
https://example.com/%5Bfoo%20bar.mkv

But when built with Xcode 13 the new URL initializer behavior is not activated and the fix fails:

Proposed fix:
Cannot parse url for https://example.com/[foo bar.mkv

Full test output:

Xcode 15, macOS Sonoma

Built using Xcode 15.3 and macOS SDK 14.4
Running under macOS Version 14.4 (Build 23E214)

----------------------------------------
String to open:
https://example.com/[foo bar.mkv

Current code:
Percent encoded https://example.com/[foo%20bar.mkv
https://example.com/%5Bfoo%2520bar.mkv

Proposed fix:
https://example.com/%5Bfoo%20bar.mkv

Conditional fix:
Compiled with Xcode 15+
https://example.com/%5Bfoo%20bar.mkv

----------------------------------------
String to open:
https://example.com/%foo bar.mkv

Current code:
Percent encoded https://example.com/%foo%20bar.mkv
https://example.com/%25foo%2520bar.mkv

Proposed fix:
https://example.com/%25foo%20bar.mkv

Conditional fix:
Compiled with Xcode 15+
https://example.com/%25foo%20bar.mkv

Xcode 13, macOS Sonoma

Built using Xcode 13.1 and macOS SDK 12.0
Running under macOS Version 14.4 (Build 23E214)

----------------------------------------
String to open:
https://example.com/[foo bar.mkv

Current code:
Percent encoded https://example.com/[foo%20bar.mkv
https://example.com/%5Bfoo%20bar.mkv

Proposed fix:
Cannot parse url for https://example.com/[foo bar.mkv

Conditional fix:
Percent encoded https://example.com/[foo%20bar.mkv
https://example.com/%5Bfoo%20bar.mkv

----------------------------------------
String to open:
https://example.com/%foo bar.mkv

Current code:
Percent encoded https://example.com/%foo%20bar.mkv
Cannot create URL from https://example.com/%foo%20bar.mkv

Proposed fix:
Cannot parse url for https://example.com/%foo bar.mkv

Conditional fix:
Percent encoded https://example.com/%foo%20bar.mkv
Cannot parse url for https://example.com/%foo%20bar.mkv

The fix has to take into account the version of Xcode IINA was built with. Although the IINA README says:

Open iina.xcodeproj in the latest public version of Xcode. IINA may not build if you use any other version.

IINA has plans to try and create one more release that supports macOS 10.11+. That requires building with Xcode 13. So it is currently undesirable to break compatibility with older versions of Xcode.

Searching shows there are other places in the code where addingPercentEncodingis called before passing a string to the URL initializer. Additional fixes may be required.

The next release of IINA is supposed to be a stabilizing release with a focus on fixing regressions. As the primary problem in this issue is a regression, that needs to be fixed. I'm thinking we should hold off fixing the other problem with failing to percent encode % until after that release as fixing that entails some risk.

xfoxfu · 2024-04-01T02:48:15Z

Thanks for your detailed test. I understand the next version is a stabilizing release, so the current pull request could be aligned with the current suggestion to avoid introducing new regressions.

As a further followup, I am wondering why the URL needs to be precent-encoded, as it is observed that mpv supports URL with special characters.

mpv 'https://example.com/[foobar] lorem ipsum.mkv'

Therefore, I would like to know if it is OK to assume that precent-encoding the URL is for making URL.init(string:) happy as it would reject URL with preserved characters before URL.init(string:encodingInvalidCharacters:) was introduced.

If so, I think it could be better to make precent-encoding as a fallback for URL.init(string:), which looks like

func parseUrl(urlStr: String) -> URL? {
    var urlComponents = URLComponents(string: urlStr)
    if urlComponents == nil {
        guard let pUrlStr = urlStr.addingPercentEncoding(withAllowedCharacters: .urlPathAllowed) else { return nil }
        urlComponents = URLComponents(string: pUrlStr)
    }
    return urlComponents?.url
}

and this logic could be shared between opening URL (1) from URL scheme and (2) from UI, which also addresses inconsistent behavior between these now on macOS Ventura. I have setup a GitHub repo with Actions to check the behavior between different macOS and XCode versions: https://github.com/xfoxfu/test-iina-swift-url.

And the following test cases should be covere 8000 d:

URL	original filename	URL Scheme
https://example.com/%5Bfoo%20bar.mkv	`[foo bar.mkv`	`iina://weblink?url=https://example.com/%5Bfoo%20bar.mkv`
https://example.com/%25%20.mkv	`% .mkv`	`iina://weblink?url=https://example.com/%2525%20.mkv`
https://example.com/%2520.mkv	`%20.mkv`	`iina://weblink?url=https://example.com/%252520.mkv`

I am not exactly sure about the encoding-decoding part, and some more time is required reading the RFC 3986 to provide precise test cases. I think this should be mostly reflecting the URL handling logic of mpv and browsers.

low-batt · 2024-04-01T21:07:06Z

I've created issue #4862 for the failure to handle %25 correctly. We should continue the discussion in that issue.

* fix: avoid double percent-encode for url scheme Remove manual percent-encoding for URL opened in URL scheme. This will make URLs with special characters no longer be encoded twice and thus openable. See: https://developer.apple.com/documentation/foundation/url/3126806-init * fix: make URL parsing backward compatible with older macos Co-authored-by: low-batt <86170219+low-batt@users.noreply.github.com>

low-batt · 2024-04-01T21:37:15Z

The fix for this issue has been merged into the IINA develop branch. GitHub automatically closed the linked issue in reaction to the merge. I am reopening this issue until the fix is available in an official release of IINA so that users intending to report this problem can easily locate this existing report. Once the fix for this issue has been included in an official release this issue will be closed.

low-batt · 2024-06-09T02:40:41Z

This was fixed in IINA 1.3.5.

low-batt added priority: high feature: playback bug: regression macOS 14 Sonoma labels Mar 31, 2024

low-batt linked a pull request Mar 31, 2024 that will close this issue

fix: avoid double percent-encode for url scheme #4860

Merged

2 tasks

low-batt mentioned this issue Mar 31, 2024

fix: avoid double percent-encode for url scheme #4860

Merged

2 tasks

low-batt mentioned this issue Apr 1, 2024

Incorrect handling of % in URL under Ventura #4862

Open

1 task

low-batt closed this as completed in #4860 Apr 1, 2024

low-batt reopened this Apr 1, 2024

low-batt added the progress: fixed/implemented label Apr 1, 2024

low-batt closed this as completed Jun 9, 2024

koitococo mentioned this issue Oct 10, 2024

Incorrect percent encoding under Sequioa #5205

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incorrect percent encoding under Sonoma #4861

Incorrect percent encoding under Sonoma #4861

Incorrect percent encoding under Sonoma #4861

Incorrect percent encoding under Sonoma #4861

Comments

Analysis

Fixing