feat: allow secret approval reviewers to read secrets (#4411)

* feat: allow secret approval reviewers to read secrets

* feat: allow secret approval reviewers to read secrets

* fix: backfill migrations

* lint: fix

* revert: license file

* Update backend/src/db/migrations/20250824192801_backfill-secret-read-compat-flag.ts

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>

* fix: rename to `shouldCheckSecretPermission`

* lint: fix

---------

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
This commit is contained in:
Sid
2025-08-27 17:51:14 +05:30
committed by GitHub
parent bceaac844f
commit af2f21fe93
6 changed files with 107 additions and 16 deletions

View File

@@ -0,0 +1,19 @@
import { Knex } from "knex";
import { TableName } from "../schemas";
export async function up(knex: Knex): Promise<void> {
if (!(await knex.schema.hasColumn(TableName.SecretApprovalPolicy, "shouldCheckSecretPermission"))) {
await knex.schema.alterTable(TableName.SecretApprovalPolicy, (t) => {
t.boolean("shouldCheckSecretPermission").nullable();
});
}
}
export async function down(knex: Knex): Promise<void> {
if (await knex.schema.hasColumn(TableName.SecretApprovalPolicy, "shouldCheckSecretPermission")) {
await knex.schema.alterTable(TableName.SecretApprovalPolicy, (t) => {
t.dropColumn("shouldCheckSecretPermission");
});
}
}

View File

@@ -0,0 +1,29 @@
import { Knex } from "knex";
import { selectAllTableCols } from "@app/lib/knex";
import { TableName } from "../schemas";
const BATCH_SIZE = 100;
export async function up(knex: Knex): Promise<void> {
if (await knex.schema.hasColumn(TableName.SecretApprovalPolicy, "shouldCheckSecretPermission")) {
// find all existing SecretApprovalPolicy rows to backfill shouldCheckSecretPermission flag
const rows = await knex(TableName.SecretApprovalPolicy).select(selectAllTableCols(TableName.SecretApprovalPolicy));
if (rows.length > 0) {
for (let i = 0; i < rows.length; i += BATCH_SIZE) {
const batch = rows.slice(i, i + BATCH_SIZE);
// eslint-disable-next-line no-await-in-loop
await knex(TableName.SecretApprovalPolicy)
.whereIn(
"id",
batch.map((row) => row.id)
)
.update({ shouldCheckSecretPermission: true });
}
}
}
}
export async function down(): Promise<void> {}

View File

@@ -17,7 +17,8 @@ export const SecretApprovalPoliciesSchema = z.object({
updatedAt: z.date(),
enforcementLevel: z.string().default("hard"),
deletedAt: z.date().nullable().optional(),
allowedSelfApprovals: z.boolean().default(true)
allowedSelfApprovals: z.boolean().default(true),
shouldCheckSecretPermission: z.boolean().nullable().optional()
});
export type TSecretApprovalPolicies = z.infer<typeof SecretApprovalPoliciesSchema>;

View File

@@ -305,7 +305,8 @@ export const registerSecretApprovalRequestRouter = async (server: FastifyZodProv
secretPath: z.string().optional().nullable(),
enforcementLevel: z.string(),
deletedAt: z.date().nullish(),
allowedSelfApprovals: z.boolean()
allowedSelfApprovals: z.boolean(),
shouldCheckSecretPermission: z.boolean().nullable().optional()
}),
environment: z.string(),
statusChangedByUser: approvalRequestUser.optional(),

View File

@@ -180,7 +180,11 @@ export const secretApprovalRequestDALFactory = (db: TDbClient) => {
tx.ref("enforcementLevel").withSchema(TableName.SecretApprovalPolicy).as("policyEnforcementLevel"),
tx.ref("allowedSelfApprovals").withSchema(TableName.SecretApprovalPolicy).as("policyAllowedSelfApprovals"),
tx.ref("approvals").withSchema(TableName.SecretApprovalPolicy).as("policyApprovals"),
tx.ref("deletedAt").withSchema(TableName.SecretApprovalPolicy).as("policyDeletedAt")
tx.ref("deletedAt").withSchema(TableName.SecretApprovalPolicy).as("policyDeletedAt"),
tx
.ref("shouldCheckSecretPermission")
.withSchema(TableName.SecretApprovalPolicy)
.as("policySecretReadAccessCompat")
);
const findById = async (id: string, tx?: Knex) => {
@@ -220,7 +224,8 @@ export const secretApprovalRequestDALFactory = (db: TDbClient) => {
enforcementLevel: el.policyEnforcementLevel,
envId: el.policyEnvId,
deletedAt: el.policyDeletedAt,
allowedSelfApprovals: el.policyAllowedSelfApprovals
allowedSelfApprovals: el.policyAllowedSelfApprovals,
shouldCheckSecretPermission: el.policySecretReadAccessCompat
}
}),
childrenMapper: [

View File

@@ -281,13 +281,22 @@ export const secretApprovalRequestServiceFactory = ({
) {
throw new ForbiddenRequestError({ message: "User has insufficient privileges" });
}
const getHasSecretReadAccess = (environment: string, tags: { slug: string }[], secretPath?: string) => {
const canRead = hasSecretReadValueOrDescribePermission(permission, ProjectPermissionSecretActions.ReadValue, {
environment,
secretPath: secretPath || "/",
secretTags: tags.map((i) => i.slug)
});
return canRead;
const getHasSecretReadAccess = (
shouldCheckSecretPermission: boolean | null | undefined,
environment: string,
tags: { slug: string }[],
secretPath?: string
) => {
if (shouldCheckSecretPermission) {
const canRead = hasSecretReadValueOrDescribePermission(permission, ProjectPermissionSecretActions.ReadValue, {
environment,
secretPath: secretPath || "/",
secretTags: tags.map((i) => i.slug)
});
return canRead;
}
return true;
};
let secrets;
@@ -309,8 +318,18 @@ export const secretApprovalRequestServiceFactory = ({
version: el.version,
secretMetadata: el.secretMetadata as ResourceMetadataDTO,
isRotatedSecret: el.secret?.isRotatedSecret ?? false,
secretValueHidden: !getHasSecretReadAccess(secretApprovalRequest.environment, el.tags, secretPath?.[0]?.path),
secretValue: !getHasSecretReadAccess(secretApprovalRequest.environment, el.tags, secretPath?.[0]?.path)
secretValueHidden: !getHasSecretReadAccess(
secretApprovalRequest.policy.shouldCheckSecretPermission,
secretApprovalRequest.environment,
el.tags,
secretPath?.[0]?.path
),
secretValue: !getHasSecretReadAccess(
secretApprovalRequest.policy.shouldCheckSecretPermission,
secretApprovalRequest.environment,
el.tags,
secretPath?.[0]?.path
)
? INFISICAL_SECRET_VALUE_HIDDEN_MASK
: el.secret && el.secret.isRotatedSecret
? undefined
@@ -326,11 +345,17 @@ export const secretApprovalRequestServiceFactory = ({
id: el.secret.id,
version: el.secret.version,
secretValueHidden: !getHasSecretReadAccess(
secretApprovalRequest.policy.shouldCheckSecretPermission,
secretApprovalRequest.environment,
el.tags,
secretPath?.[0]?.path
),
secretValue: !getHasSecretReadAccess(secretApprovalRequest.environment, el.tags, secretPath?.[0]?.path)
secretValue: !getHasSecretReadAccess(
secretApprovalRequest.policy.shouldCheckSecretPermission,
secretApprovalRequest.environment,
el.tags,
secretPath?.[0]?.path
)
? INFISICAL_SECRET_VALUE_HIDDEN_MASK
: el.secret.encryptedValue
? secretManagerDecryptor({ cipherTextBlob: el.secret.encryptedValue }).toString()
@@ -346,11 +371,17 @@ export const secretApprovalRequestServiceFactory = ({
id: el.secretVersion.id,
version: el.secretVersion.version,
secretValueHidden: !getHasSecretReadAccess(
secretApprovalRequest.policy.shouldCheckSecretPermission,
secretApprovalRequest.environment,
el.tags,
secretPath?.[0]?.path
),
secretValue: !getHasSecretReadAccess(secretApprovalRequest.environment, el.tags, secretPath?.[0]?.path)
secretValue: !getHasSecretReadAccess(
secretApprovalRequest.policy.shouldCheckSecretPermission,
secretApprovalRequest.environment,
el.tags,
secretPath?.[0]?.path
)
? INFISICAL_SECRET_VALUE_HIDDEN_MASK
: el.secretVersion.encryptedValue
? secretManagerDecryptor({ cipherTextBlob: el.secretVersion.encryptedValue }).toString()
@@ -368,7 +399,12 @@ export const secretApprovalRequestServiceFactory = ({
const encryptedSecrets = await secretApprovalRequestSecretDAL.findByRequestId(secretApprovalRequest.id);
secrets = encryptedSecrets.map((el) => ({
...el,
secretValueHidden: !getHasSecretReadAccess(secretApprovalRequest.environment, el.tags, secretPath?.[0]?.path),
secretValueHidden: !getHasSecretReadAccess(
secretApprovalRequest.policy.shouldCheckSecretPermission,
secretApprovalRequest.environment,
el.tags,
secretPath?.[0]?.path
),
...decryptSecretWithBot(el, botKey),
secret: el.secret
? {