Skip to content
This repository was archived by the owner on Aug 26, 2026. It is now read-only.

[Phase1] RLSポリシー実装(権限マトリクス準拠) - #31

Merged
reitojike merged 5 commits into
mainfrom
25-phase1-rls-policies
Aug 6, 2026
Merged

reitojike merged 5 commits into
mainfrom
25-phase1-rls-policies

Conversation

@reitojike

Copy link
Copy Markdown
Owner

概要

Issue #25 の対応。docs/permissions.md の権限マトリクスと docs/data-model.md「RLSポリシー方針」に基づき、全6テーブルにRLSポリシーを実装する。権限の設計自体は決定済みで、このPRは実装のみ。

変更内容

supabase/migrations/20260806034559_rls_policies.sql を追加。

  • profiles: 全員SELECT可、本人のみINSERT/UPDATE
  • events: deleted_at IS NULL の行のみ全員SELECT可、owner_idのみINSERT/UPDATE
  • event_participants: 本人の行 + visibility='public'の行のSELECT、自己登録(invited_by IS NULL)または参加登録済みユーザーによる招待(invited_by=自分)のみINSERT、本人のみUPDATE/DELETE
  • ticket_entries / expenses / budgets: 本人のみ全操作
  • イベント削除ガード(オーナー以外の参加者が1人でもいたら削除不可、例外なし)はBEFORE UPDATEトリガーで実装。RLSのWITH CHECKだけだとOLD/NEWの差分(「deleted_atをNULLから設定する更新か」)の判定がしづらいため
  • 全ポリシーはto authenticatedのみを対象(このアプリはGoogle SSOログイン前提で、匿名ユーザー向けの公開閲覧機能は無い)
  • profiles.is_adminを参照する条件は一切書いていない(docs/permissions.mdでMVPスコープ外と明記)

