From 16942f55a02ac3cafd3e1ab99edaa7396dc80233 Mon Sep 17 00:00:00 2001 From: Jan Eitzinger Date: Thu, 4 Jun 2026 20:08:41 +0200 Subject: [PATCH] Fix medium-severity issues from follow-up security audit Addresses the remaining medium findings from the second-pass audit: - DoS hardening: bound GraphQL query cost with FixedComplexityLimit, and reject non-positive items-per-page / page values so uint64 conversion cannot underflow into an unbounded LIMIT/OFFSET. The -1 "load all" sentinel stays valid for dashboards; REST now returns 400 for bad input. - Security headers: add X-Content-Type-Options, X-Frame-Options, Referrer-Policy and a conservative CSP (frame-ancestors/object-src/ base-uri) that hardens against clickjacking and base-tag injection without restricting the self-hosted SPA's inline scripts. - Stored XSS: render job.metaData.message as escaped text instead of {@html ...} in Job.root and JobFootprint, preserving line breaks via white-space: pre-wrap. - SQL injection hardening: parameterize the tag-scope IN list and the manager project subquery in CountTags instead of interpolating user.Username / user.Projects (externally sourced via OIDC/LDAP). - CSRF defense-in-depth: reject cross-site state-changing requests via Sec-Fetch-Site, failing open for non-browser clients, on top of the existing SameSite=Lax session cookie. Co-Authored-By: Claude Opus 4.8 (1M context) Entire-Checkpoint: de7d47a85c7c --- cmd/cc-backend/server.go | 40 ++++++++++++++++++- internal/api/job.go | 8 ++++ internal/repository/jobQuery.go | 9 ++++- internal/repository/tags.go | 29 +++++++++----- web/frontend/src/Job.root.svelte | 2 +- .../src/generic/helper/JobFootprint.svelte | 2 +- 6 files changed, 75 insertions(+), 15 deletions(-) diff --git a/cmd/cc-backend/server.go b/cmd/cc-backend/server.go index 33504988..f39cd45d 100644 --- a/cmd/cc-backend/server.go +++ b/cmd/cc-backend/server.go @@ -23,6 +23,7 @@ import ( "github.com/99designs/gqlgen/graphql" "github.com/99designs/gqlgen/graphql/handler" + "github.com/99designs/gqlgen/graphql/handler/extension" "github.com/99designs/gqlgen/graphql/handler/transport" "github.com/99designs/gqlgen/graphql/playground" "github.com/ClusterCockpit/cc-backend/internal/api" @@ -50,6 +51,12 @@ const ( envDebug = "DEBUG" ) +// maxQueryComplexity bounds the cost of a single GraphQL query to mitigate +// denial-of-service via deeply nested or heavily aliased queries. The default +// per-field cost is 1, so this leaves ample headroom for legitimate dashboard +// queries while rejecting pathological ones. +const maxQueryComplexity = 5000 + // Server encapsulates the HTTP server state and dependencies type Server struct { router chi.Router @@ -90,6 +97,7 @@ func (s *Server) init() error { generated.NewExecutableSchema(generated.Config{Resolvers: resolver})) graphQLServer.AddTransport(transport.POST{}) + graphQLServer.Use(extension.FixedComplexityLimit(maxQueryComplexity)) // Inject a per-request stats cache so that grouped statistics queries // sharing the same (filter, groupBy) pair are executed only once. @@ -136,8 +144,38 @@ func (s *Server) init() error { })) s.router.Use(func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + h := rw.Header() + h.Set("X-Content-Type-Options", "nosniff") + h.Set("X-Frame-Options", "DENY") + h.Set("Referrer-Policy", "same-origin") + // Conservative CSP: blocks clickjacking (frame-ancestors), plugin + // content (object-src) and injection (base-uri) without + // restricting scripts/styles, so it cannot break the self-hosted + // SPA which relies on inline scripts. A full script-src policy needs + // per-template nonces and should be added separately. + h.Set("Content-Security-Policy", "frame-ancestors 'none'; object-src 'none'; base-uri 'self'") if r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https" { - rw.Header().Set("Strict-Transport-Security", "max-age=31536000; includeSubDomains") + h.Set("Strict-Transport-Security", "max-age=31536000; includeSubDomains") + } + next.ServeHTTP(rw, r) + }) + }) + + // CSRF defense-in-depth on top of the SameSite=Lax session cookie: reject + // cross-site state-changing requests. Modern browsers set Sec-Fetch-Site on + // every request, so this stops a malicious site from driving cookie- + // authenticated POST/PUT/DELETE/PATCH calls. It fails open when the header is + // absent or not "cross-site", so non-browser API clients and the same-origin + // SPA are unaffected. + s.router.Use(func(next http.Handler) http.Handler { + return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + switch r.Method { + case http.MethodGet, http.MethodHead, http.MethodOptions, http.MethodTrace: + default: + if r.Header.Get("Sec-Fetch-Site") == "cross-site" { + http.Error(rw, "cross-site request blocked", http.StatusForbidden) + return + } } next.ServeHTTP(rw, r) }) diff --git a/internal/api/job.go b/internal/api/job.go index 76ec3e2a..37c0e454 100644 --- a/internal/api/job.go +++ b/internal/api/job.go @@ -166,6 +166,10 @@ func (api *RestAPI) getJobs(rw http.ResponseWriter, r *http.Request) { handleError(err, http.StatusBadRequest, rw) return } + if x < 1 { + handleError(fmt.Errorf("page must be >= 1"), http.StatusBadRequest, rw) + return + } page.Page = x case "items-per-page": x, err := strconv.Atoi(vals[0]) @@ -173,6 +177,10 @@ func (api *RestAPI) getJobs(rw http.ResponseWriter, r *http.Request) { handleError(err, http.StatusBadRequest, rw) return } + if x < 1 { + handleError(fmt.Errorf("items-per-page must be >= 1"), http.StatusBadRequest, rw) + return + } page.ItemsPerPage = x case "with-metadata": withMetadata = true diff --git a/internal/repository/jobQuery.go b/internal/repository/jobQuery.go index 0b162179..49ab1fbe 100644 --- a/internal/repository/jobQuery.go +++ b/internal/repository/jobQuery.go @@ -76,8 +76,15 @@ func (r *JobRepository) QueryJobs( } if page != nil && page.ItemsPerPage != -1 { + // -1 is the only valid non-positive value ("load all"); reject other + // non-positive values so that uint64(page.ItemsPerPage) cannot underflow + // into a huge limit. Clamp Page to >= 1 to avoid the same on the offset. + if page.ItemsPerPage < 1 { + return nil, fmt.Errorf("invalid items-per-page value: %d", page.ItemsPerPage) + } + p := max(page.Page, 1) limit := uint64(page.ItemsPerPage) - query = query.Offset((uint64(page.Page) - 1) * limit).Limit(limit) + query = query.Offset((uint64(p) - 1) * limit).Limit(limit) } for _, f := range filters { diff --git a/internal/repository/tags.go b/internal/repository/tags.go index 39ccd90d..dca8574d 100644 --- a/internal/repository/tags.go +++ b/internal/repository/tags.go @@ -311,26 +311,33 @@ func (r *JobRepository) CountTags(user *schema.User) (tags []schema.Tag, counts LeftJoin("jobtag jt ON t.id = jt.tag_id"). GroupBy("t.tag_type, t.tag_name") - // Build scope list for filtering - var scopeBuilder strings.Builder - scopeBuilder.WriteString(`"global"`) + // Build scope list for filtering. Values are parameterized rather than + // interpolated because user.Username originates from external identity + // providers (OIDC/LDAP) and must not be trusted as SQL. + scopes := []string{"global"} if user != nil { - scopeBuilder.WriteString(`,"`) - scopeBuilder.WriteString(user.Username) - scopeBuilder.WriteString(`"`) + scopes = append(scopes, user.Username) if user.HasAnyRole([]schema.Role{schema.RoleAdmin, schema.RoleSupport}) { - scopeBuilder.WriteString(`,"admin"`) + scopes = append(scopes, "admin") } } - q = q.Where("t.tag_scope IN (" + scopeBuilder.String() + ")") + q = q.Where(sq.Eq{"t.tag_scope": scopes}) // Handle Job Ownership if user != nil && user.HasAnyRole([]schema.Role{schema.RoleAdmin, schema.RoleSupport}) { // ADMIN || SUPPORT: Count all jobs // cclog.Debug("CountTags: User Admin or Support -> Count all Jobs for Tags") // Unchanged: Needs to be own case still, due to UserRole/NoRole compatibility handling in else case - } else if user != nil && user.HasRole(schema.RoleManager) { // MANAGER: Count own jobs plus project's jobs - // Build ("project1", "project2", ...) list of variable length directly in SQL string - q = q.Where("jt.job_id IN (SELECT id FROM job WHERE job.hpc_user = ? OR job.project IN (\""+strings.Join(user.Projects, "\",\"")+"\"))", user.Username) + } else if user != nil && user.HasRole(schema.RoleManager) && len(user.Projects) > 0 { // MANAGER: Count own jobs plus project's jobs + // Build a parameterized ("?", "?", ...) placeholder list for the + // variable-length project set instead of interpolating values into SQL. + args := make([]any, 0, len(user.Projects)+1) + args = append(args, user.Username) + placeholders := make([]string, len(user.Projects)) + for i, p := range user.Projects { + placeholders[i] = "?" + args = append(args, p) + } + q = q.Where("jt.job_id IN (SELECT id FROM job WHERE job.hpc_user = ? OR job.project IN ("+strings.Join(placeholders, ",")+"))", args...) } else if user != nil { // USER OR NO ROLE (Compatibility): Only count own jobs q = q.Where("jt.job_id IN (SELECT id FROM job WHERE job.hpc_user = ?)", user.Username) } diff --git a/web/frontend/src/Job.root.svelte b/web/frontend/src/Job.root.svelte index 8e55a15a..2c6fbd42 100644 --- a/web/frontend/src/Job.root.svelte +++ b/web/frontend/src/Job.root.svelte @@ -237,7 +237,7 @@ The following note was added by administrators: - {@html thisJob.metaData.message} + {thisJob.metaData.message} diff --git a/web/frontend/src/generic/helper/JobFootprint.svelte b/web/frontend/src/generic/helper/JobFootprint.svelte index 9eaf3ff9..8df3e3ae 100644 --- a/web/frontend/src/generic/helper/JobFootprint.svelte +++ b/web/frontend/src/generic/helper/JobFootprint.svelte @@ -228,7 +228,7 @@ {/each} {#if job?.metaData?.message}
- {@html job.metaData.message} + {job.metaData.message} {/if}