-
Notifications
You must be signed in to change notification settings - Fork 425
[stale] [chore] Swap sendgrid by smtp #2908
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: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR performs a major refactoring to swap email provider from SendGrid to SMTP and restructures several core modules, including:
- Removal of legacy applications, queries, and several evaluation-related tables
- Simplification of authentication logic by removing PostHog feature flag integrations
- Consolidation of database migration utilities from async to sync operations
- Renaming of evaluation entities (Results→Steps, Metrics→Metric)
- Removal of extensive test files and manual evaluation SDK examples
- Addition of new utility files and interface definitions
Reviewed Changes
Copilot reviewed 116 out of 2328 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
api/oss/src/apis/fastapi/evaluations/models.py |
Renamed evaluation entities and updated field names |
api/oss/src/apis/fastapi/applications/utils.py |
Removed entire file containing application revision parsing utilities |
api/oss/src/apis/fastapi/applications/router.py |
Removed entire legacy applications router |
api/oss/src/apis/fastapi/applications/models.py |
Removed entire file with application API models |
api/oss/src/apis/fastapi/annotations/utils.py |
Added new utility functions for annotations |
api/oss/src/apis/fastapi/annotations/router.py |
Major rewrite of annotations router with new implementation |
api/oss/src/apis/fastapi/annotations/models.py |
Restructured annotation models with new enums |
api/oss/src/__init__.py |
Simplified authentication by removing PostHog integration |
api/oss/docker/Dockerfile.gh |
Removed SDK installation and cron job setup |
api/oss/docker/Dockerfile.dev |
Removed PYTHONPATH setup and cron job configuration |
api/oss/databases/postgres/migrations/utils.py |
Converted async functions to sync for database operations |
| Multiple migration files | Removed several database migration files and updated references |
api/entrypoint.py |
Reorganized imports and router initialization |
| Multiple EE test files | Removed manual evaluation SDK test files |
| Multiple EE service files | Updated import statements and function signatures |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| span_type_enum = sa.Enum(SpanType, name="tracetype") | ||
| trace_type_enum = sa.Enum(TraceType, name="spantype") |
Copilot
AI
Nov 11, 2025
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.
The enum names are swapped. span_type_enum should use name='spantype' and trace_type_enum should use name='tracetype'. This will cause the wrong enum types to be dropped during downgrade.
| span_type_enum = sa.Enum(SpanType, name="tracetype") | |
| trace_type_enum = sa.Enum(TraceType, name="spantype") | |
| span_type_enum = sa.Enum(SpanType, name="spantype") | |
| trace_type_enum = sa.Enum(TraceType, name="tracetype") |
| ).scalar() | ||
|
|
||
| if (count or 0) > 0: | ||
| if count > 0: |
Copilot
AI
Nov 11, 2025
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.
The condition should check if (count or 0) > 0: since count could be None. The removed version handled this case, but the new version doesn't.
| if count > 0: | |
| if (count or 0) > 0: |
| status_code=409, | ||
| detail="User is already a member of the workspace", | ||
| ) | ||
| raise Exception("User is already a member of the workspace") |
Copilot
AI
Nov 11, 2025
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.
Changed from HTTPException with status 409 to generic Exception. This breaks the API contract and removes proper HTTP error handling. Should remain as HTTPException.
| "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>', |
Copilot
AI
Nov 11, 2025
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.
URL parameters are no longer URL-encoded. The email and token parameters should be URL-encoded to prevent injection issues and handle special characters properly.
| const runRes = await axios.get( | ||
| `/preview/evaluations/runs/${evaluationTableId}?project_id=${projectId}`, | ||
| ) |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
URL
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
The best way to fix the issue is to validate the evaluationTableId parameter so that only permitted (safe) values can be used as part of the outgoing request URL. Since the parameter is used in the URL path, prevent path traversal and malicious input by enforcing that it matches the expected format (e.g., alphanumeric, UUID, typical database id). In this context, a simple validation function (using a regex or type check) should be used to ensure only valid run IDs are accepted. If validation fails, the function should avoid making the request and return an error or null instead.
This change should be applied in the useEvaluationRunData hook, in the file web/ee/src/lib/hooks/useEvaluationRunData/index.ts, intercepting and validating evaluationTableId before passing it to the request at line 76.
This will require adding a helper function for validation (for example, a simple regex test for UUID or allowed characters), and updating the code to use it before the request.
-
Copy modified lines R70-R78 -
Copy modified lines R84-R94
| @@ -67,11 +67,31 @@ | ||
| const enrichRun = useEnrichEvaluationRun({debug}) | ||
|
|
||
| // New fetcher for preview runs that fetches and enriches with testsetData | ||
| // Validate evaluationTableId - allow only safe alphanumeric/uuid | ||
| function isValidEvaluationId(id: string | null): boolean { | ||
| // Accept UUIDs or alphanumeric ids (customize if needed) | ||
| if (!id) return false; | ||
| // UUIDv4 pattern or 24-char hex, or strict alphanumeric (adjust if needed) | ||
| const uuidRegex = /^[0-9a-fA-F]{24}$|^[0-9a-fA-F\-]{36}$/; | ||
| const alnumRegex = /^[a-zA-Z0-9_\-]+$/; | ||
| return uuidRegex.test(id) || alnumRegex.test(id); | ||
| } | ||
| const fetchAndEnrichPreviewRun = useCallback(async () => { | ||
| evalAtomStore().set(loadingStateAtom, (draft) => { | ||
| draft.isLoadingEvaluation = true | ||
| draft.activeStep = "eval-run" | ||
| }) | ||
| if (!isValidEvaluationId(evaluationTableId)) { | ||
| console.error("[useEvaluationRunData] Invalid evaluationTableId:", evaluationTableId) | ||
| evalAtomStore().set(loadingStateAtom, (draft) => { | ||
| draft.isLoadingEvaluation = false | ||
| draft.activeStep = null | ||
| }) | ||
| evalAtomStore().set(evaluationRunStateAtom, (draft) => { | ||
| draft.isPreview = false | ||
| }) | ||
| return null | ||
| } | ||
| const runRes = await axios.get( | ||
| `/preview/evaluations/runs/${evaluationTableId}?project_id=${projectId}`, | ||
| ) |
| export const fetchEvaluation = async (evaluationId: string) => { | ||
| const {projectId} = getCurrentProject() | ||
|
|
||
| const response = await axios.get(`/evaluations/${evaluationId}?project_id=${projectId}`) |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
URL
user-provided value
The
URL
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
General fix:
Validate and sanitize all values originating from user input (in this case evaluationId and, by implication, evaluationIds in array form) before using them to construct outbound request URLs, particularly where the path or query parameters are involved.
Best specific fix:
Since evaluationId is coming from the query parameters and may be an array (or a string), we should validate that it is a legitimate identifier. If, in the system, evaluationId is always expected to be a UUID (v4), use a regular expression (or npm package) to check for a UUID format before proceeding. If invalid, throw an error or ignore the request.
The validation should be performed at the entry point—in this case, in the consumer file (EvaluationCompare.tsx), where we parse and use the query. We should filter the evaluationIds to only include valid UUIDs, so that only these are ever passed down to the API calls. This prevents any malformed or malicious input from reaching the API layer.
What to change:
- In
EvaluationCompare.tsx, filter and validateevaluationIdsbefore passing them tofetchAllComparisonResults. - Optionally, validate/sanitize again in the API service layer as a defense-in-depth measure (optional since we cannot change the backend and may only edit the frontend here).
Required additions:
- Utility function for UUID validation (import one or add a regex).
- Replace assignments to
evaluationIdsArrayand/orevaluationIdsinEvaluationCompare.tsxwith validated arrays.
-
Copy modified lines R11-R14 -
Copy modified lines R82-R85 -
Copy modified lines R132-R136
| @@ -8,6 +8,10 @@ | ||
| import Link from "next/link" | ||
| import {createUseStyles} from "react-jss" | ||
|
|
||
| // UUIDv4 validation regex from RFC 4122 | ||
| function isValidUUIDv4(id: string): boolean { | ||
| return /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i.test(id) | ||
| } | ||
| import AgCustomHeader from "@/oss/components/AgCustomHeader/AgCustomHeader" | ||
| import CompareOutputDiff from "@/oss/components/CompareOutputDiff/CompareOutputDiff" | ||
| import {useAppTheme} from "@/oss/components/Layout/ThemeContextProvider" | ||
| @@ -75,7 +79,10 @@ | ||
| const classes = useStyles() | ||
| const {appTheme} = useAppTheme() | ||
| const [evaluationIdsStr = ""] = useQueryParam("evaluations") | ||
| const evaluationIdsArray = evaluationIdsStr.split(",").filter((item) => !!item) | ||
| const evaluationIdsArray = evaluationIdsStr | ||
| .split(",") | ||
| .map((item) => item.trim()) | ||
| .filter((item) => isValidUUIDv4(item)) | ||
| const [evalIds, setEvalIds] = useState(evaluationIdsArray) | ||
| const [hiddenVariants, setHiddenVariants] = useState<string[]>([]) | ||
| const [fetching, setFetching] = useState(false) | ||
| @@ -122,7 +129,11 @@ | ||
| }, [variants]) | ||
|
|
||
| const evaluationIds = useMemo( | ||
| () => evaluationIdsStr.split(",").filter((item) => !!item), | ||
| () => | ||
| evaluationIdsStr | ||
| .split(",") | ||
| .map((item) => item.trim()) | ||
| .filter((item) => isValidUUIDv4(item)), | ||
| [evaluationIdsStr], | ||
| ) | ||
|
|
| const {projectId} = getCurrentProject() | ||
|
|
||
| const [{data: evaluationScenarios}, evaluation] = await Promise.all([ | ||
| axios.get(`/evaluations/${evaluationId}/evaluation_scenarios?project_id=${projectId}`), |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
URL
user-provided value
The
URL
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
General fix:
Sanitize and validate all user-controlled input before using it in constructing URLs for backend requests, especially where these control resource ids or path fragments.
Detailed fix:
- Before passing
evaluationIdsintofetchAllComparisonResults, ensure every item in the array is both present and matches the expected format (likely a UUID – as per the application context). - In
fetchAllEvaluationScenarios, strongly validateevaluationIdbefore interpolating it into the URL. - Ideally, filter the array to include only properly formatted UUIDs, or throw an error if any id is invalid.
Changes required:
- In
web/ee/src/services/evaluations/api/index.ts, add a function to check that a string is a valid UUID (v4). - In
fetchAllComparisonResults, validate/filter allevaluationIdswith this function before proceeding. - Alternatively (or also), validate
evaluationIdat the start offetchAllEvaluationScenariosand throw if invalid. - Import a commonly-used UUID validation library, e.g.,
validator'sisUUID, or implement a regex-based validator for UUID v4.
-
Copy modified line R2 -
Copy modified lines R199-R201 -
Copy modified lines R229-R233 -
Copy modified lines R235-R240
| @@ -1,5 +1,5 @@ | ||
| import uniqBy from "lodash/uniqBy" | ||
| import {v4 as uuidv4} from "uuid" | ||
| import {v4 as uuidv4, validate as validateUuid, version as uuidVersion} from "uuid" | ||
|
|
||
| import {getCurrentProject} from "@/oss/contexts/project.context" | ||
| import axios from "@/oss/lib/api/assets/axiosConfig" | ||
| @@ -196,6 +196,9 @@ | ||
|
|
||
| // Evaluation Scenarios | ||
| export const fetchAllEvaluationScenarios = async (evaluationId: string) => { | ||
| if (!isValidUuidV4(evaluationId)) { | ||
| throw new Error("Invalid evaluationId") | ||
| } | ||
| const {projectId} = getCurrentProject() | ||
|
|
||
| const [{data: evaluationScenarios}, evaluation] = await Promise.all([ | ||
| @@ -223,8 +226,18 @@ | ||
| } | ||
|
|
||
| // Comparison | ||
| // Helper to check UUIDv4 | ||
| function isValidUuidV4(id: string): boolean { | ||
| return validateUuid(id) && uuidVersion(id) === 4; | ||
| } | ||
|
|
||
| export const fetchAllComparisonResults = async (evaluationIds: string[]) => { | ||
| const scenarioGroups = await Promise.all(evaluationIds.map(fetchAllEvaluationScenarios)) | ||
| // Only proceed with valid UUID v4 ids | ||
| const filteredIds = evaluationIds.filter(isValidUuidV4) | ||
| if (filteredIds.length !== evaluationIds.length) { | ||
| throw new Error('One or more evaluationIds are invalid') | ||
| } | ||
| const scenarioGroups = await Promise.all(filteredIds.map(fetchAllEvaluationScenarios)) | ||
| const testset: TestSet = await fetchTestset(scenarioGroups[0][0].evaluation?.testset?.id) | ||
|
|
||
| const inputsNameSet = new Set<string>() |
| const res = await fetch( | ||
| `${apiUrl}/preview/evaluations/scenarios/${scenarioId}?project_id=${projectId}`, | ||
| { | ||
| headers: {Authorization: `Bearer ${jwt}`}, | ||
| }, | ||
| ) |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
URL
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
The best way to fix this SSRF vulnerability is to strictly validate scenarioId before using it as a path segment of an outgoing HTTP request. Rather than using the raw, untrusted query parameter, we should ensure that scenarioId is present in a list of known safe scenario IDs—these should be IDs obtained from the backend (e.g., as part of the evaluation run's permitted scenario IDs), not directly from user input. This means in SingleScenarioViewer, when resolving the scenario ID from router query, we must validate that it is among the permissible scenario IDs. Only allow it to propagate further if it's safe. Otherwise, select a default safe scenarioId (such as the first scenario ID returned from the backend).
The only changes needed are in the block of code from SingleScenarioViewer (file: web/ee/src/components/EvalRunDetails/components/SingleScenarioViewer/index.tsx), specifically in the way activeId and currentScenarioId are resolved from router query. We should update the code so that when determining activeId, we pick from the intersection of the loaded, valid scenarioIds.
No library imports are needed, as simple array inclusion suffices.
-
Copy modified lines R35-R39 -
Copy modified line R41 -
Copy modified lines R48-R52 -
Copy modified line R54 -
Copy modified line R56
| @@ -32,18 +32,28 @@ | ||
| const setUrlState = useSetAtom(setUrlStateAtom) | ||
|
|
||
| // Prefer URL query first, then atom, then fallback | ||
| // Only accept scenarioId from router.query if found in the valid scenarios list. | ||
| const safeScenarioIdFromQuery = | ||
| typeof router.query.scenarioId === "string" && scenarioIds.includes(router.query.scenarioId) | ||
| ? router.query.scenarioId | ||
| : undefined | ||
| const activeId = | ||
| (router.query.scenarioId as string | undefined) ?? urlState.scenarioId ?? scenarioIds[0] | ||
| safeScenarioIdFromQuery ?? (scenarioIds.includes(urlState.scenarioId) ? urlState.scenarioId : undefined) ?? scenarioIds[0] | ||
|
|
||
| // Ensure URL/atom always reference a scenario visible in current list | ||
| // Ensure URL/atom correctness | ||
| useEffect(() => { | ||
| if (scenarioIds.length === 0) return | ||
|
|
||
| // Only consider router.query.scenarioId if present in allowed list | ||
| const safeScenarioIdFromQuery = | ||
| typeof router.query.scenarioId === "string" && scenarioIds.includes(router.query.scenarioId) | ||
| ? router.query.scenarioId | ||
| : undefined | ||
| const currentScenarioId = | ||
| (router.query.scenarioId as string | undefined) ?? urlState.scenarioId | ||
| safeScenarioIdFromQuery ?? (scenarioIds.includes(urlState.scenarioId) ? urlState.scenarioId : undefined) | ||
|
|
||
| if (!currentScenarioId || !scenarioIds.includes(currentScenarioId)) { | ||
| if (!currentScenarioId) { | ||
| // Default to the first scenario for this run when no valid selection/deep-link. | ||
| setUrlState({scenarioId: scenarioIds[0]}) | ||
| return |
| return await axios | ||
| .get(`${getAgentaApiUrl()}/human-evaluations/${evaluationId}?project_id=${projectId}`) |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
URL
user-provided value
The
URL
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To mitigate this SSRF risk, restrict evaluationId to an expected format before using it in the request path, e.g., ensure it matches allowed characters (such as a MongoDB ObjectId or a UUID). The best way to do this is by validating the evaluationId string in fetchLoadEvaluation before including it in the axios request. If the value fails validation, throw an error or return a controlled response instead of making the HTTP request. The validation should be implemented right at the start of fetchLoadEvaluation to ensure early exit if the input is malicious or malformed.
To implement the change:
- In
web/ee/src/services/human-evaluations/api/index.ts, update thefetchLoadEvaluationfunction (lines 84–96). - Before making the axios request, validate that
evaluationIdmatches the expected format, for example, checking if it's a 24-character hexadecimal string (for Mongo ObjectId), or matches a UUID regex if that's appropriate. - If validation fails, log an error and return
null(or throw, depending on desired error handling). - Import or define a helper validation function if needed.
No code changes are needed in the component/page that reads router.query.evaluation_id.
-
Copy modified lines R84-R86 -
Copy modified lines R89-R93
| @@ -81,8 +81,16 @@ | ||
| return results | ||
| } | ||
|
|
||
| // Validate ObjectId: 24-character hex string | ||
| const isValidObjectId = (id: string): boolean => /^[a-fA-F0-9]{24}$/.test(id); | ||
|
|
||
| export const fetchLoadEvaluation = async (evaluationId: string) => { | ||
| const {projectId} = getCurrentProject() | ||
| // SSRF mitigation: Validate evaluationId before interpolating | ||
| if (!isValidObjectId(evaluationId)) { | ||
| console.error(`Invalid evaluationId format: ${evaluationId}`) | ||
| return null | ||
| } | ||
| try { | ||
| return await axios | ||
| .get(`${getAgentaApiUrl()}/human-evaluations/${evaluationId}?project_id=${projectId}`) |
| return fromEvaluationResponseToEvaluation(responseData.data) | ||
| }) | ||
| } catch (error) { | ||
| console.error(`Error fetching evaluation ${evaluationId}:`, error) |
Check failure
Code scanning / CodeQL
Use of externally-controlled format string High
user-provided value
Format string depends on a
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To prevent the risk of user input being interpreted as a format string by console.error, the recommended fix is to use a static string as the format and pass the potentially-tainted value as a separate argument. Specifically, change from:
console.error(`Error fetching evaluation ${evaluationId}:`, error)to:
console.error("Error fetching evaluation %s:", evaluationId, error)This ensures that evaluationId will always be treated as a string and not as part of a format string, thus eliminating the opportunity for input-controlled format confusion. Only the specific console.error invocation on line 93 of web/ee/src/services/human-evaluations/api/index.ts needs to be updated. No extra imports or definitions are needed.
-
Copy modified line R93
| @@ -90,7 +90,7 @@ | ||
| return fromEvaluationResponseToEvaluation(responseData.data) | ||
| }) | ||
| } catch (error) { | ||
| console.error(`Error fetching evaluation ${evaluationId}:`, error) | ||
| console.error("Error fetching evaluation %s:", evaluationId, error) | ||
| return null | ||
| } | ||
| } |
| return await axios | ||
| .get( | ||
| `${getAgentaApiUrl()}/human-evaluations/${evaluationTableId}/evaluation_scenarios?project_id=${projectId}`, | ||
| ) |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
URL
user-provided value
The
URL
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
The best way to fix this SSRF risk is to validate and sanitize the evaluationTableId before using it in any URL path construction. Since the identifier is expected to represent an evaluation table, we should restrict its format to a known-safe pattern (e.g., MongoDB ObjectId, UUID, or some similar canonical form). In practical terms, this means:
- In
fetchAllLoadEvaluationsScenarios(web/ee/src/services/human-evaluations/api/index.ts), only allowevaluationTableIdif it matches a strict regular expression that describes the intended ID format (e.g., 24-character hex string for MongoDB ObjectId, or UUID). - Throw/return an error or empty result if the ID is invalid, and optionally log the attempt.
- Add a helper function in the same file to validate the ID, and use it at the start of the exported method.
You only need to change code in web/ee/src/services/human-evaluations/api/index.ts, specifically around the fetchAllLoadEvaluationsScenarios function. The imported modules are standard/common, so you may add e.g. a regex or a helper validation inline there.
-
Copy modified lines R109-R117 -
Copy modified lines R122-R126
| @@ -106,10 +106,24 @@ | ||
| return response.data | ||
| } | ||
|
|
||
| // Helper: only allow 24-character Mongo ObjectId or UUID v4 | ||
| function isSafeEvaluationId(id: string): boolean { | ||
| // Mongo ObjectId: 24 hex chars | ||
| const mongoId = /^[0-9a-fA-F]{24}$/ | ||
| // UUID v4: xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx | ||
| const uuidV4 = /^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-4[0-9a-fA-F]{3}-[89abAB][0-9a-fA-F]{3}-[0-9a-fA-F]{12}$/ | ||
| return mongoId.test(id) || uuidV4.test(id) | ||
| } | ||
|
|
||
| export const fetchAllLoadEvaluationsScenarios = async ( | ||
| evaluationTableId: string, | ||
| evaluation: Evaluation, | ||
| ) => { | ||
| if (!isSafeEvaluationId(evaluationTableId)) { | ||
| console.error("Invalid evaluationTableId value supplied:", evaluationTableId) | ||
| // Optionally: return empty array, null, or throw error | ||
| return [] | ||
| } | ||
| const {projectId} = getCurrentProject() | ||
|
|
||
| return await axios |
| const response = await axios.put( | ||
| `${getAgentaApiUrl()}/human-evaluations/${evaluationTableId}/evaluation_scenario/${evaluationScenarioId}/${evaluationType}?project_id=${projectId}`, | ||
| data, | ||
| ) |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
URL
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To correct the problem, you should validate (sanitize) all user-controlled input used in REST API paths, especially variables coming from the URL query (evaluationScenarioId in this case). The best fix is to ensure evaluationScenarioId matches only the allowed scenario IDs -- which are already present in-memory as part of evaluationScenarios (see usage in both components). This requires, before calling updateEvaluationScenario (or in that method), checking that evaluationScenarioId appears in evaluationScenarios.map(s => s.id). If it is not present, the operation should be aborted or an error raised.
The implementation can be made at the point where scenarioId (from useQueryParam) is passed to updateEvaluationScenarioData (and thus eventually becomes evaluationScenarioId). You should add validation in both web/ee/src/components/Evaluations/EvaluationCardView/index.tsx (where scenarioId is used) and/or in web/ee/src/components/EvaluationTable/SingleModelEvaluationTable.tsx (where id is used for updating scenarios), so that only known IDs from evaluationScenarios are accepted.
You will need to:
- Add a helper function that checks if a scenarioId is valid.
- Apply this check before passing
scenarioId(orid) to SSRF sinks. - Prevent requests with invalid or malicious IDs.
No new imports are strictly necessary.
-
Copy modified lines R191-R196
| @@ -188,6 +188,12 @@ | ||
| data: Partial<EvaluationScenario>, | ||
| showNotification = true, | ||
| ) => { | ||
| // SSRF mitigation: Only update scenarios with valid IDs | ||
| const allowedScenarioIds = evaluationScenarios.map(s => s.id) | ||
| if (!allowedScenarioIds.includes(id)) { | ||
| console.warn("Blocked invalid evaluationScenarioId for SSRF protection:", id) | ||
| return | ||
| } | ||
| await updateEvaluationScenario( | ||
| evaluation.id, | ||
| id, |
-
Copy modified lines R134-R140 -
Copy modified line R143
| @@ -131,9 +131,16 @@ | ||
|
|
||
| const depouncedUpdateEvaluationScenario = useCallback( | ||
| debounce((data: Partial<EvaluationScenario>) => { | ||
| // SSRF mitigation: Only use scenarioId if it is present in evaluationScenarios | ||
| const allowedScenarioIds = evaluationScenarios.map(s => s.id) | ||
| if (!allowedScenarioIds.includes(scenarioId)) { | ||
| // Optionally, log, show an error, or silently return | ||
| console.warn("Blocked invalid scenarioId for SSRF protection:", scenarioId) | ||
| return | ||
| } | ||
| updateEvaluationScenarioData(scenarioId, data) | ||
| }, 800), | ||
| [scenarioId], | ||
| [scenarioId, evaluationScenarios], | ||
| ) | ||
|
|
||
| const onChatChange = (chat: ChatMessage[]) => { |
Chore/swap sendgrid by smtp