Skip to content

fix(schema-compiler): Use join hints from views for join graph #10006

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

Jump to bottom
Open
wants to merge 9 commits into
base: fix/join-paths
Choose a base branch
from
Open

fix(schema-compiler): Use join hints from views for join graph #10006

wants to merge 9 commits into from
+144 −124

Conversation

KSDaemon
Copy link
Member

@KSDaemon KSDaemon commented Sep 29, 2025 •
edited
Loading

This PR updates join graph for the query construction by prioritizing hints from view definitions.

This fixes #9852

Temporarily set against fix/join-paths as a parent. I'll switch to the master after the merge.

Check List

  • Tests have been run in packages where changes have been made, if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet

github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 29, 2025
View reviewed changes
packages/cubejs-schema-compiler/src/compiler/JoinGraph.ts
const to = hint[i + 1];

if (tunedNodes[from]?.[to] !== undefined) {
tunedNodes[from][to] = PRIORITY_WEIGHT;

Check warning

Code scanning / CodeQL

Prototype-polluting assignment

This assignment may alter Object.prototype if a malicious '__proto__' string is injected from [library input](1).

Copilot Autofix

AI 6 days ago

To fix this vulnerability, we must ensure that keys assigned to tunedNodes (specifically, from and to) are filtered such that prototype-polluting keys like __proto__, constructor, and prototype cannot be used as object property names.

The single best way to do this is within the loops constructing and mutating tunedNodes. Before any assignment to an object with a computed property name, add a conditional check such that if the key is one of the dangerous property names, skip or throw an error.

The only file to change is packages/cubejs-schema-compiler/src/compiler/JoinGraph.ts. We will alter two places in getFixedWeightsGraph:

  • On building the deep copy (tunedNodes[from] = {} and tunedNodes[from][to] = weight;)
  • On updating the weight (tunedNodes[from][to] = PRIORITY_WEIGHT;)

However, the primary risk is on the assignment involving potentially tainted input, which happens in the join hints loop — that's what CodeQL flagged. We'll filter from and to in the hint-processing loop.

No external dependencies are needed.

Suggested changeset 1
packages/cubejs-schema-compiler/src/compiler/JoinGraph.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/packages/cubejs-schema-compiler/src/compiler/JoinGraph.ts b/packages/cubejs-schema-compiler/src/compiler/JoinGraph.ts
--- a/packages/cubejs-schema-compiler/src/compiler/JoinGraph.ts
+++ b/packages/cubejs-schema-compiler/src/compiler/JoinGraph.ts
@@ -290,11 +290,16 @@
   protected getFixedWeightsGraph(joinHints: JoinHints): Graph {
     const PRIORITY_WEIGHT = 20; // Lower weight for preferred paths
 
+    // Keys that can pollute object prototype
+    const POLLUTING_KEYS = ['__proto__', 'constructor', 'prototype'];
+
     // Create a deep copy of this.nodes to avoid modifying the original
     const tunedNodes: Record<string, Record<string, number>> = {};
     for (const [from, destinations] of Object.entries(this.nodes)) {
+      if (POLLUTING_KEYS.includes(from)) continue; // prevent prototype pollution
       tunedNodes[from] = {};
       for (const [to, weight] of Object.entries(destinations)) {
+        if (POLLUTING_KEYS.includes(to)) continue; // prevent prototype pollution
         tunedNodes[from][to] = weight;
       }
     }
@@ -305,7 +305,7 @@
         for (let i = 0; i < hint.length - 1; i++) {
           const from = hint[i];
           const to = hint[i + 1];
-
+          if (POLLUTING_KEYS.includes(from) || POLLUTING_KEYS.includes(to)) continue; // prevent prototype pollution
           if (tunedNodes[from]?.[to] !== undefined) {
             tunedNodes[from][to] = PRIORITY_WEIGHT;
           }
EOF
@@ -290,11 +290,16 @@
protected getFixedWeightsGraph(joinHints: JoinHints): Graph {
const PRIORITY_WEIGHT = 20; // Lower weight for preferred paths

// Keys that can pollute object prototype
const POLLUTING_KEYS = ['__proto__', 'constructor', 'prototype'];

// Create a deep copy of this.nodes to avoid modifying the original
const tunedNodes: Record<string, Record<string, number>> = {};
for (const [from, destinations] of Object.entries(this.nodes)) {
if (POLLUTING_KEYS.includes(from)) continue; // prevent prototype pollution
tunedNodes[from] = {};
for (const [to, weight] of Object.entries(destinations)) {
if (POLLUTING_KEYS.includes(to)) continue; // prevent prototype pollution
tunedNodes[from][to] = weight;
}
}
@@ -305,7 +305,7 @@
for (let i = 0; i < hint.length - 1; i++) {
const from = hint[i];
const to = hint[i + 1];

if (POLLUTING_KEYS.includes(from) || POLLUTING_KEYS.includes(to)) continue; // prevent prototype pollution
if (tunedNodes[from]?.[to] !== undefined) {
tunedNodes[from][to] = PRIORITY_WEIGHT;
}
Copilot is powered by AI and may make mistakes. Always verify output.
@KSDaemon KSDaemon changed the base branch from master to fix/join-paths September 29, 2025 10:39
@KSDaemon KSDaemon marked this pull request as ready for review September 29, 2025 10:41
@KSDaemon KSDaemon requested a review from a team as a code owner September 29, 2025 10:41
@KSDaemon KSDaemon force-pushed the fix/respect-view-join-paths branch from 84c87de to 869b651 Compare September 30, 2025 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@KSDaemon