Skip to content

security(studio): allowlist Studio variable-values endpoint#259

Open
gonzalesedwin1123 wants to merge 1 commit into
security-dci-crvs-cachefrom
security-studio-variables-oracle
Open

security(studio): allowlist Studio variable-values endpoint#259
gonzalesedwin1123 wants to merge 1 commit into
security-dci-crvs-cachefrom
security-studio-variables-oracle

Conversation

@gonzalesedwin1123

Copy link
Copy Markdown
Member

Stacked on #257 (base = security-dci-crvs-cache). It reuses the _get_data_api_pullable_domain() allowlist primitive #257 adds. Retarget base to 19.0 after #257 merges. The diff here is only the studio commit.

Summary

GET /studio/variables/{resource_type}/{identifier} (and the underlying VariableValueService.get_values_for_subject / get_values_for_subjects) read spp.data.value with .sudo(), no allowlist and no company_id filter, and with the default variables="*" returned every cached value for a subject — including DCI/CRVS (inter-registry health/vital) and scoring/PMT — to any client holding the generic studio:read scope. This is the same exposure class fixed on /Data/pull in #257, on a sibling endpoint/scope; it was surfaced by #257's post-implementation review.

The Individual/Group API response builders also route subject values through this service, so they were affected too.

Fix

Reuse #257's allowlist: both service methods now restrict the spp.data.value query to API-pullable variables and the current company.

  • _data_api_pullable_accessors() (sudo — system config; API authz is the scope check) returns the cel_accessors of pullable variables via spp.cel.variable._get_data_api_pullable_domain() (ordinary external-provider only; DCI-backed / computed / scoring excluded).
  • The query adds ("company_id","=",self.env.company.id) and ("variable_name","in", pullable_accessors). Explicit variables= requests AND with the allowlist, so a sensitive name is dropped. Empty allowlist → ("variable_name","in",[]) → matches nothing (fail-closed, verified against Odoo 19's domain optimizer).

Review

Post-implementation adversarial review: ship — fail-closed on empty allowlist, explicit-sensitive request, cross-company, and missing spp_dci_indicators; all subject-value readers covered; sudo scope appropriate. Two LOW, non-blocking follow-ups tracked separately: (1) a name vs cel_accessor cache-keying inconsistency across writers (security-safe direction — excludes, never leaks — but could drop a legit external value; also affects #257's pull); (2) covered here by an added empty-allowlist regression test.

Tests

Service-level: non-pullable/scoring values excluded under *, explicit sensitive request → {}, company isolation, and an explicit empty-allowlist → {} (locks the in [] fail-closed contract). Existing endpoint/value tests updated to use external-provider fixtures (the only pullable kind under the new policy; spp_studio seeds non-external age/income, so fixtures write them external). The endpoint runs as the API client user, so the allowlist lookup is sudo (a non-sudo lookup 403s — caught by the HttpCase tests).

All green: spp_studio_api_v2 163/163. Lint-clean.

GET /studio/variables/{resource_type}/{identifier} (and the underlying
VariableValueService.get_values_for_subject[s]) read spp.data.value with sudo,
no allowlist, no company filter, and with the default variables="*" returned
EVERY cached value for a subject -- including DCI/CRVS (inter-registry
health/vital) and scoring/PMT values -- to any client holding the generic
studio:read scope. Same exposure class as the Data API /Data/pull leak fixed
in #257, on a sibling endpoint/scope (found by #257's post-implementation
review).

Reuse the #257 allowlist: restrict both service methods to API-pullable
variables (spp.cel.variable._get_data_api_pullable_domain -> ordinary
external-provider only; DCI-backed/computed/scoring excluded) by intersecting
on cel_accessor, and add the missing company_id filter. The pullable-variable
lookup is sudo (system config; API authz is the scope check).

Tests: service-level (excludes non-pullable + scoring, company isolation) and
the existing endpoint/value tests updated to use external-provider fixtures
(the only pullable kind under the new policy). spp_studio_api_v2 162/162.

Stacked on #257 (security-dci-crvs-cache) for the shared allowlist primitive;
retarget PR base to 19.0 after #257 merges.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request enhances security and data isolation by restricting the exposed cached variable values to only API-pullable (external-provider) variables and filtering by the current company. The feedback suggests a performance optimization to pass the requested variable names to _data_api_pullable_accessors to avoid loading all pullable variables into memory when only a subset is requested.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +37 to +51
def _data_api_pullable_accessors(self):
"""cel_accessors whose cached values may be exposed through the API.

Mirrors the Data API allowlist (`spp.cel.variable._get_data_api_pullable_domain`):
only ordinary external-provider variables are returnable; DCI-backed
(inter-registry health/vital) and computed/scoring/aggregate values are
excluded so this endpoint cannot become an oracle for sensitive cached
data. Cache rows are keyed by the variable's cel_accessor.
"""
if "spp.cel.variable" not in self.env:
return []
# sudo: the allowlist is system config; API authorization is the scope
# check at the router. The API client user has no spp.cel.variable ACL.
Variable = self.env["spp.cel.variable"].sudo() # nosemgrep: odoo-sudo-without-context
return Variable.search(Variable._get_data_api_pullable_domain()).mapped("cel_accessor")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Performance Optimization: Filter allowlist query by requested variable names

Currently, _data_api_pullable_accessors fetches all active external variables from the database, even when the client has requested only a specific subset of variables (e.g., ?variables=age). If the system has a large number of variables, this can lead to unnecessary database load and memory consumption.

We can optimize this by passing the optional variable_names list to _data_api_pullable_accessors and filtering the spp.cel.variable search domain when specific variables are requested.

    def _data_api_pullable_accessors(self, variable_names: list[str] | None = None):
        """cel_accessors whose cached values may be exposed through the API.

        Mirrors the Data API allowlist (spp.cel.variable._get_data_api_pullable_domain):
        only ordinary external-provider variables are returnable; DCI-backed
        (inter-registry health/vital) and computed/scoring/aggregate values are
        excluded so this endpoint cannot become an oracle for sensitive cached
        data. Cache rows are keyed by the variable's cel_accessor.
        """
        if "spp.cel.variable" not in self.env:
            return []
        # sudo: the allowlist is system config; API authorization is the scope
        # check at the router. The API client user has no spp.cel.variable ACL.
        Variable = self.env["spp.cel.variable"].sudo()  # nosemgrep: odoo-sudo-without-context
        domain = Variable._get_data_api_pullable_domain()
        if variable_names and "*" not in variable_names:
            domain = domain + [("cel_accessor", "in", variable_names)]
        return Variable.search(domain).mapped("cel_accessor")

Comment on lines 88 to 94
domain = [
("subject_model", "=", "res.partner"),
("subject_id", "=", partner_id),
("period_key", "=", period_key),
("company_id", "=", self.env.company.id),
("variable_name", "in", self._data_api_pullable_accessors()),
]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Pass the requested variable_names to _data_api_pullable_accessors to restrict the allowlist database query to only the requested variables, avoiding loading all pullable variables into memory.

Suggested change
domain = [
("subject_model", "=", "res.partner"),
("subject_id", "=", partner_id),
("period_key", "=", period_key),
("company_id", "=", self.env.company.id),
("variable_name", "in", self._data_api_pullable_accessors()),
]
domain = [
("subject_model", "=", "res.partner"),
("subject_id", "=", partner_id),
("period_key", "=", period_key),
("company_id", "=", self.env.company.id),
("variable_name", "in", self._data_api_pullable_accessors(variable_names)),
]

Comment on lines 162 to 168
domain = [
("subject_model", "=", "res.partner"),
("subject_id", "in", partner_ids),
("period_key", "=", period_key),
("company_id", "=", self.env.company.id),
("variable_name", "in", self._data_api_pullable_accessors()),
]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Pass the requested variable_names to _data_api_pullable_accessors to restrict the allowlist database query to only the requested variables, avoiding loading all pullable variables into memory.

Suggested change
domain = [
("subject_model", "=", "res.partner"),
("subject_id", "in", partner_ids),
("period_key", "=", period_key),
("company_id", "=", self.env.company.id),
("variable_name", "in", self._data_api_pullable_accessors()),
]
domain = [
("subject_model", "=", "res.partner"),
("subject_id", "in", partner_ids),
("period_key", "=", period_key),
("company_id", "=", self.env.company.id),
("variable_name", "in", self._data_api_pullable_accessors(variable_names)),
]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant