[HOTFIX] Validate note access for interpreter bindings - #5431
Conversation
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This hotfix enforces note-level permissions for interpreter binding operations so that binding reads/writes are rejected unless the caller has the appropriate note access.
Changes:
- Add reader permission check to
GET_INTERPRETER_BINDINGS. - Add writer permission check to
SAVE_INTERPRETER_BINDINGSand suppress successful response when unauthorized. - Add regression tests covering permitted and rejected read/write requests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java | Adds authz gates for reading/writing interpreter bindings and conditions response on authorization. |
| zeppelin-server/src/test/java/org/apache/zeppelin/socket/NotebookServerTest.java | Adds regression tests verifying authz enforcement for interpreter binding read/write operations. |
Suppressed comments (1)
zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java:1
permittedis returned astrueeven whennote == null(line 649), which makes the method send anOP.INTERPRETER_BINDINGSresponse for a non-existent note. Ifpermittedis intended to reflect “authorized & actionable”, returnfalsewhennote == null(and consider initializingpermittedtofalseand setting it totrueonly after both the permission check passes and the note exists). This keeps the response behavior consistent with the intention of “stop before returning binding data or applying changes.”
/*
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
tbonelee
left a comment
There was a problem hiding this comment.
LGTM.
nit: I wonder whether these two handlers would be more consistent with the other
note handlers if they lived in NotebookService, though the current shape seems
fine for a hotfix.
|
Merged into master (2593fc0). |
### What is this PR for? Ensure that interpreter binding operations follow the permissions of the associated note. - Reading interpreter bindings requires reader permission. - Updating interpreter bindings requires writer permission. - Requests without the required permission stop before returning binding data or applying changes. - Add regression tests for permitted and rejected read/write requests. ### What type of PR is it? Hot Fix ### Todos * [x] Add note permission checks for interpreter binding operations * [x] Add regression tests * [x] Run `NotebookServerTest` ### What is the Jira issue? N/A ### How should this be tested? ```bash mkdir -p spark/interpreter/target ./mvnw -pl spark/interpreter resources:resources<at>copy-interpreter-setting ./mvnw -pl zeppelin-server -Dtest=NotebookServerTest test ``` Result: 25 tests run, 0 failures, 0 errors. ### Screenshots (if appropriate) N/A ### Questions: * Does the license files need to update? No. * Is there breaking changes for older versions? No API compatibility changes. Requests without the required note permission are now rejected as intended. * Does this needs documentation? No. Closes #5431 from jongyoul/codex/security-interpreter-bindings-authz. Signed-off-by: Jongyoul Lee <jongyoul@gmail.com> (cherry picked from commit 2593fc0) Signed-off-by: Jongyoul Lee <jongyoul@gmail.com>
What is this PR for?
Ensure that interpreter binding operations follow the permissions of the associated note.
What type of PR is it?
Hot Fix
Todos
NotebookServerTestWhat is the Jira issue?
N/A
How should this be tested?
mkdir -p spark/interpreter/target ./mvnw -pl spark/interpreter resources:resources@copy-interpreter-setting ./mvnw -pl zeppelin-server -Dtest=NotebookServerTest testResult: 25 tests run, 0 failures, 0 errors.
Screenshots (if appropriate)
N/A
Questions: