fix(security): deliver /access notifications off the request path
REVIEW-001 (M0) M2s: POST /access was a timing oracle — a hit did two synchronous SMTP sends, a miss one SELECT, so "is this email on record?" leaked (A3). Dispatch the notification through a Deps.Dispatch seam (default `go fn()`), detached from the request context (context.WithoutCancel, so audit values survive) and bounded by a 30s timeout. Tests inject a synchronous runner where they assert no-spam semantics, and a new test asserts the default is asynchronous with a blocking mailer. A panic in the detached goroutine is recovered: net/http no longer guards it, and a notification must never take the process down.
This commit is contained in:
parent
b4222077b1
commit
6f2ea67503
2 changed files with 155 additions and 1 deletions
|
|
@ -72,6 +72,9 @@ type Deps struct {
|
|||
BaseURL string
|
||||
// PendingTTL bounds the approvable window (default DefaultPendingTTL).
|
||||
PendingTTL time.Duration
|
||||
// Dispatch runs a notification OFF the request path (M2s). Defaults to
|
||||
// `go fn()`; tests inject a synchronous runner for determinism.
|
||||
Dispatch func(fn func())
|
||||
// Logger receives structured logs. Defaults to slog.Default().
|
||||
Logger *slog.Logger
|
||||
// Now is the clock (tests). Defaults to time.Now.
|
||||
|
|
@ -104,6 +107,9 @@ func New(d Deps) (*Actions, error) {
|
|||
if d.Now == nil {
|
||||
d.Now = time.Now
|
||||
}
|
||||
if d.Dispatch == nil {
|
||||
d.Dispatch = func(fn func()) { go fn() }
|
||||
}
|
||||
if d.PendingTTL <= 0 {
|
||||
d.PendingTTL = DefaultPendingTTL
|
||||
}
|
||||
|
|
@ -155,10 +161,36 @@ func (a *Actions) openRequest(ctx context.Context, requester users.User) {
|
|||
return
|
||||
}
|
||||
if created {
|
||||
a.notify(ctx, requester, req)
|
||||
a.notifyAsync(ctx, requester, req)
|
||||
}
|
||||
}
|
||||
|
||||
// notifyTimeout bounds a detached notification's work.
|
||||
const notifyTimeout = 30 * time.Second
|
||||
|
||||
// notifyAsync delivers the notifications OFF the request path (M2s): an /access
|
||||
// HIT must not take measurably longer than a MISS, or the timing difference is
|
||||
// an enumeration oracle (A3). The work is detached from the request context
|
||||
// (which is cancelled as soon as the response is written) but keeps its values,
|
||||
// so the audit trail is intact.
|
||||
//
|
||||
// A panic here is NOT recovered by net/http (it is no longer a request
|
||||
// goroutine), so it is recovered here: a notification failure must never take
|
||||
// the process down.
|
||||
func (a *Actions) notifyAsync(ctx context.Context, requester users.User, req Request) {
|
||||
base := context.WithoutCancel(ctx)
|
||||
a.d.Dispatch(func() {
|
||||
defer func() {
|
||||
if p := recover(); p != nil {
|
||||
a.d.Logger.Error("recovery: notification panicked", "panic", p)
|
||||
}
|
||||
}()
|
||||
nctx, cancel := context.WithTimeout(base, notifyTimeout)
|
||||
defer cancel()
|
||||
a.notify(nctx, requester, req)
|
||||
})
|
||||
}
|
||||
|
||||
// notify emails the approver (the requester's manager, the chain link above
|
||||
// them) and the employee (SPEC-007 §4: "notify the approver chain + employee").
|
||||
func (a *Actions) notify(ctx context.Context, requester users.User, req Request) {
|
||||
|
|
|
|||
122
internal/core/recovery/flow_test.go
Normal file
122
internal/core/recovery/flow_test.go
Normal file
|
|
@ -0,0 +1,122 @@
|
|||
package recovery_test
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"context"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"net/url"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"sync"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"git.lohmar.co.uk/cclohmar/NextMyDay/internal/core/audit"
|
||||
"git.lohmar.co.uk/cclohmar/NextMyDay/internal/core/auth"
|
||||
"git.lohmar.co.uk/cclohmar/NextMyDay/internal/core/httpx"
|
||||
"git.lohmar.co.uk/cclohmar/NextMyDay/internal/core/mail"
|
||||
"git.lohmar.co.uk/cclohmar/NextMyDay/internal/core/recovery"
|
||||
"git.lohmar.co.uk/cclohmar/NextMyDay/internal/core/store"
|
||||
"git.lohmar.co.uk/cclohmar/NextMyDay/internal/core/users"
|
||||
)
|
||||
|
||||
// blockingMailer blocks every Send until release is closed, and signals when a
|
||||
// send has begun.
|
||||
type blockingMailer struct {
|
||||
started chan struct{}
|
||||
release chan struct{}
|
||||
done chan struct{}
|
||||
once sync.Once
|
||||
}
|
||||
|
||||
func (m *blockingMailer) Send(_ context.Context, _ mail.Message) error {
|
||||
select {
|
||||
case m.started <- struct{}{}:
|
||||
default:
|
||||
}
|
||||
<-m.release
|
||||
m.once.Do(func() { close(m.done) })
|
||||
return nil
|
||||
}
|
||||
|
||||
// TestAccessNotificationIsAsynchronous covers M2s: POST /access must answer
|
||||
// uniformly whether or not the email matched, so a HIT's notification is
|
||||
// delivered OFF the request path. A synchronous implementation would block the
|
||||
// response on the (slow) mailer and leak "is this email on record?".
|
||||
func TestAccessNotificationIsAsynchronous(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
|
||||
db, err := store.Open(filepath.Join(t.TempDir(), "test.db"))
|
||||
if err != nil {
|
||||
t.Fatalf("store.Open: %v", err)
|
||||
}
|
||||
t.Cleanup(func() { _ = db.Close() })
|
||||
if err := db.Migrate(); err != nil {
|
||||
t.Fatalf("Migrate: %v", err)
|
||||
}
|
||||
|
||||
aud := audit.New(db.DB)
|
||||
us := users.New(db.DB, aud)
|
||||
if _, err := us.Create(ctx, 0, "admin@example.com", "A", users.RoleAdmin); err != nil {
|
||||
t.Fatalf("create user: %v", err)
|
||||
}
|
||||
|
||||
sealer, err := auth.NewAEADSealer(bytes.Repeat([]byte{0x7a}, auth.KeySize))
|
||||
if err != nil {
|
||||
t.Fatalf("NewAEADSealer: %v", err)
|
||||
}
|
||||
core := auth.New(db.DB, aud, auth.WithSealer(sealer))
|
||||
pre, err := httpx.NewPreSessionCSRF([]byte("flow-cookie-secret-0123456789abcdef"), false)
|
||||
if err != nil {
|
||||
t.Fatalf("NewPreSessionCSRF: %v", err)
|
||||
}
|
||||
|
||||
m := &blockingMailer{
|
||||
started: make(chan struct{}, 1),
|
||||
release: make(chan struct{}),
|
||||
done: make(chan struct{}),
|
||||
}
|
||||
// Dispatch is deliberately NOT injected: the default must be asynchronous.
|
||||
a, err := recovery.New(recovery.Deps{
|
||||
Requests: recovery.NewStore(db.DB, aud),
|
||||
Users: us,
|
||||
Identities: core.Identities(),
|
||||
Mailer: m,
|
||||
CSRF: pre,
|
||||
Audit: aud,
|
||||
BaseURL: "https://myday.example.com",
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("recovery.New: %v", err)
|
||||
}
|
||||
|
||||
form := url.Values{"email": {"admin@example.com"}}.Encode()
|
||||
req := httptest.NewRequest("POST", "/access?do=auth&action=recover", strings.NewReader(form))
|
||||
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
|
||||
rec := httptest.NewRecorder()
|
||||
|
||||
returned := make(chan error, 1)
|
||||
go func() { returned <- a.Request(rec, req) }()
|
||||
|
||||
select {
|
||||
case err := <-returned:
|
||||
if err != nil {
|
||||
t.Fatalf("Request: %v", err)
|
||||
}
|
||||
case <-time.After(2 * time.Second):
|
||||
close(m.release)
|
||||
t.Fatal("POST /access blocked on the notification — it is still synchronous (M2s)")
|
||||
}
|
||||
if rec.Code != http.StatusOK {
|
||||
t.Errorf("status = %d, want 200", rec.Code)
|
||||
}
|
||||
|
||||
// The notification did run (off the request path); let it finish.
|
||||
close(m.release)
|
||||
select {
|
||||
case <-m.done:
|
||||
case <-time.After(2 * time.Second):
|
||||
t.Fatal("the detached notification never ran")
|
||||
}
|
||||
}
|
||||
Loading…
Reference in a new issue