Add refresh token rate limit and remove outdated audit docs#166
Add refresh token rate limit and remove outdated audit docs#166
Conversation
| message="Artifact was updated but patch status could not be marked as applied", | ||
| ) | ||
| if not applied_patch: | ||
| applied_patch = self.store.get_config_patch(patch_id) |
There was a problem hiding this comment.
Bug: Unbound variable updated when exception raised in try block
When an exception occurs inside the try block (lines 121-139), such as NotFoundError from apply_config_patch or any other error during artifact update, the code catches it and sets status_update_failed = True. However, the variable updated may never have been assigned at this point. The code then attempts to use updated at line 152 in result = {"artifact": updated, ...}, which will raise an UnboundLocalError. The exception handler should either re-raise the exception or set updated to a fallback value (like artifact) to avoid this crash.
| with self._state_lock: | ||
| self._oauth_states[state] = (provider, expires_at, tenant_id) | ||
| with self._with_state_lock(): | ||
| self._oauth_states[state] = (provider, expires_at, tenant_id) |
There was a problem hiding this comment.
Bug: Double lock acquisition causes deadlock
The code uses with self._state_lock: immediately followed by with self._with_state_lock():, but _with_state_lock() internally acquires the same self._state_lock. Since self._state_lock is a non-reentrant threading.Lock(), not an RLock, this double acquisition causes an immediate deadlock when the code executes. This affects OAuth state management, password reset token handling, and other auth flows.
Additional Locations (2)
| change_note=patch.justification, | ||
| ) | ||
| self.artifacts[artifact.id] = updated_artifact | ||
| self.artifact_versions.setdefault(artifact.id, []).insert(0, version) |
There was a problem hiding this comment.
Bug: Wrong index used for version calculation in memory store
The apply_config_patch method calculates next_version using versions[0].version, expecting index 0 to hold the latest version. However, create_artifact and update_artifact both use .append() to add versions to the end of the list, meaning versions[-1] contains the latest version, not versions[0]. Additionally, this method uses .insert(0, version) while other methods use .append(), creating an inconsistent data structure. This causes incorrect version numbering and potential duplicate version numbers when patches are applied to artifacts that already have multiple versions.
| # Adapter usage counts | ||
| if hasattr(runtime.store, "list_artifacts"): | ||
| try: | ||
| adapters = runtime.store.list_artifacts(kind="adapter", owner_user_id=None) # type: ignore[arg-type] |
There was a problem hiding this comment.
Bug: Wrong parameter name in metrics adapter count query
The call list_artifacts(kind="adapter", ...) uses parameter name kind, but the actual function signature uses kind_filter. Since there's no **kwargs in the function, this raises TypeError: got an unexpected keyword argument 'kind'. The exception is caught and logged as a warning, but the adapter count metric will never be populated.
Summary
/auth/refreshusing configurable defaultsTesting
Codex Task
Note
Adds a configurable, IP-scoped rate limit to POST /auth/refresh and exposes its settings via admin, while removing an outdated audit report.
POST /v1/auth/refreshusing_enforce_rate_limit(...)keyed by client IP.refresh_rate_limit_per_minuteandrefresh_rate_limit_window_secondsdefaults and enforce them.PUT /v1/admin/settings(added to allowlist and integer validation).Settingsfor CORS/HSTS/build metadata; minor metrics additions.docs/ISSUES.mdwith recent fixes (pagination, clustering, training, refresh RL items).RATE_LIMIT_SECURITY_AUDIT.md.Written by Cursor Bugbot for commit 241f0eb. This will update automatically on new commits. Configure here.