Skip to content

extra layer of security#15

Merged
j4ys0n merged 2 commits into
mainfrom
tools-fix
Apr 21, 2026
Merged

extra layer of security#15
j4ys0n merged 2 commits into
mainfrom
tools-fix

Conversation

@j4ys0n
Copy link
Copy Markdown
Contributor

@j4ys0n j4ys0n commented Apr 21, 2026

No description provided.

@j4ys0n
Copy link
Copy Markdown
Contributor Author

j4ys0n commented Apr 21, 2026

Automated review 🤖

Summary of Changes
This PR introduces a hydration endpoint integration (/core/workflow-runs/{id}/hydrated) to retrieve full chat transcript data for completed workflow runs, enabling extraction of assistant message content and token usage beyond what’s available in the base WorkflowRunRecord. It adds retry logic to wait for final hydration readiness after a workflow completes, and implements defense-in-depth credential masking on sensitive fields (apiKey, api_key, token, accessToken, secret) across config summaries. Version bumped to 0.4.2.

Key Changes & Positives

  • Added WorkflowRunHydratedRecordSchema and parsing logic to access full chat messages and token usage, improving result fidelity 🟢
  • Implemented waitForCompletedWorkflowReadiness with exponential backoff (250ms × 8 attempts) to handle delayed hydration post-completion, reducing race conditions 🟢
  • Introduced redactSecrets and maskCredential functions to sanitize credential-like fields before returning data to AI agents, addressing incident 2026-04-19 concerns 🟢
  • Updated msq_get_workflow_run_result to prioritize hydrated content over fallback previewContent, ensuring accurate assistant responses 🟢

Potential Issues & Recommendations

  1. Issue / Risk: extractLatestAssistantMessage uses slice().reverse() which creates a shallow copy; for large message arrays (>100), this may impact performance under high load
    • Impact: Latency spikes during hydration extraction for long-running workflows
    • Recommendation: Replace with findLastIndex (ES2023) or iterate backwards manually to avoid allocation
    • Status: 🟡 Needs review
  2. Issue / Risk: waitForCompletedWorkflowReadiness throws a generic error after 8 retries without exposing attempt count or last hydrated state
    • Impact: Debugging failures becomes difficult; callers cannot distinguish timeout from transient API issues
    • Recommendation: Include maxAttempts, lastAttemptHydratedState, and delayMs in the error message
    • Status: 🟡 Needs review
  3. Issue / Risk: maskCredential truncates strings ≤8 chars to "***"; short but valid secrets (e.g., "abc123") become indistinguishable from empty values
    • Impact: May obscure legitimate short tokens during auditing or debugging
    • Recommendation: Consider configurable minimum length or preserve prefix/suffix length dynamically (e.g., show first/last 2 chars for ≥6-char strings)
    • Status: 🟡 Needs review

Language/Framework Checks

  • z.union([z.string(), z.null(), z.array(z.unknown())]) in WorkflowRunMessageRecordSchema.content is overly permissive; consider narrowing to z.string().nullable() unless array content (e.g., tool calls) is expected in messages
  • encodePathSegment(runId) used consistently for URL safety, but no validation ensures runId is non-empty before encoding — add runtime guard in fetchWorkflowRunHydratedRecord
  • isRecord utility assumed imported/defined elsewhere; confirm presence in src/utils.ts or similar

Security & Privacy

  • Credential masking applied to models, embeddingModels, and embeddedCollections in summarizeCoreConfig, preventing accidental leakage of API keys/secrets via config dumps 🟢
  • Redaction occurs after schema parsing but before serialization, ensuring no unmasked data escapes to MCP clients — aligns with defense-in-depth principle 🟢
  • However, helperChats in WorkflowRunHydratedRecordSchema are not redacted in mapWorkflowRunResult; ensure downstream callers apply redactSecrets or extend masking to nested chats

Build/CI & Ops

  • No changes to build tooling or dependencies; version bump only affects package metadata
  • New sleep utility and retry loop increase runtime complexity but remain bounded (≤2s total wait); monitor for timeout spikes in CI test suites

Tests

  • Add unit tests for maskCredential edge cases (empty string, 8-char secrets, special characters)
  • Test waitForCompletedWorkflowReadiness failure path (e.g., mock API returning incomplete hydration 8 times)
  • Verify msq_get_workflow_run_result falls back to previewContent when hydration is unavailable or malformed

