Skip to content

Commit 0c1a1c1

Browse files
authored
fix: address GitHub security findings (CodeQL #38/#40, Dependabot #207) (#163)
* fix: address GitHub security findings - ui_jobs.go: replace log.Printf with structured h.logger() calls for user-controlled jobID/eventID values to prevent log injection (CodeQL #40) - middleware_csrf.go: set CSRF cookie Secure attribute to true unconditionally; app is always served over HTTPS in production (CodeQL #38) - puppeteer-worker: add npm override to force uuid >= 11.1.1, fixing buffer bounds check vuln in transitive deps via @google-cloud/storage (Dependabot #207, CodeQL #43/#44) * fix: golangci-lint failures - ui_jobs.go: wrap long ErrorContext call to satisfy golines - middleware_csrf.go: rename unused r param to _, remove now-unused isForwardedHTTPS helper (both became dead code after Secure: true change) * fix: format ErrorContext call to satisfy golines (max-len 120)
1 parent a0feb16 commit 0c1a1c1

4 files changed

Lines changed: 20 additions & 30 deletions

File tree

services/merrymaker-go/internal/http/middleware_csrf.go

Lines changed: 4 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -137,40 +137,21 @@ type csrfCookieParams struct {
137137
}
138138

139139
// setCSRFCookie sets the CSRF token cookie.
140-
func setCSRFCookie(w http.ResponseWriter, r *http.Request, params csrfCookieParams) {
141-
// Check if request is over HTTPS, accounting for proxies
142-
isSecure := r.TLS != nil || isForwardedHTTPS(r)
143-
140+
func setCSRFCookie(w http.ResponseWriter, _ *http.Request, params csrfCookieParams) {
141+
// Secure is always true — the app is served over HTTPS (direct TLS or
142+
// a proxy terminating TLS). SameSite=Strict provides additional protection.
144143
http.SetCookie(w, &http.Cookie{
145144
Name: params.Name,
146145
Value: params.Token,
147146
Path: "/",
148147
Domain: params.Domain,
149148
HttpOnly: false, // Must be readable by JavaScript for HTMX to include it
150-
Secure: isSecure,
149+
Secure: true,
151150
SameSite: http.SameSiteStrictMode, // Strict for CSRF tokens
152151
MaxAge: 3600 * 12, // 12 hours
153152
})
154153
}
155154

156-
// isForwardedHTTPS checks if the request was forwarded over HTTPS.
157-
// Handles comma-separated values in X-Forwarded-Proto header.
158-
func isForwardedHTTPS(r *http.Request) bool {
159-
xfProto := r.Header.Get("X-Forwarded-Proto")
160-
if xfProto == "" {
161-
return false
162-
}
163-
164-
// Handle comma-separated values (e.g., "https,http")
165-
for proto := range strings.SplitSeq(xfProto, ",") {
166-
if strings.EqualFold(strings.TrimSpace(proto), "https") {
167-
return true
168-
}
169-
}
170-
171-
return false
172-
}
173-
174155
// validateCSRFToken validates the CSRF token from the request against the cookie value.
175156
// It checks both the X-Csrf-Token header (for HTMX/AJAX) and the csrf_token form field.
176157
// Uses constant-time comparison to prevent timing side-channel attacks.

services/merrymaker-go/internal/http/ui_jobs.go

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1057,7 +1057,7 @@ func (h *UIHandlers) JobEvents(w http.ResponseWriter, r *http.Request) {
10571057

10581058
pageResp, err := h.fetchJobEvents(r.Context(), req.fetchParams)
10591059
if err != nil {
1060-
log.Printf("JobEvents: failed to list events for job %s: %v", jobID, err)
1060+
h.logger().ErrorContext(r.Context(), "JobEvents: failed to list events", "job_id", jobID, "error", err)
10611061
http.Error(w, "failed to load job events", http.StatusInternalServerError)
10621062
return
10631063
}
@@ -1097,7 +1097,10 @@ func (h *UIHandlers) JobEvents(w http.ResponseWriter, r *http.Request) {
10971097

10981098
w.Header().Set("Content-Type", "text/html; charset=utf-8")
10991099
if templateErr := h.T.t.ExecuteTemplate(w, "job-events-fragment", data); templateErr != nil {
1100-
log.Printf("JobEvents: template execution failed for job %s: %v", jobID, templateErr)
1100+
h.logger().ErrorContext(
1101+
r.Context(), "JobEvents: template execution failed",
1102+
"job_id", jobID, "error", templateErr,
1103+
)
11011104
http.Error(w, "failed to render job events", http.StatusInternalServerError)
11021105
}
11031106
}
@@ -1113,7 +1116,10 @@ func (h *UIHandlers) JobEventDetails(w http.ResponseWriter, r *http.Request) {
11131116

11141117
events, err := h.EventSvc.GetByIDs(r.Context(), []string{eventID})
11151118
if err != nil {
1116-
log.Printf("JobEventDetails: failed to fetch event %s for job %s: %v", eventID, jobID, err)
1119+
h.logger().ErrorContext(
1120+
r.Context(), "JobEventDetails: failed to fetch event",
1121+
"event_id", eventID, "job_id", jobID, "error", err,
1122+
)
11171123
http.Error(w, "failed to load event", http.StatusInternalServerError)
11181124
return
11191125
}

services/puppeteer-worker/package-lock.json

Lines changed: 4 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

services/puppeteer-worker/package.json

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,9 @@
3434
"tsx": "^4.22.5",
3535
"typescript": "^6.0.3"
3636
},
37+
"overrides": {
38+
"uuid": "^11.1.1"
39+
},
3740
"dependencies": {
3841
"@aws-sdk/client-s3": "^3.1079.0",
3942
"@aws-sdk/s3-request-presigner": "^3.1079.0",

0 commit comments

Comments
 (0)