Repository navigation
fix(store): validate workspace names; restore provisioning for admin deploy - #43
Merged
Merged
Conversation
…deploy Two issues found while auditing the permission paths: 1. DorisStore interpolates the workspace name into table names (active_store_<ws>), and only the workspace-create route validated it. A crafted "workspace" query/body field could therefore inject SQL into every store statement (e.g. `UNION SELECT ...` in reads, or a second table in DROP TABLE via workspace/delete). Validate the name as a plain identifier in DorisStore.__init__ and drop_workspace_tables, and skip legacy tables with invalid names during discovery. 2. Example deploy/delete run in a background task created from the admin-cookie route, which never set the provisioning flag. On a fresh install seed() creates the database but not workspace_metadata, so bump_semantic_version() failed with "Unknown table". Mark the admin-only deploy path as allowed to provision before seeding.
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.
Follow-up to #42
#42 merged the DDL gate for ordinary users. Two issues surfaced while auditing the
permission paths afterwards — one pre-existing, one a regression from that change.
1. Workspace name was interpolated into SQL without validation (pre-existing)
DorisStorebuilds table names asactive_store_<workspace>, but only theworkspace-create route validated the name. A crafted
workspacequery/body fieldon
/mcp/web/models,/mcp/web/staging/list,/mcp/web/semantic/files, etc. wasconcatenated into every store statement:
Read paths bypass
validate_readonly(store SQL never goes through sqlglot), andworkspace/deleteallowedname = "x, dw.users"→DROP TABLE IF EXISTS active_store_x, dw.users.Fix: validate the name as a plain identifier in
DorisStore.__init__anddrop_workspace_tables, and ignore legacy tables with invalid names in discovery.2. Example deploy lost DDL rights (regression from #42)
Deploy/delete run in a background task created from the admin-cookie route, which
never set the provisioning flag. On a fresh install
seed()creates the databasebut not
workspace_metadata, sobump_semantic_version()failed with"Unknown table". Mark the admin-only deploy path as allowed to provision.
Tests
test/test_readonly_storage.pynow has 12 cases (invalid names rejected before anyconnection, discovery filters them, admin deploy path can provision, plus the
original non-admin/no-DDL invariants). Full offline suite passes.
Not addressed here
The session-affinity proxy forwards to an IP taken from an unsigned cookie
(SSRF, restricted to
/mcp/web*on the fixed MCP port). Fixing it needs an HMACsecret shared across nodes or a node allow-list — separate decision, separate PR.