-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add magic number checks to support image files without an extension #1868
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
Open
Shakes4cc
wants to merge
2
commits into
QL-Win:master
Choose a base branch
from
Shakes4cc:support-images-without-ext
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+95
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Implements magic number checks to detect images in files without extensions.
Reviewer's GuideAdds content-based magic number detection to the image viewer plugin so it can handle extension-less image files while preserving existing extension-based behavior and directory checks. Class diagram for Plugin image handling with magic number checksclassDiagram
class Plugin {
+bool CanHandle(string path)
-static bool IsImageByMagicNumber(string path)
}
class WebHandler {
+static bool TryCanHandle(string path)
}
class Directory {
+static bool Exists(string path)
}
class Path {
+static bool HasExtension(string path)
}
class File {
+static bool Exists(string path)
}
class FileStream {
+FileStream(string path, FileMode mode, FileAccess access, FileShare share)
+long Length
+int Read(byte[] buffer, int offset, int count)
+void Dispose()
}
Plugin ..> WebHandler : uses
Plugin ..> Directory : uses
Plugin ..> Path : uses
Plugin ..> File : uses
Plugin ..> FileStream : uses
class FileMode {
}
class FileAccess {
}
class FileShare {
}
FileStream ..> FileMode : uses
FileStream ..> FileAccess : uses
FileStream ..> FileShare : uses
Flow diagram for CanHandle image detection with magic numbersflowchart TD
A[Start CanHandle path] --> B{WebHandler.TryCanHandle path}
B -- true --> Z[Return true]
B -- false --> C{Directory.Exists path}
C -- true --> Y[Return false]
C -- false --> D{Has well known image extension}
D -- true --> Z
D -- false --> E{Path.HasExtension path}
E -- false --> F[Call IsImageByMagicNumber path]
F --> G{IsImageByMagicNumber result}
G -- true --> Z
G -- false --> Y
E -- true --> Y
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 1 issue, and left some high level feedback:
- The
IsImageByMagicNumbermethod catches all exceptions and silently returnsfalse; consider narrowing the exception types or at least logging unexpected exceptions to avoid hiding underlying IO or permission issues. - The magic number comparisons are currently done with repeated index checks; you might improve readability and maintainability by defining static readonly byte signatures (e.g.,
PngSignature,JpegSignature) and using a small helper to compare spans/arrays.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `IsImageByMagicNumber` method catches all exceptions and silently returns `false`; consider narrowing the exception types or at least logging unexpected exceptions to avoid hiding underlying IO or permission issues.
- The magic number comparisons are currently done with repeated index checks; you might improve readability and maintainability by defining static readonly byte signatures (e.g., `PngSignature`, `JpegSignature`) and using a small helper to compare spans/arrays.
## Individual Comments
### Comment 1
<location> `QuickLook.Plugin/QuickLook.Plugin.ImageViewer/Plugin.cs:153-156` </location>
<code_context>
+ if (bytesRead < 4)
+ return false;
+
+ // PNG: 89 50 4E 47 0D 0A 1A 0A
+ if (bytesRead >= 8 &&
+ buffer[0] == 0x89 && buffer[1] == 0x50 && buffer[2] == 0x4E && buffer[3] == 0x47 &&
+ buffer[4] == 0x0D && buffer[5] == 0x0A && buffer[6] == 0x1A && buffer[7] == 0x0A)
+ {
+ return true;
</code_context>
<issue_to_address>
**suggestion:** Consider extracting magic numbers into constants or using spans for cleaner signature checks.
The current chain of `buffer[i] == ...` checks works but is hard to read and extend as you add more formats. Defining each signature as a `ReadOnlySpan<byte>` and comparing via `SequenceEqual` on a slice of `buffer`, or moving each format check into a small helper method, would make the code clearer and easier to maintain while avoiding duplicated index logic.
Suggested implementation:
```csharp
var buffer = new byte[12];
var bytesRead = fs.Read(buffer, 0, buffer.Length);
if (bytesRead < 4)
return false;
ReadOnlySpan<byte> pngSignature = new byte[] { 0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A };
// PNG: 89 50 4E 47 0D 0A 1A 0A
if (bytesRead >= pngSignature.Length &&
buffer.AsSpan(0, pngSignature.Length).SequenceEqual(pngSignature))
{
return true;
}
if (Directory.Exists(path))
```
If `System` is not already imported at the top of this file, add `using System;` so that `ReadOnlySpan<byte>` and `AsSpan` are available. You can follow this pattern to add other signatures (e.g., JPEG, GIF, WEBP) by defining additional `ReadOnlySpan<byte>` values and comparing them via `buffer.AsSpan(start, length).SequenceEqual(signature)` to keep all magic numbers centralized and readable.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements magic number checks to detect images in files without extensions.
PR Checklist
I've never built something made with C# before (I only use python and C) so I tried installing Visual Studio to build and test the application, but I couldn't stop getting "Metadata file '.dll' could not be found" errors even after spending an hour attempting various fixes. The changes are simple enough that I hope you will accept them after a quick test on your end.
Not applicable, I would say.
Brief Description of Changes
Please briefly describe the main changes in this PR:
As indicated in the title, the goal is allowing QuickLook to preview image files even when the extension has been removed, which can be useful for example when exploring a browser's cache folders.
This complements the already present support for extension-less video files.
Related Issue (if any)
Please provide related issue numbers:
None.
Additional Notes
Add any extra notes here:
None.
Summary by Sourcery
New Features: