fix(evidence): include auth headers for evidence-trail fetches#19504
fix(evidence): include auth headers for evidence-trail fetches#19504BrianCLong wants to merge 1 commit intomainfrom
Conversation
BrianCLong
added
the
codex
ChatGPT Codex Connector
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an authentication issue in the Evidence-Trail Peek UI by ensuring that all client-side fetches to evidence-related API endpoints include the necessary authentication headers. This change guarantees that the UI can successfully retrieve and display tenant-specific data, thereby enhancing the security and correctness of the evidence-trail feature. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the 'Evidence-Trail Peek' feature, adding a UI overlay to display evidence for answers and graph nodes, encompassing new frontend components, read-only API endpoints, telemetry, and documentation. A significant security vulnerability identified is a potential Cross-Site Scripting (XSS) issue in the UI component, where untrusted URLs from the API are directly rendered into href attributes; proper sanitization is crucial to prevent malicious script execution. Additionally, potential security vulnerabilities related to token storage and tenant handling need to be addressed. On a positive note, access control and SQL injection prevention seem correctly implemented through tenant scoping and parameterized queries. The review also includes suggestions to enhance type safety and maintainability in the new components.
| const buildAuthHeaders = () => { | ||
| const token = localStorage.getItem('auth_token'); | ||
| if (!token) { | ||
| return { | ||
| 'Content-Type': 'application/json', | ||
| }; | ||
| } | ||
| return { | ||
| 'Content-Type': 'application/json', | ||
| Authorization: `Bearer ${token}`, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Storing authentication tokens in localStorage is a security risk as it makes the token accessible to any script running on the page, which can lead to token theft via Cross-Site Scripting (XSS) attacks. It is strongly recommended to store sensitive tokens in httpOnly cookies to prevent access from JavaScript. If using httpOnly cookies is not feasible, ensure that you have strong Content Security Policy (CSP) and other measures to mitigate XSS vulnerabilities.
| return [] as EvidenceBadge[]; | ||
| } | ||
|
|
||
| const href = `/api/provenance-beta/evidence/${evidenceId}?tenant=${encodeURIComponent(tenantId)}`; |
There was a problem hiding this comment.
The generated href for evidence badges includes the tenantId as a query parameter. This is a potential security vulnerability. The receiving endpoint (/api/provenance-beta/evidence/:id) must not trust this parameter. It should exclusively use the tenantId from the authenticated user's session (req.user.tenantId) for database queries. Relying on a client-controlled query parameter for tenancy checks can lead to Insecure Direct Object Reference (IDOR) vulnerabilities, allowing users to potentially access data from other tenants. Please verify that the target endpoint correctly ignores this query parameter in favor of the session tenant ID.
| </div> | ||
| {artifact.location && ( | ||
| <a | ||
| href={artifact.location} |
There was a problem hiding this comment.
The artifact.location value is directly assigned to the href attribute of an <a> tag without sanitization. If an attacker can control the storage_uri of an evidence artifact (e.g., through a malicious ingestion source), they could inject a javascript: URL, leading to execution of arbitrary JavaScript in the user's browser when the link is clicked.
| href={artifact.location} | |
| href={artifact.location.startsWith('javascript:') ? '#' : artifact.location} |
| <> | ||
| {' '} | ||
| <a | ||
| href={support.location} |
There was a problem hiding this comment.
The support.location value is used directly in an href attribute without sanitization. This allows for potential XSS if the location contains a javascript: URL.
| href={support.location} | |
| href={support.location.startsWith('javascript:') ? '#' : support.location} |
| }, [selectEntity]); | ||
|
|
||
| const handleNodeRightClick = useCallback( | ||
| (node: any, event?: MouseEvent) => { |
There was a problem hiding this comment.
The node parameter in handleNodeRightClick is typed as any. For better type safety and code maintainability, it's recommended to use a more specific type. Since only node.id is used in this function, you can type it as an object with an id property.
| (node: any, event?: MouseEvent) => { | |
| (node: { id: string }, event?: MouseEvent) => { |
| return Math.min(value, max); | ||
| }; | ||
|
|
||
| const getTenantId = (req: any) => req.user?.tenantId || req.user?.tenant_id || 'unknown'; |
There was a problem hiding this comment.
The getTenantId function falls back to 'unknown' if a tenant ID is not found on the user object. While the subsequent database queries will likely return no results for an 'unknown' tenant (which is a safe failure mode), it's better practice to fail explicitly by throwing an error or sending an error response. This "fail-fast" approach makes it easier to detect and debug issues where a user's session might be missing a tenant ID. Also, consider providing a specific type for the req parameter instead of any for improved type safety.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Temporarily closing to reduce Actions queue saturation and unblock #22241. Reopen after the golden-main convergence PR merges. |
1 similar comment
|
Temporarily closing to reduce Actions queue saturation and unblock #22241. Reopen after the golden-main convergence PR merges. |
1 participant
Motivation
apps/web/src/components/evidence/EvidenceTrailPeek.tsx).Description
buildAuthHeadershelper that readsauth_tokenfromlocalStorageand attachesAuthorization: Bearer <token>(plusContent-Type) when present, and use it for the three parallel fetches to/api/evidence-index,/api/evidence-top, and/api/claim-rankinginEvidenceTrailPeek.Testing
server/src/routes/__tests__/evidence-trail-peek.test.ts) and the Cypress E2E spec (e2e/tests/evidence-trail-peek.cy.ts) included in the feature PR to validate behavior in an authenticated environment.Codex Task