Adam
/
AyCode.Core
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity
AyCode.Core/AyCode.Core/docs/LOGGING/LOGGING_ISSUES.md

16 KiB
Raw Blame History

Logger — Known Issues

For planned/actionable work see LOGGING_TODO.md.

ACCORE-LOG-I-K7M2: NopLogWriter ctor signature mismatch (consumer-specific but framework-exposed)

Severity: Minor (caught, non-blocking, but noisy) · Status: Open · Area: Writer-instantiation contract (AcLoggerBase(string) config-reading ctor)

Description

When AcLoggerBase(string? categoryName) (config-reading ctor) runs, it iterates AyCode:Logger:LogWriters[] and calls Activator.CreateInstance(writerType, AppType, LogLevel, categoryName). Some consumer writers (notably a NopCommerce-plugin-level NopLogWriter) have DI-injected ctors instead (e.g. (INopLoggerMsSqlNopDataProvider, CommonSettings, CustomerSettings, IWebHelper, ILogger, string?)), so no ctor matches → MissingMethodException.

Exception is caught in AcLoggerBase try/catch, logged to Console.Error, loop continues with other writers. Non-blocking — protocol construction succeeds — but produces noisy startup logs per Logger instance.

Root cause

Two logger-construction paradigms coexist:

  • Config-reading ctor expects (AppType, LogLevel, string?) writer ctors for Activator convenience
  • DI-injected writers have arbitrary ctor signatures

Known workaround

Console.Error noise tolerated. Alternatively, consumer uses DI-based AddAcLoggerFactory<TLogger> (see LOGGING.md) instead of the config-reading ctor — this path doesn't touch LogWriters[].

Related TODO

LOGGING_TODO.md#accore-log-t-h6y4

ACCORE-LOG-I-R9P3: AcEnv.AppConfiguration is filesystem-bound, MAUI/WASM-unsafe

Severity: Minor · Status: Open · Area: AyCode.Core.Consts.AcEnv

Description

AcEnv.AppConfiguration is a static lazy singleton calling new ConfigurationBuilder().AddJsonFile("appsettings.json").Build() on first access. Reads current-working-directory filesystem. Throws on MAUI (no physical appsettings next to exe) and WASM (no filesystem at all).

Root cause

Design predates IOptions/DI pattern.

Known workaround

Consumer avoids the config-reading AcLoggerBase(string) ctor on these platforms. DI-based AddAcLoggerFactory<TLogger> + services.Configure<AcLoggerOptions>(...) is the replacement (see LOGGING.md).

Related TODO

LOGGING_TODO.md#accore-log-t-n2d8

ACCORE-LOG-I-L4N8: Two parallel logger-setup patterns

Severity: Minor (confusion, not functional) · Status: Open · Area: LOGGING.md / consumer code

Description

Two ways to construct a logger coexist:

  1. Config-reading: new Logger(categoryName) → AcLoggerBase reads AyCode:Logger section via AcEnv.AppConfiguration, instantiates writers via Activator.CreateInstance
  2. DI factory: services.Configure<AcLoggerOptions>(...) + services.AddAcLoggerFactory<TLogger>() → Func<string, TLogger> resolved from DI, writers pulled from DI

Consumer picks per scenario; no automatic bridge. Risk: mixing patterns causes subtle failures (e.g. MissingMethodException — see ACCORE-LOG-I-K7M2).

Related TODO

LOGGING_TODO.md#accore-log-t-r7l3

ACCORE-LOG-I-B2H5: Default LogLevel diverges across the two setup paths

Severity: Minor (surprise, not broken) · Status: Open · Area: AcLoggerBase field initializer vs AcLoggerOptions

Description

The two logger-setup paths (see ACCORE-LOG-I-L4N8) ship with different default LogLevels:

  • AcLoggerBase.cs:18 — field initializer: LogLevel = Error
  • AcLoggerOptions.cs:27 — DI options default: LogLevel = Info

Consequences:

  • Config-reading ctor WITHOUT an AyCode:Logger:LogLevel entry → silent Error minimum (loses Info/Warning in prod)
  • DI factory WITHOUT services.Configure<AcLoggerOptions>(...) → Info minimum

Same consumer code, same "no configuration" state, two different log volumes depending on which path was chosen.

Root cause

