mirror of
https://github.com/ClusterCockpit/cc-backend
synced 2026-06-06 03:37:29 +02:00
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) <noreply@anthropic.com>
Entire-Checkpoint: de7d47a85c7c
This commit is contained in:
@@ -23,6 +23,7 @@ import (
|
|||||||
|
|
||||||
"github.com/99designs/gqlgen/graphql"
|
"github.com/99designs/gqlgen/graphql"
|
||||||
"github.com/99designs/gqlgen/graphql/handler"
|
"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/handler/transport"
|
||||||
"github.com/99designs/gqlgen/graphql/playground"
|
"github.com/99designs/gqlgen/graphql/playground"
|
||||||
"github.com/ClusterCockpit/cc-backend/internal/api"
|
"github.com/ClusterCockpit/cc-backend/internal/api"
|
||||||
@@ -50,6 +51,12 @@ const (
|
|||||||
envDebug = "DEBUG"
|
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
|
// Server encapsulates the HTTP server state and dependencies
|
||||||
type Server struct {
|
type Server struct {
|
||||||
router chi.Router
|
router chi.Router
|
||||||
@@ -90,6 +97,7 @@ func (s *Server) init() error {
|
|||||||
generated.NewExecutableSchema(generated.Config{Resolvers: resolver}))
|
generated.NewExecutableSchema(generated.Config{Resolvers: resolver}))
|
||||||
|
|
||||||
graphQLServer.AddTransport(transport.POST{})
|
graphQLServer.AddTransport(transport.POST{})
|
||||||
|
graphQLServer.Use(extension.FixedComplexityLimit(maxQueryComplexity))
|
||||||
|
|
||||||
// Inject a per-request stats cache so that grouped statistics queries
|
// Inject a per-request stats cache so that grouped statistics queries
|
||||||
// sharing the same (filter, groupBy) pair are executed only once.
|
// 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 {
|
s.router.Use(func(next http.Handler) http.Handler {
|
||||||
return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
|
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 <base> 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" {
|
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)
|
next.ServeHTTP(rw, r)
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -166,6 +166,10 @@ func (api *RestAPI) getJobs(rw http.ResponseWriter, r *http.Request) {
|
|||||||
handleError(err, http.StatusBadRequest, rw)
|
handleError(err, http.StatusBadRequest, rw)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
if x < 1 {
|
||||||
|
handleError(fmt.Errorf("page must be >= 1"), http.StatusBadRequest, rw)
|
||||||
|
return
|
||||||
|
}
|
||||||
page.Page = x
|
page.Page = x
|
||||||
case "items-per-page":
|
case "items-per-page":
|
||||||
x, err := strconv.Atoi(vals[0])
|
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)
|
handleError(err, http.StatusBadRequest, rw)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
if x < 1 {
|
||||||
|
handleError(fmt.Errorf("items-per-page must be >= 1"), http.StatusBadRequest, rw)
|
||||||
|
return
|
||||||
|
}
|
||||||
page.ItemsPerPage = x
|
page.ItemsPerPage = x
|
||||||
case "with-metadata":
|
case "with-metadata":
|
||||||
withMetadata = true
|
withMetadata = true
|
||||||
|
|||||||
@@ -76,8 +76,15 @@ func (r *JobRepository) QueryJobs(
|
|||||||
}
|
}
|
||||||
|
|
||||||
if page != nil && page.ItemsPerPage != -1 {
|
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)
|
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 {
|
for _, f := range filters {
|
||||||
|
|||||||
@@ -311,26 +311,33 @@ func (r *JobRepository) CountTags(user *schema.User) (tags []schema.Tag, counts
|
|||||||
LeftJoin("jobtag jt ON t.id = jt.tag_id").
|
LeftJoin("jobtag jt ON t.id = jt.tag_id").
|
||||||
GroupBy("t.tag_type, t.tag_name")
|
GroupBy("t.tag_type, t.tag_name")
|
||||||
|
|
||||||
// Build scope list for filtering
|
// Build scope list for filtering. Values are parameterized rather than
|
||||||
var scopeBuilder strings.Builder
|
// interpolated because user.Username originates from external identity
|
||||||
scopeBuilder.WriteString(`"global"`)
|
// providers (OIDC/LDAP) and must not be trusted as SQL.
|
||||||
|
scopes := []string{"global"}
|
||||||
if user != nil {
|
if user != nil {
|
||||||
scopeBuilder.WriteString(`,"`)
|
scopes = append(scopes, user.Username)
|
||||||
scopeBuilder.WriteString(user.Username)
|
|
||||||
scopeBuilder.WriteString(`"`)
|
|
||||||
if user.HasAnyRole([]schema.Role{schema.RoleAdmin, schema.RoleSupport}) {
|
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
|
// Handle Job Ownership
|
||||||
if user != nil && user.HasAnyRole([]schema.Role{schema.RoleAdmin, schema.RoleSupport}) { // ADMIN || SUPPORT: Count all jobs
|
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")
|
// 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
|
// 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
|
} else if user != nil && user.HasRole(schema.RoleManager) && len(user.Projects) > 0 { // MANAGER: Count own jobs plus project's jobs
|
||||||
// Build ("project1", "project2", ...) list of variable length directly in SQL string
|
// Build a parameterized ("?", "?", ...) placeholder list for the
|
||||||
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)
|
// 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
|
} 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)
|
q = q.Where("jt.job_id IN (SELECT id FROM job WHERE job.hpc_user = ?)", user.Username)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -237,7 +237,7 @@
|
|||||||
The following note was added by administrators:
|
The following note was added by administrators:
|
||||||
</Card>
|
</Card>
|
||||||
<Card body>
|
<Card body>
|
||||||
{@html thisJob.metaData.message}
|
<span style="white-space: pre-wrap">{thisJob.metaData.message}</span>
|
||||||
</Card>
|
</Card>
|
||||||
</CardBody>
|
</CardBody>
|
||||||
</TabPane>
|
</TabPane>
|
||||||
|
|||||||
@@ -228,7 +228,7 @@
|
|||||||
{/each}
|
{/each}
|
||||||
{#if job?.metaData?.message}
|
{#if job?.metaData?.message}
|
||||||
<hr class="mt-1 mb-2" />
|
<hr class="mt-1 mb-2" />
|
||||||
{@html job.metaData.message}
|
<span style="white-space: pre-wrap">{job.metaData.message}</span>
|
||||||
{/if}
|
{/if}
|
||||||
</CardBody>
|
</CardBody>
|
||||||
</Card>
|
</Card>
|
||||||
|
|||||||
Reference in New Issue
Block a user