fix(SEC-33,SEC-34): address code review — missed docs, key-id guard comment, test consolidation
Same-commit docs rule (were missed in the prior commit): - docs/GalaxyRepository.md: SnapshotCachePath now documents the per-OS derived default and the GalaxyRepositoryOptionsValidator rooting/validity enforcement. - A2-galaxyrepository-adoption-handoff.md: correct the now-inaccurate NSSM caveat (SnapshotCachePath override is optional, not required; blank seeds a rooted host default, no silent no-op) and repoint the option-validation item at the new GalaxyRepositoryOptionsValidator. SEC-34 guard confirmed and documented: TryParseKeyId's '_' split cannot truncate a key id because both — and the only — gateway key-creation paths (ApiKeyAdminCommandLineParser.IsValidKeyId, DashboardApiKeyManagementService.ValidateKeyId) restrict key ids to IsAsciiLetterOrDigit || '.' || '-', and key ids are never library-generated. Added a citing comment; no behavior change. Test consolidation: moved the three host-start SqlitePath overrides into TestHostEnvironmentInitializer (per-process temp store, mirroring Secrets__SqlitePath) so future host-start tests auto-cover.
This commit is contained in:
@@ -205,6 +205,14 @@ public sealed class CachingApiKeyVerifier : IApiKeyVerifier, IApiKeyCacheInvalid
|
||||
// Parses the key id out of a "Bearer mxgw_<keyId>_<secret>" header without any store access —
|
||||
// the same split the authorization interceptor does. Returns null for a header this cache cannot
|
||||
// attribute to a key id (in which case the generation race-guard is simply not applied).
|
||||
//
|
||||
// Correctness of the SEC-34 generation guard rests on parts[1] being the FULL key id: the '_'
|
||||
// split would truncate a key id that itself contained '_', silently disarming the guard for that
|
||||
// key. This is safe because '_' is the token's field delimiter and both — and the only — key
|
||||
// creation paths in the gateway forbid it: ApiKeyAdminCommandLineParser.IsValidKeyId and
|
||||
// DashboardApiKeyManagementService.ValidateKeyId each restrict a key id to
|
||||
// char.IsAsciiLetterOrDigit || '.' || '-'. Key ids are never library-generated, so no path can
|
||||
// mint one containing '_'.
|
||||
private static string? TryParseKeyId(string? authorizationHeader)
|
||||
{
|
||||
if (string.IsNullOrEmpty(authorizationHeader))
|
||||
|
||||
@@ -9,7 +9,6 @@ using Microsoft.Extensions.Options;
|
||||
using ZB.MOM.WW.MxGateway.Server;
|
||||
using ZB.MOM.WW.MxGateway.Server.Dashboard;
|
||||
using ZB.MOM.WW.MxGateway.Server.Metrics;
|
||||
using ZB.MOM.WW.MxGateway.Tests.Security.Authentication;
|
||||
|
||||
namespace ZB.MOM.WW.MxGateway.Tests.Gateway;
|
||||
|
||||
@@ -81,13 +80,9 @@ public sealed class GatewayApplicationTests
|
||||
public async Task Build_MapsMetricsEndpoint()
|
||||
{
|
||||
// Bind an ephemeral port (:0) — xUnit runs test collections in parallel, so any
|
||||
// started-host test must avoid a fixed port to prevent a bind collision. Starting the host
|
||||
// eagerly opens the auth SQLite store; the shipped config no longer carries a SqlitePath, so
|
||||
// override it to a writable temp path (the code default resolves under an unwritable
|
||||
// /usr/share on macOS). See SEC-33.
|
||||
using TempDatabaseDirectory authDir = TempDatabaseDirectory.Create(nameof(GatewayApplicationTests));
|
||||
await using WebApplication app = GatewayApplication.Build(
|
||||
["--urls=http://127.0.0.1:0", $"--MxGateway:Authentication:SqlitePath={authDir.DatabasePath()}"]);
|
||||
// started-host test must avoid a fixed port to prevent a bind collision. The auth SQLite
|
||||
// store path is isolated to a per-process temp file by TestHostEnvironmentInitializer (SEC-33).
|
||||
await using WebApplication app = GatewayApplication.Build(["--urls=http://127.0.0.1:0"]);
|
||||
await app.StartAsync();
|
||||
try
|
||||
{
|
||||
@@ -264,13 +259,11 @@ public sealed class GatewayApplicationTests
|
||||
string expectedFailure)
|
||||
{
|
||||
// Bind an ephemeral port (:0) — xUnit runs test collections in parallel, so any
|
||||
// WebApplication-building test must avoid a fixed port to prevent a bind collision. Override
|
||||
// the auth SqlitePath to a writable temp path: startup opens the store before the injected
|
||||
// misconfiguration is validated on some paths, and the code-default path is unwritable on
|
||||
// macOS (SEC-33).
|
||||
using TempDatabaseDirectory authDir = TempDatabaseDirectory.Create(nameof(GatewayApplicationTests));
|
||||
// WebApplication-building test must avoid a fixed port to prevent a bind collision. The auth
|
||||
// store path is isolated by TestHostEnvironmentInitializer (SEC-33), so startup opens a
|
||||
// writable store and the injected misconfiguration is what fails validation.
|
||||
await using WebApplication app = GatewayApplication.Build(
|
||||
[$"--{key}={value}", "--urls=http://127.0.0.1:0", $"--MxGateway:Authentication:SqlitePath={authDir.DatabasePath()}"]);
|
||||
[$"--{key}={value}", "--urls=http://127.0.0.1:0"]);
|
||||
|
||||
OptionsValidationException exception = await Assert.ThrowsAsync<OptionsValidationException>(
|
||||
() => app.StartAsync());
|
||||
|
||||
@@ -35,11 +35,6 @@ public sealed class GatewayTlsBootstrapTests
|
||||
Environment.SetEnvironmentVariable("Kestrel__Endpoints__Test__Url", "https://127.0.0.1:0");
|
||||
Environment.SetEnvironmentVariable(
|
||||
"MxGateway__Tls__SelfSignedCertPath", Path.Combine(certDir, "gw.pfx"));
|
||||
// Starting the host opens the auth SQLite store; the shipped config no longer ships a
|
||||
// SqlitePath and the code default is unwritable on macOS (/usr/share), so pin it to the
|
||||
// writable temp dir. See SEC-33.
|
||||
Environment.SetEnvironmentVariable(
|
||||
"MxGateway__Authentication__SqlitePath", Path.Combine(certDir, "gateway-auth.db"));
|
||||
|
||||
WebApplication app = GatewayApplication.Build([]);
|
||||
await app.StartAsync();
|
||||
@@ -58,8 +53,6 @@ public sealed class GatewayTlsBootstrapTests
|
||||
{
|
||||
Environment.SetEnvironmentVariable("Kestrel__Endpoints__Test__Url", null);
|
||||
Environment.SetEnvironmentVariable("MxGateway__Tls__SelfSignedCertPath", null);
|
||||
Environment.SetEnvironmentVariable("MxGateway__Authentication__SqlitePath", null);
|
||||
Microsoft.Data.Sqlite.SqliteConnection.ClearAllPools();
|
||||
Directory.Delete(certDir, recursive: true);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -84,5 +84,19 @@ internal static class TestHostEnvironmentInitializer
|
||||
"secrets.db");
|
||||
Environment.SetEnvironmentVariable("Secrets__SqlitePath", secretsPath);
|
||||
}
|
||||
|
||||
// Starting the full host eagerly opens the auth SQLite store. Since SEC-33 the shipped
|
||||
// appsettings.json no longer carries an Authentication:SqlitePath, and the CommonApplicationData
|
||||
// code default resolves under an unwritable /usr/share on macOS. Point every host-building test at
|
||||
// a per-process temp store (same pattern as Secrets__SqlitePath above) so host-start tests are
|
||||
// auto-covered without a per-test override; a test that needs its own store still overrides this.
|
||||
if (string.IsNullOrEmpty(Environment.GetEnvironmentVariable("MxGateway__Authentication__SqlitePath")))
|
||||
{
|
||||
string authPath = Path.Combine(
|
||||
Path.GetTempPath(),
|
||||
$"mxgw-tests-{Environment.ProcessId}",
|
||||
"gateway-auth.db");
|
||||
Environment.SetEnvironmentVariable("MxGateway__Authentication__SqlitePath", authPath);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user