Make Swift Detector experimental - #1866
Ryan Brandenburg (ryanbrandenburg) wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The only noted issue is a non-blocking documentation nit.
Review tier: Lite
Findings: None
What changed in this PR
Makes the Swift Package Manager detector experimental.
Changes:
- Implements experimental detector behavior.
- Adds lifecycle coverage.
- Updates detector documentation.
| File | Summary |
|---|---|
test/Microsoft.ComponentDetection.Detectors.Tests/ComponentDetectorTests.cs |
Verifies Swift’s experimental status. |
src/Microsoft.ComponentDetection.Detectors/swiftpm/SwiftResolvedComponentDetector.cs |
Changes the detector lifecycle classification. |
docs/detectors/README.md |
Documents the detector status. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
👋 Hi! It looks like you modified some files in the
If none of the above scenarios apply, feel free to ignore this comment 🙂 |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Multiple public constructors can make System.Text.Json deserialization of SwiftComponent fail; add serializer compatibility and a deserialization test.
Get a fresh assessment by requesting another Copilot review.
Review tier: Lite
Findings: 1
Open (1)
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Constructor mismatches cause compilation failures and swap the Kind and CommitHash values.
Get a fresh assessment by requesting another Copilot review.
Review tier: Lite
Findings: 3
Open (4)
Resolved since last review (1)
| public SwiftComponent(string name, string version, string packageUrl, string hash) | ||
| /// <param name="hash">The Git commit hash of the component.</param> | ||
| /// <param name="kind">The Swift package kind.</param> | ||
| public SwiftComponent(string name, string version, string packageUrl, string hash, string kind) |
| hash: package.State.Revision); | ||
| var newDetectedSwiftComponent = new DetectedComponent(component: detectedSwiftComponent); | ||
| singleFileComponentRecorder.RegisterUsage(newDetectedSwiftComponent); | ||
|
|
There was a problem hiding this comment.
why is this being removed? If a swift package manager is using a git url one can get additional vulnerability coverage, see https://osv.dev/list?ecosystem=GIT. Not all swift components come from git tho, so adding the relevant condition is necessary to avoid noise.
There was a problem hiding this comment.
First some context
A standard "pins" entry in a Package.resolved file looks like the following:
{
"identity" : "aexml",
"kind" : "remoteSourceControl",
"location" : "https://github.com/tadija/AEXML.git",
"state" : {
"revision" : "db806756c989760b35108146381535aec231092b",
"version" : "4.7.0"
}
},There exist other kinds such as "registry", "local", and sourceControl but they are explicitly not supported by the current detector and seem less common.
And here's some example entries from before my change, both from the same ScanManifest output file:
The SwiftComponent
{
"locationsFoundAt": [
"/Cuckoo/Generator/Generator.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved"
],
"component": {
"type": "Swift",
"name": "aexml",
"version": "4.7.0",
"packageUrl": {
"Scheme": "pkg",
"Type": "swift",
"Namespace": "github.com/tadija",
"Name": "aexml",
"Version": "4.7.0",
"Qualifiers": {
"repository_url": "https://github.com/tadija/AEXML.git"
},
"Subpath": null
},
"id": "aexml 4.7.0 - Swift"
},
"detectorId": "Swift",
"isDevelopmentDependency": null,
"dependencyScope": null,
"topLevelReferrers": [],
"ancestralReferrers": [],
"containerDetailIds": [],
"containerLayerIds": {},
"targetFrameworks": []
},The GitComponent
{
"locationsFoundAt": [
"/Cuckoo/Generator/Generator.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved"
],
"component": {
"type": "Git",
"repositoryUrl": "https://github.com/tadija/AEXML.git",
"commitHash": "db806756c989760b35108146381535aec231092b",
"tag": "4.7.0",
"packageUrl": {
"Scheme": "pkg",
"Type": "github",
"Namespace": "tadija",
"Name": "aexml",
"Version": "db806756c989760b35108146381535aec231092b",
"Qualifiers": null,
"Subpath": null
},
"id": "https://github.com/tadija/AEXML.git : db806756c989760b35108146381535aec231092b - Git"
},
"detectorId": "Swift",
"isDevelopmentDependency": null,
"dependencyScope": null,
"topLevelReferrers": [],
"ancestralReferrers": [],
"containerDetailIds": [],
"containerLayerIds": {},
"targetFrameworks": []
},The commit hash was the only bit of information present in the GitComponent but not in the SwiftComponent, so I thought we could prevent some confusion/duplication by only reporting a component once. If GitComponent's get some kind of special treatment then I think there's a case for making all "kind" : "remoteSourceControl", entries (the only kind currently supported but that's a different question) GitComponents and not emitting the SwiftComponent at all (until/unless we support "kind": "registry" or others).
| [JsonPropertyName("version")] | ||
| public string Version { get; } | ||
|
|
||
| [JsonPropertyName("kind")] |
There was a problem hiding this comment.
do we truly need to commit hash and Kind? Mind adding context on why these was added, possibly Kind makes sense, but commit hash might explode the data a bit, or we can end up with duplicate packages if one has commit and the other one does not.
There was a problem hiding this comment.
I don't believe it is possible to have a Package.resolved remoteSourceControl entry without a "revision" (hash). Here's some relevant code from SwiftPM. Unfortunately they don't have a formal specification for Package.resolved files but it would make sense that for your Git-based package manager the revision would always be required (and controlling) because the tag/version can be changed at a later date (and it would even be quite common for scenerios like "latest"). I'd go so far as to say that a Package.resolved remoteSourceControl entry without state.revision is malformed.
I think there's a stronger case for removing "kind" since right now we explicitly only support "remoteSourceControl" values out of "kind", but I would still vote to include it to make future support for "kind": "registry" more straightforward, and so that we can add statistics for the same to see if adding that support is worthwhile.
I'm less worried about exploding the data here since as long as we only keep 1 of the SwiftComponent and GitComponent we're starting out having halved our output from this detector.
| }, | ||
| subpath: null); | ||
|
|
||
| protected override string ComputeBaseId() => $"{this.Name} {this.Version} - {this.Type}"; |
There was a problem hiding this comment.
this component ID, is what makes the component unique across a scan. If we are adding new properties we need to review if they indeed make a component unique and include here, or update existing entry to include the additional fields, etc. We should be consistent tho, and have a reason why to capture that metadata.
There was a problem hiding this comment.
Ah, good catch. If we land on reporting "remoteSourceControl" as a SwiftComponent I think we should update this to include the CommitHash, and potentially remove the version until we have support for kind=registry. If we report remoteSourceControl as GitComponent then I don't think this file will get used at all until we add registry support.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Address the SwiftComponent serialization and public-constructor compatibility issues before approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
| public SwiftComponent() | ||
| { | ||
| /* Reserved for deserialization */ |
This detector exists but isn't getting a lot of use since it's off by default. Let's make it an experiment so it gets a bit of exercise.