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/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/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/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/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/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 { 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
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 {