-
-
Notifications
You must be signed in to change notification settings - Fork 311
feat: event to load webpages in memory #3033
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
base: master
Are you sure you want to change the base?
Conversation
🧪 CI InsightsHere's what we observed from your CI run for f9b93e8. 🟢 All jobs passed!But CI Insights is watching 👀 |
chore: fix migrations
| return {"chatbot_user_token": None, "chatbot_enabled": False} | ||
| profile = UserProfile(user_obj=request.user) | ||
| if not getattr(profile, "experiments", False): | ||
| return {"chatbot_user_token": None, "chatbot_enabled": False} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chatbot authorization bypass after permission revocation
Medium Severity
The chatbot_user_token context processor checks profile.experiments (MongoDB user preference) but not user_has_experiments() (PostgreSQL admin-controlled permission). Other parts of the codebase correctly use user_has_experiments() to verify access. This means if an admin revokes a user's experiment access via UserExperimentSettings, the chatbot will still be shown because profile.experiments remains True in MongoDB. The context processor needs to verify both the admin permission and user preference.
|
|
||
| # Make this unique, and don't share it with anybody. | ||
| SECRET_KEY = '' | ||
| CHATBOT_USER_ID_SECRET = 'secret' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing CHATBOT_API_BASE_URL default causes AttributeError crash
High Severity
The context processor accesses settings.CHATBOT_API_BASE_URL at line 132, but this setting is only added to local_settings_example.py, not to the base sefaria/settings.py. While CHATBOT_USER_ID_SECRET is correctly added to the base settings file, CHATBOT_API_BASE_URL is not. If a deployment has a local_settings.py that doesn't define this setting, the context processor will crash with AttributeError for users with experiments enabled.
Additional Locations (1)
| } | ||
| this.setState({panels: panels}); | ||
| if (opts.saveLastPlace !== false) { | ||
| this.saveLastPlace(basePanel, 1, hasConnections && this.props.multiPanel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong panel index passed to saveLastPlace
Medium Severity
In bootstrapTextUrl, basePanel is placed at index 0 in the panels array (const panels = [basePanel]), but saveLastPlace is called with n=1. Inside saveLastPlace, doesPanelHaveSidebar(1) checks this.state.panels[2], which references stale pre-setState state at the wrong index. The openingSidebar parameter masks this in some cases, but when there's no connections panel, the sidebar detection uses incorrect old state. The index argument here needs to be 0 to match the actual position of basePanel.
Additional Locations (1)
| from sefaria.utils.util import short_to_long_lang_code | ||
| from sefaria.utils.chatbot import build_chatbot_user_token | ||
| from sefaria.utils.hebrew import hebrew_parasha_name | ||
| from reader.views import render_react_component, _get_user_calendar_params |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused imports added to context_processors module
Low Severity
Several newly added top-level imports are unused: UserHistorySet, UserWrapper, calendars, short_to_long_lang_code, hebrew_parasha_name, render_react_component, and _get_user_calendar_params. Only UserProfile and build_chatbot_user_token are actually used by the new chatbot_user_token function. Notably, the existing code pattern in this file does a local import from reader.views (line 84) to avoid heavy top-level dependencies; the new top-level from reader.views import ... breaks that convention.
| return {"chatbot_user_token": None, "chatbot_enabled": False} | ||
| if not CHATBOT_USER_ID_SECRET: | ||
| return {"chatbot_user_token": None, "chatbot_enabled": False} | ||
| profile = UserProfile(user_obj=request.user) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expensive UserProfile created on every authenticated page load
Medium Severity
The chatbot_user_token context processor constructs a UserProfile(user_obj=request.user) for every authenticated user on every user-visible page. UserProfile.__init__ executes five MongoDB queries (profile document, followers, followees, blockers, blockees). The vast majority of users won't have experiments enabled, making these queries pure overhead. A lightweight check like user_has_experiments(request.user) (a single PostgreSQL EXISTS query) before creating the UserProfile would avoid this cost for nearly all requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
|
|
||
| # Make this unique, and don't share it with anybody. | ||
| SECRET_KEY = '' | ||
| CHATBOT_USER_ID_SECRET = 'secret' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weak default secret passes the emptiness guard
Medium Severity
CHATBOT_USER_ID_SECRET defaults to the string 'secret' in base settings and the helm chart env var fallback. The guard if not CHATBOT_USER_ID_SECRET in the context processor won't catch this because 'secret' is truthy, so tokens would silently be generated with a well-known key if the environment variable is not configured. Compare with SECRET_KEY which defaults to '' (falsy), causing an obvious failure.


Note
High Risk
Adds a new user-scoped feature flag persisted via a new Django model/migration and uses it to gate chatbot token generation and template injection, touching auth-adjacent and security-sensitive paths. Also introduces new client-side URL bootstrapping logic and updates runtime dependencies, increasing rollout risk if misconfigured.
Overview
Introduces a per-user “experiments” flag stored in a new
UserExperimentSettingsmodel (with initial migration) and exposes it via Django admin and the account settings page; profile updates now dropexperimentschanges unless the user is eligible.Adds an experimental chatbot integration gated by that flag: new settings (
CHATBOT_USER_ID_SECRET,CHATBOT_API_BASE_URL,CHATBOT_USE_LOCAL_SCRIPT), achatbot_user_tokencontext processor that mints an encrypted, expiring token, and conditional rendering/loading of the<lc-chatbot>widget inbase.html.Enhances the React reader to handle a new
sefaria:bootstrap-urlevent that can initialize panels directly from a URL (including connections/versions/lang params), and tweaks copy handling to ignore selections inside the chatbot element; dependencies are updated (cryptography,gunicorn,setuptools).Written by Cursor Bugbot for commit f9b93e8. This will update automatically on new commits. Configure here.