From 8bc549047904320a00cb8aed8c3fdc894a1f9044 Mon Sep 17 00:00:00 2001 From: Bogdan FLOREA Date: Mon, 5 Oct 2026 13:22:58 +0300 Subject: [PATCH 1/3] fix(forms): a failed rename in a library form posts back to the stored name A prefix set, AS set, community, RPKI server, BMP station or policy rename that failed validation re-rendered the form posting to the name the operator had typed. When that name belonged to a sibling (the usual reason a rename is refused), the corrected resubmit edited the sibling: it took the first object's contents and the new name, and the first object stayed as it was. The forms now post back to the name the object is stored under, as the peer and template forms already do. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 10 ++- internal/web/assets.go | 6 +- internal/web/bmp.go | 5 +- internal/web/library.go | 5 +- internal/web/library_communities.go | 5 +- internal/web/library_rename_test.go | 99 +++++++++++++++++++++ internal/web/policies.go | 5 +- internal/web/rpki.go | 5 +- internal/web/templates/as_set_form.html | 2 +- internal/web/templates/bmp_form.html | 2 +- internal/web/templates/community_form.html | 2 +- internal/web/templates/policy_form.html | 2 +- internal/web/templates/prefix_set_form.html | 2 +- internal/web/templates/rpki_form.html | 2 +- 14 files changed, 137 insertions(+), 15 deletions(-) create mode 100644 internal/web/library_rename_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index ead2782..d10562d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,9 +55,13 @@ follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - The peer form's role-specific checkbox group for the *other* role (the iBGP switches on an eBGP peer, and vice versa) was visible, greyed out, instead of hidden: the group's flex layout overrode the `hidden` attribute. -- A peer rename that failed validation re-rendered the form posting to the new - name, so the corrected resubmit hit "not found" and the edit was lost. The form - now posts back to the name the peer is stored under. +- **A rename that failed validation could overwrite another object.** The peer, + prefix set, AS set, community, RPKI server, BMP station and policy forms + re-rendered posting to the name the operator had typed. If that name was free, + the corrected resubmit hit "not found" and the edit was lost; if it was taken — + the usual reason a rename is refused — the resubmit saved over the object that + held it, replacing its contents, and left the one being edited as it was. Every + form now posts back to the name the object is stored under. - **Two objects with one BIRD name are caught before BIRD is.** BIRD keeps defines, filters, functions, templates and protocols in one namespace, so a peer and a prefix set (or community, AS set, RPKI server, BMP station) of the same diff --git a/internal/web/assets.go b/internal/web/assets.go index c6fad2e..1e39b0e 100644 --- a/internal/web/assets.go +++ b/internal/web/assets.go @@ -29,6 +29,9 @@ type asSetFormView struct { Errs map[string]string Preview string PreviewErr string + // StoredName is the name the set is saved under. A form re-rendered after + // a failed rename posts back to it, not to the name the operator typed. + StoredName string } func (s *Server) handleASSetsList(w http.ResponseWriter, r *http.Request) { @@ -146,7 +149,8 @@ func (s *Server) handleASSetSave(w http.ResponseWriter, r *http.Request) { return } s.renderASSetForm(w, asSetFormView{ - Active: "library", ReadOnly: s.readOnly, IsNew: isNew, Set: as, EntryText: entryText, Errs: errs, + Active: "library", ReadOnly: s.readOnly, IsNew: isNew, StoredName: r.PathValue("name"), + Set: as, EntryText: entryText, Errs: errs, }) } diff --git a/internal/web/bmp.go b/internal/web/bmp.go index cfd7563..79801e6 100644 --- a/internal/web/bmp.go +++ b/internal/web/bmp.go @@ -24,6 +24,9 @@ type bmpFormView struct { IsNew bool Station store.BMPStation Errs map[string]string + // StoredName is the name the station is saved under. A form re-rendered + // after a failed rename posts back to it, not to the name the operator typed. + StoredName string } func (s *Server) handleBMPPage(w http.ResponseWriter, r *http.Request) { @@ -110,7 +113,7 @@ func (s *Server) handleBMPSave(w http.ResponseWriter, r *http.Request) { s.flashRedirect(w, r, "/bmp", "Saved "+st.Name, false) return } - render(w, s.log, "bmp_form.html", bmpFormView{Active: "bmp", ReadOnly: s.readOnly, IsNew: isNew, Station: st, Errs: errs}) + render(w, s.log, "bmp_form.html", bmpFormView{Active: "bmp", ReadOnly: s.readOnly, IsNew: isNew, StoredName: r.PathValue("name"), Station: st, Errs: errs}) } func (s *Server) handleBMPDelete(w http.ResponseWriter, r *http.Request) { diff --git a/internal/web/library.go b/internal/web/library.go index b598423..bd204fd 100644 --- a/internal/web/library.go +++ b/internal/web/library.go @@ -26,6 +26,9 @@ type prefixSetFormView struct { Preview string PreviewErr string Bgpq4 bool // whether IRR expansion is enabled + // StoredName is the name the set is saved under. A form re-rendered after + // a failed rename posts back to it, not to the name the operator typed. + StoredName string } func (s *Server) handlePrefixSetsList(w http.ResponseWriter, r *http.Request) { @@ -148,7 +151,7 @@ func (s *Server) handlePrefixSetSave(w http.ResponseWriter, r *http.Request) { s.flashRedirect(w, r, "/library/prefix-sets", "Saved "+ps.Name, false) return } - s.renderPrefixSetForm(w, prefixSetFormView{Active: "library", ReadOnly: s.readOnly, IsNew: isNew, Set: ps, Errs: errs}) + s.renderPrefixSetForm(w, prefixSetFormView{Active: "library", ReadOnly: s.readOnly, IsNew: isNew, StoredName: r.PathValue("name"), Set: ps, Errs: errs}) } func (s *Server) handlePrefixSetDelete(w http.ResponseWriter, r *http.Request) { diff --git a/internal/web/library_communities.go b/internal/web/library_communities.go index 2dfd2ab..63e5055 100644 --- a/internal/web/library_communities.go +++ b/internal/web/library_communities.go @@ -26,6 +26,9 @@ type communityFormView struct { Value string Errs map[string]string Preview string + // StoredName is the name the community is saved under. A form re-rendered + // after a failed rename posts back to it, not to the name the operator typed. + StoredName string } func (s *Server) handleCommunitiesList(w http.ResponseWriter, r *http.Request) { @@ -125,7 +128,7 @@ func (s *Server) handleCommunitySave(w http.ResponseWriter, r *http.Request) { return } s.renderCommunityForm(w, communityFormView{ - Active: "library", ReadOnly: s.readOnly, IsNew: isNew, Def: cd, + Active: "library", ReadOnly: s.readOnly, IsNew: isNew, StoredName: r.PathValue("name"), Def: cd, Value: r.FormValue("value"), Errs: errs, }) } diff --git a/internal/web/library_rename_test.go b/internal/web/library_rename_test.go new file mode 100644 index 0000000..f4daa0e --- /dev/null +++ b/internal/web/library_rename_test.go @@ -0,0 +1,99 @@ +package web + +import ( + "fmt" + "net/http" + "net/url" + "regexp" + "testing" +) + +// editorAction is where a re-rendered library form posts. +var editorAction = regexp.MustCompile(`
]*data-editor`) + +// A rename that fails validation re-renders the form with what the operator +// typed, and the form must still post to the name the object is stored under. +// Posting to the typed name turns the corrected resubmit into an edit of +// whatever already has that name: renaming one prefix set onto a sibling's +// name, then fixing the name, overwrote the sibling with the first set's +// entries and left the first set unrenamed. +func TestFailedLibraryRenamePostsBackToTheStoredName(t *testing.T) { + for _, c := range []struct { + kind, base string + form func(name string, n int) url.Values + id func(env *testEnv, name string) (int64, error) + }{ + {"prefix set", "/library/prefix-sets", func(name string, n int) url.Values { + return url.Values{"name": {name}, "family": {"ipv4"}, "entries": {fmt.Sprintf("198.51.%d.0/24", n)}} + }, func(env *testEnv, name string) (int64, error) { + v, err := env.store.GetPrefixSetByName(name) + return v.ID, err + }}, + {"AS set", "/library/as-sets", func(name string, n int) url.Values { + return url.Values{"name": {name}, "entries": {fmt.Sprint(64600 + n)}} + }, func(env *testEnv, name string) (int64, error) { + v, err := env.store.GetASSetByName(name) + return v.ID, err + }}, + {"community", "/library/communities", func(name string, n int) url.Values { + return url.Values{"name": {name}, "value": {fmt.Sprintf("65000:%d", n)}} + }, func(env *testEnv, name string) (int64, error) { + v, err := env.store.GetCommunityDefByName(name) + return v.ID, err + }}, + {"RPKI server", "/rpki", func(name string, n int) url.Values { + return url.Values{"name": {name}, "host": {fmt.Sprintf("rtr%d.example.net", n)}, "port": {"8282"}, + "refresh": {"900"}, "expire": {"172800"}, "enabled": {"on"}} + }, func(env *testEnv, name string) (int64, error) { + v, err := env.store.GetRPKIServerByName(name) + return v.ID, err + }}, + {"BMP station", "/bmp", func(name string, n int) url.Values { + return url.Values{"name": {name}, "address": {fmt.Sprintf("203.0.113.%d", n)}, "port": {"1790"}, "enabled": {"on"}} + }, func(env *testEnv, name string) (int64, error) { + v, err := env.store.GetBMPStationByName(name) + return v.ID, err + }}, + {"policy", "/policies", func(name string, n int) url.Values { + return url.Values{"name": {name}, "direction": {"import"}, "defaultRoute": {"reject"}, + "bogonAsns": {"off"}, "rov": {"off"}} + }, func(env *testEnv, name string) (int64, error) { + v, err := env.store.GetPolicyByName(name) + return v.ID, err + }}, + } { + t.Run(c.kind, func(t *testing.T) { + env := newTestEnv(t, false) + for i, name := range []string{"CUST_A", "CUST_C"} { + if rec := env.do(t, "POST", c.base+"/new", c.form(name, i+1)); rec.Code != http.StatusSeeOther { + t.Fatalf("create %s: %d %s", name, rec.Code, rec.Body) + } + } + idA, _ := c.id(env, "CUST_A") + idC, _ := c.id(env, "CUST_C") + + rec := env.do(t, "POST", c.base+"/CUST_A/edit", c.form("CUST_C", 1)) + if rec.Code != http.StatusOK { + t.Fatalf("a rename onto a sibling's name should re-render the form: %d", rec.Code) + } + m := editorAction.FindStringSubmatch(rec.Body.String()) + if m == nil { + t.Fatal("the re-rendered page has no editor form") + } + if want := c.base + "/CUST_A/edit"; m[1] != want { + t.Errorf("the re-rendered form posts to %s, want %s", m[1], want) + } + + // The operator corrects the name and resubmits the same form. + if rec := env.do(t, "POST", m[1], c.form("CUST_D", 1)); rec.Code != http.StatusSeeOther { + t.Fatalf("corrected resubmit: %d %s", rec.Code, rec.Body) + } + if id, err := c.id(env, "CUST_D"); err != nil || id != idA { + t.Errorf("CUST_D should be CUST_A renamed (id %d); got id %d, err %v", idA, id, err) + } + if id, err := c.id(env, "CUST_C"); err != nil || id != idC { + t.Errorf("CUST_C should be untouched (id %d); got id %d, err %v", idC, id, err) + } + }) + } +} diff --git a/internal/web/policies.go b/internal/web/policies.go index 5e952e4..f4a0f39 100644 --- a/internal/web/policies.go +++ b/internal/web/policies.go @@ -37,6 +37,9 @@ type policyFormView struct { PreviewErr string // Communities feed the match-community input's autocomplete datalist. Communities []store.CommunityDef + // StoredName is the name the policy is saved under. A form re-rendered + // after a failed rename posts back to it, not to the name the operator typed. + StoredName string } func (s *Server) handlePoliciesList(w http.ResponseWriter, r *http.Request) { @@ -229,7 +232,7 @@ func (s *Server) handlePolicySave(w http.ResponseWriter, r *http.Request) { s.flashRedirect(w, r, "/policies", "Saved "+p.Name, false) return } - s.renderPolicyForm(w, policyFormView{Active: "policies", ReadOnly: s.readOnly, IsNew: isNew, Policy: p, Errs: errs}) + s.renderPolicyForm(w, policyFormView{Active: "policies", ReadOnly: s.readOnly, IsNew: isNew, StoredName: r.PathValue("name"), Policy: p, Errs: errs}) } func (s *Server) handlePolicyDelete(w http.ResponseWriter, r *http.Request) { diff --git a/internal/web/rpki.go b/internal/web/rpki.go index 68cf5a1..90b6b7d 100644 --- a/internal/web/rpki.go +++ b/internal/web/rpki.go @@ -71,6 +71,9 @@ type rpkiFormView struct { IsNew bool Server store.RPKIServer Errs map[string]string + // StoredName is the name the server is saved under. A form re-rendered + // after a failed rename posts back to it, not to the name the operator typed. + StoredName string } func (s *Server) handleRPKIPage(w http.ResponseWriter, r *http.Request) { @@ -234,7 +237,7 @@ func (s *Server) handleRPKISave(w http.ResponseWriter, r *http.Request) { s.flashRedirect(w, r, "/rpki", "Saved "+srv.Name, false) return } - render(w, s.log, "rpki_form.html", rpkiFormView{Active: "rpki", ReadOnly: s.readOnly, IsNew: isNew, Server: srv, Errs: errs}) + render(w, s.log, "rpki_form.html", rpkiFormView{Active: "rpki", ReadOnly: s.readOnly, IsNew: isNew, StoredName: r.PathValue("name"), Server: srv, Errs: errs}) } // lastEnabledServerGuard returns a message when turning this server off would diff --git a/internal/web/templates/as_set_form.html b/internal/web/templates/as_set_form.html index 4ae6abf..b724c93 100644 --- a/internal/web/templates/as_set_form.html +++ b/internal/web/templates/as_set_form.html @@ -21,7 +21,7 @@

{{if .IsNew}}Add AS set{{else}}Edit {{.Set.Name}}{{end}}

- +
diff --git a/internal/web/templates/bmp_form.html b/internal/web/templates/bmp_form.html index 7555e09..0e4ba8d 100644 --- a/internal/web/templates/bmp_form.html +++ b/internal/web/templates/bmp_form.html @@ -21,7 +21,7 @@

{{if .IsNew}}Add BMP station{{else}}Edit {{.Station.Name}}{{end}}

- +
{{template "icon-activity"}}Station
diff --git a/internal/web/templates/community_form.html b/internal/web/templates/community_form.html index d8817c6..5854eb8 100644 --- a/internal/web/templates/community_form.html +++ b/internal/web/templates/community_form.html @@ -15,7 +15,7 @@
Dashboard/Library/{{if .IsNew}}new{{else}}{{.Def.Name}}{{end}}
- +
{{if .Def.Builtin}}

This community ships with birdy. Your edits are kept.

{{end}} diff --git a/internal/web/templates/policy_form.html b/internal/web/templates/policy_form.html index a377137..0943fd2 100644 --- a/internal/web/templates/policy_form.html +++ b/internal/web/templates/policy_form.html @@ -24,7 +24,7 @@

{{if .IsNew}}New {{.Policy.Direction}} policy{{else}}Edit {{.Policy.Name}}{{

- +
diff --git a/internal/web/templates/prefix_set_form.html b/internal/web/templates/prefix_set_form.html index 967764a..9e9996e 100644 --- a/internal/web/templates/prefix_set_form.html +++ b/internal/web/templates/prefix_set_form.html @@ -21,7 +21,7 @@

{{if .IsNew}}Add prefix set{{else}}Edit {{.Set.Name}}{{end}}

- +
{{if .Set.System}} diff --git a/internal/web/templates/rpki_form.html b/internal/web/templates/rpki_form.html index 1715352..46f099e 100644 --- a/internal/web/templates/rpki_form.html +++ b/internal/web/templates/rpki_form.html @@ -21,7 +21,7 @@

{{if .IsNew}}Add RTR server{{else}}Edit {{.Server.Name}}{{end}}

- +
{{template "icon-shield"}}RTR endpoint
From 0ae1feb4f9e0bf52fe7c418f18b9f27412fa9d7d Mon Sep 17 00:00:00 2001 From: Bogdan FLOREA Date: Mon, 5 Oct 2026 13:24:39 +0300 Subject: [PATCH 2/3] test: pin the template-delete hint and the fail-closed community delete Neither fix had a test that fails when it is reverted: the detach hint was only checked where it belongs, and nothing made the community in-use check fail. sabotageDB changes the test database behind the store's back (a trigger that aborts the delete, a renamed table) to reach both paths. Co-Authored-By: Claude Opus 5.5 --- internal/web/communities_handler_test.go | 16 ++++++++++++++++ internal/web/peer_templates_handler_test.go | 15 +++++++++++++++ internal/web/web_test.go | 15 +++++++++++++++ 3 files changed, 46 insertions(+) diff --git a/internal/web/communities_handler_test.go b/internal/web/communities_handler_test.go index aa7f05b..1924264 100644 --- a/internal/web/communities_handler_test.go +++ b/internal/web/communities_handler_test.go @@ -101,6 +101,22 @@ func TestCommunityReferenceGuards(t *testing.T) { } } +// The in-use check is what keeps a delete from leaving an undefined symbol +// behind; when the check cannot run, the delete must not go ahead blind. +func TestCommunityDeleteRefusesWhenTheUseCheckFails(t *testing.T) { + env := newTestEnv(t, false) + if rec := env.do(t, "POST", "/library/communities/new", url.Values{"name": {"CUST_TAG"}, "value": {"65000:7"}}); rec.Code != 303 { + t.Fatalf("create: %d %s", rec.Code, rec.Body.String()) + } + sabotageDB(t, env, `ALTER TABLE peer_templates RENAME TO peer_templates_gone`) + if rec := env.do(t, "POST", "/library/communities/CUST_TAG/delete", nil); rec.Code == 303 { + t.Errorf("the delete went ahead although the in-use check failed: %q", flashOf(rec)) + } + if _, err := env.store.GetCommunityDefByName("CUST_TAG"); err != nil { + t.Errorf("the community should still exist: %v", err) + } +} + // A reserved name and a malformed value are rejected without creating anything. func TestCommunityValidation(t *testing.T) { env := applyReady(t) diff --git a/internal/web/peer_templates_handler_test.go b/internal/web/peer_templates_handler_test.go index a315f3a..2f8b411 100644 --- a/internal/web/peer_templates_handler_test.go +++ b/internal/web/peer_templates_handler_test.go @@ -599,6 +599,21 @@ func TestDeletingALinkedTemplateSaysToDetachFirst(t *testing.T) { } } +// Only a template peers link to is fixed by detaching them; a delete that fails +// for another reason must not send the operator off to detach peers. +func TestAnUnlinkedTemplatesFailedDeleteDoesNotSayDetach(t *testing.T) { + env := newTestEnv(t, false) + createTemplate(env, t) // no peers linked + sabotageDB(t, env, `CREATE TRIGGER keep_templates BEFORE DELETE ON peer_templates BEGIN SELECT RAISE(ABORT, 'disk on fire'); END`) + flash := flashOf(env.do(t, "POST", "/peers/templates/IX_PEERS/delete", nil)) + if !strings.Contains(flash, "disk on fire") { + t.Fatalf("the delete should fail and say why: %q", flash) + } + if strings.Contains(flash, "Detach") { + t.Errorf("an unlinked template's failed delete should not say to detach peers: %q", flash) + } +} + // Every library object that renders a BIRD symbol refuses a template's name at // save, not only at the next render. func TestLibraryObjectsCannotTakeATemplatesName(t *testing.T) { diff --git a/internal/web/web_test.go b/internal/web/web_test.go index cecd4cd..7e2bcd3 100644 --- a/internal/web/web_test.go +++ b/internal/web/web_test.go @@ -2,6 +2,7 @@ package web import ( "context" + "database/sql" "encoding/base64" "io" "log/slog" @@ -246,6 +247,20 @@ func findCookie(cookies []*http.Cookie, name string) *http.Cookie { return nil } +// sabotageDB runs stmt against the test env's database behind the store's back, +// to make a store call fail where no input can. +func sabotageDB(t *testing.T, env *testEnv, stmt string) { + t.Helper() + db, err := sql.Open("sqlite", filepath.Join(filepath.Dir(env.confPath), "birdy.db")+"?_pragma=busy_timeout(5000)") + if err != nil { + t.Fatal(err) + } + defer db.Close() + if _, err := db.Exec(stmt); err != nil { + t.Fatal(err) + } +} + // flashOf decodes the one-shot flash message from a response's birdy_flash cookie // (they used to ride in the redirect URL's ?flash= param). func flashOf(rec *httptest.ResponseRecorder) string { From 6a01af075b27ec74198bd654f390a9582dbcffd3 Mon Sep 17 00:00:00 2001 From: Bogdan FLOREA Date: Mon, 5 Oct 2026 13:25:43 +0300 Subject: [PATCH 3/3] fix(seed): flag a session named like a template instead of offering it Import from BIRD listed such a session as importable, then skipped it on save with only its name in the "skipped" list. The page now runs the same template-name check as the save and shows the row as "can't import" with the reason. Co-Authored-By: Claude Opus 5.5 --- internal/web/seed.go | 7 ++++++- internal/web/seed_handler_test.go | 24 +++++++++++++++++++++--- 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/internal/web/seed.go b/internal/web/seed.go index f3f6fb2..61d2a39 100644 --- a/internal/web/seed.go +++ b/internal/web/seed.go @@ -165,7 +165,12 @@ func (s *Server) discoverSeedRows(ctx context.Context) ([]seedRow, string) { row.Role, row.Multihop, row.Note = p.Role, p.Multihop, note row.V6 = p.IsV6() probe := p - if errs := probe.Validate(); len(errs) > 0 { + errs := probe.Validate() + // The save skips a session named like a template, so say so here + // rather than offer it. + if err := s.refuseTemplateName(p.Name, errs); err != nil { + row.Invalid = "could not check the name: " + err.Error() + } else if len(errs) > 0 { row.Invalid = firstFieldError(errs) } rows = append(rows, row) diff --git a/internal/web/seed_handler_test.go b/internal/web/seed_handler_test.go index 5943659..7dd8ea7 100644 --- a/internal/web/seed_handler_test.go +++ b/internal/web/seed_handler_test.go @@ -84,9 +84,10 @@ func TestSeedDetectsIBGP(t *testing.T) { } } -// A live session named like a template cannot be imported as a peer: the model -// would then declare that name twice. It is skipped, not created. -func TestSeedSkipsASessionNamedLikeATemplate(t *testing.T) { +// templateNamedLikeSession is the fake router's live edge_v4 session plus a +// peer template that already took the name edge_v4. +func templateNamedLikeSession(t *testing.T) *testEnv { + t.Helper() env := applyReady(t) env.fc.details["edge_v4"] = seedDetail("edge_v4", "198.51.100.1", "64500", "65551", "external") tmpl := store.PeerTemplate{Name: "edge_v4", Role: store.RoleUpstream, ImportLimitAction: "restart"} @@ -96,6 +97,23 @@ func TestSeedSkipsASessionNamedLikeATemplate(t *testing.T) { if _, err := env.store.CreatePeerTemplate(tmpl, nil, nil); err != nil { t.Fatal(err) } + return env +} + +// The seed page says up front that such a session cannot be imported, rather +// than offering it and skipping it on save. +func TestSeedPageFlagsASessionNamedLikeATemplate(t *testing.T) { + env := templateNamedLikeSession(t) + body := env.do(t, "GET", "/peers/seed", nil).Body.String() + if !strings.Contains(body, "can't import") || !strings.Contains(body, `peer template "edge_v4" already uses this name`) { + t.Errorf("the seed page should flag a session named like a template, body:\n%s", body) + } +} + +// A live session named like a template cannot be imported as a peer: the model +// would then declare that name twice. It is skipped, not created. +func TestSeedSkipsASessionNamedLikeATemplate(t *testing.T) { + env := templateNamedLikeSession(t) env.do(t, "POST", "/peers/seed", url.Values{"include": {"edge_v4"}, "role_edge_v4": {"upstream"}}) if _, err := env.store.GetPeerByName("edge_v4"); err != store.ErrNotFound {