Repository navigation
Conversation
Co-authored-by: tteon <52625664+tteon@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
github-actions
Bot
added
area-sdk
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e891a6bc9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
github-actions
Bot
added
area-docs
tteon
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a19637a5ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73a64440a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return self.graph_store.query(cypher, params=params, database=database) | ||
| from .store.graph import enforce_read_workspace_scope | ||
|
|
||
| enforce_read_workspace_scope(cypher, reject_rebinding=True) |
There was a problem hiding this comment.
A query such as MATCH ()-[r]->() WHERE r._workspace_id <> $workspace_id RETURN r.secret AS secret passes this gate without rebinding because verify_workspace_binding() only validates identifiers recognized as node variables. It therefore executes and returns relationship data belonging to every other workspace, despite the new governed-read guarantee; relationship variables and path aliases must also be proven to match the client workspace before delegation.
AGENTS.md reference: AGENTS.md:L126-L126
Useful? React with 👍 / 👎.
| if reject_rebinding and _REBIND_CONSTRUCTS_RE.search(stripped): | ||
| raise WorkspaceScopeViolationError(cypher, "unsupported_construct") |
There was a problem hiding this comment.
For a legitimate scoped query such as MATCH (n) WHERE n._workspace_id = $workspace_id AND n.name = 'Union' RETURN n.name, this regex sees Union inside the string literal and raises unsupported_construct, even though the query contains no UNION clause or variable rebinding. Strip or tokenize Cypher string literals before applying the construct check so ordinary searches for values such as “Union”, “With”, or “Unwind” remain usable.
Useful? React with 👍 / 👎.
Severity: High
Vulnerability: The raw
client.query()method, intended for executing raw Cypher queries against the graph store in local mode, did not enforce workspace isolation boundaries. It omitted theworkspace_idparameter and theenforce_workspace_filter=Trueflag when delegating tograph_store.query().Impact: A user or agent leveraging
client.query()could potentially execute queries that traverse or leak nodes and relationships belonging to other tenants, bypassing the repository's data isolation guarantees.Fix: Modified
client.query()insrc/seocho/client.pyto explicitly injectworkspace_id=self.workspace_idandenforce_workspace_filter=Truewhen invokingself.graph_store.query(). This ensures that any raw Cypher query processed through this interface must explicitly respect the$workspace_idfilter (as enforced by the underlying graph store implementation), preventing cross-tenant data access.Validation: Ran the
bash scripts/ci/run_basic_ci.shCI script and relevant pytest suites (tests/seocho/test_operating_layer.py,tests/seocho/test_session_agent.py, etc.) locally. Tested that the addition of the strict arguments doesn't disrupt tests reliant on properly mocked components.Risks: The fix makes
client.query()strictly require the use of$workspace_idinside the raw Cypher query string (whenworkspace_idis set). Any existing automation or user scripts leveragingclient.query()with globally-scoped (non-tenant-aware) Cypher queries will start raisingWorkspaceFilterMissingError. However, this failure is by design for a multi-tenant platform to enforce safety.Modified Files:
[Draft]
PR created automatically by Jules for task 18026098678358793120 started by @tteon
Maintainer integration review (2026-10-13)
Raw client queries copy parameters and overwrite the reserved workspace_id with the client workspace before delegation. Tests exercise the actual Neo4jGraphStore up to its driver boundary and reject unscoped reads before I/O. Updated the bring-your-data notebook and SDK contract. Targeted client tests: 25 passed.
Validation of the exact combined changes on current main:
bash scripts/ci/run_basic_ci.sh— 1,252 passed, 3 skipped; all remaining lint/type/import/root/env/identity gates passed.bash scripts/ci/check-doc-contracts.shpassed. Local tracking: seocho-xlo1. Source changes remain in their owning SDK modules; local agent state stays untracked.Raw query preflight additionally rejects WITH/UNION/UNWIND because the bounded checker cannot verify variable rebinding. Three attack-shape regression cases confirm rejection before driver I/O. The opt-in strict helper is used by the public raw-query facade; existing internally generated graph-query handling retains its current default. Updated the SDK contract. Targeted client/boundary tests: 32 passed. Final combined CI rerun pending.
Deferred after current review
This PR is draft and MUST NOT be merged as a complete workspace isolation fix. The review found that the bounded checker still does not prove scope for relationship/path results, and rebinding detection can match keywords inside Cypher literals. Reserved parameter pinning, notebook corrections, and rebinding rejection are partial changes retained for a future focused validator slice. The original main behavior remains unchanged by this deferred PR. Current unresolved review threads retain the concrete attacks and compatibility examples. Local tracking: seocho-xlo1.