-
Notifications
You must be signed in to change notification settings - Fork 53
Enable full-text search keyword extraction, match counting, and contextual highlighting in log components #355
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
Reviewer's GuideThis PR implements full-text search highlighting across log entries by parsing the current query to extract keywords, counting matches in content and tags, and injecting highlighted HTML based on the theme. It also propagates highlight state to dropdown tag components, refines editor theming and styles, updates proxy configuration, removes the auth guard, and adds a scheduler dependency. Sequence diagram for full-text search highlighting in log renderingsequenceDiagram
participant User
participant LogItem
participant LogKeyTagValue
participant LogsContext
participant UI
User->>LogItem: View log entry
LogItem->>LogsContext: Get current query
LogItem->>LogKeyTagValue: Pass log content and query
LogKeyTagValue->>LogsContext: Extract keywords from query
LogKeyTagValue->>LogKeyTagValue: Count matches in content
LogKeyTagValue->>UI: Render content with highlighted keywords
UI->>User: Display highlighted log entry
Class diagram for updated log tag and value highlighting componentsclassDiagram
class LogTagDropDown {
+objKey
+value
+children
+trigger
+isFieldInQuery(query, objKey, value)
+isHighlighted
}
class LogKeyTagValue {
+title
+description
+isHighlighted
+extractFullTextSearchKeywords(query)
+countMatches(text, keywords)
+highlightText(text, keywords, theme)
}
class KeyTagValueWithHighlight {
+title
+description
+isHighlighted
}
class TagWithHighlight {
+objKey
+value
+isHighlighted
+getHighlightStyle()
}
LogTagDropDown o-- LogKeyTagValue
LogKeyTagValue <|-- KeyTagValueWithHighlight
LogTagDropDown o-- TagWithHighlight
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Detection of dangerouslySetInnerHTML from non-constant definition. This can inadvertently expose users to cross-site scripting (XSS) attacks if this comes from user-provided input. If you have to use dangerouslySetInnerHTML, consider using a sanitization library such as DOMPurify to sanitize your HTML. (link)
- Detection of dangerouslySetInnerHTML from non-constant definition. This can inadvertently expose users to cross-site scripting (XSS) attacks if this comes from user-provided input. If you have to use dangerouslySetInnerHTML, consider using a sanitization library such as DOMPurify to sanitize your HTML. (link)
- Detection of dangerouslySetInnerHTML from non-constant definition. This can inadvertently expose users to cross-site scripting (XSS) attacks if this comes from user-provided input. If you have to use dangerouslySetInnerHTML, consider using a sanitization library such as DOMPurify to sanitize your HTML. (link)
- Directly injecting HTML for highlighting may introduce XSS risks. (link)
- Authorization check is bypassed, which may expose protected routes. (link)
General comments:
- There’s duplicated logic for extracting and matching full-text keywords across components—extract these into a shared hook or util to DRY up the code.
- Highlighting via dangerouslySetInnerHTML could introduce XSS risks—consider sanitizing the HTML or using a safer React text-highlighting library.
- The AuthRouter override currently bypasses all authorization checks—please re-enable or clearly document the intended auth flow.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There’s duplicated logic for extracting and matching full-text keywords across components—extract these into a shared hook or util to DRY up the code.
- Highlighting via dangerouslySetInnerHTML could introduce XSS risks—consider sanitizing the HTML or using a safer React text-highlighting library.
- The AuthRouter override currently bypasses all authorization checks—please re-enable or clearly document the intended auth flow.
## Individual Comments
### Comment 1
<location> `frontend/src/oss/views/logs/component/FullLogs/component/LogQueryResult/QueryList/LogItem/component/LogKeyTagValue.jsx:121` </location>
<code_context>
+ keywords.forEach(keyword => {
+ if (keyword && keyword.trim()) {
+ const regex = new RegExp(`(${keyword.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')})`, 'gi')
+ highlightedText = highlightedText.replace(regex, `<span style="color: ${highlightColor}; font-weight: bold;">$1</span>`)
+ }
+ })
</code_context>
<issue_to_address>
Directly injecting HTML for highlighting may introduce XSS risks.
If log content includes user data, sanitize input or use a safer highlighting method to prevent XSS.
</issue_to_address>
### Comment 2
<location> `frontend/src/core/components/AuthRouter.tsx:40` </location>
<code_context>
)
}
- return isAllowed ? <WrappedComponent {...props} /> : <FallbackPage errorInfo={t('routeError')} />
+ // return isAllowed ? <WrappedComponent {...props} /> : <FallbackPage errorInfo={t('routeError')} />
+ return <WrappedComponent {...props} />
</code_context>
<issue_to_address>
Authorization check is bypassed, which may expose protected routes.
Disabling the isAllowed check removes route protection and may allow unauthorized access. Please confirm this change is intentional and restore the check before production if needed.
</issue_to_address>
## Security Issues
### Issue 1
<location> `frontend/src/oss/views/logs/component/FullLogs/component/LogQueryResult/QueryList/LogItem/component/LogKeyTagValue.jsx:211` </location>
<issue_to_address>
**security (typescript.react.security.audit.react-dangerouslysetinnerhtml):** Detection of dangerouslySetInnerHTML from non-constant definition. This can inadvertently expose users to cross-site scripting (XSS) attacks if this comes from user-provided input. If you have to use dangerouslySetInnerHTML, consider using a sanitization library such as DOMPurify to sanitize your HTML.
*Source: opengrep*
</issue_to_address>
### Issue 2
<location> `frontend/src/oss/views/logs/component/FullLogs/component/LogQueryResult/QueryList/LogItem/component/LogKeyTagValue.jsx:221` </location>
<issue_to_address>
**security (typescript.react.security.audit.react-dangerouslysetinnerhtml):** Detection of dangerouslySetInnerHTML from non-constant definition. This can inadvertently expose users to cross-site scripting (XSS) attacks if this comes from user-provided input. If you have to use dangerouslySetInnerHTML, consider using a sanitization library such as DOMPurify to sanitize your HTML.
*Source: opengrep*
</issue_to_address>
### Issue 3
<location> `frontend/src/oss/views/logs/component/FullLogs/component/LogQueryResult/QueryList/LogItem/component/LogKeyTagValue.jsx:256` </location>
<issue_to_address>
**security (typescript.react.security.audit.react-dangerouslysetinnerhtml):** Detection of dangerouslySetInnerHTML from non-constant definition. This can inadvertently expose users to cross-site scripting (XSS) attacks if this comes from user-provided input. If you have to use dangerouslySetInnerHTML, consider using a sanitization library such as DOMPurify to sanitize your HTML.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| keywords.forEach(keyword => { | ||
| if (keyword && keyword.trim()) { | ||
| const regex = new RegExp(`(${keyword.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')})`, 'gi') | ||
| highlightedText = highlightedText.replace(regex, `<span style="color: ${highlightColor}; font-weight: bold;">$1</span>`) |
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.
🚨 issue (security): Directly injecting HTML for highlighting may introduce XSS risks.
If log content includes user data, sanitize input or use a safer highlighting method to prevent XSS.
| ) | ||
| } | ||
|
|
||
| return isAllowed ? <WrappedComponent {...props} /> : <FallbackPage errorInfo={t('routeError')} /> |
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.
🚨 issue (security): Authorization check is bypassed, which may expose protected routes.
Disabling the isAllowed check removes route protection and may allow unauthorized access. Please confirm this change is intentional and restore the check before production if needed.
| __html: title === 'content' && fullTextKeywords.length > 0 | ||
| ? highlightText(value[index], fullTextKeywords, currentTheme) | ||
| : value[index] |
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.
security (typescript.react.security.audit.react-dangerouslysetinnerhtml): Detection of dangerouslySetInnerHTML from non-constant definition. This can inadvertently expose users to cross-site scripting (XSS) attacks if this comes from user-provided input. If you have to use dangerouslySetInnerHTML, consider using a sanitization library such as DOMPurify to sanitize your HTML.
Source: opengrep
| __html: title === 'content' && fullTextKeywords.length > 0 | ||
| ? highlightText(value.join('\n'), fullTextKeywords, currentTheme) | ||
| : value.join('\n') |
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.
security (typescript.react.security.audit.react-dangerouslysetinnerhtml): Detection of dangerouslySetInnerHTML from non-constant definition. This can inadvertently expose users to cross-site scripting (XSS) attacks if this comes from user-provided input. If you have to use dangerouslySetInnerHTML, consider using a sanitization library such as DOMPurify to sanitize your HTML.
Source: opengrep
| })() : {}} | ||
| > | ||
| {title === 'content' && fullTextKeywords.length > 0 ? ( | ||
| <div dangerouslySetInnerHTML={{ __html: highlightText(value[0], fullTextKeywords, currentTheme) }} /> |
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.
security (typescript.react.security.audit.react-dangerouslysetinnerhtml): Detection of dangerouslySetInnerHTML from non-constant definition. This can inadvertently expose users to cross-site scripting (XSS) attacks if this comes from user-provided input. If you have to use dangerouslySetInnerHTML, consider using a sanitization library such as DOMPurify to sanitize your HTML.
Source: opengrep
Summary by Sourcery
Enable full-text search keyword extraction, match counting, and contextual highlighting in log components; update query editor theming and development proxy; simplify auth routing and add scheduler dependency.
New Features:
Enhancements:
Build:
/apiproxy in Vite development server configurationscheduleras a project dependency