-
Notifications
You must be signed in to change notification settings - Fork 2k
feat: SCIM spike #39117
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.
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
feat: SCIM spike #39117
Conversation
|
Size Change: 0 B Total Size: 3.07 MB ℹ️ View Unchanged
|
| scim_user = PostHogSCIMUser.from_dict(request.data, organization_domain) | ||
| return Response(scim_user.to_dict(), status=status.HTTP_201_CREATED) | ||
| except ValueError as e: | ||
| return Response({"detail": str(e)}, status=status.HTTP_400_BAD_REQUEST) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Stack trace information
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To fix the problem, we should avoid exposing internal exception messages to users. For line 42 (and similar exception handlers), instead of returning the string representation of the error, return a generic message ("Invalid user data" or similar), and log the detailed exception server-side for debugging. Since we only see the code snippet for ee/api/scim/views.py, we can use Python's standard logging module to log the exception, which is well-known and safe to import here. We'll need to import logging at the top, configure a logger, and in exception handlers, log the full error and return a safe, generic message to users. Only the exception handler in the POST branch (ValueError on lines 41-42) needs this fix in what was shown, as CodeQL highlighted that path.
-
Copy modified line R7 -
Copy modified line R14 -
Copy modified lines R37-R38
| @@ -4,13 +4,14 @@ | ||
| from rest_framework.decorators import api_view, authentication_classes | ||
| from rest_framework.request import Request | ||
| from rest_framework.response import Response | ||
| import logging | ||
|
|
||
| from ee.api.scim.auth import SCIMBearerTokenAuthentication | ||
| from ee.api.scim.group import PostHogSCIMGroup | ||
| from ee.api.scim.user import PostHogSCIMUser | ||
| from posthog.models.organization_domain import OrganizationDomain | ||
|
|
||
|
|
||
| logger = logging.getLogger(__name__) | ||
| @csrf_exempt | ||
| @api_view(["GET", "POST"]) | ||
| @authentication_classes([SCIMBearerTokenAuthentication]) | ||
| @@ -39,7 +34,8 @@ | ||
| scim_user = PostHogSCIMUser.from_dict(request.data, organization_domain) | ||
| return Response(scim_user.to_dict(), status=status.HTTP_201_CREATED) | ||
| except ValueError as e: | ||
| return Response({"detail": str(e)}, status=status.HTTP_400_BAD_REQUEST) | ||
| logger.error("Failed to create SCIM user", exc_info=True) | ||
| return Response({"detail": "Invalid user data"}, status=status.HTTP_400_BAD_REQUEST) | ||
|
|
||
|
|
||
| @csrf_exempt |
| scim_user = PostHogSCIMUser.from_dict(request.data, organization_domain) | ||
| return Response(scim_user.to_dict()) | ||
| except ValueError as e: | ||
| return Response({"detail": str(e)}, status=status.HTTP_400_BAD_REQUEST) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Stack trace information
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To fix this problem, the code should return a generic error message to the client instead of the actual exception details. At the same time, it is still beneficial to log the full exception information on the server for internal debugging.
In practice, this involves:
- Changing the API response at the vulnerable line (
{"detail": str(e)}) to use a generic error message, such as{"detail": "Invalid user data."}. - Logging the exception internally using the
loggingmodule, including the stack trace, so that developers can investigate if necessary. - Including any necessary import (
import logging) at the top of the file.
All relevant changes should be made within the file ee/api/scim/views.py. Specifically:
- Add an
import loggingat the top (if it does not already exist). - Update the exception handler in the affected function to both log the exception and return a generic error message.
-
Copy modified line R7 -
Copy modified lines R75-R76
| @@ -4,6 +4,7 @@ | ||
| from rest_framework.decorators import api_view, authentication_classes | ||
| from rest_framework.request import Request | ||
| from rest_framework.response import Response | ||
| import logging | ||
|
|
||
| from ee.api.scim.auth import SCIMBearerTokenAuthentication | ||
| from ee.api.scim.group import PostHogSCIMGroup | ||
| @@ -71,7 +72,8 @@ | ||
| scim_user = PostHogSCIMUser.from_dict(request.data, organization_domain) | ||
| return Response(scim_user.to_dict()) | ||
| except ValueError as e: | ||
| return Response({"detail": str(e)}, status=status.HTTP_400_BAD_REQUEST) | ||
| logging.exception("Error updating user in PUT request") | ||
| return Response({"detail": "Invalid user data."}, status=status.HTTP_400_BAD_REQUEST) | ||
|
|
||
| elif request.method == "PATCH": | ||
| try: |
| scim_user.handle_replace(op.get("value", {})) | ||
| return Response(scim_user.to_dict()) | ||
| except Exception as e: | ||
| return Response({"detail": str(e)}, status=status.HTTP_400_BAD_REQUEST) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Stack trace information
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To fix this problem, you should catch generic exceptions and avoid returning the exception detail to the user. Instead, return a generic error message in the response (such as "An internal error has occurred" or similar wording). Optionally, log the detailed exception information on the server side so that developers can investigate issues without exposing stack traces or internal details to external users. Only user-friendly, non-sensitive messages should be presented in API responses. The fix will involve editing the except Exception as e: block in ee/api/scim/views.py (specifically in the method scim_user_detail_view, PATCH section) to change the response from returning {"detail": str(e)} to something like {"detail": "An internal error has occurred."}. For useful developer diagnostics, add server-side logging for the exception (using e.g. Python's logging module).
Required changes:
- Add an import for Python's logging module at the top if not already present.
- Use
logging.exception(...)orlogger.exception(...)in the relevant except block. - Update the response to use a generic message.
-
Copy modified line R8 -
Copy modified lines R79-R80
| @@ -5,12 +5,12 @@ | ||
| from rest_framework.request import Request | ||
| from rest_framework.response import Response | ||
|
|
||
| import logging | ||
| from ee.api.scim.auth import SCIMBearerTokenAuthentication | ||
| from ee.api.scim.group import PostHogSCIMGroup | ||
| from ee.api.scim.user import PostHogSCIMUser | ||
| from posthog.models.organization_domain import OrganizationDomain | ||
|
|
||
|
|
||
| @csrf_exempt | ||
| @api_view(["GET", "POST"]) | ||
| @authentication_classes([SCIMBearerTokenAuthentication]) | ||
| @@ -81,7 +76,8 @@ | ||
| scim_user.handle_replace(op.get("value", {})) | ||
| return Response(scim_user.to_dict()) | ||
| except Exception as e: | ||
| return Response({"detail": str(e)}, status=status.HTTP_400_BAD_REQUEST) | ||
| logging.exception("Exception occurred while handling SCIM PATCH request") | ||
| return Response({"detail": "An internal error has occurred."}, status=status.HTTP_400_BAD_REQUEST) | ||
|
|
||
| elif request.method == "DELETE": | ||
| scim_user.delete() |
| scim_group = PostHogSCIMGroup.from_dict(request.data, organization_domain) | ||
| return Response(scim_group.to_dict(), status=status.HTTP_201_CREATED) | ||
| except ValueError as e: | ||
| return Response({"detail": str(e)}, status=status.HTTP_400_BAD_REQUEST) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Stack trace information
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
The best way to fix the problem is to send a generic error message to the external user, while optionally logging the detailed exception on the server for debugging. In this case, edit lines 118-119 in ee/api/scim/views.py within the scim_groups_view function's POST handling block. Specifically:
- Replace
{"detail": str(e)}with a generic message such as{"detail": "Invalid group data."}(or "An internal error occurred."), ensuring no exception details are sent to the user. - Optionally, log the exception internally for debugging/tracing purposes (using Python's logging library, which would need an import if not present).
Required changes:
- Update the response line in the except block in
scim_groups_view. - Optionally, add a
loggerimport/definition at the top of the file, and log the exception in the except block (without sending it to the user).
-
Copy modified lines R119-R122
| @@ -116,7 +116,10 @@ | ||
| scim_group = PostHogSCIMGroup.from_dict(request.data, organization_domain) | ||
| return Response(scim_group.to_dict(), status=status.HTTP_201_CREATED) | ||
| except ValueError as e: | ||
| return Response({"detail": str(e)}, status=status.HTTP_400_BAD_REQUEST) | ||
| import logging | ||
| logger = logging.getLogger(__name__) | ||
| logger.warning("SCIM group creation error: %s", str(e)) | ||
| return Response({"detail": "Invalid group data."}, status=status.HTTP_400_BAD_REQUEST) | ||
|
|
||
|
|
||
| @csrf_exempt |
| scim_group = PostHogSCIMGroup.from_dict(request.data, organization_domain) | ||
| return Response(scim_group.to_dict()) | ||
| except ValueError as e: | ||
| return Response({"detail": str(e)}, status=status.HTTP_400_BAD_REQUEST) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Stack trace information
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
The best way to fix this issue is to change the error response on line 151 to return a generic error message to the client, such as "Invalid input", instead of exposing the original exception message. For server-side troubleshooting, the actual exception and its traceback should be logged—either using Python's standard logging module or Django's logger if available. This way, developers retain access to diagnostics while ensuring users do not see potentially sensitive details.
To implement this, update the except block on line 150-151 to log the error and return a generic message, and add an import for Python's logging module if not already present in the snippet.
Changes needed:
- Import the
loggingmodule at the top of the file. - In the except block, log the exception using
logger.exception(...)or similar. - Return a generic error message to the client in the response.
-
Copy modified line R8 -
Copy modified lines R146-R147
| @@ -5,12 +5,12 @@ | ||
| from rest_framework.request import Request | ||
| from rest_framework.response import Response | ||
|
|
||
| import logging | ||
| from ee.api.scim.auth import SCIMBearerTokenAuthentication | ||
| from ee.api.scim.group import PostHogSCIMGroup | ||
| from ee.api.scim.user import PostHogSCIMUser | ||
| from posthog.models.organization_domain import OrganizationDomain | ||
|
|
||
|
|
||
| @csrf_exempt | ||
| @api_view(["GET", "POST"]) | ||
| @authentication_classes([SCIMBearerTokenAuthentication]) | ||
| @@ -148,7 +143,8 @@ | ||
| scim_group = PostHogSCIMGroup.from_dict(request.data, organization_domain) | ||
| return Response(scim_group.to_dict()) | ||
| except ValueError as e: | ||
| return Response({"detail": str(e)}, status=status.HTTP_400_BAD_REQUEST) | ||
| logging.getLogger(__name__).exception("ValueError in scim_group_detail_view PUT") | ||
| return Response({"detail": "Invalid input."}, status=status.HTTP_400_BAD_REQUEST) | ||
|
|
||
| elif request.method == "PATCH": | ||
| try: |
| scim_group.handle_replace(op.get("value", {})) | ||
| return Response(scim_group.to_dict()) | ||
| except Exception as e: | ||
| return Response({"detail": str(e)}, status=status.HTTP_400_BAD_REQUEST) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Stack trace information
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To resolve the information exposure, the API should not include the exception message (str(e)) in responses to the client. Instead, it should return a generic error message indicating an internal error occurred, and optionally log the exception server-side for debugging and audit purposes. The most straightforward fix is to replace {"detail": str(e)} with {"detail": "An error occurred processing your request."} (or similar), and additionally log the exception locally (e.g., using Python's logging module) to ensure errors can still be investigated.
Implementation steps:
- Replace
Response({"detail": str(e)}, status=status.HTTP_400_BAD_REQUEST)(line 161) with a generic error message. - Import Python's standard
loggingmodule (as a well-known external library) at the top of the file. - Log the exception message and stack trace at the point of exception handling.
All these changes should be made in ee/api/scim/views.py.
-
Copy modified line R7 -
Copy modified lines R162-R163
| @@ -4,6 +4,7 @@ | ||
| from rest_framework.decorators import api_view, authentication_classes | ||
| from rest_framework.request import Request | ||
| from rest_framework.response import Response | ||
| import logging | ||
|
|
||
| from ee.api.scim.auth import SCIMBearerTokenAuthentication | ||
| from ee.api.scim.group import PostHogSCIMGroup | ||
| @@ -158,7 +159,8 @@ | ||
| scim_group.handle_replace(op.get("value", {})) | ||
| return Response(scim_group.to_dict()) | ||
| except Exception as e: | ||
| return Response({"detail": str(e)}, status=status.HTTP_400_BAD_REQUEST) | ||
| logging.exception("Error handling PATCH operation in scim_group_detail_view") | ||
| return Response({"detail": "An error occurred processing your request."}, status=status.HTTP_400_BAD_REQUEST) | ||
|
|
||
| elif request.method == "DELETE": | ||
| scim_group.delete() |
Migration SQL ChangesHey 👋, we've detected some migrations on this PR. Here's the SQL output for each migration, make sure they make sense:
|
🔍 Migration Risk AnalysisWe've analyzed your migrations for potential risks. Summary: 1 Safe | 0 Needs Review | 0 Blocked ✅ SafeNo locks, backwards compatible |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
|
This PR was closed due to lack of activity. Feel free to reopen if it's still relevant. |
Problem
Changes
How did you test this code?
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Changelog: (features only) Is this feature complete?