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

feat: replace photo library detection with human face detection #267

Closed
wants to merge 2 commits into from

Conversation

ioannisj
Copy link
Contributor

💡 Motivation and Context

Looks like we were very optimistic in our ability to detect if a UIImage originated from user's photo library. The initial thought was to detect _assetName for a UIImage that was not an image asset or a symbol image. Turns out that some system generated images, as well as remotely loaded images may also contain a UUID value in "_assetName". So it turns out that this plan will not work.

I tried experimenting with extracting GPS, TIFF and EXIF metadata from the image in an attempt to detect an image that was captured with geolocation, camera make/model or a dateTimeDigitized meta - rationale was that this would hint for a photo library image. Turns out that this metadata is removed by the OS when selecting a photo from photo library, and otherwise not accessible unless through photo library permissions. So this plan will not work as well.

In the end, I decided to repurpose the old maskPhotoLibraryImages config with maskImagesWithHumanFaces as an attempt to at least protect user's photo PII. Used CIDetector which is designed for real-time detection during video capture. In our case, the detection is only executed once and cached for each CGImage. Considering that we run at 1fps this doesn't seem to have a great impact performance (also opted for CIDetectorAccuracyLow which is even faster here). Added support for SwiftUI as well here.

As a fly-by, I noticed that

  • with maskAllImage we were also masking UIPickerView's background as well (system adds a blurred background in menu popup which we visit when iterating over the view hierarchy). Added a isSystemImageView check for that
  • with maskAllImage we were also masking some animated images (e.g loader) in SwiftUI. Added a isAnimatedImageView check for that

Original slack thread: https://posthog.slack.com/archives/C03PB072FMJ/p1732875958511529

TODO:

  • Update docs
  • Update RN session replay plugin
  • Update RN

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

@ioannisj ioannisj requested a review from a team November 30, 2024 10:59
}()

private static var ph_human_face_detected_key: UInt8 = 0
var ph_human_face_detected: Bool? {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where we mem-cache the face detection so we avoid detecting multiple times per resource

// with maskAllImages options (e.g in this case this was a blurred background view of a UIPickerView)
// Let’s be prudent and add a general check on internal classes starting with _UI.
// We may need to be more explicit in the future
String(describing: view).starts(with: "<_UI")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we should be explicit here for "<_UICutoutShadowView>" but a general test for internal types here seemed like a good idea at the time

// No way of checking if there's actual content in the image or not
config.sessionReplayConfig.maskAllImages || view.isNoCapture()
// sensitive, regardless
if view.isNoCapture() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't refactor view.isNoCapture() yet to keep the scope low

}

// an animated image view (e.g spinner view) is most likely not a sensitive asset
if isAnimatedImageView(view) {
Copy link
Contributor Author

@ioannisj ioannisj Nov 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Came across system loaders in SwiftUI. But could also be gif support through UIImage.animatedImageNamed() constructor

/// Default: true
@objc public var maskPhotoLibraryImages: Bool = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot do this otherwise we will break people's builds on react native and iOS.
If needed we would need a major bump in the sdk version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can leave and mark as deprecated for next major release? (and also turn default to false - won't have an impact but just to be clear)

@marandaneto
Copy link
Member

As a fly-by, I noticed that

with maskAllImage we were also masking UIPickerView's background as well (system adds a blurred background in menu popup which we visit when iterating over the view hierarchy). Added a isSystemImageView check for that
with maskAllImage we were also masking some animated images (e.g loader) in SwiftUI. Added a isAnimatedImageView check for that

Just an idea: Maybe this can be a separated PR so we can land this asap as this one might drag a bit longer?

@marandaneto
Copy link
Member

got it, seems it's the new config under config.sessionReplayConfig.maskPhotoLibraryImages Seems to catch picker view backgrounds as well. On it

IIRC, we added those configs because of the photo user picker, which is solved with the maskAllSandboxedViews option.
maskPhotoLibraryImages is only used if the user has an image and the picture came from that user's photo, right?

The problem is that the maskPhotoLibraryImages config had a side-effect masking what should not have been masked, instead of adding a face detector that is only about the user's face and is not as good as a general "maskPhotoLibraryImages", eg a photo of a document can still be PII but the face detector would fail, seems like an approach for a very specific use case and not worth for now, people could either enable masking all images or expicillity mask that user's face image using the ph-no-capture approach or view modifiers.

IMO we should just deprecate the maskPhotoLibraryImages, change it to false, add a description explaining that this flag has no effect (also in RN btw), and add also in the comments that you can do the ph-no-capture, etc.

WDYT?

@ioannisj
Copy link
Contributor Author

ioannisj commented Dec 2, 2024

IIRC, we added those configs because of the photo user picker, which is solved with the maskAllSandboxedViews option.
maskPhotoLibraryImages is only used if the user has an image and the picture came from that user's photo, right?

Yeah, that's correct

The problem is that the maskPhotoLibraryImages config had a side-effect masking what should not have been masked, instead of adding a face detector that is only about the user's face and is not as good as a general "maskPhotoLibraryImages", eg a photo of a document can still be PII but the face detector would fail, seems like an approach for a very specific use case and not worth for now, people could either enable masking all images or expicillity mask that user's face image using the ph-no-capture approach or view modifiers.

Agreed. The initial thought was to come up with an alternative to the originally intended functionality - thinking about this again I fully agree. It's indeed an niche use case - any images other than the picker that are in-app are controlled fully by the developers so they can manually mask themselves and run any other logic, like face detection, they want. Let’s scrap this.

IMO we should just deprecate the maskPhotoLibraryImages, change it to false, add a description explaining that this flag has no effect (also in RN btw), and add also in the comments that you can do the ph-no-capture, etc.

Agreed

@ioannisj
Copy link
Contributor Author

ioannisj commented Dec 2, 2024

see: #268

@ioannisj ioannisj closed this Dec 2, 2024
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.

2 participants