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

iOS: Use objc2 #87

Merged
merged 12 commits into from
May 4, 2024
Merged

iOS: Use objc2 #87

merged 12 commits into from
May 4, 2024

Conversation

madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Apr 24, 2024

objc2 is a refinement of objc with the following benefits to webbrowser:

  • Less error prone msg_send! macro invocations (I've refactored the rest into a separate function as recommended by the docs).
  • Prevents a leak of the options dictionary.
  • Catches errors when passing an invalid URL to NSURL.
  • Makes it easier to do something in the completion handler in the future.

Builds upon #86 to avoid a merge conflict.

Concrete benefits to `webbrowser`:
- Less error prone `msg_send!` macro invocations (I've refactored the
  rest into a separate function as recommended by the docs).
- Prevents a leak of the `options` dictionary.
- Catches errors when passing an invalid URL to `NSURL`.
- Makes it easier to do something in the completion handler in the
  future.
@amodm
Copy link
Owner

amodm commented Apr 24, 2024

Thanks @madsmtm. I remember the thread between you and the objc maintainer. While I do want to walk away from a 4+ yr stale crate, the following considerations come up:

  1. Like most maintainers, I try to be mindful of supply chain attacks*, and the recent xz issue has only highlighted the need for it. In the absence of being able to audit a codebase as large as yours, I tend to rely on the proxy of relative size of deployment base because that maximises the chances of a vulnerability being caught.
  2. There's no Security policy defined yet for objc2. Irrespective of the outcome of this PR, I'll encourage you to do that. You can look at this crate's SECURITY.md as an example.
  3. MSRV - this is a ✅

The first issue is the main blocker here, and while I do want to be supportive of migration away from objc (given the maintainer's stance on it), I'd like to make sure that I feel comfortable with security for my downstream users first. I don't have any suggestions for you on this, just sharing what's top of mind for me here.

I'm leaving this PR open for now, to allow myself more time to think through this, and for alternative perspectives to be shared.

* Edit: I want to be clear that I'm not insinuating that this is a supply chain attack. I'm just explaining why I've continued to stick with objc till now, despite it being so stale.

@madsmtm
Copy link
Contributor Author

madsmtm commented Apr 30, 2024

Thanks for the thoughtful reply!

Security policy

Thanks for the heads-up, I've added one in madsmtm/objc2@469a36f and enabled the ability to report vulnerabilities using GitHub's advisories.

MSRV

I'll note that it isn't defined by policy yet, so I may still decide to bump it in a minor version. Would be interested in your input on it, preferably in madsmtm/objc2#203.

In the absence of being able to audit a codebase as large as yours

One idea to slightly reduce the review surface would be to not use objc2-foundation, and instead only use objc2 (that's still by itself fairly large, but it can't really be split due to orphan rules and ease-of-use).

Although, objc2-foundation is automatically generated, so should also be fairly easy to review for unintended changes (assuming you also run the code generator yourself, and verify that it matches), and both crates are still under my control, so in the end probably doesn't really matter that much.

supply chain attacks
relative size of deployment base

Totally understandable worry (especially given that I'm opening the PR myself, I can definitely see how this could be negatively interpreted).

In general, I'm looking to replace objc with objc2 in major projects in the ecosystem to increase soundness, stability and performance for macOS/iOS users of Rust. But part of the reason why I opened this PR is also to gain exactly the "relative size of deployment base" that you're talking about; the more downloads/usage objc2 has compared to objc, the easier it is for me to convince other projects to use objc2, and the safer the entire ecosystem gets. And your crate is quite a popular one ;)

That said, I'm totally fine with it (and really quite respect) if you want to hold out on this for a while!

If you think of other ways I can help prove the sincerity and security of the project, please don't hesitate to tell me!

@amodm
Copy link
Owner

amodm commented May 1, 2024

Thanks @madsmtm, for deciding to have a security policy, as well as your inputs on the surface area to assess. I don't have a well defined time frame in mind currently, but I do plan to take a first look at it this weekend.

@amodm
Copy link
Owner

amodm commented May 4, 2024

This is good to go after the tests. Thanks @madsmtm, and hope that you find wider adoption quickly enough.

To document my thought process for later reference:

  1. objc2 is clearly more actively maintained. As an example, in the current case, I was able to build for visionOS using objc2, but not able to do it using objc.
  2. Concerns about security stemming from a lesser deployment base is significantly mitigated in current case because we're using it for ios/tvos/visionos, each of which is very strongly sandboxed. That leaves only build.rs as an attack vector, which is a lot more easily monitored.

@amodm amodm mentioned this pull request May 4, 2024
@amodm amodm merged commit e81d11c into amodm:main May 4, 2024
9 of 10 checks passed
@amodm
Copy link
Owner

amodm commented May 4, 2024

Proceeding with merging this, despite the ios test failure. I'm currently of the opinion that the test failure has something to do with some recent changes to the macos-latest image, or how ios simulator is now run on macos-latest.

I'll figure that out separately from this PR. Release will happen only after this has been figured out.

@madsmtm
Copy link
Contributor Author

madsmtm commented May 4, 2024

Thanks for caring so much about the security of your project, it's been really nice to see and to discuss with you!

Proceeding with merging this, despite the ios test failure. I'm currently of the opinion that the test failure has something to do with some recent changes to the macos-latest image, or how ios simulator is now run on macos-latest.

macos-latest recently changed from being run on x86_64 to ARM64, so that might be part of it?

@madsmtm madsmtm deleted the objc2 branch May 4, 2024 15:02
@amodm
Copy link
Owner

amodm commented May 6, 2024

This is released as v1.0.1

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.

None yet

2 participants