feat: Add monitoring, metrics, and security features for multi-client authentication#42
Merged
Patrick-Ehimen merged 1 commit intomainfrom Dec 1, 2025
Merged
Conversation
Reviewer's GuideAdds a full observability and security layer around multi-client API key authentication by introducing metrics collection, structured auth logging, security alerting, and in-memory secure data handling, and wiring these utilities into the auth module exports. Sequence diagram for authentication with metrics, logging, and security alertssequenceDiagram
actor "Client"
participant "AuthService"
participant "MetricsCollector"
participant "AuthLogger"
participant "SecurityAlerter"
"Client"->>"AuthService": "Send authenticated request with API key"
"AuthService"->>"AuthService": "Validate API key and produce AuthenticationResult"
"AuthService"->>"MetricsCollector": "recordAuthentication(AuthenticationResult)"
"MetricsCollector"-->>"AuthService": "Updated metrics and SecurityEvent list"
"AuthService"->>"AuthLogger": "logAuthentication(AuthenticationResult, LogContext)"
"AuthLogger"-->>"AuthService": "Log and audit trail updated"
loop "For each new SecurityEvent from MetricsCollector"
"AuthService"->>"SecurityAlerter": "processSecurityEvent(SecurityEvent)"
"SecurityAlerter"->>"AuthLogger": "logSecurityEvent(SecurityEvent, LogContext)"
"AuthLogger"-->>"SecurityAlerter": "Security log entry recorded"
alt "Event should trigger alert"
"SecurityAlerter"->>"SecurityAlerter": "evaluateEvent(SecurityEvent) and check cooldown"
"SecurityAlerter"->>"ConsoleAlertHandler": "handleAlert(AlertNotification)"
"ConsoleAlertHandler"-->>"SecurityAlerter": "Alert printed to console"
opt "Webhook notifications enabled"
"SecurityAlerter"->>"WebhookAlertHandler": "handleAlert(AlertNotification)"
"WebhookAlertHandler"-->>"SecurityAlerter": "Alert sent via webhook"
end
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- In SensitiveData.clear you hardcode DEFAULT_MEMORY_CONFIG.overwriteIterations instead of using the active MemoryManager configuration, which can lead to surprising behavior when overwriteIterations is customized; consider passing the config into SensitiveData or delegating the overwrite logic back to MemoryManager.
- MemoryManager.createSecureContext relies on mutating global variables by name for cleanup, which is brittle and may either miss actual sensitive data or accidentally clobber unrelated globals; it would be safer to restrict cleanup to explicitly passed objects/fields or return an explicit cleanup function to be called by the caller.
- AuthLogger.sanitizeDetails only redacts sensitiveFields at the top level of the details object, so nested structures can still leak sensitive data; consider making the sanitization recursive over nested objects/arrays to ensure sensitive keys are consistently redacted.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In SensitiveData.clear you hardcode DEFAULT_MEMORY_CONFIG.overwriteIterations instead of using the active MemoryManager configuration, which can lead to surprising behavior when overwriteIterations is customized; consider passing the config into SensitiveData or delegating the overwrite logic back to MemoryManager.
- MemoryManager.createSecureContext relies on mutating global variables by name for cleanup, which is brittle and may either miss actual sensitive data or accidentally clobber unrelated globals; it would be safer to restrict cleanup to explicitly passed objects/fields or return an explicit cleanup function to be called by the caller.
- AuthLogger.sanitizeDetails only redacts sensitiveFields at the top level of the details object, so nested structures can still leak sensitive data; consider making the sanitization recursive over nested objects/arrays to ensure sensitive keys are consistently redacted.
## Individual Comments
### Comment 1
<location> `apps/mcp-server/src/auth/AuthLogger.ts:395-398` </location>
<code_context>
+ /**
+ * Sanitize sensitive information from details
+ */
+ private sanitizeDetails(details: Record<string, unknown>): Record<string, unknown> {
+ const sanitized = { ...details };
+
+ for (const field of this.config.sensitiveFields) {
+ if (sanitized[field]) {
+ sanitized[field] = "[REDACTED]";
</code_context>
<issue_to_address>
**issue (bug_risk):** Sanitization skips falsy values for sensitive fields, which may leave some sensitive data unredacted.
Because the condition uses `if (sanitized[field])`, only truthy values are redacted. Falsy values like `""`, `0`, or `false` in `sensitiveFields` will bypass sanitization and may expose sensitive data. Instead, check for the field’s presence (e.g., `if (field in sanitized)` or using `hasOwnProperty`) and always redact when present, regardless of its value.
</issue_to_address>
### Comment 2
<location> `apps/mcp-server/src/auth/AuthLogger.ts:466-475` </location>
<code_context>
+ /**
+ * Export logs as CSV format
+ */
+ private exportAsCSV(): string {
+ const headers = ["timestamp", "level", "event", "keyHash", "requestId", "toolName", "details"];
+ const rows = [headers.join(",")];
+
+ for (const entry of this.logEntries) {
+ const row = [
+ entry.timestamp,
+ entry.level,
+ entry.event,
+ entry.keyHash,
+ entry.requestId || "",
+ entry.toolName || "",
+ JSON.stringify(entry.sanitizedDetails || entry.details).replace(/"/g, '""'),
+ ];
+ rows.push(row.join(","));
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** CSV export does not properly escape or quote fields, which can produce malformed CSV output.
Currently only `details` is escaped and all fields are joined with commas unquoted. Any field containing commas or newlines (e.g. `toolName`, `event`, `keyHash`) will produce invalid CSV and may break parsers. Please either quote every field and escape internal quotes for all columns, or use a CSV utility to handle this correctly.
</issue_to_address>
### Comment 3
<location> `apps/mcp-server/src/auth/MemoryManager.ts:8-11` </location>
<code_context>
+/**
+ * Memory manager configuration
+ */
+export interface MemoryManagerConfig {
+ enabled: boolean;
+ overwriteIterations: number;
+ clearIntervalMs: number;
+}
+
</code_context>
<issue_to_address>
**suggestion:** clearIntervalMs is defined in the MemoryManagerConfig but not used anywhere.
This unused setting is misleading. Please either integrate `clearIntervalMs` into a periodic cleanup mechanism (e.g., using it to schedule `executeCleanup`) or remove it from the config so it doesn’t suggest behavior that isn’t implemented.
</issue_to_address>
### Comment 4
<location> `apps/mcp-server/src/auth/MemoryManager.ts:71` </location>
<code_context>
const length = this._value.length;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/use-object-destructuring))
```suggestion
const {length} = this._value;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 5
<location> `apps/mcp-server/src/auth/MemoryManager.ts:90` </location>
<code_context>
const value = this.value;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/use-object-destructuring))
```suggestion
const {value} = this;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 6
<location> `apps/mcp-server/src/auth/MemoryManager.ts:156` </location>
<code_context>
const length = str.length;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/use-object-destructuring))
```suggestion
const {length} = str;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 7
<location> `apps/mcp-server/src/auth/MemoryManager.ts:263` </location>
<code_context>
const length = value.length;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/use-object-destructuring))
```suggestion
const {length} = value;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+395
to
+398
| private sanitizeDetails(details: Record<string, unknown>): Record<string, unknown> { | ||
| const sanitized = { ...details }; | ||
|
|
||
| for (const field of this.config.sensitiveFields) { |
There was a problem hiding this comment.
issue (bug_risk): Sanitization skips falsy values for sensitive fields, which may leave some sensitive data unredacted.
Because the condition uses if (sanitized[field]), only truthy values are redacted. Falsy values like "", 0, or false in sensitiveFields will bypass sanitization and may expose sensitive data. Instead, check for the field’s presence (e.g., if (field in sanitized) or using hasOwnProperty) and always redact when present, regardless of its value.
Comment on lines
+466
to
+475
| private exportAsCSV(): string { | ||
| const headers = ["timestamp", "level", "event", "keyHash", "requestId", "toolName", "details"]; | ||
| const rows = [headers.join(",")]; | ||
|
|
||
| for (const entry of this.logEntries) { | ||
| const row = [ | ||
| entry.timestamp, | ||
| entry.level, | ||
| entry.event, | ||
| entry.keyHash, |
There was a problem hiding this comment.
issue (bug_risk): CSV export does not properly escape or quote fields, which can produce malformed CSV output.
Currently only details is escaped and all fields are joined with commas unquoted. Any field containing commas or newlines (e.g. toolName, event, keyHash) will produce invalid CSV and may break parsers. Please either quote every field and escape internal quotes for all columns, or use a CSV utility to handle this correctly.
Comment on lines
+8
to
+11
| export interface MemoryManagerConfig { | ||
| enabled: boolean; | ||
| overwriteIterations: number; | ||
| clearIntervalMs: number; |
There was a problem hiding this comment.
suggestion: clearIntervalMs is defined in the MemoryManagerConfig but not used anywhere.
This unused setting is misleading. Please either integrate clearIntervalMs into a periodic cleanup mechanism (e.g., using it to schedule executeCleanup) or remove it from the config so it doesn’t suggest behavior that isn’t implemented.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR implements comprehensive monitoring, metrics, and security features for the multi-client API key authentication system.
🚀 Features Added
MetricsCollector
AuthLogger
SecurityAlerter
MemoryManager
Summary by Sourcery
Add monitoring and security observability utilities for the authentication subsystem, including logging, metrics, alerting, and secure memory handling.
New Features:
Enhancements: