Pasting a pasteboard that contains JPEG data into a web view results in a TIFF representation of the image getting pasted. This is undesirable, but it matches the historic behavior of NSTextView. As of macOS Mojave, NSTextView checks for NSImage objects on the pasteboard, and given an NSImage, it first tries to get the image data from an existing EPS, PICT, PDF or bitmap representation (of whichever format), before falling back to the TIFF representation. WebKit should replicate that behavior so that things don’t get unnecessarily transcoded (the JPEG to TIFF example is especially bad!).
<rdar://problem/44879244>
Created attachment 449772 [details] For EWS
Comment on attachment 449772 [details] For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=449772&action=review > Source/WebCore/ChangeLog:20 > + so, then we convert TIFF data from the pasteboard to JPEG or PNG, respectively. This sounds like reencoding, which at least in the JPEG case can degrade quality, and that doesn’t sound like what I said AppKit did.
(In reply to mitz from comment #3) > Comment on attachment 449772 [details] > For EWS > > View in context: > https://bugs.webkit.org/attachment.cgi?id=449772&action=review > > > Source/WebCore/ChangeLog:20 > > + so, then we convert TIFF data from the pasteboard to JPEG or PNG, respectively. > > This sounds like reencoding, which at least in the JPEG case can degrade > quality, and that doesn’t sound like what I said AppKit did. Indeed, this is still transcoding image data; note that we can't expose raw PNG and JPEG data on paste, so we need some kind of strategy to avoid leaking privacy-sensitive metadata on pasted images. I'm still exploring some strategies to achieve this without transcoding; we might be able to just strip out (most) metadata properties without having to reencode to the original format...
(In reply to Wenson Hsieh from comment #4) > (In reply to mitz from comment #3) > > Comment on attachment 449772 [details] > > For EWS > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=449772&action=review > > > > > Source/WebCore/ChangeLog:20 > > > + so, then we convert TIFF data from the pasteboard to JPEG or PNG, respectively. > > > > This sounds like reencoding, which at least in the JPEG case can degrade > > quality, and that doesn’t sound like what I said AppKit did. > > Indeed, this is still transcoding image data; note that we can't expose raw > PNG and JPEG data on paste, so we need some kind of strategy to avoid > leaking privacy-sensitive metadata on pasted images. > > I'm still exploring some strategies to achieve this without transcoding; we > might be able to just strip out (most) metadata properties without having to > reencode to the original format... Another strategy is to skip the transcoding step just for Mail, via SPI.
Comment on attachment 449772 [details] For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=449772&action=review >>>> Source/WebCore/ChangeLog:20 >>>> + so, then we convert TIFF data from the pasteboard to JPEG or PNG, respectively. >>> >>> This sounds like reencoding, which at least in the JPEG case can degrade quality, and that doesn’t sound like what I said AppKit did. >> >> Indeed, this is still transcoding image data; note that we can't expose raw PNG and JPEG data on paste, so we need some kind of strategy to avoid leaking privacy-sensitive metadata on pasted images. >> >> I'm still exploring some strategies to achieve this without transcoding; we might be able to just strip out (most) metadata properties without having to reencode to the original format... > > Another strategy is to skip the transcoding step just for Mail, via SPI. Skipping sanitization process for app usage seems sensible especially if we restrict the origin. So if origin of the document is local & app opts-in then don't hide content from the clipboard.
Specifically, any app that is browser like or any document whose top-level document origin isn't local will continue to use the full sanitization path.
Out of interest, how is this dealt with for <input type=file>?
(In reply to mitz from comment #8) > Out of interest, how is this dealt with for <input type=file>? When using a file input, it's assumed that the user knows they are uploading the file as-is, with all metadata. For pasted files, we don't (or rather, shouldn't) make the same assumption — we only allow the web page to read data that is "visibly exposed", from the user's point of view. A scenario where this property is especially important to uphold is when copying and pasting markup, in which case comments and other non-rendered content in the fragment can hold PII (as seen when copying from some native apps, such as Microsoft Word).
(In reply to Wenson Hsieh from comment #9) > (In reply to mitz from comment #8) > > Out of interest, how is this dealt with for <input type=file>? > > When using a file input, it's assumed that the user knows they are uploading > the file as-is, with all metadata. > > For pasted files, we don't (or rather, shouldn't) make the same assumption — > we only allow the web page to read data that is "visibly exposed", from the > user's point of view. > > A scenario where this property is especially important to uphold is when > copying and pasting markup, in which case comments and other non-rendered > content in the fragment can hold PII (as seen when copying from some native > apps, such as Microsoft Word). I see. It’s worth keeping in mind that TIFF data may contain metadata as well, so it may need to be processed as well. I think there must be a better way to do this than transcoding, though (notably, sharing photos from iOS uses something to strip some or all metadata).
(In reply to mitz from comment #10) > (In reply to Wenson Hsieh from comment #9) > > (In reply to mitz from comment #8) > > > Out of interest, how is this dealt with for <input type=file>? > > > > When using a file input, it's assumed that the user knows they are uploading > > the file as-is, with all metadata. > > > > For pasted files, we don't (or rather, shouldn't) make the same assumption — > > we only allow the web page to read data that is "visibly exposed", from the > > user's point of view. > > > > A scenario where this property is especially important to uphold is when > > copying and pasting markup, in which case comments and other non-rendered > > content in the fragment can hold PII (as seen when copying from some native > > apps, such as Microsoft Word). > > I see. It’s worth keeping in mind that TIFF data may contain metadata as > well, so it may need to be processed as well. I think there must be a better > way to do this than transcoding, though (notably, sharing photos from iOS > uses something to strip some or all metadata). Yes! I have an approach that uses CG API to copy out the image data while stripping away all metadata fields except for image orientation, and it seems to be working reasonably well — I'll post a new patch here soon.
Created attachment 449824 [details] For EWS
Comment on attachment 449824 [details] For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=449824&action=review > Source/WebCore/platform/mac/PasteboardMac.mm:438 > +static RefPtr<SharedBuffer> imageDataAfterRemovingMetadata(Ref<SharedBuffer>&& buffer) It seems a bit unfortunate that this very-generically-named function will break all animated images (GIF and APNG). I think your changelog suggests they should never get here; maybe an assertion or type check or something at the top? Or add all of the animation properties to your allowlist? > Source/WebCore/platform/mac/PasteboardMac.mm:455 > + if (![name isEqualToString:(__bridge NSString *)kCGImagePropertyTIFFDictionary]) { Not sure I follow what's going on here; you're only preserving image orientation in the TIFF properties? What about the other formats?
Comment on attachment 449824 [details] For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=449824&action=review Thanks for taking a look! >> Source/WebCore/platform/mac/PasteboardMac.mm:438 >> +static RefPtr<SharedBuffer> imageDataAfterRemovingMetadata(Ref<SharedBuffer>&& buffer) > > It seems a bit unfortunate that this very-generically-named function will break all animated images (GIF and APNG). I think your changelog suggests they should never get here; maybe an assertion or type check or something at the top? Or add all of the animation properties to your allowlist? Yes, that's right — we'll only get here for TIFF, PNG and JPEG images. In the future, if we add support for reading image data in image/gif format or support for animated PNG, we'll need to adjust this logic to preserve animation-related properties as well. As we discussed, I'll adjust this to only add the first frame of the image, since that's the behavior we currently end up with if we have animated PNG, and leave a FIXME to track making GIF/APNG readable in this codepath. >> Source/WebCore/platform/mac/PasteboardMac.mm:455 >> + if (![name isEqualToString:(__bridge NSString *)kCGImagePropertyTIFFDictionary]) { > > Not sure I follow what's going on here; you're only preserving image orientation in the TIFF properties? What about the other formats? So from local testing, it seems like image orientation is a subway of the tiff image property dictionary (i.e. the dictionary labeled {TIFF} in the image properties). All images appear to have this {TIFF} dictionary…at least, from what I could tell. I'll do more testing and double check this.
So after taking a closer look — it seems that even in the current implementation where we're transcoding from PNG/JPG => TIFF, AppKit *still* preserves all metadata properties on the image, so this extra TIFF transcoding step wasn't giving us any sanitization benefit to begin with — the only thing it does is change the image format and inflate image size 🤦🏻♂️. I think we should address this potential PII leak in a followup, but at least we know that simply reading the JPEG/PNG data isn't going to introduce any new privacy issues than we have today. I'll move my WIP on `imageDataAfterRemovingMetadata()` over to <https://webkit.org/b/235534>, and just do the "prioritize JPG/PNG data" part in this bug.
Created attachment 449845 [details] Patch
Comment on attachment 449845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449845&action=review > Source/WebCore/platform/mac/PasteboardMac.mm:442 > + Vector<PasteboardItemInfo> relevantItems; relevant items sounds very generic vague. How about itemsForIndexByFidelity? Alternatively, it might be better to create a lambda here that each case below uses: e.g. auto platformTypesFromItems = [](auto& items) { HashSet<String> types; for (auto& item : types) { for (auto& type : item.platformTypesByFidelity) nonTranscodedTypes.add(type); } return types; } if (itemIndex) { if (auto itemInfo = strategy.informationForItemAtIndex(*itemIndex, m_pasteboardName, m_changeCount, context())) { types = itemInfo->platformTypesByFidelity; nonTranscodedTypes = platformTypesFromItems(*itemInfo); } } else { strategy.getTypes(types, m_pasteboardName, context()); if (auto allItems = strategy.allPasteboardItemInfo(m_pasteboardName, m_changeCount, context())) nonTranscodedTypes = platformTypesFromItems(*itemInfo); } > Source/WebCore/platform/mac/PasteboardMac.mm:537 > + { String(legacyTIFFPasteboardType()), "image/tiff"_s }, Doesn't this prefer TIFF and PDF over PNG/JPEG if both are written into the pasteboard? Perhaps that warrants an explanation in the change log since this patch is about preferring PNG/JPEG. > Source/WebCore/platform/mac/PasteboardMac.mm:571 > + // If the original data could not be read directly, fall back by trying to transcode to any of the above types. This comment repeats what the code says. Can we instead explain when this would happen? Why may we not be able to read the original data?
Comment on attachment 449845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449845&action=review Thanks for the review! >> Source/WebCore/platform/mac/PasteboardMac.mm:442 >> + Vector<PasteboardItemInfo> relevantItems; > > relevant items sounds very generic vague. > How about itemsForIndexByFidelity? > Alternatively, it might be better to create a lambda here > that each case below uses: > > e.g. > auto platformTypesFromItems = [](auto& items) { > HashSet<String> types; > for (auto& item : types) { > for (auto& type : item.platformTypesByFidelity) > nonTranscodedTypes.add(type); > } > return types; > } > > if (itemIndex) { > if (auto itemInfo = strategy.informationForItemAtIndex(*itemIndex, m_pasteboardName, m_changeCount, context())) { > types = itemInfo->platformTypesByFidelity; > nonTranscodedTypes = platformTypesFromItems(*itemInfo); > } > } else { > strategy.getTypes(types, m_pasteboardName, context()); > if (auto allItems = strategy.allPasteboardItemInfo(m_pasteboardName, m_changeCount, context())) > nonTranscodedTypes = platformTypesFromItems(*itemInfo); > } Sounds good! Refactored this to pull the logic out into a lambda. >> Source/WebCore/platform/mac/PasteboardMac.mm:537 >> + { String(legacyTIFFPasteboardType()), "image/tiff"_s }, > > Doesn't this prefer TIFF and PDF over PNG/JPEG if both are written into the pasteboard? > Perhaps that warrants an explanation in the change log since this patch is about preferring PNG/JPEG. Ah, so this patch is more about avoiding unnecessary transcoding in the case where PNG/JPEG data is supplied to the pasteboard (before this patch, we end up forcing AppKit to transcode to TIFF anyways). I think that if TIFF is explicitly written to the pasteboard (such that reading it does not require transcoding from another format), then it's probably fine to continue choosing TIFF (even if some other formats like PNG/JPEG are also written to the pasteboard). I'll clarify this in the ChangeLog. >> Source/WebCore/platform/mac/PasteboardMac.mm:571 >> + // If the original data could not be read directly, fall back by trying to transcode to any of the above types. > > This comment repeats what the code says. > Can we instead explain when this would happen? > Why may we not be able to read the original data? So one scenario where we would end up here is when the pasteboard contains a format that isn't one of (TIFF, PDF, PNG, JPEG), so in order to still handle the paste we fall back by transcoding to one of these formats. A more concrete example is when the pasteboard only contains GIF data; in this case it matches none of the 4 types above, so we'll handle it by transcoding to TIFF instead. Instead of relying on comments here, I renamed `remainingImageTypesToRead` to `transcodedImageTypesToRead`, which I think better describes this behavior (without needing separate comments).
Created attachment 449864 [details] Patch for landing
Committed r288477 (246360@main): <https://commits.webkit.org/246360@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 449864 [details].