Repository navigation
fix(audit,models): PHI・長さ上限の検出網の穴を塞ぎ、長さ管理を監査対象から切り離す - #184
Merged
Merged
Conversation
FreeTextMaxLengthAttributeTests が見ていたのは「[Sensitive] が付いた string プロパティ」だけだった。PR #183 で PHI 分類に [NotPhi] という 2 つ目の正当な選択肢が 増えた時点で、この形が検出網の穴になっていた。 新しい string 列に [NotPhi("...")] だけ付けて [MaxLength] を書き忘れると、PHI 分類 テストは「分類済み」として緑、長さ上限テストは「[Sensitive] が無いので対象外」として緑 になり、無制限の文字列列がどちらにも引っかからず通る。しかも [NotPhi] 列は定義上マスク されないので、その無制限の値は AuditLog.ChangesJson へ平文でも積まれる(行と監査ログの 二重、かつ AuditLog は追記専用で後から消せない)。エスケープハッチを足したぶん既存の 検出網が黙って狭くなるという、#183 自身が塞いだのと同じ形の後退だった。 検査条件を分類の種類から切り離し、「監査対象エンティティで永続化される string 列は すべて [MaxLength] を持つ」に変えた。列の一覧は CLR のリフレクションではなく EF Core の モデルから引く(PHI 分類テストと同じ源)ので、将来 3 つ目の分類が増えても穴は空かない。 あわせて shadow property(CLR プロパティを持たない列)を専用検査へ切り出した。 LookupSensitiveMask は CLR プロパティの [Sensitive] を読むため shadow の string 列は 原理的にマスクできず必ず平文で書かれるが、従来はこれが分類漏れとして「[Sensitive] を 付けてください」という実行不能な指示で落ちていた。実行不能な指示を出す検出網は、いずれ 「直せないので検査を緩める」方向へ倒れるため、対処法(CLR プロパティへ昇格)が違う以上は 別の検査が固有のメッセージで落とす形にし、他の 2 検査は shadow 列を対象から外した。 列名と CLR プロパティの突き合わせは AuditedEntityModel へ集約し、検査ごとに BindingFlags / inherit の指定がずれるのを防いだ。 変異 2 通り([MaxLength] 無しの [NotPhi] 列 / shadow string 列)で、それぞれ対応する 検査だけが赤になることを実測済み。前者は変更前のテストでは全件緑になることも確認した。 dotnet restore --locked-mode / build (警告 0) / test 479 件 / npm run typecheck すべて緑。 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AAEwHDpPsNzmEnxHPmzKf5
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR #184 のレビュー指摘 5 件を反映する。いずれも「穴が無いことを根拠にしている検出網」に 残っていた穴。 1. 文字列列の判定が値変換を見落としていた(最重要) IsStoredAsString が CLR の型だけを見ていたため、HasConversion で文字列として保存される 列(Severity / IncidentType / Status / MeasureType)が分類・長さ上限の両検査から丸ごと 外れていた。将来 HasConversion<string>() で保存する自由記述の値オブジェクトを足すと、 マスクされないまま ChangesJson へ積まれても両テストが緑のまま通る。 実測すると、変換後の型が現れる場所は書き方によって違った: - HasConversion<string>() → GetProviderClrType() に出て GetValueConverter() は null - HasConversion(v => …, v => …) → GetValueConverter() に出て GetProviderClrType() は null 片方だけ見る実装だと、もう一方の書き方の列が素通りする(最初の修正案がこれで、 4 列のうち 1 列しか検出できていなかった)。両方を見るようにした。 これで上記 4 列が検査対象に入るため、閉じた語彙である旨を [NotPhi] で明示した。 2. FindClrProperty の BindingFlags が本番と違っていた 本番の LookupSensitiveMask は Public|Instance|NonPublic で解決するのに、検査側は Public|Instance だった。非公開プロパティを列にマップすると「本番は [Sensitive] を読んで マスクするのに、検査からは shadow property に見える」というずれが起き、実在して属性も 付いている列へ「CLR プロパティへ昇格させてください」という実行不能な指示を出していた。 ——ヘルパーへ集約した理由そのものを、その集約先が破っていた。 3. 長さ上限の判定を EF のモデルへ寄せた [MaxLength] 属性だけを見ると、fluent の HasMaxLength() で設定した列を「上限なし」と 誤判定する。DB の列長を決めているのはモデル側の値なので GetMaxLength() を読む。 4. AuditLogsController.AllowedEntityNames が監査対象一覧の 3 つ目の写しだった 監査対象を足すとインターセプタと 2 つのテストだけが追随し、ここが取り残される。 結果はフィルタの fail-open(Contains が false になり絞り込みが黙って外れて全件返る)と ドロップダウン欠落(証跡に書かれた行へ画面から到達できない)の二重のずれ。宣言から 導出するようにした。HashSet の列挙順は保証されないので序数順に固定する。 5. 写し・重複の解消 FieldLengthsTests の型一覧も監査対象を宣言から導出するようにし(裸の [MaxLength] 検査 だけが新集約に追随しない状態を防ぐ)、2 つのテストクラスに写経されていた TheoryData の 詰め替えをヘルパーへ集約。EF のモデルは Lazy で 1 回だけ組み立て、検査ごとに InMemory ストアをプロセス内キャッシュへ積み増すのをやめた。 変異 3 通り(値変換された未分類の列 / [MaxLength] 無しの [NotPhi] 列 / shadow string 列)で、 それぞれ対応する検査だけが赤になることを実測済み。 dotnet restore --locked-mode / build (警告 0) / test 479 件 / npm run typecheck すべて緑。 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AAEwHDpPsNzmEnxHPmzKf5
PR #184 の 2 巡目レビュー指摘 3 件を反映する。 1. EnumLabels.AuditEntityJa が導出できない 4 つ目の写しだった AllowedEntityNames を宣言から導出するようにしたぶん、そのすぐ隣で使う日本語ラベルの 変換表だけが手書きの写しとして残った。JapaneseAuditEntity は辞書に無いキーを元の値の まま返すフォールバックを持つため、監査対象を足してラベルを書き忘れても例外にならず、 監査ログ画面の 3 箇所(ドロップダウン / 一覧の各行 / 詳細)に CLR の型名が英語のまま出る。 ビルドも全テストも緑のまま通る。 フォールバック自体は残す(監査対象から外したエンティティの過去行を表示するときに、 例外で画面を落とすより元の値を出す方が安全)。代わりに AuditEntityLabelCoverageTests が 「ラベル表は監査対象を全網羅する」ことを機械的に固定する。 2. fluent の HasMaxLength() が裸の数値の抜け道になっていた 前コミットで上限の充足判定を EF のモデル(GetMaxLength())へ寄せたため、fluent で 設定した上限も「上限あり」として通るようになった。ところが裸の数値を検出する FieldLengthsTests は CLR の [MaxLength] 属性しか見ていないので、「上限はある(緑)/ その値は FieldLengths 由来ではない(誰も見ていない)」という状態が作れてしまう。 実際 ApplicationDbContext の 20 / 50 が既にその状態だった。 ——エスケープハッチを足したぶん既存の検出網が黙って狭くなるという、この PR 自身が 塞いだのとまったく同じ形。属性側と同じ許容集合でモデル側も見る検査を足し、 20 / 50 を FieldLengths.EnumCode / EnumCodeJapanese として名前付き定数にした (値は変えていないのでスキーマは不変。マイグレーション不要)。 3. PartitionStringColumns のコメントが実際の挙動を過大に述べていた 「呼び出し側が同じ走査を 2 回しない」と書いていたが、2 つの薄い射影はそれぞれ独立に 呼ぶので両方使えば 2 回走る。実際の挙動と、メモ化せず素直に再計算する判断の理由へ直した。 なお同レビューが挙げた EffectivenessScale の文言と EfCorePackageAlignmentTests の 2 件は 本 PR の差分ではなくマージ済みコミット(#182 / #181)由来のため、ここでは扱わない。 変異 2 通り(ラベル表から 1 件落とす / fluent の上限を裸の 48 へ戻す)で、それぞれ対応する 検査だけが赤になることを実測済み。 dotnet restore --locked-mode / build (警告 0) / test 484 件 / npm run typecheck すべて緑。 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AAEwHDpPsNzmEnxHPmzKf5
PR #184 の 3 巡目レビュー指摘 5 件を反映する。1 件目は前コミットで自分が入れた後退。 1. 文字列列の判定を「変換後の型」へ置き換えたのが誤りだった(最重要・自己回帰) 前コミットは CLR 型の判定を変換後の型の判定へ **置き換えて** しまったが、 SerializeChanges が ChangesJson へ書くのは prop.CurrentValue / OriginalValue、 すなわち変換**前**の CLR 側の値。つまり PHI が漏れるかどうかは CLR の型に付いて回る。 この置き換えのせいで、自由記述列に非文字列の値変換(暗号化 string → byte[] など)を 足すと「文字列として保存されない」列になって検出網から丸ごと外れる一方、ChangesJson へは 相変わらず平文の string が流れる、という取りこぼしが生まれていた。 置き換えではなく **和集合**(CLR が string である、または文字列として保存される)にした。 2. 属性側とモデル側で許容値の配列が別々だった 前コミットが EnumCode / EnumCodeJapanese をモデル側の配列にだけ足したため、 [MaxLength(FieldLengths.EnumCode)] を書くと属性側の検査が「FieldLengths 以外の裸の数値」 だと訴える矛盾した状態になっていた(CLAUDE.md には「両方を同じ許容集合で見る」と書いた のに、コードはそうなっていなかった)。1 つの配列 AllowedLengths を共有する。 3. モデル側の検査が監査対象だけに絞られていた CauseCategory は監査対象ではないが EF のモデルを持つ。属性側は [MaxLength] しか見ず、 モデル側は監査対象しか見ないため、CauseCategory に fluent で裸の数値を書くとどちらの 検査も素通りする——塞いだはずの抜け道が別の型で開いたまま。対象を「上限規約に従う型の うち EF のモデルを持つもの」に広げた。 4. ドロップダウンの表示順を黙って変えていた 前コミットは HashSet の列挙順が不定なことへの対処として型名の序数順に並べ替えたが、 その結果 日本語 UI の並びが英語の識別子の綴り順(原因分析がインシデントより先)という 利用者から見て無意味な順になっていた。AuditedEntities 自体を順序付きの IReadOnlyList に して宣言順(ルート集約が先)を正とし、導出先は並べ替えない。 順序を持たせた代わりに重複が書けるようになるので、重複を禁じる検査を足した。 5. AuditedEntities のコメントが導出先の一部しか挙げていなかった 4 つのテストと AuditLogsController のうち 2 つのテストしか書いておらず、 写しを無くす PR の中で新しい写しを作っていた。一覧は CLAUDE.md へ集約し、そこを指す。 なお同レビューが挙げた EffectivenessScale の文言 / EfCorePackageAlignmentTests / NotPhiAttribute の実行時例外の 3 件は本 PR の差分ではなくマージ済みコミット由来のため、 ここでは扱わない。 変異 1 通り(自由記述列に string → byte[] の値変換を足し [Sensitive] を外す)で、 和集合にした分類検査が正しく赤になることを実測済み(置き換えのままなら緑で素通りした)。 dotnet restore --locked-mode / build (警告 0) / test 486 件 / npm run typecheck すべて緑。 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AAEwHDpPsNzmEnxHPmzKf5
PR #184 の 4 巡目レビュー指摘 7 件を反映する。1 件目は前コミットで自分が入れた後退。 1. 許容値の配列を共有したせいで属性側の検査が緩んでいた(自己回帰) 前コミットは「2 つの検査が別々の配列を持つと矛盾する」を理由に 1 つへまとめたが、 まとめ方が誤りで、属性側の許容値に EnumCode(20) / EnumCodeJapanese(50) が混ざった。 結果、ViewModel の入力欄に裸の [MaxLength(50)] を書いても「FieldLengths の定数だ」と して通るようになっていた——対応するエンティティ側は ShortText(100) のままなので、 FieldLengths が防ぐために作られた層またぎのずれがそのまま復活する。 2 つの集合は**意図的に別**にするのが正しい。属性側は {FreeText, ShortText}、 モデル側はそれに値変換した enum 列専用の 2 つを足したもの。理由をコメントに残した。 2. 値変換した enum 列が切り詰められる経路に検査が無かった モデル側の検査が裸の数値を禁じることで FieldLengths.EnumCode の使用を積極的に 誘導する以上、定数が実際の値より短いと誘導に従った結果として切り詰めが起きる。 壊れ方はプロバイダ依存(SQL Server / PostgreSQL は例外、SQLite は黙って保存、 テストの InMemory は列長の概念すら無い)で、緑のまま本番でだけ壊れる。 ConvertedEnumColumnLengthTests が、変換器を実際に通した文字列の長さを上限と 突き合わせて固定する。 なおこの検査の初版は GetValueConverter() しか見ておらず、EnumCode を 5 へ縮める変異で 赤にならなかった(HasConversion<string>() の列が素通り)。同じ PR で 2 度目の同型の 取りこぼしなので、両方の書き方を見る形に直したうえで変異で赤を実測した。 3. shadow 列が裸の数値の検査から外れていた モデル側の検査が ClrBackedStringColumns を使っていたため、fluent で裸の数値を設定した shadow 列が「属性を付けられないから対象外」という無関係な理由で素通りしていた。 属性を読まない検査なので shadow 列も対象にできる。全文字列列を見る形に直した。 4. ラベル網羅の判定がフォールバックの観測に依存していた 変換結果と入力の一致で「未定義」と判定していたため、ラベルを意図的に型名と同じに したときに実在するのに直しようのない失敗になる。辞書のキーを公開して直接照合する。 5〜7. 写し・重複の解消 ToTheoryData の 3 つ目の写しを共有ヘルパーへ集約、GovernedTypes の重複を排除 (監査対象に CauseCategory を足すと同じ型のケースが 2 つできる)、分類テストが 解決済みの PropertyInfo を使わず同じ列名でリフレクション探索をやり直していたのを直した。 CLAUDE.md の「EF のモデルを持つ型すべて」という過大な記述も実態へ合わせた。 なお同レビューの「AllowedLengths を FieldLengths からリフレクションで導出せよ」は採らない。 全 int 定数を導出すると属性側に EnumCode / EnumCodeJapanese が入り、1 の後退がそのまま 復活するため(2 つの集合が別であることに意味がある)。 変異 1 通り(EnumCode を 5 へ縮める)で切り詰め検査が赤になることを実測済み。 dotnet restore --locked-mode / build (警告 0) / test 489 件 / npm run typecheck すべて緑。 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AAEwHDpPsNzmEnxHPmzKf5
PR #184 の 5 巡目レビュー指摘 10 件を反映する。レビューは 3 件を「変異を当てても緑だった」 として実測で示しており、いずれも実在の穴だった。 1. 長さ上限の管理範囲を監査対象から導出していた(設計の誤り) 「どのエンティティを監査するか」と「どのエンティティの列長を管理するか」は別の関心事。 前者から後者を導いていたため、監査ポリシーの変更(あるエンティティを監査対象から外す)が 無関係な長さ管理まで黙って外す状態だった——裸の [MaxLength(200)] も、上限の付け忘れも、 値変換列の切り詰めも、まとめて素通りになる(すべて fail-open)。 LengthGovernedEntityTypes() を新設し、「自分たちのモデル名前空間にあるマップ済み エンティティ」から AuditLog(監査証跡固有の列長)と ApplicationUser(Identity が決める)を 除いた集合を EF のモデルから導出する。新しいエンティティは何もしなくても検査対象に入る。 2. その結果、上限なしの列が実在していたことが判明した CauseCategory は監査対象ではないため、どの検査も届いておらず、CauseCategory.Description が 上限なし(nvarchar(max) / text 相当)のまま残っていた。範囲を広げた検査が即座に検出した。 FieldLengths.FreeText を付け、同一変更セットでマイグレーションを追加している (SQLite は TEXT に長さ制約が無いため本体は空。目的はスナップショットへの記録で、 その旨をマイグレーションのコメントに明記した)。 3. [MaxLength] と fluent の HasMaxLength() が食い違っても誰も見ていなかった 上限の充足をモデルで判定するようにした結果、両方書いて値が違ってもどちらも「上限あり」で 緑になる。fluent が優先されるので「画面は属性の上限で検証し、DB は fluent の上限で作られる」 層またぎのずれが起きる。一致そのものを固定する検査を足した。 4. 切り詰め検査に網羅ガードが無かった 判定が対象を拾えなくなると全列が読み飛ばされ「違反ゼロ=緑」で無力化される。 初版のガードは「1 件でも見たか」で書いたが、変異(判定を半分に戻す)を当てても IncidentType だけは拾えるため緑のままだった。判定とは**独立な手がかり** (enum 型でかつ長さ上限を持つ永続化列)で「見るべき列を全部見たか」を照合する形に直し、 同じ変異で Severity / Status / MeasureType の 3 件を名指しして赤になることを実測した。 5. AuditedEntities が配列へのキャストで書き換え可能だった IReadOnlyList で公開しても中身が配列のままだと ((string[])AuditedEntities)[0] = "..." で 監査対象を消せる(Incident が一致しなくなり監査ログが黙って書かれなくなる)。 Array.AsReadOnly で包んだ。 6〜10. 範囲・写し・重複の整理 切り詰め検査の範囲を「誘導する検査」と同一にそろえ(誘導する範囲より検証が狭いと差分が 死角になる)、文字列列の判定と EF モデルの組み立てを共有ヘルパー経由に統一(写しが 片方だけ取り残されるのを防ぐ/InMemory ストアの積み増しをやめる)、CLAUDE.md の 導出先一覧を実態に合わせた。 変異 5 通り([NotPhi] 上限なし / shadow 列 / EnumCode 切り詰め / ラベル欠落 / 属性と fluent の食い違い)で、それぞれ対応する検査だけが赤になることを実測済み。 dotnet restore --locked-mode / build (警告 0) / test 496 件 / npm run typecheck すべて緑。 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AAEwHDpPsNzmEnxHPmzKf5
This branch had an error being deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
コードレビューで見つけた、PR #183 が副作用として開けた検出網の穴を起点に、6 巡のレビューで芋づる式に見つかった穴をすべて塞ぐ。
発端は、
FreeTextMaxLengthAttributeTestsが見ていたのが「[Sensitive]が付いた string プロパティ」だけだったこと。#183 で PHI 分類に[NotPhi]という 2 つ目の正当な選択肢が増えた時点で、この形が穴になっていた。新しい string 列に[NotPhi("...")]だけ付けて[MaxLength]を書き忘れると:AuditedEntityPhiClassificationTests[NotPhi]があるので「分類済み」FreeTextMaxLengthAttributeTests[Sensitive]が無いのでそもそも対象外エスケープハッチを足したぶん既存の検出網が黙って狭くなるという、#183 自身が塞いだのと同じ形の後退だった。この 1 件を追ううちに、同じ形の穴が本番コードにも実在していることが分かった。
実際に見つかった「本物」
CauseCategory.Descriptionが上限なしのまま残っていた —CauseCategoryは監査対象ではないため、どの検査も届いていなかった。nvarchar(max)/text相当の無制限列。範囲を広げた検査が即座に検出した。AuditedEntitiesがキャスト 1 つで書き換え可能だった —IReadOnlySetで公開しても中身がHashSetのままなので、((HashSet<string>)AuditedEntities).Clear()で監査ログを丸ごと黙らせられた。AuditLogsController.AllowedEntityNamesが監査対象一覧の写しだった — 監査対象を足すと取り残され、フィルタの fail-open(絞り込みが黙って外れ全件返る)+ドロップダウン欠落(証跡の行へ画面から到達できない)が同時に起きる。Severity/IncidentType/Status/MeasureType)が両検査から丸ごと外れていた — CLR 型だけで絞っていたため。主な変更
1. 検出網の判定を「和集合」にする
「文字列列」の判定を CLR 型が
string∪ DB へ文字列として保存される に。片方への置き換えでは外れた側が誰にも見られなくなる(本命は CLR 側 —SerializeChangesがChangesJsonへ書くのは変換前の値なので、暗号化などでstring → byte[]に変換した自由記述列は「文字列として保存されない」のに平文が監査ログへ流れる)。さらに変換後の型が現れる場所は書き方で違う(実測):
HasConversion<string>()はGetProviderClrType()、HasConversion(v => …, v => …)はGetValueConverter()。片方だけ見ると、もう一方の書き方の列が素通りする。2. 長さ上限の管理範囲を監査対象から切り離す
「どのエンティティを監査するか」と「どのエンティティの列長を管理するか」は別の関心事。前者から後者を導くと、監査ポリシーの変更が無関係な長さ管理まで黙って外す(fail-open)。
LengthGovernedEntityTypes()が EF のモデルから名前空間ベースで導出するので、新しいエンティティは何もしなくても検査対象に入る。3. shadow property を専用検査へ切り出す
LookupSensitiveMaskは CLR プロパティの[Sensitive]を読むため、shadow の string 列は原理的にマスクできず必ず平文で書かれる。従来はこれが「分類漏れ」として[Sensitive]を付けろという実行不能な指示で落ちていた。実行不能な指示を出す検出網は、いずれ「直せないので検査を緩める」方向へ倒れるため、対処法(CLR プロパティへ昇格)が違う以上は別の検査が固有のメッセージで落とす。4. 検出網そのものが死んでいないかを見張る
EveryLengthLimitedEnumColumn_IsActuallyExaminedが「見るべき列を全部見たか」を、判定とは独立な手がかり(enum 型でかつ長さ上限を持つ永続化列)で照合する。同じ判定でガードを書くと、判定が狭まったときにガードも一緒に狭まって「違反ゼロ=緑」で無力化されるため。5. その他
[MaxLength]と fluentHasMaxLength()の値の一致を固定 / 属性側とモデル側の許容値を意図的に別集合に / ラベル表の網羅性を固定 /Array.AsReadOnlyで監査対象を真に不変に / 写しと重複の集約。検証
変異テスト 8 通り — それぞれ対応する検査だけが赤になることを実測。とくに次の 2 つは、修正前は全件緑で素通りしたことを確認している:
[MaxLength]無しの[NotPhi]列PersistedStringColumns_MustHaveMaxLengthPersistedStringColumns_MustHaveBackingClrPropertystring → byte[]変換+[Sensitive]除去EnumCodeを 5 へ縮めるSeverity/Status/MeasureTypeを名指し)ModelMaxLength_AgreesWithMaxLengthAttributeEveryAuditedEntity_HasJapaneseLabelCI と同じコマンドをローカルで実行、すべて緑:
CI 3 ジョブ(build-and-test / Docker image build & smoke test / Vercel)もすべて success。
注意(デプロイ時)
CauseCategory.Descriptionに上限を付けたため、Migrations/を PostgreSQL / SQL Server 向けに再生成する配備ではvarchar(500)になる。既存行が 500 文字を超えているとマイグレーションが失敗する(Postgres は切り詰めずエラー)。シードのマスタデータは十分短いため実害は想定していないが、runbook に記載しておくのが安全。既定の SQLite は TEXT に長さ制約が無いためマイグレーション本体は空。🤖 Generated with Claude Code
https://claude.ai/code/session_01AAEwHDpPsNzmEnxHPmzKf5