このPRに含まないもの: test/db/でのRLS検証テスト → 別Issue(#26)

注意

この環境にはDockerが無く、ポリシーが実際に意図通り機能するかはローカルで確認できない。CI(db-testジョブでマイグレーションが構文エラーなく適用されるか)で検証する。実際の権限マトリクス通りの動作検証(否定側を含む)は#26で行う。

Test plan

  • db-testジョブでマイグレーション(RLSポリシー・トリガー含む)が構文エラーなく適用される

docs/permissions.mdの権限マトリクスとdocs/data-model.mdの
「RLSポリシー方針」に基づき、全6テーブルにRLSを実装。
- profiles: 全員SELECT可、本人のみINSERT/UPDATE
- events: 未削除は全員SELECT可、owner_idのみINSERT/UPDATE
- event_participants: 本人の行+公開行のSELECT、自己登録/招待のINSERT、本人のみUPDATE/DELETE
- ticket_entries/expenses/budgets: 本人のみ全操作
- イベント削除ガード(オーナー以外の参加者がいたら削除不可)はトリガーで実装
  (RLSのWITH CHECKだけではOLD/NEWの差分判定が煩雑なため)
- profiles.is_adminを参照する条件は一切書いていない(MVPスコープ外)

test/db/での検証は別Issue(#26)。

Closes #25
Copilot AI lite review requested due to automatic review settings August 6, 2026 03:46
Comment on lines +41 to +59
create function public.guard_event_deletion()
returns trigger
language plpgsql
set search_path = public
as $$
begin
if new.deleted_at is not null and old.deleted_at is null then
if exists (
select 1
from public.event_participants ep
where ep.event_id = new.id
and ep.user_id <> new.owner_id
) then
raise exception 'event % has participants other than the owner and cannot be deleted', new.id;
end if;
end if;
return new;
end;
$$;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

削除ガードがRLS越しに沈黙してすり抜ける可能性があります。

guard_event_deletion()security definer を付けていないため、内部の
exists (select 1 from public.event_participants ep where ...)
呼び出し元(イベントを削除しようとしているオーナー)の権限、つまり
event_participants_select_own_or_public ポリシーの下で実行されます。

このポリシーは user_id = auth.uid() or visibility = 'public' の行しか見せません。
つまり、オーナー以外の参加者が visibility = 'private' で参加登録している場合、
このexists検査からはその行が見えず「参加者はいない」と判定されてしまい、
docs/data-model.md 2章・docs/permissions.md が要求する
「オーナー本人以外の参加者が1人でもいる場合は削除不可(例外なし)」の
ガードを静かにすり抜けて論理削除が成功してしまいます。

同じマイグレーション内の handle_new_user() (20260806003241_initial_schema.sql)は
まさに同じ理由(呼び出し元のRLSに関わらず内部で判定する必要がある)で
security definer を付けています。こちらの関数にも同様に
security definer が必要ではないでしょうか。

docs/permissions.md の「最小の検証セット」に "参加者がいるイベントをオーナーが削除 → 失敗" が
挙げられていますが、参加者のvisibilityがprivateのケースでこのテストを書けば
今のロジックのままでは赤くなるはずです。

Comment on lines +18 to +21
create policy "profiles_update_self" on public.profiles
for update to authenticated
using (id = auth.uid())
with check (id = auth.uid());

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

profiles_update_selfis_admin カラムの書き換えを制限していません。

using (id = auth.uid()) / with check (id = auth.uid()) は行の所有者しか
見ていないため、本人が自分の行に対して

update profiles set is_admin = true where id = auth.uid();

を実行すると通ってしまいます。

docs/permissions.md は「is_adminを参照する権限判定はRLSにもアプリ層にも一切書かない」と
明記していますが、これは「参照して分岐を作るな」という話であって、
「本人が自分でis_admin列を書き換えられる状態のまま放置してよい」ことまでは
意味していないはずです。現時点ではどこもis_adminを参照していないので実害はまだ
顕在化しませんが、フェーズ2で管理者判定を実装した瞬間、今のうちに
is_admin=trueを自分でセットしておいたユーザーがそのまま管理者権限を得ます。

eventsテーブルの削除ガードと同様に、is_adminの変更だけは
本人のUPDATEでも弾く(例: BEFORE UPDATEトリガーでold.is_admin <> new.is_adminを拒否)
などの対応が必要に見えます。

Comment on lines +10 to +12
create policy "profiles_select_all" on public.profiles
for select to authenticated
using (true);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs/data-model.md「RLSポリシー方針」の表では profiles のSELECTは
「全ユーザー(公開カラムは限定)」となっていますが、この実装は
using (true) で行全体(= emailis_admin を含む全カラム)を
全認証ユーザーに公開しています。

RLSは行単位の制御しかできないので、カラムを絞るならビュー経由にする、
あるいは列単位のGRANT/REVOKEが必要になるはずです。ドキュメントの
「公開カラムは限定」は意図的に書かれている記述に見えるので、
今回スコープ外として全カラム公開のまま進める判断なのか、
実装漏れなのかを確認したいです(特にemailは他ユーザーに
知られたくない場合がありそうです)。

@claude

claude Bot commented Aug 6, 2026

Copy link
Copy Markdown

レビュー総評

docs/permissions.md の権限マトリクスと docs/data-model.md「RLSポリシー方針」に沿った形で
6テーブル分のRLSポリシーと削除ガードトリガーが実装されており、全体の構成・命名・
to authenticated に統一している方針・is_admin を一切参照していない点は
ドキュメント通りです。CLAUDE.mdの禁止事項(asキャスト、any、eslint-disable等)は
このPRがSQLのみのためそもそも該当しません。

一方で、インラインコメントで指摘した3点のうち特に1点目は設計上の抜け穴と考えています。

  1. [要修正] 削除ガードトリガーがRLSに巻き込まれてすり抜けるguard_event_deletion()
    security definer が付いていないため、内部の参加者チェックが呼び出し元(削除しようとしている
    オーナー)のRLS越しに実行されます。他の参加者が visibility = 'private' の場合、その行が
    見えず「参加者なし」と誤判定され、docs/data-model.md が要求する削除ガードが例外を出さずに
    すり抜けます
    。同一マイグレーションセット内の handle_new_user() が同じ理由で
    security definer を使っているのと対照的です。
  2. [要確認] profiles.is_admin を本人のUPDATEで自由に書き換えられるprofiles_update_self
    ポリシーが列を絞っていないため、任意のユーザーが自分の行の is_admintrue
    セットできてしまいます。現時点でこのフラグを参照する権限判定はどこにも無いので実害は
    顕在化しませんが、フェーズ2で管理者判定を実装した瞬間に効いてくる潜在的なバックドアです。
  3. [要確認] profiles のSELECTが全カラムを公開しているdocs/data-model.md の表は
    「全ユーザー(公開カラムは限定)」となっていますが、実装は using (true)
    email / is_admin を含む全カラムを全認証ユーザーに公開しています。意図的なスコープ外
    判断か確認したいです。

プロセス面: このPRは実装のみで test/db/ での検証は別Issue(#26)に切り出されています。
docs/permissions.md は「両方に同じマトリクスを実装し、両方をテストする」ことを
明示的に求めており、特に上記1点目のような「RLSポリシー同士の相互作用で静かに壊れる」
バグは、まさに否定側テスト(「参加者がいるイベントをオーナーが削除→失敗」を
private visibilityの参加者で書く)があれば実装時点で赤くなって検出できたはずのものです。
#26を別issueにすること自体は妥当だと思いますが、少なくとも削除ガードの否定側テストは
このPRのスコープに含めるか、#26を早急に着手することを推奨します。

インラインコメント3件はいずれも confirmed: true で投稿済みです。

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Issue #25 に対応し、docs/permissions.md / docs/data-model.md の権限マトリクスに基づいて、Supabase の6テーブルに対するRLS有効化とポリシー定義、およびイベント論理削除のガード用トリガーを追加するPRです。

Changes:

  • profiles / events / event_participants / ticket_entries / expenses / budgetsENABLE ROW LEVEL SECURITY と各CRUDポリシーを追加
  • events.deleted_at 更新による論理削除を、参加者条件で拒否する BEFORE UPDATE トリガー関数を追加

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +10 to +12
create policy "profiles_select_all" on public.profiles
for select to authenticated
using (true);
Comment thread supabase/migrations/20260806034559_rls_policies.sql Outdated
Comment on lines +41 to +45
create function public.guard_event_deletion()
returns trigger
language plpgsql
set search_path = public
as $$
Comment on lines +74 to +87
create policy "event_participants_insert_self_or_invite" on public.event_participants
for insert to authenticated
with check (
(user_id = auth.uid() and invited_by is null)
or (
invited_by = auth.uid()
and exists (
select 1
from public.event_participants ep
where ep.event_id = event_participants.event_id
and ep.user_id = auth.uid()
)
)
);
…参照、招待時のvisibility)

- guard_event_deletion()にsecurity definerを追加。RLS越しに実行されると
  private参加者が見えず削除ガードがすり抜けるため
- profiles.is_adminを本人のUPDATEで書き換えられないようにするトリガーを追加
  (service_roleは除外し、将来の管理者操作を妨げない)
- profilesのSELECTを本人のみに限定し、他ユーザーへの公開はid/display_nameのみの
  profiles_publicビュー経由にする(「公開カラムは限定」の実現)
- eventsのSELECTに、削除後もオーナー自身と支出を持つユーザーは参照できる条件を追加
  (docs/data-model.md「支出から辿ってイベント名は常に参照できる」との整合)
- event_participantsの招待経路INSERTで、招待者がvisibility/participation_stateを
  任意に指定できないよう private/joinedに固定
Copilot AI review requested due to automatic review settings August 6, 2026 03:54

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

supabase/migrations/20260806034559_rls_policies.sql:10

  • コメントで参照しているビュー名が public_profiles になっていますが、このマイグレーションで作成しているのは profiles_public です。後から読むと混乱するのでコメント側も実体に合わせてください。
-- SELECTの「全ユーザー(公開カラムは限定)」はRLS(行単位)だけでは表現できないため、
-- テーブル本体は本人のみに限定し、他ユーザーへの公開はpublic_profilesビュー(id/display_nameのみ)
-- 経由に限定する。

Comment on lines +209 to +211
create policy "budgets_delete_own" on public.budgets
for delete to authenticated
using (user_id = auth.uid());
Copilot AI review requested due to automatic review settings August 6, 2026 04:00
Comment on lines +26 to +45
-- is_adminは本人のUPDATEでも書き換えられないようにする。参照して分岐を作ってはいないが
-- (docs/permissions.mdの禁止範囲)、書き換え自体を放置すると、フェーズ2で管理者判定を
-- 実装した瞬間に「今のうちに自分でis_admin=trueにしておいたユーザー」がそのまま
-- 管理者権限を得てしまう。service_role(将来の管理者操作用)はauth.role()で除外する。
create function public.guard_is_admin_immutable()
returns trigger
language plpgsql
set search_path = public
as $$
begin
if new.is_admin is distinct from old.is_admin and auth.role() <> 'service_role' then
raise exception 'is_admin cannot be changed by the profile owner';
end if;
return new;
end;
$$;

create trigger guard_is_admin_immutable_trigger
before update on public.profiles
for each row execute function public.guard_is_admin_immutable();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

このguard_is_admin_immutablenew.is_admin is distinct from old.is_adminという条件でis_adminを直接参照しています。意図(本人による書き換え防止)は理にかなっていて、権限判定にis_adminを使っているわけではないので実質的にはdocs/permissions.mdの禁止事項(「これを参照する権限判定は一切実装しない」)には抵触しないと考えますが、ファイル冒頭5行目の「profiles.is_adminを参照する条件は一切書かない」というコメント自体とは文字通り矛盾しています。

  • このガード自体はdocs/data-model.md/docs/permissions.mdのどちらにも記載がなく、権限マトリクスにもRLS方針表にも無いロジックです。フェーズ2の「同じPRで両層+テストを変更する」ルールの対象外だとしても、少なくともdocs/data-model.mdis_adminの注記に一行追記しておかないと、次にこのカラムを触る人が「参照する条件は無いはず」という前提で読んでしまいそうです。
  • このトリガーの otherwise/service_role分岐(auth.role() <> 'service_role')も含めて、test/db/側のテストが本PRには無いので(#26で対応予定とのことですが)、少なくとも「本人がis_adminをtrueに書き換えようとして失敗する」ケースは他のRLS検証と合わせて#26でカバーされるよう明記しておくと安心です。

Comment on lines +61 to +75
-- 未削除の全イベントに加えて、削除後もオーナー自身と、その支出(expenses)を持つ
-- ユーザーは引き続き参照できるようにする(docs/data-model.md 5章「eventsは論理削除なので、
-- 支出から辿ってイベント名などは常に参照できる」)。
create policy "events_select_not_deleted_or_referenced" on public.events
for select to authenticated
using (
deleted_at is null
or owner_id = auth.uid()
or exists (
select 1
from public.expenses e
where e.event_id = events.id
and e.user_id = auth.uid()
)
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

このSELECTポリシーはdocs/data-model.md「RLSポリシー方針」表のevents行(「全ユーザー(deleted_at IS NULL)」)より広く、owner_id = auth.uid()と「自分のexpensesが参照しているイベント」の場合も削除後に見えるようにしています。根拠のコメント(5章の「支出から辿ってイベント名などは常に参照できる」)は妥当で、実装自体に異論はないのですが、この例外はdocs/data-model.mdのRLSポリシー方針表にもdocs/permissions.mdの権限マトリクスにも明文化されていません。

ドキュメント駆動の運用(CLAUDE.md)である以上、この表を「正」として読む次の実装者・レビュアーが「削除済みイベントは誰からも見えないはず」と誤解しないよう、docs/data-model.mdの方針表に一行反映しておくことを推奨します。

@claude

claude Bot commented Aug 6, 2026

Copy link
Copy Markdown

レビュー総評

docs/permissions.md の権限マトリクスと docs/data-model.md 「RLSポリシー方針」を基準に読みました。全体として実装の粒度は細かく、security definerが必要な理由やvisibility/participation_stateを招待経路で固定する理由など、判断の根拠がSQLコメントにきちんと書かれている点は良いと思います。テーブルごとの方針も概ねマトリクス通りです。

良い点

  • event_participantsのINSERTで、自己登録経路(invited_by is null)と招待経路(invited_by = auth.uid()かつ招待者自身が参加登録済み)を分けて、招待経路ではvisibility/participation_stateを固定している実装は、docs/data-model.md 3章の仕様を正確に反映しています。
  • eventsの削除ガードをRLSのWITH CHECKではなくBEFORE UPDATEトリガー(+security definer)にした判断とその理由(OLD/NEWの差分判定、非公開参加者行がRLS越しに見えない問題)は妥当です。
  • profiles本体は本人SELECTのみに制限し、他ユーザーへの公開列(id, display_name)はprofiles_publicビュー経由に限定したアプローチは、「全ユーザー(公開カラムは限定)」という方針をRLS(行単位)の制約の中で実現する現実的な設計だと思います。
  • 型のハンドコピーやas/any/eslint-disableによるごまかしは見当たりません(このPRはSQLマイグレーションのみで、supabase/types.tsの差分もyarn gen:types相当の生成物に見えます)。

気になった点(インラインコメント参照)

  1. guard_is_admin_immutableトリガーis_adminカラムへの書き換えを本人UPDATEでは拒否する設計自体は将来の権限昇格を防ぐ良い工夫ですが、ファイル冒頭の「is_adminを参照する条件は一切書かない」という宣言と文字通り矛盾しています。権限判定(RLSの許可/拒否をis_adminの値で分岐させる)ではなく「is_adminというカラム自体を保護する」ロジックなので docs/permissions.md の禁止事項の趣旨には反しないと考えますが、docs/data-model.mdのどこにも書かれていない新規ロジックなので、一行でも追記しておくことをおすすめします。
  2. eventsのSELECTポリシー — 削除済みイベントでもオーナー自身とそのイベントに紐づくexpensesを持つユーザーには見えるようにしている点は、docs/data-model.md 5章の記述を正しく汲んだ実装だと思いますが、docs/data-model.mdの「RLSポリシー方針」表自体には反映されていません。表を「正」として読む前提(docs/permissions.md冒頭)なので、表の更新を検討してください。
  3. 同様に、profiles_publicビューという実装アプローチもdocs/data-model.mdprofilesセクションには記載がありません。ドキュメント駆動の運用なので、実装が仕様を拡張・具体化した場合はドキュメント側にも反映しておくと、次に触る人(人間・エージェント問わず)が迷わずに済むと思います。

テストについて(ブロッキングではないが要確認)

このPRはtest/db/のRLS検証テストを含んでおらず、Issue #26に意図的に切り出されています。db-testジョブはsupabase start後にyarn test:dbを実行するだけなので、現状は「マイグレーションが構文エラーなく適用されるか」しか検証されず、docs/permissions.mdが最重要視している「弾かれることの確認」(否定側テスト)はまだゼロです。特に以下はdocs/permissions.md「最小の検証セット」に明記されている項目なので、#26で確実にカバーされるか確認をお願いします。

  • 参加者がいるイベントをオーナーが削除しようとして失敗すること(トリガー経由)
  • 招待経路のexistsチェック(参加登録していないユーザーが招待できないこと)
  • profiles_public経由ではid/display_name以外の列(email, is_admin)が漏れないこと
  • 本人によるis_adminの書き換えが失敗すること(今回追加されたトリガー)

docs/testing.mdの「新しいテストは対象を壊して赤くなることを確認してから戻す」も、RLSポリシー・トリガーそれぞれについて#26側で実施されることを期待します。

以上、致命的な問題は見当たりませんでしたが、ドキュメントとの整合性・テスト未了の点は認識合わせのためコメントしました。

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (3)

supabase/migrations/20260806034559_rls_policies.sql:10

  • コメントで参照しているビュー名が public_profiles になっていますが、このマイグレーションで作成しているのは profiles_public です。後から検索・運用時に混乱するのでコメント側も実名に合わせてください。
-- SELECTの「全ユーザー(公開カラムは限定)」はRLS(行単位)だけでは表現できないため、
-- テーブル本体は本人のみに限定し、他ユーザーへの公開はpublic_profilesビュー(id/display_nameのみ)
-- 経由に限定する。

supabase/types.ts:78

  • budgets_user_id_fkey は初期スキーマ上 public.budgets.user_id -> public.profiles.id の外部キーですが、この生成型では同じ FK 名で profiles_public への Relationship も追加されています。実DB上はビューにFKを張れないため、この Relationship は実際のリレーションを表しておらず、型上の join が実行時に失敗する/誤解を招く可能性があります。supabase/types.ts は手編集せず、マイグレーション適用後に yarn gen:types の生成結果をそのままコミットして整合させてください。
            foreignKeyName: "budgets_user_id_fkey"
            columns: ["user_id"]
            isOneToOne: false
            referencedRelation: "profiles_public"
            referencedColumns: ["id"]

supabase/types.ts:136

  • event_participants_invited_by_fkey でも profiles に加えて profiles_public への Relationship が同じ外部キー名で並んでいます。初期スキーマでは invited_bypublic.profiles(id) を参照しており、profiles_public はビューなのでFKの参照先にはなりません。上と同様に、生成型が実スキーマと矛盾していないか確認し、必要なら yarn gen:types の生成物に合わせてください。
            foreignKeyName: "event_participants_invited_by_fkey"
            columns: ["invited_by"]
            isOneToOne: false
            referencedRelation: "profiles_public"
            referencedColumns: ["id"]

- auto_expose_new_tables=false(既定)のため、RLSポリシーだけでは
  authenticatedロールがData API経由でテーブルに一切アクセスできない
  ことが判明。全6テーブルにGRANTを追加(レビュー指摘対応、重大な抜け)
- ファイル冒頭のコメントを整理: is_adminガードは「参照する権限判定」
  ではなく列の書き換え保護であることを明記
- docs/data-model.mdのRLSポリシー方針表・profiles章を実装内容
  (profiles_publicビュー、削除済みイベントの例外閲覧、招待時の
  visibility/participation_state固定、is_admin列保護)に合わせて更新
Copilot AI review requested due to automatic review settings August 6, 2026 04:07
Comment on lines +23 to +25
create policy "profiles_insert_self" on public.profiles
for insert to authenticated
with check (id = auth.uid());

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[要確認] is_admin の保護がUPDATEのみでINSERTには効いていない

guard_is_admin_immutable_triggerbefore update にしか張られていないため、profiles_insert_selfwith checkid = auth.uid() だけで is_admin の値を制限していません。理屈上はこのポリシー経由で is_admin = true の行をINSERTできてしまい、35行目付近のコメントで説明されている「自分で is_admin=true にしておく抜け道を塞ぐ」という目的をINSERT側では達成できていません。

現状は handle_new_user()(初期マイグレーション)がサインアップ時に id, email のみで行を自動作成するため、同じ idprofiles_insert_self 経由の追加INSERTはPK重複で失敗し、かつ profiles にはDELETEポリシーが無いため本人がその行を消して作り直すこともできず、実害はほぼ顕在化しません。ただしこれは「サインアップ時に必ず行が先に作られる」という別の実装(auth.usersトリガー)に暗黙に依存した安全性で、このポリシー単体では守れていません。将来profilesの作成経路が変わった場合の静かな抜け穴になり得るので、with checkand is_admin = false を加える(または guard_is_admin_immutable_triggerbefore insert or update にする)方が、このガードの意図と一致すると思います。

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

supabase/migrations/20260806034559_rls_policies.sql:16

  • コメント内で参照しているビュー名が public_profiles になっていますが、このマイグレーションで作成しているのは profiles_public です。識別子が一致していないと、後から検索・参照するときに混乱します。
-- テーブル本体は本人のみに限定し、他ユーザーへの公開はpublic_profilesビュー(id/display_nameのみ)
-- 経由に限定する。

supabase/migrations/20260806034559_rls_policies.sql:46

  • profiles_update_self により本人が profiles.email を更新できてしまいます。email は招待の検索キーとして使う想定 (docs/data-model.md) なので、本人が任意の値に変更できると招待の宛先の乗っ取り/サインアップ阻害(UNIQUE衝突)などに繋がります。is_admin と同様に、email も本人のUPDATEでは変更不可にするガードを追加してください。
begin
  if new.is_admin is distinct from old.is_admin and auth.role() <> 'service_role' then
    raise exception 'is_admin cannot be changed by the profile owner';
  end if;
  return new;

@claude

claude Bot commented Aug 6, 2026

Copy link
Copy Markdown

レビュー総評

docs/permissions.md の権限マトリクスと docs/data-model.md「RLSポリシー方針」を基準に、既存の2ラウンドのレビュー(claudeによる指摘×2)からの修正込みで現在のHEADを読みました。

CLAUDE.mdの禁止事項について
このPRはSQLマイグレーション + ドキュメント + 生成型(supabase/types.ts)のみで、app/ / common/ / mcp/ の変更は無いため、as キャスト・anyeslint-disable@ts-ignore・判断ロジックの二重実装のいずれも該当しません。supabase/types.ts の差分も、db-test / types-check の両CIジョブがgreenであることから(supabase.yml に明記されている「Dockerが無い環境ではCIのartifactから取得してコミットする」運用に沿って)実際の生成結果と一致していると判断できます。手書き改変の懸念は無しと判断しました。

過去の指摘が解消されていることを確認
既存のレビューコメント2件で指摘されていた以下は、現在のHEADで解消済みです。

  • guard_event_deletion() への security definer 付与(private参加者がRLS越しに見えず削除ガードがすり抜ける問題)→ 修正済み
  • profiles.is_admin を本人のUPDATEで書き換えられる問題 → guard_is_admin_immutable トリガーで保護済み
  • profiles のSELECTが全カラム公開になっていた問題 → 本人の行のみに限定し、profiles_public ビュー(id/display_nameのみ)経由に変更済み
  • 全テーブルへの GRANT 漏れ → 追加済み
  • docs/data-model.md「RLSポリシー方針」表が実装(※1 削除後もexpenses参照者は閲覧可能、※2 招待経路のvisibility固定、profiles_publicビュー)を反映していなかった点 → 追記済み

権限マトリクスとSQLを1行ずつ突き合わせましたが、event_participants の自己登録/招待の分岐、events の削除ガード(オーナー以外の参加者ゼロが前提でのvisibility非公開行を含めた判定)、ticket_entries/expenses/budgets の個人スコープなど、表通りの実装になっています。

新規の指摘(インラインコメント参照)
profiles_insert_self ポリシーがINSERT時に is_admin の値を制限しておらず、guard_is_admin_immutable トリガーがUPDATEのみを保護している非対称性があります。現状は handle_new_user() によるサインアップ時の自動作成とDELETEポリシー不在により実害はほぼ顕在化しませんが、ガードの意図(自分で is_admin=true にする抜け道を塞ぐ)をポリシー単体では満たせていません。詳細はインラインコメントを参照してください。

テストについて(既存指摘の再確認・重要度は変わらず)
test/db/ は依然として空(.gitkeep のみ)で、このPRのRLS実装に対する否定側テストはゼロです。db-test ジョブは「マイグレーションが構文エラーなく適用されるか」しか検証しておらず、docs/permissions.md が本体と位置付ける「弾かれることの確認」は未着手のままIssue #26に切り出されています。

docs/testing.md は権限判定を「静かに失敗する対象」の優先順位1位に置いており、docs/permissions.md も「表の×が1つでもテストされていなければ、それは検証されていない権限とみなす」と明記しています。今回インラインで指摘した is_admin のINSERT経路や、削除ガードのvisibility非公開参加者を含めた判定など、まさに否定側テストがあれば実装時点で機械的に検出できたはずの分岐です。#26を別Issueに切り出すこと自体は妥当な判断だと思いますが、この状態でマージする場合は「RLSは実装されたが、マトリクスとしてはまだ検証されていない」という前提を関係者間で明示しておくこと、また#26を後回しにせず早期に着手することを推奨します。

以上、致命的なブロッカーとまでは考えていませんが、インラインで指摘した1点の確認と、テスト未了の状態を認識した上でのマージ判断をお願いします。

guard_is_admin_immutable(UPDATE)との非対称性を解消(レビュー指摘対応)。
Copilot AI review requested due to automatic review settings August 6, 2026 04:18
Comment on lines +135 to +150
create policy "event_participants_insert_self_or_invite" on public.event_participants
for insert to authenticated
with check (
(user_id = auth.uid() and invited_by is null)
or (
invited_by = auth.uid()
and visibility = 'private'
and participation_state = 'joined'
and exists (
select 1
from public.event_participants ep
where ep.event_id = event_participants.event_id
and ep.user_id = auth.uid()
)
)
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

event_participants_insert_self_or_invite の招待経路は「招待者自身が event_participants に行を持つこと」を必須にしています。一方 docs/data-model.md 2章は owner_id を「参加者ではない」役割として明示的に定義しており(「登録だけして自分は参加しない、というケースも表現できる」)、docs/permissions.md の権限マトリクスは「他ユーザーの招待」で「オーナー(登録者)」列を無条件で としています。

このポリシーのままだと、自分自身は参加登録していないオーナーが誰かを招待しようとするとRLSに弾かれ、マトリクスの「オーナー ○」と食い違うケースが生まれます(オーナー = 参加登録済みの前提が常に成り立つなら問題ありませんが、data-model.md はその前提を明示的に否定しています)。

  • owner_id = auth.uid() を許可条件に追加してマトリクス通りにするか
  • あるいは「オーナーも招待するには自分で参加登録している必要がある」という制約を docs/permissions.md / docs/data-model.md 側に明記するか

どちらかで揃えたほうがいいと思います。


-- 1. profiles (docs/data-model.md「RLSポリシー方針」)
-- SELECTの「全ユーザー(公開カラムは限定)」はRLS(行単位)だけでは表現できないため、
-- テーブル本体は本人のみに限定し、他ユーザーへの公開はpublic_profilesビュー(id/display_nameのみ)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: コメント中の「public_profilesビュー」は実際に作成しているビュー名 profiles_public(58行目)と語順が逆です。後で grep で追えなくなるので直しておくと良さそうです(59行目コメントの「所有者(postgres)としてprofilesを参照するため」の段落は正しい名前になっています)。

@claude

claude Bot commented Aug 6, 2026

Copy link
Copy Markdown

レビュー総評

docs/permissions.md の権限マトリクスと docs/data-model.md「RLSポリシー方針」に沿ってRLSを実装したPRとして、全体の作り込みは丁寧です。良かった点:

  • 6テーブル全てで enable row level security とマトリクス通りのポリシーが揃っている
  • profiles の列公開制限を profiles_public ビューで実現し、is_admin は「参照して分岐しない」(権限判定)と「本人による書き換えを防ぐ」(列保護)を明確に区別してコメントで説明している。両者を混同していない点はdocs/permissions.mdの「MVPでは管理者の列を作らない」という制約を正しく守れている
  • イベント削除ガードで security definer が必要な理由(RLS越しだとprivateな参加者が見えず判定をすり抜ける)をコメントで明記しており、event_participants の招待insert側は逆に本人の行しか見ないので security definer 不要、という判断も一貫している
  • auto_expose_new_tables=false によるGRANT漏れ、profiles_insert_selfis_admin 非対称性など、レビューの過程で拾った指摘を全て同一PR内で修正し、docs/data-model.md 側もその都度実装に合わせて更新している(ドキュメントとコードの乖離を残していない)
  • as キャスト/any/eslint-disable@ts-ignore による黙殺は無し。supabase/types.ts は手編集ではなく yarn gen:types による再生成である旨がコミットメッセージに明記されている

インラインで2件コメントしました。

  1. 要確認: event_participants_insert_self_or_invite の招待経路が「招待者自身が参加登録済みであること」を必須にしていますが、docs/data-model.md はオーナーを「参加者ではない」役割として明示しており、docs/permissions.md のマトリクスは「他ユーザーの招待」で「オーナー」列を無条件で としています。参加登録していないオーナーが招待しようとするとRLSに弾かれ、マトリクスの記述と食い違う可能性があります。SQL側に owner_id = auth.uid() を足すか、ドキュメント側に「オーナーも招待するには自分で参加登録が必要」と明記するか、どちらかで揃えたほうがよさそうです。
  2. nit: コメント中の「public_profilesビュー」表記が実際のビュー名 profiles_public と語順が逆(タイポ)。

懸念点(ブロッカーではない)

PR説明にある通り、test/db/ でのRLS検証(否定側を含む)はIssue #26に切り出されています。docs/permissions.md は権限判定を「静かに失敗する」最優先テスト対象と位置づけ、docs/roadmap.md フェーズ1の完了条件も「マトリクスの×が全てDBレベルで弾かれることをテストで確認」としているため、このPR単体では"実装した"ことしか保証されておらず、"マトリクス通りに弾かれる"ことはまだ機械的に検証されていません。チェックリスト上は元々別項目なので分割自体は妥当だと思いますが、#26がマージされるまではこのポリシー群は未検証という前提で扱うのがよいと思います(特に上のコメント1のようなマトリクスとの食い違いは、負のテストがあれば実装時点で機械的に検出できたはずの種類の問題です)。

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

supabase/migrations/20260806034559_rls_policies.sql:73

  • PR説明の箇条書きでは「events は deleted_at IS NULL の行のみ全員SELECT可」となっていますが、ここでは削除済みでも「オーナー本人」または「そのイベントを参照する expenses を持つユーザー」はSELECTできるポリシーになっています。PR説明側もこの実際の挙動に合わせて更新した方が、レビュー/後追い時の齟齬を避けられます。
-- 未削除の全イベントに加えて、削除後もオーナー自身と、その支出(expenses)を持つ
-- ユーザーは引き続き参照できるようにする(docs/data-model.md 5章「eventsは論理削除なので、
-- 支出から辿ってイベント名などは常に参照できる」)。
create policy "events_select_not_deleted_or_referenced" on public.events
  for select to authenticated

supabase/migrations/20260806034559_rls_policies.sql:16

  • コメント内で参照しているビュー名が public_profiles になっていますが、このマイグレーションで作っているビューは profiles_public です。将来検索/運用時に混乱するので名前を揃えてください。
-- SELECTの「全ユーザー(公開カラムは限定)」はRLS(行単位)だけでは表現できないため、
-- テーブル本体は本人のみに限定し、他ユーザーへの公開はpublic_profilesビュー(id/display_nameのみ)
-- 経由に限定する。

@reitojike
reitojike merged commit 7097fc6 into main Aug 6, 2026
8 checks passed
@reitojike
reitojike deleted the 25-phase1-rls-policies branch August 6, 2026 04:24
reitojike added a commit that referenced this pull request Aug 8, 2026
PR #68 のClaude Reviewの指摘に対応。指摘は正しかった。

誤り: 「300行を超えた5本」は事実と異なり、素の差分では9本だった
(#31 307 / #59 365 / #60 380 / #30 441 / #29 617 / #18 679 / #41 698 /
#32 1063 / #16 3528)。同じ本文が書いていた「300行以下が65%(17/26)」は
超過9本を含意しており、記述が自己矛盾していた。

原因: 母集団の統計(中央値・65%)は素の差分で数え、外れ値の説明だけに
除外規則を適用していた。数え方を混在させたうえ、超過リストを上位5本で
打ち切って全件確認しなかった。

訂正: 26本すべてを git show --numstat で数え直し、除外の段階ごとに
表で示す。素の差分(中央値135行 / 65% / 超過9本)、パス名で機械的に
判定できる除外まで(中央値128行 / 85% / 超過4本)、除外規則を最後まで
適用(超過2本 = #59#60)。落ちる7本の内訳も明記した。

あわせて、3段階目の中央値を出さない理由を書いた。supabase/config.toml は
#29 では supabase init の出力(416行)、#56 では根拠コメント付きで手で
直した6行で、同じパスでも扱いが逆になる。パス名では決まらないことが、
この節が機械的ゲートになり得ない理由そのものなので、「lintではない」の
段落の根拠もこの実測に差し替えた。

Refs #44
reitojike added a commit that referenced this pull request Aug 8, 2026
* docs: Issueの粒度とPR差分サイズの目安をCLAUDE.mdに明文化

判断ポイントは1 Issueに3個まで(5個超で分割)、PR差分は300行を目安とする。
記事の実測値をそのまま採らず、main にマージ済みのPR 26本(中央値135行、
300行以下65%)で裏を取ってから採用した。行数の数え方から生成物・ロック
ファイル・権限マトリクスを写したテスト表を除外する根拠も、超過した
PR #16 / #32 の実態から示した。

機械的ゲートにしない旨と、3回ルール(モデルを上げる) / PO確認(判断を
下せる層に上げる) / 粒度超過(Issueを分ける)の対処の違いを表で整理。
docs/roadmap.md はポインタ1行に留め、根拠は1箇所にだけ置く。

Refs #44

* docs: 判断ポイント数の境界を一本化し、裏取り済みの数値と外部実測を書き分ける

PR #68 のClaude Reviewの指摘2件に対応。

指摘1: 「3個まで、5個を超えるなら分割」で4個の扱いが未定義だった。
閾値を「3個まで。4個目が出てきたら分ける」に一本化する。機械的ゲートに
しない方針である以上、「検討」と「必ず」の二段構えは実効性のない
false precisionになるため、緩衝域を作らず単一の線にした。

指摘2: 裏取り済みの300行と、外部実測のままの3個が同じ文脈に並んでいた。
「2つの数字は裏付けの強さが違う」として段落を分け、300行はこのリポジトリの
実測(PR 26本、中央値135行/300行以下65%)で検証済み、3個は外部実測のみを
根拠とする未検証のヒューリスティックであると明示した。過去Issueの判断数は
記録がなく後から数え直せないため、このリポジトリでの裏取りが今はできない
理由も併記。採用の根拠はコストの非対称性に置いた。

あわせて、外部実測に対応値のない4個/5個を推定して線を引いていないことと、
実績が溜まったら見直す旨を記載した。

Refs #44

* docs: PR実測の集計を数え直し、超過本数の誤りを訂正する

PR #68 のClaude Reviewの指摘に対応。指摘は正しかった。

誤り: 「300行を超えた5本」は事実と異なり、素の差分では9本だった
(#31 307 / #59 365 / #60 380 / #30 441 / #29 617 / #18 679 / #41 698 /
#32 1063 / #16 3528)。同じ本文が書いていた「300行以下が65%(17/26)」は
超過9本を含意しており、記述が自己矛盾していた。

原因: 母集団の統計(中央値・65%)は素の差分で数え、外れ値の説明だけに
除外規則を適用していた。数え方を混在させたうえ、超過リストを上位5本で
打ち切って全件確認しなかった。

訂正: 26本すべてを git show --numstat で数え直し、除外の段階ごとに
表で示す。素の差分(中央値135行 / 65% / 超過9本)、パス名で機械的に
判定できる除外まで(中央値128行 / 85% / 超過4本)、除外規則を最後まで
適用(超過2本 = #59#60)。落ちる7本の内訳も明記した。

あわせて、3段階目の中央値を出さない理由を書いた。supabase/config.toml は
#29 では supabase init の出力(416行)、#56 では根拠コメント付きで手で
直した6行で、同じパスでも扱いが逆になる。パス名では決まらないことが、
この節が機械的ゲートになり得ない理由そのものなので、「lintではない」の
段落の根拠もこの実測に差し替えた。

Refs #44

* docs: PR #29/#32の除外理由が2段階なのに1段階しか書いていなかった記述漏れを修正

Claude Reviewの指摘どおり、#29(617→22)はsupabase/config.toml(416行)
だけでなくsupabase/types.ts(179行)も、#32(1063→79)はテスト表(924行)
だけでなくyarn.lock(60行)も除外して初めて数字が再現できる。
片方しか書いていなかったため、追試すると数値が合わなかった。

Refs #44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants