Compare commits
2
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
1ac8102243 | ||
|
|
0945831442 |
@@ -25,7 +25,6 @@ COPY . .
|
||||
# would need a docker daemon inside the build. Keep these steps in step with
|
||||
# Dockerfile.lint, including --network=none (see its header for why).
|
||||
RUN make fmt-check
|
||||
RUN script/assets
|
||||
RUN --network=none golangci-lint config verify --config .golangci.yml
|
||||
RUN --network=none golangci-lint run --config .golangci.yml --build-tags browser ./...
|
||||
|
||||
|
||||
@@ -31,10 +31,6 @@ FROM deps AS lint
|
||||
|
||||
COPY . .
|
||||
|
||||
# static/static.go embeds the Alpine.js file this extracts from 3p/; without
|
||||
# it the static package does not compile and cannot be linted.
|
||||
RUN script/assets
|
||||
|
||||
# `run` silently ignores config keys it does not recognize, so a typo would
|
||||
# disable a setting without a word. `config verify` is what catches that.
|
||||
RUN --network=none golangci-lint config verify --config .golangci.yml
|
||||
|
||||
@@ -1327,15 +1327,17 @@ under the real policy and checks that: both add forms stay hidden until Add is
|
||||
clicked; choosing Slack in the add target form leaves the HTTP fields out of
|
||||
what it submits, also after leaving the page and going back to it, when the
|
||||
browser restores the choice; the Copy button beside an entrypoint URL reads
|
||||
"Copied" once clicked; an event expands and collapses, and so do a delivery's
|
||||
attempts inside it; and at phone width the menu button opens and closes the
|
||||
mobile menu. It also fails if the browser reports a console warning or error,
|
||||
an uncaught exception, or anything the policy refused. `make check` and the
|
||||
image build lint it but do not run it, and `make test` leaves it out (its file
|
||||
is built only with the `browser` build tag). Run it with `make test-browser`
|
||||
after changing `templates/` or `static/js/`: that builds `Dockerfile.browser`,
|
||||
which runs the test in a digest-pinned headless browser image, so the host
|
||||
needs no browser.
|
||||
"Copied" once clicked; an entrypoint's Edit button shows its edit form in place
|
||||
of its description and hides until the form closes, Cancel hides the form and
|
||||
drops what was typed, and Save changes the description; an event expands and
|
||||
collapses, and so do a delivery's attempts inside it; and at phone width the
|
||||
menu button opens and closes the mobile menu. It also fails if the browser
|
||||
reports a console warning or error, an uncaught exception, or anything the
|
||||
policy refused. `make check` and the image build lint it but do not run it, and
|
||||
`make test` leaves it out (its file is built only with the `browser` build tag).
|
||||
Run it with `make test-browser` after changing `templates/` or `static/js/`:
|
||||
that builds `Dockerfile.browser`, which runs the test in a digest-pinned
|
||||
headless browser image, so the host needs no browser.
|
||||
|
||||
The package's tarball is committed as `3p/alpinejs-csp-3.14.9.tgz`, byte for
|
||||
byte as the npm registry publishes it. It is a dependency, not this repo's build
|
||||
@@ -1348,9 +1350,7 @@ apply. The directory is `3p/` rather than `vendor/` because Go treats a root
|
||||
where `go:embed` picks it up. `script/test`, `make build` and `make dev` run
|
||||
it first, and the Dockerfile builds through `make test` and `make build`, so
|
||||
nothing downloads Alpine.js. The extracted file is not committed, and
|
||||
`.dockerignore` keeps any host copy out of the build context. `static/static.go`
|
||||
names every file it embeds, so a build that skips the extraction, such as a
|
||||
bare `go build`, fails with an error naming `js/alpine.min.js`.
|
||||
`.dockerignore` keeps any host copy out of the build context.
|
||||
|
||||
To move to a new version: download
|
||||
`https://registry.npmjs.org/@alpinejs/csp/-/csp-<version>.tgz`, check it against
|
||||
@@ -2895,6 +2895,7 @@ returns to the page that was asked for.
|
||||
| `POST` | `/hook/{id}/deliveries/{deliveryID}/replay` | Replay a finished delivery: creates a new delivery for the same event against the target's current configuration (30 per minute per bucket, then `429`) |
|
||||
| `POST` | `/hook/{id}/events/{eventID}/resubmit` | Resubmit a stored event: creates a new event copying it and fans that out to every currently active target (30 per minute per bucket, then `429`) |
|
||||
| `POST` | `/hook/{id}/entrypoints` | Add entrypoint to webhook |
|
||||
| `POST` | `/hook/{id}/entrypoints/{entrypointID}/edit` | Change an entrypoint's description; its URL stays the same |
|
||||
| `POST` | `/hook/{id}/entrypoints/{entrypointID}/delete` | Delete an entrypoint |
|
||||
| `POST` | `/hook/{id}/entrypoints/{entrypointID}/toggle` | Enable or disable an entrypoint |
|
||||
| `POST` | `/hook/{id}/targets` | Add target to webhook |
|
||||
@@ -3247,9 +3248,9 @@ each hook. The order, read off the fx stop-hook log:
|
||||
|
||||
1. `ArchiveSweeper`
|
||||
2. `RetentionReaper`
|
||||
3. `server` — the HTTP drain, bounded separately by
|
||||
`server.ShutdownTimeout` (**3 seconds**), then a Sentry flush if
|
||||
`SENTRY_DSN` is set
|
||||
3. `server` — the HTTP drain, bounded by `server.ShutdownTimeout`
|
||||
(**3 seconds**) and by what the hooks before it left, then a Sentry
|
||||
flush if `SENTRY_DSN` is set
|
||||
4. `delivery.Engine` — waits for its workers, then closes the archive
|
||||
databases
|
||||
5. `healthcheck`
|
||||
@@ -3269,23 +3270,30 @@ exhaust the sequence budget at the instant it finished, and every
|
||||
later hook — the delivery engine, the healthcheck, the webhook DB
|
||||
manager and the database close — would be skipped in exactly the
|
||||
case where the drain mattered. 3 seconds leaves 2 seconds
|
||||
(`server.TailHookReserve`) for the tail, which is far more than the
|
||||
microseconds it needs.
|
||||
(`server.TailHookReserve`) for the tail. The reserve is that
|
||||
remainder, not a figure sized to the tail, which takes about a
|
||||
millisecond.
|
||||
|
||||
That reserve belongs to the tail hooks, not to the server hook, and
|
||||
the Sentry flush is what could take it: it runs after the drain
|
||||
**inside the same hook**, and `sentry.Flush` takes a bare duration
|
||||
and honours no context, so an unreachable Sentry endpoint would add
|
||||
its own timeout on top of a full-length drain and consume the whole
|
||||
sequence budget by itself. It is therefore clamped to whatever is
|
||||
left on the stop context minus the reserve, and skipped when that
|
||||
leaves too little to be worth attempting — so a full-length drain
|
||||
means Sentry events are dropped rather than the database close being
|
||||
skipped.
|
||||
the server hook could take it in two ways. The hooks before it may
|
||||
already have spent part of the budget, so a full 3-second drain
|
||||
would come out of the reserve; the drain is therefore also bounded
|
||||
by whatever is left on the stop context minus the reserve. And the
|
||||
Sentry flush runs after the drain **inside the same hook**, and
|
||||
`sentry.Flush` takes a bare duration and honours no context, so an
|
||||
unreachable Sentry endpoint would add its own timeout on top of a
|
||||
full-length drain and consume the whole sequence budget by itself.
|
||||
It is clamped the same way, and skipped when that leaves too little
|
||||
to be worth attempting — so a full-length drain means Sentry events
|
||||
are dropped rather than the database close being skipped.
|
||||
|
||||
This does not make the database close unconditional: a wedged
|
||||
`ArchiveSweeper` or `RetentionReaper` still runs first and can
|
||||
consume the whole budget on its own.
|
||||
This does not make the database close unconditional. A slow
|
||||
`ArchiveSweeper` or `RetentionReaper` is enough to cut the shutdown
|
||||
short, not only one that consumes the whole budget: what they spend
|
||||
comes out of the drain first, so after 2 seconds of theirs a request
|
||||
still in flight gets 1 second to finish, and after 3 it gets none.
|
||||
Past 3 seconds they spend the reserve itself, and one that takes the
|
||||
whole budget skips every hook after it, the database close included.
|
||||
|
||||
The value is chosen to sit inside the container stop grace period.
|
||||
Docker's default `docker stop` grace is 10 seconds and the Dockerfile
|
||||
|
||||
@@ -38,17 +38,19 @@ import (
|
||||
// hook that used the whole budget would exhaust it at that instant,
|
||||
// and fx would skip every hook after the server — the delivery
|
||||
// engine, the healthcheck, the webhook DB manager and the database
|
||||
// close. That hook is the 3s HTTP drain plus the Sentry flush that
|
||||
// follows it in the same hook, so the flush is clamped to the stop
|
||||
// close. That hook is the HTTP drain plus the Sentry flush that
|
||||
// follows it in the same hook, and each is clamped to the stop
|
||||
// context's remaining time less server.TailHookReserve rather than
|
||||
// running for its own fixed 2s; the reserve is what the tail hooks
|
||||
// live on, and they are microsecond-scale in normal operation.
|
||||
// running for its own fixed 3s and 2s; the reserve is what the tail
|
||||
// hooks live on, and they are microsecond-scale in normal operation.
|
||||
// TestStopTimeout_LeavesHeadroomForTailHooks pins the arithmetic
|
||||
// across every drain length.
|
||||
// across every drain length and every amount of budget the hooks
|
||||
// before the server may already have spent.
|
||||
//
|
||||
// This does not make the database close unconditional: the
|
||||
// ArchiveSweeper and RetentionReaper hooks run before the server
|
||||
// and can still consume the whole budget on their own.
|
||||
// ArchiveSweeper and RetentionReaper hooks run before the server.
|
||||
// What they spend comes out of the drain first, but past 3s it comes
|
||||
// out of the reserve, and they can consume the whole budget.
|
||||
const stopTimeout = 5 * time.Second
|
||||
|
||||
// exitUsage is the status for a command line this binary cannot make
|
||||
|
||||
@@ -252,22 +252,40 @@ const tailHeadroom = 2 * time.Second
|
||||
// can produce, since a shorter drain leaves the flush more room and
|
||||
// the worst case is not necessarily at either extreme.
|
||||
//
|
||||
// Shrinking either budget, or unbounding the flush again, must fail
|
||||
// here rather than silently recreating a hook that swallows the
|
||||
// whole sequence.
|
||||
// Nor does the hook start on a full budget: the ArchiveSweeper and
|
||||
// RetentionReaper hooks run before it, and whatever they spent is
|
||||
// gone. The outer sweep walks every amount they can spend. Once they
|
||||
// have eaten into the headroom themselves, the hook must spend
|
||||
// nothing of what is left. A drain that starts on the full budget
|
||||
// must still get all of ShutdownTimeout, so a smaller stopTimeout
|
||||
// cannot silently shorten every drain.
|
||||
//
|
||||
// Shrinking either budget, or unbounding the drain or the flush
|
||||
// again, must fail here rather than silently recreating a hook that
|
||||
// swallows the whole sequence.
|
||||
func TestStopTimeout_LeavesHeadroomForTailHooks(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
require.Less(t, server.ShutdownTimeout, stopTimeout)
|
||||
require.Equal(
|
||||
t, server.ShutdownTimeout, server.DrainBudget(stopTimeout),
|
||||
"a drain that starts on the full stop budget is cut short",
|
||||
)
|
||||
|
||||
const step = 10 * time.Millisecond
|
||||
|
||||
for drain := time.Duration(0); drain <= server.ShutdownTimeout; drain += step {
|
||||
hook := drain + server.SentryFlushBudget(stopTimeout-drain)
|
||||
for spent := time.Duration(0); spent <= stopTimeout; spent += step {
|
||||
remaining := stopTimeout - spent
|
||||
longest := max(server.DrainBudget(remaining), 0)
|
||||
|
||||
require.LessOrEqual(
|
||||
t, hook+tailHeadroom, stopTimeout,
|
||||
"a %s drain leaves the tail hooks short", drain,
|
||||
)
|
||||
for drain := time.Duration(0); drain <= longest; drain += step {
|
||||
hook := drain + server.SentryFlushBudget(remaining-drain)
|
||||
|
||||
require.GreaterOrEqual(
|
||||
t, remaining-hook, min(remaining, tailHeadroom),
|
||||
"a %s drain after %s of earlier hooks leaves "+
|
||||
"the tail hooks short", drain, spent,
|
||||
)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,95 @@
|
||||
package handlers_test
|
||||
|
||||
import (
|
||||
"context"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"net/url"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/go-chi/chi"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
"gorm.io/gorm"
|
||||
"sneak.berlin/go/webhooker/internal/database"
|
||||
)
|
||||
|
||||
// TestHandleEntrypointToggle_DoesNotUndoAnEdit proves that a toggle
|
||||
// which loaded the entrypoint before an edit of its description was
|
||||
// saved does not write the old description back over the edit. The
|
||||
// edit is submitted from a callback on the toggle's own read of the
|
||||
// entrypoint, so it is saved after that read and before the toggle
|
||||
// writes.
|
||||
func TestHandleEntrypointToggle_DoesNotUndoAnEdit(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := setupSourceTest(t)
|
||||
wh := seedWebhookWithRetention(t, env.db, 30)
|
||||
ep := seedEntrypoint(t, env.db, wh.ID)
|
||||
require.True(t, ep.Active)
|
||||
|
||||
router := chi.NewRouter()
|
||||
router.Post(
|
||||
"/hook/{sourceID}/entrypoints/{entrypointID}/edit",
|
||||
env.handlers.HandleEntrypointEdit(),
|
||||
)
|
||||
router.Post(
|
||||
"/hook/{sourceID}/entrypoints/{entrypointID}/toggle",
|
||||
env.handlers.HandleEntrypointToggle(),
|
||||
)
|
||||
|
||||
// post submits one of the entrypoint's forms as the test user and
|
||||
// returns the response's status code.
|
||||
post := func(action string, form url.Values) int {
|
||||
req := httptest.NewRequestWithContext(
|
||||
context.Background(), http.MethodPost,
|
||||
"/hook/"+wh.ID+"/entrypoints/"+ep.ID+"/"+action,
|
||||
strings.NewReader(form.Encode()),
|
||||
)
|
||||
req.Header.Set(
|
||||
"Content-Type", "application/x-www-form-urlencoded",
|
||||
)
|
||||
|
||||
for _, c := range env.cookies {
|
||||
req.AddCookie(c)
|
||||
}
|
||||
|
||||
w := httptest.NewRecorder()
|
||||
router.ServeHTTP(w, req)
|
||||
|
||||
return w.Code
|
||||
}
|
||||
|
||||
var (
|
||||
edited bool
|
||||
editCode int
|
||||
)
|
||||
|
||||
require.NoError(t, env.db.DB().Callback().Query().
|
||||
After("gorm:query").
|
||||
Register("test:edit_after_toggle_read", func(tx *gorm.DB) {
|
||||
// Only the first read of an entrypoint, the toggle's,
|
||||
// submits the edit.
|
||||
if tx.Statement.Table != "entrypoints" || edited {
|
||||
return
|
||||
}
|
||||
|
||||
edited = true
|
||||
editCode = post(
|
||||
"edit", url.Values{"description": {"Billing sender"}},
|
||||
)
|
||||
}),
|
||||
)
|
||||
|
||||
require.Equal(t, http.StatusSeeOther, post("toggle", nil))
|
||||
require.Equal(t, http.StatusSeeOther, editCode)
|
||||
|
||||
var stored database.Entrypoint
|
||||
|
||||
require.NoError(
|
||||
t, env.db.DB().First(&stored, "id = ?", ep.ID).Error,
|
||||
)
|
||||
assert.False(t, stored.Active)
|
||||
assert.Equal(t, "Billing sender", stored.Description)
|
||||
}
|
||||
@@ -20,6 +20,7 @@ const (
|
||||
webhookSaved noticeCode = "webhook-saved"
|
||||
webhookDeleted noticeCode = "webhook-deleted"
|
||||
entrypointAdded noticeCode = "entrypoint-added"
|
||||
entrypointSaved noticeCode = "entrypoint-saved"
|
||||
entrypointDeleted noticeCode = "entrypoint-deleted"
|
||||
entrypointActivated noticeCode = "entrypoint-activated"
|
||||
entrypointDeactivated noticeCode = "entrypoint-deactivated"
|
||||
@@ -48,6 +49,7 @@ func noticeFor(r *http.Request) *notice {
|
||||
webhookSaved: {Text: "Webhook saved."},
|
||||
webhookDeleted: {Text: "Webhook deleted."},
|
||||
entrypointAdded: {Text: "Entrypoint added."},
|
||||
entrypointSaved: {Text: "Entrypoint description saved."},
|
||||
entrypointDeleted: {Text: "Entrypoint deleted."},
|
||||
entrypointActivated: {Text: "Entrypoint activated."},
|
||||
entrypointDeactivated: {Text: "Entrypoint deactivated."},
|
||||
|
||||
@@ -86,12 +86,13 @@ var errInjectedDelete = errors.New("injected delete failure")
|
||||
// save of an existing row.
|
||||
var errInjectedSave = errors.New("injected save failure")
|
||||
|
||||
// seedEntrypoint inserts an entrypoint for a webhook.
|
||||
// seedEntrypoint inserts an active entrypoint for a webhook and
|
||||
// returns it.
|
||||
func seedEntrypoint(
|
||||
t *testing.T,
|
||||
db *database.Database,
|
||||
webhookID string,
|
||||
) {
|
||||
) *database.Entrypoint {
|
||||
t.Helper()
|
||||
|
||||
ep := &database.Entrypoint{
|
||||
@@ -104,6 +105,8 @@ func seedEntrypoint(
|
||||
t,
|
||||
db.DB().Omit(clause.Associations).Create(ep).Error,
|
||||
)
|
||||
|
||||
return ep
|
||||
}
|
||||
|
||||
// countRows counts the live (not soft-deleted) rows of a model
|
||||
|
||||
@@ -1396,6 +1396,70 @@ func (h *Handlers) HandleEntrypointCreate() http.HandlerFunc {
|
||||
}
|
||||
}
|
||||
|
||||
// HandleEntrypointEdit handles changing an entrypoint's description.
|
||||
// It writes only the description column, so the entrypoint keeps its
|
||||
// URL, and an activate or deactivate saved since the page was shown
|
||||
// is not undone.
|
||||
func (h *Handlers) HandleEntrypointEdit() http.HandlerFunc {
|
||||
return func(w http.ResponseWriter, r *http.Request) {
|
||||
userID, ok := h.getUserID(r)
|
||||
if !ok {
|
||||
http.Redirect(
|
||||
w, r, "/pages/login", http.StatusSeeOther,
|
||||
)
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
sourceID := chi.URLParam(r, "sourceID")
|
||||
entrypointID := chi.URLParam(r, "entrypointID")
|
||||
|
||||
var webhook database.Webhook
|
||||
|
||||
err := h.db.DB().Where(
|
||||
"id = ? AND user_id = ?", sourceID, userID,
|
||||
).First(&webhook).Error
|
||||
if err != nil {
|
||||
h.renderError(w, r, http.StatusNotFound)
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
// The body size cap is enforced by the MaxBodySize
|
||||
// middleware, which runs before CSRF parses the form.
|
||||
err = r.ParseForm()
|
||||
if err != nil {
|
||||
h.renderError(w, r, http.StatusBadRequest)
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
result := h.db.DB().Model(&database.Entrypoint{}).Where(
|
||||
"id = ? AND webhook_id = ?", entrypointID, webhook.ID,
|
||||
).Update("description", r.PostFormValue("description"))
|
||||
if result.Error != nil {
|
||||
h.serverError(
|
||||
w, r, "failed to edit entrypoint", result.Error,
|
||||
)
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
// The id came from the URL and may name another webhook's
|
||||
// entrypoint, which this webhook does not have.
|
||||
if result.RowsAffected == 0 {
|
||||
h.renderError(w, r, http.StatusNotFound)
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
http.Redirect(
|
||||
w, r, withNotice("/hook/"+webhook.ID, entrypointSaved),
|
||||
http.StatusSeeOther,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
// HandleTargetCreate handles adding a new target to a webhook.
|
||||
func (h *Handlers) HandleTargetCreate() http.HandlerFunc {
|
||||
return func(w http.ResponseWriter, r *http.Request) {
|
||||
@@ -1877,9 +1941,13 @@ func (h *Handlers) HandleEntrypointToggle() http.HandlerFunc {
|
||||
return false, err
|
||||
}
|
||||
|
||||
ep.Active = !ep.Active
|
||||
// Only the active column: saving the whole row would
|
||||
// write back the description read above over an edit
|
||||
// saved since.
|
||||
active := !ep.Active
|
||||
|
||||
return ep.Active, h.db.DB().Save(&ep).Error
|
||||
return active, h.db.DB().Model(&ep).
|
||||
Update("active", active).Error
|
||||
},
|
||||
"failed to toggle entrypoint",
|
||||
entrypointActivated, entrypointDeactivated,
|
||||
|
||||
@@ -86,6 +86,7 @@ func TestAlpineRunsUnderTheSecurityPolicy(t *testing.T) {
|
||||
checkAddForms(ctx, t, page)
|
||||
checkTargetType(ctx, t, page+"/events")
|
||||
checkCopy(ctx, t, page)
|
||||
checkEntrypointEdit(ctx, t, page)
|
||||
checkEventLog(ctx, t, page+"/events", event.ID, target.Name)
|
||||
checkMobileMenu(ctx, t, page)
|
||||
|
||||
@@ -363,6 +364,62 @@ func checkCopy(ctx context.Context, t *testing.T, url string) {
|
||||
`clicking Copy does not show "Copied"`)
|
||||
}
|
||||
|
||||
// checkEntrypointEdit loads a webhook page whose entrypoint has no
|
||||
// description, and checks that Edit shows the edit form in place of
|
||||
// the description and hides until the form closes, so the form always
|
||||
// opens on the saved description; that Cancel hides it and drops what
|
||||
// was typed; and that Save changes the description the page shows.
|
||||
func checkEntrypointEdit(ctx context.Context, t *testing.T, url string) {
|
||||
t.Helper()
|
||||
|
||||
const (
|
||||
editForm = `form[action$="/edit"]`
|
||||
input = editForm + ` input[name="description"]`
|
||||
description = `//span[text()="Entrypoint"]`
|
||||
edit = `//button[text()="Edit"]`
|
||||
)
|
||||
|
||||
require.NoError(t, chromedp.Run(ctx, loadPage(url)))
|
||||
|
||||
assert.True(t, hidden(ctx, editForm),
|
||||
"the edit form shows before Edit is clicked")
|
||||
|
||||
click(ctx, t, edit)
|
||||
assert.True(t, shown(ctx, editForm),
|
||||
"clicking Edit does not show the edit form")
|
||||
assert.True(t, hidden(ctx, description),
|
||||
"the description stays shown beside the edit form")
|
||||
assert.True(t, hidden(ctx, edit),
|
||||
"Edit stays shown while the edit form is open")
|
||||
|
||||
require.NoError(t, chromedp.Run(
|
||||
ctx, chromedp.SendKeys(input, "draft", chromedp.ByQuery),
|
||||
))
|
||||
click(ctx, t, `//button[text()="Cancel"]`)
|
||||
assert.True(t, hidden(ctx, editForm),
|
||||
"clicking Cancel does not hide the edit form")
|
||||
assert.True(t, shown(ctx, description),
|
||||
"clicking Cancel does not show the description again")
|
||||
assert.True(t, shown(ctx, edit),
|
||||
"clicking Cancel does not show Edit again")
|
||||
|
||||
var typed string
|
||||
|
||||
click(ctx, t, edit)
|
||||
require.NoError(t, chromedp.Run(
|
||||
ctx, chromedp.Value(input, &typed, chromedp.ByQuery),
|
||||
))
|
||||
assert.Empty(t, typed, "Cancel keeps what was typed")
|
||||
|
||||
require.NoError(t, chromedp.Run(
|
||||
ctx, chromedp.SendKeys(input, "Billing sender", chromedp.ByQuery),
|
||||
))
|
||||
click(ctx, t, `//button[text()="Save"]`)
|
||||
|
||||
assert.True(t, shown(ctx, `//span[text()="Billing sender"]`),
|
||||
"saving the edit form does not change the description")
|
||||
}
|
||||
|
||||
// checkEventLog loads the event log and checks that clicking an event's
|
||||
// row expands it, that in there clicking its delivery shows the
|
||||
// delivery's attempts and clicking again hides them, and that clicking
|
||||
|
||||
@@ -1,6 +1,8 @@
|
||||
package server
|
||||
|
||||
import (
|
||||
"context"
|
||||
"log/slog"
|
||||
"net/http"
|
||||
"testing"
|
||||
|
||||
@@ -37,6 +39,14 @@ func SentryClientOptionsForTest(
|
||||
return sentryClientOptions(dsn, release)
|
||||
}
|
||||
|
||||
// CleanShutdownForTest runs the server's stop hook, cleanShutdown,
|
||||
// against hs: a server the test started itself, so it can hold a
|
||||
// request open across the drain. Sentry is off.
|
||||
func CleanShutdownForTest(ctx context.Context, hs *http.Server) {
|
||||
s := &Server{log: slog.New(slog.DiscardHandler), httpServer: hs}
|
||||
s.cleanShutdown(ctx)
|
||||
}
|
||||
|
||||
// newServerForTest builds a Server through New, as the application
|
||||
// does, on a lifecycle that is never started: the hooks New adds to
|
||||
// it never run, so nothing listens.
|
||||
|
||||
@@ -289,6 +289,10 @@ func (s *Server) setupSourceRoutes() {
|
||||
"/entrypoints",
|
||||
s.h.HandleEntrypointCreate(),
|
||||
)
|
||||
r.Post(
|
||||
"/entrypoints/{entrypointID}/edit",
|
||||
s.h.HandleEntrypointEdit(),
|
||||
)
|
||||
r.Post(
|
||||
"/entrypoints/{entrypointID}/delete",
|
||||
s.h.HandleEntrypointDelete(),
|
||||
|
||||
@@ -12,6 +12,7 @@ import (
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/google/uuid"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
"go.uber.org/fx"
|
||||
@@ -398,6 +399,44 @@ func (e *testEnv) seedTarget(
|
||||
return tgt
|
||||
}
|
||||
|
||||
// seedEntrypoint creates an active entrypoint for a webhook.
|
||||
func (e *testEnv) seedEntrypoint(
|
||||
t *testing.T,
|
||||
webhookID string,
|
||||
) *database.Entrypoint {
|
||||
t.Helper()
|
||||
|
||||
ep := &database.Entrypoint{
|
||||
WebhookID: webhookID,
|
||||
Path: uuid.New().String(),
|
||||
Description: "Default entrypoint",
|
||||
Active: true,
|
||||
}
|
||||
|
||||
require.NoError(
|
||||
t,
|
||||
e.db.DB().Omit(clause.Associations).Create(ep).Error,
|
||||
)
|
||||
|
||||
return ep
|
||||
}
|
||||
|
||||
// storedEntrypoint reloads an entrypoint row.
|
||||
func (e *testEnv) storedEntrypoint(
|
||||
t *testing.T,
|
||||
entrypointID string,
|
||||
) database.Entrypoint {
|
||||
t.Helper()
|
||||
|
||||
var ep database.Entrypoint
|
||||
|
||||
require.NoError(
|
||||
t, e.db.DB().First(&ep, "id = ?", entrypointID).Error,
|
||||
)
|
||||
|
||||
return ep
|
||||
}
|
||||
|
||||
// seedFailedDelivery records a terminally failed delivery of an event
|
||||
// to a target in the webhook's own database.
|
||||
func (e *testEnv) seedFailedDelivery(
|
||||
@@ -1087,6 +1126,119 @@ func TestHook_EntrypointActions(t *testing.T) {
|
||||
assert.Zero(t, left, "the delete should remove the entrypoint")
|
||||
}
|
||||
|
||||
// TestHook_EntrypointEdit changes an entrypoint's description with the
|
||||
// edit form on the webhook page, then empties it. The entrypoint keeps
|
||||
// its URL, and with no description it shows as "Entrypoint". Without
|
||||
// the CSRF token, or without a session, the edit is refused.
|
||||
func TestHook_EntrypointEdit(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := newTestEnv(t)
|
||||
|
||||
userID, _ := env.seedUser(t, "epeditor", "somepassword")
|
||||
cookies := env.authCookies(t, userID, "epeditor")
|
||||
wh := env.seedWebhook(t, userID)
|
||||
ep := env.seedEntrypoint(t, wh.ID)
|
||||
page := "/hook/" + wh.ID
|
||||
|
||||
token, cookies := env.csrfFrom(t, page, cookies)
|
||||
action := env.urlFrom(
|
||||
t, page, `action="(/hook/[^/"]+/entrypoints/[^/"]+/edit)"`,
|
||||
cookies,
|
||||
)
|
||||
|
||||
assert.Equal(
|
||||
t, http.StatusForbidden,
|
||||
env.post(action, entrypointEditForm("", "no token"), cookies).Code,
|
||||
"an edit without a CSRF token must be refused",
|
||||
)
|
||||
|
||||
// The request without a session carries a valid CSRF token from
|
||||
// the login page, so only the session check can refuse it.
|
||||
anonToken, anon := env.csrfFrom(t, "/pages/login", nil)
|
||||
refused := env.post(
|
||||
action, entrypointEditForm(anonToken, "no session"), anon,
|
||||
)
|
||||
assert.Equal(t, http.StatusSeeOther, refused.Code)
|
||||
assert.Equal(
|
||||
t, "/pages/login", refused.Header().Get("Location"),
|
||||
"an edit without a session must be refused",
|
||||
)
|
||||
|
||||
assert.Equal(
|
||||
t, ep.Description, env.storedEntrypoint(t, ep.ID).Description,
|
||||
"a refused edit must not change the description",
|
||||
)
|
||||
|
||||
// edit submits the form with description and requires the
|
||||
// redirect back to the webhook page with the notice.
|
||||
edit := func(description string) {
|
||||
t.Helper()
|
||||
|
||||
w := env.post(
|
||||
action, entrypointEditForm(token, description), cookies,
|
||||
)
|
||||
env.requireNotice(t, w, page, "entrypoint-saved",
|
||||
"Entrypoint description saved.", cookies)
|
||||
}
|
||||
|
||||
edit("Billing sender")
|
||||
|
||||
stored := env.storedEntrypoint(t, ep.ID)
|
||||
assert.Equal(t, "Billing sender", stored.Description)
|
||||
assert.Equal(t, ep.Path, stored.Path,
|
||||
"the edit must keep the entrypoint's URL")
|
||||
|
||||
body := env.get(page, cookies).Body.String()
|
||||
assert.Contains(t, body, ">Billing sender</span>")
|
||||
assert.Contains(t, body, "/h/"+ep.Path+"</code>")
|
||||
|
||||
edit("")
|
||||
|
||||
assert.Empty(t, env.storedEntrypoint(t, ep.ID).Description)
|
||||
assert.Contains(t, env.get(page, cookies).Body.String(),
|
||||
">Entrypoint</span>", "no description shows as Entrypoint")
|
||||
}
|
||||
|
||||
// TestHook_EntrypointEdit_OtherUser404s has another logged-in user, with
|
||||
// a CSRF token of their own, try to edit an entrypoint: through the
|
||||
// owner's webhook, and through a webhook of their own. Both are 404s
|
||||
// and the description stays as it was.
|
||||
func TestHook_EntrypointEdit_OtherUser404s(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := newTestEnv(t)
|
||||
|
||||
ownerID, _ := env.seedUser(t, "epowner", "somepassword")
|
||||
wh := env.seedWebhook(t, ownerID)
|
||||
ep := env.seedEntrypoint(t, wh.ID)
|
||||
|
||||
otherID, _ := env.seedUser(t, "epother", "somepassword")
|
||||
other := env.authCookies(t, otherID, "epother")
|
||||
theirs := env.seedWebhook(t, otherID)
|
||||
token, other := env.csrfFrom(t, "/hook/"+theirs.ID, other)
|
||||
|
||||
for _, webhookID := range []string{wh.ID, theirs.ID} {
|
||||
path := "/hook/" + webhookID + "/entrypoints/" + ep.ID + "/edit"
|
||||
|
||||
w := env.post(path, entrypointEditForm(token, "not theirs"), other)
|
||||
assert.Equal(t, http.StatusNotFound, w.Code, path)
|
||||
}
|
||||
|
||||
assert.Equal(
|
||||
t, ep.Description, env.storedEntrypoint(t, ep.ID).Description,
|
||||
"another user must not change the description",
|
||||
)
|
||||
}
|
||||
|
||||
// entrypointEditForm fills in the webhook page's entrypoint edit form.
|
||||
func entrypointEditForm(token, description string) url.Values {
|
||||
return url.Values{
|
||||
"csrf_token": {token},
|
||||
"description": {description},
|
||||
}
|
||||
}
|
||||
|
||||
// TestHook_TargetActions adds a target with the form on the webhook
|
||||
// page, follows its Edit link to the target edit form and submits
|
||||
// it, then deactivates, activates and deletes it, every URL and token
|
||||
|
||||
@@ -39,6 +39,12 @@ const (
|
||||
// refuses to spend, leaving it for the hooks that run after the
|
||||
// server: the delivery engine, the healthcheck, the webhook DB
|
||||
// manager and the database close.
|
||||
//
|
||||
// Its value is not tuned to those hooks, which take about a
|
||||
// millisecond between them. It is what the 5s fx stop timeout in
|
||||
// cmd/webhooker leaves after a full ShutdownTimeout drain, so a
|
||||
// drain that starts on a full budget still gets all of
|
||||
// ShutdownTimeout.
|
||||
TailHookReserve = 2 * time.Second
|
||||
|
||||
// sentryFlushTimeout is the longest wait for Sentry to flush
|
||||
@@ -59,6 +65,16 @@ const (
|
||||
// key off it, and a zero exit would read as a deliberate stop.
|
||||
const StartupFailureExitCode = 1
|
||||
|
||||
// DrainBudget reports how long the HTTP drain may wait for in-flight
|
||||
// requests when remaining is the time left on the fx stop context as
|
||||
// the server's stop hook starts. The hooks before the server can
|
||||
// already have spent part of the budget, so the drain takes its time
|
||||
// out of what they left, never out of TailHookReserve. Zero or less
|
||||
// means no wait at all.
|
||||
func DrainBudget(remaining time.Duration) time.Duration {
|
||||
return min(ShutdownTimeout, remaining-TailHookReserve)
|
||||
}
|
||||
|
||||
// SentryFlushBudget reports how long the Sentry flush may run when
|
||||
// remaining is the time left on the fx stop context after the HTTP
|
||||
// drain. sentry.Flush takes a bare duration and honours no context,
|
||||
@@ -261,10 +277,17 @@ func (s *Server) cleanupForExit() {
|
||||
s.log.Info("cleaning up")
|
||||
}
|
||||
|
||||
// cleanShutdown drains the HTTP server and flushes Sentry inside what
|
||||
// is left of the fx stop budget. A context carrying no deadline — a
|
||||
// caller outside the fx lifecycle — gets the full ShutdownTimeout.
|
||||
func (s *Server) cleanShutdown(ctx context.Context) {
|
||||
ctxShutdown, shutdownCancel := context.WithTimeout(
|
||||
ctx, ShutdownTimeout,
|
||||
)
|
||||
drain := ShutdownTimeout
|
||||
|
||||
if deadline, ok := ctx.Deadline(); ok {
|
||||
drain = DrainBudget(time.Until(deadline))
|
||||
}
|
||||
|
||||
ctxShutdown, shutdownCancel := context.WithTimeout(ctx, drain)
|
||||
defer shutdownCancel()
|
||||
|
||||
err := s.httpServer.Shutdown(ctxShutdown)
|
||||
|
||||
@@ -1,13 +1,148 @@
|
||||
package server_test
|
||||
|
||||
import (
|
||||
"context"
|
||||
"net"
|
||||
"net/http"
|
||||
"testing"
|
||||
"testing/synctest"
|
||||
"time"
|
||||
|
||||
"github.com/stretchr/testify/require"
|
||||
"sneak.berlin/go/webhooker/internal/server"
|
||||
)
|
||||
|
||||
// TestDrainBudget covers the clamp that keeps the HTTP drain from
|
||||
// spending the tail hooks' share of the fx stop budget when the hooks
|
||||
// before the server have already used part of it.
|
||||
func TestDrainBudget(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
remaining time.Duration
|
||||
want time.Duration
|
||||
}{
|
||||
{
|
||||
name: "only the reserve is left",
|
||||
remaining: server.TailHookReserve,
|
||||
want: 0,
|
||||
},
|
||||
{
|
||||
name: "earlier hooks spent part of the budget",
|
||||
remaining: server.TailHookReserve + time.Second,
|
||||
want: time.Second,
|
||||
},
|
||||
{
|
||||
name: "capped at the nominal timeout",
|
||||
remaining: time.Hour,
|
||||
want: server.ShutdownTimeout,
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
require.Equal(t, tt.want, server.DrainBudget(tt.remaining))
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestCleanShutdown_LeavesTailHookReserve stops the server with a
|
||||
// request still in flight, after the hooks before it have spent all
|
||||
// of the stop budget but TailHookReserve. The drain must give up at
|
||||
// once rather than wait for the request: what is left belongs to the
|
||||
// hooks after the server, the database close among them. A drain
|
||||
// bounded only by ShutdownTimeout waits until the stop context
|
||||
// expires, and fx then skips those hooks.
|
||||
//
|
||||
// The test runs in a synctest bubble, whose clock moves only while
|
||||
// every goroutine in it is blocked, so a drain that gives up at once
|
||||
// leaves the stop context unexpired however slow the host is. The
|
||||
// request travels over net.Pipe because a goroutine waiting on a
|
||||
// real socket would stop that clock from moving at all.
|
||||
func TestCleanShutdown_LeavesTailHookReserve(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
synctest.Test(t, func(t *testing.T) {
|
||||
entered := make(chan struct{})
|
||||
release := make(chan struct{})
|
||||
|
||||
hs := &http.Server{
|
||||
Handler: http.HandlerFunc(
|
||||
func(http.ResponseWriter, *http.Request) {
|
||||
close(entered)
|
||||
<-release
|
||||
},
|
||||
),
|
||||
ReadHeaderTimeout: time.Second,
|
||||
}
|
||||
|
||||
srvConn, cliConn := net.Pipe()
|
||||
|
||||
listener := pipeListener{
|
||||
conns: make(chan net.Conn, 1),
|
||||
closed: make(chan struct{}),
|
||||
}
|
||||
listener.conns <- srvConn
|
||||
|
||||
go func() { _ = hs.Serve(listener) }()
|
||||
|
||||
// Cleanups run last first: the handler returns, then closing
|
||||
// the client end ends the server's write of the response.
|
||||
t.Cleanup(func() { _ = cliConn.Close() })
|
||||
t.Cleanup(func() { close(release) })
|
||||
|
||||
_, err := cliConn.Write(
|
||||
[]byte("GET / HTTP/1.1\r\nHost: webhooker.test\r\n\r\n"),
|
||||
)
|
||||
require.NoError(t, err)
|
||||
|
||||
<-entered
|
||||
|
||||
stopCtx, cancel := context.WithTimeout(
|
||||
t.Context(), server.TailHookReserve,
|
||||
)
|
||||
defer cancel()
|
||||
|
||||
server.CleanShutdownForTest(stopCtx, hs)
|
||||
|
||||
require.NoError(
|
||||
t, stopCtx.Err(), "the drain spent the tail hooks' reserve",
|
||||
)
|
||||
})
|
||||
}
|
||||
|
||||
// pipeListener is the net.Listener http.Server.Serve needs to serve
|
||||
// the server end of a net.Pipe: Accept returns that one connection,
|
||||
// then waits until Close, as a real listener with no more clients
|
||||
// does.
|
||||
type pipeListener struct {
|
||||
conns chan net.Conn
|
||||
closed chan struct{}
|
||||
}
|
||||
|
||||
func (l pipeListener) Accept() (net.Conn, error) {
|
||||
select {
|
||||
case conn := <-l.conns:
|
||||
return conn, nil
|
||||
case <-l.closed:
|
||||
return nil, net.ErrClosed
|
||||
}
|
||||
}
|
||||
|
||||
func (l pipeListener) Close() error {
|
||||
close(l.closed)
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
// Addr is never called by http.Server.Serve.
|
||||
func (pipeListener) Addr() net.Addr {
|
||||
return nil
|
||||
}
|
||||
|
||||
// TestSentryFlushBudget covers the clamp that keeps the Sentry flush
|
||||
// from spending the tail hooks' share of the fx stop budget.
|
||||
// sentry.Flush ignores the stop context, so without the clamp a
|
||||
|
||||
+2
-1
@@ -70,7 +70,8 @@ document.addEventListener("alpine:init", function () {
|
||||
"use strict";
|
||||
|
||||
// Something a click shows and hides: the mobile menu, an add form,
|
||||
// an event in the event log, a delivery's attempts.
|
||||
// an entrypoint's edit form, an event in the event log, a delivery's
|
||||
// attempts.
|
||||
window.Alpine.data("collapsible", function () {
|
||||
return {
|
||||
open: false,
|
||||
|
||||
+2
-5
@@ -5,10 +5,7 @@ import (
|
||||
"embed"
|
||||
)
|
||||
|
||||
// Static holds the CSS and JavaScript files the web UI's pages load. They
|
||||
// are named one by one so that a missing js/alpine.min.js, which make
|
||||
// assets extracts and git does not track, fails the build instead of
|
||||
// leaving the pages without Alpine.js.
|
||||
// Static holds the embedded CSS and JavaScript files for the web UI.
|
||||
//
|
||||
//go:embed css/tailwind.css css/style.css js/app.js js/alpine.min.js
|
||||
//go:embed css js
|
||||
var Static embed.FS
|
||||
|
||||
@@ -54,15 +54,26 @@
|
||||
|
||||
<div class="divide-y divide-gray-100">
|
||||
{{range .Entrypoints}}
|
||||
<div class="p-4">
|
||||
<div class="p-4" x-data="collapsible">
|
||||
<div class="flex flex-wrap items-center justify-between gap-2 mb-1">
|
||||
<span class="text-sm font-medium text-gray-900">{{if .Description}}{{.Description}}{{else}}Entrypoint{{end}}</span>
|
||||
<span x-show="closed" class="text-sm font-medium text-gray-900">{{if .Description}}{{.Description}}{{else}}Entrypoint{{end}}</span>
|
||||
<!-- Edit shows this form in place of the
|
||||
description and hides until it closes, so
|
||||
the form always opens on the saved
|
||||
description. Cancel resets what was typed. -->
|
||||
<form x-show="open" x-cloak method="POST" action="/hook/{{$.Webhook.ID}}/entrypoints/{{.ID}}/edit" class="flex w-full gap-2">
|
||||
<input type="hidden" name="csrf_token" value="{{$.CSRFToken}}">
|
||||
<input type="text" name="description" value="{{.Description}}" placeholder="Description (optional)" class="input text-sm flex-1">
|
||||
<button type="submit" class="btn-primary text-sm">Save</button>
|
||||
<button type="reset" @click="toggle" class="btn-secondary text-sm">Cancel</button>
|
||||
</form>
|
||||
<div class="flex flex-wrap items-center gap-2">
|
||||
{{if .Active}}
|
||||
<span class="badge-success">Active</span>
|
||||
{{else}}
|
||||
<span class="badge-error">Inactive</span>
|
||||
{{end}}
|
||||
<button type="button" x-show="closed" @click="toggle" class="btn-small" title="Edit">Edit</button>
|
||||
<form method="POST" action="/hook/{{$.Webhook.ID}}/entrypoints/{{.ID}}/toggle" class="inline">
|
||||
<input type="hidden" name="csrf_token" value="{{$.CSRFToken}}">
|
||||
<button type="submit" class="btn-small" title="{{if .Active}}Deactivate{{else}}Activate{{end}}">
|
||||
|
||||
Reference in New Issue
Block a user