feat(plugins): enforce apiVersion; launcher plugins default ON with "none" opt-out
Two gaps from the MossTank shipment review. **apiVersion was declared in every manifest and checked by nothing.** The loader now refuses an unsupported contract BEFORE loading any code from the plugin — checking after the fact is not equivalent, because by then the assembly is in a collectible context and the mismatch surfaces as a type-load or missing-member failure from inside the plugin, which reads like the plugin is broken rather than built for a different host. PluginApi (Current / MinimumSupported) lives in Plugin.Abstractions beside the contract it versions, and the refusal is a distinct PluginApiVersionException so callers can tell "update the client or the plugin" from "this plugin is broken". The tests pin the ordering too: a manifest with a future apiVersion AND a missing dll must fail on the version, a supported one on the dll. **A launcher-launched client loaded no plugins until the user typed ids.** LA5 distinguishes an omitted allow-list (load all) from an explicit empty one (load none); a fresh character profile's list is empty, so it composed to load-none. Direct launches pass null and load everything -- which is why the gap never showed in development: the two launch paths disagreed and the launcher was the one users get. This REVERSES the LA5 default deliberately: "nothing configured" now composes to the omitted list, so plugins are on by default, including ones installed later. The opt-out is kept -- losing it would be a real regression for stripped sessions -- respelled as the literal id "none", and the launcher's plugin box says so. The cross-host shared fixture composes its explicit-load-none case through the new spelling, keeping the reader-side contract tests (App and Headless both preserve an explicit empty list) exactly as they were. Complete Release suite: 14,469 tests pass on the standard hermetic lane filter, 0 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
parent
7e75be23d1
commit
cd6eefd0ba
8 changed files with 194 additions and 7 deletions
|
|
@ -20,6 +20,21 @@ public static class PluginLoader
|
||||||
ArgumentNullException.ThrowIfNull(manifest);
|
ArgumentNullException.ThrowIfNull(manifest);
|
||||||
ArgumentNullException.ThrowIfNull(host);
|
ArgumentNullException.ThrowIfNull(host);
|
||||||
|
|
||||||
|
// Refuse a contract we cannot honour BEFORE loading any code from it.
|
||||||
|
// Checking after the fact is not equivalent: the assembly is already in
|
||||||
|
// a collectible context, and the mismatch surfaces as a type-load or
|
||||||
|
// missing-member failure from inside the plugin, which reads like the
|
||||||
|
// plugin is broken rather than built for a different host.
|
||||||
|
if (!PluginApi.IsSupported(manifest.ApiVersion))
|
||||||
|
return new LoadedPlugin(
|
||||||
|
manifest,
|
||||||
|
Plugin: null,
|
||||||
|
LoadContext: null,
|
||||||
|
Error: new PluginApiVersionException(
|
||||||
|
$"plugin '{manifest.Id}' declares apiVersion {manifest.ApiVersion}, "
|
||||||
|
+ $"but this build supports {PluginApi.MinimumSupported}"
|
||||||
|
+ $"..{PluginApi.Current}"));
|
||||||
|
|
||||||
var dllPath = Path.Combine(pluginDirectory, manifest.EntryDll);
|
var dllPath = Path.Combine(pluginDirectory, manifest.EntryDll);
|
||||||
if (!File.Exists(dllPath))
|
if (!File.Exists(dllPath))
|
||||||
return new LoadedPlugin(
|
return new LoadedPlugin(
|
||||||
|
|
|
||||||
|
|
@ -64,6 +64,16 @@ public sealed record PluginManifest(
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// A plugin declared a contract version this build cannot load. Distinct from
|
||||||
|
/// <see cref="PluginManifestException"/> (a malformed manifest) because the
|
||||||
|
/// remedy differs: this one means update the plugin or the client.
|
||||||
|
/// </summary>
|
||||||
|
public sealed class PluginApiVersionException : Exception
|
||||||
|
{
|
||||||
|
public PluginApiVersionException(string message) : base(message) { }
|
||||||
|
}
|
||||||
|
|
||||||
public sealed class PluginManifestException : Exception
|
public sealed class PluginManifestException : Exception
|
||||||
{
|
{
|
||||||
public PluginManifestException(string message) : base(message) { }
|
public PluginManifestException(string message) : base(message) { }
|
||||||
|
|
|
||||||
|
|
@ -138,9 +138,7 @@ public static class SessionConfigComposer
|
||||||
Character = selector,
|
Character = selector,
|
||||||
Policy = policy,
|
Policy = policy,
|
||||||
Credential = new SessionCredentialDescriptor(),
|
Credential = new SessionCredentialDescriptor(),
|
||||||
// LA5 distinguishes an omitted allow-list (load all, preserving
|
Plugins = ComposePluginAllowList(character.Plugins),
|
||||||
// the developer flow) from an explicit empty list (load none).
|
|
||||||
Plugins = [.. character.Plugins],
|
|
||||||
LoginCommands = character.LoginCommands.Count > 0
|
LoginCommands = character.LoginCommands.Count > 0
|
||||||
? [.. character.LoginCommands]
|
? [.. character.LoginCommands]
|
||||||
: null,
|
: null,
|
||||||
|
|
@ -271,6 +269,41 @@ public static class SessionConfigComposer
|
||||||
string sessionId) =>
|
string sessionId) =>
|
||||||
Write(ComposeProbe(server, account, install, paths, sessionId));
|
Write(ComposeProbe(server, account, install, paths, sessionId));
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Maps a character's configured plugin ids to the session config's
|
||||||
|
/// allow-list.
|
||||||
|
/// </summary>
|
||||||
|
/// <remarks>
|
||||||
|
/// <para>
|
||||||
|
/// LA5 distinguishes an OMITTED allow-list (load every discovered plugin)
|
||||||
|
/// from an explicit EMPTY one (load none). A brand-new character profile
|
||||||
|
/// starts with an empty list, which meant a client that ships plugins
|
||||||
|
/// loaded none of them until the user typed an id — "ships with the
|
||||||
|
/// client" and "works out of the box" were different things, and the
|
||||||
|
/// difference was invisible: nothing was logged, the panel simply never
|
||||||
|
/// appeared.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// So "nothing configured" now maps to omitted — plugins are on by
|
||||||
|
/// default, including ones installed later. The opt-out is kept, because
|
||||||
|
/// losing it would be a real regression for anyone running a stripped
|
||||||
|
/// session: the literal id <c>none</c> maps to the explicit empty list.
|
||||||
|
/// </para>
|
||||||
|
/// </remarks>
|
||||||
|
private static List<string>? ComposePluginAllowList(IReadOnlyList<string> configured)
|
||||||
|
{
|
||||||
|
if (configured.Count == 0)
|
||||||
|
return null; // default: load all
|
||||||
|
|
||||||
|
if (configured.Count == 1
|
||||||
|
&& string.Equals(configured[0], "none", StringComparison.OrdinalIgnoreCase))
|
||||||
|
{
|
||||||
|
return []; // explicit: load none
|
||||||
|
}
|
||||||
|
|
||||||
|
return [.. configured];
|
||||||
|
}
|
||||||
|
|
||||||
private static ComposedSessionConfig Write(ComposedSessionConfig composed)
|
private static ComposedSessionConfig Write(ComposedSessionConfig composed)
|
||||||
{
|
{
|
||||||
string? directory = Path.GetDirectoryName(composed.ConfigFilePath);
|
string? directory = Path.GetDirectoryName(composed.ConfigFilePath);
|
||||||
|
|
|
||||||
|
|
@ -203,7 +203,7 @@
|
||||||
value is left alone; it is simply no longer offered. -->
|
value is left alone; it is simply no longer offered. -->
|
||||||
<Grid ColumnDefinitions="*,12,*">
|
<Grid ColumnDefinitions="*,12,*">
|
||||||
<StackPanel Spacing="5">
|
<StackPanel Spacing="5">
|
||||||
<TextBlock Text="Plugins (one id per line)" Classes="muted" />
|
<TextBlock Text="Plugins (one id per line — blank loads all, "none" disables)" Classes="muted" />
|
||||||
<TextBox Text="{Binding CharacterPluginsText, Mode=TwoWay}"
|
<TextBox Text="{Binding CharacterPluginsText, Mode=TwoWay}"
|
||||||
AcceptsReturn="True"
|
AcceptsReturn="True"
|
||||||
TextWrapping="Wrap"
|
TextWrapping="Wrap"
|
||||||
|
|
|
||||||
|
|
@ -1,6 +1,39 @@
|
||||||
// src/AcDream.Plugin.Abstractions/IAcDreamPlugin.cs
|
// src/AcDream.Plugin.Abstractions/IAcDreamPlugin.cs
|
||||||
namespace AcDream.Plugin.Abstractions;
|
namespace AcDream.Plugin.Abstractions;
|
||||||
|
|
||||||
|
/// <summary>The plugin contract's version.</summary>
|
||||||
|
/// <remarks>
|
||||||
|
/// <para>
|
||||||
|
/// A plugin declares the version it was built against in its
|
||||||
|
/// <c>plugin.json</c> (<c>apiVersion</c>), and the host refuses to load one
|
||||||
|
/// it cannot honour. Without that check a plugin built against a different
|
||||||
|
/// contract loads anyway and fails later as a MissingMethodException or a
|
||||||
|
/// type-load error from inside the plugin's own code — an error that reads
|
||||||
|
/// like the plugin is broken rather than mismatched.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// Bump <see cref="Current"/> only for a BREAKING change to the interfaces in
|
||||||
|
/// this assembly (a removed or re-shaped member). Purely additive changes keep
|
||||||
|
/// the number, since a plugin built against the older shape still runs.
|
||||||
|
/// </para>
|
||||||
|
/// </remarks>
|
||||||
|
public static class PluginApi
|
||||||
|
{
|
||||||
|
/// <summary>The contract version this build implements.</summary>
|
||||||
|
public const int Current = 1;
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// The oldest contract version this build can still load. Equal to
|
||||||
|
/// <see cref="Current"/> until a breaking change ships with a
|
||||||
|
/// compatibility path.
|
||||||
|
/// </summary>
|
||||||
|
public const int MinimumSupported = 1;
|
||||||
|
|
||||||
|
/// <summary>Whether a plugin declaring <paramref name="apiVersion"/> can load here.</summary>
|
||||||
|
public static bool IsSupported(int apiVersion)
|
||||||
|
=> apiVersion >= MinimumSupported && apiVersion <= Current;
|
||||||
|
}
|
||||||
|
|
||||||
public interface IAcDreamPlugin
|
public interface IAcDreamPlugin
|
||||||
{
|
{
|
||||||
/// <summary>
|
/// <summary>
|
||||||
|
|
|
||||||
|
|
@ -96,6 +96,49 @@ public class PluginLoaderTests
|
||||||
loaded.LoadContext!.Unload();
|
loaded.LoadContext!.Unload();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public void Load_UnsupportedApiVersion_IsRefusedBeforeAnyCodeLoads()
|
||||||
|
{
|
||||||
|
var host = new StubHost();
|
||||||
|
var manifest = new PluginManifest(
|
||||||
|
Id: "future.plugin",
|
||||||
|
DisplayName: "Future",
|
||||||
|
Version: "1.0.0",
|
||||||
|
EntryDll: "nope.dll", // deliberately nonexistent:
|
||||||
|
ApiVersion: PluginApi.Current + 1,
|
||||||
|
Dependencies: Array.Empty<string>());
|
||||||
|
|
||||||
|
var loaded = PluginLoader.Load("/does/not/exist", manifest, host);
|
||||||
|
|
||||||
|
Assert.False(loaded.Success);
|
||||||
|
// ...the version gate must fire FIRST, before the dll is even probed,
|
||||||
|
// so the failure names the real remedy (update the client or the
|
||||||
|
// plugin) instead of a file-not-found or a type-load error from
|
||||||
|
// half-loaded plugin code.
|
||||||
|
var mismatch = Assert.IsType<PluginApiVersionException>(loaded.Error);
|
||||||
|
Assert.Contains("future.plugin", mismatch.Message);
|
||||||
|
Assert.Null(loaded.LoadContext);
|
||||||
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public void Load_MinimumSupportedApiVersion_PassesTheGate()
|
||||||
|
{
|
||||||
|
var host = new StubHost();
|
||||||
|
var manifest = new PluginManifest(
|
||||||
|
Id: "old.plugin",
|
||||||
|
DisplayName: "Old",
|
||||||
|
Version: "1.0.0",
|
||||||
|
EntryDll: "nope.dll",
|
||||||
|
ApiVersion: PluginApi.MinimumSupported,
|
||||||
|
Dependencies: Array.Empty<string>());
|
||||||
|
|
||||||
|
var loaded = PluginLoader.Load("/does/not/exist", manifest, host);
|
||||||
|
|
||||||
|
// Fails on the missing dll, NOT on the version gate.
|
||||||
|
Assert.False(loaded.Success);
|
||||||
|
Assert.IsType<FileNotFoundException>(loaded.Error);
|
||||||
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public void Load_MissingDll_ReturnsFailure()
|
public void Load_MissingDll_ReturnsFailure()
|
||||||
{
|
{
|
||||||
|
|
|
||||||
|
|
@ -174,8 +174,20 @@ public sealed class SessionConfigComposerTests
|
||||||
Assert.Equal("+Acdream", (string?)session["character"]!["name"]);
|
Assert.Equal("+Acdream", (string?)session["character"]!["name"]);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// A character nobody has configured loads every discovered plugin.
|
||||||
|
/// </summary>
|
||||||
|
/// <remarks>
|
||||||
|
/// This REVERSES the original LA5 mapping, deliberately. LA5's
|
||||||
|
/// omitted-vs-empty distinction is kept intact at the session-config
|
||||||
|
/// level; what changed is which one an unconfigured character maps to. A
|
||||||
|
/// new profile starts with an empty list, so a client that ships plugins
|
||||||
|
/// used to load none of them until the user typed an id — and nothing
|
||||||
|
/// reported it, the panel simply never appeared. The opt-out lives on as
|
||||||
|
/// the literal id "none" (see the next test).
|
||||||
|
/// </remarks>
|
||||||
[Fact]
|
[Fact]
|
||||||
public void EmptyPluginsRemainAnExplicitLoadNoneAllowListWhileLoginCommandsAreOmitted()
|
public void UnconfiguredPluginsOmitTheAllowListSoEveryPluginLoads()
|
||||||
{
|
{
|
||||||
CharacterProfile character = Character(LaunchMode.Gui);
|
CharacterProfile character = Character(LaunchMode.Gui);
|
||||||
character.Plugins = [];
|
character.Plugins = [];
|
||||||
|
|
@ -189,10 +201,48 @@ public sealed class SessionConfigComposerTests
|
||||||
Paths,
|
Paths,
|
||||||
sessionId: "session-empty-lists");
|
sessionId: "session-empty-lists");
|
||||||
|
|
||||||
|
JsonObject session = SingleSession(composed);
|
||||||
|
Assert.False(session.ContainsKey("plugins"));
|
||||||
|
Assert.False(session.ContainsKey("loginCommands"));
|
||||||
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public void ThePluginIdNoneEmitsAnExplicitLoadNoneAllowList()
|
||||||
|
{
|
||||||
|
CharacterProfile character = Character(LaunchMode.Gui);
|
||||||
|
character.Plugins = ["none"];
|
||||||
|
|
||||||
|
ComposedSessionConfig composed = SessionConfigComposer.Compose(
|
||||||
|
Server(),
|
||||||
|
Account(),
|
||||||
|
character,
|
||||||
|
Install,
|
||||||
|
Paths,
|
||||||
|
sessionId: "session-no-plugins");
|
||||||
|
|
||||||
JsonObject session = SingleSession(composed);
|
JsonObject session = SingleSession(composed);
|
||||||
Assert.True(session.ContainsKey("plugins"));
|
Assert.True(session.ContainsKey("plugins"));
|
||||||
Assert.Empty(session["plugins"]!.AsArray());
|
Assert.Empty(session["plugins"]!.AsArray());
|
||||||
Assert.False(session.ContainsKey("loginCommands"));
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public void ConfiguredPluginIdsArePassedThroughUnchanged()
|
||||||
|
{
|
||||||
|
CharacterProfile character = Character(LaunchMode.Gui);
|
||||||
|
character.Plugins = ["acdream.mosstank"];
|
||||||
|
|
||||||
|
ComposedSessionConfig composed = SessionConfigComposer.Compose(
|
||||||
|
Server(),
|
||||||
|
Account(),
|
||||||
|
character,
|
||||||
|
Install,
|
||||||
|
Paths,
|
||||||
|
sessionId: "session-one-plugin");
|
||||||
|
|
||||||
|
JsonObject session = SingleSession(composed);
|
||||||
|
Assert.Equal(
|
||||||
|
["acdream.mosstank"],
|
||||||
|
session["plugins"]!.AsArray().Select(node => (string?)node));
|
||||||
}
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
|
|
|
||||||
|
|
@ -65,7 +65,10 @@ internal static class LauncherCoreSessionConfigFixture
|
||||||
Name = "Composer Character",
|
Name = "Composer Character",
|
||||||
Id = "0x50000001",
|
Id = "0x50000001",
|
||||||
LaunchMode = LaunchMode.Headless,
|
LaunchMode = LaunchMode.Headless,
|
||||||
Plugins = [],
|
// "none" is how a profile now spells the explicit load-none
|
||||||
|
// allow-list; an EMPTY list means unconfigured and composes to an
|
||||||
|
// OMITTED allow-list (= load all) since plugins became default-on.
|
||||||
|
Plugins = ["none"],
|
||||||
LoginCommands = [],
|
LoginCommands = [],
|
||||||
};
|
};
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue