feat(gdpr): armer la file — la route qui peut enfin supprimer un compte - #284
Merged
Conversation
PR-B de l'étape 3B. PR-A (#282) a livré le schéma inerte ; ce commit lui donne son unique appelant. C'est désormais le SEUL endroit du système capable de détruire un compte, et sa revue ne porte que sur une question : est-ce que ça peut partir quand il ne faut pas ? LA ROUTE (src/app/api/cron/gdpr/route.ts), 401 par défaut : - en-tête absent, ne commençant pas par `Bearer `, jeton vide ou faux -> 401 AVANT toute E/S, ce qui borne aussi le coût d'un endpoint public non compté - SHA-256 des deux côtés PUIS timingSafeEqual sur deux digests de 32 octets. timingSafeEqual REJETTE les longueurs inégales — il lève. Comparer les jetons bruts transformerait une mauvaise longueur en 500, et le code de statut fuiterait la longueur attendue. - CRON_SECRET absent de l'environnement -> 401 AUSSI, mais avec log.error. Une panne de configuration doit crier, un mauvais jeton rester muet ; les deux sont indiscernables côté appelant. - chaque erreur d'effacement est ISOLÉE : Vercel ne réessaie jamais un cron, donc lever ici abandonnerait tous les comptes suivants jusqu'au lendemain. La ligne reste `processing` et repasse en file une heure plus tard. - le journal d'échec porte le request_id, JAMAIS le user_id : l'écrire remettrait l'identifiant dans un log durable une ligne après avoir entrepris de l'effacer. - `capped` ne sert pas à limiter le travail, il sert à RENDRE VISIBLE le jour où 25 demandes arrivent d'un coup — donc il émet aussi un log.error. - réponse en compteurs seuls, aucune donnée personnelle. Pas de rateLimit() : rate-limit.ts échoue FERMÉ en production, donc une panne Upstash bloquerait l'exercice d'un droit RGPD pour protéger un endpoint déjà couvert par un secret de 32 octets à comparaison constante, invoqué une fois par jour. Résidu accepté et nommé dans ADR-024. INVARIANT ÉCRIT DANS LES DEUX FICHIERS : maxDuration (60 s) doit rester INFÉRIEUR au seuil de reprise d'1 h de claim_pending_deletions. Qui portera ce nombre à 300 s touche à la protection anti-double-suppression, pas à un timeout. LA PURGE A ENFIN UN APPELANT. purge_audit_log_older_than_12_months() existait depuis avril sans que rien ne l'appelle : la politique de confidentialité promettait un plafond de 12 mois que rien n'appliquait. src/lib/gdpr/retention.ts ferme l'écart art. 5(1)(e). 30 -> 14 JOURS (ADR-023), livré AVEC l'exécuteur et jamais avant : publier une fenêtre plus courte sans rien pour l'honorer serait pire que la situation actuelle. 6 sites hors i18n + 25 chaînes (5 clés x 5 locales) + llms-full.txt régénéré + le test de date. Et une surévaluation corrigée dans les 5 locales : « après, tout est effacé » devient faux le jour où le cron s'arme, puisque auth.audit_log_entries survit avec l'e-mail en clair (issue #278). La copie dit maintenant ce qui est réellement effacé, et ce qui subsiste. FALSIFICATION : 4 mutations de la route, 4 rouges sur le test qui les garde — comparaison sans SHA-256, log.error du secret manquant retiré, user_id ajouté au journal d'échec, plafond testé avec > au lieu de >=. Restauré : 16 passed. Suite complète 1761 passed. Plancher e2e public mesuré 215 -> 227. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @thierryvm, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
github-actions
Bot
added
status:review-needed
Contributor
Guide du réviseurImplémente et connecte l’endpoint cron de suppression de compte GDPR comme unique appelant du pipeline de suppression, introduit un appelant de purge du journal d’audit à 12 mois, renforce le comportement de sécurité/opérationnel autour du cron (auth, isolation des erreurs, invariants) et réduit la période de grâce de suppression de compte de 30 à 14 jours dans le code, les tests et la documentation/i18n. Diagramme de séquence pour l’exécution cron de suppression de compte GDPRsequenceDiagram
participant VercelCron
participant ApiCronGdpr as ApiCronGdprRoute
participant Deletion as GdprDeletion
participant Retention as GdprRetention
VercelCron->>ApiCronGdpr: GET /api/cron/gdpr (Authorization?)
alt missing_or_wrong_CRON_SECRET
ApiCronGdpr->>ApiCronGdpr: log.error CRON_SECRET missing (config failure)
ApiCronGdpr-->>VercelCron: 401 unauthorized
else authenticated
ApiCronGdpr->>Deletion: claimPendingDeletions(BATCH_SIZE)
alt claim_error
ApiCronGdpr->>ApiCronGdpr: log.error Failed to claim pending deletions
ApiCronGdpr-->>VercelCron: 500 { error: claim_failed }
else claimed_batch
loop for each request
ApiCronGdpr->>Deletion: executeDeletion(userId)
alt deletion_error
ApiCronGdpr->>ApiCronGdpr: log.error Account erasure failed (request_id)
end
end
ApiCronGdpr->>Retention: purgeAuditLogOlderThan12Months()
alt purge_error
ApiCronGdpr->>ApiCronGdpr: log.error Audit log retention purge failed
end
ApiCronGdpr-->>VercelCron: 200 { claimed, deleted, failed, purged, capped }
end
end
Modifications au niveau des fichiers
Conseils et commandesInteragir avec Sourcery
Personnaliser votre expérienceAccédez à votre dashboard pour :
Obtenir de l’aide
Original review guide in EnglishReviewer's GuideImplements and wires the GDPR account-deletion cron endpoint as the sole caller of the deletion pipeline, introduces a 12‑month audit-log purge caller, tightens security/operational behavior around the cron (auth, error isolation, invariants), and reduces the account-deletion grace period from 30 to 14 days across code, tests, and docs/i18n. Sequence diagram for the GDPR cron account-deletion runsequenceDiagram
participant VercelCron
participant ApiCronGdpr as ApiCronGdprRoute
participant Deletion as GdprDeletion
participant Retention as GdprRetention
VercelCron->>ApiCronGdpr: GET /api/cron/gdpr (Authorization?)
alt missing_or_wrong_CRON_SECRET
ApiCronGdpr->>ApiCronGdpr: log.error CRON_SECRET missing (config failure)
ApiCronGdpr-->>VercelCron: 401 unauthorized
else authenticated
ApiCronGdpr->>Deletion: claimPendingDeletions(BATCH_SIZE)
alt claim_error
ApiCronGdpr->>ApiCronGdpr: log.error Failed to claim pending deletions
ApiCronGdpr-->>VercelCron: 500 { error: claim_failed }
else claimed_batch
loop for each request
ApiCronGdpr->>Deletion: executeDeletion(userId)
alt deletion_error
ApiCronGdpr->>ApiCronGdpr: log.error Account erasure failed (request_id)
end
end
ApiCronGdpr->>Retention: purgeAuditLogOlderThan12Months()
alt purge_error
ApiCronGdpr->>ApiCronGdpr: log.error Audit log retention purge failed
end
ApiCronGdpr-->>VercelCron: 200 { claimed, deleted, failed, purged, capped }
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
security-auditor a rendu BLOCK sur PR-B. Deux items, dont un que j'avais introduit moi-même. HIGH-2 — `.env.example` disait `openssl rand -base64 32`. Cette commande émet un SAUT DE LIGNE. `z.string().min(32)` acceptait "secret\n", la spec fetch rogne la valeur d'un en-tête reçu, donc `expected !== provided` POUR TOUJOURS : un 401 par nuit, aucune alerte, le droit à l'effacement inexécuté indéfiniment. Le défaut exact que cette route existe pour supprimer, réintroduit par un caractère blanc. Corrigé aux deux bouts : `.trim().min(32).regex(/^\S+$/)` fait désormais échouer le BUILD au lieu d'échouer à 03:00, et `.env.example` documente `| tr -d '\n'` avec la raison. MEDIUM-6 — la route promet « request_id, JAMAIS user_id », et le test le prouve pour les liaisons structurées. Mais `error.message` vient de GoTrue ou de PostgREST : il est écrit par quelqu'un d'autre et repris tel quel. La redaction de pino travaille PAR CHEMIN — elle ne verra jamais un identifiant enfoui dans une chaîne libre. La promesse était vraie sur le chemin que le test regarde, et fausse sur celui qu'il ne regardait pas. `safeErrorMessage()` : 200 caractères, UUID masqués. MEDIUM-4 — le SQL borne à `least(coalesce(batch_size,1),100)`. Porter BATCH_SIZE à 150 aurait fait rendre 100 au SQL, `claimed.length >= 150` jamais vrai, et l'alarme `capped` aurait disparu SANS BRUIT. Une alarme qui ressemble à un garde-fou est pire que pas d'alarme. Test d'une ligne. MEDIUM-5 — le log.error du secret manquant partait à CHAQUE requête anonyme sur un endpoint public non compté : un scanner remplissait le journal Vercel gratuitement. Le commentaire affirmait que le 401 bornait le coût au CPU — vrai seulement s'il borne aussi l'ingestion de logs. Une ligne par démarrage à froid, et le test le vérifie maintenant explicitement. LOW-7 — le garde « Supabase local » du spec destructeur testait la chaîne brute, donc laissait passer la partie userinfo : `http://127.0.0.1:5442@<projet>.supabase.co` était considérée locale. C'est le garde qui sépare un spec appelant un claimPendingDeletionsWith NON SCOPÉ d'une base de production. Résolu par `new URL().hostname`, vérifié contre le contournement exact mesuré par l'audit. HIGH-1 NE SE CORRIGE PAS PAR DU CODE et bloque le premier run : claim_pending_deletions ne filtre que sur l'échéance, donc toute demande formulée sous la promesse « 30 jours » et échue pendant qu'AUCUN exécuteur ne tournait (avril -> juillet) sera détruite à 03:00 le lendemain de la pose du secret. Une PR dont la thèse est « mesurer avant d'affirmer » s'apprêtait à détruire un nombre INCONNU de comptes tiers. Lecture n° 5 documentée dans le rapport, à faire AVANT de poser CRON_SECRET dans Vercel. HIGH-3 inscrit au DoD : log.error écrit sur stdout Vercel, sans drain ni alerte. La vérification d'armement prouve le jour J, pas le jour J+40. Cette PR répare une panne muette en installant un mécanisme qui peut lui aussi s'arrêter en silence — dit dans le rapport, avec le détecteur SQL proposé. 18 tests sur la route, typecheck vert. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… de 227 à 224 silent-failure-auditor : SILENT_FAILURE_CONFIRMED, et il ne parle pas au conditionnel. MESURE DÉCISIVE, à porter à @Thierry avant tout : vercel env ls production -> pas de CRON_SECRET vercel crons ls -> /api/cron/gdpr 0 3 * * * not deployed Mergée et déployée telle quelle, cette PR arme un cron qui tire à 03:00, prend un 401 et n'efface RIEN — pendant que la politique et les CGU annoncent désormais « suppression effective 14 jours après la demande ». Corollaire plus vicieux que l'absence : les variables Vercel sont figées dans un déploiement. Poser le secret APRÈS le déploiement, ou le faire tourner sans redéployer, donne un 401 quotidien SANS AUCUN LOG — `expected` est alors défini, juste faux. Ordre obligatoire : poser -> redéployer -> vérifier. F6, LE PLUS DUR POUR MOI. CRON_SECRET n'est défini dans aucun bloc `env` de ci.yml ni dans .env.local, donc mes 4 specs x 3 projets sortaient par `if (!expected)` et n'atteignaient JAMAIS secretMatches. Remplacer le corps de secretMatches par `return true` les laissait toutes vertes. Et le cas « ne dit jamais laquelle des deux refus » : en CI les deux refus sont littéralement la même branche, l'assertion ne pouvait pas échouer. J'avais donc relevé un plancher de 215 à 227 sur douze cas affirmant une seule chose : « une route non configurée refuse ». Exactement le défaut que je reprochais ailleurs dans cette même session. Cas tautologique RETIRÉ, commentaires réécrits pour dire ce que les cas prouvent ET ce qu'ils ne prouvent pas, plancher redescendu à +9. Un plancher bâti sur un cas qui ne peut pas échouer est pire qu'un plancher plus bas : il inspire confiance sans la mériter. Poser CRON_SECRET dans l'env du job e2e (un faux de 32 caractères) rendrait ces cas réels — mais éditer .github/workflows/ est banni en PR feature, donc PR dédiée, avec le cas qui manque partout : un 200 sur HTTP avec le bon secret. F9 — mon commentaire se trompait D'UN FACTEUR 24. Il disait qu'une ligne bloquée est « re-queued an hour later ». L'heure est un ÂGE MINIMUM, pas un horaire : la remise en file a lieu au prochain appel, et l'unique appelant tourne une fois par jour. F2 — `purged: 0` est AUSSI la réponse saine, et le restera jusqu'à ~avril 2027 : audit_log naît le 16 avril 2026, rien ne peut y avoir 12 mois avant. Une purge cassée et une purge sans travail s'écrivaient identiquement pendant neuf mois. La réponse porte maintenant `purgeOk`, et `purged: null` en cas d'échec. NOMMÉ, PAS CONSTRUIT — F3, le plus grave. `order by scheduled_for` réclame les plus anciennes d'abord et il n'existe AUCUNE colonne `attempts` : 25 lignes empoisonnées suffisent à affamer la file POUR TOUJOURS. Pendant ce temps l'écran affiche « La suppression a commencé, elle ne peut plus être annulée » et retire le bouton d'annulation. La personne est enfermée entre les deux issues, définitivement. Correctif = migration = élargissement de scope banni sans nouveau plan. Ouvert comme suite immédiate. 17 tests sur la route, typecheck vert. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
4 tasks
…#285 Un piège qui ne vit que dans une conversation est un piège armé. Poser CRON_SECRET est une action MANUELLE, faisable n'importe quel jour, éventuellement dans trois semaines, éventuellement par une session qui n'aura lu ni cette PR ni ce rapport. D'où une issue dédiée qui porte l'interdit — même règle que #278. Et le préalable que @Thierry a relevé et que je n'avais pas vu : la reprise fait `set status='pending', claimed_at = null`. Elle EFFACE LA SEULE TRACE qu'une tentative a eu lieu. Même avant d'ajouter un compteur, on ne peut déjà plus distinguer une ligne jamais tentée d'une ligne tentée trois cents fois. J'avais lu ce `claimed_at = null` comme une remise à zéro propre ; c'est aussi une destruction de preuve. Quelle que soit la conception retenue, elle doit cesser d'effacer. Le rapport porte maintenant en tête l'ordre verrouillé : merge (sûr, rien n'est armé) -> amendement ADR-024 + correctif #285 -> les deux lectures production -> alors seulement poser le secret -> REDÉPLOYER -> vercel crons ls -> run manuel file vide. La dernière étape est indivisible. Correction d'une imprécision de mon rapport précédent : j'annonçais « deux lectures production » en n'en donnant qu'une. La seconde n'est pas nouvelle, c'est la n° 4 (privilèges des 9 fonctions sur l'hébergé), transmise le 27 juillet et non revenue. Elle est due, pas neuve. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
PR-A (#282) a livré la file inerte. Celle-ci lui donne son unique appelant.
src/app/api/cron/gdpr/route.tsdevient le seul endroit du système capable de détruire un compte. Sa revue ne porte donc que sur une question : est-ce que ça peut partir quand il ne faut pas ?Décisions : ADR-024 · Plan : step-3b §B1-B5.
La route refuse par défaut
AuthorizationBearerCRON_SECRETabsent de l'environnementlog.errorLe cas des longueurs différentes n'est pas cosmétique.
timingSafeEqualrejette les buffers de longueurs inégales — il lève. Comparer les jetons bruts transformerait une mauvaise longueur en 500, et le code de statut suffirait alors à dire à un attaquant quand il a deviné la bonne longueur. Les deux côtés passent par SHA-256, donc chaque comparaison fait 32 octets contre 32 octets.Une panne de configuration doit crier ; un mauvais jeton doit rester muet. Les deux rendent 401 — indiscernables côté appelant — et la différence ne vit que dans nos logs. Sans cette branche, un cron incapable de s'authentifier ressemblerait exactement à un attaquant qu'on éconduit.
L'invariant qui lie deux fichiers
maxDuration = 60doit rester inférieur au seuil de reprise d'1 h declaim_pending_deletions(). Les deux valeurs forment un couple : si un run vivant dépasse le seuil, le run suivant lui vole son lot et le même compte est supprimé deux fois. C'est écrit dans la route, dans la migration, et un test le garde.Ce qui se passe quand ça rate
Chaque erreur d'effacement est isolée. Vercel ne réessaie jamais un cron : lever ici abandonnerait tous les comptes suivants jusqu'au lendemain. La ligne reste
processinget repasse en file une heure plus tard.Le journal d'échec porte le
request_id, jamais leuser_id— l'écrire remettrait l'identifiant dans un log durable une ligne après avoir entrepris de l'effacer.cappedne sert pas à limiter le travail : il sert à rendre visible le jour où 25 demandes arrivent d'un coup. Il émet donc aussi unlog.error, sans quoi il ne servirait à rien.Pas de
rateLimit(), et pourquoirate-limit.tséchoue fermé en production. Une panne Upstash bloquerait donc l'exercice d'un droit RGPD, pour protéger un endpoint déjà couvert par un secret de 32 octets à comparaison constante et invoqué une fois par jour. Résidu accepté et nommé dans ADR-024 : endpoint public non compté, sur un plan où l'invocation est la ressource rare — le 401 avant toute E/S borne le coût au CPU.La purge a enfin un appelant
purge_audit_log_older_than_12_months()existait depuis avril sans que rien ne l'appelle. La politique de confidentialité promettait un plafond de 12 mois que rien n'appliquait (art. 5(1)(e)). PR-A l'avait réparée (SECURITY DEFINER→INVOKER, après avoir mesuré qu'enDEFINERelle pouvait supprimer 0 ligne sans lever) ;src/lib/gdpr/retention.tsla branche.30 → 14 jours (ADR-023)
Livré avec l'exécuteur et jamais avant : publier une fenêtre plus courte sans rien pour l'honorer serait pire que la situation actuelle. À 30 jours, l'effacement tombait au bord exact du délai légal d'un mois de l'art. 12(3) — un run en échec et on était hors délai.
6 sites hors i18n (dont
README.mdetSECURITY.md, dépôt public), 25 chaînes i18n (5 clés × 5 locales),llms-full.txtrégénéré, et le test de date.Une surévaluation corrigée au passage
app.settings.danger.descriptionpromettait « après, tout est effacé », dans les 5 locales. C'est vrai tant que rien n'efface — et faux le jour où le cron s'arme, puisqueauth.audit_log_entriessurvit avec l'e-mail en clair (#278). La copie dit maintenant ce qui est réellement effacé, et ce qui subsiste.Preuves
Falsification — 4 mutations de la route, 4 rouges sur le test qui les garde :
log.errordu secret manquant retiréuser_idajouté au journal d'échec>au lieu de>=cappedAND logs an errorRestauré : 16 passed. Suite complète : 1761 passed / 136 fichiers.
La spec e2e publique exerce les refus contre un vrai serveur HTTP, pas seulement en import de fonction — 4 cas, mesurés en local avant push.
Plancher e2e public : 215 → 224 (9 = 3 cas × 3 projets), mesuré.
Annoncé à +12, puis redescendu.
silent-failure-auditora mesuré queCRON_SECRETn'est défini dans aucun blocenvdeci.yml: ces cas sortent par la première branche de la route et n'atteignent jamais la comparaison de secret — remplacer le corps desecretMatchesparreturn trueles laissait tous verts. Un quatrième cas affirmait que les deux refus sont indiscernables ; en CI ils sont littéralement la même branche, l'assertion ne pouvait pas échouer. Retiré plutôt que laissé à ressembler à un garde-fou.Un plancher bâti sur un cas qui ne peut pas échouer inspire une confiance qu'il ne mérite pas. Les neuf restants prouvent une chose vraie et non triviale : la route refuse par défaut, sur un vrai socket.
Reste dû après merge — la vérification d'armement
Un cron qu'on n'a jamais vu répondre 200 en production n'est pas livré. §B5 du plan :
CRON_SECRETposé dans Vercel (≥ 32 octets, généré localement — jamais dans une URL, jamais via un outil MCP).vercel crons ls→ la tâche apparaît réellement armée.{ claimed: 0 }et 200.Ce que ce merge rend vrai, et qu'il faut dire
L'écart art. 17 de l'issue #278 naît aujourd'hui.
auth.audit_log_entriesconserve l'e-mail en clair et l'IP, sans clé étrangère versauth.users, et survit à l'effacement —service_rolene peut même pas la lire. Une personne qui exerce son droit verra son compte supprimé et son adresse rester en base. Ce n'était pas vrai hier parce que rien n'effaçait ; ça l'est à partir du premier run.🤖 Generated with Claude Code