From bb0b4758d8b0a0a012c72e335db11f4337a0e0be Mon Sep 17 00:00:00 2001 From: 128na Date: Mon, 22 Jun 2026 19:14:40 +0900 Subject: [PATCH 1/5] Add known-risks ledger and fix Assurance Audit findings No threat model or known-risks ledger existed in this repo; an Assurance Audit was run across scrape/extract, Notion sync, secret handling, and public search/feed to establish one and close the highest-priority gaps. - docs/known-risks.md: continuing ledger of protected behaviors (A1-D4) with control strength, test count, and Status per the audit rubric. - docs/assurance-audit-2026-06-22.md: frozen snapshot of the initial audit. Fixes applied in priority order: - Extract handlers no longer delete RawPage on extraction failure (destroyed the only scraped copy on a transient bug); failures are now isolated via an extract_failed_at flag, cleared on the next success. - .env.example now defaults APP_DEBUG=false to close the debug-page secret-leak path. - Added SecretScrubber + a Monolog tap + Discord converter scrubbing so known secrets (Notion token, Discord webhook, DB passwords) are masked before reaching log files or the Discord webhook. - Notion sync now isolates per-item failures in SyncAction instead of aborting the whole batch on one API error. - Added regression tests for previously-untested-but-sound controls (per-site fault isolation, extract idempotency, URL uniqueness, PageResource field whitelist, API error response shape). Co-Authored-By: Claude Sonnet 4.6 --- .env.example | 2 +- app/Actions/Extract/Japan/Handler.php | 7 +- app/Actions/Extract/MarkExtractFailed.php | 35 ++++++ app/Actions/Extract/Portal/Handler.php | 7 +- app/Actions/Extract/Twitrans/Handler.php | 7 +- app/Actions/Logging/ConvertDiscord.php | 14 +++ app/Actions/Logging/SecretScrubber.php | 87 +++++++++++++ app/Actions/SyncNotion/SyncAction.php | 117 +++++++++++------- app/Logging/RedactSecretsProcessor.php | 27 ++++ app/Logging/RedactSecretsTap.php | 26 ++++ app/Models/RawPage.php | 4 + config/logging.php | 3 + ...d_extract_failed_at_to_raw_pages_table.php | 25 ++++ docs/assurance-audit-2026-06-22.md | 59 +++++++++ docs/known-risks.md | 53 ++++++++ .../Extract/Japan/HandlerIsolationTest.php | 62 ++++++++++ .../Actions/Extract/MarkExtractFailedTest.php | 45 +++++++ .../Extract/UpdateOrCreatePageTest.php | 31 +++++ .../Actions/Logging/SecretScrubberTest.php | 65 ++++++++++ .../Scrape/Japan/HandlerFailureTest.php | 48 +++++++ .../Actions/SyncNotion/SyncActionTest.php | 60 +++++++++ tests/Feature/Http/ApiErrorResponseTest.php | 34 +++++ .../Http/Resources/PageResourceTest.php | 60 +++++++++ .../Logging/RedactSecretsProcessorTest.php | 41 ++++++ .../Models/UrlUniqueConstraintTest.php | 32 +++++ 25 files changed, 897 insertions(+), 54 deletions(-) create mode 100644 app/Actions/Extract/MarkExtractFailed.php create mode 100644 app/Actions/Logging/SecretScrubber.php create mode 100644 app/Logging/RedactSecretsProcessor.php create mode 100644 app/Logging/RedactSecretsTap.php create mode 100644 database/migrations/2026_06_22_000000_add_extract_failed_at_to_raw_pages_table.php create mode 100644 docs/assurance-audit-2026-06-22.md create mode 100644 docs/known-risks.md create mode 100644 tests/Feature/Actions/Extract/Japan/HandlerIsolationTest.php create mode 100644 tests/Feature/Actions/Extract/MarkExtractFailedTest.php create mode 100644 tests/Feature/Actions/Extract/UpdateOrCreatePageTest.php create mode 100644 tests/Feature/Actions/Logging/SecretScrubberTest.php create mode 100644 tests/Feature/Actions/Scrape/Japan/HandlerFailureTest.php create mode 100644 tests/Feature/Http/ApiErrorResponseTest.php create mode 100644 tests/Feature/Http/Resources/PageResourceTest.php create mode 100644 tests/Feature/Logging/RedactSecretsProcessorTest.php create mode 100644 tests/Feature/Models/UrlUniqueConstraintTest.php diff --git a/.env.example b/.env.example index 2f51b65..3b9dc2f 100644 --- a/.env.example +++ b/.env.example @@ -1,7 +1,7 @@ APP_NAME=Simutransアドオン横断検索 APP_ENV=local APP_KEY= -APP_DEBUG=true +APP_DEBUG=false APP_URL=http://localhost LOG_CHANNEL=stack diff --git a/app/Actions/Extract/Japan/Handler.php b/app/Actions/Extract/Japan/Handler.php index e4646a3..2d0ce0c 100644 --- a/app/Actions/Extract/Japan/Handler.php +++ b/app/Actions/Extract/Japan/Handler.php @@ -6,6 +6,7 @@ use App\Actions\Extract\ChunkRawPages; use App\Actions\Extract\HandlerInterface; +use App\Actions\Extract\MarkExtractFailed; use App\Actions\Extract\SyncPak; use App\Actions\Extract\UpdateOrCreatePage; use App\Enums\SiteName; @@ -23,6 +24,7 @@ public function __construct( private ExtractContents $extractContents, private UpdateOrCreatePage $updateOrCreatePage, private SyncPak $syncPak, + private MarkExtractFailed $markExtractFailed, ) {} #[\Override] @@ -49,9 +51,10 @@ public function __invoke(LoggerInterface $logger): void ($this->syncPak)($page, $contents['paks']); } + + $this->markExtractFailed->clear($rawPage); } catch (\Throwable $th) { - $logger->error('failed', [$rawPage->url, $th]); - $rawPage->delete(); + ($this->markExtractFailed)($logger, $rawPage, $th); } } }); diff --git a/app/Actions/Extract/MarkExtractFailed.php b/app/Actions/Extract/MarkExtractFailed.php new file mode 100644 index 0000000..c740df4 --- /dev/null +++ b/app/Actions/Extract/MarkExtractFailed.php @@ -0,0 +1,35 @@ +error('failed', [$rawPage->url, $throwable]); + $rawPage->update(['extract_failed_at' => CarbonImmutable::now()]); + } + + /** + * 抽出が成功したら過去の失敗フラグをクリアする（自己回復）。 + */ + public function clear(RawPage $rawPage): void + { + if ($rawPage->extract_failed_at !== null) { + $rawPage->update(['extract_failed_at' => null]); + } + } +} diff --git a/app/Actions/Extract/Portal/Handler.php b/app/Actions/Extract/Portal/Handler.php index 30764e2..e7a1dd4 100644 --- a/app/Actions/Extract/Portal/Handler.php +++ b/app/Actions/Extract/Portal/Handler.php @@ -6,6 +6,7 @@ use App\Actions\Extract\ChunkRawPages; use App\Actions\Extract\HandlerInterface; +use App\Actions\Extract\MarkExtractFailed; use App\Actions\Extract\SyncPak; use App\Actions\Extract\UpdateOrCreatePage; use App\Enums\SiteName; @@ -23,6 +24,7 @@ public function __construct( private ExtractContents $extractContents, private UpdateOrCreatePage $updateOrCreatePage, private SyncPak $syncPak, + private MarkExtractFailed $markExtractFailed, ) {} #[\Override] @@ -51,9 +53,10 @@ public function __invoke(LoggerInterface $logger): void ($this->syncPak)($page, $contents['paks']); } + + $this->markExtractFailed->clear($rawPage); } catch (\Throwable $th) { - $logger->error('failed', [$rawPage->url, $th]); - $rawPage->delete(); + ($this->markExtractFailed)($logger, $rawPage, $th); } } }); diff --git a/app/Actions/Extract/Twitrans/Handler.php b/app/Actions/Extract/Twitrans/Handler.php index 42a359b..2ee7ebc 100644 --- a/app/Actions/Extract/Twitrans/Handler.php +++ b/app/Actions/Extract/Twitrans/Handler.php @@ -6,6 +6,7 @@ use App\Actions\Extract\ChunkRawPages; use App\Actions\Extract\HandlerInterface; +use App\Actions\Extract\MarkExtractFailed; use App\Actions\Extract\SyncPak; use App\Actions\Extract\UpdateOrCreatePage; use App\Enums\SiteName; @@ -23,6 +24,7 @@ public function __construct( private ExtractContents $extractContents, private UpdateOrCreatePage $updateOrCreatePage, private SyncPak $syncPak, + private MarkExtractFailed $markExtractFailed, ) {} #[\Override] @@ -49,9 +51,10 @@ public function __invoke(LoggerInterface $logger): void ($this->syncPak)($page, $contents['paks']); } + + $this->markExtractFailed->clear($rawPage); } catch (\Throwable $th) { - $logger->error('failed', [$rawPage->url, $th]); - $rawPage->delete(); + ($this->markExtractFailed)($logger, $rawPage, $th); } } }); diff --git a/app/Actions/Logging/ConvertDiscord.php b/app/Actions/Logging/ConvertDiscord.php index d8a4f08..626b011 100644 --- a/app/Actions/Logging/ConvertDiscord.php +++ b/app/Actions/Logging/ConvertDiscord.php @@ -4,12 +4,18 @@ namespace App\Actions\Logging; +use Illuminate\Contracts\Config\Repository; use MarvinLabs\DiscordLogger\Converters\SimpleRecordConverter; use MarvinLabs\DiscordLogger\Discord\Embed; use MarvinLabs\DiscordLogger\Discord\Message; final class ConvertDiscord extends SimpleRecordConverter { + public function __construct(Repository $config, private readonly SecretScrubber $secretScrubber) + { + parent::__construct($config); + } + /** * @param array{datetime:\DateTime,level_name:int,message:string,context:array} $record */ @@ -17,7 +23,15 @@ final class ConvertDiscord extends SimpleRecordConverter protected function addMessageContent(Message $message, array $record): void { try { + // Discord は外部サービスへ送出されるため、組み立て前に機密値を伏字化する。 + $record['message'] = $this->secretScrubber->scrub($record['message']); + $record['context'] = $this->secretScrubber->scrubArray($record['context']); + $stacktrace = $this->getStacktrace($record); + if ($stacktrace !== null) { + $stacktrace = $this->secretScrubber->scrub($stacktrace); + } + if (! in_array($stacktrace, [null, '', '0'], true)) { $this->makeErrorMessage($message, $record, $stacktrace); } else { diff --git a/app/Actions/Logging/SecretScrubber.php b/app/Actions/Logging/SecretScrubber.php new file mode 100644 index 0000000..f8d7e4e --- /dev/null +++ b/app/Actions/Logging/SecretScrubber.php @@ -0,0 +1,87 @@ +secrets(); + if ($secrets === []) { + return $value; + } + + return str_replace($secrets, self::MASK, $value); + } + + /** + * @param array $context + * @return array + */ + public function scrubArray(array $context): array + { + $secrets = $this->secrets(); + if ($secrets === []) { + return $context; + } + + /** @var array $scrubbed */ + $scrubbed = $this->walk($context, $secrets); + + return $scrubbed; + } + + /** + * @param list $secrets + */ + private function walk(mixed $value, array $secrets): mixed + { + if (is_string($value)) { + return str_replace($secrets, self::MASK, $value); + } + + if (is_array($value)) { + return array_map(fn (mixed $item): mixed => $this->walk($item, $secrets), $value); + } + + if ($value instanceof \Throwable) { + // 例外オブジェクトはメッセージ + トレース文字列に展開してから伏字化する。 + return str_replace($secrets, self::MASK, (string) $value); + } + + return $value; + } + + /** + * 伏字化対象の機密値一覧（空値は対象から除外する）。 + * + * @return list + */ + private function secrets(): array + { + $candidates = [ + (string) Config::string('services.notion.secret', ''), + (string) Config::string('logging.channels.discord.url', ''), + (string) Config::string('database.connections.mysql.password', ''), + (string) Config::string('database.connections.portal.password', ''), + ]; + + return array_values(array_unique(array_filter( + $candidates, + fn (string $value): bool => $value !== '', + ))); + } +} diff --git a/app/Actions/SyncNotion/SyncAction.php b/app/Actions/SyncNotion/SyncAction.php index 826e6a7..dea95be 100644 --- a/app/Actions/SyncNotion/SyncAction.php +++ b/app/Actions/SyncNotion/SyncAction.php @@ -36,75 +36,98 @@ public function __invoke(string $databaseId, int $limit): void $pages = Page::query()->with('paks')->orderBy('last_modified', 'desc')->limit($limit)->get(); - $this->deleteOldNotionPages($pages, $notionPages); - $this->addNewNotionPages($database, $pages, $notionPages); + // 1 件の Notion API エラーでバッチ全体を止めず、項目単位で隔離して継続する。 + $failed = $this->deleteOldNotionPages($pages, $notionPages) + + $this->addNewNotionPages($database, $pages, $notionPages); + + if ($failed > 0) { + logger()->error('[NotionService] sync completed with failures', ['failed' => $failed]); + } } /** * @param Collection $pages * @param Collection $notionPages + * @return int 失敗件数 */ - private function deleteOldNotionPages(Collection $pages, Collection $notionPages): void + private function deleteOldNotionPages(Collection $pages, Collection $notionPages): int { + $failed = 0; foreach ($notionPages as $notionPage) { - $url = $this->getUrlProp($notionPage); - $page = $pages->first(fn (Page $page): bool => $page->url === $url); - if (! $page) { - logger('[NotionService]delete', ['url' => $url]); - $this->notion->pages()->delete($notionPage); + try { + $url = $this->getUrlProp($notionPage); + $page = $pages->first(fn (Page $page): bool => $page->url === $url); + if (! $page) { + logger('[NotionService]delete', ['url' => $url]); + $this->notion->pages()->delete($notionPage); + } + } catch (\Throwable $th) { + $failed++; + logger()->error('[NotionService] delete failed', ['url' => $url ?? null, $th]); } } + + return $failed; } /** * @param Collection $pages * @param Collection $notionPages + * @return int 失敗件数 */ - private function addNewNotionPages(Database $database, Collection $pages, Collection $notionPages): void + private function addNewNotionPages(Database $database, Collection $pages, Collection $notionPages): int { $options = $this->getOptions($database); + $failed = 0; foreach ($pages as $page) { - $exists = true; - $url = $page->url; - $np = $notionPages->first(fn (NotionPage $notionPage): bool => $this->getUrlProp($notionPage) === $url); - if (! $np) { - $exists = false; - $np = NotionPage::create(PageParent::database($database->id)); - } - - $np = $np->changeTitle($page->title) - ->addProperty( - self::PAGE_PROP_MAPPING['url'], - Url::create($page->url) - ) - ->addProperty( - self::PAGE_PROP_MAPPING['site_name'], - RichTextProperty::fromString(__('misc.'.$page->site_name->value)) - ) - ->addProperty( - self::PAGE_PROP_MAPPING['last_modified'], - Date::create($page->last_modified->toDateTimeImmutable()) - ) - ->addProperty( - self::PAGE_PROP_MAPPING['paks'], - MultiSelect::fromOptions( - ...$page - ->paks - ->map(fn (Pak $pak): array|string => __('misc.'.$pak->slug->value)) - ->filter(fn (array|string $name): bool => is_string($name)) - ->map(fn (string $name) => $options->first(fn ($opt): bool => $name === $opt->name)) - ->filter(fn ($pak): bool => ! is_null($pak)) + try { + $exists = true; + $url = $page->url; + $np = $notionPages->first(fn (NotionPage $notionPage): bool => $this->getUrlProp($notionPage) === $url); + if (! $np) { + $exists = false; + $np = NotionPage::create(PageParent::database($database->id)); + } + + $np = $np->changeTitle($page->title) + ->addProperty( + self::PAGE_PROP_MAPPING['url'], + Url::create($page->url) + ) + ->addProperty( + self::PAGE_PROP_MAPPING['site_name'], + RichTextProperty::fromString(__('misc.'.$page->site_name->value)) ) - ); - - if ($exists) { - logger('[NotionService] update', ['url' => $url]); - $this->notion->pages()->update($np); - } else { - logger('[NotionService] create', ['url' => $url]); - $this->notion->pages()->create($np); + ->addProperty( + self::PAGE_PROP_MAPPING['last_modified'], + Date::create($page->last_modified->toDateTimeImmutable()) + ) + ->addProperty( + self::PAGE_PROP_MAPPING['paks'], + MultiSelect::fromOptions( + ...$page + ->paks + ->map(fn (Pak $pak): array|string => __('misc.'.$pak->slug->value)) + ->filter(fn (array|string $name): bool => is_string($name)) + ->map(fn (string $name) => $options->first(fn ($opt): bool => $name === $opt->name)) + ->filter(fn ($pak): bool => ! is_null($pak)) + ) + ); + + if ($exists) { + logger('[NotionService] update', ['url' => $url]); + $this->notion->pages()->update($np); + } else { + logger('[NotionService] create', ['url' => $url]); + $this->notion->pages()->create($np); + } + } catch (\Throwable $th) { + $failed++; + logger()->error('[NotionService] sync failed', ['url' => $page->url, $th]); } } + + return $failed; } private function getUrlProp(NotionPage $notionPage): ?string diff --git a/app/Logging/RedactSecretsProcessor.php b/app/Logging/RedactSecretsProcessor.php new file mode 100644 index 0000000..e23f1dc --- /dev/null +++ b/app/Logging/RedactSecretsProcessor.php @@ -0,0 +1,27 @@ +with( + message: $this->secretScrubber->scrub($record->message), + context: $this->secretScrubber->scrubArray($record->context), + extra: $this->secretScrubber->scrubArray($record->extra), + ); + } +} diff --git a/app/Logging/RedactSecretsTap.php b/app/Logging/RedactSecretsTap.php new file mode 100644 index 0000000..02544d7 --- /dev/null +++ b/app/Logging/RedactSecretsTap.php @@ -0,0 +1,26 @@ +getLogger(); + if ($monolog instanceof MonologLogger) { + $monolog->pushProcessor( + new RedactSecretsProcessor(app(SecretScrubber::class)), + ); + } + } +} diff --git a/app/Models/RawPage.php b/app/Models/RawPage.php index ab56365..87d0f2c 100644 --- a/app/Models/RawPage.php +++ b/app/Models/RawPage.php @@ -19,10 +19,12 @@ * @property SiteName $site_name * @property string $url * @property string $html + * @property Carbon|null $extract_failed_at * @property Carbon|null $created_at * @property Carbon|null $updated_at * @property-read Page|null $page * + * @method static \Database\Factories\RawPageFactory factory($count = null, $state = []) * @method static \Illuminate\Database\Eloquent\Builder|RawPage newModelQuery() * @method static \Illuminate\Database\Eloquent\Builder|RawPage newQuery() * @method static \Illuminate\Database\Eloquent\Builder|RawPage query() @@ -38,11 +40,13 @@ final class RawPage extends Model 'site_name', 'url', 'html', + 'extract_failed_at', ]; protected $casts = [ 'site_name' => SiteName::class, 'html' => CompressedHtml::class, + 'extract_failed_at' => 'datetime', ]; private ?Crawler $crawler = null; diff --git a/config/logging.php b/config/logging.php index 0053fc6..5e72e6b 100644 --- a/config/logging.php +++ b/config/logging.php @@ -2,6 +2,7 @@ declare(strict_types=1); +use App\Logging\RedactSecretsTap; use MarvinLabs\DiscordLogger\Logger; use Monolog\Handler\StreamHandler; use Monolog\Processor\PsrLogMessageProcessor; @@ -64,6 +65,7 @@ 'path' => storage_path('logs/laravel.log'), 'level' => env('LOG_LEVEL', 'debug'), 'replace_placeholders' => true, + 'tap' => [RedactSecretsTap::class], ], 'api' => [ @@ -71,6 +73,7 @@ 'path' => storage_path('logs/api.log'), 'level' => env('LOG_LEVEL', 'debug'), 'replace_placeholders' => true, + 'tap' => [RedactSecretsTap::class], ], 'slack' => [ diff --git a/database/migrations/2026_06_22_000000_add_extract_failed_at_to_raw_pages_table.php b/database/migrations/2026_06_22_000000_add_extract_failed_at_to_raw_pages_table.php new file mode 100644 index 0000000..6e471e8 --- /dev/null +++ b/database/migrations/2026_06_22_000000_add_extract_failed_at_to_raw_pages_table.php @@ -0,0 +1,25 @@ +timestamp('extract_failed_at')->nullable()->after('html'); + }); + } + + public function down(): void + { + Schema::table('raw_pages', function (Blueprint $table): void { + $table->dropColumn('extract_failed_at'); + }); + } +}; diff --git a/docs/assurance-audit-2026-06-22.md b/docs/assurance-audit-2026-06-22.md new file mode 100644 index 0000000..f9ddaf8 --- /dev/null +++ b/docs/assurance-audit-2026-06-22.md @@ -0,0 +1,59 @@ +# Assurance Audit スナップショット（2026-06-22） + +これは初回の Assurance Audit（assurance-audit スキル）の診断記録。 +「現在の状態」は [known-risks.md](known-risks.md) を参照。本ファイルは**初回診断の根拠**を残すための凍結記録。 + +採点モデル: `Intent → Behavior → Control → Evidence`。 +Coverage（量）ではなく **Confidence（守られているか）** を採点する。 +スコープは認証/課金/取引のない公開スクレイピング＋検索アプリとして、ユーザー選択の 4 領域。 + +--- + +## Step -1 所見（最優先） + +**脅威モデル・既知リスク・runbook が一切存在しない。** `docs/dependency-debt.md` は依存負債のみ。 +「何を守るべきか」の台帳が無いこと自体が最大の欠陥。本監査を機に [known-risks.md](known-risks.md) を新設した。 + +## New Candidate Risks（脅威探索で発見、既存台帳に無かった項目） + +1. **🟡 抽出失敗時に RawPage を削除している (A4)** — `app/Actions/Extract/{Twitrans,Japan,Portal}/Handler.php` + の catch で `$rawPage->delete()`。一過性バグでも唯一のスクレイプ結果が恒久消失。 +2. **🟡 例外 → Discord/ログへのスタックトレース素通し (B4/C3)** — `bootstrap/app.php` の空ハンドラ、 + `config/discord-logger.php` の `'stacktrace' => 'smart'`、`ConvertDiscord` が trace/context を無加工送出。 + 機密値（Notion secret/webhook/DB パスワード）混入経路が開いている。 +3. **🔴 `.env.example` が `APP_DEBUG=true` 既定 (C1)** — redaction 無し、API も独自ハンドリング無し。 +4. **Notion 同期の再同期スロットリング無し (B3)** — 毎回最新 100 件を re-PUSH。データ破損ではなく API クォータ浪費。 + +--- + +## 全挙動マトリクス（初回値） + +| # | 挙動 → Expected Outcome | 制御 | テスト | Status | +|---|---|---|---|---| +| A1 | 1 サイト失敗で他が止まらない | Preventive（try/catch） | 0 | 🔴 Untested | +| A2 | HTTP 失敗時に空 HTML 上書きしない | Preventive（成功時のみ upsert） | 0 | 🔴 Untested | +| A3 | extract 再実行で Page 重複なし | Preventive（unique + HasOne） | 0 | 🔴 Untested | +| A4 | 抽出失敗時にデータ破壊しない | 誤り（delete で消去） | 0 | 🟡 Structural Weakness | +| A5 | 同一 URL 重複行なし | Preventive（DB 一意制約） | 0 | 🔴 Untested | +| B1 | 再実行で Notion 重複作成なし | Preventive（URL 突合） | 1 Strong | 🟡 SPOF | +| B2 | 1 件 API エラーで全体停止しない | None | 0 | 🔴 Missing | +| B3 | 未変更項目を re-PUSH しない | None | 0 | 🔴 Missing | +| B4 | 機密値を例外出力に混入させない | Detective のみ | 0 | 🟡 Structural Weakness | +| C1 | デバッグページが trace を露出しない | None（DEBUG=true 既定） | 0 | 🟡 Structural Weakness | +| C2 | API 例外が生 trace を返さない | None | 0 | 🔴 Missing | +| C3 | log/Discord に機密値を混入させない | Detective のみ | 0 | 🟡 Structural Weakness | +| C4 | git に実シークレット混入なし | Preventive | N/A | 🟢 OK | +| D1 | raw_pages を公開経路に露出しない | Preventive（eager-load せず） | 0 | 🔴 Untested | +| D2 | 非公開コンテンツを出さない | N/A（公開状態の概念なし） | — | 対象外 | +| D3 | 応答に内部フィールドを含めない | Preventive（Resource 許可リスト） | 0 | 🔴 Untested | +| D4 | 半書き Page を検索に見せない | None（transaction なし） | 0 | 🟡 Structural Weakness | + +--- + +## 所見 + +- 🟢 は C4（git シークレット混入チェック）のみ。 +- 「Untested」行（A1/A2/A3/A5/D1/D3）は機構自体は健全。回帰検知テストが無いことが欠陥。 +- 最優先の構造的弱点: **A4**（証跡破壊）と **C1+C3/B4**（機密漏洩経路、1 つの修正で複数行を閉じる）。 +- **B2** は最もクリアな Missing Control（scrape/extract と違い項目単位のフォールト分離が皆無）。 +- 機密値が「過去に実際に漏れた痕跡」は発見していない。確認されたのは**開いた経路**であり、過去のインシデントではない。 diff --git a/docs/known-risks.md b/docs/known-risks.md new file mode 100644 index 0000000..98ae668 --- /dev/null +++ b/docs/known-risks.md @@ -0,0 +1,53 @@ +# Known Risks（保護すべき挙動の台帳） + +「壊れてはいけない挙動」の台帳。1 行＝1 保護挙動。Assurance Audit の採点結果を記録し、 +今後の棚卸し・監査の対象にする（`docs/dependency-debt.md` と同じ運用思想）。 + +- **制御**: None（無し）/ Detective（事後に検知・通知のみ）/ Preventive（事前に阻止） +- **Status**: 🔴 Missing/Stale/Weak ・ 🟡 Structural Weakness/SPOF ・ 🟢 OK +- 是正が完了したら Status とテスト欄を更新する（行は削除せず履歴として残す）。 +- 初回診断の根拠は [assurance-audit-2026-06-22.md](assurance-audit-2026-06-22.md) を参照。 + +## A. Scrape / Extract パイプライン + +| ID | 保護すべき挙動 / Expected Outcome | 制御 | テスト | Status | 是正条件 | 記録日 | +|----|----------------------------------|------|--------|--------|----------|--------| +| A1 | 1 サイト/1URL の失敗で他サイト・他 URL の処理が止まらない | Preventive（ハンドラ毎の try/catch） | 1（`Extract/Japan/HandlerIsolationTest`） | 🟡 SPOF | テストを 2 本以上に（scrape 側 / 他サイトにも展開） | 2026-06-22 | +| A2 | HTTP 失敗時に RawPage を空/部分 HTML で上書きしない | Preventive（fetch 例外時は upsert に到達しない） | 1（`Scrape/Japan/HandlerFailureTest`） | 🟡 SPOF | 残課題: 非2xx でも body を書き込む経路がある。`->throw()` 等で非2xx も失敗扱いにするか検討 | 2026-06-22 | +| A3 | 同一 RawPage に extract を再実行しても Page 重複が出ない（冪等） | Preventive（`pages_url_unique` + HasOne） | 1（`Extract/UpdateOrCreatePageTest`） | 🟡 SPOF | テストを 2 本以上に | 2026-06-22 | +| A4 | 抽出失敗時にスクレイプ済データ（RawPage）を破壊しない | Preventive（`MarkExtractFailed`：削除せず `extract_failed_at` で隔離、成功時にクリア） | 4（`MarkExtractFailedTest`×3 + `HandlerIsolationTest`） | 🟢 OK | — | 2026-06-22 | +| A5 | 同一 URL の重複行を作らない | Preventive（DB 一意制約） | 2（`Models/UrlUniqueConstraintTest`） | 🟢 OK | — | 2026-06-22 | + +## B. Notion 同期 + +| ID | 保護すべき挙動 / Expected Outcome | 制御 | テスト | Status | 是正条件 | 記録日 | +|----|----------------------------------|------|--------|--------|----------|--------| +| B1 | 再実行で Notion ページを重複作成しない | Preventive（URL 突合で create/update 分岐） | 1（Strong） | 🟡 SPOF | テストを 2 本以上に増やす | 2026-06-22 | +| B2 | 1 件の Notion API エラーでバッチ全体が止まらない | Preventive（項目単位 try/catch + 継続 + 失敗件数を error ログ） | 1（`SyncActionTest::test_continues_syncing_when_one_item_fails`） | 🟡 SPOF | テストを 2 本以上に（delete 側の失敗継続も） | 2026-06-22 | +| B3 | 未変更項目を毎回 re-PUSH せず API クォータを浪費しない | None（`synced_at`/状態フラグ無し） | 0 | 🔴 Missing | **記録のみ**。API 呼び出し回数が問題化したら `synced_at` を導入 | 2026-06-22 | +| B4 | Notion シークレットを例外/ログ/Discord に流出させない | Preventive（`SecretScrubber` で送出前に伏字化。C3 と一体） | 4（C3 のテストと共有） | 🟢 OK | — | 2026-06-22 | + +## C. シークレット / 資格情報の取扱い + +| ID | 保護すべき挙動 / Expected Outcome | 制御 | テスト | Status | 是正条件 | 記録日 | +|----|----------------------------------|------|--------|--------|----------|--------| +| C1 | デバッグページが config/スタックトレースを露出しない | Preventive（`.env.example`/`.ci`/`.deploy` すべて `APP_DEBUG=false`、コード既定も false） | 1（C2 の `ApiErrorResponseTest` で間接担保） | 🟡 SPOF | Web 側デバッグページ向けの直接テストは未追加 | 2026-06-22 | +| C2 | API 例外が生のトレースを返さない | Preventive（`APP_DEBUG=false` で汎用 JSON エラー） | 1（`Http/ApiErrorResponseTest`） | 🟡 SPOF | テストを 2 本以上に | 2026-06-22 | +| C3 | 例外レポート（log/Discord）に機密値を混入させない | Preventive（`SecretScrubber` + `RedactSecretsProcessor`/tap + `ConvertDiscord` で送出前に伏字化） | 4（`SecretScrubberTest`×3 + `RedactSecretsProcessorTest`） | 🟢 OK | — | 2026-06-22 | +| C4 | 実シークレットを git に混入させない | Preventive（`.gitignore` で `.env` 除外、committed な `.env.*` は空値） | N/A | 🟢 OK | — | 2026-06-22 | + +## D. 公開検索 / Feed / API + +| ID | 保護すべき挙動 / Expected Outcome | 制御 | テスト | Status | 是正条件 | 記録日 | +|----|----------------------------------|------|--------|--------|----------|--------| +| D1 | raw_pages（生 HTML）を公開経路に露出しない | Preventive（検索/Feed で rawPage を eager-load しない、Page に html 列無し） | 1（`PageResourceTest::test_search_does_not_eager_load_raw_page_relation`） | 🟡 SPOF | Feed 経路の assert も追加 | 2026-06-22 | +| D2 | 非公開コンテンツを検索/Feed に出さない | N/A（Page に公開状態の概念が無い） | — | — | 対象外（将来 status 列を足す場合は再評価） | 2026-06-22 | +| D3 | API/Feed 応答に内部フィールドを含めない | Preventive（PageResource 許可リスト） | 1（`PageResourceTest::test_resource_exposes_only_whitelisted_fields`） | 🟡 SPOF | テストを 2 本以上に | 2026-06-22 | +| D4 | 半分書かれた Page 行を検索に見せない（原子性） | None（`UpdateOrCreatePage` に transaction 無し） | 0 | 🟡 Structural Weakness | （低優先・データ品質）将来 transaction か status 列で是正 | 2026-06-22 | + + diff --git a/tests/Feature/Actions/Extract/Japan/HandlerIsolationTest.php b/tests/Feature/Actions/Extract/Japan/HandlerIsolationTest.php new file mode 100644 index 0000000..fe9cd6d --- /dev/null +++ b/tests/Feature/Actions/Extract/Japan/HandlerIsolationTest.php @@ -0,0 +1,62 @@ +create([ + 'site_name' => SiteName::Japan, + 'url' => 'https://japanese.simutrans.com/index.php?Addon128%2FBroken', + 'html' => '
no lastmodified here
', + ]); + + // 正常に抽出できる HTML。 + $rawPageB = RawPage::factory()->create([ + 'site_name' => SiteName::Japan, + 'url' => 'https://japanese.simutrans.com/index.php?Addon128%2FOk', + 'html' => 'OK Addon - Simutrans日本語化･解説' + .'
本文
Last-modified: 2024-01-01 00:00:00 (月)
', + ]); + + $handler = new Handler( + new ChunkRawPages, + new ExtractLastModified, + new ExtractContents, + new UpdateOrCreatePage, + new SyncPak, + new MarkExtractFailed, + ); + + $handler(new NullLogger); + + // b は失敗した a の後でも処理され、Page が作られる。 + $this->assertDatabaseHas('pages', ['raw_page_id' => $rawPageB->id]); + $this->assertDatabaseMissing('pages', ['raw_page_id' => $rawPageA->id]); + + // a は削除されず、失敗フラグが立つ。b はフラグなし。 + $this->assertDatabaseHas('raw_pages', ['id' => $rawPageA->id]); + $this->assertNotNull($rawPageA->fresh()?->extract_failed_at); + $this->assertNull($rawPageB->fresh()?->extract_failed_at); + } +} diff --git a/tests/Feature/Actions/Extract/MarkExtractFailedTest.php b/tests/Feature/Actions/Extract/MarkExtractFailedTest.php new file mode 100644 index 0000000..45af9f9 --- /dev/null +++ b/tests/Feature/Actions/Extract/MarkExtractFailedTest.php @@ -0,0 +1,45 @@ +create(); + + (new MarkExtractFailed)(new NullLogger, $rawPage, new \RuntimeException('parse error')); + + // 削除されず残存していること（旧実装は delete() で消していた）。 + $this->assertDatabaseHas('raw_pages', ['id' => $rawPage->id]); + $this->assertNotNull($rawPage->fresh()?->extract_failed_at); + } + + public function test_clear_resets_failed_flag_on_success(): void + { + $rawPage = RawPage::factory()->create(['extract_failed_at' => now()]); + + (new MarkExtractFailed)->clear($rawPage); + + $this->assertNull($rawPage->fresh()?->extract_failed_at); + } + + public function test_clear_is_noop_when_not_failed(): void + { + $rawPage = RawPage::factory()->create(['extract_failed_at' => null]); + + (new MarkExtractFailed)->clear($rawPage); + + $this->assertNull($rawPage->fresh()?->extract_failed_at); + } +} diff --git a/tests/Feature/Actions/Extract/UpdateOrCreatePageTest.php b/tests/Feature/Actions/Extract/UpdateOrCreatePageTest.php new file mode 100644 index 0000000..1ebf1aa --- /dev/null +++ b/tests/Feature/Actions/Extract/UpdateOrCreatePageTest.php @@ -0,0 +1,31 @@ +create(); + $updateOrCreatePage = new UpdateOrCreatePage; + + $page = $updateOrCreatePage($rawPage, 'タイトル', '本文', CarbonImmutable::now()); + $second = $updateOrCreatePage($rawPage->fresh(), 'タイトル更新', '本文更新', CarbonImmutable::now()); + + // 行は 1 件のまま、同一 Page が更新される。 + $this->assertSame(1, Page::query()->where('raw_page_id', $rawPage->id)->count()); + $this->assertSame($page->id, $second->id); + $this->assertSame('タイトル更新', $second->fresh()?->title); + } +} diff --git a/tests/Feature/Actions/Logging/SecretScrubberTest.php b/tests/Feature/Actions/Logging/SecretScrubberTest.php new file mode 100644 index 0000000..58606e3 --- /dev/null +++ b/tests/Feature/Actions/Logging/SecretScrubberTest.php @@ -0,0 +1,65 @@ +scrub('Authorization: Bearer ntn_supersecret failed for db-pass-123'); + + $this->assertStringNotContainsString('ntn_supersecret', $result); + $this->assertStringNotContainsString('db-pass-123', $result); + $this->assertStringContainsString('[REDACTED]', $result); + } + + public function test_scrub_array_recurses_and_masks_throwable(): void + { + $secretScrubber = new SecretScrubber; + + $result = $secretScrubber->scrubArray([ + 'url' => 'https://discord.com/api/webhooks/abc/xyz', + 'nested' => ['token' => 'value=ntn_supersecret'], + 'exception' => new \RuntimeException('leaked ntn_supersecret here'), + ]); + + $encoded = json_encode($result); + $this->assertIsString($encoded); + $this->assertStringNotContainsString('ntn_supersecret', $encoded); + $this->assertStringNotContainsString('discord.com/api/webhooks/abc/xyz', $encoded); + } + + public function test_scrub_is_noop_when_no_secrets_configured(): void + { + Config::set('services.notion.secret', ''); + Config::set('logging.channels.discord.url', ''); + Config::set('database.connections.mysql.password', ''); + Config::set('database.connections.portal.password', ''); + + $secretScrubber = new SecretScrubber; + + // 空の機密値で全文が伏字化されてしまわないこと。 + $this->assertSame('plain text', $secretScrubber->scrub('plain text')); + } +} diff --git a/tests/Feature/Actions/Scrape/Japan/HandlerFailureTest.php b/tests/Feature/Actions/Scrape/Japan/HandlerFailureTest.php new file mode 100644 index 0000000..2b6ec5f --- /dev/null +++ b/tests/Feature/Actions/Scrape/Japan/HandlerFailureTest.php @@ -0,0 +1,48 @@ +
'; + + Http::fake([ + 'https://japanese.simutrans.com?cmd=list' => Http::response($listHtml, 200), + // PSR-7 URI は文字列化時に既定ポート(443)を省略するため、 + // Http::fake のキーは href の表記（:443 付き）ではなく実際の送信先 URL に合わせる。 + 'https://japanese.simutrans.com/index.php?Addon128%2FTest' => fn (): never => throw new ConnectionException('connection failed'), + ]); + + $handler = new Handler( + new FetchHtml(retryTimes: 1, sleepMilliseconds: 1, useCache: false), + new FindUrls(new FetchHtml(retryTimes: 1, sleepMilliseconds: 1, useCache: false)), + new UpdateOrCreateRawPage, + ); + + $handler(new NullLogger); + + $this->assertSame(0, RawPage::query()->count()); + } +} diff --git a/tests/Feature/Actions/SyncNotion/SyncActionTest.php b/tests/Feature/Actions/SyncNotion/SyncActionTest.php index 1604f57..7b05d25 100644 --- a/tests/Feature/Actions/SyncNotion/SyncActionTest.php +++ b/tests/Feature/Actions/SyncNotion/SyncActionTest.php @@ -93,4 +93,64 @@ public function test_sync_action_creates_updates_and_deletes_pages(): void $syncAction = new SyncAction($mock); $syncAction('test_database_id', 10); } + + /** + * B2: 1 件の Notion API エラーでバッチ全体が止まらず、残り項目が処理されること。 + */ + public function test_continues_syncing_when_one_item_fails(): void + { + Page::factory()->create([ + 'title' => 'Fails', + 'site_name' => SiteName::Japan, + 'url' => 'https://example.com/fail', + 'last_modified' => now()->subDay(), + ]); + Page::factory()->create([ + 'title' => 'Succeeds', + 'site_name' => SiteName::Japan, + 'url' => 'https://example.com/ok', + 'last_modified' => now(), + ]); + + $database = Database::fromArray([ + 'id' => 'test_database_id', + 'created_time' => '2023-01-01T00:00:00.000Z', + 'last_edited_time' => '2023-01-01T00:00:00.000Z', + 'title' => [], + 'description' => [], + 'icon' => null, + 'cover' => null, + 'properties' => [ + 'Title' => ['id' => 'title', 'type' => 'title', 'name' => 'Title', 'title' => []], + 'パックセット' => [ + 'id' => 'prop_id', + 'type' => 'multi_select', + 'name' => 'パックセット', + 'multi_select' => ['options' => []], + ], + ], + 'parent' => ['type' => 'workspace', 'workspace' => true], + 'url' => 'https://notion.so', + 'is_inline' => false, + ]); + + $mock = Mockery::mock(Notion::class); + $mock->shouldReceive('databases->find')->with('test_database_id')->andReturn($database); + $mock->shouldReceive('databases->queryAllPages')->with($database)->andReturn([]); + + // 1 件目（fail）は Notion API がエラーを投げる。 + $mock->shouldReceive('pages->create') + ->with(Mockery::on(fn (NotionPage $notionPage): bool => $notionPage->getProperty('URL')->url === 'https://example.com/fail')) + ->andThrow(new \RuntimeException('rate limited')); + + // 2 件目（ok）は 1 件目の失敗後も必ず処理される。 + $mock->shouldReceive('pages->create') + ->once() + ->with(Mockery::on(fn (NotionPage $notionPage): bool => $notionPage->getProperty('URL')->url === 'https://example.com/ok')); + + $syncAction = new SyncAction($mock); + + // バッチ全体が中断せず正常終了すること（例外が伝播しない）。 + $syncAction('test_database_id', 10); + } } diff --git a/tests/Feature/Http/ApiErrorResponseTest.php b/tests/Feature/Http/ApiErrorResponseTest.php new file mode 100644 index 0000000..be0168c --- /dev/null +++ b/tests/Feature/Http/ApiErrorResponseTest.php @@ -0,0 +1,34 @@ +getJson('/__test/throw'); + + $testResponse->assertStatus(500); + $testResponse->assertJsonMissingPath('trace'); + $testResponse->assertJsonMissingPath('exception'); + // 汎用メッセージのみで、例外の中身が露出しないこと。 + $this->assertSame('Server Error', $testResponse->json('message')); + $this->assertStringNotContainsString('super-secret-detail-xyz', $testResponse->getContent() ?: ''); + } +} diff --git a/tests/Feature/Http/Resources/PageResourceTest.php b/tests/Feature/Http/Resources/PageResourceTest.php new file mode 100644 index 0000000..78b9abc --- /dev/null +++ b/tests/Feature/Http/Resources/PageResourceTest.php @@ -0,0 +1,60 @@ +create(['html' => 'secret-internal-html']); + $page = Page::factory()->create([ + 'raw_page_id' => $rawPage->id, + 'site_name' => SiteName::Japan, + ]); + + $array = (new PageResource($page))->toArray(Request::create('/')); + + $this->assertSame(['title', 'site', 'paks', 'url', 'last_modified'], array_keys($array)); + $this->assertArrayNotHasKey('raw_page_id', $array); + $this->assertArrayNotHasKey('id', $array); + $this->assertArrayNotHasKey('text', $array); + $this->assertStringNotContainsString('secret-internal-html', (string) json_encode($array)); + } + + public function test_search_does_not_eager_load_raw_page_relation(): void + { + $pak = Pak::factory()->create(['slug' => PakSlug::Pak128]); + $page = Page::factory()->create([ + 'site_name' => SiteName::Japan, + 'title' => 'Addon', + 'text' => 'body', + ]); + $page->paks()->attach($pak); + + $result = (new SearchAction)([ + 'keyword' => '', + 'paks' => [PakSlug::Pak128->value], + 'sites' => [SiteName::Japan->value], + ]); + + $this->assertCount(1, $result->items()); + // 生 HTML を持つ rawPage リレーションがロードされていないこと。 + $this->assertFalse($result->items()[0]->relationLoaded('rawPage')); + } +} diff --git a/tests/Feature/Logging/RedactSecretsProcessorTest.php b/tests/Feature/Logging/RedactSecretsProcessorTest.php new file mode 100644 index 0000000..145d443 --- /dev/null +++ b/tests/Feature/Logging/RedactSecretsProcessorTest.php @@ -0,0 +1,41 @@ + 'token=ntn_supersecret'], + extra: [], + ); + + $result = $redactSecretsProcessor($logRecord); + + $this->assertStringNotContainsString('ntn_supersecret', $result->message); + $this->assertStringNotContainsString('ntn_supersecret', (string) json_encode($result->context)); + $this->assertStringContainsString('[REDACTED]', $result->message); + } +} diff --git a/tests/Feature/Models/UrlUniqueConstraintTest.php b/tests/Feature/Models/UrlUniqueConstraintTest.php new file mode 100644 index 0000000..36cc93d --- /dev/null +++ b/tests/Feature/Models/UrlUniqueConstraintTest.php @@ -0,0 +1,32 @@ +create(['url' => 'https://example.com/dup']); + + $this->expectException(QueryException::class); + RawPage::factory()->create(['url' => 'https://example.com/dup']); + } + + public function test_pages_url_is_unique(): void + { + Page::factory()->create(['url' => 'https://example.com/dup']); + + $this->expectException(QueryException::class); + Page::factory()->create(['url' => 'https://example.com/dup']); + } +} From 3d6d75be7951dc069858102dc3c2cdf63f2b5abc Mon Sep 17 00:00:00 2001 From: 128na Date: Mon, 22 Jun 2026 19:46:59 +0900 Subject: [PATCH 2/5] Fix CI failure and address review feedback in audit changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - SecretScrubber: use Config::get() instead of Config::string() — the latter throws when a config key is present but explicitly null (e.g. LOG_DISCORD_WEBHOOK_URL unset in CI), which broke RedactSecretsProcessorTest in the "test (Feature)" CI job. Also drop secrets shorter than 4 chars from the redaction list so a short dev password (e.g. "root") can't mass-redact unrelated log content. - SyncAction::deleteOldNotionPages: initialize $url before the try block so a failure on getUrlProp() doesn't log the previous iteration's URL instead of the one that actually failed. Co-Authored-By: Claude Sonnet 4.6 --- app/Actions/Logging/SecretScrubber.php | 14 ++++++++------ app/Actions/SyncNotion/SyncAction.php | 3 ++- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/app/Actions/Logging/SecretScrubber.php b/app/Actions/Logging/SecretScrubber.php index f8d7e4e..7989632 100644 --- a/app/Actions/Logging/SecretScrubber.php +++ b/app/Actions/Logging/SecretScrubber.php @@ -66,22 +66,24 @@ private function walk(mixed $value, array $secrets): mixed } /** - * 伏字化対象の機密値一覧（空値は対象から除外する）。 + * 伏字化対象の機密値一覧（短すぎる値・空値は誤爆防止のため対象から除外する）。 * * @return list */ private function secrets(): array { $candidates = [ - (string) Config::string('services.notion.secret', ''), - (string) Config::string('logging.channels.discord.url', ''), - (string) Config::string('database.connections.mysql.password', ''), - (string) Config::string('database.connections.portal.password', ''), + Config::get('services.notion.secret'), + Config::get('logging.channels.discord.url'), + Config::get('database.connections.mysql.password'), + Config::get('database.connections.portal.password'), ]; return array_values(array_unique(array_filter( $candidates, - fn (string $value): bool => $value !== '', + // 未設定(null)や開発環境の "root" 等の短い値まで伏字化すると + // ログ本文を無関係な部分まで壊してしまうため、4文字未満は対象外にする。 + fn (mixed $value): bool => is_string($value) && mb_strlen($value) >= 4, ))); } } diff --git a/app/Actions/SyncNotion/SyncAction.php b/app/Actions/SyncNotion/SyncAction.php index dea95be..06e60d8 100644 --- a/app/Actions/SyncNotion/SyncAction.php +++ b/app/Actions/SyncNotion/SyncAction.php @@ -54,6 +54,7 @@ private function deleteOldNotionPages(Collection $pages, Collection $notionPages { $failed = 0; foreach ($notionPages as $notionPage) { + $url = null; try { $url = $this->getUrlProp($notionPage); $page = $pages->first(fn (Page $page): bool => $page->url === $url); @@ -63,7 +64,7 @@ private function deleteOldNotionPages(Collection $pages, Collection $notionPages } } catch (\Throwable $th) { $failed++; - logger()->error('[NotionService] delete failed', ['url' => $url ?? null, $th]); + logger()->error('[NotionService] delete failed', ['url' => $url, $th]); } } From 7e92ae183645eb1a5559943bcc7da031fe12c1a8 Mon Sep 17 00:00:00 2001 From: 128na Date: Mon, 22 Jun 2026 19:58:06 +0900 Subject: [PATCH 3/5] Fix Discord stacktrace leak and address remaining review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ConvertDiscord: extract the stacktrace before scrubbing context (scrubArray() stringifies any Throwable, so getStacktrace() would return null if called after). More importantly, the inherited addMessageStacktrace() was still running after our override and overwriting $message->file with the *unscrubbed* raw trace, which would have defeated the whole redaction path for error-level Discord messages. Override it as a no-op since the scrubbed trace is already attached in addMessageContent(). Added ConvertDiscordTest covering both behaviors directly. - SecretScrubber: cache the resolved secret list per instance (not static — a static cache would leak Config::set() changes across PHPUnit test methods in the same process) to avoid recomputing Config::get() + array filtering on every log line. - SyncAction::deleteOldNotionPages: key $pages by url once before the loop instead of calling Collection::first() per Notion page, cutting the lookup from O(N*M) to O(N+M). Co-Authored-By: Claude Sonnet 4.6 --- app/Actions/Logging/ConvertDiscord.php | 18 +++++- app/Actions/Logging/SecretScrubber.php | 13 +++- app/Actions/SyncNotion/SyncAction.php | 4 +- .../Actions/Logging/ConvertDiscordTest.php | 63 +++++++++++++++++++ 4 files changed, 94 insertions(+), 4 deletions(-) create mode 100644 tests/Feature/Actions/Logging/ConvertDiscordTest.php diff --git a/app/Actions/Logging/ConvertDiscord.php b/app/Actions/Logging/ConvertDiscord.php index 626b011..74191ba 100644 --- a/app/Actions/Logging/ConvertDiscord.php +++ b/app/Actions/Logging/ConvertDiscord.php @@ -23,11 +23,14 @@ public function __construct(Repository $config, private readonly SecretScrubber protected function addMessageContent(Message $message, array $record): void { try { + // context['exception'] が Throwable のままの状態で先に取得する。 + // scrubArray() は Throwable を文字列化してしまうため、先に呼ぶと取得できなくなる。 + $stacktrace = $this->getStacktrace($record); + // Discord は外部サービスへ送出されるため、組み立て前に機密値を伏字化する。 $record['message'] = $this->secretScrubber->scrub($record['message']); $record['context'] = $this->secretScrubber->scrubArray($record['context']); - $stacktrace = $this->getStacktrace($record); if ($stacktrace !== null) { $stacktrace = $this->secretScrubber->scrub($stacktrace); } @@ -42,6 +45,19 @@ protected function addMessageContent(Message $message, array $record): void } } + /** + * 親クラスの addMessageStacktrace は未伏字化の生スタックトレースで + * $message->file を上書きしてしまうため、何もしないようにする + * （伏字化済みのスタックトレースは addMessageContent 内で既に添付済み）。 + * + * @param array{datetime:\DateTime,level_name:int,message:string,context:array} $record + */ + #[\Override] + protected function addMessageStacktrace(Message $message, array $record): void + { + // no-op: see method docblock. + } + /** * @param array{datetime:\DateTime,level_name:int,message:string,context:array} $record */ diff --git a/app/Actions/Logging/SecretScrubber.php b/app/Actions/Logging/SecretScrubber.php index 7989632..ee04dbd 100644 --- a/app/Actions/Logging/SecretScrubber.php +++ b/app/Actions/Logging/SecretScrubber.php @@ -17,6 +17,11 @@ final class SecretScrubber { private const string MASK = '[REDACTED]'; + /** + * @var list|null + */ + private ?array $cachedSecrets = null; + public function scrub(string $value): string { $secrets = $this->secrets(); @@ -67,11 +72,17 @@ private function walk(mixed $value, array $secrets): mixed /** * 伏字化対象の機密値一覧（短すぎる値・空値は誤爆防止のため対象から除外する）。 + * リクエスト中に変わらない値なのでインスタンス単位でキャッシュする + * （静的キャッシュにすると PHPUnit のテスト間で Config 変更が反映されなくなるため避ける）。 * * @return list */ private function secrets(): array { + if ($this->cachedSecrets !== null) { + return $this->cachedSecrets; + } + $candidates = [ Config::get('services.notion.secret'), Config::get('logging.channels.discord.url'), @@ -79,7 +90,7 @@ private function secrets(): array Config::get('database.connections.portal.password'), ]; - return array_values(array_unique(array_filter( + return $this->cachedSecrets = array_values(array_unique(array_filter( $candidates, // 未設定(null)や開発環境の "root" 等の短い値まで伏字化すると // ログ本文を無関係な部分まで壊してしまうため、4文字未満は対象外にする。 diff --git a/app/Actions/SyncNotion/SyncAction.php b/app/Actions/SyncNotion/SyncAction.php index 06e60d8..8c24168 100644 --- a/app/Actions/SyncNotion/SyncAction.php +++ b/app/Actions/SyncNotion/SyncAction.php @@ -53,12 +53,12 @@ public function __invoke(string $databaseId, int $limit): void private function deleteOldNotionPages(Collection $pages, Collection $notionPages): int { $failed = 0; + $pagesByUrl = $pages->keyBy('url'); foreach ($notionPages as $notionPage) { $url = null; try { $url = $this->getUrlProp($notionPage); - $page = $pages->first(fn (Page $page): bool => $page->url === $url); - if (! $page) { + if ($url === null || ! $pagesByUrl->has($url)) { logger('[NotionService]delete', ['url' => $url]); $this->notion->pages()->delete($notionPage); } diff --git a/tests/Feature/Actions/Logging/ConvertDiscordTest.php b/tests/Feature/Actions/Logging/ConvertDiscordTest.php new file mode 100644 index 0000000..a3514a9 --- /dev/null +++ b/tests/Feature/Actions/Logging/ConvertDiscordTest.php @@ -0,0 +1,63 @@ +callProtected($converter, 'addMessageContent', [$message, $this->record()]); + + // スタックトレースが取得・添付されていること（先に scrub していたら null になっていたはず）。 + $this->assertNotNull($message->file); + $this->assertStringNotContainsString('ntn_supersecret', (string) ($message->content ?? '')); + } + + public function test_inherited_add_message_stacktrace_does_not_overwrite_with_raw_trace(): void + { + $converter = new ConvertDiscord(app('config'), new SecretScrubber); + $message = Message::make(); + $message->file('[REDACTED-SENTINEL]', 'f.txt'); + + // 親クラスの addMessageStacktrace が未伏字化の生トレースで上書きしないこと（no-op であること）。 + $this->callProtected($converter, 'addMessageStacktrace', [$message, $this->record()]); + + $this->assertSame('[REDACTED-SENTINEL]', $message->file['contents']); + } + + private function record(): array + { + return [ + 'datetime' => new \DateTime, + 'level_name' => 'ERROR', + 'message' => 'sync failed with ntn_supersecret', + 'context' => [ + 'exception' => new \RuntimeException('boom'), + ], + ]; + } + + private function callProtected(object $object, string $method, array $args): mixed + { + $reflection = new \ReflectionMethod($object, $method); + + return $reflection->invoke($object, ...$args); + } +} From 7e951fe1a0409c7d8bc07e5bbe4378ab50b07f21 Mon Sep 17 00:00:00 2001 From: GitHub Action Date: Mon, 22 Jun 2026 10:59:45 +0000 Subject: [PATCH 4/5] [rector] Rector fixes --- tests/Feature/Actions/Logging/ConvertDiscordTest.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/Feature/Actions/Logging/ConvertDiscordTest.php b/tests/Feature/Actions/Logging/ConvertDiscordTest.php index a3514a9..5c16a2f 100644 --- a/tests/Feature/Actions/Logging/ConvertDiscordTest.php +++ b/tests/Feature/Actions/Logging/ConvertDiscordTest.php @@ -20,10 +20,10 @@ public function test_stacktrace_is_extracted_even_though_context_gets_scrubbed() { Config::set('services.notion.secret', 'ntn_supersecret'); - $converter = new ConvertDiscord(app('config'), new SecretScrubber); + $convertDiscord = new ConvertDiscord(app('config'), new SecretScrubber); $message = Message::make(); - $this->callProtected($converter, 'addMessageContent', [$message, $this->record()]); + $this->callProtected($convertDiscord, 'addMessageContent', [$message, $this->record()]); // スタックトレースが取得・添付されていること（先に scrub していたら null になっていたはず）。 $this->assertNotNull($message->file); @@ -32,12 +32,12 @@ public function test_stacktrace_is_extracted_even_though_context_gets_scrubbed() public function test_inherited_add_message_stacktrace_does_not_overwrite_with_raw_trace(): void { - $converter = new ConvertDiscord(app('config'), new SecretScrubber); + $convertDiscord = new ConvertDiscord(app('config'), new SecretScrubber); $message = Message::make(); $message->file('[REDACTED-SENTINEL]', 'f.txt'); // 親クラスの addMessageStacktrace が未伏字化の生トレースで上書きしないこと（no-op であること）。 - $this->callProtected($converter, 'addMessageStacktrace', [$message, $this->record()]); + $this->callProtected($convertDiscord, 'addMessageStacktrace', [$message, $this->record()]); $this->assertSame('[REDACTED-SENTINEL]', $message->file['contents']); } @@ -56,8 +56,8 @@ private function record(): array private function callProtected(object $object, string $method, array $args): mixed { - $reflection = new \ReflectionMethod($object, $method); + $reflectionMethod = new \ReflectionMethod($object, $method); - return $reflection->invoke($object, ...$args); + return $reflectionMethod->invoke($object, ...$args); } } From 98caddc71e9037710299efa28f10c9716ba73b13 Mon Sep 17 00:00:00 2001 From: 128na Date: Mon, 22 Jun 2026 20:11:17 +0900 Subject: [PATCH 5/5] Fix Notion sync isolation gaps, scrub threshold, and DB index MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - SyncAction::deleteOldNotionPages: a Notion page with a null URL (e.g. a manually created draft) no longer gets deleted — only pages with a non-null URL absent from our DB are removed. - SyncAction::addNewNotionPages: pre-key Notion pages by URL once (keyNotionPagesByUrl()) instead of calling Collection::first() with a closure that calls getUrlProp() per page inside the loop. That closure could throw on a single malformed Notion page and take down the entire $pages loop, defeating the per-item fault isolation this PR introduced. The new helper isolates URL-parsing failures per Notion page and also cuts the lookup from O(N*M) to O(N+M). - SecretScrubber: raise the minimum scrub length from 4 to 5 so a 4-char dev password like "root" doesn't redact unrelated words (chroot, uproot, root user, etc). Added a regression test pinning this. Documented the Throwable->string side effect in walk() for future Monolog processor ordering. - Migration: index extract_failed_at since it will be queried via whereNotNull/whereNull as the table grows. Co-Authored-By: Claude Sonnet 4.6 --- app/Actions/Logging/SecretScrubber.php | 9 ++++-- app/Actions/SyncNotion/SyncAction.php | 31 +++++++++++++++++-- ...d_extract_failed_at_to_raw_pages_table.php | 3 +- .../Actions/Logging/SecretScrubberTest.php | 11 +++++++ 4 files changed, 48 insertions(+), 6 deletions(-) diff --git a/app/Actions/Logging/SecretScrubber.php b/app/Actions/Logging/SecretScrubber.php index ee04dbd..304f767 100644 --- a/app/Actions/Logging/SecretScrubber.php +++ b/app/Actions/Logging/SecretScrubber.php @@ -64,6 +64,9 @@ private function walk(mixed $value, array $secrets): mixed if ($value instanceof \Throwable) { // 例外オブジェクトはメッセージ + トレース文字列に展開してから伏字化する。 + // 注意: これにより context['exception'] は Throwable から string に変わる。 + // Sentry 等、Throwable のままであることを期待する Monolog processor/handler を + // 将来追加する場合は、本プロセッサより前段に置くこと。 return str_replace($secrets, self::MASK, (string) $value); } @@ -92,9 +95,9 @@ private function secrets(): array return $this->cachedSecrets = array_values(array_unique(array_filter( $candidates, - // 未設定(null)や開発環境の "root" 等の短い値まで伏字化すると - // ログ本文を無関係な部分まで壊してしまうため、4文字未満は対象外にする。 - fn (mixed $value): bool => is_string($value) && mb_strlen($value) >= 4, + // 未設定(null)や開発環境の "root"(4文字) 等の短い値まで伏字化すると + // chroot/uproot 等の無関係な単語まで壊してしまうため、5文字未満は対象外にする。 + fn (mixed $value): bool => is_string($value) && mb_strlen($value) >= 5, ))); } } diff --git a/app/Actions/SyncNotion/SyncAction.php b/app/Actions/SyncNotion/SyncAction.php index 8c24168..cbc2950 100644 --- a/app/Actions/SyncNotion/SyncAction.php +++ b/app/Actions/SyncNotion/SyncAction.php @@ -58,7 +58,8 @@ private function deleteOldNotionPages(Collection $pages, Collection $notionPages $url = null; try { $url = $this->getUrlProp($notionPage); - if ($url === null || ! $pagesByUrl->has($url)) { + // url が無い Notion ページ（手動作成の下書き等）は削除対象にしない。 + if ($url !== null && ! $pagesByUrl->has($url)) { logger('[NotionService]delete', ['url' => $url]); $this->notion->pages()->delete($notionPage); } @@ -80,11 +81,13 @@ private function addNewNotionPages(Database $database, Collection $pages, Collec { $options = $this->getOptions($database); $failed = 0; + $notionPagesByUrl = $this->keyNotionPagesByUrl($notionPages); + foreach ($pages as $page) { try { $exists = true; $url = $page->url; - $np = $notionPages->first(fn (NotionPage $notionPage): bool => $this->getUrlProp($notionPage) === $url); + $np = $notionPagesByUrl->get($url); if (! $np) { $exists = false; $np = NotionPage::create(PageParent::database($database->id)); @@ -131,6 +134,30 @@ private function addNewNotionPages(Database $database, Collection $pages, Collec return $failed; } + /** + * url 取得に失敗した Notion ページ（プロパティ欠落等）が 1 件あっても + * 残りのページの同期を止めないよう、ここで隔離してマップ化する。 + * + * @param Collection $notionPages + * @return Collection + */ + private function keyNotionPagesByUrl(Collection $notionPages): Collection + { + $byUrl = collect(); + foreach ($notionPages as $notionPage) { + try { + $url = $this->getUrlProp($notionPage); + if ($url !== null) { + $byUrl->put($url, $notionPage); + } + } catch (\Throwable $th) { + logger()->error('[NotionService] failed to read url property', [$th]); + } + } + + return $byUrl; + } + private function getUrlProp(NotionPage $notionPage): ?string { $property = $notionPage->getProperty(self::PAGE_PROP_MAPPING['url']); diff --git a/database/migrations/2026_06_22_000000_add_extract_failed_at_to_raw_pages_table.php b/database/migrations/2026_06_22_000000_add_extract_failed_at_to_raw_pages_table.php index 6e471e8..ca26f9c 100644 --- a/database/migrations/2026_06_22_000000_add_extract_failed_at_to_raw_pages_table.php +++ b/database/migrations/2026_06_22_000000_add_extract_failed_at_to_raw_pages_table.php @@ -12,7 +12,8 @@ public function up(): void { Schema::table('raw_pages', function (Blueprint $table): void { // 抽出失敗を隔離・可視化する。失敗時に行を削除する代わりにここへ記録する。 - $table->timestamp('extract_failed_at')->nullable()->after('html'); + // whereNotNull/whereNull で頻繁に絞り込まれるため index を付与する。 + $table->timestamp('extract_failed_at')->nullable()->after('html')->index(); }); } diff --git a/tests/Feature/Actions/Logging/SecretScrubberTest.php b/tests/Feature/Actions/Logging/SecretScrubberTest.php index 58606e3..9d92c08 100644 --- a/tests/Feature/Actions/Logging/SecretScrubberTest.php +++ b/tests/Feature/Actions/Logging/SecretScrubberTest.php @@ -50,6 +50,17 @@ public function test_scrub_array_recurses_and_masks_throwable(): void $this->assertStringNotContainsString('discord.com/api/webhooks/abc/xyz', $encoded); } + public function test_scrub_does_not_mangle_common_words_when_secret_is_short(): void + { + Config::set('database.connections.mysql.password', 'root'); + + $secretScrubber = new SecretScrubber; + + // "root"(4文字) のような開発環境の短いパスワードは伏字化対象に含めない。 + // chroot/uproot 等の無関係な単語まで破壊してしまうため。 + $this->assertSame('chroot jail for root', $secretScrubber->scrub('chroot jail for root')); + } + public function test_scrub_is_noop_when_no_secrets_configured(): void { Config::set('services.notion.secret', '');