Approval Recommendation
Approve with caveats

  • Address extractLatestAssistantMessage performance concern
  • Enhance error message in waitForCompletedWorkflowReadiness
  • Clarify content union type expectations in WorkflowRunMessageRecordSchema
  • Extend redactSecrets to helperChats or document intentional exclusion

@j4ys0n
Copy link
Copy Markdown
Contributor Author

j4ys0n commented Apr 21, 2026

Automated review 🤖

Summary of Changes
This PR introduces a hydration endpoint integration (/core/workflow-runs/{id}/hydrated) to fetch complete assistant message content for workflow runs, replacing reliance on preview-only fields in the base WorkflowRunRecord. It adds retry logic to wait for final assistant content to become available post-completion and implements defense-in-depth credential masking across summarized config data (models, embedding models, embedded collections) to prevent accidental exposure of API keys/tokens to AI agents. Version bumped to 0.4.2.

Key Changes & Positives

  • Added WorkflowRunHydratedRecordSchema and supporting types to parse detailed chat messages with token usage ✅
  • Implemented fetchWorkflowRunHydratedRecord, extractLatestAssistantMessage, and extractHydratedMainResult to reliably retrieve authoritative assistant output ✅
  • Added retry loop (waitForCompletedWorkflowReadiness) with exponential backoff (250ms × 8 attempts) to handle eventual consistency of hydrated results ✅
  • Introduced redactSecrets and maskCredential functions to sanitize credential-shaped fields (apiKey, token, etc.) before returning config summaries ✅
  • Updated msq_get_workflow_result and msq_get_workflow_run_status to prefer hydrated content over preview content, with fallback ✅

Potential Issues & Recommendations

  1. Issue / Risk: The waitForCompletedWorkflowReadiness function throws an error after 8 retries if hydrated content remains unavailable despite status === 'completed', but the error message suggests re-calling msq_get_workflow_run_status, which may cause infinite loops if the backend remains inconsistent.
    Impact: Clients may enter retry loops or fail with unclear guidance.
    Recommendation: Add a maximum total wait timeout (e.g., 2s) and return a structured error indicating "hydration delayed" rather than throwing.
    Status: 🟡 Needs review

  2. Issue / Risk: The sleep function is implemented inline in tools.ts, but waitForCompletedWorkflowReadiness uses a fixed delay without jitter, risking thundering herd under high concurrency.
    Impact: Could increase load on upstream API during recovery windows.
    Recommendation: Introduce random jitter (±20%) to delayMs or use exponential backoff with jitter.
    Status: 🟡 Needs review

  3. Issue / Risk: Credential masking (maskCredential) truncates strings ≤8 chars to "***", which may obscure legitimate short tokens (e.g., AB123456) and break debugging.
    Impact: Legitimate short secrets may be indistinguishable from redacted ones in logs.
    Recommendation: Increase threshold to ≥12 chars or log redaction events for auditability.
    Status: 🟡 Needs review

Language/Framework Checks

  • Zod schemas (WorkflowRunMessageRecordSchema, WorkflowChatHydrationSchema, etc.) correctly use .passthrough() to preserve unknown fields ✅
  • Type inference (z.infer) applied consistently for new hydrated record types ✅
  • encodePathSegment used for runId in hydrated endpoint path ✅
  • isRecord guard used before mutating objects in redactSecrets

Security & Privacy

  • Defense-in-depth redaction of credential fields (apiKey, api_key, token, accessToken, secret) in summarizeCoreConfig output prevents leakage even if upstream API regresses (citing incident 2026-04-19) ✅
  • Masking preserves minimal prefix/suffix (e.g., eyJ...kuj4) for traceability while hiding full values ✅

Tests

  • New test redacts apiKey/token-shaped fields across all summarized sub-trees validates masking for long keys and JWTs ✅
  • Tests added for hydrated fallback paths: missing mainChat, incomplete assistant message, and retry behavior ✅
  • Tests verify both msq_get_workflow_result and msq_get_workflow_run_status now call /hydrated endpoint ✅

Approval Recommendation
Approve with caveats

  • Address retry loop risk in waitForCompletedWorkflowReadiness (add timeout or clearer error)
  • Consider adding jitter to sleep delay to avoid thundering herd
  • Document credential masking thresholds and behavior in code comments

@j4ys0n j4ys0n merged commit ad241ba into main Apr 21, 2026
1 check passed
@j4ys0n j4ys0n deleted the tools-fix branch April 21, 2026 03:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant