Repository navigation
fix: serve the console metrics dashboard to admin sessions only - #727
Open
LeeroyHannigan wants to merge 4 commits into
Open
LeeroyHannigan wants to merge 4 commits into
LeeroyHannigan wants to merge 4 commits into
Conversation
The console metrics feed at /console/metrics-data returned the rows of the shared metrics table to any console session. That table has no account column, so an IAM user of one account read the table names, index names, operations, request counts and latencies of every account. The page put an IAM user's account id in a script variable that no script read, and the server sent every row. The metrics page now redirects an IAM-user session to the console dashboard, and the data feed answers it with HTTP 403 and no metric data before the metrics store or the in-memory collector is read. Admin sessions get the same page and payload as before. The navigation bar shows the Metrics link to admins only, and the unused account variable is removed.
Two accounts each create a table and send it reads and writes. Once the metrics flush has written both tables' rows, as the admin's feed shows, an IAM user of the second account gets HTTP 403 and no table names from /console/metrics-data under four query strings, is redirected from /console/metrics, and has no Metrics link in the navigation bar. The admin gets the page and both accounts' tables. The module waits up to three minutes for the flush, so it carries the slow marker, like the TTL tests that wait.
The architecture guide, admin guide, getting started guide and security model say the metrics dashboard serves admins and covers every account. The upgrade manual records the v0.1.15 behaviour change: IAM users no longer get the dashboard or its data feed. The demo manual says to open the dashboard as the admin user.
…orrect comments The fake metrics store's rows now carry an index name. The IAM-user 403 test checks that the refusal does not contain it, and the admin test checks that the admin's feed reports it as a GlobalSecondaryIndexName dimension. With no index name in the rows, the first check could not fail. The router comments for the metrics page and its data feed, and the comment in the cross-account isolation test, now say both serve admin sessions only. The admin guide's console feature list says an IAM-user session is redirected to the console home page, /console. The metrics example in AGENTS.md passes admin credentials, which /metrics requires.
LeeroyHannigan
requested review from
amrith,
c33howard,
jcshepherd,
pdf-amzn,
robinnsc and
yesyayen
as code owners
November 1, 2026 12:24
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The console metrics dashboard and its data feed now serve admin sessions only.
GET /console/metricsredirects an IAM-user session to/console.GET /console/metrics-dataanswers an IAM-user session with HTTP 403 and{"__type":"AccessDeniedException","message":"Metrics are visible to admin users only"}. The handler returns the refusal before it reads the metrics store or the in-memory collector.identity_labelnow returns the label together with whether the caller is an admin, so the 38nav_barcall sites are unchanged.const accountFilterfor IAM users. The dashboard script never read that variable.Admin sessions get the same payload as before, and the same page without the unused
accountFiltervariable.The architecture guide, admin guide, getting started guide and security model now say the dashboard is admin only and covers every account. The upgrade manual has a v0.1.15 entry for the change, and the demo manual says to open the dashboard as the admin user. AGENTS.md's
/metricsexample now passes admin credentials, which that endpoint requires.Merging with #704 and #734
This PR and #704 (console session revocation) conflict only in the imports of
crates/server/src/console/pages/mod.rs:use crate::console::html;here againstuse crate::console::session::SessionCheck;there. Keeping both lines resolves it. On a trial merge,cargo test -p extenddb-server --lib consolepassed 20 tests, andtests/test_console_metrics_admin_only.pypassed 7 against a SQLite build of the merged tree.#734 (console account pages) adds a sentence to
docs/manuals/10-security-model.mdthat says/console/metrics-dataserves every console session the table metrics of every account until this PR merges. The two branches merge without a conflict, so git does not flag the sentence. Whichever PR merges second removes it: this PR if #734 is onmainfirst, or #734 before it merges if this one is.Why
/console/metrics-datachecked only that the caller had a console session, then returned the rows of themetricstable. That table has no account column, and the store query filters on time, table name and metric only. So any IAM user who could log in to the console read the table names, index names, operations, request counts and latencies of every account. The account id the page set for IAM users stayed in the browser and did not limit what the server sent.Reproduction on main's console code (
f49c5f823), SQLite backend, 2026-11-01:/console/loginand request/console/metrics-data.TableNamevalues, all of them tables of other accounts on the server.?window=LastHourand?window=AllTimereturn the same 18 names.The same steps on this branch return HTTP 403 with the body above and no table names, and
/console/metricsreturns a redirect to/console.Testing done
New tests:
crates/server/src/console/pages/metrics_pages_tests.rs, 5 tests. An IAM-user session gets 403 from the data feed with no table name, index name or operation in the body, and the store is not read. An admin gets both accounts' rows, their index dimension and the latency segments. An IAM-user session is redirected from the page. An admin gets the page withoutaccountFilter. The nav bar has the Metrics link for an admin and not for an IAM user.tests/test_console_metrics_admin_only.py, 7 cases against a server. Two accounts each create a table and send it reads and writes. The fixture waits until the admin's feed shows both tables, which takes up to two flush intervals of 60 seconds. The IAM user of the second account then gets 403 and no table names from the feed under four query strings, a redirect from the page, and no Metrics link. The admin gets the page and both tables.Commands, all on this branch:
cargo +1.97.0 fmt --all -- --check: clean.cargo +1.97.0 clippy --locked -p extenddb-server --all-targets -- -D warnings: clean. With-W clippy::pedanticthe crate has 499 warnings and none of them are in the changed files.cargo test --locked -p extenddb-serveron Rust 1.99.0 and on 1.88.0: 242 passed on each.cargo check --locked --workspace --all-targets: clean.tests/test_console*.py42 passed, including the 7 new cases, andtests/test_metrics.pywithtests/test_cross_account_isolation.py23 passed.Negative controls on the committed tree, each restored with git afterwards:
Checklist
cargo test --workspace)cargo fmt --check)cargo clippy -- -W clippy::pedantic)Storagetrait, auth model, on-diskformat, or public CLI surface, an RFC has been accepted or is linked
below. Otherwise, an ADR captures the decision (link below).
CI ran the workspace tests on Rust 1.88.0 and on stable, and the integration suites on all four backends, and all of them passed. Clippy ran with
-D warnings, without the pedantic group.ADR / RFC: n/a
Breaking changes
IAM users lose the console metrics dashboard. The page redirects them to
/console, the data feed returns HTTP 403, and the navigation bar no longer links it. Admin sessions and the admin-authenticated/metricsendpoint are unchanged.Not covered by this change
metricstable has no account column on any of the four backends, and the collector and the flush worker carry no account dimension. A view of its own metrics for an IAM user needs that column, a migration on each backend, and the account carried from the request through the collector and the flush worker. Without the column, two accounts that use the same table name also share one row, so an admin sees their counts and latencies added together.curl .../metricswithout credentials, which answers 401. docs: correct the quick start, monitoring commands, backend count and design index #668 corrects them.By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache License 2.0 and I agree to the Developer Certificate of
Origin (DCO). See CONTRIBUTING.md for details.