diff --git a/CHANGELOG.md b/CHANGELOG.md index 64d99df..99f121f 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 d1c7028..df9a23c 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 b117d90..8d1d1fb 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 58583e0..1161475 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 d0ee62d..31b5959 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 0000000..7aea259 --- /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 ccf9241..4fee20e 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 00b0e92..96efe0a 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 3e7f614..aa7520b 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",