Skip to content
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
handle codebase values that are uris
  • Loading branch information
jmtpt committed Aug 28, 2024
commit c2cfe06ee9fc51f75d21c121c44dbf0df6585bf7
20 changes: 16 additions & 4 deletions src/Sign.Core/DataFormatSigners/ClickOnceSigner.cs
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ 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
var appManifestFromDeploymentManifest = GetApplicationManifestForDeploymentManifest(file);
FileInfo? manifestFile = filteredFiles.SingleOrDefault(f => f.Name.Equals(appManifestFromDeploymentManifest?.Name, StringComparison.OrdinalIgnoreCase));
FileInfo? manifestFile = filteredFiles.SingleOrDefault(f => f.Name.Equals(appManifestFromDeploymentManifest, StringComparison.OrdinalIgnoreCase));
Copy link
Collaborator

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 to applicationManifestFile 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).


string fileArgs = $@"-update ""{manifestFile}"" {args}";

Expand Down Expand Up @@ -278,11 +278,13 @@ public void CopySigningDependencies(FileInfo deploymentManifestFile, DirectoryIn
/// <summary>
/// Try and find the application manifest (.manifest) file from a clickonce application manifest (.application / .vsto
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be:

Suggested change
/// Try and find the application manifest (.manifest) file from a clickonce application manifest (.application / .vsto
/// Try and find the application manifest (.manifest) file from a clickonce deployment manifest (.application / .vsto

/// 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 FileInfo pointing to the Application manifest, or null if it couldn't be found.</returns>
/// <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 FileInfo? GetApplicationManifestForDeploymentManifest(FileInfo deploymentManifest)
private string? GetApplicationManifestForDeploymentManifest(FileInfo deploymentManifest)
{
var xml = new XmlDocument();
xml.Load(deploymentManifest.FullName);
Expand All @@ -308,7 +310,17 @@ public void CopySigningDependencies(FileInfo deploymentManifestFile, DirectoryIn
return null;
}

return new FileInfo(Path.Combine(deploymentManifest.Directory!.FullName, applicationManifestFileNameAttribute.Value));
// 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a bug here. I think you meant:

Suggested change
Path.GetFileName(uri.LocalPath); // works for http(s) and file:// uris
return Path.GetFileName(uri.LocalPath); // works for http(s) and file:// uris

Seems worth adding a unit test for this case. I think you can convert your new, existing unit test into a theory and parameterize codebase with both scenarios.

}

return Path.GetFileName(applicationManifestFileNameAttribute.Value);
}
}
}