Skip to content

Enforce metrics view access policy in the metrics_annotations resolver#9705

Merged
nishantmonu51 merged 1 commit into
mainfrom
nishantmonu51/fix-annotations-resolver-access-check
Jul 20, 2026
Merged

Enforce metrics view access policy in the metrics_annotations resolver#9705
nishantmonu51 merged 1 commit into
mainfrom
nishantmonu51/fix-annotations-resolver-access-check

Conversation

@nishantmonu51

Copy link
Copy Markdown
Collaborator
  • The metrics_annotations resolver resolved the metrics view's security policy but never checked CanAccess(), unlike the other metrics-view-backed resolvers (metrics, metricsview_summary, metricsview_time_range, metricsview_cache_key).
  • Since Executor.Annotations does not apply the access check either, a user with instance-level ReadMetrics permission could fetch annotation rows (times, labels, descriptions) for a metrics view whose security.access policy denies them.
  • This adds the missing if !security.CanAccess() { return nil, runtime.ErrForbidden } guard in newAnnotationsResolver, matching the sibling resolvers, so the annotations endpoint now returns a permission error like the other query paths.

Steps to reproduce (before this fix):

  1. Create a metrics view with an annotations: section and a security policy that denies the test user, e.g. security: { access: "{{ .user.admin }}" }.
  2. Deploy and log in as a non-admin user with viewer (ReadMetrics) access to the project.
  3. Control: query the metrics view normally (open its dashboard or POST to /v1/instances/{id}/queries/metrics-views/{name}/aggregation) — returns 403 as expected.
  4. POST to /v1/instances/{id}/queries/metrics-views/{name}/annotations with the annotation measures and a time range.
  5. Before the fix: HTTP 200 with the metrics view's annotation rows despite the denied access policy. After the fix: 403, consistent with the aggregation query.

Checklist:

  • Covered by tests
  • Ran it and it works as intended
  • Reviewed the diff before requesting a review
  • Checked for unhandled edge cases
  • Linked the issues it closes
  • Checked if the docs need to be updated. If so, create a separate Linear DOCS issue
  • Intend to cherry-pick into the release branch
  • I'm proud of this work!

@nishantmonu51

Copy link
Copy Markdown
Collaborator Author

@codex: review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: 46160364ee

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@nishantmonu51
nishantmonu51 merged commit 9367e47 into main Jul 20, 2026
19 of 20 checks passed
@nishantmonu51
nishantmonu51 deleted the nishantmonu51/fix-annotations-resolver-access-check branch July 20, 2026 06:34
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.

2 participants