Skip to content

ref(search): Add EAP API attribute visibility checks#116091

Merged
nsdeschenes merged 18 commits into
masterfrom
nd/feat-attributes-visibility-core
May 27, 2026
Merged

ref(search): Add EAP API attribute visibility checks#116091
nsdeschenes merged 18 commits into
masterfrom
nd/feat-attributes-visibility-core

Conversation

@nsdeschenes
Copy link
Copy Markdown
Contributor

@nsdeschenes nsdeschenes commented May 22, 2026

The goal of this PR is to lie down the foundational work of removing/hiding internal attributes from non-staff/non-super-user requests.

To determine if an attribute should be hidden or not, we pull that information from the Sentry conventions package from the visibility field from attribute metadata.

These changes are limited to the eap search resolver, as such this PR does not doing any enforcement of these attributes, that will come in follow up PRs. As well, this PR adds in some tests.

Add a shared helper for hiding internal Sentry convention attributes from API surfaces and let SearchResolver track attributes hidden by API visibility configuration.

This keeps default resolver behavior unchanged unless an API caller opts into visibility enforcement.
The `as ATTRIBUTE_METADATA` pattern tells mypy this is an intentional
re-export, fixing the attr-defined errors in tests that access it via
`eap_utils.ATTRIBUTE_METADATA`.
@linear-code
Copy link
Copy Markdown

linear-code Bot commented May 22, 2026

EXP-966

@nsdeschenes nsdeschenes marked this pull request as ready for review May 22, 2026 14:17
@nsdeschenes nsdeschenes requested review from a team as code owners May 22, 2026 14:17
Comment thread src/sentry/search/eap/resolver.py Outdated
Comment thread src/sentry/search/eap/utils.py
Copy link
Copy Markdown
Contributor

@adrianviquez adrianviquez left a comment

Choose a reason for hiding this comment

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

Couple of comments, lmk what you think and can take a 2nd look!

The candidates set already prevents duplicates since we check
`replacement not in candidates` before adding to pending.
@nsdeschenes nsdeschenes requested a review from adrianviquez May 22, 2026 16:18
Comment thread src/sentry/search/eap/resolver.py Outdated
Reverts the import move to avoid a circular import risk between
resolver and utils modules.
When public_alias_override is set (e.g. equation resolution), the
column_definition.public_alias becomes a synthetic label like
"equation|…" which can_expose_attribute_to_api cannot evaluate
against conventions, prefixes, or mappings.
Comment thread src/sentry/search/eap/utils.py
Comment thread tests/sentry/search/eap/test_spans.py Outdated
Comment thread src/sentry/search/eap/resolver.py Outdated
nsdeschenes and others added 4 commits May 25, 2026 13:13
Update EAP attribute visibility tests to exercise real sentry_conventions metadata instead of mocked convention metadata. Keep alias and replacement edge-case coverage while pointing those mappings at real internal DSC attributes.

Co-Authored-By: Codex <noreply@openai.com>
Apply API attribute visibility while resolving selected attributes instead of storing hidden attribute names on the SearchResolver instance.

Keep the API visibility config as the switch for this behavior and add coverage for hidden internal attributes and the include-internal override.

Co-Authored-By: Codex <noreply@openai.com>
@nsdeschenes nsdeschenes requested a review from wmak May 25, 2026 17:57
Comment thread src/sentry/search/eap/resolver.py
Comment thread src/sentry/search/eap/utils.py
Apply API attribute visibility filtering to aggregate and equation operands so hidden attributes cannot be embedded in resolved RPC columns. Reject orderby fallback resolution when the selected column was filtered out for API visibility.

Co-Authored-By: GPT-5 Codex <codex@openai.com>
@nsdeschenes nsdeschenes requested a review from a team as a code owner May 25, 2026 18:34
Comment thread src/sentry/search/eap/resolver.py
Comment thread src/sentry/search/eap/resolver.py Outdated
nsdeschenes and others added 2 commits May 25, 2026 15:54
Reject API-hidden attributes when resolving EAP WHERE terms, HAVING aggregate terms, and nested query combinator filters. This keeps api_attribute_visibility_item_type from exposing internal attributes through filter-only query paths.

Co-Authored-By: Codex <noreply@openai.com>
Reject API-hidden attributes when resolving required EAP attributes. This prevents timeseries group_by requests from silently dropping hidden fields and becoming ungrouped aggregate queries.

Co-Authored-By: Codex <noreply@openai.com>
Comment thread src/sentry/search/eap/resolver.py Outdated
nsdeschenes and others added 3 commits May 26, 2026 07:19
Validate API attribute visibility against remapped storage attributes for virtual contexts. This prevents hidden backing attributes from being exposed through timeseries group-bys and order-bys that use virtual context aliases.

Co-Authored-By: Codex <noreply@openai.com>
…check

Use the same "Could not parse" error message for hidden API attributes
as for genuinely unknown fields, preventing attribute enumeration via
distinct error messages.

Remove the spans-only guard on the dsc./\_internal. prefix expansion in
candidate generation so the visibility check applies to all item types.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@nsdeschenes
Copy link
Copy Markdown
Contributor Author

@sentry review

@nsdeschenes
Copy link
Copy Markdown
Contributor Author

@cursor review

Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit fe11be7. Configure here.

Copy link
Copy Markdown
Member

@wmak wmak left a comment

Choose a reason for hiding this comment

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

small nits but lgtm

Comment thread src/sentry/search/eap/resolver.py Outdated
Comment thread src/sentry/search/eap/resolver.py Outdated
nsdeschenes and others added 2 commits May 26, 2026 14:25
Combine _raise_if_hidden_api_attribute and _raise_if_hidden_resolved_attribute
into one function that accepts both ResolvedAttribute and ResolvedFunction,
doing the isinstance check internally.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit c309e19. Configure here.

if isinstance(resolved_column, ResolvedAttribute) and self._should_hide_api_attribute(
column, resolved_column
):
continue
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hidden fields skipped versus rejected

Medium Severity

With API visibility configured, the same hidden attribute is handled differently by code path: resolve_columns, resolve_functions, and resolve_equations silently drop it, while resolve_attributes, _resolve_term, and aggregate resolution raise HiddenApiAttribute. Users can see empty or misaligned results (dropped series/columns) alongside parse errors for filters, or orderby failures when a hidden name remains in selected_columns but was stripped from resolved output.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit c309e19. Configure here.

@nsdeschenes nsdeschenes merged commit aff6a03 into master May 27, 2026
70 checks passed
@nsdeschenes nsdeschenes deleted the nd/feat-attributes-visibility-core branch May 27, 2026 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants