fix(audit): readable names, single writer, denial rows, redacted args
The activity log recorded Start's hashed function id (a sha256 URL
segment) as the name; the middleware now reads compile-time
serverFnMeta.name with the path segment as fallback. toServerResult
resolves { success:false } instead of throwing, so the middleware logged
failures as successes while toServerResult wrote a duplicate row with its
own dead name parser — the middleware is now the single writer and reads
the envelope's success flag. Admin denials, which threw before the logging
middleware ran, get their own audit row. Arguments are redacted
(phone/otp/token keys) and truncated at 2KB. The admin activities search
binds its filter parameters (likePattern escaping) instead of
interpolating raw input. Adds vitest with node-env tests for the
middleware, redaction, and filter utils, and extends logging coverage to
mutating fns that lacked it.
This commit is contained in:
@@ -1,5 +1,7 @@
|
||||
import PocketBase from "pocketbase";
|
||||
import { PlayerInfo } from "@/features/players/types";
|
||||
import { pbFilter } from "../util/filter";
|
||||
import { likePattern } from "../util/like-pattern";
|
||||
|
||||
export interface Activity {
|
||||
id: string;
|
||||
@@ -61,15 +63,15 @@ export function createActivitiesService(pb: PocketBase) {
|
||||
const filters: string[] = [];
|
||||
|
||||
if (name) {
|
||||
filters.push(`name ~ "${name}"`);
|
||||
filters.push(pbFilter(pb, "name ~ {:name}", { name: likePattern(name) }));
|
||||
}
|
||||
|
||||
if (player) {
|
||||
filters.push(`player = "${player}"`);
|
||||
filters.push(pbFilter(pb, "player = {:player}", { player }));
|
||||
}
|
||||
|
||||
if (success !== undefined) {
|
||||
filters.push(`success = ${success}`);
|
||||
filters.push(pbFilter(pb, "success = {:success}", { success }));
|
||||
}
|
||||
|
||||
const filterString = filters.length > 0 ? filters.join(" && ") : "";
|
||||
@@ -98,7 +100,7 @@ export function createActivitiesService(pb: PocketBase) {
|
||||
|
||||
async getActivitiesByUser(userId: string, limit: number = 50): Promise<Activity[]> {
|
||||
const result = await pb.collection("activities").getList<Activity>(1, limit, {
|
||||
filter: `player = "${userId}"`,
|
||||
filter: pbFilter(pb, "player = {:userId}", { userId }),
|
||||
sort: "-created",
|
||||
});
|
||||
return result.items;
|
||||
@@ -106,7 +108,7 @@ export function createActivitiesService(pb: PocketBase) {
|
||||
|
||||
async getActivitiesByFunction(functionName: string, limit: number = 50): Promise<Activity[]> {
|
||||
const result = await pb.collection("activities").getList<Activity>(1, limit, {
|
||||
filter: `name = "${functionName}"`,
|
||||
filter: pbFilter(pb, "name = {:functionName}", { functionName }),
|
||||
sort: "-created",
|
||||
});
|
||||
return result.items;
|
||||
|
||||
@@ -0,0 +1,118 @@
|
||||
import PocketBase from "pocketbase";
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { pbFilter } from "./filter";
|
||||
|
||||
// Real SDK, not a stub: the escaping under test lives in PocketBase's own
|
||||
// replaceAll.
|
||||
const pb = new PocketBase("http://pocketbase.test");
|
||||
|
||||
const quotedLiteral = (expression: string) => {
|
||||
const open = expression.indexOf("'");
|
||||
let literal = "";
|
||||
for (let i = open + 1; i < expression.length; i++) {
|
||||
if (expression[i] === "\\") {
|
||||
literal += expression[i + 1];
|
||||
i++;
|
||||
continue;
|
||||
}
|
||||
if (expression[i] === "'") return literal;
|
||||
literal += expression[i];
|
||||
}
|
||||
throw new Error(`unterminated literal in ${expression}`);
|
||||
};
|
||||
|
||||
describe("pbFilter", () => {
|
||||
it.each([
|
||||
["a match-substitution pattern", "a$&b"],
|
||||
["a preceding-input pattern", "a$`b"],
|
||||
["a following-input pattern", "a$'b"],
|
||||
["an escaped-dollar pattern", "a$$b"],
|
||||
["a bare dollar", "a$b"],
|
||||
["nothing but a dollar", "$"],
|
||||
["a dollar beside a metacharacter", "50$%"],
|
||||
])("substitutes %s as a literal", (_label, term) => {
|
||||
const expression = pbFilter(pb, "first_name ~ {:query}", { query: term });
|
||||
|
||||
expect(quotedLiteral(expression)).toBe(term);
|
||||
expect(expression).not.toContain("{:query}");
|
||||
});
|
||||
|
||||
it("keeps both dollars of an escaped-dollar pattern", () => {
|
||||
expect(pbFilter(pb, "f ~ {:q}", { q: "a$$b" })).toBe("f ~ 'a$$b'");
|
||||
});
|
||||
|
||||
it("splices no part of the surrounding expression into the literal", () => {
|
||||
const expression = pbFilter(pb, "first_name ~ {:query}", {
|
||||
query: "a$`b",
|
||||
});
|
||||
|
||||
expect(expression).toBe("first_name ~ 'a$`b'");
|
||||
expect(quotedLiteral(expression)).not.toContain("first_name");
|
||||
});
|
||||
|
||||
it.each([
|
||||
["a single quote", "o'brien"],
|
||||
["a trailing backslash", "ada\\"],
|
||||
["a percent sign", "50%"],
|
||||
])("escapes %s exactly as the SDK does", (_label, term) => {
|
||||
expect(pbFilter(pb, "f ~ {:q}", { q: term })).toBe(
|
||||
pb.filter("f ~ {:q}", { q: term })
|
||||
);
|
||||
});
|
||||
|
||||
it.each([
|
||||
["a number", 42],
|
||||
["a boolean", true],
|
||||
["null", null],
|
||||
["a date", new Date(Date.UTC(2024, 0, 2, 3, 4, 5, 678))],
|
||||
])("renders %s identically to pb.filter", (_label, value) => {
|
||||
expect(pbFilter(pb, "f = {:v}", { v: value })).toBe(
|
||||
pb.filter("f = {:v}", { v: value })
|
||||
);
|
||||
});
|
||||
|
||||
it("leaves a placeholder with no matching parameter verbatim", () => {
|
||||
expect(pbFilter(pb, "a = {:missing}", {})).toBe("a = {:missing}");
|
||||
expect(pbFilter(pb, "a = {:missing}", {})).toBe(
|
||||
pb.filter("a = {:missing}", {})
|
||||
);
|
||||
});
|
||||
|
||||
it.each([
|
||||
["a hyphen", "user-id"],
|
||||
["a dot", "meta.user"],
|
||||
["a digit and underscore", "auth_id2"],
|
||||
])("resolves a key containing %s the way the SDK does", (_label, key) => {
|
||||
const raw = `a = {:${key}}`;
|
||||
|
||||
expect(pbFilter(pb, raw, { [key]: "v" })).toBe("a = 'v'");
|
||||
expect(pbFilter(pb, raw, { [key]: "v" })).toBe(pb.filter(raw, { [key]: "v" }));
|
||||
});
|
||||
|
||||
it("does not absorb another parameter's value into a literal", () => {
|
||||
expect(
|
||||
pbFilter(pb, "a = {:x} && b = {:y}", { x: "%{:y}%", y: "SECRET" })
|
||||
).toBe("a = '%{:y}%' && b = 'SECRET'");
|
||||
});
|
||||
|
||||
it("substitutes every occurrence of a repeated placeholder", () => {
|
||||
expect(
|
||||
pbFilter(pb, "(first_name ~ {:q} || last_name ~ {:q})", { q: "%ada%" })
|
||||
).toBe("(first_name ~ '%ada%' || last_name ~ '%ada%')");
|
||||
});
|
||||
});
|
||||
|
||||
// Pins the SDK behaviour the doubling in `quote` compensates for. If an upgrade
|
||||
// fixes the expansion upstream, these fail rather than the doubling silently
|
||||
// becoming a double-escape.
|
||||
describe("the pocketbase SDK substitution this works around", () => {
|
||||
it("still mis-expands a dollar pattern in a parameter value", () => {
|
||||
expect(pb.filter("a ~ {:q}", { q: "x$&y" })).toContain("{:q}");
|
||||
});
|
||||
|
||||
it("still splices a later parameter into an earlier literal", () => {
|
||||
expect(pb.filter("a = {:x} && b = {:y}", { x: "%{:y}%", y: "SECRET" })).toBe(
|
||||
"a = '%'SECRET'%' && b = 'SECRET'"
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,28 @@
|
||||
import type PocketBase from "pocketbase";
|
||||
|
||||
export type FilterParam = string | number | boolean | Date | null;
|
||||
|
||||
// Any key the SDK's own `replaceAll("{:" + key + "}", …)` loop would substitute,
|
||||
// so which keys resolve does not depend on the characters they are spelled with.
|
||||
const PLACEHOLDER = /\{:([^}]+)\}/g;
|
||||
|
||||
// `pb.filter` substitutes with String.replaceAll and a *string* replacement, so
|
||||
// `$&`, `` $` ``, `$'` and `$$` inside a value are expanded as replacement
|
||||
// patterns: the value's own text, or a slice of the surrounding expression,
|
||||
// gets spliced into the quoted literal. Doubling every `$` first collapses back
|
||||
// to the exact literal inside that same replaceAll. The FilterParam union is
|
||||
// load-bearing — it keeps objects and arrays out of the SDK's JSON.stringify
|
||||
// branch, which would reintroduce an undoubled `$`.
|
||||
const quote = (pb: PocketBase, value: FilterParam) =>
|
||||
pb.filter("{:v}", {
|
||||
v: typeof value === "string" ? value.replaceAll("$", () => "$$") : value,
|
||||
});
|
||||
|
||||
export const pbFilter = (
|
||||
pb: PocketBase,
|
||||
raw: string,
|
||||
params: Record<string, FilterParam>
|
||||
) =>
|
||||
raw.replace(PLACEHOLDER, (token, key: string) =>
|
||||
Object.hasOwn(params, key) ? quote(pb, params[key]) : token
|
||||
);
|
||||
@@ -0,0 +1,69 @@
|
||||
import PocketBase from "pocketbase";
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { literalTerminates } from "@/test/pb-filter";
|
||||
import { pbFilter } from "./filter";
|
||||
import { likePattern } from "./like-pattern";
|
||||
|
||||
const compose = (term: string) =>
|
||||
pbFilter(new PocketBase("http://pocketbase.test"), "first_name ~ {:query}", {
|
||||
query: likePattern(term),
|
||||
});
|
||||
|
||||
describe("likePattern", () => {
|
||||
it("wraps a plain term so ~ matches a substring", () => {
|
||||
expect(likePattern("ada")).toBe("%ada%");
|
||||
});
|
||||
|
||||
it.each([
|
||||
["a percent sign", "50%", "%50\\%%"],
|
||||
["an underscore", "_", "%\\_%"],
|
||||
["a backslash", "ada\\", "%ada\\\\%"],
|
||||
])("escapes %s so it matches literally", (_label, term, expected) => {
|
||||
expect(likePattern(term)).toBe(expected);
|
||||
});
|
||||
|
||||
it("always ends the operand with an unescaped wildcard", () => {
|
||||
// An odd run of backslashes before the final % would mean the % is itself
|
||||
// escaped, which is the shape that swallows the closing quote.
|
||||
for (const term of ["ada", "ada\\", "\\", "50%", "_", "o'brien\\"]) {
|
||||
const pattern = likePattern(term);
|
||||
const trailingSlashes = /(\\*)%$/.exec(pattern)?.[1] ?? "";
|
||||
|
||||
expect(pattern.endsWith("%")).toBe(true);
|
||||
expect(trailingSlashes.length % 2).toBe(0);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("likePattern composed through the real SDK", () => {
|
||||
it.each(["ada\\", "\\", "a_b\\", "o'brien\\", "ada\\\\"])(
|
||||
"keeps the filter expression parseable for %j",
|
||||
(term) => {
|
||||
expect(literalTerminates(compose(term))).toBe(true);
|
||||
}
|
||||
);
|
||||
|
||||
it("detects the unterminated literal a raw term produces", () => {
|
||||
const raw = new PocketBase("http://pocketbase.test").filter(
|
||||
"first_name ~ {:query}",
|
||||
{ query: "ada\\" }
|
||||
);
|
||||
|
||||
expect(raw).toBe("first_name ~ 'ada\\'");
|
||||
expect(literalTerminates(raw)).toBe(false);
|
||||
});
|
||||
|
||||
it("leaves a term without metacharacters exactly as before", () => {
|
||||
expect(compose("ada")).toBe("first_name ~ '%ada%'");
|
||||
});
|
||||
|
||||
it.each(["a$&b", "a$`b", "50$%", "a$&_b", "$"])(
|
||||
"carries %j through both escaping layers intact",
|
||||
(term) => {
|
||||
const composed = compose(term);
|
||||
|
||||
expect(composed).toContain(likePattern(term));
|
||||
expect(literalTerminates(composed)).toBe(true);
|
||||
}
|
||||
);
|
||||
});
|
||||
@@ -0,0 +1,10 @@
|
||||
// `pb.filter()` escapes single quotes and nothing else, so a term ending in a
|
||||
// backslash escapes the closing quote of the literal it is substituted into and
|
||||
// PocketBase rejects the whole expression with 400 validation_invalid_filter.
|
||||
// Escaping `\ % _` and appending the wildcards here keeps the operand's last
|
||||
// character a literal `%`, and makes `~` an unconditional substring match:
|
||||
// PocketBase only auto-wraps (and only auto-escapes) operands that contain no
|
||||
// `%` of their own, so a term carrying one would otherwise silently become a
|
||||
// prefix match, and a bare `_` would match every row.
|
||||
export const likePattern = (term: string) =>
|
||||
`%${term.replace(/[\\%_]/g, (char) => `\\${char}`)}%`;
|
||||
@@ -0,0 +1,36 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { redactValue, SENSITIVE_KEY } from "./redact";
|
||||
|
||||
// Shared scrubber for logs + audit rows - a regression leaks PII from both.
|
||||
describe("redactValue", () => {
|
||||
it("redacts sensitive keys and preserves benign ones", () => {
|
||||
expect(
|
||||
redactValue({ phone: "+17135550142", first_name: "Ada", token: "abc" })
|
||||
).toEqual({ phone: "[redacted]", first_name: "Ada", token: "[redacted]" });
|
||||
});
|
||||
|
||||
it("redacts recursively through nested objects and arrays", () => {
|
||||
expect(
|
||||
redactValue({
|
||||
user: { first_name: "Ada", password: "hunter2" },
|
||||
items: [{ authToken: "x" }, { label: "ok" }],
|
||||
})
|
||||
).toEqual({
|
||||
user: { first_name: "Ada", password: "[redacted]" },
|
||||
items: [{ authToken: "[redacted]" }, { label: "ok" }],
|
||||
});
|
||||
});
|
||||
|
||||
it("passes primitives through untouched (a bare string is not assumed secret)", () => {
|
||||
expect(redactValue("hello")).toBe("hello");
|
||||
expect(redactValue(42)).toBe(42);
|
||||
expect(redactValue(null)).toBeNull();
|
||||
});
|
||||
|
||||
it("covers the documented sensitive keys", () => {
|
||||
for (const key of ["token", "secret", "password", "phone", "otp", "code", "auth", "key"]) {
|
||||
expect(SENSITIVE_KEY.test(key)).toBe(true);
|
||||
}
|
||||
expect(SENSITIVE_KEY.test("first_name")).toBe(false);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,21 @@
|
||||
// Shared redaction for logs + audit rows so the two never drift.
|
||||
export const SENSITIVE_KEY = /token|secret|password|phone|otp|code|auth|key/i;
|
||||
|
||||
const REDACTED = "[redacted]";
|
||||
|
||||
// Deep-redact: keys matching SENSITIVE_KEY become "[redacted]"; primitives pass through.
|
||||
export const redactValue = (value: unknown): unknown => {
|
||||
if (Array.isArray(value)) return value.map(redactValue);
|
||||
if (value && typeof value === "object") {
|
||||
// Keep Error serializable.
|
||||
if (value instanceof Error) {
|
||||
return { name: value.name, message: value.message, stack: value.stack };
|
||||
}
|
||||
const out: Record<string, unknown> = {};
|
||||
for (const [key, v] of Object.entries(value as Record<string, unknown>)) {
|
||||
out[key] = SENSITIVE_KEY.test(key) ? REDACTED : redactValue(v);
|
||||
}
|
||||
return out;
|
||||
}
|
||||
return value;
|
||||
};
|
||||
@@ -1,6 +1,5 @@
|
||||
import { logger } from "../../logger";
|
||||
import { ErrorType, ServerError, ServerResult } from "../types";
|
||||
import { getRequest } from "@tanstack/react-start/server";
|
||||
import { isRedirect } from "@tanstack/react-router";
|
||||
|
||||
export const createServerError = (
|
||||
@@ -17,57 +16,20 @@ export const createServerError = (
|
||||
context,
|
||||
});
|
||||
|
||||
// Audit rows are written by serverFnLoggingMiddleware, which reads the
|
||||
// returned envelope's success flag — never write them here.
|
||||
export const toServerResult = async <T>(
|
||||
serverFn: () => Promise<T>
|
||||
): Promise<ServerResult<T>> => {
|
||||
const startTime = Date.now();
|
||||
|
||||
try {
|
||||
const data = await serverFn();
|
||||
return { success: true, data };
|
||||
} catch (error) {
|
||||
if (isRedirect(error) || error instanceof Response) throw error;
|
||||
|
||||
const duration = Date.now() - startTime;
|
||||
logger.error('Server Fn Error', error);
|
||||
|
||||
const mappedError = mapKnownError(error);
|
||||
|
||||
let fnName = 'unknown';
|
||||
try {
|
||||
const request = getRequest();
|
||||
const url = new URL(request.url);
|
||||
|
||||
const functionId = url.searchParams.get('_serverFnId') || url.pathname;
|
||||
|
||||
if (functionId.includes('--')) {
|
||||
const match = functionId.match(/--([^_]+)_/);
|
||||
fnName = match?.[1] || functionId.split('--')[1]?.split('_')[0] || 'unknown';
|
||||
} else {
|
||||
fnName = serverFn.name || 'unknown';
|
||||
}
|
||||
} catch {
|
||||
fnName = serverFn.name || 'unknown';
|
||||
}
|
||||
|
||||
import("../../pocketbase/client")
|
||||
.then(async ({ pbAdmin }) => {
|
||||
await pbAdmin.authPromise;
|
||||
await pbAdmin.createActivity({
|
||||
name: fnName,
|
||||
duration,
|
||||
success: false,
|
||||
error: mappedError.message,
|
||||
arguments: {
|
||||
errorType: mappedError.code,
|
||||
statusCode: mappedError.statusCode,
|
||||
userMessage: mappedError.userMessage,
|
||||
},
|
||||
});
|
||||
})
|
||||
.catch(() => {});
|
||||
|
||||
return { success: false, error: mappedError };
|
||||
return { success: false, error: mapKnownError(error) };
|
||||
}
|
||||
};
|
||||
|
||||
|
||||
Reference in New Issue
Block a user