-
Notifications
You must be signed in to change notification settings - Fork 115
Conversation
30bdd3a to
0eebbb8
Compare
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.
Pull request overview
Adds dormant ClickOnce file-graph resolution for the version 2 signing pipeline.
Changes:
- Adds graph models and application/deployment manifest resolvers.
- Implements payload mapping, fallback resolution, diagnostics, and adjacent executable discovery.
- Adds comprehensive tests and localized resources.
Show a summary per file
| File | Description |
|---|---|
test/Sign.Core.Test/Tools/ClickOnce/ClickOnceFileGraphResolverTests.cs |
Tests resolution behavior and edge cases. |
test/Sign.Core.Test/Tools/ClickOnce/ClickOnceFileGraphModelTests.cs |
Tests graph models and validation. |
src/Sign.Core/Tools/ClickOnce/ClickOncePayloadFileResolver.cs |
Resolves referenced payload files. |
src/Sign.Core/Tools/ClickOnce/ClickOnceManifestDiagnostic.cs |
Models manifest diagnostics. |
src/Sign.Core/Tools/ClickOnce/ClickOnceFileGraphResolutionException.cs |
Defines resolution failures. |
src/Sign.Core/Tools/ClickOnce/ClickOnceFileGraphEntryKind.cs |
Defines graph entry categories. |
src/Sign.Core/Tools/ClickOnce/ClickOnceFileGraphEntry.cs |
Models individual graph entries. |
src/Sign.Core/Tools/ClickOnce/ClickOnceFileGraph.cs |
Models resolved ClickOnce graphs. |
src/Sign.Core/Tools/ClickOnce/ClickOnceDeployManifestFileGraphResolver.cs |
Resolves deployment-manifest graphs. |
src/Sign.Core/Tools/ClickOnce/ClickOnceApplicationManifestFileGraphResolver.cs |
Resolves application-manifest graphs. |
src/Sign.Core/Resources.resx |
Adds resolution messages. |
src/Sign.Core/Resources.Designer.cs |
Exposes generated resource properties. |
src/Sign.Core/xlf/Resources.zh-Hant.xlf |
Adds Traditional Chinese localization entries. |
src/Sign.Core/xlf/Resources.zh-Hans.xlf |
Adds Simplified Chinese localization entries. |
src/Sign.Core/xlf/Resources.tr.xlf |
Adds Turkish localization entries. |
src/Sign.Core/xlf/Resources.ru.xlf |
Adds Russian localization entries. |
src/Sign.Core/xlf/Resources.pt-BR.xlf |
Adds Brazilian Portuguese localization entries. |
src/Sign.Core/xlf/Resources.pl.xlf |
Adds Polish localization entries. |
src/Sign.Core/xlf/Resources.ko.xlf |
Adds Korean localization entries. |
src/Sign.Core/xlf/Resources.ja.xlf |
Adds Japanese localization entries. |
src/Sign.Core/xlf/Resources.it.xlf |
Adds Italian localization entries. |
src/Sign.Core/xlf/Resources.fr.xlf |
Adds French localization entries. |
src/Sign.Core/xlf/Resources.es.xlf |
Adds Spanish localization entries. |
src/Sign.Core/xlf/Resources.de.xlf |
Adds German localization entries. |
src/Sign.Core/xlf/Resources.cs.xlf |
Adds Czech localization entries. |
Review details
Tip
Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Files not reviewed (1)
- src/Sign.Core/Resources.Designer.cs: Generated file
- Files reviewed: 24/25 changed files
- Comments generated: 0
- Review effort level: Balanced
0eebbb8 to
24496c7
Compare
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.
Review details
Files not reviewed (1)
- src/Sign.Core/Resources.Designer.cs: Generated file
- Files reviewed: 24/25 changed files
- Comments generated: 0 new
- Review effort level: Balanced
24496c7 to
d99a714
Compare
8e4dc41 to
57f5560
Compare
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.
Could we strengthen the file-graph model before merging this PR? I think follow-up PR #1058 demonstrates that the current shape requires consumers to reconstruct invariants that could instead be encoded here.
1. Deployment state can be half-present
ClickOnceFileGraph represents deployment input with two independently nullable properties:
ClickOnceFileGraphEntry? DeploymentManifest IDeployManifest? DeployManifest
That produces four possible states:
DeploymentManifest |
DeployManifest |
Meaning |
|---|---|---|
| present | present | Valid deployment input |
| absent | absent | Valid explicit application input |
| present | absent | Invalid half-present deployment |
| absent | present | Invalid half-present deployment |
The constructor currently accepts all four. For example, this compiles:
ClickOnceFileGraph graph = new( deploymentManifest: deploymentEntry, deployManifest: null, applicationManifest, applicationManifestModel, payloads, adjacentExecutables, diagnostics);
The inverse also compiles:
ClickOnceFileGraph graph = new( deploymentManifest: null, deployManifest, applicationManifest, applicationManifestModel, payloads, adjacentExecutables, diagnostics);
This is already visible in PR #1058's candidate selection:
bool isDeploymentInput = graph.DeploymentManifest is not null; if (isDeploymentInput) { candidates.Add( new StagingCandidate( graph.DeploymentManifest!, isUpdateInputOnly: false)); }
The stager must choose one nullable property as the authority and use !. Consequently:
DeploymentManifest != nullandDeployManifest == nullis silently treated as deployment input.DeploymentManifest == nullandDeployManifest != nullis silently treated as application input.
The current resolvers construct valid pairs, but the result type itself does not preserve that invariant for future consumers.
2. Deployment-only adjacent files can exist without a deployment
The model also permits this:
ClickOnceFileGraph graph = new( deploymentManifest: null, deployManifest: null, applicationManifest, applicationManifestModel, payloads: Array.Empty<ClickOnceFileGraphEntry>(), adjacentExecutables: new[] { new ClickOnceFileGraphEntry( setupFile, "setup.exe", ClickOnceFileGraphEntryKind.Setup) }, diagnostics);
This represents an explicit application-manifest input containing a deployment-only setup.exe.
PR #1058 will actually include that file in default staging because adjacent executables are added independently of isDeploymentInput:
if (mode == ClickOnceSigningMode.Default) { candidates.AddRange( graph.AdjacentExecutables.Select( entry => new StagingCandidate( entry, isUpdateInputOnly: false))); }
The resolvers currently avoid constructing this state, but the aggregate allows it and the next layer assigns it behavior.
3. A generic entry permits contradictory roles
ClickOnceFileGraphEntry accepts any combination of:
Source TargetPath Kind ManifestReference? MappingAddedSuffix?
For example, this payload can be mislabeled as a setup executable:
ClickOnceFileGraphEntry entry = new( payloadFile, @"bin\payload.dll", ClickOnceFileGraphEntryKind.Setup, payloadReference);
PR #1058 uses Kind to determine the target-path base directory:
string basePath = graphEntry.Kind == ClickOnceFileGraphEntryKind.Payload ? applicationDirectoryPath : _rootPath;
The mislabeled payload would therefore be staged relative to the deployment root rather than the application-manifest directory.
Other currently constructible combinations include:
// Payload without the manifest reference that owns it. new ClickOnceFileGraphEntry( payloadFile, "payload.dll", ClickOnceFileGraphEntryKind.Payload, manifestReference: null); // Setup executable with a manifest reference and mapping suffix. new ClickOnceFileGraphEntry( setupFile, "setup.exe", ClickOnceFileGraphEntryKind.Setup, payloadReference, mappingAddedSuffix: ".deploy"); // Entry path disagrees with the manifest reference path. new ClickOnceFileGraphEntry( payloadFile, @"bin\payload.dll", ClickOnceFileGraphEntryKind.Payload, new FileReference { TargetPath = @"lib\payload.dll" });
The follow-up therefore needs additional validation and null-forgiving operations, such as validating that a mapped entry has a reference and later using ManifestReference!.
Suggested model
Would it be safer to represent the fixed hierarchy directly?
internal sealed class ResolvedClickOncePublication { internal ResolvedClickOnceDeployment? Deployment { get; } internal ResolvedClickOnceApplication Application { get; } internal IReadOnlyList<ClickOnceManifestDiagnostic> Diagnostics { get; } } internal sealed class ResolvedClickOnceDeployment { internal FileInfo Source { get; } internal IDeployManifest Manifest { get; } internal AssemblyReference ApplicationManifestReference { get; } internal IReadOnlyList<ClickOnceAdjacentExecutable> AdjacentExecutables { get; } } internal sealed class ResolvedClickOnceApplication { internal FileInfo Source { get; } internal IApplicationManifest Manifest { get; } internal IReadOnlyList<ResolvedClickOncePayload> Payloads { get; } } internal sealed class ResolvedClickOncePayload { internal FileInfo Source { get; } internal BaseReference Reference { get; } internal string ApplicationRelativeTargetPath => Reference.TargetPath; internal string? MappingAddedSuffix { get; } }
This would make the important invariants structural:
- deployment entry and model are either both present or both absent;
- adjacent executables cannot exist without a deployment;
- every payload has a manifest reference;
- payload paths are explicitly application-relative;
- the exposed target path cannot disagree with
Reference.TargetPath; - downstream code does not need
Kindbranching to recover an object's role.
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.
Thanks for the detailed feedback, @kartheekp-ms . I adopted the typed hierarchy you suggested and addressed the three invalid model states you identified:
ResolvedClickOncePublishLayouthas one optionalResolvedClickOnceDeploymentand a requiredResolvedClickOnceApplication, eliminating independently nullable deployment state.- Deployment groups its source, manifest, and application-manifest reference. The constructor requires that reference to be the manifest's actual
EntryPointobject. - Application groups its source, manifest, and typed payloads.
- Dedicated payload and adjacent-executable types replace the generic graph entry and broad role enum. Every payload requires its manifest reference, while adjacent executables cannot exist without deployment context.
- Payloads validate the source/target filename relationship and derive
IsFileExtensionMapped; adjacent executables derive their target path from their source filename and validatesetup.exeorLauncher.exeagainst their narrow kind.
I made a few deliberate departures from the suggested shape:
- I used
ResolvedClickOncePublishLayoutrather thanResolvedClickOncePublication, because the result describes the resolved on-disk layout. - Adjacent executables remain on the top-level layout rather than under
Deployment. They are discovered by publish-directory convention rather than referenced by the deployment manifest, but the layout constructor still requires deployment context. ResolvedClickOncePayload.TargetPathis an immutable resolution-time snapshot ofReference.TargetPath, not a live projection. This preserves the target value against which source matching and file-extension mapping were validated, while the underlyingManifestUtilitiesreference remains mutable.- I retained
TargetPathrather thanApplicationRelativeTargetPath. It is the manifest's target-path value; rooted values are rejected, but relative traversal such as..is still permitted pending containment validation. IsFileExtensionMappedis a Boolean rather than a nullable suffix because ClickOnce mapping adds only the fixed.deploysuffix.
I also updated the authoritative signing specification to require fail-fast deployment-reference validation before ResolveFiles. A deployment manifest must contain exactly one assembly dependency and no file references; that dependency must be DeployManifest.EntryPoint itself and must not be prerequisite, optional, or resource-fallback. Resource-fallback attributes are inspected in the preserved XML because ManifestUtilities neither models nor preserves them in its object model. Unsupported structures report the planned --clickonce-signing-version 2 --no-update-clickonce-manifest alternative.
Staging and signing remain outside this PR. The updated PR description records the additional integration requirements; they are not implemented by this resolver change.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: be91288d-5106-4c59-a1fa-c5353dccb828
Add typed resolved models and enforce deployment, payload, mapping, diagnostic, and target-path invariants. Preserve manifest input, validate unsupported deployment shapes, and expand resolver regression coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: de6a34fe-6989-49f6-a128-ee13323c7c9e
57f5560 to
588e39d
Compare
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.
Copilot review overview
🔵 Needs a closer look
Its broad manifest-resolution and path-safety behavior warrants final human review despite no unresolved comments.
Review tier: Balanced
Findings: None
Files not reviewed (1)
- src/Sign.Core/Resources.Designer.cs: Generated file
Uh oh!
There was an error while loading. Please reload this page.
Part of #1049.
Summary
ClickOnceApplicationPublishLayoutResolverClickOnceDeploymentPublishLayoutResolverClickOncePayloadResolverResolvedClickOncePublishLayoutResolvedClickOnceDeploymentResolvedClickOnceApplicationResolvedClickOncePayloadResolvedClickOnceAdjacentExecutableClickOncePublishLayoutResolutionExceptionwhile preserving ordered manifest diagnostics.ManifestUtilitiesneither models nor preserves. Reject unsupported shapes before file resolution soManifestUtilitiesormage.execannot reorder or rewrite them before signing.ResolvedClickOncePayloadvalidates that the source filename matches the target filename, with or without exactly one additional.deploysuffix, and derivesIsFileExtensionMappedfrom that relationship.ResolvedClickOnceAdjacentExecutablederives its target path from its source filename and validates the filename against its kind.setup.exeand unreferenced root-levelLauncher.exefiles; referenced launchers remain application payloads.Intentional scope boundary
This PR resolves and models the publish layout only. It does not register the resolvers with dependency injection and does not change production signing behavior or the CLI surface. Diagnostic text may reference the planned
--clickonce-signing-version 2 --no-update-clickonce-manifestoption, but the resolvers remain dormant until signer integration adds that option.Staging, relative-path containment validation, copying, temporary
.deploysuffix removal and restoration, manifest metadata updates, signing-operation coordination, and signing remain deferred to the signer-integration work. Resolution rejects rooted target paths but does not rewrite or remap accepted relative paths. Staging must ensure those relative paths remain within the staging directory and fail when a referenced layout cannot be represented safely.The deployment resolver establishes that
Application.Sourceis the file initially identified byDeployment.ApplicationManifestReference.ResolvedPath. This is a resolution-time postcondition rather than a durable model invariant because staging must redirect that mutableResolvedPath. The staging implementation must stageApplication.Source, redirect the deployment entry point to that exact staged copy, verify that it is contained within the staging directory, and only then update deployment-manifest metadata. At the start of staging, it must capture the deployment entry point's target path. Immediately before deployment-manifest metadata update and serialization, it must fail unless the live entry point is still the resolvedApplicationManifestReference, remains the sole assembly reference with no file references, and its target path still ordinally equals the captured value. It must not reconcile or serialize mutated deployment structure.For payloads,
ResolvedClickOncePayload.TargetPathis an immutable resolution-time snapshot ofReference.TargetPath, not a live projection. Staging must use that snapshot to determine the intended layout, preserve the manifest-relative meaning rather than remapping it, and bind each mutable reference'sResolvedPathto the corresponding contained staged file before updating metadata. Before computing staging destinations or collision claims, and again immediately before updating application-manifest metadata and serializing the manifest, staging must verify with ordinal comparison that each mutableReference.TargetPathstill equals its resolved snapshot. A mismatch must fail staging rather than reconcile or emit an inconsistent manifest.The deployment-directory payload fallback is a source-discovery mechanism. Signer integration must retain an explicit mapping from every staged file to the original
ResolvedClickOncePayload.Sourceit was staged from and copy signed bytes back to that exact source path. It must not derive copy-back destinations from staging-relative paths or reuse a directory-mirroring copy-back seam that would create a new application-directory file for a deployment-directory fallback source. Integration coverage must verify that a fallback payload is returned to its original source and that no unintended target-relative file is created.Discovery can also produce two resolved references to one physical file: a root-level
setup.exereferenced by the application manifest is classified as both aResolvedClickOncePayloadand aResolvedClickOnceAdjacentExecutable, with bothSourcevalues identifying the same file and potentially the same target path. Signer integration must apply the same source-keyed coordinated signing operation to both references so the file is staged, signed, and copied back exactly once rather than processed as two independent operations.