fix(core): Correctness, robustness and cleanup from secret-field review

- Redact config args by definition only, not by whether the value looks encrypted,
  removing a corruption path for non-secret args whose value starts with enc:v1:.
- Add the operation code to SecretAccessInput; type customField entity as possibly
  undefined (quotes / default-resolved types).
- Remove the forbidden non-null assertion (lint error) and unify arg output shape.
- Tolerate a single undecryptable value: log and fall back to the placeholder rather
  than failing the whole list query (transformer and resolver).
- Give a clear error for a malformed ciphertext instead of a raw TypeError.
- Check the key-check write result in the verifier so a failed write is not silent.
- Reject secret:true with a defaultValue at registration.
- Remove the now-empty PaymentMethodEntityResolver.

Relates to #2648

Author: michaelbromley

packages/core/src/api/api-internal-modules.ts

@@ -69,7 +69,6 @@ import {
     PaymentAdminEntityResolver,
     PaymentEntityResolver,
 } from './resolvers/entity/payment-entity.resolver';
-import { PaymentMethodEntityResolver } from './resolvers/entity/payment-method-entity.resolver';
 import {
     ProductAdminEntityResolver,
     ProductEntityResolver,
@@ -177,7 +176,6 @@ export const adminEntityResolvers = [
     AdministratorEntityResolver,
     CustomerAdminEntityResolver,
     OrderAdminEntityResolver,
-    PaymentMethodEntityResolver,
     FulfillmentAdminEntityResolver,
     PaymentAdminEntityResolver,
     ProductVariantAdminEntityResolver,

packages/core/src/api/resolvers/entity/configurable-operation-entity.resolver.ts

@@ -4,6 +4,7 @@ import { REDACTED_SECRET_PLACEHOLDER } from '@vendure/common/lib/shared-constant
 import { GraphQLResolveInfo } from 'graphql';
 
 import { ConfigService } from '../../../config/config.service';
+import { Logger } from '../../../config/logger/vendure-logger';
 import { ConfigArgService } from '../../../service/helpers/config-arg/config-arg.service';
 import { RequestContext } from '../../common/request-context';
 import { Ctx } from '../../decorators/request-context.decorator';
@@ -15,9 +16,12 @@ import { Ctx } from '../../decorators/request-context.decorator';
  * config arg values are gated: a value is either decrypted (when the {@link SecretAccessStrategy}
  * permits) or replaced with a redaction placeholder, so no per-operation-type wiring is required.
  *
- * An arg is treated as secret if its definition declares `secret: true`, so that a value which is
- * not (or not yet) encrypted at rest — e.g. legacy plaintext written before the field was marked
- * secret — is still redacted rather than served in the clear.
+ * An arg is treated as secret based on its definition (`secret: true`), so that a value which is not
+ * (or not yet) encrypted at rest — e.g. legacy plaintext written before the field was marked secret —
+ * is still redacted rather than served in the clear. Conversely, a non-secret arg whose value happens
+ * to look like ciphertext is left untouched, so it is not accidentally redacted or run through
+ * `decrypt()`. If `secret: true` is later removed from a def, any encrypted values already stored are
+ * passed through unchanged until the operation is next saved.
  */
 @Resolver('ConfigurableOperation')
 export class ConfigurableOperationEntityResolver {
@@ -33,15 +37,14 @@ export class ConfigurableOperationEntityResolver {
         const owner = this.deriveOwner(info);
         const output: ConfigArg[] = [];
         for (const arg of operation.args) {
-            const isEncrypted = arg.value != null && encryptionStrategy?.isEncrypted(arg.value) === true;
-            const isSecret = isEncrypted || this.configArgService.hasSecretArg(operation.code, arg.name);
-            if (arg.value == null || !isSecret) {
+            if (arg.value == null || !this.configArgService.hasSecretArg(operation.code, arg.name)) {
                 output.push({ ...arg });
                 continue;
             }
             const canReveal = secretAccessStrategy
                 ? await secretAccessStrategy.canAccessSecret(ctx, {
                       kind: 'configArg',
+                      code: operation.code,
                       entityType: owner.entityType,
                       field: owner.field,
                       argName: arg.name,
@@ -50,11 +53,22 @@ export class ConfigurableOperationEntityResolver {
             let value: string;
             if (!canReveal) {
                 value = REDACTED_SECRET_PLACEHOLDER;
+            } else if (encryptionStrategy && encryptionStrategy.isEncrypted(arg.value)) {
+                try {
+                    value = encryptionStrategy.decrypt(arg.value);
+                } catch (e: any) {
+                    // A single arg that cannot be decrypted must not fail the whole query.
+                    Logger.error(
+                        `Failed to decrypt secret arg "${arg.name}" of operation "${operation.code}": ` +
+                            (e.message as string),
+                    );
+                    value = REDACTED_SECRET_PLACEHOLDER;
+                }
             } else {
-                // When revealing, decrypt if encrypted; a legacy plaintext value is returned as-is.
-                value = isEncrypted ? encryptionStrategy!.decrypt(arg.value) : arg.value;
+                // A legacy plaintext value is returned as-is.
+                value = arg.value;
             }
-            output.push({ name: arg.name, value });
+            output.push({ ...arg, value });
         }
         return output;
     }

packages/core/src/api/resolvers/entity/payment-method-entity.resolver.ts

@@ -88,6 +88,11 @@ export class DefaultEncryptionStrategy implements EncryptionStrategy {
         }
         const key = this.assertKey();
         const [ivPart, authTagPart, dataPart] = ciphertext.slice(CIPHERTEXT_PREFIX.length).split(':');
+        if (ivPart == null || authTagPart == null || dataPart == null) {
+            throw new Error(
+                'The value is not a well-formed ciphertext (expected `enc:v1:<iv>:<authTag>:<data>`).',
+            );
+        }
         try {
             const decipher = createDecipheriv(ALGORITHM, key, Buffer.from(ivPart, 'base64url'));
             decipher.setAuthTag(Buffer.from(authTagPart, 'base64url'));

packages/core/src/config/system/default-encryption-strategy.ts

@@ -30,12 +30,22 @@ export type SecretAccessInput =
           fieldName: string;
           /**
            * @description
-           * The entity instance carrying the custom field.
+           * The entity instance carrying the custom field. Available for the common case where the
+           * entity's `customFields` are resolved by the built-in entity resolver, but may be
+           * `undefined` for types whose `customFields` are resolved by GraphQL's default resolver
+           * (e.g. `ShippingMethodQuote`/`PaymentMethodQuote`, or some plugin-defined types).
            */
-          entity: VendureEntity;
+          entity: VendureEntity | undefined;
       }
     | {
           kind: 'configArg';
+          /**
+           * @description
+           * The code of the configurable operation the arg belongs to, e.g. the payment method
+           * handler or collection filter code. Always defined, and together with `argName` uniquely
+           * identifies which secret is being accessed.
+           */
+          code: string;
           /**
            * @description
            * The name of the entity type the operation belongs to, e.g. `'PaymentMethod'` or

packages/core/src/config/system/secret-access-strategy.ts

@@ -59,6 +59,12 @@ function registerCustomFieldsForEntity(
                             'with "list: true".',
                     );
                 }
+                if (defaultValue !== undefined) {
+                    throw new Error(
+                        `ERROR: The custom field "${customField.name}" cannot combine "secret: true" ` +
+                            'with a "defaultValue", because a column default would be stored unencrypted.',
+                    );
+                }
             }
             const instance = new ctor();
             const registerColumn = () => {

packages/core/src/entity/register-custom-entity-fields.ts

@@ -1,6 +1,8 @@
+import type { EncryptionStrategy } from '../config/system/encryption-strategy';
+import { REDACTED_SECRET_PLACEHOLDER } from '@vendure/common/lib/shared-constants';
 import { ValueTransformer } from 'typeorm';
 
-import type { EncryptionStrategy } from '../config/system/encryption-strategy';
+import { Logger } from '../config/logger/vendure-logger';
 
 /**
  * Decimal types are returned as strings (e.g. "20.00") by some DBs, e.g. MySQL & Postgres
@@ -39,7 +41,17 @@ export class EncryptedFieldTransformer implements ValueTransformer {
         const stringValue = String(value);
         // Only values produced by encrypt() may be passed to decrypt(). A legacy plaintext value,
         // written before the field was marked secret, is returned unchanged.
-        return strategy.isEncrypted(stringValue) ? strategy.decrypt(stringValue) : value;
+        if (!strategy.isEncrypted(stringValue)) {
+            return value;
+        }
+        try {
+            return strategy.decrypt(stringValue);
+        } catch (e: any) {
+            // A single row whose value cannot be decrypted (corrupted ciphertext, a manual edit, or a
+            // partial key change) must not fail the entire query. Log and fall back to the placeholder.
+            Logger.error(`Failed to decrypt a secret custom field value: ${e.message as string}`);
+            return REDACTED_SECRET_PLACEHOLDER;
+        }
     }
 
     private strategy(): EncryptionStrategy {

packages/core/src/entity/value-transformers.ts

@@ -79,11 +79,19 @@ export class EncryptionKeyVerifierService implements OnModuleInit, OnApplication
             // First use of an encryption key against this database: bind it to the current key.
             // The settings store value type does not accept a bare string, hence the cast (as in
             // the InstallationIdCollector).
-            await this.settingsStoreService.set(
+            const result = await this.settingsStoreService.set(
                 ctx,
                 SETTINGS_KEY,
                 encryptionStrategy.encrypt(KEY_CHECK_PLAINTEXT) as any,
             );
+            if (!result.result) {
+                // `set` catches internally and returns a result rather than throwing, so without this
+                // a failed write would silently leave the wrong-key guard un-armed on later boots.
+                Logger.warn(
+                    'Could not persist the encryption key check; the key-mismatch guard will not be ' +
+                        `active until it succeeds. ${result.error ?? ''}`,
+                );
+            }
             return;
         }
         let decrypted: string | undefined;

packages/core/src/service/helpers/encryption-key-verifier/encryption-key-verifier.service.ts

@@ -11,6 +11,7 @@ import { ActiveOrderService } from './helpers/active-order/active-order.service'
 import { ConfigArgService } from './helpers/config-arg/config-arg.service';
 import { CustomFieldRelationService } from './helpers/custom-field-relation/custom-field-relation.service';
 import { CustomerChannelAssignmentService } from './helpers/customer-channel-assignment/customer-channel-assignment.service';
+import { EncryptionKeyVerifierService } from './helpers/encryption-key-verifier/encryption-key-verifier.service';
 import { EntityDuplicatorService } from './helpers/entity-duplicator/entity-duplicator.service';
 import { EntityHydrator } from './helpers/entity-hydrator/entity-hydrator.service';
 import { EntitySlugService } from './helpers/entity-slug.service';
@@ -29,12 +30,11 @@ import { PaymentStateMachine } from './helpers/payment-state-machine/payment-sta
 import { ProductPriceApplicator } from './helpers/product-price-applicator/product-price-applicator';
 import { RefundStateMachine } from './helpers/refund-state-machine/refund-state-machine';
 import { RequestContextService } from './helpers/request-context/request-context.service';
-import { EncryptionKeyVerifierService } from './helpers/encryption-key-verifier/encryption-key-verifier.service';
 import { SettingsStoreService } from './helpers/settings-store/settings-store.service';
-import { StoredMediaService } from './helpers/stored-media/stored-media.service';
 import { ShippingCalculator } from './helpers/shipping-calculator/shipping-calculator';
 import { SlugValidator } from './helpers/slug-validator/slug-validator';
 import { SlugService } from './helpers/slug.service';
+import { StoredMediaService } from './helpers/stored-media/stored-media.service';
 import { TranslatableSaver } from './helpers/translatable-saver/translatable-saver';
 import { TranslatorService } from './helpers/translator/translator.service';
 import { VerificationTokenGenerator } from './helpers/verification-token-generator/verification-token-generator';