[stale] [AGE-2476] Add sidebar changelog notifications #2907
Conversation
|
GitHub CI seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
Pull Request Overview
This pull request implements a major refactoring focused on removing legacy code, consolidating database access patterns, and cleaning up deprecated migration files. The changes streamline the codebase by removing unused routers, models, and utilities while updating database session management patterns.
Key changes include:
- Removal of deprecated legacy applications, annotations, queries, testsets, evaluations, and invocations routers and services
- Consolidation of database engine usage from
engine.core_session()todb_engine.get_core_session() - Cleanup of multiple database migration files and data migration utilities
- Removal of blocked domains/emails functionality and associated PostHog feature flag logic
- Updates to entrypoint routing and middleware configuration
Reviewed Changes
Copilot reviewed 130 out of 3124 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| api/oss/src/apis/fastapi/applications/* | Removed legacy application router, models, and utilities |
| api/oss/src/apis/fastapi/annotations/* | Refactored annotations router with new implementation and added utility functions |
| api/oss/src/init.py | Removed blocked domains/emails checking logic and PostHog feature flag functionality |
| api/oss/docker/Dockerfile.* | Cleaned up Docker build steps removing SDK installation and cron setup |
| api/oss/databases/postgres/migrations/utils.py | Changed async functions to sync and updated database protocol handling |
| api/oss/databases/postgres/migrations/**/versions/* | Removed multiple migration files for evaluations, queries, testsets, and span fixes |
| api/oss/databases/postgres/migrations/runner.py | Removed migration runner script |
| api/oss/databases/postgres/migrations/core/data_migrations/* | Updated import paths and removed deprecated migration utilities |
| api/entrypoint.py | Restructured application initialization, removed legacy routers, simplified imports |
| api/ee/tests/manual/evaluations/sdk/* | Removed manual test files and notebooks |
| api/ee/src/utils/permissions.py | Uncommented API key permission checking function and added cache TTL |
| api/ee/src/utils/entitlements.py | Removed subscription caching logic |
| api/ee/src/services/* | Updated parameter names and database access patterns |
| api/ee/src/models/extended/deprecated_transfer_models.py | Renamed TestsetDB to TestSetDB |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| or env.POSTGRES_URI_CORE | ||
| or env.POSTGRES_URI_TRACING | ||
| or "postgresql+asyncpg://username:password@localhost:5432/agenta_oss" | ||
| or "postgresql://username:password@localhost:5432/agenta_oss" |
There was a problem hiding this comment.
Corrected spelling of 'recieve' to 'receive'.
| status_code=409, | ||
| detail="User is already a member of the workspace", | ||
| ) | ||
| raise Exception("User is already a member of the workspace") |
There was a problem hiding this comment.
This generic Exception should be replaced with HTTPException to maintain consistency with the rest of the codebase and provide proper HTTP status codes to API consumers.
| import sqlalchemy as sa | ||
|
|
||
| from oss.src.dbs.postgres.secrets.custom_fields import PGPString | ||
| from oss.src.dbs.secrets.custom_fields import PGPString |
There was a problem hiding this comment.
Import path changed from oss.src.dbs.postgres.secrets.custom_fields to oss.src.dbs.secrets.custom_fields. Verify that this import path is correct and the module exists at this location.
| from oss.src.dbs.secrets.custom_fields import PGPString | |
| from oss.src.dbs.postgres.secrets.custom_fields import PGPString |
| "Click the link below to accept the invitation:</p><br>" | ||
| f'<a href="{invite_link}">Accept Invitation</a>' | ||
| ), | ||
| call_to_action=f'Click the link below to accept the invitation:</p><br><a href="{env.AGENTA_WEB_URL}/auth?token={token}&email={email}&org_id={organization.id}&workspace_id={workspace.id}&project_id={project_id}">Accept Invitation</a>', |
There was a problem hiding this comment.
URL parameters are not properly encoded, which could lead to injection vulnerabilities or broken links. The removed quote() calls should be restored or URL parameters should be properly encoded using urllib.parse.urlencode() or similar.
| project_id, payload_invite.email | ||
| ) | ||
| if not existing_invitation and not existing_role: | ||
| if not existing_invitation: |
There was a problem hiding this comment.
The function previously returned two values (existing_invitation, existing_role) but now only returns one. The conditional check at line 179 needs to handle the case where existing_invitation might be None differently, and the logic for handling existing roles appears to be lost.
| if not existing_invitation: | |
| # Check if the user already has a role in the workspace | |
| existing_role = await db_manager_ee.get_user_role_in_workspace_by_email( | |
| payload_invite.email, str(workspace.id) | |
| ) | |
| if not existing_invitation and not existing_role: |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const response = await axios.get(`/evaluations/${encodeURIComponent(id)}`, { | ||
| params: {project_id: projectId}, | ||
| }) | ||
| const response = await axios.get(`/api/evaluations/${evaluationId}?project_id=${projectId}`) |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 months ago
The best way to fix this SSRF risk is to validate the evaluationId before constructing or sending the HTTP request. Since evaluationId is a path segment and apparently represents an evaluation UUID, we should:
- Permit only fixed, safe formats for
evaluationId(e.g., UUID v4 or possible known formats). - Implement validation on the client-side before using the value to construct the outgoing request.
- Reject or ignore any values that do not match the expected format, preventing path traversal or invalid resource lookups.
To implement the fix in the relevant file (web/oss/src/services/evaluations/api/index.ts), do the following:
- Import a well-established UUID validation library, such as
validator'sisUUID(sinceevaluationIdlikely should be a UUID). - Add a validation step in
fetchEvaluation,fetchEvaluationStatus, andfetchAllEvaluationScenariosfunctions to check thatevaluationIdis a valid UUID. If not, throw an error or return safely. - If
evaluationIdsis an array (as infetchAllComparisonResults), validate all elements. - You do not need to validate server-side, but strong client-side validation makes exploitation much less likely.
| @@ -3,6 +3,7 @@ | ||
|
|
||
| import {getCurrentProject} from "@/oss/contexts/project.context" | ||
| import axios from "@/oss/lib/api/assets/axiosConfig" | ||
| import isUUID from "validator/lib/isUUID" | ||
| import {getTagColors} from "@/oss/lib/helpers/colors" | ||
| import {calcEvalDuration} from "@/oss/lib/helpers/evaluate" | ||
| import {isDemo, stringToNumberInRange} from "@/oss/lib/helpers/utils" | ||
| @@ -144,6 +145,9 @@ | ||
| } | ||
|
|
||
| export const fetchEvaluation = async (evaluationId: string) => { | ||
| if (!isUUID(evaluationId, 4)) { | ||
| throw new Error("Invalid evaluation ID format.") | ||
| } | ||
| const {projectId} = getCurrentProject() | ||
|
|
||
| const response = await axios.get(`/api/evaluations/${evaluationId}?project_id=${projectId}`) | ||
| @@ -151,6 +155,9 @@ | ||
| } | ||
|
|
||
| export const fetchEvaluationStatus = async (evaluationId: string) => { | ||
| if (!isUUID(evaluationId, 4)) { | ||
| throw new Error("Invalid evaluation ID format.") | ||
| } | ||
| const {projectId} = getCurrentProject() | ||
|
|
||
| const response = await axios.get( | ||
| @@ -183,6 +190,9 @@ | ||
|
|
||
| // Evaluation Scenarios | ||
| export const fetchAllEvaluationScenarios = async (evaluationId: string) => { | ||
| if (!isUUID(evaluationId, 4)) { | ||
| throw new Error("Invalid evaluation ID format.") | ||
| } | ||
| const {projectId} = getCurrentProject() | ||
|
|
||
| const [{data: evaluationScenarios}, evaluation] = await Promise.all([ | ||
| @@ -201,7 +211,12 @@ | ||
|
|
||
| // Comparison | ||
| export const fetchAllComparisonResults = async (evaluationIds: string[]) => { | ||
| const scenarioGroups = await Promise.all(evaluationIds.map(fetchAllEvaluationScenarios)) | ||
| // Validate all IDs before making requests | ||
| const validIds = evaluationIds.filter((id) => isUUID(id, 4)) | ||
| if (validIds.length === 0) { | ||
| throw new Error("No valid evaluation IDs provided.") | ||
| } | ||
| const scenarioGroups = await Promise.all(validIds.map(fetchAllEvaluationScenarios)) | ||
| const testset: TestSet = await fetchTestset(scenarioGroups[0][0].evaluation?.testset?.id) | ||
|
|
||
| const inputsNameSet = new Set<string>() |
| @@ -114,7 +114,8 @@ | ||
| "use-animation-frame": "^0.2.1", | ||
| "usehooks-ts": "^3.1.1", | ||
| "uuid": "^9.0.1", | ||
| "uuidjs": "^5.1.0" | ||
| "uuidjs": "^5.1.0", | ||
| "validator": "^13.15.23" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/node": "^20.8.10", |
| Package | Version | Security advisories |
| validator (npm) | 13.15.23 | None |
| return await axios | ||
| .get(`${getAgentaApiUrl()}/human-evaluations/${evaluationId}?project_id=${projectId}`) |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 months ago
How to fix:
- The best general fix is to strictly validate or sanitize the
evaluationIdreceived from untrusted sources before passing it into the backend API call. Ideally, this means enforcing thatevaluationIdmatches the expected format (for example, a UUID, hex string, MongoDB ObjectId, or whatever the legitimate values are). - We should implement this input validation as close as possible to where the untrusted value enters the system, but at minimum right before it is interpolated into the URL in the backend request.
- In the shown code, the responsible place is inside
fetchLoadEvaluation(inweb/oss/src/services/human-evaluations/api/index.ts)—here, validateevaluationId. If invalid, throw an error or return an error promise instead of making the backend request.
Specific steps:
- Add a helper function that validates whether a value is a valid evaluation ID (based on expected format, e.g., a 24-char hex for MongoDB ObjectIds, or otherwise a suitable regular expression).
- In
fetchLoadEvaluation, before making the API call, check thatevaluationIdpasses the validation. If not, throw an error or return a rejected promise. - Optionally, you may also sanitize edge input earlier (in
index.tsx), but it is critical that any place where a potentially unsafe value is sent to the backend, validation is enforced. - If required, import a suitable validation helper or define a minimal regexp-based function inline.
| @@ -81,7 +81,15 @@ | ||
| return results | ||
| } | ||
|
|
||
| // Utility to validate a MongoDB ObjectId (24 hex chars), update regexp as needed for your ID format. | ||
| function isValidEvaluationId(id: string) { | ||
| return typeof id === "string" && /^[a-fA-F0-9]{24}$/.test(id); | ||
| } | ||
|
|
||
| export const fetchLoadEvaluation = async (evaluationId: string) => { | ||
| if (!isValidEvaluationId(evaluationId)) { | ||
| throw new Error("Invalid evaluationId format"); | ||
| } | ||
| const {projectId} = getCurrentProject() | ||
|
|
||
| return await axios |
| const response = await axios.get( | ||
| `${getAgentaApiUrl()}/testsets/${testsetId}?project_id=${projectId}`, | ||
| ) |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 months ago
To remediate SSRF risk, it is essential to validate or sanitize the user-supplied testset_id before it is interpolated into the request URL. Since testsetId is expected to be a valid string identifier for a testset, ideally a UUID or database identifier, the frontend can apply a whitelist validation (e.g., regex test for allowed identifier formats). This should occur before calling fetchTestset.
Specifically, in TestsetTable.tsx, we should check that testset_id is either:
- a non-empty string
- matches an explicit pattern (e.g., UUID, or alphanumeric, dashes/underscores)
If it does not match, either:
- refuse to make the API call (show error message/client-side fallback)
- use a placeholder/fallback value
Concretely, in TestsetTable.tsx, place logic prior to calling fetchTestset, to only allow testset_ids with allowed characters (e.g., /^[a-zA-Z0-9_-]+$/).
No changes to the API layer are required if this input is validated at the call-site.
| @@ -128,7 +128,12 @@ | ||
| setInputValues(newColDefs.filter((col) => !!col.field).map((col) => col.field)) | ||
| } | ||
|
|
||
| if (writeMode === "edit" && testset_id && isProjectId) { | ||
| // SSRF fix: validate testset_id before using | ||
| const isSafeTestsetId = | ||
| typeof testset_id === "string" && | ||
| /^[a-zA-Z0-9_-]+$/.test(testset_id) && // <-- update this regexp if UUID is required | ||
| testset_id.length > 0; | ||
| if (writeMode === "edit" && isSafeTestsetId && isProjectId) { | ||
| fetchTestset(testset_id as string).then((data) => { | ||
| setTestsetName(data.name) | ||
| if (data.csvdata.length > 0) { |
junaway
changed the title
[AGE-2476] Add sidebar changelog notifications