From b6ed9898262680abad9d4a83811f3291c222fdf1 Mon Sep 17 00:00:00 2001 From: mohammed naji Date: Fri, 18 Sep 2026 01:14:01 +0400 Subject: [PATCH] fix(identity): reject control characters in identity fingerprints An identity fingerprint containing a control character passed configuration validation and passed verification, and was then silently rewritten into a verification failure by the binding store. Three layers disagreed about what a storable identity field is. The configuration schema and the probe parser both accepted control characters, while the durable binding store rejected any code point below 0x20 or equal to 0x7f. A fingerprint that genuinely matched its probe therefore returned "verified" and was immediately downgraded to failed / IDENTITY_BINDING_ UNAVAILABLE, because the store raises its record rejection outside save()'s own error handling, the manager catches it as a bare failure, and the resulting unavailable flag is process-sticky and so downgraded identity verification for every profile in that process. Extract the single shared predicate and use it in all three layers so they cannot drift again. An unstorable fingerprint is now refused at configuration time with its exact path, and unstorable probe evidence can never reach the store. Doctor appends the IDENTITY_* code to its explanation so the cause is recoverable from its output rather than requiring a patched build. Fail-closed behavior on a genuine binding-storage outage is deliberate and is unchanged. Probe tools whose response spans multiple lines remain unusable for identity verification; that limitation is now reported by validate instead of surfacing as an unexplained runtime failure. Refs #451 Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 4 +++ src/cli/doctor.ts | 4 ++- src/config/schema.ts | 10 ++++++- src/identity/identity-binding-store.ts | 6 ++--- src/identity/identity-manager.ts | 4 ++- src/utils/control-characters.ts | 12 +++++++++ tests/config.test.ts | 24 +++++++++++++++++ tests/doctor.test.ts | 8 +++--- tests/identity-manager.test.ts | 36 ++++++++++++++++++++++++++ 9 files changed, 97 insertions(+), 11 deletions(-) create mode 100644 src/utils/control-characters.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 64d99df5..99f121f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file. The format ## [Unreleased] +### Fixed + +- [#451](https://github.com/mohanagy/miftah/issues/451) Stopped a configured identity fingerprint containing a control character from validating, verifying, and then being silently converted into a verification failure. Three layers disagreed about what a storable identity field is: the configuration schema and the probe parser both accepted control characters, while the durable binding store rejected any code point below `0x20` or equal to `0x7f`. A fingerprint that genuinely matched its probe therefore returned `verified` and was then rewritten to `failed` / `IDENTITY_BINDING_UNAVAILABLE`, because the store's record rejection is raised outside `save()`'s own error handling, is caught as a bare failure, and sets a process-sticky unavailable flag that downgraded identity verification for every profile in that process. The single shared predicate is now used by all three layers, so an unstorable fingerprint is refused at configuration time with its exact path and unstorable probe evidence can never reach the store. `doctor` now appends the `IDENTITY_*` code to its explanation, so the cause is recoverable from its output rather than requiring a patched build. Fail-closed behavior on a genuine binding-storage outage is unchanged. Probe tools whose response spans multiple lines remain unusable for identity verification; that limitation is now reported by `miftah validate` instead of surfacing as an unexplained runtime failure. + ## [1.1.5] - 2026-09-11 ### Changed diff --git a/src/cli/doctor.ts b/src/cli/doctor.ts index d1c70280..df9a23c1 100644 --- a/src/cli/doctor.ts +++ b/src/cli/doctor.ts @@ -686,7 +686,9 @@ export async function runDoctor(configPath: string): Promise { : identityCheck( required ? "error" : "warning", targetText, - "Configured upstream identity verification did not complete.", + `Configured upstream identity verification did not complete${ + identity.errorCode === undefined ? "" : ` (${identity.errorCode})` + }.`, identity.status === "unsupported" ? "Upgrade or configure the upstream to expose the expected read-only, no-required-input identity probe. Property access is not account identity evidence." : "Review the configured expected fingerprint and identity probe before relying on risky operations." diff --git a/src/config/schema.ts b/src/config/schema.ts index b117d909..8d1d1fb3 100644 --- a/src/config/schema.ts +++ b/src/config/schema.ts @@ -4,6 +4,7 @@ import { canonicalizeOAuthResource } from "../oauth/canonical-resource.js"; import { parseOAuthConnectionRef, validateOAuthIssuer } from "../oauth/connection-types.js"; import { isSafeOAuthHttpsUrl } from "../oauth/url-safety.js"; import { hasMergedHeader } from "../upstream/headers.js"; +import { containsControlCharacter } from "../utils/control-characters.js"; import { SUPPORTED_CONFIG_VERSIONS } from "./versions.js"; const recordSchema = z.record(z.string(), z.unknown()); @@ -153,7 +154,14 @@ function isLoopbackHostname(hostname: string): boolean { ); } -const identityFieldSchema = z.string().trim().min(1).max(256); +const identityFieldSchema = z + .string() + .trim() + .min(1) + .max(256) + .refine((value) => !containsControlCharacter(value), { + message: "identity fields cannot contain control characters" + }); const opaqueIdentityAccountIdSchema = z .string() .regex(/^[A-Za-z0-9](?:[A-Za-z0-9._:-]{0,255})$/u, { diff --git a/src/identity/identity-binding-store.ts b/src/identity/identity-binding-store.ts index 58583e02..11614750 100644 --- a/src/identity/identity-binding-store.ts +++ b/src/identity/identity-binding-store.ts @@ -3,6 +3,7 @@ import { chmod, mkdir, open, readFile, rename, rm } from "node:fs/promises"; import { homedir, platform } from "node:os"; import { basename, dirname, isAbsolute, join, resolve } from "node:path"; import { OAuthLocalLockUnavailableError, withOAuthLocalLock } from "../oauth/local-lock.js"; +import { containsControlCharacter } from "../utils/control-characters.js"; import { MiftahError } from "../utils/errors.js"; import type { IdentityBindingRecord, IdentityBindingStore } from "./identity-manager.js"; @@ -36,10 +37,7 @@ function boundedIdentifier(value: unknown): value is string { value.length > 0 && value.length <= 256 && value.trim() === value && - ![...value].some((character) => { - const code = character.codePointAt(0); - return code === undefined || code < 0x20 || code === 0x7f; - }) + !containsControlCharacter(value) ); } diff --git a/src/identity/identity-manager.ts b/src/identity/identity-manager.ts index d0ee62de..31b59592 100644 --- a/src/identity/identity-manager.ts +++ b/src/identity/identity-manager.ts @@ -3,6 +3,7 @@ import type { Tool } from "@modelcontextprotocol/server"; import type { IdentityConfig, IdentityFingerprint, MiftahConfig, RiskLevel, ToolingConfig } from "../config/types.js"; import { classifyRisk } from "../policy/risk-classifier.js"; import type { UpstreamRequestOptions, UpstreamSession } from "../upstream/upstream-session.js"; +import { containsControlCharacter } from "../utils/control-characters.js"; import { MiftahError } from "../utils/errors.js"; import type { IdentityProbeCapabilityDiagnostic, IdentityStatus } from "./identity-types.js"; @@ -591,7 +592,8 @@ function matches(expected: IdentityFingerprint, actual: IdentityFingerprint): bo function boundedIdentityField(value: string): string | undefined { const normalized = value.trim(); - return normalized.length > 0 && normalized.length <= maxIdentityFieldLength ? normalized : undefined; + if (normalized.length === 0 || normalized.length > maxIdentityFieldLength) return undefined; + return containsControlCharacter(normalized) ? undefined : normalized; } function boundedFingerprintField(field: keyof IdentityFingerprint, value: string): string | undefined { diff --git a/src/utils/control-characters.ts b/src/utils/control-characters.ts new file mode 100644 index 00000000..7aea259a --- /dev/null +++ b/src/utils/control-characters.ts @@ -0,0 +1,12 @@ +/** + * Identity fingerprints travel from configuration through verification into the durable + * binding store, and every layer must agree on what a storable field looks like. The + * binding store cannot persist C0 control characters or DEL, so they are rejected at the + * configuration boundary rather than surfacing later as an unavailable binding store. + */ +export function containsControlCharacter(value: string): boolean { + return [...value].some((character) => { + const code = character.codePointAt(0); + return code === undefined || code < 0x20 || code === 0x7f; + }); +} diff --git a/tests/config.test.ts b/tests/config.test.ts index ccf92417..4fee20e4 100644 --- a/tests/config.test.ts +++ b/tests/config.test.ts @@ -649,6 +649,30 @@ describe("config foundation", () => { ).toThrow(duplicateIdentityRiskPattern); }); + it.each([ + ["a newline", "mona\nextra"], + ["a tab", "mona\textra"], + ["a delete character", "mona\u007fextra"] + ])("rejects an identity fingerprint containing %s", (_label, login) => { + expect(() => + validateConfig({ + version: "1", + name: "github", + defaultProfile: "work", + upstream: { transport: "stdio", command: "node", args: ["server.js"] }, + profiles: { + work: { + identity: { + expected: { login }, + probe: { tool: "whoami", resultFormat: "text" }, + maxAgeMs: 60_000 + } + } + } + }) + ).toThrow(identityExpectedLoginPattern); + }); + it.each([ ["accountId", { accountId: "google-sub-work" }], ["organization", { organization: "github" }], diff --git a/tests/doctor.test.ts b/tests/doctor.test.ts index 00b0e928..96efe0a2 100644 --- a/tests/doctor.test.ts +++ b/tests/doctor.test.ts @@ -287,7 +287,7 @@ describe("doctor readiness runner", () => { expect(check(report, DOCTOR_CODES.TOOLS_DISCOVERY).status).toBe("pass"); expect(check(report, DOCTOR_CODES.IDENTITY)).toMatchObject({ status: "error", - explanation: "Configured upstream identity verification did not complete." + explanation: "Configured upstream identity verification did not complete (IDENTITY_MISMATCH)." }); expect(report).toMatchObject({ overallStatus: "failed", ok: false }); expect(codes.indexOf(DOCTOR_CODES.TOOLS_DISCOVERY)).toBeLessThan(codes.indexOf(DOCTOR_CODES.IDENTITY)); @@ -327,7 +327,7 @@ describe("doctor readiness runner", () => { expect(check(report, DOCTOR_CODES.TOOLS_DISCOVERY).status).toBe("pass"); expect(check(report, DOCTOR_CODES.IDENTITY)).toMatchObject({ status: "warning", - explanation: "Configured upstream identity verification did not complete." + explanation: "Configured upstream identity verification did not complete (IDENTITY_VERIFICATION_FAILED)." }); expect(report).toMatchObject({ overallStatus: "degraded", ok: true }); for (const sensitiveValue of [configPath, rawIdentityResponse, expectedAccount, "whoami", "miftah_verify_identity"]) { @@ -357,7 +357,7 @@ describe("doctor readiness runner", () => { expect(check(report, DOCTOR_CODES.TOOLS_DISCOVERY).status).toBe("pass"); expect(check(report, DOCTOR_CODES.IDENTITY)).toMatchObject({ status: "warning", - explanation: "Configured upstream identity verification did not complete.", + explanation: "Configured upstream identity verification did not complete (IDENTITY_PROBE_UNSUPPORTED).", remediation: "Upgrade or configure the upstream to expose the expected read-only, no-required-input identity probe. Property access is not account identity evidence." }); expect(report).toMatchObject({ overallStatus: "degraded", ok: true }); @@ -388,7 +388,7 @@ describe("doctor readiness runner", () => { expect(check(report, DOCTOR_CODES.PROMPTS_DISCOVERY).status).toBe("pass"); expect(check(report, DOCTOR_CODES.IDENTITY)).toMatchObject({ status: "warning", - explanation: "Configured upstream identity verification did not complete." + explanation: "Configured upstream identity verification did not complete (IDENTITY_PROBE_UNSUPPORTED)." }); expect(report).toMatchObject({ overallStatus: "degraded", ok: true }); }); diff --git a/tests/identity-manager.test.ts b/tests/identity-manager.test.ts index 3e7f614f..aa7520b7 100644 --- a/tests/identity-manager.test.ts +++ b/tests/identity-manager.test.ts @@ -1117,6 +1117,42 @@ describe("identity verifier", () => { }); }); + it("refuses probe evidence containing control characters instead of binding unstorable evidence", async () => { + const config = validateConfig({ + version: "1", + name: "identity-test", + defaultProfile: "work", + upstream: { transport: "stdio", command: process.execPath, args: [fixture] }, + profiles: { + work: { + env: { TEST_ACCOUNT_NAME: "mona\nextra" }, + identity: { + expected: { login: "mona" }, + probe: { tool: "whoami", resultFormat: "text" }, + maxAgeMs: 60_000 + } + } + } + }); + const upstreams = new UpstreamProcessManager(config.upstream!, config.profiles); + managers.push(upstreams); + let saves = 0; + const verifier = new IdentityManager(config, { + bindingStore: { + load: async () => [], + save: async () => { + saves += 1; + } + } + }); + + const result = await verifier.verify("work", undefined, await upstreams.get("work")); + + expect(result).toMatchObject({ status: "failed", errorCode: "IDENTITY_VERIFICATION_FAILED" }); + expect(saves).toBe(0); + expect(verifier.status("work", undefined)).not.toMatchObject({ errorCode: "IDENTITY_BINDING_UNAVAILABLE" }); + }); + it("reports persisted evidence as expired without using it as a live result", async () => { const config = validateConfig({ version: "1",