Security audit follow-up: rate-limit git-http, cap notifications
Adversarial review of everything added this session (anonymous HTTPS clone, federated notifications, pins) — full report in docs/security/AUDIT-2026-07-29.md. Three real gaps fixed, each with a regression test where practical: - The anonymous git-clone endpoints had no rate limiting at all, unlike every other unauthenticated endpoint (login, notify). ratelimit.go generalized from loginLimiter to a reusable rateLimiter; new gitHTTPByIP at 60 req/min/IP. - Notifications had no per-recipient cap, unlike pins — any instance could bloat a real user's storage indefinitely with fake claims (nothing about receiving one requires a real relationship). MaxNotificationsPerPrincipal = 200, oldest evicted to make room. - A notification's recipient username was never checked to actually exist locally before being accepted and stored. Also fixed: git's stderr was silently going to /dev/null on the HTTP clone endpoints instead of being logged (internal/gitexec's SSH path already did this correctly). Strengthened the /notifications disclaimer to state explicitly that content isn't verified, only the sender's signature is.
14 files changed
+410 −29
M
CHANGELOG.md
+10 −0
M
README.fr.md
+1 −0
M
README.md
+1 −0
M
cmd/gitfed-web/handlers_auth.go
+2 −2
M
cmd/gitfed-web/handlers_git_http.go
+17 −2
M
cmd/gitfed-web/main.go
+11 −4
M
cmd/gitfed-web/ratelimit.go
+17 −16
A
docs/security/AUDIT-2026-07-29.md
+163 −0
M
internal/federation/notify.go
+10 −3
A
internal/federation/notify_test.go
+61 −0
M
internal/i18n/strings_en.go
+1 −1
M
internal/i18n/strings_fr.go
+1 −1
M
internal/store/notifications.go
+20 −0
A
internal/store/notifications_test.go
+95 −0
CHANGELOG.md
@@ -1,5 +1,15 @@
# Changelog
+## 0.9.2
+
+Follow-up security audit on everything added since the pre-prod one (`docs/security/AUDIT-2026-07-29.md`) — the anonymous HTTPS clone, federated notifications, pinned repos. No critical findings; three real gaps fixed:
+
+- The anonymous git-clone endpoints had no rate limiting at all (unlike login/notify) — now capped at 60 requests/minute/IP.
+- Notifications had no per-recipient cap — any instance could bloat a real user's storage with fake claims; now capped at 200, oldest evicted first.
+- A notification's recipient username was never checked to actually exist locally before being stored.
+
+Also: git's stderr was being silently discarded on the HTTP clone endpoints instead of logged, and the "this is informational only" disclaimer on `/notifications` now explicitly says the *content* isn't verified, only the sender's signature.
+
## 0.9.1
- Settings and repo-settings visually brought up to date with the rest of the app: icons on the settings tabs, a real profile card, collaborators shown as icon+role rows instead of a bare table (with a shield for the owner, a globe for federated collaborators), and a visually distinct danger zone.
README.fr.md
@@ -64,6 +64,7 @@ instance en fonctionnement.
| [`docs/HOW_IT_WORKS.md`](docs/HOW_IT_WORKS.md) | Le modèle de fédération/identité de bout en bout : certificats, magasin de confiance, ACL, un vrai push détaillé étape par étape. |
| [`docs/ARCHITECTURE.md`](docs/ARCHITECTURE.md) | Comment le code est organisé : les quatre binaires, les paquets `internal/`, pourquoi il existe un socket RPC d'administration, la topologie Kubernetes. |
| [`docs/security/AUDIT.md`](docs/security/AUDIT.md) | L'audit de sécurité pré-production et ce qui en a été corrigé. |
+| [`docs/security/AUDIT-2026-07-29.md`](docs/security/AUDIT-2026-07-29.md) | Audit de suivi couvrant le clone HTTPS anonyme, les notifications fédérées et les dépôts épinglés ajoutés depuis. |
| [`DESIGN.md`](DESIGN.md) | Le document de conception d'origine — le « pourquoi » derrière l'architecture, écrit avant le début de l'implémentation. |
| [`CHANGELOG.md`](CHANGELOG.md) | Historique des versions (également servi sur `/changelog` dans l'interface web). |
| [`THIRD_PARTY_LICENSES.md`](THIRD_PARTY_LICENSES.md) | Chaque dépendance Go et sa licence. |
README.md
@@ -60,6 +60,7 @@ instance.
| [`docs/HOW_IT_WORKS.en.md`](docs/HOW_IT_WORKS.en.md) | The federation/identity model end to end: certificates, trust store, ACLs, a real push walked through step by step. |
| [`docs/ARCHITECTURE.en.md`](docs/ARCHITECTURE.en.md) | How the code is organized: the four binaries, the `internal/` packages, why there's an admin RPC socket, the Kubernetes topology. |
| [`docs/security/AUDIT.md`](docs/security/AUDIT.md) *(French)* | The pre-production security audit and what was fixed as a result. |
+| [`docs/security/AUDIT-2026-07-29.md`](docs/security/AUDIT-2026-07-29.md) *(French)* | Follow-up audit covering the anonymous HTTPS clone, federated notifications and pinned repos added afterward. |
| [`DESIGN.md`](DESIGN.md) *(French)* | The original design rationale — the "why" behind the architecture, written before implementation started. |
| [`CHANGELOG.md`](CHANGELOG.md) | Version history (also served at `/changelog` in the web UI). |
| [`THIRD_PARTY_LICENSES.md`](THIRD_PARTY_LICENSES.md) | Every Go dependency and its license. |
cmd/gitfed-web/handlers_auth.go
@@ -61,8 +61,8 @@ func (s *server) handleLogin(w http.ResponseWriter, r *http.Request) {
return
}
if !ok {
- s.loginByIP.recordFailure(ip)
- s.loginByUser.recordFailure(username)
+ s.loginByIP.record(ip)
+ s.loginByUser.record(username)
redirectWithMsg(w, r, loginURL, i18n.T(lang, "auth.invalid_login"), true)
return
}
cmd/gitfed-web/handlers_git_http.go
@@ -1,6 +1,7 @@
package main
import (
+ "bytes"
"compress/gzip"
"fmt"
"io"
@@ -42,6 +43,11 @@ func (s *server) handleGitInfoRefs(w http.ResponseWriter, r *http.Request) {
http.NotFound(w, r)
return
}
+ if !s.gitHTTPByIP.allowed(clientIP(r)) {
+ http.Error(w, "rate limited", http.StatusTooManyRequests)
+ return
+ }
+ s.gitHTTPByIP.record(clientIP(r))
if r.URL.Query().Get("service") != "git-upload-pack" {
// No dumb-HTTP fallback and no receive-pack advertisement — smart
// upload-pack only.
@@ -61,10 +67,12 @@ func (s *server) handleGitInfoRefs(w http.ResponseWriter, r *http.Request) {
writePktLine(w, "# service=git-upload-pack\n")
writeFlushPkt(w)
+ var stderr bytes.Buffer
cmd := exec.Command("git", "upload-pack", "--stateless-rpc", "--advertise-refs", repo.Path)
cmd.Stdout = w
+ cmd.Stderr = &stderr
if err := cmd.Run(); err != nil {
- log.Printf("gitfed-web: git-http info/refs %s: %v", repoName, err)
+ log.Printf("gitfed-web: git-http info/refs %s: %v: %s", repoName, err, stderr.String())
}
}
@@ -74,6 +82,11 @@ func (s *server) handleGitUploadPack(w http.ResponseWriter, r *http.Request) {
http.NotFound(w, r)
return
}
+ if !s.gitHTTPByIP.allowed(clientIP(r)) {
+ http.Error(w, "rate limited", http.StatusTooManyRequests)
+ return
+ }
+ s.gitHTTPByIP.record(clientIP(r))
repo, err := s.ops.GetRepo(repoName)
if err != nil || !repo.Public {
@@ -96,11 +109,13 @@ func (s *server) handleGitUploadPack(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/x-git-upload-pack-result")
w.Header().Set("Cache-Control", "no-cache")
+ var stderr bytes.Buffer
cmd := exec.Command("git", "upload-pack", "--stateless-rpc", repo.Path)
cmd.Stdin = body
cmd.Stdout = w
+ cmd.Stderr = &stderr
if err := cmd.Run(); err != nil {
- log.Printf("gitfed-web: git-http upload-pack %s: %v", repoName, err)
+ log.Printf("gitfed-web: git-http upload-pack %s: %v: %s", repoName, err, stderr.String())
}
}
cmd/gitfed-web/main.go
@@ -27,8 +27,9 @@ type server struct {
ops admin.Ops
domain string
- loginByIP *loginLimiter
- loginByUser *loginLimiter
+ loginByIP *rateLimiter
+ loginByUser *rateLimiter
+ gitHTTPByIP *rateLimiter
}
func main() {
@@ -56,8 +57,14 @@ func main() {
// Per-account is the tighter bound (an attacker targeting one login);
// per-IP is looser but catches spraying across many usernames from
// one source. Either tripping blocks the attempt.
- loginByUser: newLoginLimiter(5, 15*time.Minute),
- loginByIP: newLoginLimiter(20, 15*time.Minute),
+ loginByUser: newRateLimiter(5, 15*time.Minute),
+ loginByIP: newRateLimiter(20, 15*time.Minute),
+ // The anonymous git-clone endpoints (handlers_git_http.go) have no
+ // login to fail, so this counts every request rather than just
+ // failures — generous enough for a real clone (which is a handful
+ // of requests: one info/refs, one or more upload-pack), tight
+ // enough to blunt someone hammering it for disk/CPU/bandwidth.
+ gitHTTPByIP: newRateLimiter(60, time.Minute),
}
mux := http.NewServeMux()
cmd/gitfed-web/ratelimit.go
@@ -8,14 +8,15 @@ import (
"time"
)
-// loginLimiter is a small in-memory fixed-window rate limiter used to blunt
-// brute-force / credential-stuffing against the web login. It counts failed
-// attempts per key (client IP or username) and refuses new attempts once
-// max failures accumulate within window. It is intentionally process-local:
-// the web UI runs as a single replica (see deploy/k8s/deployment.yaml), so
-// there's no shared-state requirement, and a restart clearing the counters
-// is an acceptable, fail-open-on-restart trade-off.
-type loginLimiter struct {
+// rateLimiter is a small in-memory fixed-window rate limiter, used both to
+// blunt brute-force/credential-stuffing against the web login (counting
+// failed attempts per key) and to bound how often the anonymous git-HTTP
+// clone endpoints can be hit per source IP (counting every request). It is
+// intentionally process-local: the web UI runs as a single replica (see
+// deploy/k8s/deployment.yaml), so there's no shared-state requirement, and
+// a restart clearing the counters is an acceptable, fail-open-on-restart
+// trade-off.
+type rateLimiter struct {
mu sync.Mutex
hits map[string]*hitWindow
max int
@@ -27,16 +28,16 @@ type hitWindow struct {
reset time.Time
}
-func newLoginLimiter(max int, window time.Duration) *loginLimiter {
- l := &loginLimiter{hits: make(map[string]*hitWindow), max: max, window: window}
+func newRateLimiter(max int, window time.Duration) *rateLimiter {
+ l := &rateLimiter{hits: make(map[string]*hitWindow), max: max, window: window}
go l.gcLoop()
return l
}
// allowed reports whether an attempt for key may proceed right now. It does
-// not record anything — call recordFailure only on an actual auth failure,
-// so a legitimate user who logs in correctly is never counted.
-func (l *loginLimiter) allowed(key string) bool {
+// not record anything — call record separately, so a caller that only wants
+// to count failures (e.g. login) can skip it on success.
+func (l *rateLimiter) allowed(key string) bool {
l.mu.Lock()
defer l.mu.Unlock()
w := l.hits[key]
@@ -46,7 +47,7 @@ func (l *loginLimiter) allowed(key string) bool {
return w.count < l.max
}
-func (l *loginLimiter) recordFailure(key string) {
+func (l *rateLimiter) record(key string) {
l.mu.Lock()
defer l.mu.Unlock()
now := time.Now()
@@ -59,13 +60,13 @@ func (l *loginLimiter) recordFailure(key string) {
// reset clears the counter for key, e.g. after a successful login so the
// window doesn't linger against a user who has proven who they are.
-func (l *loginLimiter) reset(key string) {
+func (l *rateLimiter) reset(key string) {
l.mu.Lock()
defer l.mu.Unlock()
delete(l.hits, key)
}
-func (l *loginLimiter) gcLoop() {
+func (l *rateLimiter) gcLoop() {
for range time.Tick(l.window) {
l.mu.Lock()
now := time.Now()
docs/security/AUDIT-2026-07-29.md
@@ -0,0 +1,163 @@
+# Audit de sécurité — nouvelles surfaces d'attaque (2026-07-29)
+
+- **Périmètre** : uniquement ce qui a été ajouté depuis le premier audit
+ (`AUDIT.md`, commit `523f049`) — clone HTTPS anonyme, notifications
+ fédérées, dépôts épinglés, historique de commits, croissance du RPC
+ admin, et le changement d'Ingress qui les accompagne.
+- **Nature** : revue de code adversariale + tests de reproduction pour
+ chaque correctif.
+- **État** : les correctifs identifiés ci-dessous sont **déjà appliqués**
+ dans ce commit, avec un test de régression pour chacun des deux qui s'y
+ prêtaient (`internal/store/notifications_test.go`,
+ `internal/federation/notify_test.go`).
+
+---
+
+## 1. Synthèse
+
+Aucune faille critique (pas de fuite de dépôt privé, pas de contournement
+de l'authentification, pas d'injection). Les points trouvés sont des trous
+de limitation de débit / de plafonnement — la même catégorie que H1 dans
+l'audit précédent, désormais étendue aux deux nouvelles surfaces qui
+acceptent du trafic non authentifié : le clone HTTP anonyme et la
+réception de notifications fédérées.
+
+| ID | Sévérité | Titre | Statut |
+|----|----------|-------|--------|
+| M1 | 🟠 Medium | Aucune limitation de débit sur le clone HTTP anonyme | ✅ corrigé |
+| M2 | 🟠 Medium | Notifications non plafonnées par destinataire | ✅ corrigé |
+| M3 | 🟠 Medium | Destinataire de notification jamais vérifié comme utilisateur réel | ✅ corrigé |
+| L1 | 🟡 Low | `stderr` de git ignoré silencieusement sur les endpoints HTTP | ✅ corrigé |
+| L2 | 🟡 Low | Le contenu d'une notification n'est pas vérifié, seule sa signature l'est | ⚠️ atténué (texte), limite inhérente au modèle |
+
+---
+
+## 2. Détail
+
+### 🟠 M1 — Aucune limitation de débit sur le clone HTTP anonyme
+
+**Fichier** : `cmd/gitfed-web/handlers_git_http.go`
+
+`/{owner}/{repo}.git/info/refs` et `/git-upload-pack` n'avaient, avant ce
+correctif, aucune limite d'appels — contrairement au login (`loginByIP`)
+et à la réception de notifications (limiteur dédié). Chaque requête peut
+déclencher un sous-processus `git upload-pack`, et la fenêtre d'écriture y
+est délibérément étendue à 10 minutes (pour laisser le temps à un clone
+volumineux sur une connexion lente) — ce qui aggrave l'impact d'un flot de
+requêtes plutôt que de le limiter.
+
+**Correctif** : `ratelimit.go` généralisé (`loginLimiter` → `rateLimiter`,
+réutilisable) ; nouveau `gitHTTPByIP` à 60 requêtes/minute par IP, appliqué
+avant toute autre logique dans les deux handlers. Vérifié manuellement :
+la 61ᵉ requête consécutive depuis la même IP reçoit un `429`, un clone
+normal (1-2 requêtes) n'est jamais affecté.
+
+---
+
+### 🟠 M2 — Notifications non plafonnées par destinataire
+
+**Fichier** : `internal/store/notifications.go`
+
+Recevoir une notification ne suppose aucune relation préexistante — c'est
+volontaire (voir le commentaire de paquet dans `internal/federation/notify.go`).
+Mais sans plafond, n'importe quelle instance peut faire grossir
+indéfiniment la part d'un utilisateur réel dans la base en lui envoyant de
+nombreuses fausses réclamations distinctes (des `Repo`/`Actor` différents
+à chaque fois contournent la déduplication, qui ne joue que sur un
+doublon exact encore en attente).
+
+**Correctif** : `MaxNotificationsPerPrincipal = 200`. Au-delà, la plus
+ancienne notification est évincée pour faire de la place à la nouvelle
+(plutôt que de rejeter la nouvelle, ce qui permettrait à du spam de
+masquer une notification légitime derrière lui). Testé dans
+`TestNotificationCapEvictsOldest`.
+
+---
+
+### 🟠 M3 — Destinataire de notification jamais vérifié comme utilisateur réel
+
+**Fichier** : `internal/federation/notify.go`
+
+La vérification d'origine ne contrôlait que le **domaine** du principal
+ciblé (`bob@ailleurs.net` → le suffixe doit être le domaine local), jamais
+que `bob` existe réellement. Une notification pour un nom d'utilisateur
+inventé était donc acceptée et stockée sans jamais pouvoir être vue par
+personne — un vecteur de gonflement de la base par du spam ciblant des
+comptes qui n'existent pas.
+
+**Correctif** : `st.GetUser(username)` vérifié avant tout traitement
+supplémentaire (avant même la validation de fraîcheur ou l'appel réseau de
+vérification de signature — échoue tôt et sans consommer de ressources
+inutiles). Testé dans `TestNotifyHandlerRejectsUnknownRecipient` /
+`TestNotifyHandlerAcceptsKnownRecipient`.
+
+---
+
+### 🟡 L1 — `stderr` de git ignoré silencieusement
+
+**Fichier** : `cmd/gitfed-web/handlers_git_http.go`
+
+`exec.Command` sans `Stderr` défini envoie la sortie d'erreur vers
+`/dev/null` (comportement par défaut de `os/exec` en Go) — pas une fuite
+(rien n'était renvoyé au client), mais un angle mort opérationnel : un
+`git upload-pack` qui échoue sur un dépôt corrompu ne laissait aucune
+trace exploitable, contrairement au chemin SSH équivalent
+(`internal/gitexec.Serve`), qui capture correctement `stderr`.
+
+**Correctif** : `stderr` capturé dans un buffer et inclus dans le
+`log.Printf` d'erreur côté serveur, jamais renvoyé au client — cohérent
+avec le reste du code base.
+
+---
+
+### 🟡 L2 — Le contenu d'une notification n'est pas vérifié
+
+**Fichier** : `internal/federation/notify.go`
+
+La signature prouve que le message vient bien de l'instance qui prétend
+l'envoyer — elle ne prouve rien sur la véracité de son **contenu**
+(`Repo`, `Role`, `Actor` sont des champs libres). N'importe quelle
+instance peut donc envoyer une notification prétendant "vous avez un accès
+admin sur tel dépôt" sans que ce soit vrai, à des fins d'ingénierie
+sociale (crédibiliser un message de phishing envoyé par un autre canal).
+
+**Analyse d'impact** : c'est une limite *inhérente* à un système
+volontairement non-autoritaire (voir le commentaire de paquet) — accepter
+une notification ne fait qu'épingler un lien, jamais échanger le moindre
+identifiant ni ouvrir de session. Le pire abus reste donc « recevoir un
+message trompeur mais inerte », pas une élévation de privilège.
+
+**Traitement retenu** : renforcement du texte affiché sur `/notifications`
+pour rendre cette limite explicite (« son contenu n'est pas vérifié
+indépendamment... votre accès réel est celui que le propriétaire de cette
+instance a réellement défini, peu importe ce qu'affirme une notification »)
+plutôt qu'un changement de mécanisme — ajouter une vérification de
+contenu supposerait que l'instance réceptrice puisse interroger l'ACL de
+l'instance émettrice, ce qui est exactement le couplage fort que ce
+système a été conçu pour éviter (voir `DESIGN.md` §3, non-objectifs).
+
+---
+
+## 3. Contrôles vérifiés et jugés sains
+
+| Surface | Constat |
+|---|---|
+| `repo.Path` dans le clone HTTP | Toujours résolu via `store.GetRepo`, jamais dérivé d'une entrée attaquant — même absence d'injection d'argument que sur le chemin SSH existant. |
+| Redirections HTTP pendant la vérification de signature | `guardedDial` s'applique à **chaque** connexion TCP que le `Transport` établit, y compris après une redirection — impossible de contourner le blocage d'IP privée par ce biais. `Fetch` rejette en plus tout document dont le `domain` déclaré ne correspond pas à celui demandé. |
+| Exemption CSRF sur `/git-upload-pack` | Le suffixe de chemin qui déclenche l'exemption n'est atteignable que par le handler `handleGitUploadPack` lui-même (aucune autre route ne se termine ainsi) — impossible de faire passer une requête vers un handler différent sous couvert de cette exemption. |
+| Écriture via le clone HTTP | Aucune route `git-receive-pack` n'existe côté HTTP, point de vérification trivial mais confirmé (`grep` sur le fichier de routes). |
+| Portée des nouvelles méthodes `admin.Ops` (pins, notifications) | Toujours appelées avec `sess.Principal` dérivé de la session HTTP, jamais d'un paramètre soumis par le client — cohérent avec le modèle de confiance déjà documenté du socket RPC (accès total une fois le socket atteint, la vraie barrière est côté HTTP). |
+| XSS via un dépôt épinglé | `html/template` échappe `.Domain`/`.Repo` dans le contexte HTML ; le préfixe `https://` est en dur dans le template, donc une valeur du type `javascript:...` saisie comme "domaine" ne produit jamais une URI `javascript:` exploitable, juste un lien cassé. |
+| Historique de commits | Format `git log` fixe (non influencé par l'attaquant), chemin de dépôt toujours résolu via le store, taille bornée (`--max-count=200`). |
+| Élargissement de l'Ingress (`/.well-known/gitfed.json` exact → `/.well-known/` préfixe) | Sans risque : la surface réelle reste bornée par les deux seules routes enregistrées côté `gitfed-server` (`gitfed.json`, `gitfed-notify`) ; tout le reste sous ce préfixe reçoit un 404 du mux Go, quelle que soit la largeur de la règle Ingress. |
+
+---
+
+## 4. Note opérationnelle (hors code)
+
+`deploy/update.sh` n'applique que `deployment.yaml` — un changement dans
+`ingress.yaml`, `service.yaml`, `networkpolicy.yaml` etc. doit être
+appliqué manuellement (`kubectl apply -f deploy/k8s/<fichier>.yaml`) après
+le déploiement, sans quoi il reste committé sans effet en production.
+C'est exactement ce qui s'est produit pour le changement de préfixe
+Ingress ci-dessus, repéré et corrigé pendant cette session.
internal/federation/notify.go
@@ -166,14 +166,21 @@ func NotifyHandler(st *store.Store, localDomain string, insecure bool) http.Hand
return
}
- // Cheap local checks before any network call: the recipient must
- // actually be local, and the claimed sender must look like a real
+ // Cheap local checks before any network call: the recipient must be
+ // a real local user (not just domain-shaped — otherwise anyone
+ // could bloat the store by notifying made-up usernames, since
+ // nothing about receiving one requires a real relationship to
+ // exist first), and the claimed sender must look like a real
// public hostname.
- _, principalDomain, ok := strings.Cut(payload.Principal, "@")
+ username, principalDomain, ok := strings.Cut(payload.Principal, "@")
if !ok || principalDomain != localDomain {
http.NotFound(w, r)
return
}
+ if _, err := st.GetUser(username); err != nil {
+ http.NotFound(w, r)
+ return
+ }
if !insecure {
if err := ValidatePublicDomain(payload.FromDomain); err != nil {
http.Error(w, "invalid from_domain", http.StatusBadRequest)
internal/federation/notify_test.go
@@ -0,0 +1,61 @@
+package federation
+
+import (
+ "net/http"
+ "net/http/httptest"
+ "strings"
+ "testing"
+
+ "gitfed/internal/store"
+)
+
+// TestNotifyHandlerRejectsUnknownRecipient reproduces the gap the audit
+// found: NotifyHandler only checked that a notification's principal ended
+// in "@" + the local domain, never that the username part was a real local
+// account. Since receiving a notification requires no pre-existing
+// relationship (that's the point — see the package doc comment), anything
+// that didn't check this let any instance bloat the store by notifying
+// made-up usernames. This must be rejected before the signature is even
+// looked at (a nonsense signature here still exercises the ordering).
+func TestNotifyHandlerRejectsUnknownRecipient(t *testing.T) {
+ st := newTestStore(t)
+
+ body := `{"payload":{"from_domain":"chez-moi.fr","principal":"nobody@local.test","repo":"alice/x","role":"read","actor":"alice@chez-moi.fr","issued_at":0},"sig_format":"x","sig_blob":"AA=="}`
+ req := httptest.NewRequest(http.MethodPost, "/.well-known/gitfed-notify", strings.NewReader(body))
+ rec := httptest.NewRecorder()
+
+ NotifyHandler(st, "local.test", true).ServeHTTP(rec, req)
+
+ if rec.Code != http.StatusNotFound {
+ t.Fatalf("status = %d, want %d for a notification addressed to a nonexistent local user", rec.Code, http.StatusNotFound)
+ }
+ notifs, err := st.ListNotifications("nobody@local.test")
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(notifs) != 0 {
+ t.Fatal("a notification was stored for a nonexistent user")
+ }
+}
+
+// TestNotifyHandlerAcceptsKnownRecipient confirms the same request format
+// clears the recipient check (and only fails later, on signature
+// verification, which is exercised elsewhere) once the user actually
+// exists — i.e. the fix in the test above didn't just make the handler
+// reject everything.
+func TestNotifyHandlerAcceptsKnownRecipient(t *testing.T) {
+ st := newTestStore(t)
+ if err := st.CreateUser(store.User{Username: "bob"}); err != nil {
+ t.Fatal(err)
+ }
+
+ body := `{"payload":{"from_domain":"chez-moi.fr","principal":"bob@local.test","repo":"alice/x","role":"read","actor":"alice@chez-moi.fr","issued_at":0},"sig_format":"x","sig_blob":"AA=="}`
+ req := httptest.NewRequest(http.MethodPost, "/.well-known/gitfed-notify", strings.NewReader(body))
+ rec := httptest.NewRecorder()
+
+ NotifyHandler(st, "local.test", true).ServeHTTP(rec, req)
+
+ if rec.Code == http.StatusNotFound {
+ t.Fatal("a known local user's notification was rejected as if the user didn't exist")
+ }
+}
internal/i18n/strings_en.go
@@ -143,7 +143,7 @@ var en = map[string]string{
// ---------- notifications ----------
"notif.title": "Notifications",
- "notif.note": "Another instance telling you that you were granted access somewhere — informational only. Accepting just pins the link; it grants nothing by itself.",
+ "notif.note": "Another instance telling you that you were granted access somewhere — informational only, and its content isn't independently verified (only that it really was signed by that instance). Accepting just pins the link; it grants nothing by itself, and your actual access is whatever that instance's owner set it to, regardless of what a notification claims.",
"notif.granted_by": "granted by",
"notif.accept": "Accept",
"notif.dismiss": "Dismiss",
internal/i18n/strings_fr.go
@@ -143,7 +143,7 @@ var fr = map[string]string{
// ---------- notifications ----------
"notif.title": "Notifications",
- "notif.note": "Une autre instance vous informe qu'un accès vous a été accordé chez elle — purement informatif. Accepter épingle juste le lien ; ça n'accorde rien par soi-même.",
+ "notif.note": "Une autre instance vous informe qu'un accès vous a été accordé chez elle — purement informatif, et son contenu n'est pas vérifié indépendamment (seule la signature de cette instance l'est). Accepter épingle juste le lien ; ça n'accorde rien par soi-même, et votre accès réel est celui que le propriétaire de cette instance a réellement défini, peu importe ce qu'affirme une notification.",
"notif.granted_by": "accordé par",
"notif.accept": "Accepter",
"notif.dismiss": "Ignorer",
internal/store/notifications.go
@@ -37,6 +37,15 @@ type Notification struct {
func notifKey(principal, id string) string { return principal + "\x00" + id }
+// MaxNotificationsPerPrincipal bounds how many notifications one recipient
+// accumulates — nothing about receiving one requires any relationship to
+// exist first (see internal/federation/notify.go), so without a cap, any
+// instance could bloat a real user's slice of the store indefinitely by
+// sending many distinct fake claims. Oldest is evicted to make room rather
+// than rejecting the newest, so spam can't hide a legitimate notification
+// behind it.
+const MaxNotificationsPerPrincipal = 200
+
// CreateNotification records n, generating an ID if it doesn't have one.
// If a pending notification already exists for the same
// (Principal, FromDomain, Repo, Actor) — the common case being the same
@@ -55,6 +64,17 @@ func (s *Store) CreateNotification(n Notification) error {
return putJSON(tx, bucketNotifs, notifKey(e.Principal, e.ID), e)
}
}
+ if len(existing) >= MaxNotificationsPerPrincipal {
+ oldest := existing[0]
+ for _, e := range existing[1:] {
+ if e.ReceivedAt.Before(oldest.ReceivedAt) {
+ oldest = e
+ }
+ }
+ if err := deleteKey(tx, bucketNotifs, notifKey(oldest.Principal, oldest.ID)); err != nil {
+ return err
+ }
+ }
if n.ID == "" {
id, err := randomToken()
if err != nil {
internal/store/notifications_test.go
@@ -0,0 +1,95 @@
+package store
+
+import (
+ "fmt"
+ "path/filepath"
+ "testing"
+)
+
+// TestNotificationDedup guards the "same grant notified twice" path: a
+// second CreateNotification for the same (Principal, FromDomain, Repo,
+// Actor) while the first is still pending must refresh it in place, not
+// pile up a duplicate.
+func TestNotificationDedup(t *testing.T) {
+ s, err := Open(filepath.Join(t.TempDir(), "gitfed.db"))
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer s.Close()
+
+ n := Notification{Principal: "bob@ailleurs.net", FromDomain: "chez-moi.fr", Repo: "alice/x", Role: "read", Actor: "alice@chez-moi.fr"}
+ if err := s.CreateNotification(n); err != nil {
+ t.Fatal(err)
+ }
+ n.Role = "write"
+ if err := s.CreateNotification(n); err != nil {
+ t.Fatal(err)
+ }
+
+ all, err := s.ListNotifications("bob@ailleurs.net")
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(all) != 1 {
+ t.Fatalf("got %d notifications, want 1 (second create should have refreshed the first)", len(all))
+ }
+ if all[0].Role != "write" {
+ t.Fatalf("role = %q, want the refreshed value %q", all[0].Role, "write")
+ }
+}
+
+// TestNotificationCapEvictsOldest reproduces the gap the audit found: with
+// no cap, any instance could bloat a real user's slice of the store
+// indefinitely by sending distinct fake claims (nothing about receiving a
+// notification requires the underlying grant to be real). Once the cap is
+// hit, the oldest entry must be evicted to make room for a new one rather
+// than the new one being silently dropped — otherwise spam could bury a
+// legitimate notification behind itself.
+func TestNotificationCapEvictsOldest(t *testing.T) {
+ s, err := Open(filepath.Join(t.TempDir(), "gitfed.db"))
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer s.Close()
+
+ const principal = "bob@ailleurs.net"
+ for i := 0; i < MaxNotificationsPerPrincipal; i++ {
+ n := Notification{Principal: principal, FromDomain: "chez-moi.fr", Repo: fmt.Sprintf("alice/repo-%d", i), Role: "read", Actor: "alice@chez-moi.fr"}
+ if err := s.CreateNotification(n); err != nil {
+ t.Fatalf("create %d: %v", i, err)
+ }
+ }
+ all, err := s.ListNotifications(principal)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(all) != MaxNotificationsPerPrincipal {
+ t.Fatalf("got %d notifications, want the cap %d", len(all), MaxNotificationsPerPrincipal)
+ }
+
+ // One more, distinct from all the others, should evict the oldest
+ // rather than being dropped or growing past the cap.
+ newest := Notification{Principal: principal, FromDomain: "chez-moi.fr", Repo: "alice/one-too-many", Role: "read", Actor: "alice@chez-moi.fr"}
+ if err := s.CreateNotification(newest); err != nil {
+ t.Fatal(err)
+ }
+ all, err = s.ListNotifications(principal)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(all) != MaxNotificationsPerPrincipal {
+ t.Fatalf("got %d notifications after exceeding the cap, want it to stay at %d", len(all), MaxNotificationsPerPrincipal)
+ }
+ found := false
+ for _, n := range all {
+ if n.Repo == "alice/one-too-many" {
+ found = true
+ }
+ if n.Repo == "alice/repo-0" {
+ t.Fatal("oldest notification (repo-0) should have been evicted, but is still present")
+ }
+ }
+ if !found {
+ t.Fatal("newest notification was dropped instead of the oldest being evicted")
+ }
+}