Repository navigation
fix(platform-objects): cite what enforces client_secret's hashed-at-rest claim - #8354
Merged
os-zhuang merged 1 commit intoAug 13, 2026
Merged
Conversation
…t_secret's hashed-at-rest claim The field description asserted "hashed/encrypted at rest" without citing what enforces it, so a declaration-reading survey could not confirm the claim from the storage side and had to re-measure it — this is the second time (#7902, then #8011). #8011 already verified by real round-trip that the column stores a SHA-256 digest and never receives cleartext; this adds the citation (@better-auth/oauth-provider's storeClientSecret, wired in plugin-auth's AuthManager.buildPluginList() oidcProvider branch) so a third re-measurement is unnecessary. The card's suggested auth-manager.ts:2473 citation had drifted (that line now falls inside the phoneNumber block); the description cites the symbol instead. Also hand-updates the generated English i18n bundle's mirrored "help" string, since check:i18n's merge-mode extractor preserves existing values across regeneration and would not have picked up the source change. Fixes #8313
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 1 package(s): 2 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
os-zhuang
marked this pull request as ready for review
August 13, 2026 08:49
os-zhuang
deleted the
claude/issue-8313-client-secret-description-citation
branch
August 13, 2026 09:06
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.
Fixes #8313
What changed
One field description,
packages/platform-objects/src/identity/sys-oauth-application.object.ts:304(sys_oauth_application.client_secret): the old text asserted "hashed/encrypted at rest" with nothing backing it up. The new text cites what actually enforces it, so a declaration-reading survey can confirm the claim from the storage side instead of re-measuring it a third time (this is the cross-lane remainder of #8011, which already verified by real round-trip that the column stores a SHA-256 digest and never receives cleartext).sys_scim_provideris untouched — it already citesstoreSCIMToken: 'hashed'in its object docstring, which is exactly why the earlier survey scored it near-closed while this field stayed open.Citation drift note
The card's suggested wording named
auth-manager.ts:2473for the wiring site. That line has drifted — it currently falls inside thephoneNumberplugin block, not the OIDC-provider one. The actualjwt+oauthProviderregistration is inAuthManager.buildPluginList()'sif (enabled.oidcProvider)branch, currently aroundauth-manager.ts:2606-2611. The field description now cites the symbol (AuthManager.buildPluginList(), oidcProvider branch) rather than a line number, so it does not go stale the same way.Verified directly against the installed
@better-auth/oauth-provider@1.7.0-rc.2dist (not docs):storeClientSecret()(dist/resource-challenge-2qgK1B-a.mjs) has no cleartext branch — every path hashes, encrypts, or throws (Unsupported storeClientSecret type). The default resolves viaopts.storeClientSecret ?? (opts.disableJwtPlugin ? "encrypted" : "hashed"), and neitherstoreClientSecretnordisableJwtPluginis set anywhere inpackages/, with the jwt plugin unconditionally registered alongsideoauthProvider— so the default resolves to"hashed", matching the existing regression pin (credential-at-rest-posture.test.ts, #8192 / PR #8305).Not comment-only in the narrowest sense — a generated i18n bundle needed a hand-edit
The field
descriptionis extracted intopackages/platform-objects/src/apps/translations/en.objects.generated.ts(thehelpkey underclient_secret), so this is not a pure single-file comment change. Important nuance, worth flagging for the next person who assumes "it's just a comment":pnpm check:i18n/check-i18n-bundles.mjs --writeruns the extractor in merge mode — it treats every already-committed value (English included) as the translation baseline and preserves it across regeneration; it only reacts to added or dropped keys, never to a changed default/description string. Running--writeafter this edit reports "in sync" and touches nothing, becauseclient_secret's key already exists with a value. So the generated English bundle would have silently kept the old, uncited text forever unless hand-edited.Per AGENTS.md's own guardrail table ("Translation values are hand-written and expected to be: … editing a string is fine, while adding or dropping keys is drift"), the fix is a direct hand-edit of the
helpvalue inen.objects.generated.tsto match the new source description verbatim — not a generator run. Done here;check:i18n(self-test + full scan, all 9 gated packages) is green afterward. The three translated locales (es-ES, ja-JP, zh-CN) are independently hand-maintained values under the same key and are deliberately left untouched — they still convey the same underlying fact ("hashed"/"encrypted at rest"), andcheck:i18nnever compares a translation's content against the English source, so leaving them unsynced is not drift.Carried forward for future auditors (not acted on in this PR)
Two⚠️ notes from #8313's body, recorded here because a later audit applying either mechanically would manufacture a false finding on a clean surface:
sys_scim_provider.scim_tokenandsys_oauth_application.client_secret— both are plainField.textand API-readable #8011's acceptance-bar sentence misfires if read literally. It states "a round-trip that reproduces the stored value from the plaintext proves it is not hashed." Read literally that is backwards — unsalted SHA-256 is reproducible from the plaintext by construction (that's how every hash verifier, including better-auth's own, works) — so the literal sentence would score every correctly-hashed credential in the repo as unhashed. The discriminator actually applied was whether the stored value equals the plaintext or is reversible to it; neither holds here (better-auth's only inverse is recompute-and-compare viaconstantTimeEqual, never a decrypt).scim_token's stored digest covers the inner BASE token, not the returned bearer. The bearer handed to the IdP admin isbase64url("BASE:providerId:organizationId"), but the stored digest is overBASEalone. An audit that hashes the full returned bearer and compares will get a false negative on a correct implementation.Scope
Comment-only in behavioural terms — no type,
apiMethods, or other file touched.sys_scim_provideris deliberately untouched (rule 4 on the card): it already carries its citation.Verification
pnpm --filter @objectstack/platform-objects build— green.pnpm --filter @objectstack/platform-objects test— 17 test files, 351 tests, all passing.pnpm check:i18n(self-test + full scan) — green, 9/9 gated packages in sync, no undeclared authoring keys.pnpm lint— green.check:*step in.github/workflows/lint.yml'sESLintjob (43 steps) — all green.node scripts/pm/dispatch-gates.mjs(given the two changed paths) re-derived against the actual diff — surfacescheck:cross-package-test-inputs(already run, green) and otherwise confirms this is apackages/platform-objects-scoped change with no other convention-scoped gate mechanically discoverable.check:i18nitself lives in thetypecheckjob (not named by path-derivation, since the gate walkspackages/dynamically rather than matching a path literal) but was clearly implicated by the diff and run directly, see above.check:i18n-coveragecould not be measured locally — it hard-fails with "1 of 12 configs failed to lint" becauseexamples/app-showcase's config fails to load@objectstack/connector-mcp(no build output in this scoped-build worktree), unrelated to this change. Not chased further per "targeted gates locally, the full farm is CI's job" — CI does a fullpnpm buildfirst.Generated by Claude Code