Skip to content

Commit eff69a7

Browse files
compscidrclaude
andcommitted
Address all review feedback on plugin system
Security: - Analytics: validate tracking ID with regex (alphanumeric + hyphens only) - Social icons: escape URLs and labels with html.EscapeString - Dynamic plugin loading now opt-in via ENABLE_DYNAMIC_PLUGINS env var Concurrency: - Scholar: use sync.Once for ensureScholar instead of bare nil check - Scheduled jobs: copy r.db under RLock before using in goroutine - Registry.Plugins() returns a copy of the internal slice Robustness: - Nil DB guards in getPluginSettings and InjectTemplateData - Scholar OnInit: check for gorm.ErrRecordNotFound specifically - Scholar OnInit: check db.Create error - Dynamic loader: return descriptive error on type assertion failure - Dynamic loader: document package main requirement - Yaegi symbols: fix import path key, export Plugin interface - Plugin registry stored on goblog struct, UpdateDb/Init/StartScheduledJobs called when wizard establishes DB connection Admin UI: - Fix duplicate plugin_footer_html injection in default footer - Theme reload only fires when theme value actually changed - Checkbox reverts on failed plugin toggle save - SettingDefinition comment updated to match plugin_settings table Migration: - Narrow cleanup to known plugin prefixes instead of all dot-namespaced keys - Registry test errors properly checked Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 3eb48a7 commit eff69a7

12 files changed

Lines changed: 112 additions & 45 deletions

File tree

