From 0dfea2a23a032be0900550ad99e655d033ec901a Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Sat, 1 Aug 2026 02:37:37 +0200 Subject: [PATCH 1/2] docs: inventory of all 131 cron jobs (#4555) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `docs/cron-jobs.md` lists every `@DfxCron` declaration with its interval, its process flag and its location: 131 jobs across 92 files and 33 areas. The list is derived by a balanced-paren scan of the decorator arguments, so multi-line declarations are included — a line-based match misses four of them. The parsed count is asserted against a raw text count of the decorator. Two things the list makes visible: - 21 jobs carry no process flag and therefore run unconditionally. Three of those are deliberate: the `ProcessService::resync*` jobs maintain the disabled set and the JWT denylists themselves, so making them switchable would let a configuration change disable the mechanism that reads configuration changes. The remaining 18 are an omission and are listed individually. - `CitreaBaseStrategy::checkPayInEntries` is declared on an abstract class. NestJS discovers providers rather than classes, so such a job is registered once per concrete subclass. There is one subclass today, so the runtime count still equals the declaration count. Documentation only. No source file is touched. --- CONTRIBUTING.md | 7 ++ docs/cron-jobs.md | 249 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 256 insertions(+) create mode 100644 docs/cron-jobs.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1a7212d758..262cd00f6d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -29,6 +29,7 @@ Every PR must include: 2. **Environment/Infrastructure updates** (config, environment variables) 3. **Service updates** (if DTOs/interfaces changed) 4. **Frontend synchronization** (if API contracts changed) +5. **Cron job inventory** (if a `@DfxCron` job was added, removed or re-scheduled) — [docs/cron-jobs.md](docs/cron-jobs.md) Missing any of these = changes requested. @@ -530,6 +531,12 @@ async processPayments(): Promise { } ``` +Declare a `process` flag unless the job maintains the disabled set itself. Without one the job +runs unconditionally and cannot be switched off without a deploy. + +[docs/cron-jobs.md](docs/cron-jobs.md) lists every scheduled job with its interval and flag. +**Adding, removing or re-scheduling a job must be reflected there in the same PR.** + Prefer longer intervals (15min) over aggressive polling (1min). Only use short intervals when truly needed. ### Await Discipline diff --git a/docs/cron-jobs.md b/docs/cron-jobs.md new file mode 100644 index 0000000000..ccb451d1ed --- /dev/null +++ b/docs/cron-jobs.md @@ -0,0 +1,249 @@ +# Cron jobs + +Every scheduled job this service runs: **131 `@DfxCron` declarations** across 92 files and 33 areas. + +## Columns + +| Column | Meaning | +| ------ | ------- | +| **Interval** | The `CronExpression` / `CustomCronExpression` the job is registered with | +| **Flag** | The `process:` kill switch that disables the job at runtime. `—` means the job has none and always runs | +| **Job** | Class and method | +| **File** | Path below `src/` | + +## Flags + +110 of the 131 jobs carry a `process` flag, 21 do not. A job with a flag can be switched off +without a deploy — `DfxCronService` skips it when the process appears in the disabled set, which +`ProcessService` refreshes from the `disabledProcesses` setting and the `DISABLED_PROCESSES` +environment variable every 30 seconds. + +A job **without** a flag runs unconditionally. That is deliberate for three of them — the +`ProcessService::resync*` jobs maintain the disabled set and the JWT denylists themselves, so +making them switchable would let a configuration change disable the mechanism that reads +configuration changes. For the remaining 18 it is simply an omission: + +| Job | Interval | +| --- | --- | +| `DexService::finalizePurchaseOrders` | 30 seconds | +| `ExchangeController::checkTrades` | 30 seconds | +| `AuthService::checkLists` | minute | +| `JwtRevocationSyncService::syncDeniedJwtAccounts` | minute | +| `TransactionController::checkLists` | minute | +| `UserDataService::processCleanupMailSecretCache` | minute | +| `TransactionHelper::updateCache` | 5 minutes | +| `RefService::checkRefs` | hour | +| `BuyService` / `SellService` / `SwapService` / `UserService` / `UserDataService` `::resetMonthlyVolumes` | 1st of month | +| `BuyService` / `SellService` / `SwapService` / `UserService` / `UserDataService` `::resetAnnualVolumes` | year | + +New jobs should declare a flag unless there is a reason like the one above. + +## Distribution + +| Interval | Jobs | +| -------- | ---: | +| second | 5 | +| 10 seconds | 3 | +| 30 seconds | 8 | +| minute | 49 | +| 5 minutes | 17 | +| 10 minutes | 15 | +| hour | 16 | +| day at 4am | 3 | +| day at 5am | 1 | +| day at 6am | 1 | +| day at 11pm | 1 | +| week | 1 | +| weekend | 1 | +| 1st day of month at midnight | 5 | +| year | 5 | + +Jobs by area: + +| Area | Jobs | Without flag | +| ---- | ---: | -----------: | +| `subdomains/generic/user` | 15 | 7 | +| `subdomains/core/monitoring` | 14 | — | +| `subdomains/core/accounting` | 13 | — | +| `subdomains/supporting/payin` | 12 | — | +| `integration/blockchain` | 6 | — | +| `subdomains/core/buy-crypto` | 6 | 4 | +| `subdomains/core/sell-crypto` | 5 | 2 | +| `subdomains/core/payment-link` | 4 | — | +| `subdomains/generic/kyc` | 4 | — | +| `subdomains/supporting/bank-tx` | 4 | — | +| `subdomains/supporting/bank` | 4 | — | +| `subdomains/supporting/fiat-output` | 4 | — | +| `subdomains/supporting/support-issue` | 4 | — | +| `shared` | 3 | 3 | +| `subdomains/core/liquidity-management` | 3 | — | +| `subdomains/core/referral` | 3 | 1 | +| `subdomains/core/trading` | 3 | — | +| `subdomains/supporting/payment` | 3 | 1 | +| `subdomains/supporting/pricing` | 3 | — | +| `integration/exchange` | 2 | 1 | +| `subdomains/core/custody` | 2 | — | +| `subdomains/supporting/log` | 2 | — | +| `subdomains/supporting/realunit` | 2 | — | +| `integration/binance-pay` | 1 | — | +| `subdomains/core/aml` | 1 | — | +| `subdomains/core/faucet-request` | 1 | — | +| `subdomains/core/history` | 1 | 1 | +| `subdomains/core/statistic` | 1 | — | +| `subdomains/generic/admin` | 1 | — | +| `subdomains/supporting/dex` | 1 | 1 | +| `subdomains/supporting/fiat-payin` | 1 | — | +| `subdomains/supporting/notification` | 1 | — | +| `subdomains/supporting/payout` | 1 | — | + +## How this list is produced + +Every `@DfxCron(` occurrence in `src/**/*.ts`. Decorator arguments are read by a balanced-paren +scan, so multi-line declarations are included — a line-based match misses four of them. The parsed +count is asserted against a raw text count of the decorator: **131 = 131**, no gap. Class and +method come from the enclosing `export class` (including `export abstract class`) and the +identifier following the decorator. + +## Known discrepancy + +`CitreaBaseStrategy::checkPayInEntries` is declared on an **abstract** class. NestJS discovers +providers rather than classes, so such a job is registered once per concrete subclass, not once per +declaration. There is currently one subclass, so the runtime count equals the declaration count — +but a second subclass would silently add another registration of the same job, sharing the flag and +the interval while running as an independent timer with its own lock. + +## Jobs + +| Interval | Flag | Job | File | +| -------- | ---- | --- | ---- | +| second | `PAY_IN` | `BitcoinStrategy::checkPayInEntries` | `subdomains/supporting/payin/strategies/register/impl/bitcoin.strategy.ts` | +| second | `PAY_IN` | `FiroStrategy::checkPayInEntries` | `subdomains/supporting/payin/strategies/register/impl/firo.strategy.ts` | +| second | `PAY_IN` | `MoneroStrategy::checkPayInEntries` | `subdomains/supporting/payin/strategies/register/impl/monero.strategy.ts` | +| second | `MONITOR_CONNECTION_POOL` | `MonitorConnectionPoolService::monitorConnectionPool` | `subdomains/core/monitoring/monitor-connection-pool.service.ts` | +| second | `PAY_IN` | `ZanoStrategy::checkPayInEntries` | `subdomains/supporting/payin/strategies/register/impl/zano.strategy.ts` | +| 10 seconds | `LIQUIDITY_MANAGEMENT` | `LiquidityManagementPipelineService::processPipelines` | `subdomains/core/liquidity-management/services/liquidity-management-pipeline.service.ts` | +| 10 seconds | `MONITOR_CONNECTION_POOL` | `MonitorConnectionPoolService::monitorConnectionPoolStatic` | `subdomains/core/monitoring/monitor-connection-pool.service.ts` | +| 10 seconds | `MONITOR_EVENT_LOOP` | `MonitorEventLoopService::monitorEventLoop` | `subdomains/core/monitoring/monitor-event-loop.service.ts` | +| 30 seconds | `LNURL_AUTH_CACHE` | `AuthLnUrlService::processCleanupAccessToken` | `subdomains/generic/user/models/auth/auth-lnurl.service.ts` | +| 30 seconds | `BANK_TX` | `BankTxService::checkBankTx` | `subdomains/supporting/bank-tx/bank-tx/services/bank-tx.service.ts` | +| 30 seconds | — | `DexService::finalizePurchaseOrders` | `subdomains/supporting/dex/services/dex.service.ts` | +| 30 seconds | — | `ExchangeController::checkTrades` | `integration/exchange/controllers/exchange.controller.ts` | +| 30 seconds | `PAY_OUT` | `PayoutService::processOrders` | `subdomains/supporting/payout/services/payout.service.ts` | +| 30 seconds | — | `ProcessService::resyncDeniedJwtAccounts` | `shared/services/process.service.ts` | +| 30 seconds | — | `ProcessService::resyncDeniedJwtAddresses` | `shared/services/process.service.ts` | +| 30 seconds | — | `ProcessService::resyncDisabledProcesses` | `shared/services/process.service.ts` | +| minute | `PAY_OUT` | `AdminService::completeLiquidityOrders` | `subdomains/generic/admin/admin.service.ts` | +| minute | `MONITORING` | `AmlObserver::fetch` | `subdomains/core/monitoring/observers/aml.observer.ts` | +| minute | — | `AuthService::checkLists` | `subdomains/generic/user/models/auth/auth.service.ts` | +| minute | `BANK_DATA_VERIFICATION` | `BankDataService::checkAndSetActive` | `subdomains/generic/user/models/bank-data/bank-data.service.ts` | +| minute | `MONITORING` | `BankObserver::fetch` | `subdomains/core/monitoring/observers/bank.observer.ts` | +| minute | `BANK_TX_RETURN_MAIL` | `BankTxReturnNotificationService::sendBankTxReturnMail` | `subdomains/supporting/bank-tx/bank-tx-return/bank-tx-return-notification.service.ts` | +| minute | `BUY_CRYPTO` | `BuyCryptoJobService::process` | `subdomains/core/buy-crypto/process/services/buy-crypto-job.service.ts` | +| minute | `BUY_FIAT` | `BuyFiatJobService::addFiatOutputs` | `subdomains/core/sell-crypto/process/services/buy-fiat-job.service.ts` | +| minute | `BUY_FIAT` | `BuyFiatJobService::checkCryptoPayIn` | `subdomains/core/sell-crypto/process/services/buy-fiat-job.service.ts` | +| minute | `BUY_FIAT_MAIL` | `BuyFiatNotificationService::sendNotificationMails` | `subdomains/core/sell-crypto/process/services/buy-fiat-notification.service.ts` | +| minute | `PAY_IN` | `CardanoStrategy::checkPayInEntries` | `subdomains/supporting/payin/strategies/register/impl/cardano.strategy.ts` | +| minute | `MONITORING` | `CheckoutObserver::fetch` | `subdomains/core/monitoring/observers/checkout.observer.ts` | +| minute | `PAY_IN` | `CitreaBaseStrategy::checkPayInEntries` | `subdomains/supporting/payin/strategies/register/impl/base/citrea.strategy.ts` | +| minute | `CUSTODY` | `CustodyJobService::handleOrders` | `subdomains/core/custody/services/custody-job.service.ts` | +| minute | `FIAT_OUTPUT` | `FiatOutputJobService::fillFiatOutput` | `subdomains/supporting/fiat-output/fiat-output-job.service.ts` | +| minute | `FIAT_PAY_IN` | `FiatPayInSyncService::syncCheckout` | `subdomains/supporting/fiat-payin/services/fiat-payin-sync.service.ts` | +| minute | `PAY_IN` | `InternetComputerStrategy::checkPayInEntries` | `subdomains/supporting/payin/strategies/register/impl/icp.strategy.ts` | +| minute | — | `JwtRevocationSyncService::syncDeniedJwtAccounts` | `subdomains/generic/user/models/user-data/jwt-revocation-sync.service.ts` | +| minute | `KYC` | `KycService::reviewKycSteps` | `subdomains/generic/kyc/services/kyc.service.ts` | +| minute | `LEDGER_BOOKING_BANK_TX` | `LedgerBookingJobService::runBankTx` | `subdomains/core/accounting/services/ledger-booking-job.service.ts` | +| minute | `LEDGER_BOOKING_BUY_CRYPTO` | `LedgerBookingJobService::runBuyCrypto` | `subdomains/core/accounting/services/ledger-booking-job.service.ts` | +| minute | `LEDGER_BOOKING_BUY_FIAT` | `LedgerBookingJobService::runBuyFiat` | `subdomains/core/accounting/services/ledger-booking-job.service.ts` | +| minute | `LEDGER_BOOKING_CRYPTO_INPUT` | `LedgerBookingJobService::runCryptoInput` | `subdomains/core/accounting/services/ledger-booking-job.service.ts` | +| minute | `LEDGER_BOOKING_EXCHANGE_TX` | `LedgerBookingJobService::runExchangeTx` | `subdomains/core/accounting/services/ledger-booking-job.service.ts` | +| minute | `LEDGER_BOOKING_LIQUIDITY_MANAGEMENT` | `LedgerBookingJobService::runLiquidityMgmt` | `subdomains/core/accounting/services/ledger-booking-job.service.ts` | +| minute | `LEDGER_BOOKING_LIQUIDITY_ORDER` | `LedgerBookingJobService::runLiquidityOrderDex` | `subdomains/core/accounting/services/ledger-booking-job.service.ts` | +| minute | `LEDGER_BOOKING_PAYOUT` | `LedgerBookingJobService::runPayoutOrder` | `subdomains/core/accounting/services/ledger-booking-job.service.ts` | +| minute | `LEDGER_BOOKING_TRADING_ORDER` | `LedgerBookingJobService::runTradingOrder` | `subdomains/core/accounting/services/ledger-booking-job.service.ts` | +| minute | `LIQUIDITY_MANAGEMENT_CHECK_BALANCES` | `LiquidityManagementService::checkLiquidityBalances` | `subdomains/core/liquidity-management/services/liquidity-management.service.ts` | +| minute | `MONITORING` | `LiquidityObserver::fetch` | `subdomains/core/monitoring/observers/liquidity.observer.ts` | +| minute | `TRADING_LOG` | `LogJobService::saveTradingLog` | `subdomains/supporting/log/log-job.service.ts` | +| minute | `MONITORING` | `NodeHealthObserver::fetch` | `subdomains/core/monitoring/observers/node-health.observer.ts` | +| minute | `ORGANIZATION_SYNC` | `OrganizationService::syncOrganization` | `subdomains/generic/user/models/organization/organization.service.ts` | +| minute | `PAY_IN` | `PayInService::checkConfirmations` | `subdomains/supporting/payin/services/payin.service.ts` | +| minute | `PAY_IN` | `PayInService::forwardPayInEntries` | `subdomains/supporting/payin/services/payin.service.ts` | +| minute | `PAY_IN` | `PayInService::returnPayInEntries` | `subdomains/supporting/payin/services/payin.service.ts` | +| minute | `PAYMENT_CONFIRMATIONS` | `PaymentCronService::checkTxConfirmations` | `subdomains/core/payment-link/services/payment-cron.service.ts` | +| minute | `PAYMENT_EXPIRATION` | `PaymentCronService::processExpiredPayments` | `subdomains/core/payment-link/services/payment-cron.service.ts` | +| minute | `UPDATE_BLOCKCHAIN_FEE` | `PaymentLinkFeeService::updateFees` | `subdomains/core/payment-link/services/payment-link-fee.service.ts` | +| minute | `REALUNIT_QUOTE_COMPLETION` | `RealUnitJobService::completeSettledQuotes` | `subdomains/supporting/realunit/realunit-job.service.ts` | +| minute | `SUPPORT_BOT` | `SupportIssueJobService::sendAutoResponses` | `subdomains/supporting/support-issue/services/support-issue-job.service.ts` | +| minute | `TFA_CACHE` | `TfaService::processCleanupSecretCache` | `subdomains/generic/kyc/services/tfa.service.ts` | +| minute | `TRADING` | `TradingJobService::processOrders` | `subdomains/core/trading/services/trading-job.service.ts` | +| minute | `TRADING` | `TradingJobService::processRules` | `subdomains/core/trading/services/trading-job.service.ts` | +| minute | — | `TransactionController::checkLists` | `subdomains/core/history/controllers/transaction.controller.ts` | +| minute | `TX_MAIL` | `TransactionNotificationService::sendNotificationMails` | `subdomains/supporting/payment/services/transaction-notification.service.ts` | +| minute | `USER_DATA` | `UserDataJobService::fillUserData` | `subdomains/generic/user/models/user-data/user-data-job.service.ts` | +| minute | — | `UserDataService::processCleanupMailSecretCache` | `subdomains/generic/user/models/user-data/user-data.service.ts` | +| minute | `USER` | `UserJobService::fillUser` | `subdomains/generic/user/models/user/user-job.service.ts` | +| 5 minutes | `PRICING` | `AssetPricesJobService::updatePaymentPrices` | `subdomains/supporting/pricing/services/asset-prices-job.service.ts` | +| 5 minutes | `LNURL_AUTH_CACHE` | `AuthLnUrlService::processCleanupAuthCache` | `subdomains/generic/user/models/auth/auth-lnurl.service.ts` | +| 5 minutes | `BANK_TX_RETURN` | `BankTxReturnService::fillBankTxReturn` | `subdomains/supporting/bank-tx/bank-tx-return/bank-tx-return.service.ts` | +| 5 minutes | `BANK_TX` | `BankTxService::enrichYapealTransactions` | `subdomains/supporting/bank-tx/bank-tx/services/bank-tx.service.ts` | +| 5 minutes | `BLOCKCHAIN_CONFIG_CHECK` | `BlockchainConfigCheckService::logUnconfiguredClients` | `integration/blockchain/shared/services/blockchain-config-check.service.ts` | +| 5 minutes | `EXCHANGE_TX_SYNC` | `ExchangeTxService::syncExchangeJob` | `integration/exchange/services/exchange-tx.service.ts` | +| 5 minutes | `CRYPTO_PAYOUT` | `FaucetRequestService::checkFaucetRequests` | `subdomains/core/faucet-request/services/faucet-request.service.ts` | +| 5 minutes | `LEDGER_COA_BOOTSTRAP` | `LedgerBookingJobService::runCoaBootstrap` | `subdomains/core/accounting/services/ledger-booking-job.service.ts` | +| 5 minutes | `LEDGER_CUTOVER` | `LedgerCutoverService::run` | `subdomains/core/accounting/services/ledger-cutover.service.ts` | +| 5 minutes | `LIMIT_REQUEST_MAIL` | `LimitRequestNotificationService::sendNotificationMails` | `subdomains/supporting/support-issue/services/limit-request-notification.service.ts` | +| 5 minutes | `LIQUIDITY_MANAGEMENT` | `LiquidityManagementRuleService::reactivateRules` | `subdomains/core/liquidity-management/services/liquidity-management-rule.service.ts` | +| 5 minutes | `PAY_IN_MAIL` | `PayInNotificationService::sendNotificationMails` | `subdomains/supporting/payin/services/payin-notification.service.ts` | +| 5 minutes | `REALUNIT_TRANSFER_RECONCILIATION` | `RealUnitJobService::reconcilePendingTransfers` | `subdomains/supporting/realunit/realunit-job.service.ts` | +| 5 minutes | `SUPPORT_BOT` | `SupportEscalationService::checkEscalations` | `subdomains/supporting/support-issue/services/support-escalation.service.ts` | +| 5 minutes | `TRADING` | `TradingJobService::reactivateRules` | `subdomains/core/trading/services/trading-job.service.ts` | +| 5 minutes | — | `TransactionHelper::updateCache` | `subdomains/supporting/payment/services/transaction-helper.ts` | +| 5 minutes | `WEBHOOK` | `WebhookNotificationService::sendWebhooks` | `subdomains/generic/user/services/webhook/webhook-notification.service.ts` | +| 10 minutes | `BANK_ACCOUNT` | `BankAccountService::reloadUncheckedBankAccounts` | `subdomains/supporting/bank/bank-account/bank-account.service.ts` | +| 10 minutes | `DEURO_LOG_INFO` | `DEuroService::processLogInfo` | `integration/blockchain/deuro/deuro.service.ts` | +| 10 minutes | `MONITORING` | `ExchangeObserver::fetch` | `subdomains/core/monitoring/observers/exchange.observer.ts` | +| 10 minutes | `MONITORING` | `ExternalServicesObserver::fetch` | `subdomains/core/monitoring/observers/external-services.observer.ts` | +| 10 minutes | `BLOCKCHAIN_FEE_UPDATE` | `FeeService::updateBlockchainFees` | `subdomains/supporting/payment/services/fee.service.ts` | +| 10 minutes | `FRANKENCOIN_LOG_INFO` | `FrankencoinService::processLogInfo` | `integration/blockchain/frankencoin/frankencoin.service.ts` | +| 10 minutes | `JUICE_LOG_INFO` | `JuiceService::processLogInfo` | `integration/blockchain/juice/juice.service.ts` | +| 10 minutes | `MONITORING` | `NodeBalanceObserver::fetch` | `subdomains/core/monitoring/observers/node-balance.observer.ts` | +| 10 minutes | `MAIL_RETRY` | `NotificationJobService::resendUncompletedMails` | `subdomains/supporting/notification/services/notification-job.service.ts` | +| 10 minutes | `PAY_IN` | `PayInService::updateFailedPayments` | `subdomains/supporting/payin/services/payin.service.ts` | +| 10 minutes | `MONITORING` | `PaymentObserver::fetch` | `subdomains/core/monitoring/observers/payment.observer.ts` | +| 10 minutes | `MONITORING` | `RealUnitW2wGasObserver::fetch` | `subdomains/core/monitoring/observers/realunit-w2w-gas.observer.ts` | +| 10 minutes | `REF_PAYOUT` | `RefRewardJobService::processPendingRefRewards` | `subdomains/core/referral/reward/services/ref-reward-job.service.ts` | +| 10 minutes | `MONITORING` | `UserObserver::fetch` | `subdomains/core/monitoring/observers/user.observer.ts` | +| 10 minutes | `ZANO_ASSET_WHITELIST` | `ZanoService::setupAssetWhitelist` | `integration/blockchain/zano/services/zano.service.ts` | +| hour | `PRICING` | `AssetPricesJobService::updatePrices` | `subdomains/supporting/pricing/services/asset-prices-job.service.ts` | +| hour | `BANK_ACCOUNT` | `BankAccountService::reloadErrorBankAccounts` | `subdomains/supporting/bank/bank-account/bank-account.service.ts` | +| hour | `BINANCE_PAY_CERTIFICATES_UPDATE` | `BinancePayService::updateCertificates` | `integration/binance-pay/services/binance-pay.service.ts` | +| hour | `BUY_CRYPTO_AGGREGATION` | `BuyCryptoJobService::checkAggregatingTransactions` | `subdomains/core/buy-crypto/process/services/buy-crypto-job.service.ts` | +| hour | `ASSET_DECIMALS` | `EvmDecimalsService::setDecimals` | `integration/blockchain/shared/evm/evm-decimals.service.ts` | +| hour | `FIAT_OUTPUT` | `FiatOutputFrickService::checkFrickOrderStatus` | `subdomains/supporting/fiat-output/fiat-output-frick.service.ts` | +| hour | `FIAT_OUTPUT` | `FiatOutputJobService::checkOlkypayOrderStatus` | `subdomains/supporting/fiat-output/fiat-output-job.service.ts` | +| hour | `FIAT_OUTPUT` | `FiatOutputJobService::generateReports` | `subdomains/supporting/fiat-output/fiat-output-job.service.ts` | +| hour | `PRICING` | `FiatPricesService::updatePrices` | `subdomains/supporting/pricing/services/fiat-prices.service.ts` | +| hour | `KYC_MAIL` | `KycNotificationService::sendNotificationMails` | `subdomains/generic/kyc/services/kyc-notification.service.ts` | +| hour | `PAYMENT_FORWARDING` | `PaymentCronService::forwardDeposits` | `subdomains/core/payment-link/services/payment-cron.service.ts` | +| hour | — | `RefService::checkRefs` | `subdomains/core/referral/process/ref.service.ts` | +| hour | `UPDATE_STATISTIC` | `StatisticService::doUpdate` | `subdomains/core/statistic/statistic.service.ts` | +| hour | `SUPPORT_BOT` | `SupportIssueJobService::autoOnHold` | `subdomains/supporting/support-issue/services/support-issue-job.service.ts` | +| hour | `BLACK_SQUAD_MAIL` | `UserDataNotificationService::sendNotificationMails` | `subdomains/generic/user/models/user-data/user-data-notification.service.ts` | +| hour | `VIRTUAL_IBAN_FRICK_ISSUANCE_RECONCILIATION` | `VirtualIbanFrickIssuanceReconciliationService::reconcileRetiredIssuanceReferences` | `subdomains/supporting/bank/virtual-iban/virtual-iban-frick-issuance-reconciliation.service.ts` | +| day at 4am | `CUSTODY` | `CustodyJobService::resetExpiredConfirmedOrders` | `subdomains/core/custody/services/custody-job.service.ts` | +| day at 4am | `KYC` | `KycService::checkIdentSteps` | `subdomains/generic/kyc/services/kyc.service.ts` | +| day at 4am | `LEDGER_MARK_TO_MARKET` | `LedgerMarkToMarketService::run` | `subdomains/core/accounting/services/ledger-mark-to-market.service.ts` | +| day at 5am | `LEDGER_RECONCILIATION` | `LedgerReconciliationService::run` | `subdomains/core/accounting/services/ledger-reconciliation.service.ts` | +| day at 6am | `REF_PAYOUT` | `RefRewardJobService::createPendingRefRewards` | `subdomains/core/referral/reward/services/ref-reward-job.service.ts` | +| day at 11pm | `LOG_CLEANUP` | `LogService::cleanup` | `subdomains/supporting/log/log.service.ts` | +| week | `BANK_ACCOUNT` | `BankAccountService::checkFailedBankAccounts` | `subdomains/supporting/bank/bank-account/bank-account.service.ts` | +| weekend | `SANCTION_SYNC` | `SanctionService::syncList` | `subdomains/core/aml/services/sanction.service.ts` | +| 1st day of month at midnight | — | `BuyService::resetMonthlyVolumes` | `subdomains/core/buy-crypto/routes/buy/buy.service.ts` | +| 1st day of month at midnight | — | `SellService::resetMonthlyVolumes` | `subdomains/core/sell-crypto/route/sell.service.ts` | +| 1st day of month at midnight | — | `SwapService::resetMonthlyVolumes` | `subdomains/core/buy-crypto/routes/swap/swap.service.ts` | +| 1st day of month at midnight | — | `UserDataService::resetMonthlyVolumes` | `subdomains/generic/user/models/user-data/user-data.service.ts` | +| 1st day of month at midnight | — | `UserService::resetMonthlyVolumes` | `subdomains/generic/user/models/user/user.service.ts` | +| year | — | `BuyService::resetAnnualVolumes` | `subdomains/core/buy-crypto/routes/buy/buy.service.ts` | +| year | — | `SellService::resetAnnualVolumes` | `subdomains/core/sell-crypto/route/sell.service.ts` | +| year | — | `SwapService::resetAnnualVolumes` | `subdomains/core/buy-crypto/routes/swap/swap.service.ts` | +| year | — | `UserDataService::resetAnnualVolumes` | `subdomains/generic/user/models/user-data/user-data.service.ts` | +| year | — | `UserService::resetAnnualVolumes` | `subdomains/generic/user/models/user/user.service.ts` | From 2e761a487ac701f9df2caf68e4b046084a73b374 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Sat, 1 Aug 2026 03:12:52 +0200 Subject: [PATCH 2/2] Record client-side errors as ERROR log lines (#4550) * Record client-side errors as ERROR log lines Frontend errors were visible only in the customer browser console, so a failure that many customers hit left no trace in log monitoring at all. Adds POST /v1/log/clientError, which writes the reported error as an ERROR line through DfxLogger. Unauthenticated on purpose: the errors worth catching happen before or without a session. Rate limited per client, with every field length-capped. Because the endpoint is unauthenticated, every field is attacker-controlled. Sensitive query parameters are redacted and line breaks are collapsed, so a forged payload can neither leak credentials into the log nor open a line that reads like a log entry of its own. * Harden the client error log against forged and sensitive input Redaction was anchored on a leading ? or &, which only holds if the value is a well-formed query string. message and stack are free text, so a credential reached the log unredacted whenever it sat at the start of the value, behind a fragment, behind a matrix parameter, inside a percent-encoded URL, or in a compound parameter name such as accessToken. Now matched on a word boundary with the name as a substring, and = or its percent-encoded form. The free text also sat in front of the key=value context using the same syntax, so a payload could forge context fields without needing a line break at all - collapsing line breaks did not prevent that. Context now comes first and every value is embedded as a quoted string, which also turns control characters and ANSI escapes into escape sequences instead of letting them repaint a terminal that tails the log. Adds a process-wide budget: per-client throttling bounds one caller, but nothing bounded a distributed flood of the ERROR stream that alerting reads. Over budget, reports are dropped and the gap is recorded rather than left silent. Also reads the header through the shared constant, trims the stack like every other string field, and pins the route mounting and rate-limit wiring in a controller spec. * Redact by stripping URL parameters instead of matching their names Matching parameter names lost a round to every new encoding: after the first pass closed the prefix-anchored gaps, a colon separator, JSON-style quoting, a double-encoded =, and a percent-encoded character inside the name itself all still carried a credential into the log. That search space has no end, so the approach goes rather than the next patch. Every URL in the text is now cut to its origin and path, which drops the query and the fragment whole. How a parameter is named, spelled or encoded stops mattering, because none of it survives. Bare assignments outside a URL are still masked, but only for names that are unambiguously secret. That second list also had to shrink. As a substring, code matched statusCode, errorCode, countryCode and currencyCode, and key matched keyboardLayout and monkey - the report would have arrived stripped of exactly the fields worth reading. Tests now pin both directions: twelve shapes a credential can arrive in, and ten diagnostic values that must survive, including the asset URL of a failed chunk. The redaction no longer claims more than it does: a secret with no recognisable name outside a URL is indistinguishable from a diagnostic string, and the comment says so. * Cut parameters from any path, not only from a URL with a scheme The parameter stripping required a literal scheme://, which left three shapes untouched: a relative path - the normal shape for an app calling its own API - a protocol-relative one, and a fully percent-encoded one. All three fell back on the name list, and that list had just been narrowed, so the gap reopened exactly where the earlier rounds had closed it. A path now qualifies on its own, and the separators are recognised percent-encoded as well. A credential can also sit in the authority (scheme://user:secret@host), where it has no separator in front of it and survived the cut untouched. That part is now dropped too. The colon separator added in the previous commit was matching secret names as substrings, which made it fire on ordinary prose: `401 Unauthorized: invalid credentials` and `Gmail: no app found` lost the half of the sentence that says what happened - and those sentences are what a frontend error usually consists of. A bare colon now requires the name to stand on its own word boundaries; the substring form stays for `name=value` and for quoted JSON keys, where prose does not collide. Tests pin all of it, in both directions: sixteen shapes a credential can arrive in, and twenty diagnostic values and sentences that have to survive. * Take the cost and the blind spots out of the assignment matching The name pattern sat in front of the separator, so it had to be retried at every position of a long word. A field of repeated word characters within the accepted length took seconds of CPU, on the one thread that serves every other request - a single post was enough to stall the process. The match is now anchored on the separator and the name read from the text in front of it, which takes the same field from seconds to well under a millisecond. A test pins it. The budget check also moved ahead of the sanitizing, so a request that will be dropped no longer pays for work nobody reads. Splitting the separators in the previous commit had cost the colon form its compound names: accessToken, sessionId, refreshToken and apiSecret all passed through unmasked, which is exactly the shape a frontend produces when it puts a caught error into a message. A colon now matches a secret name that is a part of the name in its own right, so those are covered while Unauthorized and Gmail still are not. auth is gone from the list and spelled out as authorization: as a name part it means authentication rather than a credential, and it would have taken authMethod with it. And a value is now ended by whitespace alone - a comma inside a password used to leave the rest of it in the log. The comment no longer claims the redaction covers every name: outside a URL it depends on recognising one, and it now says so. * Stop one assignment from swallowing the secret behind it Ending a value at whitespace alone was the wrong half of the trade. A harmless assignment matched first, its value ate everything up to the next space - and because a global replace never re-reads what a match consumed, the assignment behind it was never examined. debug: a=1;password= went into the log untouched, and so did the same shape from a serialised form, a referrer, or a comma-joined context dump. Those are ordinary frontend error payloads, not crafted ones. A value now ends at any joiner. The cost is the tail of a value that legitimately contains one: a password with a comma in it is masked in part rather than in full. That is the safe direction of the two, and it is pinned by a test so it does not get "fixed" back into swallowing. The comment claimed the gap was limited to names the code does not know. It now also states that masking ends where the value ends, even under a name it does know. * Type the segment list so the build stops inferring never String.match returns null when nothing matches, so the fallback made the type a union of RegExpMatchArray and an empty array literal, and the compiler settled on never for the element. It surfaced only in CI, where the compile runs through ts-jest over the whole project rather than the file alone. --- .../__tests__/client-error.controller.spec.ts | 54 +++ .../__tests__/client-error.service.spec.ts | 322 ++++++++++++++++++ .../supporting/log/client-error.controller.ts | 36 ++ .../supporting/log/client-error.service.ts | 183 ++++++++++ .../log/dto/create-client-error.dto.ts | 43 +++ src/subdomains/supporting/log/log.module.ts | 6 +- 6 files changed, 642 insertions(+), 2 deletions(-) create mode 100644 src/subdomains/supporting/log/__tests__/client-error.controller.spec.ts create mode 100644 src/subdomains/supporting/log/__tests__/client-error.service.spec.ts create mode 100644 src/subdomains/supporting/log/client-error.controller.ts create mode 100644 src/subdomains/supporting/log/client-error.service.ts create mode 100644 src/subdomains/supporting/log/dto/create-client-error.dto.ts diff --git a/src/subdomains/supporting/log/__tests__/client-error.controller.spec.ts b/src/subdomains/supporting/log/__tests__/client-error.controller.spec.ts new file mode 100644 index 0000000000..49597cffb3 --- /dev/null +++ b/src/subdomains/supporting/log/__tests__/client-error.controller.spec.ts @@ -0,0 +1,54 @@ +import { createMock, DeepMocked } from '@golevelup/ts-jest'; +import { GUARDS_METADATA, METHOD_METADATA, PATH_METADATA } from '@nestjs/common/constants'; +import { RequestMethod } from '@nestjs/common/enums'; +import { THROTTLER_LIMIT, THROTTLER_TTL } from '@nestjs/throttler/dist/throttler.constants'; +import { RateLimitGuard } from 'src/shared/auth/rate-limit.guard'; +import { ClientErrorController } from '../client-error.controller'; +import { ClientErrorService } from '../client-error.service'; +import { CreateClientErrorDto } from '../dto/create-client-error.dto'; + +// This endpoint is deliberately unauthenticated, so the rate limit is the only thing standing +// between a public route and unbounded log writes. That wiring is worth pinning. +describe('ClientErrorController', () => { + let controller: ClientErrorController; + let service: DeepMocked; + + const dto: CreateClientErrorDto = Object.assign(new CreateClientErrorDto(), { message: 'boom' }); + + beforeEach(() => { + service = createMock(); + controller = new ClientErrorController(service); + }); + + it('passes the reported error and the request context to the service', () => { + controller.logError(dto, 'dfx-services', 'Mozilla/5.0'); + + expect(service.logError).toHaveBeenCalledWith(dto, 'dfx-services', 'Mozilla/5.0'); + }); + + it('accepts a report without client or user agent', () => { + controller.logError(dto); + + expect(service.logError).toHaveBeenCalledWith(dto, undefined, undefined); + }); + + // --- ROUTING & SECURITY METADATA --- // + + const handler = ClientErrorController.prototype.logError; + + it('is mounted as POST log/clientError', () => { + expect(Reflect.getMetadata(PATH_METADATA, ClientErrorController)).toBe('log/clientError'); + expect(Reflect.getMetadata(METHOD_METADATA, handler)).toBe(RequestMethod.POST); + }); + + it('guards the route with RateLimitGuard', () => { + expect(Reflect.getMetadata(GUARDS_METADATA, handler)).toEqual([RateLimitGuard]); + }); + + it('carries a route-level throttle, which is what gives the guard a limit at all', () => { + // RateLimitGuard resolves `routeOrClassLimit || this.options.limit`, and ThrottlerModule.forRoot() + // is registered without options - so without this decorator nothing would be throttled. + expect(Reflect.getMetadata(THROTTLER_LIMIT, handler)).toBe(20); + expect(Reflect.getMetadata(THROTTLER_TTL, handler)).toBe(60); + }); +}); diff --git a/src/subdomains/supporting/log/__tests__/client-error.service.spec.ts b/src/subdomains/supporting/log/__tests__/client-error.service.spec.ts new file mode 100644 index 0000000000..435306d9de --- /dev/null +++ b/src/subdomains/supporting/log/__tests__/client-error.service.spec.ts @@ -0,0 +1,322 @@ +import { DfxLogger } from 'src/shared/services/dfx-logger'; +import { ClientErrorService } from '../client-error.service'; +import { CreateClientErrorDto } from '../dto/create-client-error.dto'; + +const TOKEN = 'eyJhbGciOiJIUzI1NiJ9.abc'; + +describe('ClientErrorService', () => { + let service: ClientErrorService; + let error: jest.SpyInstance; + + function dto(values: Partial = {}): CreateClientErrorDto { + return Object.assign(new CreateClientErrorDto(), { message: 'Loading chunk 42 failed', ...values }); + } + + function loggedLine(): string { + return error.mock.calls[0][0]; + } + + beforeEach(() => { + service = new ClientErrorService(); + error = jest.spyOn(DfxLogger.prototype, 'error').mockImplementation(); + }); + + afterEach(() => jest.restoreAllMocks()); + + it('logs the error at ERROR level', () => { + service.logError(dto({ type: 'ChunkLoadError' })); + + expect(error).toHaveBeenCalledTimes(1); + expect(loggedLine()).toContain('message="Loading chunk 42 failed"'); + expect(loggedLine()).toContain('type="ChunkLoadError"'); + }); + + it('logs client, route, version and user agent', () => { + service.logError(dto({ route: '/buy', version: '1.2.3' }), 'dfx-services', 'Mozilla/5.0'); + + expect(loggedLine()).toContain('client="dfx-services"'); + expect(loggedLine()).toContain('route="/buy"'); + expect(loggedLine()).toContain('version="1.2.3"'); + expect(loggedLine()).toContain('userAgent="Mozilla/5.0"'); + }); + + it('logs absent context as an empty value', () => { + service.logError(dto()); + + expect(loggedLine()).toContain('client=""'); + expect(loggedLine()).toContain('route=""'); + expect(loggedLine()).toContain('version=""'); + expect(loggedLine()).toContain('userAgent=""'); + }); + + it('appends the stack when given', () => { + service.logError(dto({ stack: 'at BuyScreen (main.js:1:2)' })); + + expect(loggedLine()).toContain('stack="at BuyScreen (main.js:1:2)"'); + }); + + it('omits the stack field when no stack is given', () => { + service.logError(dto()); + + expect(loggedLine()).not.toContain('stack='); + }); + + // --- REDACTING --- // + // The endpoint is unauthenticated, so every field is attacker-controlled. A URL is cut to its + // path, which is what makes the encoding of a parameter irrelevant; matching parameter names + // alone loses a round to every new encoding. + + it.each([ + ['a query string', `GET https://app.example.com/buy?session=${TOKEN}&asset=BTC failed`], + ['a fragment', `https://app.example.com/cb#session=${TOKEN}`], + ['a matrix parameter', `https://app.example.com/buy;session=${TOKEN}`], + ['a percent-encoded parameter', `https://app.example.com/buy?session%3D${TOKEN}`], + ['a double-encoded parameter', `https://app.example.com/buy?session%253D${TOKEN}`], + ['a percent-encoded parameter name', `https://app.example.com/buy?sess%69on=${TOKEN}`], + ['a parameter with no recognisable name', `https://app.example.com/cb?x=${TOKEN}`], + ])('drops %s from a URL', (_case, message) => { + service.logError(dto({ message })); + + expect(loggedLine()).not.toContain(TOKEN); + }); + + // A URL does not need a scheme to carry a credential. A relative path is the normal shape for an + // app calling its own API, which makes this the common case rather than the exotic one. + it.each([ + ['a relative path', `GET /buy?address=${TOKEN} failed`], + ['a relative path with no recognisable name', `GET /cb?x=${TOKEN}`], + ['a protocol-relative URL', `//app.example.com/buy?walletAddress=${TOKEN}`], + ['a fully percent-encoded URL', `https%3A%2F%2Fapp.example.com%2Fbuy%3Faddress%3D${TOKEN}`], + ])('drops parameters from %s', (_case, message) => { + service.logError(dto({ message })); + + expect(loggedLine()).not.toContain(TOKEN); + }); + + // Credentials in the authority sit in front of every separator, so cutting at the query misses + // them entirely. + it('drops a credential embedded in the authority', () => { + service.logError(dto({ message: `https://admin:${TOKEN}@app.example.com/buy` })); + + expect(loggedLine()).not.toContain(TOKEN); + }); + + it.each([ + ['at the start of the value', `session=${TOKEN} lookup failed`], + ['behind a colon', `session: ${TOKEN}`], + ['in JSON', `{"token":"${TOKEN}"}`], + ['in a compound name', `accessToken=${TOKEN}`], + ['in a compound name behind a colon', `accessToken: ${TOKEN}`], + ['in a compound name with the secret part first', `sessionId: ${TOKEN}`], + ['in a snake_case name', `access_token=${TOKEN}`], + ['in an all-caps name', `SESSIONID=${TOKEN}`], + ['behind an authorization scheme', `authorization: Bearer ${TOKEN}`], + // A harmless assignment must not swallow the one behind it: these are the shapes a serialised + // form, a referrer or a debug dump arrives in, and a global replace never re-reads what a + // match already consumed. + ['behind an unrelated assignment', `debug: a=1;password=${TOKEN}`], + ['behind an unrelated query parameter', `Referrer: page=checkout&sessionToken=${TOKEN}`], + ['between unrelated form fields', `formData: name=John&password=${TOKEN}&city=Zurich`], + ['behind a comma-joined pair', `state: step=2,token=${TOKEN}`], + ['in a quoted value', `session="${TOKEN} more"`], + ['regardless of case', `Signature=${TOKEN}`], + ])('masks a bare secret assignment %s', (_case, message) => { + service.logError(dto({ message })); + + expect(loggedLine()).toContain(''); + expect(loggedLine()).not.toContain(TOKEN); + }); + + it.each(['session', 'signature', 'password', 'secret', 'token', 'otp', 'jwt', 'authorization', 'mail', 'apikey'])( + 'masks the %s assignment', + (name) => { + service.logError(dto({ message: `${name}=${TOKEN}` })); + + expect(loggedLine()).not.toContain(TOKEN); + }, + ); + + // `auth` on its own names authentication, not a credential. Listing it would take authMethod and + // OAuthProvider with it, so `authorization` is spelled out instead. + it('does not treat auth as a secret name on its own', () => { + service.logError(dto({ message: 'authMethod=MetaMask authProvider=walletconnect' })); + + expect(loggedLine()).toContain('authMethod=MetaMask'); + expect(loggedLine()).toContain('authProvider=walletconnect'); + }); + + // The documented limit, pinned so it is not "fixed" back into the swallowing behaviour above: + // a value ends at a joiner, so a secret that contains one keeps its tail. Masked in part beats + // the alternative, where an unrelated assignment in front of it hides it entirely. + it('masks only up to the joiner when a secret value contains one', () => { + service.logError(dto({ message: `password=hunter2,${TOKEN}` })); + + expect(loggedLine()).toContain('password='); + expect(loggedLine()).toContain(TOKEN); + }); + + it('redacts a credential carried in a stack', () => { + service.logError(dto({ stack: `at load (https://app.example.com/buy?signature=${TOKEN})` })); + + expect(loggedLine()).not.toContain(TOKEN); + }); + + // The counterpart failure: a name list broad enough to catch everything also redacts the fields + // that make a report worth reading. These are the ones a frontend error actually carries. + it.each([ + 'statusCode=502', + 'errorCode=E_TIMEOUT', + 'countryCode=CH', + 'currencyCode=CHF', + 'zipCode=8000', + 'keyboardLayout=qwerty', + 'monkey=banana', + 'asset=BTC', + 'amount=300', + 'chunkId=738', + ])('keeps the diagnostic value %s', (value) => { + service.logError(dto({ message: `failed with ${value}` })); + + expect(loggedLine()).toContain(value); + }); + + // Ordinary prose puts a colon after words that contain a secret name as a substring. Matching + // those would eat the half of the sentence that says what actually happened — and these are the + // sentences a frontend error most often consists of. + it.each([ + '401 Unauthorized: invalid credentials', + 'Error: Unauthorized: Session expired', + 'Failed to open in Gmail: no app found', + 'authMethod: MetaMask', + 'OAuthProvider: google', + 'TypeError: x is not a function', + ])('keeps the prose "%s"', (message) => { + service.logError(dto({ message })); + + expect(loggedLine()).toContain(message); + }); + + // Cutting a URL at its parameters also cuts a regex literal at a `?` or `#` inside it. The + // sentence around it stays readable, which is the trade-off taken here — pinned so a later + // change to the pattern does not widen the loss unnoticed. + it('keeps the sentence around a regex literal, even though the pattern itself is cut', () => { + service.logError(dto({ message: "Invalid email, expected /^\\S+?@\\S+$/ but got 'foo'" })); + + expect(loggedLine()).toContain('Invalid email, expected'); + expect(loggedLine()).toContain("but got 'foo'"); + }); + + // The endpoint takes free text from anyone, and Node runs it on the one thread that serves every + // other request. A pattern that backtracks over long words turns a single post into a stall. + it('sanitizes a full-length field without measurable cost', () => { + const worstCase = 'token-'.repeat(666); // 3996 chars, just inside the stack field limit + + const start = process.hrtime.bigint(); + service.logError(dto({ stack: worstCase })); + const elapsedMs = Number(process.hrtime.bigint() - start) / 1e6; + + expect(elapsedMs).toBeLessThan(100); + }); + + it('keeps the asset path of a failed chunk, which is the point of the report', () => { + service.logError(dto({ message: 'Loading chunk 738 failed (missing: https://app.example.com/static/js/738.js)' })); + + expect(loggedLine()).toContain('https://app.example.com/static/js/738.js'); + }); + + it('discards the query string, fragment and matrix parameters of a route', () => { + service.logError(dto({ route: `/buy;session=${TOKEN}?asset=BTC#top` })); + + expect(loggedLine()).toContain('route="/buy"'); + expect(loggedLine()).not.toContain(TOKEN); + }); + + // --- LOG INTEGRITY --- // + + it('quotes the free text so it cannot forge a log line', () => { + service.logError(dto({ message: 'real\n[Nest] 7 - ERROR [PayoutService] forged' })); + + expect(loggedLine()).not.toContain('\n'); + expect(loggedLine()).toContain('message="real\\n[Nest] 7 - ERROR [PayoutService] forged"'); + }); + + // Without quoting, this reads as context fields of its own and a log query cannot tell the + // forged ones from the genuine ones that follow. + it('quotes the free text so it cannot forge context fields', () => { + service.logError(dto({ message: 'boom route=/internal client=support trace_id=deadbeef' })); + + expect(loggedLine()).toContain('message="boom route=/internal client=support trace_id=deadbeef"'); + expect(loggedLine()).toContain('route=""'); + }); + + it('escapes control characters so a payload cannot repaint a terminal tailing the log', () => { + service.logError(dto({ stack: '\u001b[2J\u001b[31mFAKE CRITICAL\u001b[0m' })); + + expect(loggedLine()).not.toContain('\u001b'); + expect(loggedLine()).toContain('\\u001b'); + }); + + it.each([ + ['line separator', '\u2028'], + ['paragraph separator', '\u2029'], + ['next line', '\u0085'], + ['delete', '\u007f'], + ])('replaces the %s character, which survives string escaping', (_name, char) => { + service.logError(dto({ message: `a${char}b` })); + + expect(loggedLine()).toContain('message="a b"'); + }); + + it('escapes a quote so it cannot close the field early', () => { + service.logError(dto({ message: 'boom" route="/internal' })); + + expect(loggedLine()).toContain('\\"'); + expect(loggedLine()).toContain('route=""'); + }); + + // --- BUDGET --- // + // Per-IP throttling bounds one client. Without a ceiling across all of them, a distributed flood + // would bury genuine incidents in the same ERROR stream that alerting reads. + + it('logs every report while within the budget', () => { + for (let i = 0; i < 120; i++) service.logError(dto()); + + expect(error).toHaveBeenCalledTimes(120); + }); + + it('drops reports beyond the budget instead of flooding the stream', () => { + for (let i = 0; i < 130; i++) service.logError(dto()); + + expect(error).toHaveBeenCalledTimes(120); + }); + + it('reports how many were dropped once the window rolls over', () => { + jest.useFakeTimers(); + try { + for (let i = 0; i < 130; i++) service.logError(dto()); + jest.advanceTimersByTime(60001); + + service.logError(dto()); + + const lines = error.mock.calls.map((c) => c[0] as string); + expect(lines).toContain('Client error reporting over budget: 10 reports dropped'); + } finally { + jest.useRealTimers(); + } + }); + + it('logs again after the window rolls over', () => { + jest.useFakeTimers(); + try { + for (let i = 0; i < 130; i++) service.logError(dto()); + error.mockClear(); + jest.advanceTimersByTime(60001); + + service.logError(dto({ message: 'after the window' })); + + expect(error.mock.calls.some((c) => (c[0] as string).includes('after the window'))).toBe(true); + } finally { + jest.useRealTimers(); + } + }); +}); diff --git a/src/subdomains/supporting/log/client-error.controller.ts b/src/subdomains/supporting/log/client-error.controller.ts new file mode 100644 index 0000000000..aa8a9e34c0 --- /dev/null +++ b/src/subdomains/supporting/log/client-error.controller.ts @@ -0,0 +1,36 @@ +import { Body, Controller, Headers, HttpCode, HttpStatus, Post, UseGuards } from '@nestjs/common'; +import { ApiOperation, ApiTags } from '@nestjs/swagger'; +import { Throttle } from '@nestjs/throttler'; +import { RateLimitGuard } from 'src/shared/auth/rate-limit.guard'; +import { CLIENT_HEADER } from 'src/shared/utils/request-client'; +import { ClientErrorService } from './client-error.service'; +import { CreateClientErrorDto } from './dto/create-client-error.dto'; + +@ApiTags('log') +@Controller('log/clientError') +export class ClientErrorController { + constructor(private readonly clientErrorService: ClientErrorService) {} + + @Post() + @ApiOperation({ + summary: 'Report a frontend error', + description: + 'Records a client-side error as an ERROR log line so it becomes visible in log monitoring. ' + + 'Unauthenticated on purpose: the errors worth catching happen before or without a session.', + }) + // RateLimitGuard buckets by /24 (IPv4) or /64 (IPv6), so customers sharing a NAT share this + // budget and can crowd each other out. Accepted: an error report is diagnostic, not a customer + // action, and the alternative — no per-client limit at all — is worse. The service holds a + // second, process-wide budget, because a per-client limit alone does not bound a distributed + // flood of the ERROR stream. + @UseGuards(RateLimitGuard) + @Throttle(20, 60) + @HttpCode(HttpStatus.NO_CONTENT) + logError( + @Body() dto: CreateClientErrorDto, + @Headers(CLIENT_HEADER) client?: string, + @Headers('user-agent') userAgent?: string, + ): void { + this.clientErrorService.logError(dto, client, userAgent); + } +} diff --git a/src/subdomains/supporting/log/client-error.service.ts b/src/subdomains/supporting/log/client-error.service.ts new file mode 100644 index 0000000000..7750470f80 --- /dev/null +++ b/src/subdomains/supporting/log/client-error.service.ts @@ -0,0 +1,183 @@ +import { Injectable } from '@nestjs/common'; +import { DfxLogger } from 'src/shared/services/dfx-logger'; +import { CreateClientErrorDto } from './dto/create-client-error.dto'; + +// Anything shaped like a URL or a path is cut down to the part before its parameters. This is what +// carries the redaction: the query and the fragment are dropped whole, so it makes no difference +// how a parameter is named, spelled or encoded. Matching parameter names instead loses a round to +// every new encoding — percent-encoded, double-encoded, an encoded character inside the name. +// +// Anchored on the slash rather than on a scheme, which also covers the shapes that carry no +// scheme: a relative path — the normal shape for an app calling its own API — and a +// protocol-relative one. Anchoring matters for cost too: a scheme pattern has to be retried at +// every position of a long word, and this endpoint takes input from anyone. +const URL_LIKE_REGEX = /(?:\/|%2f)[^\s"'<>]*/gi; + +// Where the meaningful part of a URL ends, percent-encoded or not: ? # ; +const PARAMETER_START = /[?#;]|%3[fb]|%23/i; + +// A credential in the authority (//user:secret@host) has no separator in front of it and would +// survive the cut above. +const USERINFO_REGEX = /^(\/\/)[^/\s@]*@/; + +// A bare assignment outside a URL is still worth masking — but only for names that are +// unambiguously secret. A broad list fails the other way round: `code` would redact statusCode and +// countryCode, `key` would redact keyboardLayout, and the diagnostic value this endpoint exists +// for would go with them. +// `auth` is deliberately absent and spelled out instead: as a name part it means authentication, +// not a credential, and would take `authMethod` with it — while `Unauthorized` is not a name part +// at all. +const SECRET_NAMES = [ + 'session', + 'signature', + 'password', + 'secret', + 'token', + 'otp', + 'jwt', + 'authorization', + 'mail', + 'email', + 'apikey', +]; + +// Anchored on the separator, with the name read from the text in front of it. A name pattern +// placed before the separator has to be retried at every position of a long word, which is +// quadratic on input this endpoint accepts from anyone. +// The scheme of an `authorization: Bearer ` is matched separately, so that the credential +// after it is redacted rather than just the word naming the scheme. +// +// A value ends at any joiner, not only at whitespace. Letting it run to the next space would make +// a harmless assignment swallow the one behind it — `a=1;password=secret` matches at `a=`, the +// value eats the rest, and because a global replace never re-reads what a match consumed, the +// password is never examined. Stopping early costs the tail of a value that legitimately contains +// a comma; that leaves part of it masked instead of none of it. +const ASSIGNMENT_REGEX = /(=|%3D|:|%3A)(\s*)(["']?)((?:bearer|basic|digest|token)\s+)?([^\s"'=:,&;]*)/gi; +const NAME_BEFORE_REGEX = /([\w-]+)(["']?)\s*$/; + +// Splits a name into its parts: accessToken, session_id and api-key all yield their components. +const NAME_SEGMENT_REGEX = /[A-Z]?[a-z]+|[A-Z]+(?![a-z])|\d+/g; + +// How far back to look for the name of an assignment. Longer than any realistic parameter name. +const NAME_LOOKBEHIND = 64; + +// JSON string escaping covers everything below U+0020, where the ordinary line breaks and the +// ANSI escape live. These sit above it, survive the escaping, and still break a line or move a +// cursor in terminals and log viewers. +const EXOTIC_LINE_BREAKS = /[\u0085\u007f\u2028\u2029]/g; + +// Budget for one process and one minute. The endpoint is unauthenticated, and per-IP throttling +// only bounds a single client: without a ceiling across all of them, a distributed flood would +// bury genuine incidents in the same ERROR stream that alerting reads. +const LOG_BUDGET_PER_MINUTE = 120; +const BUDGET_WINDOW = 60000; + +@Injectable() +export class ClientErrorService { + private readonly logger = new DfxLogger(ClientErrorService); + + private windowStart = 0; + private windowCount = 0; + private suppressedCount = 0; + + logError(dto: CreateClientErrorDto, client?: string, userAgent?: string): void { + // Checked before the fields are built, so a flood cannot buy sanitizing work it never uses. + if (!this.isWithinBudget()) return; + + const { message, type, stack, route, version } = dto; + + // Context first, free text last and quoted: message, type and stack are attacker-controlled + // and would otherwise be indistinguishable from the key=value context a log query parses. + const fields = [ + `client=${ClientErrorService.quote(client)}`, + `route=${ClientErrorService.quote(ClientErrorService.toPath(route))}`, + `version=${ClientErrorService.quote(version)}`, + `userAgent=${ClientErrorService.quote(userAgent)}`, + `type=${ClientErrorService.quote(type)}`, + `message=${ClientErrorService.quote(message)}`, + ]; + if (stack) fields.push(`stack=${ClientErrorService.quote(stack)}`); + + this.logger.error(`Client error: ${fields.join(' ')}`); + } + + // --- BUDGET --- // + + private isWithinBudget(): boolean { + const now = Date.now(); + + if (now - this.windowStart >= BUDGET_WINDOW) { + const suppressed = this.suppressedCount; + + this.windowStart = now; + this.windowCount = 0; + this.suppressedCount = 0; + + // Report the gap rather than leaving a silent hole in the record. + if (suppressed) this.logger.error(`Client error reporting over budget: ${suppressed} reports dropped`); + } + + if (this.windowCount >= LOG_BUDGET_PER_MINUTE) { + this.suppressedCount++; + return false; + } + + this.windowCount++; + return true; + } + + // --- SANITIZING --- // + + // Reduces a URL or path to the part in front of its parameters, and drops any credential embedded + // in the authority. The query carries the session and the signature the frontend authenticates + // with; a matrix parameter (;key=value) and a percent-encoded separator carry them just as well. + private static toPath(value?: string): string | undefined { + return value?.split(PARAMETER_START)[0].replace(USERINFO_REGEX, '$1'); + } + + // A colon appears all over ordinary prose, so there a secret name only counts as a part of the + // name in its own right: `accessToken:` and `sessionId:` are matched, while `Unauthorized:` and + // `Gmail:` are not — and those sentences are what a frontend error usually consists of. In front + // of an equals sign, or quoted as a JSON key, no sentence collides, so a plain substring is the + // safer choice there: it also catches spellings that have no parts to split, such as SESSIONID. + private static isSecretName(name: string, quoted: boolean, separator: string): boolean { + const isProseSeparator = !quoted && (separator === ':' || separator.toLowerCase() === '%3a'); + if (!isProseSeparator) return SECRET_NAMES.some((secret) => name.toLowerCase().includes(secret)); + + const segments: string[] = name.match(NAME_SEGMENT_REGEX) ?? []; + + return segments.some((segment) => SECRET_NAMES.includes(segment.toLowerCase())); + } + + // Strips URL parameters, masks bare secret assignments, then embeds the value as a JSON string. + // The quoting is what makes the line unforgeable: line breaks, control characters and ANSI + // escapes come out as escape sequences, so a payload can neither open a log line of its own nor + // repaint a terminal that is tailing the log. + // + // What is guaranteed: a credential carried the way this app carries one — as a parameter of a URL + // or a path — cannot reach the log, whatever it is called and however it is encoded, because the + // parameters are dropped rather than inspected. + // + // What is not: outside a URL, masking depends on recognising the name in front of the value, so + // a secret under an unknown name, or under no name at all, is indistinguishable from an ordinary + // diagnostic string and is logged. Even under a known name the masking ends where the value + // ends — a value that itself contains a comma or an equals sign keeps its tail. + private static quote(value?: string): string { + if (value == null) return '""'; + + const redacted = value + .replace(URL_LIKE_REGEX, (match) => ClientErrorService.toPath(match) ?? match) + .replace(ASSIGNMENT_REGEX, (match, separator, space, quote, scheme, assigned, offset: number, whole: string) => { + if (!assigned) return match; + + const name = NAME_BEFORE_REGEX.exec(whole.slice(Math.max(0, offset - NAME_LOOKBEHIND), offset)); + + return name && ClientErrorService.isSecretName(name[1], Boolean(name[2]), separator) + ? `${separator}${space}${quote}${scheme ?? ''}` + : match; + }) + .replace(EXOTIC_LINE_BREAKS, ' '); + + return JSON.stringify(redacted); + } +} diff --git a/src/subdomains/supporting/log/dto/create-client-error.dto.ts b/src/subdomains/supporting/log/dto/create-client-error.dto.ts new file mode 100644 index 0000000000..189f8fc61f --- /dev/null +++ b/src/subdomains/supporting/log/dto/create-client-error.dto.ts @@ -0,0 +1,43 @@ +import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger'; +import { Transform } from 'class-transformer'; +import { IsNotEmpty, IsOptional, IsString, MaxLength } from 'class-validator'; +import { Util } from 'src/shared/utils/util'; + +export class CreateClientErrorDto { + @ApiProperty({ description: 'Error message' }) + @IsNotEmpty() + @IsString() + @Transform(Util.trim) + @MaxLength(500) + message: string; + + @ApiPropertyOptional({ description: 'Error type (e.g. ChunkLoadError, TypeError)' }) + @IsOptional() + @IsString() + @Transform(Util.trim) + @MaxLength(100) + type?: string; + + @ApiPropertyOptional({ description: 'Stack trace' }) + @IsOptional() + @IsString() + @Transform(Util.trim) + @MaxLength(4000) + stack?: string; + + @ApiPropertyOptional({ + description: 'Route the error occurred on (query string, fragment and matrix parameters are discarded server-side)', + }) + @IsOptional() + @IsString() + @Transform(Util.trim) + @MaxLength(500) + route?: string; + + @ApiPropertyOptional({ description: 'Frontend build version' }) + @IsOptional() + @IsString() + @Transform(Util.trim) + @MaxLength(50) + version?: string; +} diff --git a/src/subdomains/supporting/log/log.module.ts b/src/subdomains/supporting/log/log.module.ts index ea4fb08b6f..271dde52b7 100644 --- a/src/subdomains/supporting/log/log.module.ts +++ b/src/subdomains/supporting/log/log.module.ts @@ -1,6 +1,8 @@ import { Module } from '@nestjs/common'; import { TypeOrmModule } from '@nestjs/typeorm'; import { SharedModule } from 'src/shared/shared.module'; +import { ClientErrorController } from './client-error.controller'; +import { ClientErrorService } from './client-error.service'; import { LogController } from './log.controller'; import { Log } from './log.entity'; import { LogRepository } from './log.repository'; @@ -8,8 +10,8 @@ import { LogService } from './log.service'; @Module({ imports: [TypeOrmModule.forFeature([Log]), SharedModule], - controllers: [LogController], - providers: [LogRepository, LogService], + controllers: [LogController, ClientErrorController], + providers: [LogRepository, LogService, ClientErrorService], exports: [LogService], }) export class LogModule {}