security(secrets): the #16982 wiring guard passes on a docstring, and the scope pre-check loads legacy rows and 403s a missing secret

Two review findings carried out of #17772, which merged with them open

Both were raised on the PR, both are real, neither blocks the fix it rides with — recorded here rather than lost on a merged PR.

1. The wiring guard can pass on a docstring (the more important one)

repo_tests/secret_access_check_is_wired_16982_test.py proves that Secret.is_accessible_by is reachable from production, which is #16982's whole criterion. Its _references_symbol matches raw text, so a comment or docstring mentioning the symbol satisfies it.

It passes today for the right reason — services/secrets_coordinator.py:131 genuinely calls it. But remove that call and leave a docstring that mentions it, and the guard stays green while the access check is gone. That is precisely the state #16982 exists to end, in the guard that asserts #16982 was delivered.

_references_symbol walks AST nodes rather than matching text.
Contrast pair: a module whose only mention is a docstring or comment must not match; one that calls or forwards it must.

This is one of three sightings of the same class (the others: an assert "EXPIRED" not in source that matched its own explanatory docstring, and a frontend ratchet regex that counted the pattern spelled in a comment). Home for the class is #17769; this instance is the first inside a security guard.

2. The scope pre-check is unfiltered and maps missing to the wrong status

secrets_coordinator.py:157 — secret = await session.get(Secret, secret_id) also loads legacy rows (sealed_value IS NULL), which an envelope-scope pre-check has no business inspecting. And a missing row takes the same branch as a scope refusal (:158), raising SecretAccessError, which the read endpoint maps to 403 where 404 is correct.

Explicitly not an existence leak — missing and refused both return 403, so the response discloses nothing by the difference. It is an over-broad query plus a wrong status.

The pre-check is restricted to envelope rows.
A genuinely missing secret returns 404; a scope refusal returns 403; a test pins both.

Verified as delivered in #17772, for context

_scope_permits is called on both paths (:158 read, :173 list), and the ORGANIZATION tautology is avoided the right way: org_id = secret.org_id if secret.org_id and str(secret.org_id) in facts.company_roles else None — the candidate company comes from the secret and is supplied only when the principal holds it, so resource.company_id == principal.company_id stays meaningful instead of comparing a value with itself.

Refs #16982, #17769, #17773

security(secrets): ask the secret's own scope, not just the vault gra…

A third finding, and the reason the first two were under-rated

This issue says of its two findings: "neither blocks the fix it rides with". Both in fact break main, and there is a third that is worse.

#17772 merged with migration-tests skipped. On its head 36b0799c4f:

success  migration-matrix
skipped  migration-tests

The run rolled up to success, so the Migration Gate read as green while the job that executes these tests never ran. Five pre-existing tests have been failing on main since:

test	failure
`test_secrets_api.py::test_share_then_grantee_reads_then_revoke`	`KeyError: 'value'`
`test_secrets_api.py::test_delete_then_404`	`assert 403 == 404`
`test_secrets_api.py::test_read_missing_404`	`assert 403 == 404`
`test_secrets_coordinator.py::test_share_to_user_then_grantee_reads`	`SecretAccessError`
`test_secrets_coordinator.py::test_delete_then_not_found`	`SecretAccessError`

Observed on run 36696133239, 5 failed, 168 passed.

Finding 3: sharing a secret with a user no longer lets them read it

Not recorded here yet, and the most serious of the three.

coordinator.share authorizes by issuing a SecretGrant to the grantee's own user vault. The scope check consults Secret._grant_lookup, which reads the model's shared_with list. Nothing on the share path ever writes that list, so the two records disagree and a grantee holding a real grant is refused.

The grant is the system of record: without it there is no wrapped DEK and decryption is impossible. shared_with is the derived copy.

A secret shared with a principal by name is readable by them.
The narrowing is exact-match on the principal's own user vault, so security(secrets): Secret.is_accessible_by has no production caller -- the secrets rules it models decide nothing #16982's vault-reachability hole stays closed.
A test covers granted-but-scope-refuses and its contrast pair, neither-granted-nor-scoped.

Why the scope check never saw this

The tests #17772 added cover organization, session, owner and team. None covers a SHARED secret or a per-user grant, and the integration tests that did cover it were the skipped ones. The gap was in the guard's population, not in its logic — the same shape this repo keeps finding (#15826).

Status

#17824 delivers findings 2 and 3 as a base unblock. Finding 1 — the wiring guard matching raw text, so a docstring satisfies it — is deliberately not in it, to keep the unblock minimal. This issue stays open on finding 1.

test(secrets): pin forbidden and not-found as distinct at integration…

test(secrets): assert what the grant lookup binds, not merely that it…

fix(secrets): the scope pre-check broke sharing and 403'd a missing s…

Findings 2 and 3 delivered and verified — #17824 merged as `26ecfab9cd`

Evidence at the merged head 3aa21d2eab, Migration Gate run 36708031720, migration-tests success, 172 passed, 0 failed:

test_share_then_grantee_reads_then_revoke          PASSED
test_delete_then_404                               PASSED
test_read_missing_404                              PASSED
test_share_to_user_then_grantee_reads              PASSED
test_delete_then_not_found                         PASSED
test_a_scope_refusal_is_forbidden_not_not_found    PASSED   (new, the contrast half)
test_the_owner_still_reads_their_own_after_the_precheck  PASSED   (new, non-vacuity)

Finding 2 — ticked

The pre-check is restricted to envelope rows. read now uses the service's own condition, secret is None or secret.sealed_value is None, so legacy rows are not loaded by a check that has no business inspecting them.
A genuinely missing secret returns 404; a scope refusal returns 403; a test pins both. test_delete_then_not_found and test_read_missing_404 pin the not-found side; test_a_scope_refusal_is_forbidden_not_not_found pins the forbidden side against a secret that exists. Pinning only one side is satisfied by collapsing the two the other way, which is what the bug was.

Finding 3 — ticked

A secret shared with a principal by name is readable by them. test_share_to_user_then_grantee_reads, green above.
The narrowing is exact-match on the principal's own user vault. _holds_direct_grant compares SecretGrant.grantee to VaultRef(USER, facts.user_id).to_str(), so reaching a shared, org or team vault does not match and security(secrets): Secret.is_accessible_by has no production caller -- the secrets rules it models decide nothing #16982's hole stays closed.
A test covers granted-but-scope-refuses and its contrast pair. Both in secrets_coordinator_scope_16982_test.py, and after a review thread the granted case also asserts the bound parameters of the lookup, mutation-checked: swapping in a fixed zero UUID fails it.

Finding 1 — still open, and this issue stays open with it

The wiring guard still matches raw text, so a docstring satisfies it. Not touched: a base unblock stays minimal.

Also uncovered, and only half fixed

The Migration Gate's path filter watches where its tests live, not the code they exercise. That is why #17772 skipped. The fix plus a guard that re-derives the list from the tests' actual imports is written and verified locally (621 passed across all 41 repo_tests guards reading .github/workflows) but unpushed: editing a workflow selects ~35 guards and pre-push allows 198s, and both documented exits are refused as safety-bypass flags here. Preserved on branch wip-17822-filter-guard; measurement recorded on #17451.

Until that lands, a change to autobot-backend/services/secrets_*.py alone still will not run these tests.

fix(guards): a call that cannot run is not wiring evidence (#17822, #…

fix(guards): an AST finder for the secrets wiring guard, a reason on …

fix(guards): a call that cannot run is not wiring evidence (#17822, #…

fix(guards): an AST finder for the secrets wiring guard, a reason on …

test(ci): pin the #17822 migration-gate entries from a data file, not…

ci(migration-gate): run the gate on the code its tests exercise, re-d…

Navigation Menu

Repository navigation

security(secrets): the #16982 wiring guard passes on a docstring, and the scope pre-check loads legacy rows and 403s a missing secret #17822

Two review findings carried out of #17772, which merged with them open

1. The wiring guard can pass on a docstring (the more important one)

2. The scope pre-check is unfiltered and maps missing to the wrong status

Verified as delivered in #17772, for context

A third finding, and the reason the first two were under-rated

Finding 3: sharing a secret with a user no longer lets them read it

Why the scope check never saw this

Status

Findings 2 and 3 delivered and verified — #17824 merged as `26ecfab9cd`

Finding 2 — ticked

Finding 3 — ticked

Finding 1 — still open, and this issue stays open with it

Also uncovered, and only half fixed

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

security(secrets): the #16982 wiring guard passes on a docstring, and the scope pre-check loads legacy rows and 403s a missing secret #17822

Description

Two review findings carried out of #17772, which merged with them open

1. The wiring guard can pass on a docstring (the more important one)

2. The scope pre-check is unfiltered and maps missing to the wrong status

Verified as delivered in #17772, for context

Activity

mrveiss commented on Sep 30, 2026

A third finding, and the reason the first two were under-rated

Finding 3: sharing a secret with a user no longer lets them read it

Why the scope check never saw this

Status

mrveiss commented on Sep 30, 2026

Findings 2 and 3 delivered and verified — #17824 merged as 26ecfab9cd

Finding 2 — ticked

Finding 3 — ticked

Finding 1 — still open, and this issue stays open with it

Also uncovered, and only half fixed

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

Findings 2 and 3 delivered and verified — #17824 merged as `26ecfab9cd`