Historical: field initializer predates the Options class; Options was added as part of the DI-factory refactor with a developer-friendly Info default.

Related TODO

LOGGING_TODO.md#accore-log-t-j9g2

ACCORE-LOG-I-X7Q1: AcConsoleLogWriter.Initialize() runs twice on parameterless ctor

Severity: Minor (wasted work, not broken) · Status: Open · Area: AcConsoleLogWriter ctor chain

Description

AcConsoleLogWriter.cs:14-17:

protected AcConsoleLogWriter() : this(null)   // chains to (string?) ctor
{
    Initialize();                              // ← second call
}

protected AcConsoleLogWriter(string? categoryName = null) : base(categoryName)
{
    Initialize();                              // ← first call (via chain)
}

Chain : this(null) invokes the (string?) ctor, which calls Initialize(). Control returns to the parameterless ctor body and calls Initialize() again. Net effect: Console.ForegroundColor = ConsoleColor.White set twice — harmless but reveals a copy-paste oversight.

Fix direction

Remove the second Initialize() call in the parameterless ctor body (the chain already covered it).

Related TODO

LOGGING_TODO.md#accore-log-t-b8k5

ACCORE-LOG-I-V3J6: ILogger.IsEnabled(MsLogLevel.None) incorrectly reports enabled

Severity: Low (semantic bug, rare path) · Status: Open · Area: AcLoggerBase.IsEnabled + MapFromMsLogLevel

Description

AcLoggerBase.cs:184-188:

public bool IsEnabled(MsLogLevel logLevel)
{
    var acLogLevel = MapFromMsLogLevel(logLevel);
    return LogLevel <= acLogLevel;
}

MapFromMsLogLevel(MsLogLevel.None) → AcLogLevel.Disabled (byte 255). For any instance LogLevel (max Error=25), LogLevel <= 255 is always true → IsEnabled(None) returns true despite None meaning "off".

The subsequent Log<TState> switch has case MsLogLevel.None: default: break; so no actual write occurs, BUT:

  • The MS-logging framework may query IsEnabled as a gate before calling Log — hosts using that pattern get misleading "enabled" signals
  • Any caller that inspects IsEnabled(None) (diagnostics, test assertions) gets the wrong answer

Fix direction

Either:

  • (a) Explicit if (logLevel == MsLogLevel.None) return false; at the top of IsEnabled
  • (b) Change the comparison to strict < with Disabled as sentinel (larger refactor — affects semantic of Disabled elsewhere)

Related TODO

LOGGING_TODO.md#accore-log-t-x1v4

ACCORE-LOG-I-T8F2: Misleading inline comment in AcLoggerBase.Log<TState>

Severity: Trivial (doc-only) · Status: Open · Area: AcLoggerBase.cs:210-211

Description

// Use eventId.Name as memberName if available, otherwise null (will show as empty, not "Log")
var memberName = !string.IsNullOrEmpty(eventId.Name) ? $"{eventId.Name}:{eventId.Id}" : "Log";

Comment says fallback is null (empty display), the code assigns "Log". Contradicts both the doc in LOGGING.md#CallerMemberName Auto-Capture and the actual behaviour observed in logs.

Fix direction

Update comment to match code: // Use eventId.Name:eventId.Id if Name is set, otherwise fallback to "Log" per LOGGING.md convention.

Related TODO

Folded into LOGGING_TODO.md#accore-log-t-m7p2 (cleanup batch).

ACCORE-LOG-I-M4C9: Server-side NopCommerce plugin still uses legacy config-reading Logger ctor

Severity: Minor (works, but inconsistent with modern pattern + triggers ACCORE-LOG-I-K7M2 noise) · Status: Open · Area: Consumer adoption gap in Nop.Plugin.Misc.AIPlugin/Infrastructure/PluginNopStartup.cs

Description

