fix(content-drive): fold folder-scoped candidate resolution into a materialized CTE (#37229) - #37397
Conversation
…terialized CTE (#37229) Restructures BrowserAPIImpl#selectQuery/buildSelectBaseQuery to resolve folder (+ per-case host_inode + fileName) scoping via a 'with candidates as materialized (...)' CTE, joined in place of the raw identifier table, before joining out to contentlet_version_info/structure/ contentlet -- targeting the unstable-planner behavior on large folders (FR-002). Scoped strictly to folder-scoped requests (folder != null && !skipFolder); every other caller of this shared method is byte-identical to before. Also adds a deterministic id.id ORDER BY tiebreaker (FR-001) so tied mod_date rows sort reproducibly. UNVALIDATED against FR-010's mandatory EXPLAIN ANALYZE gate -- no live Postgres/reference dataset in this environment. Explicit, developer- accepted risk (see specs/37229-content-drive-folder-cte/tasks.md T004). This is the single most important outstanding verification step before this change can be trusted. Tests: tiebreaker determinism, lineage-independent index-column validation (pg_index catalog query, not index name), empty/small-folder no-regression, two of the five R3 site-scoping paths (the other three were not constructible via the public BrowserQuery builder in this pass -- flagged, not faked), and permission scoping through the new CTE path.
|
Claude finished @ihoffmann-dot's task in 2m 38s —— View job Code Review — PR #37397 (folder-scoped candidate CTE)
I focused on bugs this diff introduces. The three dotbot reviewers already confirmed the predicate semantics and binding order; I independently re-verified the parameter-ordering and column-alias concerns and agree the production change is correct. New Issues(none blocking)
Verified correct (no change needed)
Resolved
Notes on disclosed gaps (not blocking, already acknowledged in the PR body)
Verdict: No new correctness/security bug in the production change. The only open code-level item is the |
![semgrep-dotcms[bot]](https://avatars.githubusercontent.com/u/1005263?s=60&v=4){kind=small}
| final StringBuilder baseQuery = new StringBuilder(candidatesCte) | ||
| .append("select cvi.").append(workingLiveInode).append(" as inode ").append(baseClause); |
There was a problem hiding this comment.
Semgrep identified a blocking 🔴 issue in your code:
baseQuery concatenates workingLiveInode and candidatesCte into SQL syntax without constraining their contents. An attacker who controls either value may inject SQL clauses, bypass joins and filters, or read unintended tables.
More details about this
baseQuery builds SQL by appending workingLiveInode directly after cvi. and also embeds candidatesCte as the query prefix. These values are treated as SQL syntax rather than data; if either can be influenced by a request or another untrusted source, an attacker can alter the query structure instead of selecting only the intended inode column.
For example, if an attacker can reach workingLiveInode through a query parameter, they could submit a value such as live_inode FROM sensitive_table --. StringBuilder would produce select cvi.live_inode FROM sensitive_table -- as inode ...; the -- comments out the remaining SQL, allowing the attacker to change the table being read and bypass the intended joins and filters. Similarly, an attacker-controlled candidatesCte such as WITH candidates AS (...) could inject arbitrary CTE SQL before the select assembled by baseQuery, potentially exposing or altering data through the resulting query.
The same dynamic identifier is inserted again in baseClause (cvi. + workingLiveInode), so one attacker-controlled value changes multiple parts of the generated statement. The risk is present even though the matched text is the literal "select cvi.": the following .append(workingLiveInode) makes the complete SQL statement dynamic.
To resolve this comment:
✨ Commit fix suggestion
| final StringBuilder baseQuery = new StringBuilder(candidatesCte) | |
| .append("select cvi.").append(workingLiveInode).append(" as inode ").append(baseClause); | |
| final String validatedWorkingLiveInode; | |
| if ("live_inode".equals(workingLiveInode) || "working_inode".equals(workingLiveInode)) { | |
| validatedWorkingLiveInode = workingLiveInode; | |
| } else { | |
| throw new IllegalArgumentException("Invalid working live inode identifier"); | |
| } | |
| if (UtilMethods.isSet(candidatesCte)) { | |
| throw new IllegalArgumentException("Untrusted candidates CTE"); | |
| } | |
| final StringBuilder baseQuery = new StringBuilder() | |
| .append("select cvi.").append(validatedWorkingLiveInode).append(" as inode ").append(baseClause); |
View step-by-step instructions
-
Validate
workingLiveInodeagainst a fixed allowlist of column names before appending it to the query. Do not use the raw method argument as a SQL identifier; reject any value that is not an expected identifier such aslive_inodeorworking_inode. -
Restrict
candidatesCteto SQL fragments generated by this application. Prefer selecting between fixed query fragments, for example""and a predefined CTE constant, instead of accepting arbitrary SQL text from a caller. -
Keep SQL structure in fixed constants and append only validated identifiers or trusted fragments. For example, build the query from a fixed
SELECTtemplate after validatingworkingLiveInodeandidentifierSource, rather than allowing untrusted text to reachnew StringBuilder(...). -
Validate
baseTypesas numeric values derived from theBaseContentTypeenum before appending them. Do not append arbitrary strings to theINclause. -
Parameterize
contentTypeIdsandexcludedContentTypeIdsinstead of concatenating them inside quoted SQL. Generate placeholders such as:contentTypeId0, bind each ID through the query’s existingparamsmechanism, and append only the placeholders to the SQL. -
Apply the same validation and parameter binding to every later
baseQuery.append(...)operation so the completed query contains only fixed SQL syntax, validated identifiers, placeholders, and bound values. This prevents input values from being interpreted as SQL code.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by CUSTOM_INJECTION-2.
If this is a critical or high severity finding, please also link this issue in the #security channel in Slack.
You can view more details about this finding in the Semgrep AppSec Platform.
There was a problem hiding this comment.
/fp Neither concatenated value is attacker-controlled. workingLiveInode is assigned from a fixed ternary (browserQuery.showWorking || browserQuery.showArchived ? "working_inode" : "live_inode") — it can never be anything other than those two hardcoded column names, and this concatenation pattern already existed before this PR. candidatesCte is assembled entirely from fixed SQL text plus parameterized predicates: every append helper that contributes to it (appendFolderQuery, appendSiteQuery, appendFileNameQuery) binds values via ? placeholders into the parameters list, never inlining caller input into the SQL string. No request data reaches either value as syntax.
There was a problem hiding this comment.
Status updated to ignored - false positive by @ihoffmann-dot. Re-run the pipeline to make this finding non-blocking.
Reply with /open to re-open this finding
…sion-scoping test (#37229) - test_getPaginatedContents_folderScopedCte_permissionScopingUnchanged reused the shared TestUserUtils.getChrisPublisherUser fixture (a singleton looked up by hardcoded email across the whole suite) and granted it new permissions, which polluted a pre-existing, unrelated test in this same file (test_getContent_Using_LimitedUser_WithRead_ Permissions) that also uses it. Switched to a dedicated, freshly created user, matching test_exhaustive_pagination_with_permission_ filtering's proven pattern. - Folder-level READ does not propagate to newly created content by inheritance in this flow -- the 'readable' contentlet needs its own explicit permission grant, same as the working reference test. - An extra permissionIndividually() call on the 'restricted' contentlet did not actually block read access as intended; removed it in favor of the proven pattern (an explicit permission entry for a role the user doesn't have, with no additional individually() call). - Added two doesUserHavePermission sanity-check assertions ahead of the getPaginatedContents call, isolating permission-setup issues from the CTE candidate-scan path this PR actually changes -- neither failure found here was in that path. Confirms none of the three bugs found were in the CTE/tiebreaker fix itself (#37229's actual production change) -- all were in this test's own permission fixture setup.
…ped requests only (#37229) Found in code review (claude[bot] on PR #37397): appendOrderByQuery unconditionally added the id.id tiebreaker for any caller with sortBy set, not only useFolderCte requests. FR-001 scopes this guarantee to 'every folder-scoped listing request' -- every other caller's ORDER BY must stay byte-identical to before, matching this PR's own stated blast-radius claim.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (fileNameHandledByDb) { | ||
| appendFileNameQuery(candidatesPredicates, browserQuery.fileName, parameters); | ||
| } | ||
| candidatesCte = "with candidates as materialized (select * from identifier id where 1=1 " |
There was a problem hiding this comment.
🟡 [P2] BrowserAPIImpl.java:2044 Postgres-only AS MATERIALIZED CTE breaks non-Postgres dialects
Current code:
candidatesCte = "with candidates as materialized (select * from identifier id where 1=1 "
+ candidatesPredicates + ") ";Problem: WITH ... AS MATERIALIZED is PostgreSQL-only syntax; other dialects in the repo's support surface (MySQL/MSSQL/Oracle — see dotcms-integration/src/test/resources/mysql-db-config.properties, mssql-db-config.properties, oracle-db-config.properties, and DbConnectionFactory.isMySql()/isMsSql()/isOracle()) will reject it with a syntax error on every folder-scoped listing request. This file already branches on DbConnectionFactory.isPostgres() for dialect-specific SQL (BrowserAPIImpl.java:2805). MATERIALIZED is only a planner hint here; a plain with candidates as (...) is portable and semantically equivalent for this fix.
Fix:
candidatesCte = "with candidates as (select * from identifier id where 1=1 "
+ candidatesPredicates + ") ";Assumption: non-Postgres backends are still supported deployments; the repo's driver/db-config files and DbConnectionFactory dialect branches suggest so. What to verify: whether the product still officially supports MySQL/MSSQL/Oracle; if not, downgrade to P3. (If Postgres-only behavior must be preserved, gate the MATERIALIZED keyword on DbConnectionFactory.isPostgres().)
|
dotbot code review:
The CTE refactor preserves the original predicate guard conditions and parameter-binding order with no dropped or duplicated predicates, the folder/site/fileName scoping moves into a materialized CTE correctly aliased as Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads. reviewed by dotbot · deepseek/deepseek-v4-pro-0813 · medium |
|
dotbot code review:
Folder/site/fileName predicates move into the materialized CTE with matching placeholder and parameter order and identical guard conditions; non-folder-scoped SQL stays byte-identical and the id.id tiebreaker is scoped to folder-scoped requests only. No P0/P1 bug introduced by this patch. Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads. reviewed by dotbot · meta/muse-spark-1.3 · medium |
| if (fileNameHandledByDb) { | ||
| appendFileNameQuery(candidatesPredicates, browserQuery.fileName, parameters); | ||
| } | ||
| candidatesCte = "with candidates as materialized (select * from identifier id where 1=1 " |
There was a problem hiding this comment.
⚪ [P3] BrowserAPIImpl.java:2044 gate Postgres-only as materialized on DbConnectionFactory.isPostgres()
Current code:
candidatesCte = "with candidates as materialized (select * from identifier id where 1=1 "
+ candidatesPredicates + ") ";Problem: AS MATERIALIZED is Postgres-specific syntax and fails to parse on other SQL dialects.
Fix:
final String materializedKeyword = DbConnectionFactory.isPostgres() ? " materialized" : "";
candidatesCte = "with candidates as" + materializedKeyword + " (select * from identifier id where 1=1 "
+ candidatesPredicates + ") ";Assumption: dotCMS appears Postgres-only in practice; this same query path already emits Postgres-only SQL unconditionally (contentlet_as_json::text ILIKE at line 2289, POSTGRES_BINARY_ASSETNAME_COLUMN at line 2824), so this adds little new portability risk. What to verify: whether any non-Postgres backend remains a supported deployment. If Postgres-only is confirmed, this can be dropped entirely.
|
dotbot code review:
The CTE refactor preserves predicate semantics, parameter binding order (folder/site/fileName parameters are added before buildSelectBaseQuery and the CTE text is prepended, keeping placeholder and parameter order aligned), and original guard conditions; the id.id ORDER BY tiebreaker is correctly scoped to folder-scoped requests only, leaving all other callers byte-identical. The only open item is the Postgres-only Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads. reviewed by dotbot · ~z-ai/glm-latest · medium |
fabrizzio-dotCMS
left a comment
fabrizzio-dotCMS
left a comment
There was a problem hiding this comment.
Reviewed against specs/37229-content-drive-folder-cte/spec.md.
The FR-010 disclosure at the top of the description, and then the follow-up validation with real EXPLAIN ANALYZE and pg_stat_statements attribution on the 418k-contentlet reference dataset, is the right way to handle this. Recording that SC-002's "+25-30ms" was optimistic and updating the spec to +25-40ms rather than quietly letting the original number stand is worth more than the fix itself. Two things I checked and found sound:
- Parameter binding order is correct. The CTE text is prepended to the query while its parameters (folder, then site, then fileName) are appended to
parametersfirst, so positional order still matches: CTE predicates, then workflow, then filter. Easy thing to get wrong in a change that moves predicates to the front of a statement. - The "byte-identical for non-folder-scoped callers" claim holds.
shouldApplySiteFilteringalready requiresfolder != null, so the!useFolderCtebranch is only reachable withskipFolder=true, and thefolder == nullcase appends no site predicate either way — same as before. Scoping theid.idtiebreaker touseFolderCtein 7bcb119 was the right correction; unconditional would have changed tie order and pagination cursors for every other caller.
Three code findings inline, plus one on mechanics:
Base branch. This targets issue-37229-content-drive-folder-cte, not main, while the three sibling implementation PRs (#37394, #37395, #37396) all target main. As it stands, merging this ships nothing to main, and CI is running against a base that isn't main — so a green build here says less than a green build on those. Worth retargeting once #37230's spec update is re-approved, or at least saying explicitly that the merge order is #37230 first and then a rebase.
| if (fileNameHandledByDb) { | ||
| appendFileNameQuery(candidatesPredicates, browserQuery.fileName, parameters); | ||
| } | ||
| candidatesCte = "with candidates as materialized (select * from identifier id where 1=1 " |
There was a problem hiding this comment.
select * materializes every column of identifier for every row in the folder, and the outer query needs almost none of them.
I grepped the whole generated statement for id.-qualified references. Outside the CTE there are exactly two: id.id (the join to cvi.identifier, the id.id in (...) tag subquery, and the new ORDER BY tiebreaker) and id.asset_subtype (the join to struc.velocity_var_name). The other three columns the CTE touches — parent_path, host_inode, asset_name — appear only inside the CTE's own where, so they need to be scanned but not projected.
Everything else identifier carries is materialized into the work table and never read: asset_type, owner, create_date, syspublish_date, sysexpire_date, full_path_lc. On /outreach/ that's 21,383 rows' worth of columns written to a work table to serve two of them.
This looks like the source of the trade-off the re-check found. On /uploads/news/ you measured +40ms with buffer counts nearly identical before and after, and concluded the delta is materialization overhead rather than extra data reads — which is exactly what a narrower projection reduces. Worth re-running that same measurement with:
with candidates as materialized (
select id.id, id.asset_subtype from identifier id where 1=1 ...
)If the +40ms shrinks meaningfully, SC-002 gets better rather than being documented as a cost. Since /uploads/news/ is all file assets and exposed the largest delta, that is the right folder to test it on — which also lines up with the re-check's own recommendation to add that shape to the permanent regression matrix.
| sqlQuery.append(" order by "); | ||
| if (orderByDesc) { | ||
| sqlQuery.append(" c.mod_date desc"); | ||
| sqlQuery.append(" c.mod_date desc").append(useFolderCte ? ", id.id desc" : ""); |
There was a problem hiding this comment.
(c.mod_date, id.id) is still not a total order, so FR-001's determinism guarantee has a hole left in it.
appendLanguageQuery emits cvi.lang in (...) over however many language ids the request carries, and showDefaultLangItems can add the default language on top of that. Nothing in the statement restricts the result to one row per identifier — the join is cvi.identifier = id.id with cvi.lang free — so a single identifier legitimately comes back as several rows, one per language.
For those rows id.id is identical. When they also share a mod_date (which they routinely do: a multi-language contentlet's versions are often saved together), the sort has nothing left to disambiguate them by, and the order is planner-dependent again — the precise condition the comment above says this tiebreaker converts into "a deterministic, reproducible-run-to-run guarantee". On a single-language folder the guarantee holds; on a multi-language one it does not.
Adding cvi.lang closes it, and it's already in scope with no new join:
sqlQuery.append(" c.mod_date desc").append(useFolderCte ? ", id.id desc, cvi.lang desc" : "");The existing tie test wouldn't catch this — it creates two file assets in the default language, so their id.id values differ and the tiebreaker resolves them. A version of it with the same identifier in two languages, both forced to one mod_date, would.
| .collect(Collectors.toList()); | ||
|
|
||
| assertEquals("Both tied rows must be present", 2, firstRunOrder.size()); | ||
| assertEquals("Order among tied mod_date rows must be reproducible run-to-run", |
There was a problem hiding this comment.
This proves run-to-run reproducibility within a single page, which is real but is the weaker half of FR-001. The half that matters operationally — deep-pagination cursor stability, listed under Known gaps as having no test — is what the tiebreaker actually buys, and this fixture is already most of the way there.
The mechanism worth pinning: getContentByChunks pages by DB offset (buildPaginatedDotConnect(sqlQuery, chunkSize, dbOffset)), so with a non-total ORDER BY two requests for adjacent pages can order the tied rows differently and the boundary row is then either returned twice or skipped entirely. At the ~1.2% tie rate #37148 measured, that is not exotic on a 21k folder. Before this fix the ordering had nothing to disambiguate ties; after it, page boundaries are stable — that is the guarantee, and nothing currently fails if it regresses.
What would cover it, reusing this test's forced-tie approach: create enough contentlets in one folder to span several pages, force an identical mod_date across all of them with the same DotConnect update you use here, then page through with a small maxResults, collecting identifiers. Assert the union has no duplicates and its size equals the unpaged result count. That fails loudly on any non-total order and passes deterministically with the tiebreaker in place.
3 participants
This spec's own plan makes running
EXPLAIN ANALYZEagainst a real dataset (with workflow,tag/relationship sub-queries, and the new tiebreaker in play) a mandatory gate before the query
shape is trusted — explicitly called "the single highest-risk unknown in this spec." That
validation requires a live Postgres instance with realistic data volume, which was not available
in the environment this PR was written in. This PR was written and pushed without that
validation, as an explicit, disclosed risk — see
specs/37229-content-drive-folder-cte/tasks.mdtask T004/T023, both left unchecked.
Do not merge without running the FR-010 EXPLAIN ANALYZE gate locally first. If it doesn't
hold, the query-shape tasks (see below) need rework, not just the tests.
Summary
BrowserAPIImpl#selectQuery/buildSelectBaseQueryto resolve folder (+ per-casehost_inode+fileName) scoping via awith candidates as materialized (...)CTE, joined in place of the rawidentifiertable, before joining out tocontentlet_version_info/structure/contentlet(FR-002).folder != null && !skipFolder) — every other caller of this shared method is byte-identical to before, by design, to contain the blast radius of an unvalidated change.id.idORDER BYtiebreaker (FR-001) so rows sharing the samemod_datesort reproducibly.Known gaps
ignoreSiteForFolders=true) were exercised with a test — the other three require constructing aBrowserQuerywithsite == null, which isn't reachable through the publicwithHostOrFolderIdbuilder path as read in this pass. Flagged rather than faked.Test plan
EXPLAIN ANALYZEper quickstart.md against a folder confirmed to trigger today's slow plan, with the full predicate set (workflow, tags, tiebreaker)just test-integration-ide./mvnw verify -pl :dotcms-integration -Dcoreit.test.skip=false -Dit.test=BrowserAPITestSystem.out/System.getProperty/System.getenvintroduced (checked via diff)Branched off the approved spec branch per this repo's Spec-Kit flow (spec.md-only in PR1, not merged to
mainyet).🤖 Generated with Claude Code
This PR fixes: #37229
Verification (2026-09-04, local) — correctness tests only, FR-010 still NOT run
BrowserAPIImplTest) pass.BrowserAPITest) pass.permissionIndividually()call that didn't block access as intended. Confirmed via twodoesUserHavePermissionsanity checks isolating the permission setup from the candidate-scan path this PR actually touches.EXPLAIN ANALYZEgate) is still not run. These tests confirm the CTE produces correct results; they say nothing about the query plan / latency claim, which is the actual point of this fix and requires a real dataset with a folder already confirmed to trigger today's slow plan. Still do not merge without running it.FR-010 — VALIDATED (2026-09-05)
Re-checked against
dotcms/dotcms:issue-37229-content-drive-folder-cte_SNAPSHOT(commit95b6031025) on the original #37148/#37183 reference dataset (418k contentlets), using realEXPLAIN ANALYZE+pg_stat_statementsattribution — not a simplified query.Result: the fix works where it matters.
/outreach/(21,383 children, the pathological case from Content Drive: folder listing query scans by mod_date instead of by folder (450ms to 101ms), plus 3 related performance items #37148): plan now enters viaidentifier_parent_path_asset_name_host_inode_keyinstead ofidx_contentlet_mod_date. Buffer hits: ~909k baseline → 90,351. Per-request DB-time attribution: 272ms → 114ms (~2.4x).Trade-off found, spec updated: on folders where the pre-fix plan was already good, the materialized CTE adds a real (not catastrophic) overhead purely from materialization. The spec's documented "+25-30ms" (SC-002) was optimistic — a second reference folder (
/uploads/news/, 7,154 all-file children) measured +40ms (35-37ms → 75-77ms, ~2.1x), with buffer counts nearly identical before/after confirming the delta is materialization overhead, not extra data reads.specs/37229-content-drive-folder-cte/spec.md's SC-001/SC-002 were updated to +25-40ms and to record this confirmation (pushed to the spec branch, PR #37230 — needs re-approval since the spec changed after sign-off).Known limitation of this re-check: row-count parity was verified (40/40 in every case, matching pre-fix), but not row-by-row ID parity against the old code path, since the old code is no longer deployed and redeploying it for this specific diff was judged disproportionate. Recommendation from the re-check: add a large, all-file-asset folder shape (like
/uploads/news/) to the permanent regression matrix, since it exposed a bigger delta than the original single reference case.Verdict: fix confirmed to do what it claims, no correctness regression, overhead trade-off is bounded and now accurately documented. Recommend merge once PR #37230's spec update is re-approved.