Repository navigation
feat(deployment): open hidden variables sealed under a team's data key - #4474
Merged
Merged
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
Contributor
|
Warning Review limit reached
This review includes 19 billable files and costs up to $4.75.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 7 minutes for your next included review. View limit details
Comment |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ℹ️ No critical issues — one minor suggestion inline.
Reviewed changes
- Team-sealed token opening in
SdlSecretsService:openStorednow takes the row'sorganizationIdand routes anorg-claimed token to the organization's data key with the{ org, dseq }binding taken from the row; member tokens keep the{ sub, dseq }binding, a team-claimed token on an organization-less row is refused as unreadable, and a member token on a row whose filer is gone stays a 409. - Read paths pass the row's organization: patch preparation, saved-patch apply, the lease manifest, redeploy inheritance and the rekey pass all forward the row's own
organizationId;npx tsc --noEmitplus a repo-wide grep ofopenStoredconfirm no caller was missed. - Inheritance accepts a team-sealed source whose filer deleted their account (
outlivesItsFiler), since the team's key outlives them; anything else with a null filer still answers 404. rekey-user-data-keyleaves team-sealed rows to the team, counting them asdeploymentsSealedForOrganizationahead of the kid checks so they cannot surface asdeploymentsUnderUnknownKey.- Binding and key-ownership semantics verified at the cipher layer: the row (not the header) decides the key owner and the binding, and
SecretCipherServiceenforces exact header-claim equality plus org-ownedkidlookup, so a token moved between rows, users or organizations fails closed rather than decrypting. - Tests: unit specs for sdl-secrets, inheritance, lease-manifest, deployment-writer and rekey pass (556 tests), and the typecheck is clean.
GLM-5.3 ｜ 𝕏
baktun14
force-pushed
the
feat/deployment-open-team-sealed-secrets
branch
from
October 18, 2026 02:32
6197b7a to
6c5144d
Compare
This was referenced Oct 18, 2026
baktun14
force-pushed
the
feat/deployment-open-team-sealed-secrets
branch
from
October 18, 2026 06:10
6c5144d to
2e37d98
Compare
Contributor
There was a problem hiding this comment.
✅ No new issues found — the prior review's runbook finding is fully addressed.
Reviewed changes
Since the previous pullfrog review, the one open finding was closed by 2e37d98 (the rebase to 6338b7c8 left the reviewed patch byte-identical):
- Documented
deploymentsSealedForOrganizationin the re-key runbook: the report-field list now names it and explains that the user's key never sealed those deployments, so they neither move nor hold back the retired key's deletion. - Retargeted §5.1 triage for team-sealed tokens: an operator now compares a team token's
claims.kidagainst the organization'sdata_keysrows (organization_id = claims.org) instead of the user's, so a legitimate team-sealed state no longer reads as tampering. - Carried the team-sealed count into
DATA_KEY_REKEY_BATCH:#countslogssealedForOrganizationmid-run, and the rekey spec asserts it alongside the end-report counts. - Updated the events table's
USER_DATA_KEY_UNWRAPPEDrow to carryuserId, ororganizationIdfor a team's key — verified againstDataKeyUnwrapperService, which spreads the owner into the log.
The new runbook text checks out against the code: the org-claim routing and org-owned data_keys lookup are real, and "a team token whose kid the team does not hold is tampering" holds because no org-side rekey exists to orphan one. The changed rekey spec passes (22 tests).
GLM-5.3 ｜ 𝕏
A stored token whose header names an organization instead of a member opens under the data key and binding of the organization its row is filed in. Patching, redeploying, inheriting and sending the manifest of a team deployment read it that way, including after the member who filed the row deleted their account. A user re-key leaves such a row to the team and counts it apart. Nothing seals under a team's key yet, so every stored token still opens as before. Part of CON-1213
…riage team tokens The re-key's batch log now carries the count of team deployments it leaves to the team's key, as its end report already did. The runbook names that report field and tells the decrypt-failure triage to compare a team token's kid with the organization's data keys rather than the member's. Part of CON-1213
baktun14
force-pushed
the
feat/deployment-open-team-sealed-secrets
branch
from
October 18, 2026 07:10
2e37d98 to
0dc4a8f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4474 +/- ##
==========================================
+ Coverage 93.87% 94.07% +0.20%
==========================================
Files 1722 1613 -109
Lines 49516 46718 -2798
Branches 12117 11565 -552
==========================================
- Hits 46481 43949 -2532
+ Misses 2720 2470 -250
+ Partials 315 299 -16
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
This was referenced Oct 18, 2026
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.
Why
Part of CON-1213. Builds on #4473 (merged).
Every read path for a deployment's hidden variables can now open a token sealed under the team's data key. The writer that seals under it comes in the next PR, behind
team_data_keys, so this PR has to be in prod before that flag is turned on.What
SdlSecretsService.openStoredtakes the row'sorganizationId. A token whose header carries anorgclaim opens under the key and binding ({ org, dseq }) of the organization the row is filed in. A member's token keeps the{ sub, dseq }binding as before. The row decides the owner and the header only says which kind.rekey-user-data-keyleaves team-sealed rows to the team and reports them asdeploymentsSealedForOrganization, in its end report and its batch log.deployment-secrets.md: the re-key report field, and the decrypt-failure triage compares a team token'skidwith the organization'sdata_keysrather than the member's.Rollback floor: once
team_data_keysis on, the API must not roll back below this release. Older versions can't open team-sealed tokens, so patch, redeploy and manifest sends would fail for those deployments.Testing
deployment-patch,deployment-definition,deployment-sealed-secrets,deployments,deployment-patch-in-background,organization-wallet.