Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion src/cli/doctor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -686,7 +686,9 @@ export async function runDoctor(configPath: string): Promise<DoctorReport> {
: 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."
Expand Down
10 changes: 9 additions & 1 deletion src/config/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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, {
Expand Down
6 changes: 2 additions & 4 deletions src/identity/identity-binding-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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)
);
}

Expand Down
4 changes: 3 additions & 1 deletion src/identity/identity-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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 {
Expand Down
12 changes: 12 additions & 0 deletions src/utils/control-characters.ts
Original file line number Diff line number Diff line change
@@ -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;
});
}
24 changes: 24 additions & 0 deletions tests/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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" }],
Expand Down
8 changes: 4 additions & 4 deletions tests/doctor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down Expand Up @@ -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"]) {
Expand Down Expand Up @@ -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 });
Expand Down Expand Up @@ -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 });
});
Expand Down
36 changes: 36 additions & 0 deletions tests/identity-manager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down