diff --git a/runtime/resolver.go b/runtime/resolver.go index 6925aea2238..9670d606dad 100644 --- a/runtime/resolver.go +++ b/runtime/resolver.go @@ -9,9 +9,7 @@ import ( "errors" "fmt" "io" - "strconv" - "github.com/mitchellh/hashstructure/v2" runtimev1 "github.com/rilldata/rill/proto/gen/rill/runtime/v1" "github.com/rilldata/rill/runtime/drivers" "github.com/rilldata/rill/runtime/pkg/jsonval" @@ -207,14 +205,19 @@ func (r *Runtime) Resolve(ctx context.Context, opts *ResolveOptions) (res Resolv if _, err := hash.Write(cacheKey); err != nil { return nil, nil, err } - if opts.Claims.UserAttributes != nil { - h, err := hashstructure.Hash(opts.Claims.UserAttributes, hashstructure.FormatV2, nil) - if err != nil { - return nil, nil, err - } - if _, err = hash.Write([]byte(strconv.FormatUint(h, 16))); err != nil { - return nil, nil, err - } + // Hash the security claims, not just the user attributes: + // permissions, additional rules (e.g. locked filters on magic auth tokens) and skipped checks + // all change the resolved security policy, and results must not be shared across them. + // The user ID is excluded since it does not affect the resolved policy, + // which enables sharing results between users that resolve to the same policy (common for embeds). + claimsForKey := *opts.Claims + claimsForKey.UserID = "" + claimsJSON, err := json.Marshal(&claimsForKey) + if err != nil { + return nil, nil, err + } + if _, err := hash.Write(claimsJSON); err != nil { + return nil, nil, err } for _, ref := range resolver.Refs() { res, err := ctrl.Get(ctx, ref, false) diff --git a/runtime/resolvers/testdata/metrics_security.yaml b/runtime/resolvers/testdata/metrics_security.yaml index d314a8c287b..a303de3e77c 100644 --- a/runtime/resolvers/testdata/metrics_security.yaml +++ b/runtime/resolvers/testdata/metrics_security.yaml @@ -33,3 +33,67 @@ tests: result: - country: US sum: 9 + # The following tests run the same query back-to-back with identical (empty) user attributes, + # varying only the additional rules and skipped security checks. + # They protect against cached results leaking across different security contexts. + - name: cache_isolation_row_filter_dk + resolver: metrics + properties: + metrics_view: metrics_no_security + dimensions: + - name: country + measures: + - name: sum + sort: + - name: country + additional_rules: + - row_filter: country = 'DK' + result: + - country: DK + sum: 6 + - name: cache_isolation_row_filter_us + resolver: metrics + properties: + metrics_view: metrics_no_security + dimensions: + - name: country + measures: + - name: sum + sort: + - name: country + additional_rules: + - row_filter: country = 'US' + result: + - country: US + sum: 9 + - name: cache_isolation_no_rules + resolver: metrics + properties: + metrics_view: metrics_no_security + dimensions: + - name: country + measures: + - name: sum + sort: + - name: country + result: + - country: DK + sum: 6 + - country: US + sum: 9 + - name: cache_isolation_skip_checks + resolver: metrics + properties: + metrics_view: metrics_no_security + dimensions: + - name: country + measures: + - name: sum + sort: + - name: country + skip_security_checks: true + result: + - country: DK + sum: 6 + - country: US + sum: 9