-
Notifications
You must be signed in to change notification settings - Fork 103
Better manifest file detection #758
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
Changes from all commits
6a01d81
929a797
31708b4
c2cfe06
11499e0
1b46d6e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -5,6 +5,7 @@ | |||||
using System.Globalization; | ||||||
using System.Security.Cryptography; | ||||||
using System.Security.Cryptography.X509Certificates; | ||||||
using System.Xml; | ||||||
using Microsoft.Extensions.DependencyInjection; | ||||||
using Microsoft.Extensions.FileSystemGlobbing.Abstractions; | ||||||
using Microsoft.Extensions.Logging; | ||||||
|
@@ -126,8 +127,8 @@ await Parallel.ForEachAsync(files, _parallelOptions, async (file, state) => | |||||
|
||||||
// Inner files are now signed | ||||||
// now look for the manifest file and sign that if we have one | ||||||
|
||||||
FileInfo? manifestFile = filteredFiles.SingleOrDefault(f => ".manifest".Equals(f.Extension, StringComparison.OrdinalIgnoreCase)); | ||||||
var appManifestFromDeploymentManifest = GetApplicationManifestForDeploymentManifest(file); | ||||||
FileInfo? manifestFile = filteredFiles.SingleOrDefault(f => f.Name.Equals(appManifestFromDeploymentManifest, StringComparison.OrdinalIgnoreCase)); | ||||||
|
||||||
string fileArgs = $@"-update ""{manifestFile}"" {args}"; | ||||||
|
||||||
|
@@ -273,5 +274,53 @@ public void CopySigningDependencies(FileInfo deploymentManifestFile, DirectoryIn | |||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
/// <summary> | ||||||
/// Try and find the application manifest (.manifest) file from a clickonce application manifest (.application / .vsto | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't it be:
Suggested change
|
||||||
/// There might not be one, if the user is attempting to only re-sign the deployment manifest without touching other files. | ||||||
/// This is necessary because there might be multiple *.manifest files present, e.g. if a DLL that's part of the clickonce | ||||||
/// package ships its own assembly manifest which isn't a clickonce application manifest. | ||||||
/// </summary> | ||||||
/// <param name="deploymentManifest"></param> | ||||||
/// <returns>A string containing the file name of the Application manifest, or null if it couldn't be found.</returns> | ||||||
/// <exception cref="InvalidDataException"></exception> | ||||||
private string? GetApplicationManifestForDeploymentManifest(FileInfo deploymentManifest) | ||||||
{ | ||||||
var xml = new XmlDocument(); | ||||||
xml.Load(deploymentManifest.FullName); | ||||||
// there should only be a single result here, if the file is a valid clickonce manifest. | ||||||
var dependentAssemblies = xml.GetElementsByTagName("dependentAssembly"); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We prefer using the explicit type if the implied type is not super obvious. It will facilitate understanding both in a browser and IDE. |
||||||
if (dependentAssemblies.Count != 1) | ||||||
{ | ||||||
Logger.LogDebug(Resources.ApplicationManifestNotFound); | ||||||
return null; | ||||||
} | ||||||
|
||||||
var node = dependentAssemblies.Item(0); | ||||||
if (node is null || node.Attributes is null) | ||||||
{ | ||||||
Logger.LogDebug(Resources.ApplicationManifestNotFound); | ||||||
return null; | ||||||
} | ||||||
|
||||||
var applicationManifestFileNameAttribute = node.Attributes["codebase"]; | ||||||
if (applicationManifestFileNameAttribute is null || string.IsNullOrEmpty(applicationManifestFileNameAttribute.Value)) | ||||||
{ | ||||||
Logger.LogDebug(Resources.ApplicationManifestNotFound); | ||||||
return null; | ||||||
} | ||||||
|
||||||
// the codebase attribute can be a relative file path (e.g. Application Files\MyApp_1_0_0_0\MyApp.dll.manifest) or | ||||||
// a URI (e.g. https://my.cdn.com/clickonce/MyApp/ApplicationFiles/MyApp_1_0_0_0/MyApp.dll.manifest) so we need to | ||||||
// handle both cases and extract just the file name part. | ||||||
// | ||||||
// we only try and parse absolute uris, because a relative uri can just be treated like a file path for our purposes | ||||||
if (Uri.TryCreate(applicationManifestFileNameAttribute.Value, UriKind.Absolute, out var uri)) | ||||||
{ | ||||||
Path.GetFileName(uri.LocalPath); // works for http(s) and file:// uris | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a bug here. I think you meant:
Suggested change
Seems worth adding a unit test for this case. I think you can convert your new, existing unit test into a theory and parameterize |
||||||
} | ||||||
|
||||||
return Path.GetFileName(applicationManifestFileNameAttribute.Value); | ||||||
} | ||||||
} | ||||||
} | ||||||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
This is avoidable if
appManifestFromDeploymentManifest
is null.Also, can we rename
manifestFile
toapplicationManifestFile
since now we're dealing with potentially many different kinds of manifest files. We had deployment and application manifests, but now we're dealing with other manifests (like assembly manifests).