diff --git a/.claude/settings.local.json b/.claude/settings.local.json index 2fb1317..3e3141a 100644 --- a/.claude/settings.local.json +++ b/.claude/settings.local.json @@ -95,7 +95,13 @@ "Bash(curl -s -o /dev/null -X POST -H 'Content-Type: application/json' -d '{\"email\":\"cooldown@test.no\"}' http://127.0.0.1:8099/auth/request-link)", "Bash(curl -s -w '\\\\n[HTTP %{http_code}]\\\\n' -X POST -H 'Content-Type: application/json' -d '{\"email\":\"ikke-en-epost\"}' http://127.0.0.1:8099/auth/request-link)", "Bash(curl -s -w '\\\\n[HTTP %{http_code}]\\\\n' -H 'X-Debug-User-Id: 43b4ed80-5bc1-49bf-b6eb-4190f68554a2' http://127.0.0.1:8099/orgs/11111111-1111-1111-1111-111111111111/tournaments)", - "Bash(rm -f __TRACKED_VAR__/002_scratch.sql __TRACKED_VAR__/002_wrapper.sql __TRACKED_VAR__/cookies.txt __TRACKED_VAR__/cookies2.txt)" + "Bash(rm -f __TRACKED_VAR__/002_scratch.sql __TRACKED_VAR__/002_wrapper.sql __TRACKED_VAR__/cookies.txt __TRACKED_VAR__/cookies2.txt)", + "Bash(python3 -m py_compile /opt/teecup/app/routers/auth.py)", + "Bash(curl -s -w '\\\\n[HTTP %{http_code}]\\\\n' -b /tmp/claude-1000/-opt-teecup/a8bd2fc3-4b9c-4682-a2be-cf36e143de78/scratchpad/cookies_rls.txt -X POST -H 'Content-Type: application/json' -d '{\"name\":\"Bug-repro-cup\"}' http://127.0.0.1:8099/orgs/11111111-1111-1111-1111-111111111111/tournaments)", + "Bash(curl -s -w '\\\\n[HTTP %{http_code}]\\\\n' -b /tmp/claude-1000/-opt-teecup/a8bd2fc3-4b9c-4682-a2be-cf36e143de78/scratchpad/cookies_rls.txt http://127.0.0.1:8099/auth/me)", + "Bash(curl -s -o /dev/null -X POST -H 'Content-Type: application/json' -b /tmp/claude-1000/-opt-teecup/a8bd2fc3-4b9c-4682-a2be-cf36e143de78/scratchpad/cookies_rls.txt -d '{\"name\":\"Bug-repro-cup-2\"}' http://127.0.0.1:8099/orgs/11111111-1111-1111-1111-111111111111/tournaments)", + "Bash(curl -s -w '\\\\n[HTTP %{http_code}]\\\\n' -b /tmp/claude-1000/-opt-teecup/a8bd2fc3-4b9c-4682-a2be-cf36e143de78/scratchpad/cookies_rls.txt http://127.0.0.1:8099/orgs/11111111-1111-1111-1111-111111111111/tournaments)", + "Bash(rm -f __TRACKED_VAR__/002_scratch.sql __TRACKED_VAR__/002_wrapper.sql __TRACKED_VAR__/cookies_rls.txt)" ] } } diff --git a/005_rls_null_guard.sql b/005_rls_null_guard.sql new file mode 100644 index 0000000..49d4954 --- /dev/null +++ b/005_rls_null_guard.sql @@ -0,0 +1,60 @@ +-- ===================================================================== +-- TeeCup — migrasjon 005 +-- RLS-fiks: tomstreng i app.current_org (se FEATURE_BACKLOG.md) +-- ===================================================================== +-- Kjøres etter 001-004. +-- +-- BUG: current_setting('app.current_org', true)::uuid håndterer NULL trygt +-- (gir ingen rader, som skjemaets kommentar i 001 lover: "trygg standard"), +-- men IKKE tomstreng. En gjenbrukt asyncpg-pool-tilkobling der en TIDLIGERE +-- forespørsel satte GUC-en via SET LOCAL (org_connection(), app/db.py) kan +-- lese den tilbake som '' etter at den transaksjonen er ferdig — en custom +-- GUC sin nullstilte tilstand er tomstreng når placeholderen først er +-- opprettet i sesjonen, ikke fullstendig fraværende. ''::uuid kaster en +-- feil (500) i stedet for trygt null rader. Oppdaget da /auth/me (egen +-- runde) spurte `organization` via plain_connection() (ingen org-kontekst). +-- +-- FIKS: NULLIF(current_setting(...), '')::uuid — konverterer tomstreng til +-- NULL FØR cast. Samlet i én STABLE SQL-funksjon fremfor duplisert i 15 +-- policyer. +-- ===================================================================== + +\set ON_ERROR_STOP on + +-- Feil raskt fremfor å blokkere hele appen hvis en ALTER POLICY skulle køe +-- bak en lang spørring — hver ALTER POLICY under tar ACCESS EXCLUSIVE på +-- måltabellen til COMMIT. +SET lock_timeout = '5s'; + +CREATE OR REPLACE FUNCTION app_current_org() RETURNS uuid +LANGUAGE sql STABLE AS $$ + SELECT NULLIF(current_setting('app.current_org', true), '')::uuid +$$; + +-- De 14 org_isolation-tabellene: 12 fra 001 + 2 fra 003 (samme array som +-- CREATE-loopene der — hold denne i sync hvis en fremtidig migrasjon +-- legger til en ny org-scopet tabell). +DO $$ +DECLARE t text; +BEGIN + FOREACH t IN ARRAY ARRAY[ + 'player','course','hole','tee','tee_rating', + 'tournament','team','team_roster','session', + 'match','match_participant','hole_score', + 'match_hole_result','lineup_lock' + ] + LOOP + EXECUTE format($p$ + ALTER POLICY org_isolation ON %I + USING (organization_id = app_current_org()) + WITH CHECK (organization_id = app_current_org()); + $p$, t); + END LOOP; +END $$; + +-- org_self ble opprinnelig laget UTEN eksplisitt WITH CHECK (polwithcheck er +-- NULL i pg_policy, ikke en snapshot av USING) — Postgres sin +-- speiler-USING-når-WITH-CHECK-mangler-fallback er dynamisk, ikke en +-- engangskopi tatt ved CREATE. Å re-issue kun USING her er derfor korrekt. +ALTER POLICY org_self ON organization + USING (id = app_current_org()); diff --git a/CLAUDE.md b/CLAUDE.md index 5df47b4..2f8e56b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -102,23 +102,30 @@ Ferdig og verifisert: **Fant og fikset underveis:** `ON CONFLICT (email)` matchet ikke den nye PARTIELLE unike indeksen uten eksplisitt `WHERE email IS NOT NULL` (samme klasse feil som `hole_score`s partielle indekser i scoring-runden). - **Fant, IKKE fikset her (egen sak, se FEATURE_BACKLOG):** `organization`- - tabellens RLS-policy (`org_self`, og trolig ALLE `org_isolation`-policyer i - 001/003) kaster en 500 i stedet for skjemaets lovede "trygg standard: se - ingenting" når `current_setting('app.current_org', true)` returnerer - TOMSTRENG (ikke NULL) — noe som kan skje på en gjenbrukt asyncpg-pool- - tilkobling der en tidligere forespørsel satte GUC-en via `SET LOCAL`. Kun - et problem for kode som spør org-scopede tabeller via `plain_connection()` - (ingen org-kontekst) — `/auth/me` unngår det bevisst ved å ikke joine mot - `organization`. Fiksen (`NULLIF(current_setting(...), '')::uuid` i alle 15 - policyer) er reell, billig, og lav risiko, men berører selve - isolasjonsgrunnmuren (ADR-003) og fortjener en egen, fokusert - rettingsrunde med skikkelig testing — ikke en hastefiks boltet på noe annet. + **Fant, IKKE fikset i denne runden (egen runde rett etterpå — se under):** + `organization`-tabellens RLS-policy kastet en 500 i stedet for skjemaets + lovede "trygg standard: se ingenting" ved tomstreng-GUC. +- **RLS-tomstreng-bug FIKSET (2026-07-16):** ny migrasjon + `005_rls_null_guard.sql` — delt `STABLE` SQL-funksjon `app_current_org()` + gjør `NULLIF(current_setting('app.current_org', true), '')::uuid` i stedet + for det rå uttrykket, brukt av alle 15 RLS-policyer (`ALTER POLICY`, + 14 `org_isolation` + `org_self`). Verifisert med 3 nye regresjonstester i + `test_isolation.sql` (Test 10-12) OG ved faktisk å gjenskape original- + buggen mot en ekte container (pool-størrelse 1, varm opp med + `org_connection()`, deretter `/auth/me` på samme gjenbrukte tilkobling — + gikk fra 500 til 200). + **Viktig presisering fra denne runden:** fiksen gjør IKKE at `/auth/me` kan + joine `organization` direkte via `plain_connection()` — det var en feilaktig + antakelse i forrige runde. `org_self` krever fortsatt en MATCHENDE + `app.current_org` for å vise en rad (riktig RLS-design, ikke noe fiksen + skulle endre), og en bruker kan tilhøre flere organisasjoner samtidig, så + det finnes ingen ÉN kontekst å sette for en tverr-org-spørring. `/auth/me` + slår derfor opp hvert org-navn ett om gangen via `org_connection()` (N+1, + N = antall org-er brukeren tilhører) — dette er riktig løsning, ikke en + omvei. Neste steg: -1. **Fiks RLS-tomstreng-buggen** beskrevet over (egen liten runde, migrasjon - 005) — reell, men isolert og lavrisiko. -2. Containerisere TeeCup-API-et (Dockerfile + compose-tjeneste), koble mot +1. Containerisere TeeCup-API-et (Dockerfile + compose-tjeneste), koble mot `teecup_db` med `teecup_app`, rute via eksisterende Caddy til `teecup.teeoff.no`. (Under scratch-verifisering måtte hele `/opt/teecup` monteres, ikke bare `app/`, fordi `handicap_engine.py` er et @@ -126,5 +133,5 @@ Neste steg: med samme relative plassering.) Ekte SMTP-utsending av magic-link må også kobles inn før dette går live (i dag: dev-only logging bak `TEECUP_DEV_LOG_MAGIC_LINKS`). -3. Deretter frontend (PWA, offline-first) og kommunikasjon (migrasjon 006, +2. Deretter frontend (PWA, offline-first) og kommunikasjon (migrasjon 006, siden 004/005 nå er tatt av auth og RLS-fiksen). diff --git a/FEATURE_BACKLOG.md b/FEATURE_BACKLOG.md index c2ba07b..55cf617 100644 --- a/FEATURE_BACKLOG.md +++ b/FEATURE_BACKLOG.md @@ -28,34 +28,38 @@ | Banedata fra teeoff via API | 🔀 | ADR-004. Endret fra Geminis «delt database direkte». | | Konfigurerbar handicap-pipeline (4 brytere) | ✅ | ADR-014. Bygget i `app/handicap.py`, brukt av scoring-runden. | | Ekte autentisering (magic-link + JWT-sesjon) | ✅ | ADR-009. `app/routers/auth.py` + migrasjon `004_auth.sql`. `X-Debug-User-Id`-stubben er helt fjernet. Ekte SMTP-utsending gjenstår (i dag: dev-only logging). | +| RLS-tomstreng-fiks (`app_current_org()`) | ✅ | Migrasjon `005_rls_null_guard.sql`. Se detaljer under. | --- -### RLS-tomstreng-bug (funnet 2026-07-16, IKKE fikset ennå) -- **Status:** ❓ trenger egen rettingsrunde (lavrisiko, men berører ADR-003s - isolasjonsgrunnmur — fortjener fokusert testing, ikke en hastefiks). -- Alle RLS-policyer i 001/003 (`org_isolation` på 13 tabeller + `org_self` på - `organization`) bruker `current_setting('app.current_org', true)::uuid`. - Denne håndterer NULL trygt (gir ingen rader, som tiltenkt), men IKKE - tomstreng — og en gjenbrukt asyncpg-pool-tilkobling der en TIDLIGERE - forespørsel satte GUC-en via `SET LOCAL` kan lese den tilbake som `''` - (tomstreng) i stedet for NULL etter at den transaksjonen er ferdig. Da - kaster casten en 500 (`invalid input syntax for type uuid: ""`) i stedet - for skjemaets lovede "trygg standard: se ingenting". -- **Oppdaget av:** `/auth/me` (ny denne runden) prøvde å joine mot - `organization`-tabellen via `plain_connection()` (ingen org-kontekst) for å - hente organisasjonsnavn til en multi-org-liste — det er FØRSTE gang noe - spør en org-scopet, RLS-beskyttet tabell via en tilkobling uten - org-kontekst. Mitigert MIDLERTIDIG i `/auth/me` ved rett og slett å ikke - joine mot `organization` (returnerer kun `organization_id` + `role`, ikke - navn) — unngår buggen, løser den ikke. -- **Fiks:** `NULLIF(current_setting('app.current_org', true), '')::uuid` i - stedet for `current_setting(...)::uuid`, i alle 15 policyer (`ALTER - POLICY`, egen migrasjon 005). NULLIF konverterer tomstreng til NULL FØR - cast, så den trygge "se ingenting"-oppførselen gjenopprettes uansett hvilken - tilstand GUC-en er i. -- **Følgeoppgave når fikset:** `/auth/me` kan da trygt joine mot - `organization` igjen og returnere organisasjonsnavn, ikke bare ID+rolle. +### RLS-tomstreng-bug — ✅ FIKSET 2026-07-16 +- Alle RLS-policyer i 001/003 (`org_isolation` på 14 tabeller + `org_self` på + `organization`) brukte `current_setting('app.current_org', true)::uuid`. + Denne håndterte NULL trygt (ga ingen rader, som tiltenkt), men IKKE + tomstreng — en gjenbrukt asyncpg-pool-tilkobling der en TIDLIGERE + forespørsel satte GUC-en via `SET LOCAL` kunne lese den tilbake som `''` + etter at transaksjonen var ferdig, og casten kastet da en 500 i stedet for + skjemaets lovede "trygg standard: se ingenting". +- **Fiks:** samlet i én `STABLE` SQL-funksjon `app_current_org()` (migrasjon + `005_rls_null_guard.sql`) som gjør `NULLIF(current_setting(...), '')::uuid` + — konverterer tomstreng til NULL FØR cast. Alle 15 policyer alteret + (`ALTER POLICY`) til å bruke funksjonen i stedet for det rå uttrykket. + Verifisert med 3 nye regresjonstester i `test_isolation.sql` (Test 10-12: + tomstreng-lesing gir 0 rader ikke krasj, tomstreng-skriving avvises av RLS + ikke krasj, org-bootstrap-innsetting fungerer rett etter tomstreng- + tilstand) OG ved å faktisk gjenskape original-buggen mot en ekte container + (pool-størrelse 1, varm opp med `org_connection()`, deretter kall + `/auth/me` på samme gjenbrukte tilkobling — gikk fra 500 til 200). +- **Viktig presisering oppdaget underveis:** den opprinnelige planen antok at + `/auth/me` kunne joine `organization` direkte igjen når tomstreng-buggen + var fikset. Det var FEIL — fiksen gjør bare at tomstreng oppfører seg som + NULL (trygt: se ingenting), den endrer IKKE at `org_self`-policyen krever + en MATCHENDE `app.current_org` for å vise en rad i det hele tatt (riktig + RLS-oppførsel, ikke en bug). En bruker kan tilhøre flere organisasjoner + samtidig, så det finnes ingen ÉN kontekst å sette for en tverr-org- + spørring. `/auth/me` slår derfor opp hvert org-navn ETT OM GANGEN via + `org_connection()` (N+1 spørringer, N = antall org-er brukeren tilhører, + typisk 1-3) — verifisert at dette faktisk returnerer navnet korrekt. --- diff --git a/app/routers/auth.py b/app/routers/auth.py index 31f567e..6f08dfe 100644 --- a/app/routers/auth.py +++ b/app/routers/auth.py @@ -26,7 +26,7 @@ from pydantic import BaseModel, EmailStr from ..auth import CurrentUser, SESSION_COOKIE_NAME, create_session_token, get_current_user, should_use_secure_cookies from ..config import settings -from ..db import plain_connection +from ..db import org_connection, plain_connection router = APIRouter(prefix="/auth", tags=["auth"]) @@ -156,6 +156,7 @@ async def logout(response: Response) -> dict: class MyOrg(BaseModel): organization_id: str + name: str role: str @@ -168,33 +169,37 @@ class Me(BaseModel): @router.get("/me", response_model=Me) async def me(user: CurrentUser = Depends(get_current_user)) -> Me: - # MERK: henter bevisst IKKE organisasjonens navn her. Det ville krevd å - # spørre `organization`-tabellen (som HAR RLS, org_self-policyen) via - # plain_connection() -- altså UTEN org-kontekst satt. Under scratch- - # testing avdekket dette en reell, dypere bug: org_self sin - # `current_setting('app.current_org', true)::uuid` håndterer NULL trygt, - # men IKKE tomstreng (som en custom GUC kan lese tilbake som på en - # gjenbrukt pool-tilkobling der en annen forespørsel tidligere satte den - # via SET LOCAL) -- det gir en 500 i stedet for skjemaets lovede "trygg - # standard: se ingenting". Dette er en tverrgående RLS-sak (berører alle - # 15 policyer i skjemaet, ikke noe auth-spesifikt) som fortjener sin egen - # fokuserte rettingsrunde, ikke en hastefiks her. Se FEATURE_BACKLOG.md. + # MERK: `organization` (org_self-policyen) kan IKKE joines direkte her via + # plain_connection() -- migrasjon 005 fikset kun tomstreng-krasjen, den + # endret ikke at org_self krever en MATCHENDE app.current_org for å vise + # en rad i det hele tatt (riktig RLS-oppførsel, ikke en bug). En bruker + # kan tilhøre flere organisasjoner samtidig, så det finnes ingen ÉN + # kontekst å sette for en tverr-org-spørring som denne. Løsningen er + # derfor å slå opp hvert org-navn ETT OM GANGEN gjennom org_connection() + # (som setter riktig kontekst for akkurat den ene raden) -- N+1 spørringer, + # men N er antall organisasjoner brukeren tilhører (typisk 1-3), og dette + # er den eneste måten å gjøre det på uten å endre selve RLS-modellen. async with plain_connection() as conn: user_row = await conn.fetchrow( "SELECT id::text AS id, email::text AS email, display_name FROM app_user WHERE id = $1", user.user_id, ) - org_rows = await conn.fetch( - """ - SELECT organization_id::text AS organization_id, role - FROM organization_membership - WHERE user_id = $1 - """, + membership_rows = await conn.fetch( + "SELECT organization_id::text AS organization_id, role FROM organization_membership WHERE user_id = $1", user.user_id, ) + + organizations = [] + for m in membership_rows: + async with org_connection(m["organization_id"]) as org_conn: + name = await org_conn.fetchval( + "SELECT name FROM organization WHERE id = $1", m["organization_id"] + ) + organizations.append(MyOrg(organization_id=m["organization_id"], name=name, role=m["role"])) + return Me( id=user_row["id"], email=user_row["email"], display_name=user_row["display_name"], - organizations=[MyOrg(**dict(r)) for r in org_rows], + organizations=organizations, ) diff --git a/test_isolation.sql b/test_isolation.sql index 3cb6999..dc4f958 100644 --- a/test_isolation.sql +++ b/test_isolation.sql @@ -5,9 +5,10 @@ -- sett fra runtime-rollen teecup_app (som IKKE er superuser/BYPASSRLS). -- -- Kjør som admin/superbruker mot en scratch- eller test-database der --- 001_initial_schema.sql, 002_roles_and_grants.sql og --- 003_scoring_and_blinddraw.sql allerede er kjørt (003 kreves for --- match_hole_result/lineup_lock, testet her i tillegg til grunnskjemaet): +-- 001_initial_schema.sql, 002_roles_and_grants.sql, +-- 003_scoring_and_blinddraw.sql og 005_rls_null_guard.sql allerede er kjørt +-- (003 kreves for match_hole_result/lineup_lock, 005 for +-- app_current_org()-funksjonen testene 10-12 bruker indirekte): -- psql -d teecup_scratch -f test_isolation.sql -- -- Alt kjøres i én transaksjon som RULLES TILBAKE til slutt — ingen testdata @@ -202,13 +203,61 @@ BEGIN RAISE NOTICE 'OK Test 9b: lineup_lock — kontekstbytte gir riktig organisasjons data (%).', tid; END $$; +-- --- Regresjonstester for RLS-tomstreng-buggen (migrasjon 005) ---------- +-- current_setting('app.current_org', true)::uuid håndterte NULL trygt, men +-- IKKE tomstreng — en gjenbrukt pool-tilkobling kunne lese GUC-en tilbake +-- som '' og få en kastet feil i stedet for trygt "se ingenting". Simulerer +-- tilstanden direkte (set_config med is_local=false, ikke SET LOCAL) siden +-- den er lettere å fremtvinge deterministisk enn selve pool-gjenbruken som +-- utløste den i praksis. + +-- Test 10: tomstreng-GUC — SELECT skal gi 0 rader, IKKE en kastet feil. +SELECT set_config('app.current_org', '', false); +DO $$ +DECLARE n int; +BEGIN + SELECT count(*) INTO n FROM tournament; + IF n <> 0 THEN + RAISE EXCEPTION 'ISOLASJON FEILET (tomstreng-lesing): ser % rader, forventet 0', n; + END IF; + RAISE NOTICE 'OK Test 10: tomstreng-GUC — SELECT gir trygt 0 rader, ikke feil (%).', n; +END $$; + +-- Test 11: tomstreng-GUC — INSERT skal avvises av RLS (insufficient_privilege), +-- IKKE feile med "invalid input syntax for type uuid". +DO $$ +BEGIN + BEGIN + INSERT INTO tournament (organization_id, name) + VALUES ('aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa', 'tomstreng-forsøk'); + RAISE EXCEPTION 'ISOLASJON FEILET (tomstreng-skriving): fikk sette inn data uten gyldig org-kontekst'; + EXCEPTION + WHEN insufficient_privilege THEN + RAISE NOTICE 'OK Test 11: tomstreng-GUC — WITH CHECK blokkerte innsetting trygt (ikke krasj).'; + END; +END $$; + +-- Test 12: bootstrap-mønster (sett current_org til en FERSK uuid, deretter +-- INSERT organization med SAMME id) skal fungere korrekt selv RETT ETTER at +-- tilstanden nettopp var tomstreng — beviser at en eksplisitt satt verdi +-- alltid overstyrer uansett tidligere GUC-tilstand. Dobler som første test +-- av selve org-bootstrap-innsettingsstien (002s kommentar, tidligere utestet). +SELECT set_config('app.current_org', 'eeeeeeee-eeee-eeee-eeee-eeeeeeeeeeee', false); +DO $$ +BEGIN + INSERT INTO organization (id, name) + VALUES ('eeeeeeee-eeee-eeee-eeee-eeeeeeeeeeee', 'Bootstrap-test-org'); + RAISE NOTICE 'OK Test 12: org-bootstrap — INSERT lykkes rett etter tomstreng-tilstand.'; +END $$; + RESET ROLE; RESET app.current_org; ROLLBACK; -- ingen testdata blir liggende igjen \echo '============================================' -\echo ' Alle 9 isolasjonstester bestått (se OK-linjer)' -\echo ' Dekker: tournament/organization (001) samt' -\echo ' match_hole_result/lineup_lock (003, ADR-012/013)' +\echo ' Alle 12 isolasjonstester bestått (se OK-linjer)' +\echo ' Dekker: tournament/organization (001),' +\echo ' match_hole_result/lineup_lock (003, ADR-012/013),' +\echo ' og RLS-tomstreng-fiksen (005)' \echo '============================================'