fix(express-context): key tenant-DB module discovery by database_id, ship the auth-surface and identity-provider loaders - #1644
Merged
Conversation
…ship auth loaders authSettings and agentChat discovered their module row with an unfiltered LIMIT 1 against the tenant database. In a schema-per-tenant serving database that does not fail — it returns a neighbouring tenant's row, and the loader cache then serves that answer for its whole TTL. Adds the two loaders every auth consumer was hand-writing (auth surface, identity providers), and a structural test asserting every metaschema_modules_public discovery query is filtered by database_id. constructive-planning#1403, constructive-planning#1414
Contributor
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
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.
Summary
constructive-planning#1403reported one unfiltered discovery query. Auditing the rest found a second, and the reason both survived review is that neither fails:FROM metaschema_modules_public.sessions_module sm JOIN metaschema_public.schema s ON s.id = sm.schema_id + WHERE sm.database_id = $1 LIMIT 1In a schema-per-tenant serving database that
LIMIT 1returns whichever tenant the planner reaches first, step 2 then reads that tenant'sauth_settings— cookie domain,SameSite, captcha policy — andcreateModuleLoadercaches the result under the requesting tenant's id for five minutes. Wrong config, no error, sticky.agentChathad the identical shape (agent_chat_module, noWHERE).A context with no
databaseIdnow throws instead of silently running unkeyed:The audit is now a test, not a promise —
__tests__/loaders/tenant-keying.test.tsextracts everyFROM metaschema_modules_public.<module> …span out of the loader sources and asserts each is filtered bydatabase_id = $1, plus that any file containing$1passes a values array (pg acceptsquery(text)with no params happily, so that one is a runtime error rather than a type error). It also asserts it matched ≥6 queries, so a regex that quietly stops matching can't make the whole suite vacuously green. That test is what foundagentChat.Two loaders shipped (
constructive-planning#1414), lifted from the SSO proof where they were hand-written:authSurfaceLoader— the tenant's auth schemas + physical identifier relation names, one round trip. Registered in the default registry. The companion joins are ondatabase_idtoo, since joiningconnected_accounts_moduleby schema id alone can pair one tenant's providers with another's accounts and still return a plausible row.identityProvidersLoader— provider config keyed by slug, with the tenant's own<secrets_table>_get()inlined so a rotated secret is picked up with no coordination. Not in the default registry: three round trips and it decrypts secrets, so it's opt-in for services actually serving an auth flow.The provider shape is the point of shipping it. Deriving it per integration is how one of them ends up not checking
nonce, soissuerUrl,jwks,acceptableClientIds,skipNonceCheckandpkceEnabledare fixed inIdentityProviderConfig, and the two policy flags default to the safe side (skipNonceCheck ?? false,pkceEnabled ?? true) rather than to whatever the row omitted.Failure modes follow the package contract — module row absent →
undefined— with two deliberate exceptions that are configuration faults rather than absence: a provider with noclient_id, and a tenant that provisionsidentity_providers_modulebut nointernal_secrets_module(returning secret-less providers there would present a confidential client as a public one).Not in this PR
constructive-db#2725,#2724) are unmerged branches in the other repo consuming the published@constructive-io/express-context@0.24.x, so they can't drop their copies until this ships. The loaders here are their code, moved, not a reinterpretation of it.graphile/graphile-llm/src/plugins/agent-discovery-plugin.tscarries a third copy of the unkeyedagent_chat_modulequery (DISCOVERY_SQL, also cached per database). Same bug, different package and different cache — flagged rather than fixed here to keep this diff to the loaders.packages/express-context: 27 tests passing, lint and build clean.Link to Devin session: https://app.devin.ai/sessions/087553534c774929918ec4d378845881
Requested by: @pyramation