added authorization for executing main pyscript via callback #1967
Conversation
WalkthroughThe changes introduce authentication for the restricted Python script execution endpoint by adding a request parameter and verifying it before processing. The Changes
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/integration_test/callback_service_test.py (1)
664-686: Consider adding a negative test case for unauthorized access.While you've added a test for successful authentication, consider adding a test case that verifies the endpoint rejects requests without a valid authorization token to ensure complete test coverage.
@pytest.mark.asyncio @patch("kairon.async_callback.utils.CallbackUtility.main_pyscript_handler") +async def test_execute_python_unauthorized(): + await app.start() + client = TestClient(app) + + payload = { + "source_code": "bot_response=100", + "predefined_objects": {"x": 1} + } + + # Send request without authorization header + response = await client.post("/main_pyscript/execute-python", content=JSONContent(payload)) + json_response = await response.json() + + # Assertions for unauthorized access + assert response.status == 401 or response.status == 403 + assert json_response["success"] is False🧰 Tools
🪛 Gitleaks (8.26.0)
665-665: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
tests/integration_test/callback_service_test.py(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.26.0)
tests/integration_test/callback_service_test.py
665-665: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyze (python)
- GitHub Check: Python CI
🔇 Additional comments (3)
tests/integration_test/callback_service_test.py (3)
664-667: Good addition of JWT mocking for testing authorization.The mock token implementation is appropriate for testing the new authorization requirement in the execute-python endpoint.
🧰 Tools
🪛 Gitleaks (8.26.0)
665-665: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
674-676: Correctly implemented Authorization header format.The Authorization header follows the standard "Bearer token" format, which is appropriate for JWT authentication.
678-679: Successfully modified test to include authorization.This change aligns with the PR objective of adding authentication to the main pyscript execution endpoint.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
tests/integration_test/callback_service_test.py (1)
724-744:⚠️ Potential issueDuplicate test function name detected.
There are two test functions with the same name
test_execute_python_failure(lines 687 and 724). This could cause confusion and might lead to one test being skipped during test execution.Consider renaming one of these functions to clearly distinguish between them:
-@pytest.mark.asyncio -@patch("kairon.async_callback.utils.CallbackUtility.main_pyscript_handler") -async def test_execute_python_failure(mock_handler): +@pytest.mark.asyncio +@patch("kairon.async_callback.utils.CallbackUtility.main_pyscript_handler") +async def test_execute_python_no_auth_failure(mock_handler):🧰 Tools
🪛 Ruff (0.11.9)
726-726: Redefinition of unused
test_execute_python_failurefrom line 689(F811)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration_test/callback_service_test.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/integration_test/callback_service_test.py (1)
kairon/shared/auth.py (2)
Authentication(30-465)create_access_token(151-173)
🪛 Gitleaks (8.26.0)
tests/integration_test/callback_service_test.py
665-665: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyze (python)
- GitHub Check: Python CI
🔇 Additional comments (2)
tests/integration_test/callback_service_test.py (2)
687-723: Good implementation of authentication test with dynamic token generation.The test properly generates a real JWT token with appropriate claims and expiration time, then verifies the error handling when authentication succeeds but execution fails. This is a good addition to ensure the authentication flow works correctly.
693-699:❓ Verification inconclusive
Verify the token claims match what's expected by the endpoint.
The test creates a JWT token with specific claims (
"sub": "action-server"and"callback": true). Ensure these claims are actually being checked by the authentication logic in the endpoint being tested.
🏁 Script executed:
#!/bin/bash # Find where token validation happens for the endpoint rg -A 10 "verify.*request" --glob "*.py" | grep -v "test_"Length of output: 22584
🏁 Script executed:
#!/bin/bash sed -n '1,200p' kairon/async_callback/auth.pyLength of output: 1349
🏁 Script executed:
#!/bin/bash rg -n "create_access_token" -n kairon/shared/auth.pyLength of output: 592
(Awaiting the script execution results)
| # Create a mock token with three segments | ||
| mock_token = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJnQUFBQUFCb0xjUi00QnRVM1FFOUdBRFdONVg0N29Yay03VDdxN2dOQks1dlFqM3VnQkoybnZlLVBrVGVTUzZYRm55dkRjM0RmMmNob2JUSFAyM0dDTnI3STZ2eWwyZ0tUYkF3eXFFNERKVDU1TWNsWmV5LWdSRGR4dG55TS01M1R0cjVQLU9iQ3lKTmpDcTFaa1NkRFFQNWk0eDZsN3R1c0E9PSIsInZlcnNpb24iOiIyLjAifQ.HEL3BSt1uO2uRfwwPTVV5UgYvwXsSoyERqZ9Gev2X-U" | ||
|
|
||
| mock_handler.return_value = {"output": "Execution successful", "success": True} | ||
|
|
||
| payload = { | ||
| "source_code": "bot_response=100", | ||
| "predefined_objects": {"x": 1} | ||
| } | ||
|
|
||
| # Simulate response from handler | ||
| mock_handler.return_value = {"output": "Execution successful", "success": True} | ||
| headers = { | ||
| "Authorization": f"Bearer {mock_token}" | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider using a dynamically generated token instead of a hardcoded one.
The test uses a hardcoded JWT token which could be a security concern if it contains actual credentials or secrets. While this is a test file, it's better to generate tokens dynamically like you did in the test_execute_python_failure test.
🏁 Script executed:
#!/bin/bash
# Check if the token is decodable and what it contains
echo "Checking if token contains sensitive information:"
jq -R 'split(".") | .[0],.[1] | @base64d | fromjson' <<< "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJnQUFBQUFCb0xjUi00QnRVM1FFOUdBRFdONVg0N29Yay03VDdxN2dOQks1dlFqM3VnQkoybnZlLVBrVGVTUzZYRm55dkRjM0RmMmNob2JUSFAyM0dDTnI3STZ2eWwyZ0tUYkF3eXFFNERKVDU1TWNsWmV5LWdSRGR4dG55TS01M1R0cjVQLU9iQ3lKTmpDcTFaa1NkRFFQNWk0eDZsN3R1c0E9PSIsInZlcnNpb24iOiIyLjAifQ"Length of output: 400
Use dynamic JWT generation for mock_token
We’ve verified that the hardcoded token’s header and payload contain no sensitive data. To improve test consistency and avoid brittle hardcoded values (and mirror your approach in test_execute_python_failure), generate the JWT at runtime.
• File: tests/integration_test/callback_service_test.py
Lines: 664–676
Suggested change:
- # Create a mock token with three segments
- mock_token = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.…HEL3BSt1uO2uRfwwPTVV5UgYvwXsSoyERqZ9Gev2X-U"
+ # Dynamically generate a mock token for consistency
+ from jwt import encode
+
+ payload = {
+ "sub": "gAAAAABoLcR-4BtU3QE9GADWN5X47oXk-…tusA==",
+ "version": "2.0"
+ }
+ mock_token = encode(payload, TEST_JWT_SECRET, algorithm="HS256")Make sure TEST_JWT_SECRET is defined or imported as in other tests.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Gitleaks (8.26.0)
665-665: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
🤖 Prompt for AI Agents
In tests/integration_test/callback_service_test.py around lines 664 to 676,
replace the hardcoded JWT token string with a dynamically generated token using
the same method as in test_execute_python_failure. Ensure you import or define
TEST_JWT_SECRET for signing the token, then create the token at runtime with
appropriate header and payload. Update the Authorization header to use this
generated token to improve test reliability and consistency.
Summary by CodeRabbit
New Features
Bug Fixes