diff --git a/dataLayer/schema.ts b/dataLayer/schema.ts index d5debf43e..03fd8f8aa 100644 --- a/dataLayer/schema.ts +++ b/dataLayer/schema.ts @@ -23,7 +23,11 @@ const DB_NAME_CONSTRAINTS = Joi.string() .min(1) .max(commonValidators.schema_length.maximum) .pattern(schemaRegex) - .messages({ 'string.pattern.base': '{:#label} ' + commonValidators.schema_format.message }); + .invalid(...hdbTerms.RESERVED_DATABASE_NAMES) + .messages({ + 'string.pattern.base': '{:#label} ' + commonValidators.schema_format.message, + 'any.invalid': "'{#value}' is a reserved name and cannot be used as a database name", + }); const TABLE_NAME_CONSTRAINTS = Joi.string() .min(1) diff --git a/resources/databases.ts b/resources/databases.ts index 32f141146..94254c3cf 100644 --- a/resources/databases.ts +++ b/resources/databases.ts @@ -11,7 +11,12 @@ import { } from '../dataLayer/harperBridge/lmdbBridge/lmdbUtility/initializePaths.js'; import { makeTable } from './Table.ts'; import OpenEnvironmentObject from '../utility/lmdb/OpenEnvironmentObject.ts'; -import { CONFIG_PARAMS, LEGACY_DATABASES_DIR_NAME, DATABASES_DIR_NAME } from '../utility/hdbTerms.ts'; +import { + CONFIG_PARAMS, + LEGACY_DATABASES_DIR_NAME, + DATABASES_DIR_NAME, + RESERVED_DATABASE_NAMES, +} from '../utility/hdbTerms.ts'; import { getConfigPath } from '../config/configUtils.ts'; import { ClientError } from '../utility/errors/hdbError.ts'; import { _assignPackageExport } from '../globals.js'; @@ -1089,6 +1094,16 @@ export function table(tableDefinition: TableDefinition): Tabl cacheControl, } = tableDefinition; if (!databaseName) databaseName = DEFAULT_DATABASE_NAME; + // Reject reserved names here too, not only at the operations API: a database + // is also created by schema authoring — a `schema.graphql` `@table(database:)` + // or a programmatic `table()` call — which bypasses the create_schema + // validation. A reserved name collides with a role permission flag (harper#1016). + // Deliberately on this authoring path only, NOT in `database()`/`makeTable`: those + // are also the load and drop paths, so a reserved-name database created before this + // fix still loads (data stays accessible) and can be dropped to remediate. + if ((RESERVED_DATABASE_NAMES as readonly string[]).includes(databaseName)) { + throw new ClientError(`'${databaseName}' is a reserved name and cannot be used as a database name`); + } const rootStore = database({ database: databaseName, table: tableName }); const tables = databases[databaseName]; logger.trace(`Defining ${tableName} in ${databaseName}`); diff --git a/resources/roles.ts b/resources/roles.ts index 117ed0f17..0e150ef14 100644 --- a/resources/roles.ts +++ b/resources/roles.ts @@ -2,8 +2,12 @@ import { getDatabases } from './databases.ts'; import { alterRole, addRole } from '../security/role.ts'; import { parseDocument } from 'yaml'; import { isEqual } from 'lodash'; +import { RESERVED_DATABASE_NAMES } from '../utility/hdbTerms.ts'; -const USERS_NOT_DBS = ['super_user', 'structure_user']; +// Named permission flags are keyed alongside per-database permissions in a role's +// `permission` object; skip them when walking database permissions. This is the +// same reserved set that database names are rejected from (harper#1016). +const USERS_NOT_DBS: readonly string[] = RESERVED_DATABASE_NAMES; /** * This is the component for handling role declarations in the Harper system. This will read roles.yaml for role diff --git a/security/permissionsTranslator.js b/security/permissionsTranslator.js index d921d4ec7..a80bc4521 100644 --- a/security/permissionsTranslator.js +++ b/security/permissionsTranslator.js @@ -132,6 +132,9 @@ function translateRolePermissions(role, schema) { const perms = role.permission; finalPermissions[terms.SYSTEM_SCHEMA_NAME] = perms[terms.SYSTEM_SCHEMA_NAME]; finalPermissions.structure_user = perms.structure_user; + // Preserve cluster_user like the other named flags so a translated role keeps + // what it declared, rather than silently dropping it (harper#1016). + finalPermissions.cluster_user = perms.cluster_user; // Preserve operations allowlist and its pre-expanded Set so the gate check in // verifyPerms still has access after role.permission is replaced with fullRolePerms. finalPermissions.operations = perms.operations; diff --git a/unitTests/commonTestErrors.js b/unitTests/commonTestErrors.js index 786d6a8f8..debf9ce51 100644 --- a/unitTests/commonTestErrors.js +++ b/unitTests/commonTestErrors.js @@ -110,8 +110,6 @@ const TEST_ROLE_PERMS_ERROR = { SU_ROLE_MISSING_ERROR: "Missing 'super_user' key/value in permission set", SU_CU_ROLE_BOOLEAN_ERROR: (role) => `Value for '${role}' permission must be a boolean`, SU_CU_ROLE_NO_PERMS_ALLOWED: (role) => `Roles with '${role}' set to true cannot have other permissions set.`, - SU_CU_ROLE_COMBINED_ERROR: - "Roles cannot have both 'super_user' and 'cluster_user' values included in their permissions set.", TABLE_PERM_MISSING: (perm) => `Missing table ${perm.toUpperCase()} permission`, TABLE_PERM_NOT_BOOLEAN: (perm) => `Table ${perm.toUpperCase()} permission must be a boolean`, STRUCTURE_USER_ROLE_TYPE_ERROR: (role) => `Value for '${role}' permission must be a boolean or Array`, diff --git a/unitTests/dataLayer/reservedSchemaNames.test.js b/unitTests/dataLayer/reservedSchemaNames.test.js new file mode 100644 index 000000000..fda1a7275 --- /dev/null +++ b/unitTests/dataLayer/reservedSchemaNames.test.js @@ -0,0 +1,53 @@ +const assert = require('node:assert'); +const schema = require('#src/dataLayer/schema'); +const { table } = require('#src/resources/databases'); + +// harper#1016: a role's `permission` object keys per-database permissions by +// database name alongside named flags (super_user / cluster_user / +// structure_user / operations / _expandedOperations). A database named after one +// of those flags collides with it, so those names are rejected as database +// identifiers — at both the operations API (create_schema / create_table) and +// the schema-authoring path (graphql `@table(database:)` / programmatic table()). +const RESERVED = ['super_user', 'cluster_user', 'structure_user', 'operations', '_expandedOperations']; + +describe('#1016 reserved database names', () => { + for (const name of RESERVED) { + it(`create_schema rejects reserved name '${name}'`, async () => { + await assert.rejects( + () => schema.createSchemaStructure({ operation: 'create_schema', schema: name }), + /reserved name/, + `create_schema accepted reserved name '${name}'` + ); + }); + + it(`create_table rejects reserved database '${name}'`, async () => { + await assert.rejects( + () => schema.createTableStructure({ operation: 'create_table', schema: name, table: 't', primary_key: 'id' }), + /reserved name/, + `create_table accepted reserved database '${name}'` + ); + }); + + it(`schema authoring (table()) rejects reserved database '${name}'`, () => { + // graphql `@table(database:)` and programmatic table() funnel through here, + // bypassing the operations-API validation. The check throws before any store IO. + assert.throws( + () => table({ database: name, table: 't', attributes: [{ name: 'id', isPrimaryKey: true }] }), + /reserved name/, + `table() accepted reserved database '${name}'` + ); + }); + } + + it('does not reject a non-reserved database name as reserved', async () => { + // A normal name must pass the reserved-name check. It may still fail later + // (no schema metadata / bridge in this unit context) — just not as reserved. + let err; + try { + await schema.createSchemaStructure({ operation: 'create_schema', schema: 'my_normal_db' }); + } catch (e) { + err = e; + } + if (err) assert.doesNotMatch(err.message, /reserved name/); + }); +}); diff --git a/unitTests/validation/role_validation.test.js b/unitTests/validation/role_validation.test.js index 5463812c9..d63292a0a 100644 --- a/unitTests/validation/role_validation.test.js +++ b/unitTests/validation/role_validation.test.js @@ -249,6 +249,53 @@ describe('Test role_validation module ', () => { expect(test_result).to.equal(null); }); + it('NOMINAL - should return null for valid cluster_user = true ADD_ROLE object', () => { + const test_role_json = TEST_ADD_ROLE_OBJECT(); + test_role_json.permission = { cluster_user: true }; + const test_result = customValidate_rw(test_role_json, getAddRoleConstraints()); + + expect(test_result).to.equal(null); + }); + + // harper#1016: cluster_user is a boolean role flag, not a database name. A + // non-boolean value must report the boolean-type error, not a misleading + // "database 'cluster_user' does not exist". + it('should return the boolean error (not SCHEMA_NOT_FOUND) for non-boolean cluster_user', () => { + const test_role_json = TEST_ADD_ROLE_OBJECT(); + test_role_json.permission = { cluster_user: 'wut' }; + const test_result = customValidate_rw(test_role_json, getAddRoleConstraints()); + + expect(test_result.statusCode).to.equal(400); + expect(test_result.http_resp_msg.main_permissions).to.include( + TEST_ROLE_PERMS_ERROR.SU_CU_ROLE_BOOLEAN_ERROR('cluster_user') + ); + expect(test_result.http_resp_msg.main_permissions).to.not.include( + TEST_SCHEMA_OP_ERROR.SCHEMA_NOT_FOUND('cluster_user') + ); + }); + + // harper#1016 (review): the boolean check must gate on key presence, not truthiness — a falsey + // non-boolean (0, '', null) must still be rejected rather than silently skipping both the check + // and the database-permission loop. Covers cluster_user and the latent super_user variant. + [ + { role: 'cluster_user', value: 0 }, + { role: 'cluster_user', value: null }, + { role: 'cluster_user', value: '' }, + { role: 'super_user', value: 0 }, + { role: 'super_user', value: null }, + ].forEach(({ role, value }) => { + it(`should reject a falsey non-boolean ${role} = ${JSON.stringify(value)} as a boolean error`, () => { + const test_role_json = TEST_ADD_ROLE_OBJECT(); + test_role_json.permission = { [role]: value }; + const test_result = customValidate_rw(test_role_json, getAddRoleConstraints()); + + expect(test_result.statusCode).to.equal(400); + expect(test_result.http_resp_msg.main_permissions).to.include( + TEST_ROLE_PERMS_ERROR.SU_CU_ROLE_BOOLEAN_ERROR(role) + ); + }); + }); + it('NOMINAL - should return null for valid ALTER_ROLE object', () => { const test_result = customValidate_rw(TEST_ALTER_ROLE_OBJECT(), getAlterRoleConstraints()); expect(test_result).to.equal(null); diff --git a/utility/hdbTerms.ts b/utility/hdbTerms.ts index 2c1f04bfe..bba4fa3cb 100644 --- a/utility/hdbTerms.ts +++ b/utility/hdbTerms.ts @@ -83,11 +83,36 @@ export const LAUNCH_SERVICE_SCRIPTS = { MAIN: 'dist/bin/harper.js', } as const; -/** Specifies user role types */ +/** + * Special boolean role flags validated as role types (not database permissions). + * `cluster_user` belongs here alongside `super_user`: role validation checks both + * are booleans (SU_CU_ROLE_BOOLEAN_ERROR), and without it a non-boolean + * `cluster_user` falls through to database validation and reports a misleading + * "database 'cluster_user' does not exist" error (harper#1016). + */ export const ROLE_TYPES_ENUM = { SUPER_USER: 'super_user', + CLUSTER_USER: 'cluster_user', } as const; +/** + * Named permission flags that live in a role's `permission` object alongside + * per-database permission blocks, which are keyed by database name. A database + * named after one of these flags collides with it — the role parser cannot tell + * a `super_user` flag from a `super_user` database's permissions — producing + * undefined behavior, so these are rejected as database/schema identifiers + * (harper#1016). Keep in sync with `UserRoleNamedPermissions` in security/user.ts. + * `access` is deliberately NOT here: roles.ts strips it out of `permission` + * before persistence (scoped-delegation), so it never collides at runtime. + */ +export const RESERVED_DATABASE_NAMES = [ + 'super_user', + 'cluster_user', + 'structure_user', + 'operations', + '_expandedOperations', +] as const; + /** Email address for support requests */ export const HDB_SUPPORT_ADDRESS = 'support@harperdb.io'; diff --git a/validation/role_validation.ts b/validation/role_validation.ts index d49b862d5..d170db5ef 100644 --- a/validation/role_validation.ts +++ b/validation/role_validation.ts @@ -100,7 +100,11 @@ function customValidate(object, constraints) { } //check if cu or su values, if included, are booleans ROLE_TYPES.forEach((role) => { - if (object.permission[role as any] && !validate.isBoolean(object.permission[role as any])) { + // Gate on key presence, not truthiness: a falsey non-boolean (0, '', null) must still be + // rejected as a non-boolean. A truthiness gate would let those skip both this check AND the + // database-permission loop below (the key is a role type, so that loop excludes it), so an + // add/alter role would silently accept a value that violates the boolean contract. + if (role in object.permission && !validate.isBoolean(object.permission[role as any])) { addPermError(HDB_ERROR_MSGS.SU_CU_ROLE_BOOLEAN_ERROR(role as any), validationErrors, undefined, undefined); } });