Client side (FruitBankHybridApp.* — Web, Web.Client, MAUI) was migrated to the DI-factory pattern: services.Configure<AcLoggerOptions>(...) + services.AddAcLoggerFactory<Logger>(). The server-side plugin was NOT migrated — it still:

  1. Directly constructs a logger via new Logger(nameof(AyCodeBinaryHubProtocol)) (PluginNopStartup.cs:169) and passes it as opts.Logger to the protocol — bypasses ILogger<AcBinaryHubProtocol> DI resolution that AcSignalRProtocolExtensions.BuildProtocol already implements.
  2. Does NOT call services.Configure<AcLoggerOptions>(configuration.GetSection("AyCode:Logger")).
  3. Does NOT call services.AddAcLoggerFactory<Logger>().
  4. Writer registration exists (services.AddScoped<IAcLogWriterBase, ConsoleLogWriter>() + NopLogWriter) but those DI-registered singletons are NOT the instances the new Logger(...) ctor sees — the legacy ctor creates a parallel set via Activator.CreateInstance.

The legacy config-reading ctor DOES find the appsettings AyCode:Logger section via AcEnv.AppConfiguration (filesystem-backed, works on server) — so logging functions. But every new Logger(...) call:

  • Triggers ACCORE-LOG-I-K7M2 (NopLogWriter ctor mismatch → Console.Error noise)
  • Reconstructs writer instances via Activator (not singleton-shared with DI-registered writers)
  • Is inconsistent with the client-side pattern → two mental models for the same framework

Fix direction

See LOGGING_TODO.md#accore-log-t-w4h9.

Related

  • ACCORE-LOG-I-K7M2 (trigger — NopLogWriter ctor mismatch, currently causing Console.Error noise)
  • ACCORE-LOG-I-L4N8 (root cause — two coexisting setup patterns)
  • ACCORE-LOG-I-B2H5 (consequence — different defaults between paths; server legacy path silently ships with Error default unless AyCode:Logger:LogLevel is set)
  • Sibling gap: ../SIGNALR/SIGNALR_ISSUES.md#accore-sig-i-b5g9 (same server-side plugin, protocol-options adoption gap)
  • Plugin doc drift: Nop.Plugin.Misc.AIPlugin/docs/SIGNALR/README.md:22 still documents the pre-migration new AcBinaryHubProtocol() registration (actual code uses .AddAcBinaryProtocol(opts => {...})). Update needed.

ACCORE-LOG-I-P5W3: JWT signing key written to log (CRITICAL security)

Severity: 🛑 Critical (security) · Status: Closed (2026-04-25) · Area: AyCode.Services.Server/Logins/AcLoginServiceServer.cs:192

Description

GenerateAccessToken writes the JWT signing key to the logger via GlobalLogger.Detail:

GlobalLogger.Detail($"Key: {configuration["JWT:Key"]}");

The signing key is the cryptographic secret used to sign all JWT access tokens. Anyone with read access to the logs (file system, log aggregators, retention archives, operator dashboards, screenshots during incident response) can:

  1. Forge arbitrary tokens for any user
  2. Impersonate any role / claim
  3. Bypass authentication entirely

Log storage is typically at a lower trust level than dedicated secret stores. This exposure path is a complete authentication bypass.

Fix direction

Remove the GlobalLogger.Detail($"Key: ...") line entirely. The signing key must NEVER appear in any log output. If presence verification is needed for diagnostics, log only a hash prefix (e.g. Key configured: SHA256[0..8] = abc12345).

Resolution

What: Wrapped the GlobalLogger.Detail($"Key: ...") call in #if DEBUG / #endif with a // SECURITY: comment block explaining the risk. Line preserved (not deleted) — user opted for DEBUG-gating over removal so local auth-debug retains the diagnostic.

Where: AyCode.Services.Server/Logins/AcLoginServiceServer.cs:192 (line numbers shifted slightly due to the inserted comment).

Why: User-directed deviation from the originally proposed "remove entirely" fix direction (bearer-token ADR planning session, 2026-04-25). DEBUG-gating ensures production (-c Release) builds strip the line at compile time, while preserving dev-time diagnostics.

Caveat: DEBUG builds still write the key to log. CI/CD must build with -c Release for production deployments. The inline // SECURITY: comment in the code warns future maintainers.

