Skip to content

fix(content-drive): fold folder-scoped candidate resolution into a materialized CTE (#37229) #37397

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ihoffmann-dot wants to merge 9 commits into
base: issue-37229-content-drive-folder-cte
Choose a base branch
from
+515 −25
Open

fix(content-drive): fold folder-scoped candidate resolution into a materialized CTE (#37229) #37397

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b5d1935
fix(content-drive): fold folder-scoped candidate resolution into a ma…
ihoffmann-dot Sep 4, 2026
0ffff94
fix(content-drive): fix real bugs found running BrowserAPITest permis…
ihoffmann-dot Sep 5, 2026
95b6031
chore: trigger CI re-run (retry Semgrep scan for #37229)
ihoffmann-dot Sep 5, 2026
7bcb119
fix(content-drive): scope the id.id ORDER BY tiebreaker to folder-sco…
ihoffmann-dot Sep 8, 2026
e84f188
chore: trigger CI re-run (retry Semgrep scan for #37229, after /fp tr…
ihoffmann-dot Sep 8, 2026
f1de847
chore: trigger CI re-run (retry Semgrep scan for #37229, FP marked in…
ihoffmann-dot Sep 9, 2026
9684ca4
fix(content-drive): narrow folder-scoped CTE SELECT to id.id, id.asse…
ihoffmann-dot Sep 10, 2026
d990c6d
fix(content-drive): add cvi.lang to the folder-scoped ORDER BY tiebre…
ihoffmann-dot Sep 10, 2026
510cdd0
test(content-drive): cover deep-pagination cursor stability for tied …
ihoffmann-dot Sep 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 104 additions & 25 deletions dotCMS/src/main/java/com/dotcms/browser/BrowserAPIImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2008,31 +2008,76 @@ private SelectQuery selectQuery(final BrowserQuery browserQuery) {
final String workingLiveInode = browserQuery.showWorking || browserQuery.showArchived ?
"working_inode" : "live_inode";

final StringBuilder selectQuery = new StringBuilder(buildSelectBaseQuery(browserQuery, workingLiveInode));

final List<Object> parameters = new ArrayList<>();

// issue #37229: fold folder (+ per-case host_inode + fileName) scoping into a materialized
// CTE, resolved BEFORE this query joins out to contentlet_version_info/structure/
// contentlet -- instead of joining the full `identifier` table first and filtering
// afterward, which is the source of the unstable-planner behavior on large folders
// (FR-002). Scoped ONLY to the folder-scoped case this fix targets: this shared method's
// behavior is byte-identical to before for every caller that does not scope by folder
// (folder == null, or skipFolder=true) -- forcing materialization of the full identifier
// table with no scoping predicate would be a regression, not a fix, for those callers.
// NOT validated against EXPLAIN ANALYZE with the real predicate set (FR-010) -- flagged
// as an explicit, developer-accepted risk; see PR description.
final boolean useFolderCte = browserQuery.folder != null && !browserQuery.skipFolder;
// Handle site filtering based on ignoreSiteForFolders flag
final boolean shouldApplySiteFiltering = !browserQuery.ignoreSiteForFolders && browserQuery.folder != null;
final boolean fileNameHandledByDb = !browserQuery.useElasticsearchFiltering
&& UtilMethods.isSet(browserQuery.fileName);

String candidatesCte = BLANK;
if (useFolderCte) {
final StringBuilder candidatesPredicates = new StringBuilder();
appendFolderQuery(candidatesPredicates, browserQuery.folder.getPath(), parameters);
if (shouldApplySiteFiltering) {
if (browserQuery.site != null) {
appendSiteQuery(candidatesPredicates, browserQuery.site.getIdentifier(),
browserQuery.forceSystemHost, parameters);
} else if (browserQuery.forceSystemHost) {
appendSystemHostQuery(candidatesPredicates);
}
}
if (fileNameHandledByDb) {
appendFileNameQuery(candidatesPredicates, browserQuery.fileName, parameters);
}
// Only project what the outer query actually reads through the `candidates` alias
// (`id.id` and `id.asset_subtype`, verified by grepping every `id.`-qualified
// reference outside this CTE) -- `parent_path`, `host_inode` and `asset_name` are
// still scanned (they're referenced in the predicates below) but not materialized,
// and everything else `identifier` carries (asset_type, owner, create_date,
// syspublish_date, sysexpire_date, full_path_lc, ...) is now skipped entirely instead
// of being written into the work table for every row in the folder (code review,
// PR #37397).
candidatesCte = "with candidates as materialized (select id.id, id.asset_subtype "

@github-actions github-actions Bot Sep 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚪ [P3] BrowserAPIImpl.java:2052 gate Postgres-only as materialized on DbConnectionFactory.isPostgres()

Current code:

candidatesCte = "with candidates as materialized (select id.id, id.asset_subtype "
        + "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 id.id, id.asset_subtype "
        + "from identifier id where 1=1 " + candidatesPredicates + ") ";

Assumption: this code path already emits Postgres-only SQL unconditionally (contentlet_as_json::text ILIKE at line 2292, POSTGRES_BINARY_ASSETNAME_COLUMN at line ~2827), so the blast radius is unchanged. What to verify: whether any non-Postgres backend remains a supported deployment; if Postgres-only is confirmed, this can be dropped entirely.

+ "from identifier id where 1=1 "
+ candidatesPredicates + ") ";
}

final StringBuilder selectQuery = new StringBuilder(
buildSelectBaseQuery(browserQuery, workingLiveInode, candidatesCte));

if (!browserQuery.languageIds.isEmpty()) {
appendLanguageQuery(selectQuery, browserQuery.languageIds,
browserQuery.showDefaultLangItems);
}
// Handle site filtering based on ignoreSiteForFolders flag
final boolean shouldApplySiteFiltering = !browserQuery.ignoreSiteForFolders && browserQuery.folder != null;

if (shouldApplySiteFiltering) {
if (browserQuery.site != null) {
appendSiteQuery(selectQuery, browserQuery.site.getIdentifier(),
browserQuery.forceSystemHost, parameters);
} else {
if (browserQuery.forceSystemHost) {
appendSystemHostQuery(selectQuery);
if (!useFolderCte) {
// Pre-existing shape, unchanged: no folder scopes this request (or skipFolder=true),
// so there is nothing for the CTE above to target -- site/host filtering (independent
// of skipFolder) still applies directly against `identifier` exactly as before this
// fix. (The folder predicate itself is never appended here: useFolderCte's negation
// means folder == null || skipFolder, the same condition that gated it originally.)
if (shouldApplySiteFiltering) {
if (browserQuery.site != null) {
appendSiteQuery(selectQuery, browserQuery.site.getIdentifier(),
browserQuery.forceSystemHost, parameters);
} else {
if (browserQuery.forceSystemHost) {
appendSystemHostQuery(selectQuery);
}
}
}
}
//This property allows the exclusion of the folder in the base query
if (browserQuery.folder != null && !browserQuery.skipFolder) {
appendFolderQuery(selectQuery, browserQuery.folder.getPath(), parameters);
}
// Detect archive-target steps once per request (cached WorkflowAPI lookups, never per row).
// Only step-pinned entries can be archive-target; scheme-only entries always stay live-only.
// Skipped when archived rows are already admitted, so the archive-step logic must not run
Expand All @@ -2055,7 +2100,11 @@ private SelectQuery selectQuery(final BrowserQuery browserQuery) {
if (UtilMethods.isSet(browserQuery.filter)) {
appendFilterQuery(selectQuery, browserQuery.filter, parameters);
}
if (UtilMethods.isSet(browserQuery.fileName)) {
// fileNameHandledByDb is true under the exact same condition this block already
// guards (isSet(fileName), not using ES) -- when useFolderCte, it was already folded
// into the candidates CTE above (resolved scoping decision, research.md); appending
// it again here would be redundant, not incorrect, but is skipped for clarity.
if (fileNameHandledByDb && !useFolderCte) {
appendFileNameQuery(selectQuery, browserQuery.fileName, parameters);
}
}
Expand Down Expand Up @@ -2083,7 +2132,7 @@ private SelectQuery selectQuery(final BrowserQuery browserQuery) {
appendMIMETypeQuery(selectQuery, browserQuery.mimeTypes);
}
if (null != browserQuery.sortBy) {
appendOrderByQuery(selectQuery, browserQuery.sortByDesc);
appendOrderByQuery(selectQuery, browserQuery.sortByDesc, useFolderCte);
}

Logger.debug(this, "Select Query: " + selectQuery);
Expand All @@ -2108,17 +2157,28 @@ static class SelectQuery {
*
* @param browserQuery The {@link BrowserQuery} object specifying the filtering criteria.
* @param workingLiveInode The identifier of the working live inode.
* @param candidatesCte Issue #37229: when set, a {@code with candidates as materialized
* (...)} clause that pre-resolves the folder-scoped candidate set
* (parent_path, and per-case host_inode/fileName) before this query
* joins out to {@code contentlet_version_info}/{@code structure}/
* {@code contentlet} -- see {@link #selectQuery(BrowserQuery)}. When
* blank, the query joins directly against {@code identifier} exactly
* as before this fix (every non-folder-scoped caller is unaffected).
* @return The base SQL SELECT query string.
*/
private String buildSelectBaseQuery(final BrowserQuery browserQuery, final String workingLiveInode) {
private String buildSelectBaseQuery(final BrowserQuery browserQuery, final String workingLiveInode,
final String candidatesCte) {

final String identifierSource = UtilMethods.isSet(candidatesCte) ? "candidates" : "identifier";

final String baseClause = " from contentlet_version_info cvi, identifier id, structure struc, contentlet c "
final String baseClause = " from contentlet_version_info cvi, " + identifierSource
+ " id, structure struc, contentlet c "
+ " where cvi.identifier = id.id and struc.velocity_var_name = id.asset_subtype and "
+ " c.inode = cvi." + workingLiveInode + " and cvi.variant_id='"
+ DEFAULT_VARIANT.name() + "' ";

final StringBuilder baseQuery = new StringBuilder(
"select cvi." + workingLiveInode + " as inode " + baseClause);
final StringBuilder baseQuery = new StringBuilder(candidatesCte)
.append("select cvi.").append(workingLiveInode).append(" as inode ").append(baseClause);
Comment on lines +2180 to +2181

@semgrep-dotcms semgrep-dotcms Bot Sep 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
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

  1. Validate workingLiveInode against 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 as live_inode or working_inode.

  2. Restrict candidatesCte to 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.

  3. Keep SQL structure in fixed constants and append only validated identifiers or trusted fragments. For example, build the query from a fixed SELECT template after validating workingLiveInode and identifierSource, rather than allowing untrusted text to reach new StringBuilder(...).

  4. Validate baseTypes as numeric values derived from the BaseContentType enum before appending them. Do not append arbitrary strings to the IN clause.

  5. Parameterize contentTypeIds and excludedContentTypeIds instead of concatenating them inside quoted SQL. Generate placeholders such as :contentTypeId0, bind each ID through the query’s existing params mechanism, and append only the placeholders to the SQL.

  6. 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.

@ihoffmann-dot ihoffmann-dot Sep 8, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/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.

@semgrep-dotcms semgrep-dotcms Bot Sep 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


final boolean showAllBaseTypes = browserQuery.baseTypes.contains(BaseContentType.ANY);
if (!showAllBaseTypes) {
Expand Down Expand Up @@ -2688,12 +2748,31 @@ private void appendExcludeArchivedQuery(StringBuilder sqlQuery) {
* @param sqlQuery
* @param orderByDesc
*/
private void appendOrderByQuery(StringBuilder sqlQuery, boolean orderByDesc) {
private void appendOrderByQuery(StringBuilder sqlQuery, boolean orderByDesc, boolean useFolderCte) {
// issue #37229 (FR-001): `mod_date` alone has no tiebreaker, so rows sharing the same
// mod_date get an unspecified, planner-dependent order today (~1.2% of rows per #37148).
// `id.id` (the identifier row's own primary key, already joined/in scope -- no new join)
// makes tied-row order -- and the pagination cursor derived from it -- a deterministic,
// reproducible-run-to-run guarantee. This is a NEW guarantee, not a reproduction of
// whatever arbitrary order those tied rows happened to return before this fix.
//
// FR-001 scopes this to folder-scoped requests only ("every folder-scoped listing
// request"), matching useFolderCte exactly -- every other caller's ORDER BY stays
// byte-identical to before (found in review: this was previously unconditional for any
// caller with sortBy set, silently changing tie order and pagination cursors for
// non-folder-scoped callers too).
//
// `(mod_date, id.id)` is still not a total order on a multi-language folder: a single
// identifier legitimately comes back as several rows, one per language
// (appendLanguageQuery's `cvi.lang in (...)`), so id.id is identical across those rows.
// When they also share mod_date, nothing is left to break the tie. `cvi.lang` closes it
// and is already joined/in scope via contentlet_version_info -- no new join (code
// review, PR #37397).
sqlQuery.append(" order by ");
if (orderByDesc) {
sqlQuery.append(" c.mod_date desc");
sqlQuery.append(" c.mod_date desc").append(useFolderCte ? ", id.id desc, cvi.lang desc" : "");
} else {
sqlQuery.append(" c.mod_date asc");
sqlQuery.append(" c.mod_date asc").append(useFolderCte ? ", id.id asc, cvi.lang asc" : "");
}
}

Expand Down
Loading
Loading