EXIF tags that affect correct viewing are stripped

Thanks, this looks good to me.

The patch keeps the existing behaviour of stripping metadata, but restores the tags that affect correct visual rendering:

-all= -tagsfromfile=@ -Orientation -ColorSpaceTags

This addresses the main issue with images being displayed in the wrong orientation after sanitizing, while also preserving color-space related tags to avoid unwanted color shifts.

I also checked that the change is applied in both sanitization paths:

MetadataSanitizerCommands
MetadataSanitizerRunner

I think this is ready to be marked RTBC.

Minor follow-up only: it may be useful to add/update test coverage later with an image containing an EXIF Orientation tag, but I don’t think that should block this fix.

I’m changing this back from RTBC after reviewing the pipeline results more closely.

The overall approach still looks correct for addressing the orientation/color-profile issue, but the MR currently has failing CI jobs:

phpstan fails because an ignored error pattern is no longer matched in MetadataSanitizerCommands.php
phpcs fails due to a long array declaration line in MetadataSanitizerRunner.php
cspell fails because tagsfromfile is not added to the dictionary

I also still think it would be useful to add either automated test coverage or at least a documented manual test case using an image with an EXIF Orientation tag, to confirm the sanitized image keeps the expected visual orientation.

Setting back to Needs work until the pipeline is green.

I would like to help test as well and get this branch RTBC - @zarabatana are you also ok with adding https://github.com/ddev/ddev-drupal-contrib so its easier to spin the project up for others?

I also think we need some very small test files saved to the repo so that we can validate sanitization are you open to that as well?

Yes, I’m OK with adding ddev-drupal-contrib if it helps make the project easier to spin up and test locally.

I also agree that adding a few very small test fixtures would be useful here, especially an image with an EXIF Orientation tag and, if possible, one with color-profile/color-space metadata. That would make it easier to validate that sanitization still removes unwanted metadata while preserving the tags needed for correct visual rendering.

Given the current CI failures, I think the issue should remain Needs work until the pipeline is green, but these additions would definitely help get the branch to RTBC.

Quick update - Got started really diving deep into the module, ddev up an running, actual file testing in Gitlab is a larger task than I anticipated, so probably going to put that work on its own issue#.

BUT early testing did reveal that for PDF documents we also need to preserve title attributes so our pdfs pass accessibility checks (we use our pdf_services module). So just like images rotation those need to be added back. Hate to scope creep things but should this be something and admin config page should allow, people to choose which ones to allow? Or do we just assume everyone will want titles added?

Thanks for digging into this and testing it more deeply — that’s really helpful.

I agree that preserving the PDF Title can make sense for accessibility checks, but I’d be hesitant to add it unconditionally in this MR. Since this module is specifically for metadata sanitization, document titles may contain sensitive or identifying information in some use cases.

For this issue, I think it’s best to keep the MR focused on the image rendering fix with Orientation and ColorSpaceTags, and open a follow-up issue for PDF accessibility metadata. That follow-up could explore making preserved tags configurable, so sites can choose the right balance between accessibility and stricter metadata removal.

I agree, i'll open up new issues for the other non-image related stuff. I think this one is ready for review.

Thanks @nicholass — yes, small test fixtures sound good to me, especially one image with an EXIF Orientation tag so we can confirm the sanitized image still displays correctly.

Adding ddev-drupal-contrib support is also fine if it helps local testing, but I don’t think it needs to block this fix.

Before moving this back to RTBC, I think we just need to fix the current CI failures:

• phpstan
• phpcs
• cspell

And add either a small test fixture or a clear manual test case.

Thanks for helping move this forward!

Thanks @nicholass for cleaning this up.

The functional fix looks good to me now, and the previous CI issues appear to have been addressed.

As mentioned earlier, I’m OK with keeping the DDEV contrib setup if it helps local testing. I only noticed one small cleanup: .ddev/config.yaml uses an issue-specific project name, metadata-sanitizer-3583882.

If we keep the DDEV files, I think that should be changed to a generic project name, for example metadata-sanitizer, so it does not reference this specific issue.

After that small cleanup, and assuming the latest pipeline is green, this looks ready to move back to RTBC.