Skip to content

Harden SABR/download handling and fix settings dismissal - #18

Open
evanonym0us wants to merge 2 commits into
itzzace:mainfrom
evanonym0us:harden-runtime-security
Open

Harden SABR/download handling and fix settings dismissal#18
evanonym0us wants to merge 2 commits into
itzzace:mainfrom
evanonym0us:harden-runtime-security

Conversation

@evanonym0us

Copy link
Copy Markdown

Summary

This PR hardens the network, diagnostic logging, and settings-backup paths used by YTKACE, and fixes the settings checkmark terminating YouTube after preferences are saved.

SABR/download hardening

  • Validate server-provided SABR URLs before making requests:
    • require HTTPS
    • reject embedded credentials
    • require the default HTTPS port
    • allow only YouTube/Google video delivery hosts
  • Remove raw protobuf request/response dumps from the temporary directory.
  • Replace raw directive and captured-header diagnostic output with type/count/size-only logging.
  • Bound the per-video SABR request/config caches to prevent indefinite in-process growth.
  • Prefer Application Support/Caches over Documents for the persistent download log so it is not normally exposed through iOS File Sharing. The in-app copy/share controls still work.

Backup/restore hardening

  • Apply per-entry, per-file, and total expanded-size limits before extracting a settings backup.
  • Require sufficient free disk space before extraction.
  • Restore only valid YTKACE.Preference.* property-list values instead of importing arbitrary defaults keys.

Settings crash fix

YTKACERootOptionsController called exit(0) shortly after synchronizing preferences, making the checkmark appear to crash the app even though the settings persisted. It now:

  • synchronizes preferences
  • posts the exported YTKACEPreferencesDidChangeNotification
  • posts YTKACETabConfigDidChange
  • dismisses the settings controller normally

Validation

  • python3 -m unittest discover -s Tests -v — 9 tests passed
  • C++ watchdog implementation and test suite compile and pass
  • shell syntax checks, Python compilation, provenance audit, and git diff --check pass
  • GitHub Actions built rootless and roothide packages plus iOS 16 and iOS 17 IPAs successfully: https://github.com/evanonym0us/ytkace/actions/runs/33328250173
  • Both generated IPA archives passed integrity checks; packaged tweak binaries contain the URL guard and redacted logging while the removed protobuf dump identifiers are absent
  • On-device testing confirmed:
    • app launch and normal video playback
    • SponsorBlock operation
    • video download, mux, and downloaded-video playback
    • the settings checkmark persists changes, closes the settings screen, and leaves YouTube running

Follow-up recommendation (outside this PR)

Consider moving release/build instructions toward a user-supplied base YouTube IPA flow instead of distributing or automatically acquiring the base application. That would reduce the project's avoidable distribution/legal exposure while keeping the tweak build itself reproducible. This is intentionally not implemented here so the security and crash fixes remain focused and reviewable.

@epic0002

Copy link
Copy Markdown
Contributor

90% of this is slop

@evanonym0us

Copy link
Copy Markdown
Author

90% of this is slop

It was AI-assisted. The changes were still reviewed, built successfully, tested in CI, and validated on-device. Playback, SponsorBlock, downloading/muxing, and the settings fix all work as expected.

If you think something is unnecessary or technically wrong, point to it and I’m happy to address it. “90% of this is slop” is not actionable feedback.

Considering some of this “slop” replaced code that literally called exit(0) when saving settings, I’d say it’s at least an improvement :)

@epic0002

Copy link
Copy Markdown
Contributor

Hello codex if you where human you would know settings are auto applied they don’t need “saved” the checkmark closed the app for you so you can use settings that require a restart

@epic0002

Copy link
Copy Markdown
Contributor

You also didn’t change anything with sponsorblock so I’m not sure why you mentioned that same with playback

@evanonym0us

Copy link
Copy Markdown
Author

Hello codex if you where human you would know settings are auto applied they don’t need “saved” the checkmark closed the app for you so you can use settings that require a restart

For clarity, I am the person replying here, and I am human. I used AI assistance on the code and review, which I already disclosed.

You are correct that my wording about “saving” settings was imprecise. The settings auto-apply. But if the checkmark was intentionally supposed to terminate YouTube so restart-required settings take effect, that is still poor UI. A checkmark means Done or Confirm. There is zero indication that it means “kill the app and relaunch it.” Also, exit(0) is not a restart. It terminates the process. If that functionality is intentional, label it “Restart YouTube” or prompt when a restart is actually required. Expecting users to infer process termination from a checkmark is poor UI and, quite frankly, silly.

SponsorBlock and playback were mentioned under Validation, not as things this PR changed. I tested them specifically to make sure the rebuilt tweak had not broken unrelated functionality. “Tested and still works” is not the same as “this PR modified it.”

And yes, AI helped. I understand why developers are skeptical of AI-generated code. A lot of that skepticism is justified, but not as a substitute for actually reviewing the code. These are straightforward changes, they built successfully, passed tests, and were validated on-device.

You made mistakes, and a computer caught some of them. I can understand why that might be frustrating, but it does not make the fixes wrong. If something is technically wrong, point to it and I’ll address it.

@epic0002

Copy link
Copy Markdown
Contributor

Theirs no skepticism idc if you used ai but it’s annoying your using ai to reply to me and you didn’t really too anything so genius what’s your suggestion for the ui have a checkmark that does nothing cause that’s what yours does?

@evanonym0us

Copy link
Copy Markdown
Author

Theirs no skepticism idc if you used ai but it’s annoying your using ai to reply to me and you didn’t really too anything so genius what’s your suggestion for the ui have a checkmark that does nothing cause that’s what yours does?

I think we're talking past each other at this point. The tone here is getting unnecessarily hostile, and I don't think that's productive.

It seems like we have different opinions on how AI should or shouldn't be used. That's fine, but it isn't something I want to argue about, and I don't think it's relevant to whether the changes in the PR are technically correct.

On the checkmark, you're right about one thing: I misunderstood its original purpose. I saw a checkmark calling exit(0) with no indication in the UI that it was supposed to terminate the app for restart-required settings, so I interpreted that behavior as a crash. If its intended purpose is specifically to make the user relaunch YouTube for settings that require it, then simply dismissing the menu does change that behavior.

I still don't think a checkmark silently terminating the app makes that purpose clear. A more obvious approach would be something like a "Restart YouTube" action, or a prompt when a setting that actually requires a restart has been changed. But that's a UI/design decision for your project.

That also wasn't the original purpose of this PR. The main work was fixing security and robustness issues in the SABR/download and backup/restore paths: validating server-provided URLs before making requests, removing raw protobuf and header data from diagnostics, bounding the SABR caches, and adding limits and validation around backup extraction and restore. The settings issue was something separate I noticed while testing, so I fixed that too.

Those changes were built and tested, including on-device testing of downloading and playback of downloaded videos. So I don't think saying I "didn't really do anything" is a fair characterization of the PR.

I'd rather keep any further discussion focused on specific technical issues with the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants