Repository navigation
fix(plugin-catalog): the repair lane re-asserts only from a source trusted to be the recorded one - #6533
Conversation
…usted to be the recorded one The boot repair lane picked the listing to re-assert from by package id, preferring one whose PackageManifest.Source equals the install record's. A directly read repository can author that field, so another repository listing the same id under the recorded source's name could land its files over the installed package. Where the record names a source, the lane now requires a provenance-trusted match (a registry stamp, or the candidate's configured source name equals the recorded source), and names the refusal when none serves it. A legacy record with no source keeps the first listing. Test: ARepairBindsToATrustedSourceTest — an impostor configured as another source, listed first and authoring the recorded name, landed its Guide body before the change; the recorded source's body comes back after it. Closes #6512 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approval recommended
The implementation and regression coverage are sound; only a non-blocking XML documentation placement issue remains.
1 open finding
What changed in this PR
Hardens plugin repair so packages are restored only from their recorded, provenance-trusted source.
Changes:
- Adds trusted-source candidate selection and fail-closed logging.
- Adds an impostor-source regression test.
- Documents repair provenance requirements.
| File | Description |
|---|---|
InstanceAutoRegistrationService.cs |
Enforces trusted repair provenance. |
ARepairBindsToATrustedSourceTest.cs |
Covers source impersonation. |
InstallCompleteness.md |
Documents the security behavior. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// <summary>Whether two source names are the same non-empty source (case-insensitive, like | ||
| /// <see cref="PluginGrantEntry.Matches"/>). Pure.</summary> | ||
| /// <summary> | ||
| /// The listing the repair lane re-asserts an installed package from (#6512), or null when the | ||
| /// record names a source and no listing is TRUSTED to be that source. Trusted means a registry's | ||
| /// stamp, or the candidate's source being the name its source was configured under | ||
| /// (<see cref="ProvenanceIsTrusted"/>); a manifest that merely authors the recorded name is not | ||
| /// enough. A legacy record with no source keeps the first listing. Pure. | ||
| /// </summary> | ||
| /// <param name="listings">Every listing of one package id, in source order.</param> | ||
| /// <param name="recordedSource">The source the install record was stamped with, or null.</param> | ||
| private static InstallCandidate? RepairCandidate(IReadOnlyList<InstallCandidate> listings, string? recordedSource) => | ||
| string.IsNullOrWhiteSpace(recordedSource) | ||
| ? listings.FirstOrDefault() | ||
| : listings.FirstOrDefault(c => ProvenanceIsTrusted(c) && SameSource(c.Package.Source, recordedSource)); | ||
|
|
||
| private static bool SameSource(string? a, string? b) => | ||
| !string.IsNullOrWhiteSpace(a) && string.Equals(a.Trim(), b?.Trim(), StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
Confirmed: the one-line SameSource summary (lines 1953-1954) now sits directly above RepairCandidate's own summary, so RepairCandidate carries two <summary> elements and SameSource carries none. It is a misplaced doc comment only: no behaviour, no compile warning (the CI build is -warnaserror and green on this head), and RepairCandidate's own summary, which states its actual contract, is intact.
Under this round's converge rule a comment-only correction does not earn a push (every push restarts the review and the dependent-suites run on a congested queue), so I am leaving the head as it is. The two lines move back above SameSource the next time this file is edited.
Test Results 12 files - 12 12 suites - 12 36m 24s ⏱️ - 17m 26s Results for commit 381a9c9. ± Comparison against base commit c8f1f54. This pull request removes 4411 and adds 8 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |

What was wrong
InstanceAutoRegistrationService.ReassertInstalled, the boot repair lane, chose the listing to re-assert from by package id. Since #6496 it preferred a listing whosePackageManifest.Sourceequals the install record's source. A directly read repository can author that field in itspackage.json. So another direct repository listing the same id, and claiming the recorded source's name, could be chosen. For a free package,Landthen fetched and wrote that repository's files over the installed package. (No commercial authority leaked:ProvenanceIsTrustedalready withheld it.)The change
RepairCandidate(listings, recordedSource):ProvenanceIsTrusted(a registry's stamp, or the candidate'sSourceequals the name its source was configured under) and the same source name.The partition, module-version and config-authority checks after selection are unchanged.
Tests
ARepairBindsToATrustedSourceTest(Memex.Portal.Shared.Test) has two direct sources listing the same id at the same module version:registered-1, served the install, so the record namesregistered-1;registered-0and listed FIRST, authors"source": "registered-1"and serves a differentGuide.md.After a lost
Guide, the repair restores the honest body. Control: the record namesregistered-1.Falsified: with the old selection restored, the test fails — the restored Guide reads
IMPOSTOR body.Local:
dotnet build test/Memex.Portal.Shared.Test -c Release -warnaserror→ 0 warnings, 0 errors. The new test plusIncompleteInstallIsReassertedTest(the legacy no-source path),ADeploymentNamedCommercialPackage*,AnAuthoredSourceClaimsNoAuthorityTest,InstallCompletenessTestandInstallRecordFollowsItsPartitionTest→ 45/45 passed.Doc:
Doc/Architecture/InstallCompleteness— new paragraph …and only from the source the record names, by provenance.Closes #6512
Pairs-with: none — no public type or member removed.
🤖 Generated with Claude Code