From ead5a9444ae36048c773d1a0bccaf7f5ffa96d72 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 20 Oct 2026 03:54:27 +0000 Subject: [PATCH 1/2] fix: add redacted URI diagnostics to /healthz The response shows length, whitespace, quotes, the @ count, the host, the username, and the password length. It does not show the password or the full URI. The encode hint is added only when the driver reports a parse error. Co-authored-by: Jared Wray --- site/docs/bins.md | 19 ++++- site/docs/how-to-deploy/cloudflare.md | 4 +- src/bin-store-diagnostics.ts | 105 +++++++++++++++++++++++--- src/routes/health.ts | 33 +++++++- test/bin-store-diagnostics.test.ts | 104 +++++++++++++++++++++++++ test/bin-store-ping.test.ts | 50 +++++++++++- test/worker-runtime.test.ts | 44 +++++++++-- test/worker.test.ts | 35 +++++++++ worker/app.ts | 1 + worker/bin-store.ts | 3 + 10 files changed, 376 insertions(+), 22 deletions(-) diff --git a/site/docs/bins.md b/site/docs/bins.md index b6a1a0f..a6d739e 100644 --- a/site/docs/bins.md +++ b/site/docs/bins.md @@ -87,6 +87,19 @@ A failed ping sends HTTP 503: "connected": false, "uriPresent": true, "scheme": "mongodb+srv", + "uri": { + "length": 78, + "leadingWhitespace": false, + "trailingWhitespace": false, + "newline": false, + "wrappedQuotes": false, + "atCount": 2, + "host": "cluster0.example.invalid", + "username": "user", + "passwordLength": 8, + "passwordReserved": ["@"], + "passwordHasPercent": false + }, "error": { "name": "MongoParseError", "message": "Invalid connection string. Percent-encode reserved characters in the username and password (@ : / ? # [ ]).", @@ -96,9 +109,11 @@ A failed ping sends HTTP 503: } ``` -The response does not contain the connection string, the username, the password, or the host. The logs do not contain them. `uriPresent` is a boolean. `scheme` is `mongodb`, `mongodb+srv`, or `null`. +A MongoDB result includes `bins.uri` on success and on failure. A memory result omits `bins.uri`. `uri.host` is the text after the last `@` and before the first `/` or `?`. `uri.username` is the username. `uri.passwordLength` is the password length. -`Invalid connection string` means that the URI has a second `@`. It also means that you did not encode a reserved character in the username or the password. Encode these characters: +`uri.passwordReserved` lists raw reserved characters in the password. `uri.passwordHasPercent` is true when the password contains `%`. The response does not contain the connection string or the password. The logs do not contain the connection string, the username, the password, or the host. `uriPresent` is a boolean. `scheme` is `mongodb`, `mongodb+srv`, or `null`. + +`Invalid connection string` is the driver text. MockHTTP adds the percent-encode hint only for that text or for `unescaped characters`. A DNS error stays a DNS error. `Invalid connection string` means that the URI has a second `@`. It also means that you did not encode a reserved character in the username or the password. Encode these characters: - `@` as `%40` - `:` as `%3A` diff --git a/site/docs/how-to-deploy/cloudflare.md b/site/docs/how-to-deploy/cloudflare.md index fb3268c..159cfe3 100644 --- a/site/docs/how-to-deploy/cloudflare.md +++ b/site/docs/how-to-deploy/cloudflare.md @@ -29,7 +29,9 @@ Bins stay in memory in the isolate when `MONGODB_URI` is not set. A bin from one Percent-encode reserved characters in the username and the password. `@` is `%40`. An unencoded `@` makes the driver throw `Invalid connection string` before it contacts Atlas. -`GET /healthz` reports the bin store. A successful admin ping returns HTTP 200 and `bins.connected: true`. A failed ping returns HTTP 503 with the driver error name, a redacted message, and a code. The body has `uriPresent` (boolean) and `scheme` (`mongodb` or `mongodb+srv`). The body does not have the URI, the username, the password, or the host. On the Node server, this route reports the memory store. Connection failures are also one JSON line in the Worker logs (`mockhttp.bin-store`). Those logs use the same redaction. +`GET /healthz` reports the bin store. A successful admin ping returns HTTP 200 and `bins.connected: true`. A failed ping returns HTTP 503 with the driver error name, a redacted message, and a code. The body has `uriPresent` (boolean) and `scheme` (`mongodb` or `mongodb+srv`). On the Node server, this route reports the memory store. + +When `MONGODB_URI` is set, `bins.uri` has the URI length, leading or trailing whitespace, a newline, quote marks, the `@` count, the host, the username, and the password length. The host is the text after the last `@` and before the first `/` or `?`. The body does not have the password or the full URI. The percent-encode hint is added only when the driver message is `Invalid connection string` or `unescaped characters`. A DNS error keeps the driver text. Worker logs still omit the URI, the username, the password, and the host. Taps are a library API (`mock.taps.inject`). They have no HTTP routes, so a deployed Worker has no way to inject one. diff --git a/src/bin-store-diagnostics.ts b/src/bin-store-diagnostics.ts index 081a2aa..0f126e6 100644 --- a/src/bin-store-diagnostics.ts +++ b/src/bin-store-diagnostics.ts @@ -1,8 +1,9 @@ /** * Diagnostics for the bin store. - * Logs and HTTP bodies can show the error name and a safe message. - * They can show if the URI is present. - * They cannot show the URI, the username, the password, or the host. + * Logs can show the error name, a safe message, and URI shape flags. + * Logs cannot show the URI, the username, the password, or the host. + * `GET /healthz` can show the host, the username, and the password length. + * It cannot show the password or the full URI. */ export type BinStoreScheme = "mongodb" | "mongodb+srv"; @@ -22,6 +23,7 @@ export type BinHealth = latencyMs: number; uriPresent: boolean; scheme: BinStoreScheme | null; + uri?: UriDiagnostics; }; } | { @@ -31,6 +33,7 @@ export type BinHealth = connected: false; uriPresent: boolean; scheme: BinStoreScheme | null; + uri?: UriDiagnostics; error: BinStoreErrorInfo; }; }; @@ -41,6 +44,23 @@ export type UriShape = { extraAt: boolean; }; +/** Redacted URI facts for `GET /healthz`. No password and no full URI. */ +export type UriDiagnostics = { + length: number; + leadingWhitespace: boolean; + trailingWhitespace: boolean; + newline: boolean; + wrappedQuotes: boolean; + atCount: number; + host: string | null; + username: string | null; + passwordLength: number | null; + passwordReserved: string[]; + passwordHasPercent: boolean; +}; + +const PASSWORD_RESERVED = ["@", ":", "/", "?", "#", "[", "]"] as const; + const ENCODING_HINT = "Percent-encode reserved characters in the username and password (@ : / ? # [ ])."; @@ -98,6 +118,67 @@ export function uriShape(uri: string): UriShape { }; } +function hasWrappedQuotes(uri: string): boolean { + const trimmed = uri.trim(); + return ( + trimmed.startsWith('"') || + trimmed.startsWith("'") || + trimmed.endsWith('"') || + trimmed.endsWith("'") + ); +} + +function userinfoBeforeLastAt(uri: string): string | null { + const at = uri.lastIndexOf("@"); + if (at < 0) { + return null; + } + const before = uri.slice(0, at).replace(/^['"\s]+/, ""); + const scheme = before.match(/^[a-z][a-z0-9+.-]*:\/\//i); + return scheme ? before.slice(scheme[0].length) : before; +} + +/** + * Host is the text after the last `@` and before the first `/` or `?`. + * Username is the text before the first `:` in the userinfo. + * The password is not returned. + */ +export function uriDiagnostics(uri: string): UriDiagnostics { + const at = uri.lastIndexOf("@"); + const after = at < 0 ? "" : uri.slice(at + 1); + const cut = after.search(/[/?]/); + const hostText = cut < 0 ? after : after.slice(0, cut); + const userinfo = userinfoBeforeLastAt(uri); + let username: string | null = null; + let password: string | null = null; + if (userinfo !== null) { + const colon = userinfo.indexOf(":"); + if (colon < 0) { + username = userinfo.length > 0 ? userinfo : null; + } else { + username = userinfo.slice(0, colon) || null; + password = userinfo.slice(colon + 1); + } + } + const secret = password; + return { + length: uri.length, + leadingWhitespace: /^\s/.test(uri), + trailingWhitespace: /\s$/.test(uri), + newline: /[\r\n]/.test(uri), + wrappedQuotes: hasWrappedQuotes(uri), + atCount: uri.split("@").length - 1, + host: hostText.length > 0 ? hostText : null, + username, + passwordLength: secret === null ? null : secret.length, + passwordReserved: + secret === null + ? [] + : PASSWORD_RESERVED.filter((char) => secret.includes(char)), + passwordHasPercent: secret?.includes("%") === true, + }; +} + export function isBinStoreConnectionFailure(error: unknown): boolean { if (!(error instanceof Error)) { return false; @@ -174,13 +255,14 @@ export function redactSecrets(message: string, uri?: string): string { return out; } -function withEncodingHint( - message: string, - shape: UriShape | undefined, -): string { - const needsHint = - shape?.extraAt === true || - /invalid connection string|unescaped characters/i.test(message); +/** + * Add the encode hint only when the driver message is already a parse error. + * A DNS error or a scheme error keeps its own text. + */ +function withEncodingHint(message: string): string { + const needsHint = /invalid connection string|unescaped characters/i.test( + message, + ); if (!needsHint || message.includes("Percent-encode")) { return message; } @@ -205,10 +287,9 @@ export function redactStoreError( typeof codeValue === "string" || typeof codeValue === "number" ? codeValue : null; - const shape = uri ? uriShape(uri) : undefined; return { name, - message: withEncodingHint(redactSecrets(raw, uri), shape), + message: withEncodingHint(redactSecrets(raw, uri)), code, }; } diff --git a/src/routes/health.ts b/src/routes/health.ts index ecf69c0..d753e7a 100644 --- a/src/routes/health.ts +++ b/src/routes/health.ts @@ -37,17 +37,48 @@ export async function checkMemoryBinStore(keyv: { } } +const uriDiagnosticsSchema = { + type: "object", + properties: { + length: { type: "integer" }, + leadingWhitespace: { type: "boolean" }, + trailingWhitespace: { type: "boolean" }, + newline: { type: "boolean" }, + wrappedQuotes: { type: "boolean" }, + atCount: { type: "integer" }, + host: { type: ["string", "null"] }, + username: { type: ["string", "null"] }, + passwordLength: { type: ["integer", "null"] }, + passwordReserved: { type: "array", items: { type: "string" } }, + passwordHasPercent: { type: "boolean" }, + }, + required: [ + "length", + "leadingWhitespace", + "trailingWhitespace", + "newline", + "wrappedQuotes", + "atCount", + "host", + "username", + "passwordLength", + "passwordReserved", + "passwordHasPercent", + ], +}; + const binHealthFields = { store: { type: "string", enum: ["mongo", "memory"] }, connected: { type: "boolean" }, latencyMs: { type: "integer" }, uriPresent: { type: "boolean" }, scheme: { type: ["string", "null"], enum: ["mongodb", "mongodb+srv", null] }, + uri: uriDiagnosticsSchema, }; const healthSchema: FastifySchema = { description: - "Report whether bin storage is reachable. MongoDB failures include the driver error with credentials removed. The URI, username, password, and host are never returned.", + "Report whether bin storage is reachable. MongoDB failures include the driver error. The password and the full URI are never returned. A MongoDB result can include the host, the username, the URI length, and the password length.", tags: ["Health"], response: { 200: { diff --git a/test/bin-store-diagnostics.test.ts b/test/bin-store-diagnostics.test.ts index e2df04a..1ea0a2d 100644 --- a/test/bin-store-diagnostics.test.ts +++ b/test/bin-store-diagnostics.test.ts @@ -7,6 +7,7 @@ import { redactSecrets, redactStoreError, toSafeStoreError, + uriDiagnostics, uriShape, } from "../src/bin-store-diagnostics.js"; @@ -101,6 +102,80 @@ describe("bin store diagnostics", () => { ).not.toContain("onlyuser"); }); + it("reports redacted URI diagnostics and hides the password", () => { + const clean = "mongodb://user:pass@cluster.example.invalid:27017/db"; + expect(uriDiagnostics(clean)).toEqual({ + length: clean.length, + leadingWhitespace: false, + trailingWhitespace: false, + newline: false, + wrappedQuotes: false, + atCount: 1, + host: "cluster.example.invalid:27017", + username: "user", + passwordLength: 4, + passwordReserved: [], + passwordHasPercent: false, + }); + + const dirty = ` "mongodb+srv://user:p@ss:/?#[]%40rd@cluster0.example.invalid/mockhttp"\n`; + const report = uriDiagnostics(dirty); + expect(report.length).toBe(dirty.length); + expect(report.leadingWhitespace).toBe(true); + expect(report.trailingWhitespace).toBe(true); + expect(report.newline).toBe(true); + expect(report.wrappedQuotes).toBe(true); + expect(report.atCount).toBe(2); + expect(report.host).toBe("cluster0.example.invalid"); + expect(report.username).toBe("user"); + expect(report.passwordLength).toBe("p@ss:/?#[]%40rd".length); + expect(report.passwordReserved).toEqual([ + "@", + ":", + "/", + "?", + "#", + "[", + "]", + ]); + expect(report.passwordHasPercent).toBe(true); + expect(JSON.stringify(report)).not.toContain("p@ss"); + expect(JSON.stringify(report)).not.toContain("%40rd"); + + expect(uriDiagnostics("mongodb://127.0.0.1:1/mockhttp")).toMatchObject({ + atCount: 0, + host: null, + username: null, + passwordLength: null, + passwordReserved: [], + passwordHasPercent: false, + }); + expect( + uriDiagnostics("mongodb://onlyuser@cluster.example.invalid/db"), + ).toMatchObject({ + username: "onlyuser", + passwordLength: null, + host: "cluster.example.invalid", + }); + expect(uriDiagnostics("not-a-uri@example.invalid/db")).toMatchObject({ + username: "not-a-uri", + host: "example.invalid", + passwordLength: null, + }); + expect(uriDiagnostics("mongodb://user:pass@")).toMatchObject({ + host: null, + username: "user", + passwordLength: 4, + }); + const emptyUser = uriDiagnostics("mongodb://:secret@example.invalid/db"); + expect(emptyUser).toMatchObject({ + username: null, + passwordLength: 6, + host: "example.invalid", + }); + expect(JSON.stringify(emptyUser)).not.toContain("secret"); + }); + it("adds a percent-encoding hint for parser failures", () => { const info = redactStoreError( Object.assign(new Error("Invalid connection string"), { @@ -130,6 +205,35 @@ describe("bin store diagnostics", () => { expect(redactStoreError(numeric).code).toBe(50); const weird = Object.assign(new Error("timeout"), { code: { n: 1 } }); expect(redactStoreError(weird).code).toBeNull(); + + const dns = redactStoreError( + Object.assign( + new Error( + "querySrv ENOTFOUND _mongodb._tcp.cluster0.abcde.mongodb.net", + ), + { name: "MongoServerSelectionError", code: "ENOTFOUND" }, + ), + uri, + ); + expect(dns.name).toBe("MongoServerSelectionError"); + expect(dns.code).toBe("ENOTFOUND"); + expect(dns.message).toContain("querySrv"); + expect(dns.message).not.toContain("Invalid connection string"); + expect(dns.message).not.toContain("Percent-encode"); + expect(dns.message).not.toContain("cluster0"); + + const scheme = redactStoreError( + Object.assign( + new Error( + 'Invalid scheme, expected connection string to start with "mongodb://" or "mongodb+srv://"', + ), + { name: "MongoParseError" }, + ), + " mongodb://user:pass@host/db", + ); + expect(scheme.name).toBe("MongoParseError"); + expect(scheme.message).not.toContain("Percent-encode"); + expect(scheme.message).not.toContain("Invalid connection string"); }); it("marks safe store errors and logs each phase once", () => { diff --git a/test/bin-store-ping.test.ts b/test/bin-store-ping.test.ts index 8457cb5..3bd0312 100644 --- a/test/bin-store-ping.test.ts +++ b/test/bin-store-ping.test.ts @@ -38,8 +38,23 @@ describe("mongo bin store ping", () => { } const body = JSON.stringify(report); expect(body).not.toContain("p@ssw"); - expect(body).not.toContain("cluster0"); expect(body).not.toContain(uri); + if (report.status === "error") { + expect(report.bins.uri).toEqual({ + length: uri.length, + leadingWhitespace: false, + trailingWhitespace: false, + newline: false, + wrappedQuotes: false, + atCount: 2, + host: "cluster0.example.invalid", + username: "user", + passwordLength: 8, + passwordReserved: ["@"], + passwordHasPercent: false, + }); + expect(report.bins.error.message).not.toContain("cluster0"); + } expect(errorLog.mock.calls.join("\n")).not.toContain("p@ssw"); expect(errorLog.mock.calls.join("\n")).not.toContain("cluster0"); }); @@ -68,7 +83,11 @@ describe("mongo bin store ping", () => { }); if (report.status === "ok") { expect(report.bins.latencyMs).toEqual(expect.any(Number)); + expect(report.bins.uri?.host).toBe("cluster0.example.invalid:27017"); + expect(report.bins.uri?.username).toBe("user"); + expect(report.bins.uri?.passwordLength).toBe(4); } + expect(JSON.stringify(report)).not.toContain("user:pass"); expect(errorLog.mock.calls.join("\n")).toContain("healthz.close"); expect(errorLog.mock.calls.join("\n")).not.toContain("cluster0"); expect(errorLog.mock.calls.join("\n")).not.toContain("user:pass"); @@ -96,8 +115,37 @@ describe("mongo bin store ping", () => { if (report.status === "error") { expect(report.bins.error.code).toBe("ENOTFOUND"); expect(report.bins.error.message).toContain("querySrv"); + expect(report.bins.error.message).not.toContain( + "Invalid connection string", + ); + expect(report.bins.error.message).not.toContain("Percent-encode"); expect(report.bins.error.message).not.toContain("cluster0"); + expect(report.bins.uri?.host).toBe("cluster0.example.invalid"); + expect(report.bins.uri?.passwordHasPercent).toBe(false); + } + }); + + it("passes the URI to MongoClient without trimming or extra characters", async () => { + const uri = "mongodb://user:pass@cluster.example.invalid/mockhttp\n"; + let seen: string | undefined; + vi.spyOn(MongoClient.prototype, "db").mockImplementation(function ( + this: MongoClient, + ) { + seen = (this as unknown as { s: { url: string } }).s.url; + return { + command: vi.fn().mockResolvedValue({ ok: 1 }), + } as never; + }); + vi.spyOn(MongoClient.prototype, "close").mockResolvedValue(); + const report = await binStoreRuntime.ping(uri); + expect(seen).toBe(uri); + expect(report.status).toBe("ok"); + if (report.status === "ok") { + expect(report.bins.uri?.newline).toBe(true); + expect(report.bins.uri?.trailingWhitespace).toBe(true); + expect(report.bins.uri?.length).toBe(uri.length); } + expect(JSON.stringify(report)).not.toContain("user:pass"); }); it("still throws the first socket error that has no listener", () => { diff --git a/test/worker-runtime.test.ts b/test/worker-runtime.test.ts index 27574b9..e95a371 100644 --- a/test/worker-runtime.test.ts +++ b/test/worker-runtime.test.ts @@ -153,10 +153,27 @@ describe("worker srv and unencoded password under workerd", () => { const health = await worker.fetch("https://mockhttp.org/healthz"); const body = await health.text(); expect(health.status, body).toBe(503); + const report = JSON.parse(body) as { + bins: { + error: { message: string }; + uri: { + host: string; + username: string; + passwordLength: number; + atCount: number; + }; + }; + }; expect(body).toContain('"scheme":"mongodb+srv"'); - expect(body).not.toContain("Invalid connection string"); + expect(report.bins.error.message).not.toContain( + "Invalid connection string", + ); + expect(report.bins.error.message).not.toContain("Percent-encode"); + expect(report.bins.uri.host).toBe("cluster0.example.invalid"); + expect(report.bins.uri.username).toBe("user"); + expect(report.bins.uri.passwordLength).toBe(4); + expect(report.bins.uri.atCount).toBe(1); expect(body).not.toContain("user:pass"); - expect(body).not.toContain("cluster0"); expect(body).not.toContain("mongodb+srv://"); } finally { await worker.stop(); @@ -177,10 +194,27 @@ describe("worker srv and unencoded password under workerd", () => { const health = await worker.fetch("https://mockhttp.org/healthz"); const body = await health.text(); expect(health.status, body).toBe(503); - expect(body).toContain("Invalid connection string"); - expect(body).toContain("Percent-encode"); + const report = JSON.parse(body) as { + bins: { + error: { name: string; message: string }; + uri: { + host: string; + username: string; + passwordLength: number; + passwordReserved: string[]; + atCount: number; + }; + }; + }; + expect(report.bins.error.name).toBe("MongoParseError"); + expect(report.bins.error.message).toContain("Invalid connection string"); + expect(report.bins.error.message).toContain("Percent-encode"); + expect(report.bins.uri.atCount).toBe(2); + expect(report.bins.uri.host).toBe("cluster0.example.invalid"); + expect(report.bins.uri.username).toBe("user"); + expect(report.bins.uri.passwordLength).toBe(8); + expect(report.bins.uri.passwordReserved).toEqual(["@"]); expect(body).not.toContain("p@ssw"); - expect(body).not.toContain("cluster0"); expect(body).not.toContain("mongodb+srv://"); } finally { await worker.stop(); diff --git a/test/worker.test.ts b/test/worker.test.ts index 803e1ad..3f63d3e 100644 --- a/test/worker.test.ts +++ b/test/worker.test.ts @@ -488,6 +488,41 @@ describe("worker Fastify app", () => { delete process.env.MONGODB_URI; }); + it("passes MONGODB_URI through without trimming or joining text", async () => { + const uri = "mongodb://user:pass@cluster.example.invalid/mockhttp\n"; + delete process.env.MONGODB_URI; + process.env.MONGODB_URI = "mongodb://from-process-env"; + expect(workerMongoUri({ MONGODB_URI: uri })).toBe(uri); + expect(workerMongoUri({ MONGODB_URI: "" })).toBe( + "mongodb://from-process-env", + ); + delete process.env.MONGODB_URI; + + const ping = vi.spyOn(binStoreRuntime, "ping").mockResolvedValue({ + status: "ok", + bins: { + store: "mongo", + connected: true, + latencyMs: 1, + uriPresent: true, + scheme: "mongodb", + }, + }); + const mockHttp = await createWorkerApp({ MONGODB_URI: uri }); + const health = await mockHttp.server.inject({ + method: "GET", + url: "/healthz", + }); + expect(health.statusCode).toBe(200); + expect(ping).toHaveBeenCalledWith(uri); + await mockHttp.close(); + ping.mockRestore(); + + workerMongoState.uri = "mongodb://other"; + applyWorkerMongoEnv({ MONGODB_URI: uri }); + expect(workerMongoState.uri).toBe(uri); + }); + it("reads the Atlas URI from process.env when the binding is unset", async () => { const uri = "mongodb+srv://user:pass@cluster.mongodb.net/mockhttp"; process.env.MONGODB_URI = uri; diff --git a/worker/app.ts b/worker/app.ts index 19b5fd9..b483b52 100644 --- a/worker/app.ts +++ b/worker/app.ts @@ -38,6 +38,7 @@ export type WorkerEnv = { /** * Atlas URI for this request. The Worker binding wins. An empty string is * treated as unset so a blank secret does not open a client. + * The returned text is the env value. It is not trimmed, and no characters are added. */ export function workerMongoUri(env: WorkerEnv = {}): string | undefined { const uri = env.MONGODB_URI || process.env.MONGODB_URI; diff --git a/worker/bin-store.ts b/worker/bin-store.ts index 6a519f8..cb87233 100644 --- a/worker/bin-store.ts +++ b/worker/bin-store.ts @@ -9,6 +9,7 @@ import { mongoUriFacts, redactStoreError, toSafeStoreError, + uriDiagnostics, uriShape, } from "../src/bin-store-diagnostics.js"; import type { MockHttpBinsOptions } from "../src/mock-http-bins.js"; @@ -277,6 +278,7 @@ export async function pingMongoBinStore(uri: string): Promise { latencyMs: Date.now() - started, uriPresent: facts.present, scheme: facts.scheme, + uri: uriDiagnostics(uri), }, }; } catch (error) { @@ -288,6 +290,7 @@ export async function pingMongoBinStore(uri: string): Promise { connected: false, uriPresent: facts.present, scheme: facts.scheme, + uri: uriDiagnostics(uri), error: redactStoreError(error, uri), }, }; From 8c268068338bf82c33f0af7d17c1c41cee643170 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 20 Oct 2026 04:06:45 +0000 Subject: [PATCH 2/2] fix: correct URI diagnostics from the review The host stops before the query string. $ is a reserved password character. A password of only reserved characters does not publish its length. Co-authored-by: Jared Wray --- site/docs/bins.md | 7 +- site/docs/how-to-deploy/cloudflare.md | 4 +- src/bin-store-diagnostics.ts | 95 ++++++++++++++++++++------- test/bin-store-diagnostics.test.ts | 36 ++++++++++ test/mock-http-bins.test.ts | 2 +- 5 files changed, 117 insertions(+), 27 deletions(-) diff --git a/site/docs/bins.md b/site/docs/bins.md index a6d739e..5289e86 100644 --- a/site/docs/bins.md +++ b/site/docs/bins.md @@ -102,16 +102,16 @@ A failed ping sends HTTP 503: }, "error": { "name": "MongoParseError", - "message": "Invalid connection string. Percent-encode reserved characters in the username and password (@ : / ? # [ ]).", + "message": "Invalid connection string. Percent-encode reserved characters in the username and password (@ : / ? # [ ] $).", "code": null } } } ``` -A MongoDB result includes `bins.uri` on success and on failure. A memory result omits `bins.uri`. `uri.host` is the text after the last `@` and before the first `/` or `?`. `uri.username` is the username. `uri.passwordLength` is the password length. +A MongoDB result includes `bins.uri` on success and on failure. A memory result omits `bins.uri`. `uri.host` is the text after the last `@` before the first `/` or `?`. An `@` in the query string does not change the host. `uri.username` is the username. `uri.passwordLength` is the password length. -`uri.passwordReserved` lists raw reserved characters in the password. `uri.passwordHasPercent` is true when the password contains `%`. The response does not contain the connection string or the password. The logs do not contain the connection string, the username, the password, or the host. `uriPresent` is a boolean. `scheme` is `mongodb`, `mongodb+srv`, or `null`. +`uri.passwordReserved` lists raw reserved characters in the password. `uri.passwordLength` is null when the password has only reserved characters. `uri.passwordHasPercent` is true when the password contains `%`. The response does not contain the connection string or the password. The logs do not contain the connection string, the username, the password, or the host. `uriPresent` is a boolean, and `scheme` is `mongodb`, `mongodb+srv`, or `null`. `Invalid connection string` is the driver text. MockHTTP adds the percent-encode hint only for that text or for `unescaped characters`. A DNS error stays a DNS error. `Invalid connection string` means that the URI has a second `@`. It also means that you did not encode a reserved character in the username or the password. Encode these characters: @@ -122,6 +122,7 @@ A MongoDB result includes `bins.uri` on success and on failure. A memory result - `#` as `%23` - `[` as `%5B` - `]` as `%5D` +- `$` as `%24` A correct `mongodb+srv://` URI does not fail at parse time. A DNS failure or a network failure has a different error name. diff --git a/site/docs/how-to-deploy/cloudflare.md b/site/docs/how-to-deploy/cloudflare.md index 159cfe3..621488d 100644 --- a/site/docs/how-to-deploy/cloudflare.md +++ b/site/docs/how-to-deploy/cloudflare.md @@ -31,7 +31,9 @@ Percent-encode reserved characters in the username and the password. `@` is `%40 `GET /healthz` reports the bin store. A successful admin ping returns HTTP 200 and `bins.connected: true`. A failed ping returns HTTP 503 with the driver error name, a redacted message, and a code. The body has `uriPresent` (boolean) and `scheme` (`mongodb` or `mongodb+srv`). On the Node server, this route reports the memory store. -When `MONGODB_URI` is set, `bins.uri` has the URI length, leading or trailing whitespace, a newline, quote marks, the `@` count, the host, the username, and the password length. The host is the text after the last `@` and before the first `/` or `?`. The body does not have the password or the full URI. The percent-encode hint is added only when the driver message is `Invalid connection string` or `unescaped characters`. A DNS error keeps the driver text. Worker logs still omit the URI, the username, the password, and the host. +When `MONGODB_URI` is set, `bins.uri` has the URI length and the `@` count. It also has flags for whitespace, a newline, and quote marks. It has the host, the username, and the password length. The host is the text after the last `@` before `/` or `?`. An `@` in the query string does not change the host. The body does not have the password or the full URI. + +The percent-encode hint is added only for `Invalid connection string` or `unescaped characters`. A DNS error keeps the driver text. `uri.passwordLength` is null when the password has only reserved characters. Worker logs still omit the URI, the username, the password, and the host. Taps are a library API (`mock.taps.inject`). They have no HTTP routes, so a deployed Worker has no way to inject one. diff --git a/src/bin-store-diagnostics.ts b/src/bin-store-diagnostics.ts index 0f126e6..3059ae7 100644 --- a/src/bin-store-diagnostics.ts +++ b/src/bin-store-diagnostics.ts @@ -59,10 +59,10 @@ export type UriDiagnostics = { passwordHasPercent: boolean; }; -const PASSWORD_RESERVED = ["@", ":", "/", "?", "#", "[", "]"] as const; +const PASSWORD_RESERVED = ["@", ":", "/", "?", "#", "[", "]", "$"] as const; const ENCODING_HINT = - "Percent-encode reserved characters in the username and password (@ : / ? # [ ])."; + "Percent-encode reserved characters in the username and password (@ : / ? # [ ] $)."; const loggedPhases = new WeakMap>(); @@ -128,39 +128,90 @@ function hasWrappedQuotes(uri: string): boolean { ); } -function userinfoBeforeLastAt(uri: string): string | null { - const at = uri.lastIndexOf("@"); +function looksLikeHost(value: string): boolean { + if (value === "localhost" || /^localhost:\d+$/.test(value)) { + return true; + } + if (/^\d{1,3}(?:\.\d{1,3}){3}(?::\d+)?$/.test(value)) { + return true; + } + if (/^\[[0-9a-fA-F:.]+\](?::\d+)?$/.test(value)) { + return true; + } + const name = value.replace(/:\d+$/, ""); + return name.includes(".") && /^[A-Za-z0-9._-]+$/.test(name); +} + +/** + * Keep the authority section. Stop at the `/` or `?` that follows the host. + * An `@` in the query string or the path is outside that section. + */ +function authorityBody(body: string): string { + const firstAt = body.indexOf("@"); + if (firstAt < 0) { + return body; + } + for (let i = firstAt + 1; i < body.length; i++) { + const mark = body[i]; + if (mark !== "/" && mark !== "?") { + continue; + } + const at = body.lastIndexOf("@", i - 1); + const between = body.slice(at + 1, i); + if (looksLikeHost(between)) { + return body.slice(0, i); + } + } + return body; +} + +function userinfoAndHost(uri: string): { + userinfo: string | null; + host: string | null; +} { + const cleaned = uri.trim().replace(/^['"]+|['"]+$/g, ""); + const scheme = cleaned.match(/^[a-z][a-z0-9+.-]*:\/\//i); + const body = scheme ? cleaned.slice(scheme[0].length) : cleaned; + const authority = authorityBody(body); + const at = authority.lastIndexOf("@"); if (at < 0) { - return null; + return { userinfo: null, host: null }; } - const before = uri.slice(0, at).replace(/^['"\s]+/, ""); - const scheme = before.match(/^[a-z][a-z0-9+.-]*:\/\//i); - return scheme ? before.slice(scheme[0].length) : before; + const host = authority.slice(at + 1); + return { + userinfo: authority.slice(0, at), + host: host.length > 0 ? host : null, + }; } /** - * Host is the text after the last `@` and before the first `/` or `?`. + * Host is the text after the last `@` in the authority section. + * The search stops at the first `/` or `?` after that host. * Username is the text before the first `:` in the userinfo. * The password is not returned. + * When every password character is reserved, the length and the list stay empty. + * Those two values rebuild the password. */ export function uriDiagnostics(uri: string): UriDiagnostics { - const at = uri.lastIndexOf("@"); - const after = at < 0 ? "" : uri.slice(at + 1); - const cut = after.search(/[/?]/); - const hostText = cut < 0 ? after : after.slice(0, cut); - const userinfo = userinfoBeforeLastAt(uri); + const parts = userinfoAndHost(uri); let username: string | null = null; let password: string | null = null; - if (userinfo !== null) { - const colon = userinfo.indexOf(":"); + if (parts.userinfo !== null) { + const colon = parts.userinfo.indexOf(":"); if (colon < 0) { - username = userinfo.length > 0 ? userinfo : null; + username = parts.userinfo.length > 0 ? parts.userinfo : null; } else { - username = userinfo.slice(0, colon) || null; - password = userinfo.slice(colon + 1); + username = parts.userinfo.slice(0, colon) || null; + password = parts.userinfo.slice(colon + 1); } } const secret = password; + const onlyReserved = + secret !== null && + secret.length > 0 && + [...secret].every((char) => + (PASSWORD_RESERVED as readonly string[]).includes(char), + ); return { length: uri.length, leadingWhitespace: /^\s/.test(uri), @@ -168,11 +219,11 @@ export function uriDiagnostics(uri: string): UriDiagnostics { newline: /[\r\n]/.test(uri), wrappedQuotes: hasWrappedQuotes(uri), atCount: uri.split("@").length - 1, - host: hostText.length > 0 ? hostText : null, + host: parts.host, username, - passwordLength: secret === null ? null : secret.length, + passwordLength: secret === null || onlyReserved ? null : secret.length, passwordReserved: - secret === null + secret === null || onlyReserved ? [] : PASSWORD_RESERVED.filter((char) => secret.includes(char)), passwordHasPercent: secret?.includes("%") === true, diff --git a/test/bin-store-diagnostics.test.ts b/test/bin-store-diagnostics.test.ts index 1ea0a2d..cc87e64 100644 --- a/test/bin-store-diagnostics.test.ts +++ b/test/bin-store-diagnostics.test.ts @@ -174,6 +174,42 @@ describe("bin store diagnostics", () => { host: "example.invalid", }); expect(JSON.stringify(emptyUser)).not.toContain("secret"); + + const query = uriDiagnostics( + "mongodb://user:pass@cluster.example.invalid/db?appName=ops@example.com", + ); + expect(query.host).toBe("cluster.example.invalid"); + expect(query.username).toBe("user"); + expect(query.passwordLength).toBe(4); + expect(query.passwordReserved).toEqual([]); + expect(query.atCount).toBe(2); + expect(JSON.stringify(query)).not.toContain("ops@example.com"); + + expect( + uriDiagnostics("mongodb://user:pa$ss@cluster.example.invalid/db") + .passwordReserved, + ).toEqual(["$"]); + + expect( + uriDiagnostics("mongodb://user:pass@localhost/db?appName=ops@example.com") + .host, + ).toBe("localhost"); + expect( + uriDiagnostics( + "mongodb://user:pass@127.0.0.1:27017/db?appName=ops@example.com", + ).host, + ).toBe("127.0.0.1:27017"); + expect( + uriDiagnostics("mongodb://user:pass@[::1]/db?appName=ops@example.com") + .host, + ).toBe("[::1]"); + + const marks = uriDiagnostics("mongodb://user:@@@@@cluster.example.invalid"); + expect(marks.atCount).toBe(5); + expect(marks.host).toBe("cluster.example.invalid"); + expect(marks.passwordLength).toBeNull(); + expect(marks.passwordReserved).toEqual([]); + expect(JSON.stringify(marks)).not.toContain("@@@@"); }); it("adds a percent-encoding hint for parser failures", () => { diff --git a/test/mock-http-bins.test.ts b/test/mock-http-bins.test.ts index 5a52eb5..7929ce5 100644 --- a/test/mock-http-bins.test.ts +++ b/test/mock-http-bins.test.ts @@ -297,7 +297,7 @@ describe("MockHttpBins", () => { const keyv = new Keyv({ store: new Map() }); vi.spyOn(keyv, "set").mockRejectedValue( new BinStoreFailure( - "Invalid connection string. Percent-encode reserved characters in the username and password (@ : / ? # [ ]).", + "Invalid connection string. Percent-encode reserved characters in the username and password (@ : / ? # [ ] $).", ), ); const failing = new MockHttpBins({ keyv });