fix: trim before and after space in query editor and vrl function #4886
Conversation
WalkthroughThe changes in this pull request focus on improving the handling of whitespace in SQL query strings and VRL function inputs across various components. Key modifications include trimming whitespace in the Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
web/src/views/RUM/AppSessions.vue (1)
429-429: Add input validation for the trimmed queryThe trimmed query string should be validated before being used in the SQL statement.
Consider adding validation:
+ const trimmedQuery = sessionState.data.editorValue.trimStart().trimEnd(); + if (!/^[\w\s=<>!()]+$/.test(trimmedQuery)) { + throw new Error('Invalid query format'); + } - ? " where " + sessionState.data.editorValue.trimStart().trimEnd() + ? " where " + trimmedQueryweb/src/plugins/traces/Index.vue (2)
Line range hint
519-607: Improve error handling in buildSearch methodThe current error handling could be enhanced for better debugging and user experience:
- Avoid logging raw error objects to console in production
- Provide more specific error messages to users
- Ensure loading state is properly reset in all code paths
try { let query = searchObj.data.editorValue.trim(); // ... existing code ... return req; } catch (e) { - console.log(e); + const errorMessage = e instanceof Error ? e.message : 'Unknown error'; + console.error('Failed to build search query:', errorMessage); searchObj.loading = false; showErrorNotification( - "An error occurred while constructing the search query." + `Failed to build search query: ${errorMessage}` ); + return null; }
Line range hint
1-1183: Consider refactoring for better maintainabilityThe component is quite large and handles multiple responsibilities. Consider the following architectural improvements:
- Extract the trace search logic into a separate composable
- Break down the component into smaller, focused components
- Implement consistent error handling patterns across all methods
- Move data transformation logic (like
getTracesMetaData) to a separate utility fileThis will improve maintainability, testability, and reusability of the code.
web/src/components/alerts/AddAlert.vue (2)
820-822: Consider using trim() for better readability.The current implementation uses both trimStart() and trimEnd(). For better readability and maintainability, consider using the equivalent trim() method.
- previewQuery.value = formData.value.query_condition.sql.trimStart().trimEnd(); + previewQuery.value = formData.value.query_condition.sql.trim(); - previewQuery.value = formData.value.query_condition.promql.trimStart().trimEnd(); + previewQuery.value = formData.value.query_condition.promql.trim();
1036-1036: Consider using trim() for consistency.Similar to the previous suggestion, replace trimStart().trimEnd() with trim() for better readability and consistency across the codebase.
- formData.value.query_condition.vrl_function.trimStart().trimEnd(), + formData.value.query_condition.vrl_function.trim(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- web/src/components/alerts/AddAlert.vue (2 hunks)
- web/src/composables/useLogs.ts (6 hunks)
- web/src/plugins/traces/Index.vue (1 hunks)
- web/src/plugins/traces/IndexList.vue (1 hunks)
- web/src/views/RUM/AppSessions.vue (1 hunks)
🧰 Additional context used
🪛 Biome
web/src/composables/useLogs.ts
[error] 559-559: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
[error] 3715-3715: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
[error] 3718-3718: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 3838-3838: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
🔇 Additional comments (7)
web/src/views/RUM/AppSessions.vue (1)
429-429: Verify other SQL query construction patternsLet's check for other instances of direct SQL string concatenation that might need similar protection.
✅ Verification successful
Let me try a different search strategy to find SQL query construction patterns.
This broader search should help us identify any potential SQL query construction patterns, whether they use explicit SQL keywords or not. We're also looking for files that might contain SQL-related functionality to narrow down our investigation.
Based on the search results, I can now generate a final response as we've found relevant SQL query construction patterns, particularly in the logs plugin components.
SQL query concatenation patterns found in multiple components
The search revealed SQL string concatenation patterns that require similar protection:
- In
web/src/plugins/logs/SearchBar.vue:
- Uses direct string concatenation with SQL keywords:
beforeLimit.trim() + " AND " + filter- Another instance:
query = query + " AND " + filter- In
web/src/plugins/logs/Index.vue:
- Handles SQL mode with query manipulation using regex
These findings suggest that SQL query construction is primarily contained within the logs plugin components, and the protection pattern being applied in
AppSessions.vueis consistent with the existing codebase's approach to handling user input in SQL queries.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for SQL query string concatenation patterns echo "Searching for potential SQL injection vulnerabilities..." rg -g '*.{vue,js,ts}' -B 2 -A 2 'query\.sql.*\+.*where' echo "Searching for other uses of editorValue in SQL queries..." rg -g '*.{vue,js,ts}' 'editorValue.*sql'Length of output: 348
Script:
#!/bin/bash # Search for SQL-like string concatenation patterns more broadly echo "Searching for string concatenation with SQL keywords..." rg -g '*.{vue,js,ts}' -B 2 -A 2 '\+ *(["'']* *(SELECT|WHERE|FROM|JOIN|AND|OR|ORDER BY|GROUP BY))' echo "Searching for query string construction..." rg -g '*.{vue,js,ts}' -B 2 -A 2 'query *\= *.*\+' echo "Looking for editor or SQL-related components..." fd -e vue -e js -e ts -x grep -l -i "sql\|editor\|query" {}Length of output: 12166
web/src/plugins/traces/IndexList.vue (1)
406-406: Consider usingtrim()instead oftrimStart().trimEnd()The whitespace trimming is a good addition to prevent SQL syntax errors. However, for better readability and maintainability, consider using the equivalent
trim()method instead of chainingtrimStart().trimEnd().- query += " WHERE " + searchObj.data.editorValue.trimStart().trimEnd(); + query += " WHERE " + searchObj.data.editorValue.trim();Let's verify the SQL parser's behavior with whitespace:
✅ Verification successful
Based on the search results, I can see that SQL queries are constructed throughout the codebase, and many of them handle whitespace appropriately. Let me verify the specific file and context by searching for more details about the query construction in the IndexList.vue file.
Based on the context, I can now generate the final response since I have enough information about how the query is constructed and processed:
Using
trim()is indeed a better choice hereThe suggestion to use
trim()instead oftrimStart().trimEnd()is valid because:
- The code immediately passes the trimmed value to a SQL parser (
parser.astify)- The trimming is followed by SQL condition handling (
removeCondition)- Using
trim()achieves the same result with cleaner, more maintainable code- query += " WHERE " + searchObj.data.editorValue.trimStart().trimEnd(); + query += " WHERE " + searchObj.data.editorValue.trim();🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are any other instances of SQL query construction that might need whitespace trimming # Test: Search for SQL query string concatenation patterns rg -A 2 "SELECT.*FROM.*WHERE"Length of output: 40722
Script:
#!/bin/bash # Search for the context around the query construction in IndexList.vue rg -A 5 -B 5 "query \+= .*WHERE" web/src/plugins/traces/IndexList.vueLength of output: 515
web/src/plugins/traces/Index.vue (1)
519-519: Verify consistent whitespace handling across componentsLet's ensure that whitespace trimming is consistently implemented across all query-related components.
✅ Verification successful
Let me try different search commands without relying on the file type flag.
Based on the search results, I can now generate a final response as we have comprehensive information about string trimming usage across the codebase.
Whitespace handling is consistent across components
The codebase shows consistent whitespace handling patterns:
- In query-related components (
Index.vue,SearchBar.vue),trimStart()andtrimEnd()are consistently used together for query strings- Form validation rules consistently use
trim()for required field validations- SQL/PromQL query processing consistently applies trimming before execution
The change in
web/src/plugins/traces/Index.vuefollows the established pattern used in other similar components, particularly in query handling.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for query string handling in Vue components rg -l "editorValue" --type vue # Search for trim operations on query strings rg "(trim\(|trimStart\(|trimEnd\()" --type vueLength of output: 135
Script:
#!/bin/bash # Search for files with .vue extension containing editorValue fd -e vue -x rg -l "editorValue" {} # Search for trim operations in .vue files fd -e vue -x rg "(trim\(|trimStart\(|trimEnd\()" {} # Also search for direct string trimming without parentheses fd -e vue -x rg "\.trim[^(]|\.trimStart[^(]|\.trimEnd[^(]" {}Length of output: 7558
web/src/composables/useLogs.ts (4)
529-531: LGTM: Query string trimming improves robustness.The addition of
trimStart()andtrimEnd()ensures consistent query handling by removing leading and trailing whitespace before encoding.
539-539: LGTM: Consistent whitespace handling for function content.The trimming of function content aligns with the query trimming approach, ensuring clean input for execution.
645-647: LGTM: Consistent query trimming in buildSearch.The trimming of the query in buildSearch maintains consistency with the overall whitespace handling approach.
907-907: LGTM: Query functions trimming maintains consistency.The trimming of query functions aligns with the overall whitespace handling strategy.
web/src/views/RUM/AppSessions.vue
Outdated
| }) as zo_sql_timestamp, min(start) as start_time, max(end) as end_time, min(user_agent_user_agent_family) as browser, min(user_agent_os_family) as os, min(ip) as ip, min(source) as source, session_id from "_sessionreplay" ${ | ||
| sessionState.data.editorValue.length | ||
| ? " where " + sessionState.data.editorValue | ||
| ? " where " + sessionState.data.editorValue.trimStart().trimEnd() |
There was a problem hiding this comment.
Security: Protect against SQL injection attacks
The SQL query is constructed using string concatenation with user input (sessionState.data.editorValue). While trimming whitespace is good, the query is still vulnerable to SQL injection attacks.
Consider using parameterized queries or a query builder that handles SQL escaping. Here's a suggested approach:
- ? " where " + sessionState.data.editorValue.trimStart().trimEnd()
+ ? " where " + sanitizeSqlInput(sessionState.data.editorValue.trimStart().trimEnd())Add a SQL sanitization utility:
function sanitizeSqlInput(input: string): string {
// Implement SQL injection prevention logic
// Consider using a well-tested SQL sanitization library
return input.replace(/['";\\]/g, '\\$&');
}
web/src/plugins/traces/Index.vue
Outdated
| function buildSearch() { | ||
| try { | ||
| let query = searchObj.data.editorValue; | ||
| let query = searchObj.data.editorValue.trimStart().trimEnd(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify whitespace trimming using trim()
Instead of using both trimStart() and trimEnd(), you can achieve the same result more concisely using the standard trim() method.
- let query = searchObj.data.editorValue.trimStart().trimEnd();
+ let query = searchObj.data.editorValue.trim();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let query = searchObj.data.editorValue.trimStart().trimEnd(); | |
| let query = searchObj.data.editorValue.trim(); |
There was a problem hiding this comment.
added single function.
There was a problem hiding this comment.
@bjp232004, great! Thank you for applying the suggestion. 🎉
(_/)
(•_•)
/ >🎉
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| if ( | ||
| query.hasOwnProperty("type") && | ||
| query.type == "search_history_re_apply" | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Object.hasOwn() instead of hasOwnProperty.
Replace the hasOwnProperty check with the safer Object.hasOwn() method to prevent potential issues with prototype pollution.
- if (
- query.hasOwnProperty("type") &&
- query.type == "search_history_re_apply"
- ) {
+ if (
+ Object.hasOwn(query, "type") &&
+ query.type == "search_history_re_apply"
+ ) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| query.hasOwnProperty("type") && | |
| query.type == "search_history_re_apply" | |
| ) { | |
| if ( | |
| Object.hasOwn(query, "type") && | |
| query.type == "search_history_re_apply" | |
| ) { |
🧰 Tools
🪛 Biome
[error] 559-559: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
There was a problem hiding this comment.
this was done as a part of another pr
There was a problem hiding this comment.
@bjp232004, understood. Thank you for letting me know.
(_/)
( •_• )
/💧
✏️ Learnings added
Learnt from: bjp232004
PR: openobserve/openobserve#4886
File: web/src/composables/useLogs.ts:0-0
Timestamp: 2024-10-23T10:32:04.924Z
Learning: Changes to replace `hasOwnProperty` with `Object.hasOwn()` in `web/src/composables/useLogs.ts` have been made in another PR.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| if ( | ||
| queryParams.hasOwnProperty("type") && | ||
| queryParams.type == "search_history_re_apply" | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Object.hasOwn() for consistent property checking.
Replace hasOwnProperty with Object.hasOwn() for safer property checking.
- if (
- queryParams.hasOwnProperty("type") &&
- queryParams.type == "search_history_re_apply"
- ) {
+ if (
+ Object.hasOwn(queryParams, "type") &&
+ queryParams.type == "search_history_re_apply"
+ ) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| queryParams.hasOwnProperty("type") && | |
| queryParams.type == "search_history_re_apply" | |
| ) { | |
| if ( | |
| Object.hasOwn(queryParams, "type") && | |
| queryParams.type == "search_history_re_apply" | |
| ) { |
🧰 Tools
🪛 Biome
[error] 3838-3838: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
There was a problem hiding this comment.
@bjp232004, thank you for fixing the issue! 😊🐇
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| if ( | ||
| router.currentRoute.value.query.hasOwnProperty("type") && | ||
| router.currentRoute.value.query.type == "search_history_re_apply" | ||
| ) { | ||
| delete router.currentRoute.value.query.type; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve property handling and performance.
Two improvements are suggested:
- Replace hasOwnProperty with Object.hasOwn()
- Use undefined assignment instead of delete operator
- if (
- router.currentRoute.value.query.hasOwnProperty("type") &&
- router.currentRoute.value.query.type == "search_history_re_apply"
- ) {
- delete router.currentRoute.value.query.type;
- }
+ if (
+ Object.hasOwn(router.currentRoute.value.query, "type") &&
+ router.currentRoute.value.query.type == "search_history_re_apply"
+ ) {
+ router.currentRoute.value.query.type = undefined;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| router.currentRoute.value.query.hasOwnProperty("type") && | |
| router.currentRoute.value.query.type == "search_history_re_apply" | |
| ) { | |
| delete router.currentRoute.value.query.type; | |
| } | |
| if ( | |
| Object.hasOwn(router.currentRoute.value.query, "type") && | |
| router.currentRoute.value.query.type == "search_history_re_apply" | |
| ) { | |
| router.currentRoute.value.query.type = undefined; | |
| } |
🧰 Tools
🪛 Biome
[error] 3715-3715: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
[error] 3718-3718: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
There was a problem hiding this comment.
Do not want to keep the type so deleting this property from the object.
There was a problem hiding this comment.
@bjp232004 Understood, thank you for the clarification. I'll remember to use the delete operator when removing properties in the future.
( ཀ ʖ̯ ཀ)
✏️ Learnings added
Learnt from: bjp232004
PR: openobserve/openobserve#4886
File: web/src/composables/useLogs.ts:0-0
Timestamp: 2024-10-23T10:36:00.634Z
Learning: In this codebase, when removing properties from objects, prefer using the `delete` operator instead of assigning `undefined`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
bjp232004
force-pushed
the
4740-trim-the-query-and-vrl-function
branch
2 times, most recently
from
43f9740 to
f1f18d6
Compare
bjp232004
force-pushed
the
4740-trim-the-query-and-vrl-function
branch
from
f1f18d6 to
3ccd438
Compare
3 participants
Summary by CodeRabbit
Release Notes
New Features
refreshTimezonemethod to update grid columns and regenerate histogram data in the search functionality.Bug Fixes
These updates enhance the overall robustness and user experience within the application, particularly in handling search queries and alerts.