goblog.go

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ type goblog struct {
3939
_blog *blog.Blog
4040
_auth *auth.Auth
4141
_admin *admin.Admin
42+
_registry *gplugin.Registry
4243
sessionKey string
4344
router *gin.Engine
4445
handlersRegistered bool
@@ -164,6 +165,9 @@ func (g goblog) rootHandler(c *gin.Context) {
164165
g._auth.UpdateDb(db)
165166
g._admin.UpdateDb(db)
166167
g._wizard.UpdateDb(db)
168+
g._registry.UpdateDb(db)
169+
g._registry.Init()
170+
g._registry.StartScheduledJobs()
167171

168172
if !isAuthConfigured() {
169173
g._wizard.Landing(c)
@@ -299,26 +303,29 @@ func main() {
299303
}
300304
router.SetTrustedProxies(trustedProxies)
301305

302-
goblog := goblog{
303-
_wizard: &_wizard,
304-
_blog: &_blog,
305-
_auth: &_auth,
306-
_admin: &_admin,
307-
sessionKey: sessionKey,
308-
router: router,
309-
}
310-
311306
// Initialize plugin system
312307
registry := gplugin.NewRegistry(db)
313308
registry.Register(analytics.New())
314309
registry.Register(socialicons.New())
315310
registry.Register(scholarplugin.New())
316-
gplugin.LoadDynamicPlugins(registry, "plugins/dynamic")
311+
if os.Getenv("ENABLE_DYNAMIC_PLUGINS") == "true" {
312+
gplugin.LoadDynamicPlugins(registry, "plugins/dynamic")
313+
}
317314
if db != nil {
318315
registry.Init()
319316
registry.StartScheduledJobs()
320317
}
321318

319+
goblog := goblog{
320+
_wizard: &_wizard,
321+
_blog: &_blog,
322+
_auth: &_auth,
323+
_admin: &_admin,
324+
_registry: registry,
325+
sessionKey: sessionKey,
326+
router: router,
327+
}
328+
322329
// Filter nav pages: hide pages owned by disabled plugins
323330
_blog.PageFilter = func(page blog.Page) bool {
324331
if !registry.HasPageType(page.PageType) {

plugin/loader.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package plugin
22

33
import (
4+
"fmt"
45
"log"
56
"os"
67
"path/filepath"
@@ -11,7 +12,8 @@ import (
1112
)
1213

1314
// LoadDynamicPlugins scans a directory for .go plugin files and loads them
14-
// using the Yaegi Go interpreter. Each file must define a function:
15+
// using the Yaegi Go interpreter. Each file must use `package main` and
16+
// define a function:
1517
//
1618
// func NewPlugin() plugin.Plugin
1719
//
@@ -67,7 +69,7 @@ func loadPlugin(path string) (Plugin, error) {
6769

6870
p, ok := v.Interface().(Plugin)
6971
if !ok {
70-
return nil, err
72+
return nil, fmt.Errorf("%s: NewPlugin() did not return a plugin.Plugin", path)
7173
}
7274

7375
return p, nil

plugin/plugin.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import (
1414
)
1515

1616
// SettingDefinition describes a single setting that a plugin requires.
17-
// Settings are stored in the blog's Setting table namespaced as "pluginname.key".
17+
// Settings are stored in the plugin_settings table keyed by plugin name and setting key.
1818
type SettingDefinition struct {
1919
Key string // short key, e.g. "tracking_id"
2020
Type string // "text", "textarea", "file", "bool"

plugin/registry.go

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,9 @@ func (r *Registry) Register(p Plugin) {
5353
func (r *Registry) Plugins() []Plugin {
5454
r.mu.RLock()
5555
defer r.mu.RUnlock()
56-
return r.plugins
56+
result := make([]Plugin, len(r.plugins))
57+
copy(result, r.plugins)
58+
return result
5759
}
5860

5961
// Init seeds plugin settings and calls OnInit for all plugins.
@@ -94,8 +96,14 @@ func (r *Registry) StartScheduledJobs() {
9496
for {
9597
select {
9698
case <-ticker.C:
99+
r.mu.RLock()
100+
db := r.db
101+
r.mu.RUnlock()
102+
if db == nil {
103+
continue
104+
}
97105
settings := r.getPluginSettings(p.Name())
98-
if err := job.Run(r.db, settings); err != nil {
106+
if err := job.Run(db, settings); err != nil {
99107
log.Printf("Plugin %s job %s error: %v", p.Name(), job.Name, err)
100108
}
101109
case <-r.stopCh:
@@ -114,6 +122,9 @@ func (r *Registry) Stop() {
114122

115123
// getPluginSettings returns a plugin's settings as a simple key→value map.
116124
func (r *Registry) getPluginSettings(pluginName string) map[string]string {
125+
if r.db == nil {
126+
return make(map[string]string)
127+
}
117128
var settings []PluginSetting
118129
r.db.Where("plugin_name = ?", pluginName).Find(&settings)
119130
result := make(map[string]string)
@@ -130,6 +141,10 @@ func (r *Registry) InjectTemplateData(c *gin.Context, templateName string, data
130141
r.mu.RLock()
131142
defer r.mu.RUnlock()
132143

144+
if r.db == nil {
145+
return data
146+
}
147+
133148
pluginsData := gin.H{}
134149
headHTML := ""
135150
footerHTML := ""

plugin/registry_test.go

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,13 @@ func (p *testPlugin) TemplateData(ctx *plugin.HookContext) gin.H {
4141
}
4242

4343
func TestRegistryBasics(t *testing.T) {
44-
db, _ := gorm.Open(sqlite.Open(":memory:"))
45-
db.AutoMigrate(&plugin.PluginSetting{})
44+
db, err := gorm.Open(sqlite.Open(":memory:"))
45+
if err != nil {
46+
t.Fatal(err)
47+
}
48+
if err := db.AutoMigrate(&plugin.PluginSetting{}); err != nil {
49+
t.Fatal(err)
50+
}
4651

4752
reg := plugin.NewRegistry(db)
4853
tp := &testPlugin{}
@@ -57,8 +62,13 @@ func TestRegistryBasics(t *testing.T) {
5762
}
5863

5964
func TestRegistryInit(t *testing.T) {
60-
db, _ := gorm.Open(sqlite.Open(":memory:"))
61-
db.AutoMigrate(&plugin.PluginSetting{})
65+
db, err := gorm.Open(sqlite.Open(":memory:"))
66+
if err != nil {
67+
t.Fatal(err)
68+
}
69+
if err := db.AutoMigrate(&plugin.PluginSetting{}); err != nil {
70+
t.Fatal(err)
71+
}
6272

6373
reg := plugin.NewRegistry(db)
6474
reg.Register(&testPlugin{})
@@ -75,8 +85,13 @@ func TestRegistryInit(t *testing.T) {
7585
}
7686

7787
func TestRegistryInjectTemplateData(t *testing.T) {
78-
db, _ := gorm.Open(sqlite.Open(":memory:"))
79-
db.AutoMigrate(&plugin.PluginSetting{})
88+
db, err := gorm.Open(sqlite.Open(":memory:"))
89+
if err != nil {
90+
t.Fatal(err)
91+
}
92+
if err := db.AutoMigrate(&plugin.PluginSetting{}); err != nil {
93+
t.Fatal(err)
94+
}
8095

8196
reg := plugin.NewRegistry(db)
8297
reg.Register(&testPlugin{})
@@ -119,8 +134,13 @@ func TestRegistryInjectTemplateData(t *testing.T) {
119134
}
120135

121136
func TestGetAllSettings(t *testing.T) {
122-
db, _ := gorm.Open(sqlite.Open(":memory:"))
123-
db.AutoMigrate(&plugin.PluginSetting{})
137+
db, err := gorm.Open(sqlite.Open(":memory:"))
138+
if err != nil {
139+
t.Fatal(err)
140+
}
141+
if err := db.AutoMigrate(&plugin.PluginSetting{}); err != nil {
142+
t.Fatal(err)
143+
}
124144

125145
reg := plugin.NewRegistry(db)
126146
reg.Register(&testPlugin{})

plugin/symbols.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,8 @@ import "reflect"
55
// Symbols exports the plugin package types for the Yaegi interpreter,
66
// allowing dynamic plugins to use plugin.BasePlugin, plugin.HookContext, etc.
77
var Symbols = map[string]map[string]reflect.Value{
8-
"goblog/plugin/plugin": {
8+
"goblog/plugin": {
9+
"Plugin": reflect.ValueOf((*Plugin)(nil)),
910
"BasePlugin": reflect.ValueOf((*BasePlugin)(nil)),
1011
"HookContext": reflect.ValueOf((*HookContext)(nil)),
1112
"SettingDefinition": reflect.ValueOf((*SettingDefinition)(nil)),

plugins/analytics/analytics.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,13 @@ package analytics
55

66
import (
77
"goblog/plugin"
8+
"regexp"
89

910
"gorm.io/gorm"
1011
)
1112

13+
var validTrackingID = regexp.MustCompile(`^[A-Za-z0-9-]+$`)
14+
1215
// AnalyticsPlugin implements Google Analytics tracking.
1316
type AnalyticsPlugin struct {
1417
plugin.BasePlugin
@@ -50,6 +53,9 @@ func (p *AnalyticsPlugin) TemplateHead(ctx *plugin.HookContext) string {
5053
if trackingID == "" || enabled != "true" {
5154
return ""
5255
}
56+
if !validTrackingID.MatchString(trackingID) {
57+
return ""
58+
}
5359
return `<script async src="https://www.googletagmanager.com/gtag/js?id=` + trackingID + `"></script>
5460
<script>
5561
window.dataLayer = window.dataLayer || [];

plugins/scholar/scholar.go

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,12 @@
44
package scholar
55

66
import (
7+
"errors"
78
"fmt"
89
"goblog/blog"
910
"log"
1011
"sort"
12+
"sync"
1113
"time"
1214

1315
gplugin "goblog/plugin"
@@ -20,7 +22,8 @@ import (
2022
// ScholarPlugin displays Google Scholar publications.
2123
type ScholarPlugin struct {
2224
gplugin.BasePlugin
23-
sch *scholarlib.Scholar
25+
sch *scholarlib.Scholar
26+
scholarOnce sync.Once
2427
}
2528

2629
// New creates a new scholar plugin.
@@ -48,6 +51,9 @@ func (p *ScholarPlugin) OnInit(db *gorm.DB) error {
4851
var page blog.Page
4952
result := db.Where("page_type = ?", "research").First(&page)
5053
if result.Error != nil {
54+
if !errors.Is(result.Error, gorm.ErrRecordNotFound) {
55+
return fmt.Errorf("scholar plugin: failed to query research page: %w", result.Error)
56+
}
5157
// No research page exists — create the default
5258
page = blog.Page{
5359
Title: "Research",
@@ -57,7 +63,9 @@ func (p *ScholarPlugin) OnInit(db *gorm.DB) error {
5763
NavOrder: 20,
5864
Enabled: true,
5965
}
60-
db.Create(&page)
66+
if err := db.Create(&page).Error; err != nil {
67+
return fmt.Errorf("scholar plugin: failed to create research page: %w", err)
68+
}
6169
log.Println("Scholar plugin: created research page")
6270
}
6371

@@ -89,7 +97,7 @@ func (p *ScholarPlugin) Pages() []gplugin.PageDefinition {
8997
}
9098

9199
func (p *ScholarPlugin) ensureScholar(settings map[string]string) {
92-
if p.sch == nil {
100+
p.scholarOnce.Do(func() {
93101
profileCache := settings["profile_cache"]
94102
articleCache := settings["article_cache"]
95103
if profileCache == "" {
@@ -99,7 +107,7 @@ func (p *ScholarPlugin) ensureScholar(settings map[string]string) {
99107
articleCache = "articles.json"
100108
}
101109
p.sch = scholarlib.New(profileCache, articleCache)
102-
}
110+
})
103111
}
104112

105113
func (p *ScholarPlugin) RenderPage(ctx *gplugin.HookContext, pageType string) (string, gin.H) {

plugins/socialicons/socialicons.go

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ package socialicons
55

66
import (
77
"goblog/plugin"
8+
"html"
89

910
"github.com/gin-gonic/gin"
1011
"gorm.io/gorm"
@@ -84,14 +85,16 @@ func (p *SocialIconsPlugin) TemplateFooter(ctx *plugin.HookContext) string {
8485
if ctx.Settings["enabled"] != "true" {
8586
return ""
8687
}
87-
html := `<div class="text-center" style="padding: 10px 0;">`
88+
out := `<div class="text-center" style="padding: 10px 0;">`
8889
for _, s := range socials {
8990
url := ctx.Settings[s.key]
9091
if url == "" {
9192
continue
9293
}
93-
html += `<a href="` + url + `" target="_blank" rel="noopener noreferrer" title="` + s.label + `" style="margin: 0 6px; color: inherit;"><i class="` + s.icon + ` fa-1x"></i></a>`
94+
safeURL := html.EscapeString(url)
95+
safeLabel := html.EscapeString(s.label)
96+
out += `<a href="` + safeURL + `" target="_blank" rel="noopener noreferrer" title="` + safeLabel + `" style="margin: 0 6px; color: inherit;"><i class="` + s.icon + ` fa-1x"></i></a>`
9497
}
95-
html += `</div>`
96-
return html
98+
out += `</div>`
99+
return out
97100
}

themes/default/templates/footer.html

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
<div id="footer" class="text-center">
2-
{{ with .plugin_footer_html }}{{ . | rawHTML }}{{ end }}
32
<div class="footer"></div>
43
<div class="spacer version">Powered by <a href="https://github.com/compscidr/goblog" target="goblog {{ .version }}">goblog {{ .version }}</a></div>
54
</div>

0 commit comments

Comments
 (0)