Related

  • Superseded by: ACCORE-AUTH-I-B3X5 — entry relocated to AUTH topic per ADR-0001 "Status migration on AUTH topic creation" follow-up. AUTH is now the canonical home for current discussion; this entry preserved per append-only governance (TOPIC_CODES.md Status conventions).
  • Reference: ADR 0001-user-bearer-token-flow.md — formal architectural decision; this entry's Resolution is the pre-flight fix referenced in the ADR's Consequences.
  • ACCORE-LOG-I-K1Z7 (sibling — access token also leaked, same file, same code path)
  • LOGGING/README.md may need a new "Never log secrets" guideline section (separate _TODO.md candidate)
  • Discovery context: this issue and ACCORE-LOG-I-K1Z7 emerged during the user-bearer-token ADR context-gathering phase (2026-04-25 Copilot session, while reading AcLoginServiceServer.cs). If a dedicated AUTH topic-folder is created later (likely as part of the bearer token ADR follow-ups), these two entries are candidates for relocation to AUTH_ISSUES.md with Status: SUPERSEDED by AUTH-I-N cross-references.

ACCORE-LOG-I-K1Z7: JWT access token written to log (CRITICAL security)

Severity: 🛑 Critical (security) · Status: Closed (2026-04-25) · Area: AyCode.Services.Server/Logins/AcLoginServiceServer.cs:212

Description

After GenerateAccessToken produces a token, the token text itself is written to the logger:

GlobalLogger.Detail($"AccesToken: {writtenToken}");

(Side note: AccesToken is a typo for AccessToken — fix while at the line.)

Access tokens are bearer credentials — anyone holding the token can authenticate as the user until token expiry (currently 6h per GenerateAccessToken config). Logged tokens leak via the same channels as ACCORE-LOG-I-P5W3 (live streams, retention archives, screenshots, shared logs during ops).

Fix direction

Remove the GlobalLogger.Detail($"AccesToken: ...") line entirely. If issuance verification is needed, log only metadata — user ID, expiry timestamp, issuer — never the token itself.

Resolution

What: Two changes:

  1. Wrapped the GlobalLogger.Detail($"AccessToken: ...") call in #if DEBUG / #endif with a // SECURITY: comment block.
  2. Fixed the AccesToken → AccessToken typo (flagged in original Description as a side-note) in the log-message string literal.

Where: AyCode.Services.Server/Logins/AcLoginServiceServer.cs:212 (line numbers shifted slightly due to the inserted comment).

Why: Same as ACCORE-LOG-I-P5W3 — user opted for DEBUG-gating over deletion (2026-04-25, bearer-token ADR session). Typo fix is opportunistic since the line was being touched.

Caveat: Same DEBUG-build leak constraint as ACCORE-LOG-I-P5W3. CI/CD must build with -c Release for production.

Related

  • Superseded by: ACCORE-AUTH-I-T8N2 — entry relocated to AUTH topic per ADR-0001 "Status migration on AUTH topic creation" follow-up. AUTH is now the canonical home for current discussion; this entry preserved per append-only governance (TOPIC_CODES.md Status conventions).
  • Reference: ADR 0001-user-bearer-token-flow.md — formal architectural decision; this entry's Resolution is the pre-flight fix referenced in the ADR's Consequences.
  • ACCORE-LOG-I-P5W3 (sibling — JWT signing key also leaked, same file)
  • Same LOGGING/README.md "Never log secrets" guideline gap as ACCORE-LOG-I-P5W3
  • Same discovery-context note as ACCORE-LOG-I-P5W3 (bearer token ADR session, candidate for AUTH topic relocation if/when that emerges)

Evaluated review findings — NOT bugs (by-design)

Cross-session review (2026-04) flagged several items that are intentional defense-in-depth / YAGNI and should NOT be "fixed":

  • "Triple LogLevel gate on the ILogger.Log path is redundant" — NOT redundant. Info()/Debug() can be called directly (not only via ILogger.Log), so each named method MUST own its gate. Per-writer gate is the documented Gate-2 (see LOGGING.md "Two independent level gates"). Removing any gate breaks a real path.
  • "AcTextLogWriterBase.GetDiagnosticText defensive level-check is redundant" — same rationale; direct callers exist.
  • "IAcLoggerBase narrow interface (no dynamic LogLevel setter via interface)" — YAGNI. Consumers needing dynamic level can cast to AcLoggerBase. Widening the interface for a rare scenario pollutes the abstraction.
  • "protected List<IAcLogWriterBase> LogWriters is mutable" — acceptable; GetWriters returns a defensive copy ([.. LogWriters]), direct mutation is only reachable from derived classes (which control their own ctor-time population).