You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
db-migrate: let bootstrap-roles leave excluded relations ungranted - #1310
bootstrap-roles grants the application roles (space_user, space_ro, space_creds) privileges on every relation in schema public, views included,
on every run. A planned rename leaves compatibility views under the old table
names. Before those views are dropped, a brownout step revokes the application
roles' privileges on them so that any remaining reader fails loudly. Without
this change, the next bootstrap-roles run restores those privileges and the
brownout is quietly undone.
This adds GRANT_EXCLUDED_RELATIONS in crates/l30_storage/db/migrate/src/bootstrap_roles.rs: relations bootstrap grants
the application roles nothing on. converge_grants takes the list as a
parameter. Excluded relations are skipped by the per-relation grant loop and by
the space_ro column-scoped re-grant. Revokes still apply to them: the
credential fence, and the column fence's space_ro revoke. Bootstrap logs each
skipped relation at info level. Every SQL statement stays whole; the exclusion
is a Rust-side filter on the existing catalog query's results.
The production list is empty. A brownout adds its views to it. An entry is
removed only after the migration that drops its view has applied everywhere:
provisioning runs bootstrap-roles before migrations, so an image without the
entry would re-grant the view before the drop. Landing the mechanism now keeps
the brownout step to a list change. It does not protect a brownout by itself: a
bootstrap run from any image that lacks the brownout's entry, this one
included, re-grants the views.
An exclusion list is smaller than the alternative of having bootstrap re-grant
only relations it declares. Bootstrap does not declare per-relation grants: it
grants on every relation in public, so new and adopted tables converge with
no list at all. Declaring grants would mean listing every application table.
Validation
On this head, with Docker running and the Rust toolchain from rust-toolchain.toml:
Test-first, grant loop: before the exclusion existed, a test that revoked space_user, space_ro and space_creds on a fixture view
(CREATE VIEW legacy_fixture AS SELECT id FROM renamed_fixture) and then ran
bootstrap failed with space_user must stay revoked on the view. That
confirms bootstrap restores the revoked privileges.
grant_exclusion_keeps_revoked_view_privileges_revoked (a check that first_bootstrap_adopts_admin_owned_tables_routines_and_types runs on its
already bootstrapped server) sends the fixture view through the production converge_grants path with its own exclusion list. It checks that all three roles stay revoked on the view while its base
table is granted, then that a normal bootstrap run (production list) grants
the view again.
Test-first, column fence: grant_exclusion_still_fences_denylisted_columns_from_ro
(the second check run inside the same test) creates, as space_migrator, a view named sessions (an RO_COLUMN_DENYLIST entry) over id and token_hash. Its precondition
checks that space_ro starts with SELECT from the default privileges. With sessions excluded, converge_grants must leave space_ro no column
(has_any_column_privilege) while space_user keeps its table SELECT. A
normal bootstrap run then must grant space_roid but not token_hash.
Before the fix, the column fence skipped excluded relations entirely, and the
test failed with space_ro must lose every column of an excluded denylisted view. It passes after the fix.
The two checks share first_bootstrap_adopts_admin_owned_tables_routines_and_types's
container instead of starting their own. Each bootstrap-roles test starts its
own Postgres container, and as separate tests the column-fence check took
4.1-4.6 s, over the 4 s gate in ci/test-duration-budget.toml. The host test,
listed with a 5 s ceiling, now takes 2.9-3.4 s on the validation machine.
Mutation checks on this head, each restored afterwards. Each one fails first_bootstrap_adopts_admin_owned_tables_routines_and_types, and only that test:
grant loop's skip removed: space_user must stay revoked on the excluded view;
column fence's continue for excluded relations removed (re-grant): space_ro must lose every column of an excluded denylisted view;
excluded relations skipping the column-fence revoke as well: the same message;
bootstrap passing the fixture names instead of GRANT_EXCLUDED_RELATIONS: space_user must be regranted a view that is not excluded.
cargo build -p pplx-space-db-test-fixtures --bin space-nextest-postgres-setup && cargo nextest run -p pplx-space-db-migrate:
run as root as CI's privileged step does (--run-ignored all --ignore-default-filter):
54 tests run, 54 passed, 0 skipped (SPACE_TEST_DEPS_OPTIONAL unset). The
bootstrap Postgres tests ran against real containers.
python3 ci/scripts/check-test-durations.py target/nextest/default/junit.xml ci/test-duration-budget.toml,
6 runs: the host test took 2.9-3.4 s against its 5 s ceiling. Three runs failed
because ro_reads_api_keys_without_key_hash or ro_reads_publication_grant_metadata_without_authorization_id took 5.0-5.3 s
against their 5 s ceilings on that machine; those tests already reach 4.86 s on
main there, and CI set their ceilings from runs of 3.33 s or less.
Not validated: workspace-wide clippy on this head; the full privileged test group
together and the full CI matrix (durations above are from one machine, not CI);
Kind.
Severity and risk
Severity: low on its own, because the list is empty. It is a precondition for a
safe brownout: without it, a routine bootstrap run would silently undo one.
Risk: with an empty list, the grant loop and both fences run exactly as before.
Once a relation is listed, bootstrap stops granting the application roles on
it, so a listed relation that is not actually being retired loses access on its
next revoke; if it is in RO_COLUMN_DENYLIST, space_ro loses access on the
next bootstrap run. Roll back by removing the entry, or by reverting this
commit, and re-running bootstrap-roles.
Rollout
No migration, flag or configuration. With the empty list, every image behaves
as before. Once a brownout adds an entry, a bootstrap run from an image without
that entry re-grants the view. From the brownout's revoke until the view's drop
has applied everywhere, every bootstrap run, a rollback included, must come
from an image that carries the entry. The migration-isolation check rejects
changes to this list in a PR that also changes migrations, so entries are added
and removed in PRs of their own.
AI assistance: An AI agent (Claude Code) wrote the code, the tests and this description.
Why: The exclusion list is currently empty. Activating it changes application-role access, so the privilege-policy rollout requires care.
Commit: 6c44e1c86fee.
No actionable comments were generated in the recent review.
Walkthrough
Bootstrap grant convergence accepts excluded relation names and skips grants for those relations while retaining applicable revokes. PostgreSQL tests cover excluded views, related table grants, and column-scoped access before and after a regular bootstrap.
Possibly related PRs
perplexityai/SPACE#1309: Creates execution-record compatibility views and tests bootstrap-role privileges for them.
Merge Risk: High · up to 6c44e
The exclusion list is currently empty. Activating it changes application-role access, so the privilege-policy rollout requires care.
Security architecture risk: Low · up to 6c44e
The production exclusion list is empty, so this change does not currently alter application access. The implementation preserves the existing credential and column protections. Future exclusions require coordinated revocation and deployment ordering; they are not a durable access-denial mechanism by themselves.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
inferred — The direct authorization scope is the configured control-plane database's public schema and the shared space_user, space_ro and space_creds roles, rather than a tenant-specific policy. Future list entries can suppress grants for matching relations wherever that image runs; the empty production list activates no such exclusions now.
Trust Boundaries and Controls
observed — The new filter does not bypass the credential fence: space_user and space_ro revocations remain outside the skipped grant loop. For excluded column-denylisted relations, the space_ro revoke occurs before the exclusion check suppresses the safe-column re-grant.
Resilience and Maintainability Implications
observed — The added tests assert that a pre-revoked excluded view stays revoked while its backing table receives grants, and that ordinary bootstrap later restores view grants. They also assert retained column fencing for an excluded denylisted view. These cover sequential transitions, not interruption or concurrent recreation.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title starts with the lowercase area "db-migrate:", followed by a lowercase summary that accurately describes the change. It has no trailing period, ticket ID, or internal link.
Description Follows The Pr Template
The PR description contains all required template content: a substantive "## Summary" describing the change, a "## Validation" section naming commands and results, a "## Severity and risk" section wit…
Bug Fixes Name A Regression Test
The PR fixes application behavior under crates/. Its Validation section names grant_exclusion_keeps_revoked_view_privileges_revoked and grant_exclusion_still_fences_denylisted_columns_from_ro, a…
Weakened Safeguards Are Justified
The diff does not remove or weaken a test, assertion, lint, CI check, or repository guard. The only deletions change the converge_grants signature and its call. The new GRANT_EXCLUDED_RELATIONS li…
Public-Safe Content
The title, description, and diff contain no credentials, private keys, internal URLs or hostnames, cloud account IDs, personal absolute paths, production logs, incident details, or undisclosed securit…
Full details: Description Follows The Pr Template
Explanation
The PR description contains all required template content: a substantive "## Summary" describing the change, a "## Validation" section naming commands and results, a "## Severity and risk" section with severity and risk, and an "AI assistance:" line identifying Claude Code and the code, tests, and description it drafted.
Full details: Bug Fixes Name A Regression Test
Explanation
The PR fixes application behavior under crates/. Its Validation section names grant_exclusion_keeps_revoked_view_privileges_revoked and grant_exclusion_still_fences_denylisted_columns_from_ro, and states that each failed before the fix and passed after it. The diff adds both checks and invokes them from the bootstrap test.
Full details: Weakened Safeguards Are Justified
Explanation
The diff does not remove or weaken a test, assertion, lint, CI check, or repository guard. The only deletions change the converge_grants signature and its call. The new GRANT_EXCLUDED_RELATIONS list is empty, and the diff adds two grant-exclusion test checks with assertions.
Full details: Public-Safe Content
Explanation
The title, description, and diff contain no credentials, private keys, internal URLs or hostnames, cloud account IDs, personal absolute paths, production logs, incident details, or undisclosed security vulnerability. The only token-like content is the test identifier token_hash, which is an obviously fake fixture value.
Recent review info
Automatic approval policy
Commit: 6c44e1c86fee.
Setting
Effective value
Source
Automatic approvals
Enabled
Global configuration
Merge risk
Low
Override 2
Security risk
Not set
Global configuration
Matching overrides (numbered as in the resolved configuration):
Any matching disabled override withholds approval, regardless of risk. Otherwise, the strictest explicitly set limit wins for each score. Limits omitted by all matching overrides inherit the global configuration.
Not set means no limit was configured after inheritance. Use @coderabbitai configuration to inspect the resolved settings and their sources.
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 0 remain after this review.
On every run, bootstrap-roles grants the application roles (space_user,
space_ro, space_creds) privileges on every relation in schema public,
views included. A brownout revokes those privileges on a compatibility
view before a migration drops it, so any remaining reader fails loudly.
The next bootstrap run grants them back and quietly undoes the
brownout.
Add GRANT_EXCLUDED_RELATIONS. The per-relation grant loop and the
space_ro column-scoped re-grant skip the relations it lists. Revokes
still apply to them: the credential fence, and the column fence's
space_ro revoke. So when a migration recreates a listed view that is in
RO_COLUMN_DENYLIST, and space_migrator's default privileges grant
space_ro table-level SELECT on it, the next bootstrap run still revokes
that SELECT.
The list is empty. An entry has to ship in every migrator image that can
run bootstrap while its brownout is in effect. It is removed only after
the migration that drops its view has applied everywhere: provisioning
runs bootstrap before migrations, so an image without the entry would
re-grant the view before the drop. Landing the empty list now keeps the
brownout step to a list change. It does not protect a brownout by
itself: a bootstrap run from an image without the entry re-grants the
views.
Two Postgres checks cover the list. They run inside
first_bootstrap_adopts_admin_owned_tables_routines_and_types, on the
server it has already bootstrapped, rather than as tests of their own:
each bootstrap-roles test starts its own Postgres container, and that
alone would put a new test over the Rust test-time budget. The first
check revokes the application roles' privileges on a fixture view, runs
the grant loop with the view listed, and checks that the view stays
revoked while its base table is granted. A bootstrap run with the
production list then grants the view again. The second creates a view
under a column-denylisted name as space_migrator, so space_ro starts
with SELECT on it. With the view listed, converge_grants leaves space_ro
no column of it while space_user keeps its grant. A bootstrap run with
the production list then restores the normal column fence: id is
readable and token_hash is not.
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
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.
Summary
bootstrap-rolesgrants the application roles (space_user,space_ro,space_creds) privileges on every relation in schemapublic, views included,on every run. A planned rename leaves compatibility views under the old table
names. Before those views are dropped, a brownout step revokes the application
roles' privileges on them so that any remaining reader fails loudly. Without
this change, the next
bootstrap-rolesrun restores those privileges and thebrownout is quietly undone.
This adds
GRANT_EXCLUDED_RELATIONSincrates/l30_storage/db/migrate/src/bootstrap_roles.rs: relations bootstrap grantsthe application roles nothing on.
converge_grantstakes the list as aparameter. Excluded relations are skipped by the per-relation grant loop and by
the
space_rocolumn-scoped re-grant. Revokes still apply to them: thecredential fence, and the column fence's
space_rorevoke. Bootstrap logs eachskipped relation at info level. Every SQL statement stays whole; the exclusion
is a Rust-side filter on the existing catalog query's results.
The production list is empty. A brownout adds its views to it. An entry is
removed only after the migration that drops its view has applied everywhere:
provisioning runs
bootstrap-rolesbefore migrations, so an image without theentry would re-grant the view before the drop. Landing the mechanism now keeps
the brownout step to a list change. It does not protect a brownout by itself: a
bootstrap run from any image that lacks the brownout's entry, this one
included, re-grants the views.
An exclusion list is smaller than the alternative of having bootstrap re-grant
only relations it declares. Bootstrap does not declare per-relation grants: it
grants on every relation in
public, so new and adopted tables converge withno list at all. Declaring grants would mean listing every application table.
Validation
On this head, with Docker running and the Rust toolchain from
rust-toolchain.toml:space_user,space_roandspace_credson a fixture view(
CREATE VIEW legacy_fixture AS SELECT id FROM renamed_fixture) and then ranbootstrap failed with
space_user must stay revoked on the view. Thatconfirms bootstrap restores the revoked privileges.
grant_exclusion_keeps_revoked_view_privileges_revoked(a check thatfirst_bootstrap_adopts_admin_owned_tables_routines_and_typesruns on itsalready bootstrapped server) sends the fixture view through the production
converge_grantspath with its own exclusion list. It checks that all three roles stay revoked on the view while its basetable is granted, then that a normal bootstrap run (production list) grants
the view again.
grant_exclusion_still_fences_denylisted_columns_from_ro(the second check run inside the same test) creates, as
space_migrator, a view namedsessions(anRO_COLUMN_DENYLISTentry) overidandtoken_hash. Its preconditionchecks that
space_rostarts with SELECT from the default privileges. Withsessionsexcluded,converge_grantsmust leavespace_rono column(
has_any_column_privilege) whilespace_userkeeps its table SELECT. Anormal bootstrap run then must grant
space_roidbut nottoken_hash.Before the fix, the column fence skipped excluded relations entirely, and the
test failed with
space_ro must lose every column of an excluded denylisted view. It passes after the fix.first_bootstrap_adopts_admin_owned_tables_routines_and_types'scontainer instead of starting their own. Each bootstrap-roles test starts its
own Postgres container, and as separate tests the column-fence check took
4.1-4.6 s, over the 4 s gate in
ci/test-duration-budget.toml. The host test,listed with a 5 s ceiling, now takes 2.9-3.4 s on the validation machine.
first_bootstrap_adopts_admin_owned_tables_routines_and_types, and only that test:space_user must stay revoked on the excluded view;continuefor excluded relations removed (re-grant):space_ro must lose every column of an excluded denylisted view;bootstrappassing the fixture names instead ofGRANT_EXCLUDED_RELATIONS:space_user must be regranted a view that is not excluded.cargo build -p pplx-space-db-test-fixtures --bin space-nextest-postgres-setup && cargo nextest run -p pplx-space-db-migrate:run as root as CI's privileged step does (
--run-ignored all --ignore-default-filter):54 tests run, 54 passed, 0 skipped (
SPACE_TEST_DEPS_OPTIONALunset). Thebootstrap Postgres tests ran against real containers.
python3 ci/scripts/check-test-durations.py target/nextest/default/junit.xml ci/test-duration-budget.toml,6 runs: the host test took 2.9-3.4 s against its 5 s ceiling. Three runs failed
because
ro_reads_api_keys_without_key_hashorro_reads_publication_grant_metadata_without_authorization_idtook 5.0-5.3 sagainst their 5 s ceilings on that machine; those tests already reach 4.86 s on
main there, and CI set their ceilings from runs of 3.33 s or less.
cargo fmt --all -- --check: clean.cargo clippy --locked -p pplx-space-db-migrate --all-targets --all-features -- -D warnings: clean.bash ci/scripts/run-archcheck.sh <base sha> ""(base and its baseline,renames and
ARCHITECTURE.toml, as CI passes them): exit 0, no findings.cargo check --workspace --all-targets --all-features --locked: passed.Not validated: workspace-wide clippy on this head; the full privileged test group
together and the full CI matrix (durations above are from one machine, not CI);
Kind.
Severity and risk
Severity: low on its own, because the list is empty. It is a precondition for a
safe brownout: without it, a routine bootstrap run would silently undo one.
Risk: with an empty list, the grant loop and both fences run exactly as before.
Once a relation is listed, bootstrap stops granting the application roles on
it, so a listed relation that is not actually being retired loses access on its
next revoke; if it is in
RO_COLUMN_DENYLIST,space_roloses access on thenext bootstrap run. Roll back by removing the entry, or by reverting this
commit, and re-running
bootstrap-roles.Rollout
No migration, flag or configuration. With the empty list, every image behaves
as before. Once a brownout adds an entry, a bootstrap run from an image without
that entry re-grants the view. From the brownout's revoke until the view's drop
has applied everywhere, every bootstrap run, a rollback included, must come
from an image that carries the entry. The migration-isolation check rejects
changes to this list in a PR that also changes migrations, so entries are added
and removed in PRs of their own.
AI assistance: An AI agent (Claude Code) wrote the code, the tests and this description.