From 38cd09f723b68c4aacc30453931bf46ac9f7b781 Mon Sep 17 00:00:00 2001 From: kevin Date: Wed, 19 Aug 2026 12:33:09 +0800 Subject: [PATCH] =?UTF-8?q?fix:=20=E5=AE=89=E5=85=A8=E5=8A=A0=E5=9B=BA?= =?UTF-8?q?=EF=BC=8C=E4=BF=AE=E5=A4=8D=20P0/P1=20=E5=AE=89=E5=85=A8?= =?UTF-8?q?=E6=BC=8F=E6=B4=9E?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P0（高危）: - 新增全局 CSRF 中间件（同步器令牌），覆盖全部 30 个表单与 AJAX 请求 - 修复附件上传/列表/删除越权（IDOR），增加 admin/上传者/文章作者所有权校验 - 登录/注册成功后会话轮换，修复会话固定 - 会话密钥改用 crypto/rand 生成，配置缺失 secret 时拒绝启动 P1（中危）: - session 与 comment_uid cookie 增加 Secure/SameSite 标志 - 新增安全响应头：CSP、X-Content-Type-Options、X-Frame-Options、HSTS 等 - 新增 web.trusted_proxies 配置，修复 X-Forwarded-For 伪造 - 修复浏览量记录 goroutine 访问已回收 gin.Context 的数据竞争 补充 17 个安全回归测试（middleware/handlers），go test -race 全绿 --- SECURITY_TODO.md | 154 ++++++++++ config/config.go | 48 +++- handlers/attachment.go | 59 +++- handlers/auth.go | 26 +- handlers/comment.go | 7 +- handlers/helpers.go | 19 +- handlers/home.go | 52 ++-- handlers/security_test.go | 380 +++++++++++++++++++++++++ main.go | 38 ++- middleware/clientip_test.go | 59 ++++ middleware/csrf.go | 93 ++++++ middleware/csrf_test.go | 154 ++++++++++ middleware/https.go | 23 ++ middleware/security_headers.go | 42 +++ middleware/security_headers_test.go | 102 +++++++ templates/admin/article_create.html | 14 +- templates/admin/article_list.html | 1 + templates/admin/comment_list.html | 3 + templates/admin/dashboard.html | 1 + templates/admin/settings_comment.html | 1 + templates/admin/settings_download.html | 4 + templates/admin/settings_navlinks.html | 4 + templates/admin/settings_site.html | 1 + templates/admin/settings_upload.html | 5 + templates/admin/user_form.html | 1 + templates/admin/user_list.html | 1 + templates/layouts/base.html | 2 + templates/pages/article.html | 1 + templates/pages/login.html | 1 + templates/pages/profile.html | 5 + templates/pages/register.html | 1 + templates/user/my_article_form.html | 1 + templates/user/my_articles.html | 1 + 33 files changed, 1233 insertions(+), 71 deletions(-) create mode 100644 SECURITY_TODO.md create mode 100644 handlers/security_test.go create mode 100644 middleware/clientip_test.go create mode 100644 middleware/csrf.go create mode 100644 middleware/csrf_test.go create mode 100644 middleware/https.go create mode 100644 middleware/security_headers.go create mode 100644 middleware/security_headers_test.go diff --git a/SECURITY_TODO.md b/SECURITY_TODO.md new file mode 100644 index 0000000..3b28ca6 --- /dev/null +++ b/SECURITY_TODO.md @@ -0,0 +1,154 @@ +# 安全修复 TODO + +基于 2026-08-19 的安全审计（源码 + haibara.ai 线上验证）整理。 +按优先级排序，完成后勾选并标注日期。 + +--- + +## P0 — 立即修复 + +### [x] 1. 会话密钥弱回退（可伪造管理员会话）✅ 2026-08-19 +- **位置**: `config/config.go`（`generateSecret` / `applyDefaults` 回退） +- **问题**: secret 缺失时回退为 SHA-256(主机名+PID)，两者均可被外部推测/爆破，攻击者可离线伪造任意用户会话 cookie。 +- **修复**: + - [x] `generateSecret()` 改用 `crypto/rand` 生成 32 字节随机数 + - [x] 已有配置加载路径中 secret 为空时：拒绝启动（`log.Fatalf`），不再静默回退；配置文件读取失败也改为直接退出 + - [x] 首次生成配置文件时写入强随机 secret（保持 `install_linux.sh` 的 openssl 路径不变） +- **验证**: ✅ 新密钥为 crypto/rand 输出；缺失 secret 时启动直接报错 + +### [x] 2. 全站无 CSRF 防护 ✅ 2026-08-19 +- **位置**: 全部 POST 路由（登录/注册/文章/评论/管理后台/设置/附件） +- **问题**: 仅靠 cookie 认证，无 CSRF token；线上 cookie 无 SameSite 属性，浏览器默认 Lax 保护不完整（Chrome Lax+POST 豁免、Safari 差异）。 +- **修复**: + - [x] 新增 `middleware/csrf.go`：同步器令牌模式（session 存储、常量时间比较），表单 `_csrf` 字段或 `X-CSRF-Token` 头二选一，不匹配返回 403 + - [x] 覆盖全部 30 个 POST 表单（含游客评论表单）；AJAX（附件上传/删除、头像上传）经 `` 下发 token 并以请求头携带 + - [x] `/article/:slug/comments` 游客 POST 一并覆盖（游客同样有 session） +- **验证**: ✅ `middleware/csrf_test.go` 7 用例 + 端到端 curl 冒烟（无 token/伪造 token 403，有效 token 302） + +### [x] 3. 附件接口越权（IDOR）✅ 2026-08-19 +- **位置**: `handlers/attachment.go`（DeleteAttachment / ListAttachments / UploadAttachment 的 article_id） +- **问题**: `/my/articles/attachments/*` 仅要求登录，无所有权校验；任意登录用户可删除/列出全站任意附件、向他人文章挂附件。 +- **修复**: + - [x] `DeleteAttachment`：admin / 上传者 / 所属文章作者三者之一，否则 403 + - [x] `ListAttachments`：文章作者或 admin，否则 403 + - [x] `UploadAttachment`：`article_id != 0` 时校验文章归属（admin 除外），否则 403 + - [x] 单元测试（`handlers/security_test.go`：越权 403 / 本人 200 / admin 覆盖） +- **验证**: ✅ 普通用户 A 删除用户 B 的附件 -> 403（测试覆盖） + +### [x] 4. 会话固定（Session Fixation）✅ 2026-08-19 +- **位置**: `handlers/auth.go`（Login / Register 自动登录） +- **问题**: 登录成功后未清空旧 session，直接写入 user_id，固定攻击可劫持登录后会话。 +- **修复**: + - [x] 认证成功后先 `session.Clear()` 再写入 `user_id`/`username` 并 Save；保留 lang 与 csrf_token（避免多标签页已渲染表单失效） +- **验证**: ✅ 登录前后 cookie 值不同，旧 cookie 无法访问受保护路由（`TestLoginRotatesSession`） + +--- + +## P1 — 近期修复 + +### [x] 5. Cookie 缺 Secure / SameSite 标志 ✅ 2026-08-19 +- **位置**: `main.go`（session store）、`handlers/comment.go:82`（comment_uid） +- **修复**: + - [x] store 默认 `SameSite: Lax`；`Secure` 按请求动态设置（`middleware/https.go` 检测 TLS 或 X-Forwarded-Proto），通过中间件在每次请求时应用到 session cookie + - [x] `comment_uid` 游客 cookie 同步补齐 `SameSite=Lax` + HTTPS 下 `Secure` +- **验证**: ✅ 模拟 HTTPS 请求响应头 `Set-Cookie: ... HttpOnly; Secure; SameSite=Lax`；冒烟测试通过 + +### [x] 6. 缺失安全响应头 ✅ 2026-08-19 +- **位置**: 新增 `middleware/security_headers.go`（全局第一个注册） +- **修复**: + - [x] `Content-Security-Policy`（default-src 'self' + 现有 CDN 白名单 + frame-ancestors 'none' 等） + - [x] `X-Content-Type-Options: nosniff`、`X-Frame-Options: DENY`、`Referrer-Policy`、`Permissions-Policy` + - [x] `Strict-Transport-Security`（仅 HTTPS 请求下发，未加 includeSubDomains 以免影响 HTTP 子域） +- **说明**: CSP 含 `'unsafe-inline'`（模板内联 script/style 必需）；待 P2-9 CDN 本地化后可进一步收紧 +- **验证**: ✅ `middleware/security_headers_test.go`（headers 存在性、HSTS 条件下发）+ 冒烟 curl 确认 + +### [x] 7. X-Forwarded-For 伪造（IP 审计/浏览量可刷）✅ 2026-08-19 +- **位置**: `handlers/helpers.go`（GetClientIP）、`config/config.go`（WebConfig.TrustedProxies）、`main.go` +- **修复**: + - [x] 删除手动解析 XFF 首值逻辑，`GetClientIP` 改为 `c.ClientIP()` + - [x] `router.SetTrustedProxies(cfg.Web.TrustedProxies)`；新增 `web.trusted_proxies` 配置项（默认 `["127.0.0.1", "::1"]`，unix socket 部署自动信任） + - [x] gin 内部 XFF 从右往左取第一个不可信 IP：直接客户端伪造的 XFF 被忽略 +- **验证**: ✅ `middleware/clientip_test.go`（直接连接带假 XFF 取真实 IP / 代理链取最右不可信条目） + +### [x] 8. goroutine 数据竞争（use-after-return）✅ 2026-08-19 +- **位置**: `handlers/home.go`（ArticleDetail → recordArticleView） +- **修复**: + - [x] goroutine 启动前同步提取 userID / ip / UA 为局部变量，`recordArticleView` 不再触碰 gin.Context 与 session +- **验证**: ✅ `go test -race ./...` 全绿 + +--- + +## P2 — 计划修复 + +### [ ] 9. 第三方 CDN 无 SRI / Tailwind dev CDN +- **位置**: `templates/layouts/base.html:16-20`、`:118-122` +- **修复**: + - [ ] 将 marked / DOMPurify / highlight.js / cropperjs / easymde 下载到 `static/vendor/`，走 go:embed 本地分发（静态管线已具备） + - [ ] 替换 `cdn.tailwindcss.com` 为构建期生成的静态 CSS（或至少加 SRI） + - [ ] 本地化后配合 #6 收紧 CSP 为 `default-src 'self'` +- **验证**: 断网第三方域名后页面渲染功能完整；CSP 无违规报告 + +### [ ] 10. 登录无速率限制 +- **位置**: `handlers/auth.go:33` +- **修复**: + - [ ] 按 IP + 用户名维度做失败计数（内存或 DB），如 5 次失败锁定 15 分钟 + - [ ] 失败提示保持统一（现有 `?error=1` 已做用户名枚举防护，保持） +- **验证**: 连续错误登录后返回锁定提示 + +### [ ] 11. 配置文件权限过宽 +- **位置**: `config/config.go:115` +- **修复**: `os.WriteFile(configFile, data, 0640)`；secret 写入后可选 `os.Chmod` + +### [ ] 12. 首启弱凭据 admin/admin +- **位置**: `models/db.go:58-81` +- **修复**: + - [ ] 方案 A：首启生成随机密码打印一次性提示 + - [ ] 方案 B：admin 账户标记"必须改密"，登录后强制跳转改密页 +- **说明**: 线上已改密（已验证），此项为防御新部署 + +### [ ] 13. Unix socket 权限 666 +- **位置**: `install_linux.sh:80` +- **修复**: `chmod 660` + `chown root:blog_go`（反向代理进程加入同组），避免本机任意用户绕过 Cloudflare 直连 + +--- + +## P3 — 低优先级 / 观察项 + +### [ ] 14. 上传不校验文件真实类型 +- **位置**: `handlers/upload_validator.go:33-58` +- **修复**: 用 `github.com/gabriel-vasile/mimetype`（已在依赖树中）校验 magic bytes 与扩展名/MIME 一致；不一致则拒绝 +- **说明**: 白名单无 .svg/.html，存储型 XSS 风险低，主要是恶意文件托管风险 + +### [ ] 15. Gravatar MD5 邮箱哈希可反查 +- **位置**: `handlers/comment.go:88-91` +- **说明**: Gravatar 协议本身如此；若在意隐私可加后台开关（已有 UseGravatar 开关可关闭） + +### [ ] 16. RSS 以 Host 头构造 baseURL +- **位置**: `handlers/rss.go:60-64` +- **修复**: 从站点设置中读取固定站点 URL，仅在与请求 Host 不符时告警 +- **说明**: Cloudflare 会校验 Host，实际可利用性低 + +### [ ] 17. bcrypt cost 偏低 +- **位置**: `models/user.go:41`（DefaultCost=10） +- **修复**: 提升到 12；已有哈希在用户下次改密时自然升级 + +--- + +## 不需要修复（已确认安全） + +- SQL 注入：全参数化查询（GORM） +- XSS：html/template 自动转义 + 评论双防御（服务端 strip + DOMPurify） +- 密码哈希：bcrypt +- 附件路径穿越：SHA-256 内容寻址文件名 +- 线上默认凭据：已修改（已验证） +- 注册接口：已关闭（已验证） + +--- + +## 建议执行顺序 + +1. **#1 → #4 → #5**（一次提交：会话安全三件套，改动小、风险低） +2. **#3**（附件越权，纯 handler 层校验） +3. **#2**（CSRF，涉及全站表单，改动面最大，单独一个 PR 充分回归） +4. **#7 → #8 → #6**（IP/竞态/响应头） +5. P2/P3 按迭代排入 diff --git a/config/config.go b/config/config.go index 064d4b7..bd40507 100644 --- a/config/config.go +++ b/config/config.go @@ -1,8 +1,8 @@ package config import ( - "crypto/sha256" - "fmt" + "crypto/rand" + "encoding/hex" "log" "os" "path/filepath" @@ -27,10 +27,19 @@ type DatabaseConfig struct { // WebConfig holds web-server listening configuration. type WebConfig struct { - Port string `yaml:"port"` // TCP port, "" or "0" to disable - Socket string `yaml:"socket"` // Unix socket path, "" to disable + Port string `yaml:"port"` // TCP port, "" or "0" to disable + Socket string `yaml:"socket"` // Unix socket path, "" to disable + // TrustedProxies lists proxy IPs/CIDRs whose X-Forwarded-For / + // X-Forwarded-Proto headers are trusted (e.g. the Caddy/nginx box in + // front of the app). Defaults to loopback. If the app is exposed + // directly to clients, leave the default so client-supplied + // X-Forwarded-For cannot spoof the logged IP. + TrustedProxies []string `yaml:"trusted_proxies"` } +// defaultTrustedProxies is used when the config omits trusted_proxies. +var defaultTrustedProxies = []string{"127.0.0.1", "::1"} + const defaultPort = "8080" // mysqlExampleDSN is written into new config files as a reference. @@ -62,12 +71,15 @@ func getDefaultStoragePath() string { } } -// generateSecret returns a random-ish hex string for the session secret. +// generateSecret returns a cryptographically random hex string for the +// session secret. A failure of crypto/rand is unrecoverable, so the program +// terminates instead of falling back to a predictable value. func generateSecret() string { - hostname, _ := os.Hostname() - input := fmt.Sprintf("%s-%d", hostname, os.Getpid()) - hash := sha256.Sum256([]byte(input)) - return fmt.Sprintf("%x", hash) + b := make([]byte, 32) + if _, err := rand.Read(b); err != nil { + log.Fatalf("Failed to generate session secret: %v", err) + } + return hex.EncodeToString(b) } // getDefaultSocketPath returns the OS-aware default unix socket path. @@ -122,9 +134,7 @@ func LoadConfig(customPath string) *Config { // Read existing config file. data, err := os.ReadFile(configFile) if err != nil { - log.Printf("Warning: could not read config file %s: %v, using defaults", configFile, err) - cfg := &Config{} - return applyDefaults(cfg, defaultPath) + log.Fatalf("Failed to read config file %s: %v", configFile, err) } cfg := &Config{} @@ -132,16 +142,19 @@ func LoadConfig(customPath string) *Config { log.Printf("Warning: malformed config file %s: %v, using defaults", configFile, err) } - return applyDefaults(cfg, defaultPath) + return applyDefaults(cfg, defaultPath, configFile) } // applyDefaults fills zero-value fields with sensible defaults. -func applyDefaults(cfg *Config, defaultPath string) *Config { +func applyDefaults(cfg *Config, defaultPath, configFile string) *Config { // If the entire web block is empty (old config without "web" key), // fill default port so the app still starts on 8080. if cfg.Web.Port == "" && cfg.Web.Socket == "" { cfg.Web.Port = defaultPort } + if len(cfg.Web.TrustedProxies) == 0 { + cfg.Web.TrustedProxies = defaultTrustedProxies + } if cfg.Database.Type == "" { cfg.Database.Type = "sqlite" } @@ -149,7 +162,12 @@ func applyDefaults(cfg *Config, defaultPath string) *Config { cfg.Path = defaultPath } if cfg.Secret == "" { - cfg.Secret = generateSecret() + // The config file exists but has no secret. Refuse to start: a + // silently generated fallback would either be predictable (old + // hostname+pid scheme) or invalidate all sessions on every restart. + log.Fatalf("Config file %s is missing a session secret. "+ + "Add a random value, e.g. `secret: %s`, and restart.", + configFile, generateSecret()) } return cfg } diff --git a/handlers/attachment.go b/handlers/attachment.go index f97f482..abe778c 100644 --- a/handlers/attachment.go +++ b/handlers/attachment.go @@ -49,6 +49,32 @@ func attachmentURL(stored string) string { // ---------------- Upload ---------------- +// currentUserIsAdmin reports whether the authenticated user has the admin +// role, based on the context populated by the SetUserContext middleware. +func currentUserIsAdmin(c *gin.Context) bool { + role, _ := c.Get("role") + r, _ := role.(string) + return r == models.RoleAdmin +} + +// canManageArticle reports whether the current user may attach files to (or +// manage attachments of) the given article: admins always, the article +// author otherwise. +func canManageArticle(c *gin.Context, db *gorm.DB, articleID uint) bool { + if currentUserIsAdmin(c) { + return true + } + uid, ok := sessionAuthorID(c) + if !ok || articleID == 0 { + return false + } + var article models.Article + if err := db.First(&article, "id = ?", articleID).Error; err != nil { + return false + } + return article.AuthorID == uid +} + // UploadAttachment handles AJAX attachment uploads from the article create/edit // form. The request carries either a real article_id (edit page) or a // session_token (create page, pending binding). Files are content-addressed by @@ -69,6 +95,12 @@ func UploadAttachment(db *gorm.DB, storagePath string) gin.HandlerFunc { return } + // Ownership check: a non-admin may only attach to their own articles. + if articleID != 0 && !canManageArticle(c, db, articleID) { + c.JSON(http.StatusForbidden, gin.H{"error": "forbidden"}) + return + } + file, header, err := c.Request.FormFile("file") if err != nil { c.JSON(http.StatusBadRequest, gin.H{"error": "no file provided"}) @@ -146,7 +178,8 @@ func UploadAttachment(db *gorm.DB, storagePath string) gin.HandlerFunc { // DeleteAttachment soft-deletes an attachment record and removes the on-disk // file only when no remaining records reference it (reference counting, since -// content-addressed files may be shared). +// content-addressed files may be shared). Only admins, the uploader, or the +// author of the article the file is attached to may delete it. func DeleteAttachment(db *gorm.DB, storagePath string) gin.HandlerFunc { return func(c *gin.Context) { id := parseUintParam(c, "id") @@ -155,6 +188,19 @@ func DeleteAttachment(db *gorm.DB, storagePath string) gin.HandlerFunc { c.JSON(http.StatusNotFound, gin.H{"error": "not found"}) return } + + // Ownership check: admin, uploader, or the owning article's author. + if !currentUserIsAdmin(c) { + uid, ok := sessionAuthorID(c) + owned := ok && att.UploaderID == uid + if !owned && att.ArticleID != 0 { + owned = canManageArticle(c, db, att.ArticleID) + } + if !owned { + c.JSON(http.StatusForbidden, gin.H{"error": "forbidden"}) + return + } + } stored := att.StoredName if err := db.Delete(&att).Error; err != nil { @@ -175,10 +221,19 @@ func DeleteAttachment(db *gorm.DB, storagePath string) gin.HandlerFunc { // ---------------- List ---------------- // ListAttachments returns the attachments for an article as JSON (used by the -// edit page to repopulate the list on load). +// edit page to repopulate the list on load). Only the article's author (or an +// admin) may list them. func ListAttachments(db *gorm.DB) gin.HandlerFunc { return func(c *gin.Context) { articleID := parseUintParam(c, "id") + if articleID == 0 { + c.JSON(http.StatusBadRequest, gin.H{"error": "invalid article id"}) + return + } + if !canManageArticle(c, db, articleID) { + c.JSON(http.StatusForbidden, gin.H{"error": "forbidden"}) + return + } var atts []models.Attachment db.Where("article_id = ?", articleID).Order("created_at ASC").Find(&atts) diff --git a/handlers/auth.go b/handlers/auth.go index da80688..c6c8258 100644 --- a/handlers/auth.go +++ b/handlers/auth.go @@ -52,8 +52,20 @@ func Login(db *gorm.DB) gin.HandlerFunc { return } - // Create session. + // Rotate the session on privilege change to prevent session + // fixation: drop all pre-authentication state, keep only the + // harmless UI preferences (language and CSRF token so forms + // already rendered in other tabs stay valid). session := sessions.Default(c) + lang, _ := session.Get("lang").(string) + csrfTok, _ := session.Get("csrf_token").(string) + session.Clear() + if lang != "" { + session.Set("lang", lang) + } + if csrfTok != "" { + session.Set("csrf_token", csrfTok) + } session.Set("user_id", user.ID) session.Set("username", user.Username) if err := session.Save(); err != nil { @@ -169,8 +181,18 @@ func Register(db *gorm.DB) gin.HandlerFunc { return } - // Auto-login after successful registration + // Auto-login after successful registration (with session + // rotation, mirroring the login handler). session := sessions.Default(c) + lang, _ := session.Get("lang").(string) + csrfTok, _ := session.Get("csrf_token").(string) + session.Clear() + if lang != "" { + session.Set("lang", lang) + } + if csrfTok != "" { + session.Set("csrf_token", csrfTok) + } session.Set("user_id", user.ID) session.Set("username", user.Username) if err := session.Save(); err != nil { diff --git a/handlers/comment.go b/handlers/comment.go index 3ffdf02..b2322bf 100644 --- a/handlers/comment.go +++ b/handlers/comment.go @@ -16,6 +16,7 @@ import ( "github.com/gin-gonic/gin" "gorm.io/gorm" + "go_blog/middleware" "go_blog/models" ) @@ -78,8 +79,10 @@ func guestTokenFrom(c *gin.Context) string { token = newGuestToken() } // (Re)set the cookie so returning visitors keep their identity. HttpOnly - // prevents JS access; SameSite=Lax is the gin default and is appropriate. - c.SetCookie(guestCookieName, token, guestCookieMaxAge, "/", "", false, true) + // prevents JS access; SameSite=Lax plus Secure-over-HTTPS mirror the + // session cookie hardening. + c.SetSameSite(http.SameSiteLaxMode) + c.SetCookie(guestCookieName, token, guestCookieMaxAge, "/", "", middleware.IsHTTPSRequest(c), true) return token } diff --git a/handlers/helpers.go b/handlers/helpers.go index ed2c01d..66d18b3 100644 --- a/handlers/helpers.go +++ b/handlers/helpers.go @@ -1,8 +1,6 @@ package handlers import ( - "strings" - "github.com/gin-gonic/gin" ) @@ -29,6 +27,7 @@ func DefaultData(c *gin.Context) gin.H { siteHomeSubtitle, _ := c.Get("site_home_subtitle") siteFooterText, _ := c.Get("site_footer_text") navLinks, _ := c.Get("nav_links") + csrfToken, _ := c.Get("csrf_token") return gin.H{ "Tr": tr, @@ -50,6 +49,7 @@ func DefaultData(c *gin.Context) gin.H { "SiteHomeSubtitle": siteHomeSubtitle, "SiteFooterText": siteFooterText, "NavLinks": navLinks, + "CSRFToken": csrfToken, } } @@ -67,17 +67,10 @@ func getTr(c *gin.Context) map[string]string { return m } -// GetClientIP extracts the real client IP address, accounting for CDN/reverse proxy setups. -// It checks X-Forwarded-For and X-Real-IP headers before falling back to c.ClientIP(). +// GetClientIP returns the real client IP. It relies on gin's proxy-aware +// ClientIP(), which honors the trusted_proxies config: only IPs in that list +// are allowed to influence X-Forwarded-For, so direct clients cannot spoof +// the value. func GetClientIP(c *gin.Context) string { - if xff := c.GetHeader("X-Forwarded-For"); xff != "" { - if i := strings.IndexByte(xff, ','); i != -1 { - return strings.TrimSpace(xff[:i]) - } - return strings.TrimSpace(xff) - } - if xri := c.GetHeader("X-Real-IP"); xri != "" { - return strings.TrimSpace(xri) - } return c.ClientIP() } diff --git a/handlers/home.go b/handlers/home.go index 4253841..e61e5f3 100644 --- a/handlers/home.go +++ b/handlers/home.go @@ -198,8 +198,18 @@ func ArticleDetail(db *gorm.DB) gin.HandlerFunc { db.Model(&models.Article{}).Where("id = ?", article.ID). UpdateColumn("view_count", gorm.Expr("view_count + 1")) - // Record unique article view asynchronously (doesn't block page response). - go recordArticleView(db, article.ID, c) + // Record unique article view asynchronously (doesn't block page + // response). All request-derived values are extracted synchronously: + // the gin context is pool-reused and must never be touched from + // another goroutine after the handler returns. + uid := userIDFromSession(c) + var userID *uint + if uid != 0 { + userID = &uid + } + ip := GetClientIP(c) + ua := c.Request.UserAgent() + go recordArticleView(db, article.ID, userID, ip, ua) // One-time flash notice (set by PostComment on success/pending). Reading // consumes the flash, so refreshing the page no longer re-shows it. @@ -281,35 +291,13 @@ func readCommentFlash(c *gin.Context) string { } // recordArticleView records a unique article view in the database. -// This function is designed to be called asynchronously (via goroutine) to avoid -// blocking the page response. It checks for existing records to ensure each -// user/IP combination only records one view per article. -func recordArticleView(db *gorm.DB, articleID uint, c *gin.Context) { - session := sessions.Default(c) - - // Extract user ID from session if logged in - var userID *uint - if uid := session.Get("user_id"); uid != nil { - switch v := uid.(type) { - case uint: - userID = &v - case int: - u := uint(v) - userID = &u - case int64: - u := uint(v) - userID = &u - case float64: - u := uint(v) - userID = &u - } - } - - // Get client IP and User-Agent - ip := GetClientIP(c) - userAgent := c.Request.UserAgent() - isBot := models.IsBot(userAgent) - +// This function is designed to be called asynchronously (via goroutine) to +// avoid blocking the page response. It checks for existing records to ensure +// each user/IP combination only records one view per article. All request +// derived values (userID, ip, userAgent) must be extracted by the caller +// before the goroutine is spawned - this function never touches the gin +// context. +func recordArticleView(db *gorm.DB, articleID uint, userID *uint, ip, userAgent string) { // Check if this view already exists (deduplication) var count int64 query := db.Model(&models.ArticleView{}). @@ -337,7 +325,7 @@ func recordArticleView(db *gorm.DB, articleID uint, c *gin.Context) { UserID: userID, IP: ip, UserAgent: userAgent, - IsBot: isBot, + IsBot: models.IsBot(userAgent), } // Create the view record (BeforeCreate hook in model handles deduplication) diff --git a/handlers/security_test.go b/handlers/security_test.go new file mode 100644 index 0000000..492c5d8 --- /dev/null +++ b/handlers/security_test.go @@ -0,0 +1,380 @@ +package handlers + +import ( + "fmt" + "io" + "mime/multipart" + "net/http" + "net/http/httptest" + "net/url" + "os" + "path/filepath" + "regexp" + "strings" + "testing" + + "github.com/gin-contrib/sessions" + "github.com/gin-contrib/sessions/cookie" + "github.com/gin-gonic/gin" + "github.com/glebarez/sqlite" + "gorm.io/gorm" + + "go_blog/middleware" + "go_blog/models" +) + +// securityTestEnv wires a router that mirrors the production middleware chain +// (sessions -> CSRF -> user context) plus the routes under test. +type securityTestEnv struct { + router *gin.Engine + db *gorm.DB + storageDir string +} + +var csrfTokenRe = regexp.MustCompile(`name="_csrf" value="([^"]+)"`) + +func newSecurityTestEnv(t *testing.T) *securityTestEnv { + t.Helper() + gin.SetMode(gin.TestMode) + + db, err := gorm.Open(sqlite.Open(filepath.Join(t.TempDir(), "test.db")), &gorm.Config{}) + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + if err := db.AutoMigrate(&models.User{}, &models.Article{}, &models.Attachment{}, &models.SiteSetting{}, + &models.UploadConfig{}, &models.UploadFileType{}, &models.CommentConfig{}, &models.NavLink{}, + &models.DownloadBaseURL{}); err != nil { + t.Fatalf("migrate: %v", err) + } + + storageDir := t.TempDir() + + // Seed the upload policy so ValidateUpload accepts .txt files. + db.Create(&models.SiteSetting{ID: 1}) + db.Create(&models.CommentConfig{ID: 1, Enabled: true, AllowGuest: true}) + db.Create(&models.UploadConfig{ID: 1, Enabled: true, DefaultMaxSize: 1024 * 1024, StorageDir: "attachments"}) + db.Create(&models.UploadFileType{Extension: ".txt", MimeType: "text/plain", Category: models.CategoryDocument, Enabled: true}) + models.LoadConfigCache(db) + + // Seed users. + mustUser(t, db, "admin", models.RoleAdmin) + alice := mustUser(t, db, "alice", models.RoleAuthor) + bob := mustUser(t, db, "bob", models.RoleAuthor) + + // Seed one article per author. + aliceArt := models.Article{AuthorID: alice.ID, Title: "alice post", Slug: "alice-post", Content: "x", Status: models.ArticlePublished} + bobArt := models.Article{AuthorID: bob.ID, Title: "bob post", Slug: "bob-post", Content: "x", Status: models.ArticlePublished} + db.Create(&aliceArt) + db.Create(&bobArt) + + r := gin.New() + if err := r.SetTrustedProxies(nil); err != nil { + t.Fatalf("set trusted proxies: %v", err) + } + r.LoadHTMLGlob("../templates/**/*.html") + store := cookie.NewStore([]byte("test-secret")) + r.Use(sessions.Sessions("blog_session", store)) + r.Use(middleware.CSRFProtect()) + r.Use(middleware.SetUserContext(db)) + + r.GET("/login", LoginPage()) + r.POST("/login", Login(db)) + r.POST("/logout", Logout()) + + protected := r.Group("/my", middleware.AuthRequired()) + { + protected.POST("/articles/attachments", UploadAttachment(db, storageDir)) + protected.POST("/articles/attachments/:id/delete", DeleteAttachment(db, storageDir)) + protected.GET("/articles/:id/attachments", ListAttachments(db)) + protected.GET("/whoami", func(c *gin.Context) { + uid, _ := sessionAuthorID(c) + c.String(http.StatusOK, "uid=%d", uid) + }) + } + + return &securityTestEnv{router: r, db: db, storageDir: storageDir} +} + +func mustUser(t *testing.T, db *gorm.DB, username, role string) models.User { + t.Helper() + u := models.User{Username: username, DisplayName: username, Role: role, Status: models.StatusNormal} + if err := u.SetPassword("pw-" + username); err != nil { + t.Fatalf("set password: %v", err) + } + if err := db.Create(&u).Error; err != nil { + t.Fatalf("create user %s: %v", username, err) + } + return u +} + +// login performs the full login flow (GET the form for a CSRF token, then POST +// credentials) and returns the authenticated session cookie. +func (e *securityTestEnv) login(t *testing.T, username string) string { + t.Helper() + + // Anonymous GET to obtain CSRF token + session cookie. + req := httptest.NewRequest(http.MethodGet, "/login", nil) + w := httptest.NewRecorder() + e.router.ServeHTTP(w, req) + if w.Code != http.StatusOK { + t.Fatalf("GET /login: status = %d", w.Code) + } + m := csrfTokenRe.FindStringSubmatch(w.Body.String()) + if m == nil { + t.Fatal("login page did not render a CSRF token") + } + cookie := e.sessionCookie(w) + + // POST credentials with the token. + form := url.Values{} + form.Set("username", username) + form.Set("password", "pw-"+username) + form.Set("_csrf", m[1]) + req = httptest.NewRequest(http.MethodPost, "/login", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + if cookie != "" { + req.Header.Set("Cookie", cookie) + } + w = httptest.NewRecorder() + e.router.ServeHTTP(w, req) + if w.Code != http.StatusFound { + t.Fatalf("POST /login (%s): status = %d, body %s", username, w.Code, w.Body.String()) + } + authCookie := e.sessionCookie(w) + if authCookie == "" { + t.Fatal("login did not set a session cookie") + } + return authCookie +} + +// sessionCookie extracts the blog_session cookie from a recorder. When +// several Set-Cookie headers are present (e.g. middleware and handler both +// save the session), the LAST one is the effective value - browsers apply +// them in order. +func (e *securityTestEnv) sessionCookie(w *httptest.ResponseRecorder) string { + cookie := "" + for _, c := range w.Result().Cookies() { + if c.Name == "blog_session" { + cookie = c.Name + "=" + c.Value + } + } + return cookie +} + +func (e *securityTestEnv) do(method, path, cookie string, body io.Reader, contentType string) *httptest.ResponseRecorder { + req := httptest.NewRequest(method, path, body) + if contentType != "" { + req.Header.Set("Content-Type", contentType) + } + if cookie != "" { + req.Header.Set("Cookie", cookie) + } + w := httptest.NewRecorder() + e.router.ServeHTTP(w, req) + return w +} + +func (e *securityTestEnv) upload(t *testing.T, cookie, csrfToken, articleID string) *httptest.ResponseRecorder { + t.Helper() + var buf strings.Builder + mw := multipart.NewWriter(&buf) + if articleID != "" { + mw.WriteField("article_id", articleID) + } else { + mw.WriteField("session_token", "test-pending-token") + } + mw.WriteField("_csrf", csrfToken) + fw, _ := mw.CreateFormFile("file", "hello.txt") + fw.Write([]byte("hello world")) + mw.Close() + return e.do(http.MethodPost, "/my/articles/attachments", cookie, strings.NewReader(buf.String()), mw.FormDataContentType()) +} + +// csrfTokenFor fetches a fresh CSRF token for an authenticated session. +func (e *securityTestEnv) csrfTokenFor(t *testing.T, cookie string) string { + t.Helper() + w := e.do(http.MethodGet, "/login", cookie, nil, "") + if w.Code != http.StatusOK { + t.Fatalf("GET /login: status = %d", w.Code) + } + m := csrfTokenRe.FindStringSubmatch(w.Body.String()) + if m == nil { + t.Fatal("login page did not render a CSRF token") + } + return m[1] +} + +func TestLoginRotatesSession(t *testing.T) { + e := newSecurityTestEnv(t) + + // Obtain an anonymous session (pre-login cookie). + req := httptest.NewRequest(http.MethodGet, "/login", nil) + w := httptest.NewRecorder() + e.router.ServeHTTP(w, req) + preLoginCookie := e.sessionCookie(w) + if preLoginCookie == "" { + t.Fatal("expected anonymous session cookie") + } + + authCookie := e.login(t, "alice") + if authCookie == preLoginCookie { + t.Fatal("session cookie was not rotated on login (fixation risk)") + } + + // The authenticated session works. + w = e.do(http.MethodGet, "/my/whoami", authCookie, nil, "") + if w.Code != http.StatusOK || !strings.Contains(w.Body.String(), "uid=") { + t.Fatalf("authenticated request failed: status=%d body=%s", w.Code, w.Body.String()) + } + + // The old (fixated) session must NOT carry the login. + w = e.do(http.MethodGet, "/my/whoami", preLoginCookie, nil, "") + if w.Code != http.StatusFound { + t.Fatalf("pre-login session still authenticated after login: status=%d", w.Code) + } +} + +func TestAttachmentListRequiresOwnership(t *testing.T) { + e := newSecurityTestEnv(t) + var aliceArt, bobArt models.Article + e.db.Where("slug = ?", "alice-post").First(&aliceArt) + e.db.Where("slug = ?", "bob-post").First(&bobArt) + + alice := e.login(t, "alice") + + // Own article: allowed. + w := e.do(http.MethodGet, fmt.Sprintf("/my/articles/%d/attachments", aliceArt.ID), alice, nil, "") + if w.Code != http.StatusOK { + t.Fatalf("list own attachments: status = %d, want 200", w.Code) + } + + // Someone else's article: forbidden. + w = e.do(http.MethodGet, fmt.Sprintf("/my/articles/%d/attachments", bobArt.ID), alice, nil, "") + if w.Code != http.StatusForbidden { + t.Fatalf("list other user's attachments: status = %d, want 403", w.Code) + } +} + +func TestAttachmentUploadRequiresOwnership(t *testing.T) { + e := newSecurityTestEnv(t) + var bobArt models.Article + e.db.Where("slug = ?", "bob-post").First(&bobArt) + + alice := e.login(t, "alice") + token := e.csrfTokenFor(t, alice) + + // Upload pending (article_id=0 + session token): allowed. + w := e.upload(t, alice, token, "") + if w.Code != http.StatusOK { + t.Fatalf("pending upload: status = %d, body %s", w.Code, w.Body.String()) + } + + // Upload to someone else's article: forbidden. + w = e.upload(t, alice, token, fmt.Sprint(bobArt.ID)) + if w.Code != http.StatusForbidden { + t.Fatalf("upload to other user's article: status = %d, want 403", w.Code) + } +} + +func TestAttachmentDeleteRequiresOwnership(t *testing.T) { + e := newSecurityTestEnv(t) + var aliceArt, bobArt models.Article + e.db.Where("slug = ?", "alice-post").First(&aliceArt) + e.db.Where("slug = ?", "bob-post").First(&bobArt) + + alice := e.login(t, "alice") + token := e.csrfTokenFor(t, alice) + + // Alice uploads an attachment to her own article. + w := e.upload(t, alice, token, fmt.Sprint(aliceArt.ID)) + if w.Code != http.StatusOK { + t.Fatalf("upload: status = %d, body %s", w.Code, w.Body.String()) + } + + // Bob uploads an attachment to his own article. + bob := e.login(t, "bob") + bobToken := e.csrfTokenFor(t, bob) + w = e.upload(t, bob, bobToken, fmt.Sprint(bobArt.ID)) + if w.Code != http.StatusOK { + t.Fatalf("upload (bob): status = %d, body %s", w.Code, w.Body.String()) + } + + var bobAtt models.Attachment + if err := e.db.Where("uploader_id = ?", userIDByUsername(t, e.db, "bob")).First(&bobAtt).Error; err != nil { + t.Fatalf("bob attachment not found: %v", err) + } + + // Alice cannot delete Bob's attachment. + form := url.Values{} + form.Set("_csrf", token) + w = e.do(http.MethodPost, fmt.Sprintf("/my/articles/attachments/%d/delete", bobAtt.ID), alice, + strings.NewReader(form.Encode()), "application/x-www-form-urlencoded") + if w.Code != http.StatusForbidden { + t.Fatalf("delete other user's attachment: status = %d, want 403", w.Code) + } + + // Bob can delete his own. + form.Set("_csrf", bobToken) + w = e.do(http.MethodPost, fmt.Sprintf("/my/articles/attachments/%d/delete", bobAtt.ID), bob, + strings.NewReader(form.Encode()), "application/x-www-form-urlencoded") + if w.Code != http.StatusOK { + t.Fatalf("delete own attachment: status = %d, want 200 (body %s)", w.Code, w.Body.String()) + } + + // Bob's attachment record should be gone. + var count int64 + e.db.Model(&models.Attachment{}).Where("id = ?", bobAtt.ID).Count(&count) + if count != 0 { + t.Fatal("attachment was not deleted") + } +} + +func TestAttachmentCSRFEnforced(t *testing.T) { + e := newSecurityTestEnv(t) + alice := e.login(t, "alice") + + // POST without a CSRF token must be rejected before reaching the handler. + var buf strings.Builder + mw := multipart.NewWriter(&buf) + fw, _ := mw.CreateFormFile("file", "hello.txt") + fw.Write([]byte("hello")) + mw.Close() + w := e.do(http.MethodPost, "/my/articles/attachments", alice, strings.NewReader(buf.String()), mw.FormDataContentType()) + if w.Code != http.StatusForbidden { + t.Fatalf("upload without CSRF token: status = %d, want 403", w.Code) + } +} + +func TestAttachmentAdminOverride(t *testing.T) { + e := newSecurityTestEnv(t) + var bobArt models.Article + e.db.Where("slug = ?", "bob-post").First(&bobArt) + + admin := e.login(t, "admin") + token := e.csrfTokenFor(t, admin) + + // Admin may list and upload to any article. + w := e.do(http.MethodGet, fmt.Sprintf("/my/articles/%d/attachments", bobArt.ID), admin, nil, "") + if w.Code != http.StatusOK { + t.Fatalf("admin list: status = %d, want 200", w.Code) + } + w = e.upload(t, admin, token, fmt.Sprint(bobArt.ID)) + if w.Code != http.StatusOK { + t.Fatalf("admin upload: status = %d, want 200 (body %s)", w.Code, w.Body.String()) + } + + // Clean up files created during the test (best effort). + entries, _ := os.ReadDir(filepath.Join(e.storageDir, "attachments")) + for _, ent := range entries { + os.Remove(filepath.Join(e.storageDir, "attachments", ent.Name())) + } +} + +func userIDByUsername(t *testing.T, db *gorm.DB, username string) uint { + t.Helper() + var u models.User + if err := db.Where("username = ?", username).First(&u).Error; err != nil { + t.Fatalf("user %s not found: %v", username, err) + } + return u.ID +} diff --git a/main.go b/main.go index 4c5466a..22da3f1 100644 --- a/main.go +++ b/main.go @@ -45,14 +45,26 @@ func main() { store := cookie.NewStore([]byte(cfg.Secret)) store.Options(sessions.Options{ Path: "/", - MaxAge: 86400, // 24 hours - HttpOnly: true, // prevent XSS access - Secure: false, // set true in production with HTTPS + MaxAge: 86400, // 24 hours + HttpOnly: true, // prevent XSS access + SameSite: http.SameSiteLaxMode, // CSRF defense-in-depth; token check is the primary control + // Secure is set per request (over HTTPS only) in the middleware below. }) // 4. Create Gin router. router := gin.Default() + // 4b. Trusted proxies: only IPs listed here may influence the client IP + // (X-Forwarded-For). Without this, gin trusts every proxy and a client + // can spoof the IP recorded for comments/article views. + if err := router.SetTrustedProxies(cfg.Web.TrustedProxies); err != nil { + log.Fatalf("Invalid trusted_proxies in config: %v", err) + } + + // 4c. Security response headers (registered first so they are present + // even on rejected responses). + router.Use(middleware.SecurityHeaders()) + // 5. Load HTML templates. router.LoadHTMLGlob("templates/**/*.html") @@ -69,6 +81,26 @@ func main() { // 6. Global session middleware. router.Use(sessions.Sessions("blog_session", store)) + // 6a. Per-request session cookie hardening: Secure only over HTTPS, and + // SameSite=Lax. Applied per request because the app sits behind a TLS + // terminator (Caddy/Cloudflare) and cannot know at startup whether the + // client connection is encrypted. + router.Use(func(c *gin.Context) { + opts := sessions.Options{ + Path: "/", + MaxAge: 86400, + HttpOnly: true, + SameSite: http.SameSiteLaxMode, + } + if middleware.IsHTTPSRequest(c) { + opts.Secure = true + } + sessions.Default(c).Options(opts) + }) + + // 6b. CSRF protection (must run after the session middleware). + router.Use(middleware.CSRFProtect()) + // 7. Global context middleware (sets IsLoggedIn, Username for templates). router.Use(middleware.SetUserContext(db)) diff --git a/middleware/clientip_test.go b/middleware/clientip_test.go new file mode 100644 index 0000000..1b2e83d --- /dev/null +++ b/middleware/clientip_test.go @@ -0,0 +1,59 @@ +package middleware + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/gin-gonic/gin" +) + +// newClientIPRouter mirrors the production trusted-proxy configuration: +// only loopback is trusted (the Caddy/nginx host). +func newClientIPRouter() *gin.Engine { + gin.SetMode(gin.TestMode) + r := gin.New() + if err := r.SetTrustedProxies([]string{"127.0.0.1", "::1"}); err != nil { + panic(err) + } + r.GET("/ip", func(c *gin.Context) { + c.String(http.StatusOK, c.ClientIP()) + }) + return r +} + +func TestClientIPSpoofingBlocked(t *testing.T) { + r := newClientIPRouter() + + // A direct (untrusted) client sending a forged X-Forwarded-For must not + // be able to change the recorded IP. + req := httptest.NewRequest(http.MethodGet, "/ip", nil) + req.RemoteAddr = "203.0.113.5:12345" + req.Header.Set("X-Forwarded-For", "6.6.6.6") + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + if got := w.Body.String(); got != "203.0.113.5" { + t.Errorf("direct client with forged XFF: got %q, want 203.0.113.5", got) + } + + // A trusted proxy (loopback) forwarding a real chain: the rightmost + // untrusted entry wins, earlier (client-supplied) entries are ignored. + req = httptest.NewRequest(http.MethodGet, "/ip", nil) + req.RemoteAddr = "127.0.0.1:54321" + req.Header.Set("X-Forwarded-For", "6.6.6.6, 198.51.100.42") + w = httptest.NewRecorder() + r.ServeHTTP(w, req) + if got := w.Body.String(); got != "198.51.100.42" { + t.Errorf("proxy-forwarded chain: got %q, want 198.51.100.42 (client-supplied entry must be ignored)", got) + } + + // A trusted proxy forwarding a single entry: that entry is the client. + req = httptest.NewRequest(http.MethodGet, "/ip", nil) + req.RemoteAddr = "127.0.0.1:54321" + req.Header.Set("X-Forwarded-For", "198.51.100.42") + w = httptest.NewRecorder() + r.ServeHTTP(w, req) + if got := w.Body.String(); got != "198.51.100.42" { + t.Errorf("proxy-forwarded single entry: got %q, want 198.51.100.42", got) + } +} diff --git a/middleware/csrf.go b/middleware/csrf.go new file mode 100644 index 0000000..d404607 --- /dev/null +++ b/middleware/csrf.go @@ -0,0 +1,93 @@ +package middleware + +import ( + "crypto/rand" + "encoding/hex" + "net/http" + "strconv" + + "github.com/gin-contrib/sessions" + "github.com/gin-gonic/gin" +) + +// CSRF protection uses the synchronizer-token pattern on top of the existing +// session store: +// - Safe methods (GET/HEAD/OPTIONS): a per-session token is created on first +// use and exposed to templates / JS so it can be embedded in forms. +// - Unsafe methods (POST/PUT/PATCH/DELETE): the request must carry the token +// either as the "_csrf" form field (regular forms, multipart uploads) or +// in the "X-CSRF-Token" header (AJAX). A mismatch aborts with 403. +// +// The token is bound to the session, so it works for anonymous visitors (e.g. +// the comment form) as well as for logged-in users. + +const ( + // CSRFFieldName is the form field carrying the token. + CSRFFieldName = "_csrf" + // CSRFHeaderName is the HTTP header carrying the token (AJAX). + CSRFHeaderName = "X-CSRF-Token" + // CSRFSessionKey stores the token server-side. + CSRFSessionKey = "csrf_token" + // CSRFContextKey exposes the token to handlers/templates via c.Set. + CSRFContextKey = "csrf_token" +) + +// newCSRFToken returns a 256-bit random hex token. A crypto/rand failure is +// unrecoverable; panic rather than degrade the defense. +func newCSRFToken() string { + b := make([]byte, 32) + if _, err := rand.Read(b); err != nil { + panic("csrf: failed to read random bytes: " + err.Error()) + } + return hex.EncodeToString(b) +} + +// csrfTokensEqual compares two tokens in constant time. +func csrfTokensEqual(a, b string) bool { + if len(a) != len(b) { + return false + } + var v byte + for i := 0; i < len(a); i++ { + v |= a[i] ^ b[i] + } + return v == 0 +} + +// CSRFProtect validates unsafe requests against the per-session CSRF token. +// It must be registered after the sessions middleware. +func CSRFProtect() gin.HandlerFunc { + return func(c *gin.Context) { + session := sessions.Default(c) + token, _ := session.Get(CSRFSessionKey).(string) + + switch c.Request.Method { + case http.MethodGet, http.MethodHead, http.MethodOptions, http.MethodTrace: + // Safe method: make sure a token exists and hand it to the + // template layer. + if token == "" { + token = newCSRFToken() + session.Set(CSRFSessionKey, token) + _ = session.Save() + } + c.Set(CSRFContextKey, token) + c.Next() + return + } + + // Unsafe method: require a matching token. + supplied := c.PostForm(CSRFFieldName) + if supplied == "" { + supplied = c.GetHeader(CSRFHeaderName) + } + if token == "" || supplied == "" || !csrfTokensEqual(token, supplied) { + c.Header("Cache-Control", "no-store") + c.Header("Content-Type", "text/plain; charset=utf-8") + c.String(http.StatusForbidden, "403 Forbidden: CSRF token missing or invalid ("+strconv.Quote(c.Request.Method)+" "+c.Request.URL.Path+")") + c.Abort() + return + } + c.Set(CSRFContextKey, token) + c.Next() + } +} diff --git a/middleware/csrf_test.go b/middleware/csrf_test.go new file mode 100644 index 0000000..04ac19e --- /dev/null +++ b/middleware/csrf_test.go @@ -0,0 +1,154 @@ +package middleware + +import ( + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/gin-contrib/sessions" + "github.com/gin-contrib/sessions/cookie" + "github.com/gin-gonic/gin" +) + +func newCSRFTestRouter() *gin.Engine { + gin.SetMode(gin.TestMode) + r := gin.New() + store := cookie.NewStore([]byte("test-secret")) + r.Use(sessions.Sessions("test_session", store)) + r.Use(CSRFProtect()) + + r.GET("/form", func(c *gin.Context) { + c.String(http.StatusOK, "TOKEN="+c.GetString(CSRFContextKey)) + }) + r.HEAD("/form", func(c *gin.Context) { + c.String(http.StatusOK, "TOKEN="+c.GetString(CSRFContextKey)) + }) + r.POST("/action", func(c *gin.Context) { + c.String(http.StatusOK, "ok") + }) + return r +} + +// tokenFromForm performs GET /form with the given session cookie and returns +// the issued CSRF token plus the (possibly new) session cookie. +func tokenFromForm(t *testing.T, r *gin.Engine, sessionCookie string) (token, cookie string) { + t.Helper() + req := httptest.NewRequest(http.MethodGet, "/form", nil) + if sessionCookie != "" { + req.Header.Set("Cookie", sessionCookie) + } + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + if w.Code != http.StatusOK { + t.Fatalf("GET /form: status = %d, want 200", w.Code) + } + body := w.Body.String() + const prefix = "TOKEN=" + if !strings.HasPrefix(body, prefix) { + t.Fatalf("GET /form: unexpected body %q", body) + } + token = strings.TrimPrefix(body, prefix) + cookie = w.Header().Get("Set-Cookie") + return token, cookie +} + +func postAction(r *gin.Engine, sessionCookie, token string, useHeader bool) *httptest.ResponseRecorder { + form := url.Values{} + if !useHeader { + form.Set(CSRFFieldName, token) + } + req := httptest.NewRequest(http.MethodPost, "/action", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + if sessionCookie != "" { + req.Header.Set("Cookie", sessionCookie) + } + if useHeader { + req.Header.Set(CSRFHeaderName, token) + } + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + return w +} + +func TestCSRFTokenIssuedOnGET(t *testing.T) { + r := newCSRFTestRouter() + token, cookie := tokenFromForm(t, r, "") + if token == "" { + t.Fatal("expected a token to be issued on GET") + } + if !strings.Contains(cookie, "test_session=") { + t.Fatalf("expected session cookie to be set, got %q", cookie) + } + + // A second GET with the same session must return the same token. + token2, _ := tokenFromForm(t, r, cookie) + if token2 != token { + t.Fatalf("token changed between requests: %q vs %q", token, token2) + } +} + +func TestCSRFPostRejectedWithoutToken(t *testing.T) { + r := newCSRFTestRouter() + _, cookie := tokenFromForm(t, r, "") + + w := postAction(r, cookie, "", false) + if w.Code != http.StatusForbidden { + t.Fatalf("POST without token: status = %d, want 403", w.Code) + } +} + +func TestCSRFPostRejectedWithWrongToken(t *testing.T) { + r := newCSRFTestRouter() + _, cookie := tokenFromForm(t, r, "") + + w := postAction(r, cookie, "bogus-token", false) + if w.Code != http.StatusForbidden { + t.Fatalf("POST with wrong token: status = %d, want 403", w.Code) + } +} + +func TestCSRFPostRejectedWithoutSession(t *testing.T) { + r := newCSRFTestRouter() + // No prior GET: no session, no token issued. + w := postAction(r, "", "some-token", false) + if w.Code != http.StatusForbidden { + t.Fatalf("POST without session: status = %d, want 403", w.Code) + } +} + +func TestCSRFPostAcceptedWithFormField(t *testing.T) { + r := newCSRFTestRouter() + token, cookie := tokenFromForm(t, r, "") + + w := postAction(r, cookie, token, false) + if w.Code != http.StatusOK { + t.Fatalf("POST with valid token: status = %d, want 200 (body: %s)", w.Code, w.Body.String()) + } +} + +func TestCSRFPostAcceptedWithHeader(t *testing.T) { + r := newCSRFTestRouter() + token, cookie := tokenFromForm(t, r, "") + + w := postAction(r, cookie, token, true) + if w.Code != http.StatusOK { + t.Fatalf("POST with token in header: status = %d, want 200 (body: %s)", w.Code, w.Body.String()) + } +} + +func TestCSRFSafeMethodsPassWithoutToken(t *testing.T) { + r := newCSRFTestRouter() + // GET and HEAD are registered routes; OPTIONS is not (gin does not + // auto-register it), so it falls to noRoute - but in all cases the CSRF + // middleware itself must not reject with 403. + for _, method := range []string{http.MethodGet, http.MethodHead, http.MethodOptions} { + req := httptest.NewRequest(method, "/form", nil) + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + if w.Code == http.StatusForbidden { + t.Fatalf("%s /form: status = 403, CSRF middleware must not reject safe methods", method) + } + } +} diff --git a/middleware/https.go b/middleware/https.go new file mode 100644 index 0000000..f3deca5 --- /dev/null +++ b/middleware/https.go @@ -0,0 +1,23 @@ +package middleware + +import ( + "strings" + + "github.com/gin-gonic/gin" +) + +// IsHTTPSRequest reports whether the client reached us over TLS. Behind a +// reverse proxy (Caddy/nginx) the app itself usually terminates plain +// connections, so X-Forwarded-Proto is consulted as well. The header is only +// honored when a trusted proxy forwarded the request - untrusted clients +// spoofing it can at worst break their own session (the cookie turns Secure +// and is refused over plain HTTP). +func IsHTTPSRequest(c *gin.Context) bool { + if c.Request.TLS != nil { + return true + } + if strings.EqualFold(c.GetHeader("X-Forwarded-Proto"), "https") { + return true + } + return false +} diff --git a/middleware/security_headers.go b/middleware/security_headers.go new file mode 100644 index 0000000..89ebb72 --- /dev/null +++ b/middleware/security_headers.go @@ -0,0 +1,42 @@ +package middleware + +import "github.com/gin-gonic/gin" + +// csp is the Content-Security-Policy for HTML responses. +// +// 'unsafe-inline' is required because templates embed inline