You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by wu...@apache.org on 2026/08/23 06:49:30 UTC
(skywalking-horizon-ui) branch fix/audit-username-exact-match created (now 25a80b67)
This is an automated email from the ASF dual-hosted git repository.
wu-sheng pushed a change to branch fix/audit-username-exact-match
in repository https://gitbox.apache.org/repos/asf/skywalking-horizon-ui.git
at 25a80b67 fix(audit): match a login id exactly, and give every statement its own bound
This branch includes the following new commits:
new 25a80b67 fix(audit): match a login id exactly, and give every statement its own bound
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(skywalking-horizon-ui) 01/01: fix(audit): match a login id exactly, and give every statement its own bound
Posted by wu...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
wu-sheng pushed a commit to branch fix/audit-username-exact-match
in repository https://gitbox.apache.org/repos/asf/skywalking-horizon-ui.git
commit 25a80b67d95734f997d27c24b3470b17771e34f9
Author: Wu Sheng <wu...@foxmail.com>
AuthorDate: Sun Aug 23 14:49:19 2026 +0800
fix(audit): match a login id exactly, and give every statement its own bound
Two corrections to the audit store, both found while planning a second
backend.
The Login ID filter matched by PREFIX. That was a LIKE-shaped answer to a
question nobody asked: the filter names one principal, and matching a fragment
made the result depend on how much of a name was typed — `alice` also returned
`alice_ci` and `alice.smith`. It matches exactly now.
The prefix also could not be reproduced. BanyanDB's operator set has no
prefix, no LIKE and no wildcard, so a second backend would have had to pick
between exact match and analyzer-driven full text — leaving one control on one
page meaning two different things depending on which store was configured.
Making the contract exact settles that before the port rather than after it,
so the next backend inherits a behaviour it can reproduce.
`likePrefix` and its escaping tests are gone with it, and the schema no longer
creates the `text_pattern_ops` companion index: that operator class exists to
serve `LIKE 'x%'` and is pure write cost for an equality filter. An index an
existing database already carries is left alone — this list only creates, and
dropping an index an operator may rely on is not a decision a boot path should
be making.
Separately: the statistics query was the one statement that carried no LIMIT.
Its GROUP BY already bounded the result to the window's hours, but that is a
bound that holds only while the data is well-formed. It now carries one
derived from the declared stat windows rather than a literal, so it cannot
drift if a window is added.
The live PostgreSQL suite asserts the new behaviour directly: `alice` matches,
`ali` finds nothing, and `%` is a name nobody has rather than a full read.
---
apps/bff/src/store/audit/postgres/live.test.ts | 8 +++---
apps/bff/src/store/audit/postgres/rows.test.ts | 20 +--------------
apps/bff/src/store/audit/postgres/rows.ts | 11 --------
apps/bff/src/store/audit/postgres/schema.ts | 12 +++++----
apps/bff/src/store/audit/postgres/sql.test.ts | 35 ++++++++++++--------------
apps/bff/src/store/audit/postgres/store.ts | 28 +++++++++++++++------
apps/bff/src/store/audit/types.ts | 17 ++++++++-----
docs/setup/login-audit.md | 2 +-
8 files changed, 62 insertions(+), 71 deletions(-)
diff --git a/apps/bff/src/store/audit/postgres/live.test.ts b/apps/bff/src/store/audit/postgres/live.test.ts
index b55c5941..8b90a7a5 100644
--- a/apps/bff/src/store/audit/postgres/live.test.ts
+++ b/apps/bff/src/store/audit/postgres/live.test.ts
@@ -111,9 +111,11 @@ describe.skipIf(!URL)('the audit log against a real PostgreSQL', () => {
expect(agg[0].count).toBe(9);
});
- it('matches a prefix and escapes a bare wildcard', async () => {
- expect((await svc.query({ pageNum: 1, pageSize: 50, username: 'ali' })).rows).toHaveLength(1);
- // Unescaped this would match everything, turning a filter into a full read.
+ /** The filter names ONE principal: a fragment finds nothing, and a wildcard
+ * is just a name nobody has rather than a full table read. */
+ it('matches one principal exactly', async () => {
+ expect((await svc.query({ pageNum: 1, pageSize: 50, username: 'alice' })).rows).toHaveLength(1);
+ expect((await svc.query({ pageNum: 1, pageSize: 50, username: 'ali' })).rows).toHaveLength(0);
expect((await svc.query({ pageNum: 1, pageSize: 50, username: '%' })).rows).toHaveLength(0);
});
diff --git a/apps/bff/src/store/audit/postgres/rows.test.ts b/apps/bff/src/store/audit/postgres/rows.test.ts
index 44ba5325..b5a6c841 100644
--- a/apps/bff/src/store/audit/postgres/rows.test.ts
+++ b/apps/bff/src/store/audit/postgres/rows.test.ts
@@ -16,7 +16,7 @@
*/
import { describe, it, expect } from 'vitest';
-import { likePrefix, toEntry, toId, toInet, toNumber, valuesClause, type RawAuditRow } from './rows.js';
+import { toEntry, toId, toInet, toNumber, valuesClause, type RawAuditRow } from './rows.js';
import { CHUNK_ROWS, EVENT_COLUMNS, AGGREGATE_COLUMNS } from './store.js';
describe('bigint handling', () => {
@@ -45,24 +45,6 @@ describe('bigint handling', () => {
});
});
-describe('prefix filters', () => {
- it('escapes LIKE metacharacters so a prefix means what it says', () => {
- // Unescaped, `alice_ci` would also match `aliceXci`.
- expect(likePrefix('alice_ci')).toBe('alice\\_ci%');
- expect(likePrefix('50%')).toBe('50\\%%');
- // A lone `%` would otherwise turn the filter into a full table read.
- expect(likePrefix('%')).toBe('\\%%');
- });
-
- it('escapes the escape character first, so it cannot double-escape', () => {
- expect(likePrefix('a\\b')).toBe('a\\\\b%');
- });
-
- it('leaves an ordinary prefix alone', () => {
- expect(likePrefix('alice')).toBe('alice%');
- });
-});
-
describe('inet handling', () => {
/** A malformed address raises a type error mid-statement, which would cost
* the whole batch. A bad address must not cost the row it came on. */
diff --git a/apps/bff/src/store/audit/postgres/rows.ts b/apps/bff/src/store/audit/postgres/rows.ts
index 31847865..97e7c09d 100644
--- a/apps/bff/src/store/audit/postgres/rows.ts
+++ b/apps/bff/src/store/audit/postgres/rows.ts
@@ -48,17 +48,6 @@ export function toId(value: unknown): string {
return String(value);
}
-/**
- * Escape `LIKE` metacharacters so a prefix filter means what it says.
- *
- * `%` and `_` are wildcards, so searching for `alice_ci` would otherwise match
- * `aliceXci`, and a lone `%` would match everything — turning a filter into a
- * full table read. The backslash is escaped first or it would double-escape
- * the escapes.
- */
-export function likePrefix(value: string): string {
- return `${value.replace(/[\\%_]/g, (ch) => `\\${ch}`)}%`;
-}
/**
* A malformed address raises a Postgres type error mid-statement, which would
diff --git a/apps/bff/src/store/audit/postgres/schema.ts b/apps/bff/src/store/audit/postgres/schema.ts
index 8a10ee5c..93be0baf 100644
--- a/apps/bff/src/store/audit/postgres/schema.ts
+++ b/apps/bff/src/store/audit/postgres/schema.ts
@@ -75,11 +75,13 @@ export const SCHEMA_STATEMENTS: readonly string[] = [
`CREATE INDEX IF NOT EXISTS horizon_audit_username_idx
ON horizon_audit (username, at DESC)`,
- // PREFIX search needs its own operator class: a default-collation B-tree
- // does NOT serve `LIKE 'x%'` on any collation but C, so without this the
- // filter silently degrades to a sequential scan of the whole table.
- `CREATE INDEX IF NOT EXISTS horizon_audit_username_prefix_idx
- ON horizon_audit (username text_pattern_ops)`,
+ // No `text_pattern_ops` companion: that operator class exists to serve
+ // `LIKE 'x%'`, and the filter is an exact match. The plain index above
+ // serves equality on any collation.
+ //
+ // The index is not dropped here for a database that already has it — this
+ // list only creates. It is harmless, and dropping an index an operator may
+ // have come to rely on is not something a boot path should decide.
// No index on outcome, provider or roles: the page filters by time, kind
// and username only, and an index nothing queries is write cost for
diff --git a/apps/bff/src/store/audit/postgres/sql.test.ts b/apps/bff/src/store/audit/postgres/sql.test.ts
index f5ce188a..64e96efc 100644
--- a/apps/bff/src/store/audit/postgres/sql.test.ts
+++ b/apps/bff/src/store/audit/postgres/sql.test.ts
@@ -33,7 +33,7 @@
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
import { PGlite } from '@electric-sql/pglite';
import { SCHEMA_STATEMENTS } from './schema.js';
-import { likePrefix, toNumber, valuesClause } from './rows.js';
+import { toNumber, valuesClause } from './rows.js';
import { EVENT_COLUMNS as EVENT_COLS, AGGREGATE_COLUMNS as AGG_COLS } from './store.js';
let db: PGlite;
@@ -68,12 +68,11 @@ describe('the schema', () => {
for (const s of SCHEMA_STATEMENTS) await expect(db.query(s)).resolves.toBeDefined();
});
- it('creates the prefix indexes that make a LIKE filter index-served', async () => {
+ it('creates the indexes the list and the aggregate upsert depend on', async () => {
const r = await db.query<{ indexname: string }>(
`SELECT indexname FROM pg_indexes WHERE tablename = 'horizon_audit'`,
);
const names = r.rows.map((x) => x.indexname);
- expect(names).toContain('horizon_audit_username_prefix_idx');
expect(names).toContain('horizon_audit_bucket_idx');
});
});
@@ -154,29 +153,27 @@ describe('reading', () => {
`SELECT id, at, kind, provider, outcome, reason, username, mail, roles, host(client_ip) AS client_ip, host(horizon_ip) AS horizon_ip,
horizon_node, hour_bucket, count
FROM horizon_audit
- WHERE at >= $1 AND at < $2 AND kind = ANY($3) AND username LIKE $4
- ORDER BY at DESC, id DESC LIMIT $5 OFFSET $6`;
+ WHERE at >= $1 AND at < $2 AND kind = ANY($3) AND username = $4
+ ORDER BY at DESC, id DESC LIMIT $5`;
const r = await db.query(sql, [
new Date('2026-01-01T00:00:00Z'), new Date('2027-01-01T00:00:00Z'),
- ['local', 'sso'], likePrefix('al'), 51, 0,
+ ['local', 'sso'], 'alice', 51,
]);
expect(Array.isArray(r.rows)).toBe(true);
});
- /** An unescaped `_` would match any character, and a lone `%` would turn a
- * filter into a full table read. */
- it('treats an underscore in a prefix as a literal', async () => {
- await db.query(
- `INSERT INTO horizon_audit (at,kind,outcome,username,horizon_node) VALUES ($1,$2,$3,$4,$5)`,
- [new Date('2026-08-22T15:00:00Z'), 'api-token', 1, 'alice_ci', 'pod-1:aaa'],
- );
- await db.query(
- `INSERT INTO horizon_audit (at,kind,outcome,username,horizon_node) VALUES ($1,$2,$3,$4,$5)`,
- [new Date('2026-08-22T15:00:00Z'), 'api-token', 1, 'aliceXci', 'pod-1:aaa'],
- );
+ /** The filter names ONE principal. `alice_ci` must not also return
+ * `aliceXci`, which is what a LIKE prefix did with an unescaped `_`. */
+ it('matches one principal exactly, never a lookalike', async () => {
+ for (const u of ['alice_ci', 'aliceXci', 'alice_ci_2']) {
+ await db.query(
+ `INSERT INTO horizon_audit (at,kind,outcome,username,horizon_node) VALUES ($1,$2,$3,$4,$5)`,
+ [new Date('2026-08-22T15:00:00Z'), 'api-token', 1, u, 'pod-1:aaa'],
+ );
+ }
const r = await db.query<{ username: string }>(
- `SELECT username FROM horizon_audit WHERE username LIKE $1`,
- [likePrefix('alice_ci')],
+ `SELECT username FROM horizon_audit WHERE username = $1`,
+ ['alice_ci'],
);
expect(r.rows.map((x) => x.username)).toEqual(['alice_ci']);
});
diff --git a/apps/bff/src/store/audit/postgres/store.ts b/apps/bff/src/store/audit/postgres/store.ts
index 6f6c7dde..1e6cb77e 100644
--- a/apps/bff/src/store/audit/postgres/store.ts
+++ b/apps/bff/src/store/audit/postgres/store.ts
@@ -35,6 +35,7 @@ import {
type AuditAggregate,
type AuditPageResult,
type AuditEvent,
+ AUDIT_STAT_WINDOWS,
type AuditFilter,
type AuditStat,
type AuditStatColumn,
@@ -45,7 +46,7 @@ import {
type StoreStamp,
} from '../types.js';
import { SCHEMA_LOCK_ID, SCHEMA_STATEMENTS } from './schema.js';
-import { likePrefix, toEntry, toInet, toNumber, valuesClause, type RawAuditRow } from './rows.js';
+import { toEntry, toInet, toNumber, valuesClause, type RawAuditRow } from './rows.js';
/** Rows per statement. The protocol caps a statement at 65535 bind
* parameters; at the widest insert here that is a few thousand rows, and 500
@@ -55,6 +56,10 @@ import { likePrefix, toEntry, toInet, toNumber, valuesClause, type RawAuditRow }
export const CHUNK_ROWS = 500;
/** Rows per sweep pass. A 90-day purge as one statement holds locks and
* bloats WAL, so it goes in bounded passes. */
+/** The widest statistics window, so the grouped result has a bound that does
+ * not depend on the stat table containing only the hours it should. */
+const MAX_STAT_HOURS = Math.max(...AUDIT_STAT_WINDOWS);
+
const SWEEP_BATCH = 5_000;
const SWEEP_MAX_PASSES = 200;
/** How long to wait behind another replica's schema lock before giving up and
@@ -425,10 +430,12 @@ export class PostgresAuditStore implements AuditStore {
if (filter.from !== undefined) where.push(`at >= ${bind(new Date(filter.from))}`);
if (filter.to !== undefined) where.push(`at < ${bind(new Date(filter.to))}`);
if (filter.kind?.length) where.push(`kind = ANY(${bind(filter.kind)})`);
- // No explicit ESCAPE clause: backslash is already the LIKE default, and
- // spelling it in a literal breaks under `standard_conforming_strings=off`,
- // where the literal itself would need doubling.
- if (filter.username) where.push(`username LIKE ${bind(likePrefix(filter.username))}`);
+ // EXACT, not a prefix. A prefix was a LIKE-shaped answer to a question
+ // nobody asked: the filter names one principal, and matching a fragment
+ // makes the result depend on how much of a name was typed. It also could
+ // not be reproduced on a backend whose operator set has no prefix, which
+ // would have made the same control mean two different things.
+ if (filter.username) where.push(`username = ${bind(filter.username)}`);
// Keyset: resume strictly AFTER the previous page's last row in the
// `(at DESC, id DESC)` ordering. A row-value comparison is what makes the
@@ -468,6 +475,12 @@ export class PostgresAuditStore implements AuditStore {
async queryStat(window: AuditStatWindow): Promise<AuditStatResult> {
const from = hourBucketOf(Date.now() - (window - 1) * 3_600_000);
+ // Every statement this store sends carries its own bound. The grouping
+ // already caps this one at the window's hours, but a query whose size
+ // depends on the data being well-formed is a query that gets surprising
+ // when it is not.
+ const params: unknown[] = [from];
+ const bindStat = (v: unknown): string => `$${params.push(v)}`;
try {
const res = await this.db.query<Record<string, unknown>>(
`SELECT hour_bucket,
@@ -489,8 +502,9 @@ export class PostgresAuditStore implements AuditStore {
FROM horizon_audit_stat
WHERE hour_bucket >= $1
GROUP BY hour_bucket
- ORDER BY hour_bucket`,
- [from],
+ ORDER BY hour_bucket
+ LIMIT ${bindStat(MAX_STAT_HOURS)}`,
+ params,
);
const byHour = new Map<number, AuditStatColumn>();
diff --git a/apps/bff/src/store/audit/types.ts b/apps/bff/src/store/audit/types.ts
index a1493da6..17000e69 100644
--- a/apps/bff/src/store/audit/types.ts
+++ b/apps/bff/src/store/audit/types.ts
@@ -205,13 +205,18 @@ export interface AuditFilter {
from?: number;
to?: number;
kind?: AuditKind[];
- /** PREFIX match. Not a substring: a leading wildcard is an unindexed scan
- * and a fishing tool, while a prefix is index-served and is what makes the
- * token kinds reachable — nobody types an opaque token id from memory.
+ /**
+ * EXACT match on the verified principal.
+ *
+ * Not a prefix and not a substring. A prefix made the result depend on how
+ * much of a name was typed, and it could not be reproduced on a store whose
+ * operator set has none — which would have left one control meaning two
+ * different things depending on the backend behind it.
*
- * Who a credential belongs to is NOT recorded here: that is a fact about
- * configuration, which the tokens file already holds, not a fact about a
- * sign-in. */
+ * Who a credential belongs to is NOT recorded here: that is a fact about
+ * configuration, which the tokens file already holds, not a fact about a
+ * sign-in.
+ */
username?: string;
/**
* Where the previous page stopped: `(at, id)` of its last row.
diff --git a/docs/setup/login-audit.md b/docs/setup/login-audit.md
index e401f63c..37e1efb4 100644
--- a/docs/setup/login-audit.md
+++ b/docs/setup/login-audit.md
@@ -115,7 +115,7 @@ Records refused by the hourly limit are deliberately *not* drawn as a bar. They
Two other counts are kept but **not shown on the page**: writes whose outcome could not be confirmed, and token uses discarded when a long outage exhausted the in-memory buffers. They are *unconfirmed* rather than known-lost — a write that commits and then times out leaves the row in the database and the count here, and nothing on Horizon's side can tell that from a write that never landed. Both reach the application log rather than the page: the recovery line names how many accumulated [...]
-**Filters** are **Time range**, **Auth Channel** and **Login ID** — the same three the list is headed with. `Login ID` matches by prefix, so the first characters of an opaque token id are enough. There is deliberately nothing else: this page is read, not queried, and every extra predicate is an index to carry and another way to ask the database something slow. The other fields on a row are for reading once you have found it.
+**Filters** are **Time range**, **Auth Channel** and **Login ID** — the same three the list is headed with. `Login ID` matches **exactly** — it names one principal, so a partial name finds nothing. A token id is shown in full on its row and can be copied from there. There is deliberately nothing else: this page is read, not queried, and every extra predicate is an index to carry and another way to ask the database something slow. The other fields on a row are for reading once you have found it.
**The list holds two shapes of row, and marks them apart.** Most rows are a single sign-in at a single instant. Rows tagged **hourly total** are not: they summarise one credential's use over one hour, and are the only way token traffic is recorded. Such a row shows the hour rather than a timestamp, a count in `Uses`, and no client address — a credential-hour has no single address, so the cell is blank because the question does not apply, not because the answer is unknown.