Conversation
Summary of ChangesHello @AZborovskyyEpam, 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 a critical security concern by mitigating Regular Expression Denial of Service (ReDoS) attacks. It introduces a standardized approach to handling regular expressions by validating patterns for safety and escaping user-provided input, thereby preventing malicious or overly complex regexes from consuming excessive server resources and causing application slowdowns or crashes. The changes ensure that all regex operations involving dynamic input are secure and performant. Highlights
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
|
![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
The pull request effectively addresses potential Regular Expression Denial of Service (ReDoS) vulnerabilities by introducing a regex_utils.ts file with functions to escape special characters and validate regex patterns using the safe-regex library. The changes are applied consistently across search_input.tsx, place_utils.ts, search_utils.tsx, tile_utils.tsx, and query.ts to sanitize user input before creating regular expressions or using it in API calls. The package.json file is updated to include the safe-regex dependency. This is a critical security improvement. Comments regarding redundant safety checks for hardcoded regex patterns have been noted.
| const safeQuery = isSafeRegexPattern(escapeRegexSpecialChars(query)) | ||
| ? query | ||
| : ""; |
There was a problem hiding this comment.
The safeQuery variable is introduced to sanitize the query parameter from the request. This is a critical security improvement to prevent ReDoS attacks by ensuring that only safe regex patterns are processed. If the rawQuery is deemed unsafe, it defaults to an empty string, which is a reasonable defensive measure.
| const rawQuery = (req.query.q as string) || ""; | ||
| const query = isSafeRegexPattern(escapeRegexSpecialChars(rawQuery)) | ||
| ? rawQuery | ||
| : ""; |
There was a problem hiding this comment.
The rawQuery from the request is now sanitized using escapeRegexSpecialChars and validated with isSafeRegexPattern before being assigned to query. This is a critical security measure to prevent ReDoS attacks from user-supplied input. If the rawQuery is unsafe, it defaults to an empty string, which is a robust way to handle malicious input.
| // TODO: only generate related things when we need to generate related question | ||
| let nlResp = null; | ||
| let url = `${apiRoot}/api/explore/detect-and-fulfill?q=${query}&detector=${ | ||
| let url = `${apiRoot}/api/explore/detect-and-fulfill?q=${safeQuery}&detector=${ |
There was a problem hiding this comment.
The url construction now uses safeQuery instead of the original query. This ensures that the query parameter passed to the /api/explore/detect-and-fulfill endpoint is sanitized, mitigating potential ReDoS vulnerabilities in downstream services if they process this query as a regex.
| .filter((result) => result !== null); | ||
| processedResults.forEach((result) => { | ||
| result.dcUrl = DC_URL_ROOT + encodeURIComponent(query as string); | ||
| result.dcUrl = DC_URL_ROOT + encodeURIComponent(safeQuery as string); |
There was a problem hiding this comment.
The dcUrl now uses safeQuery when encoding the URL. This is important for consistency and to ensure that any potentially malicious or malformed regex patterns in the original query do not propagate to the generated Data Commons URL.
| export function isSafeRegexPattern(pattern: string | RegExp): boolean { | ||
| try { | ||
| return Boolean(safeRegex(pattern)); | ||
| } catch { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The isSafeRegexPattern function uses a try-catch block around safeRegex(pattern). While this handles potential errors from safe-regex, it might mask issues if safeRegex throws an unexpected error. It's good practice to log the error in the catch block to aid debugging.
| const matches = inputText | ||
| .split(" ") | ||
| .filter((match) => | ||
| isSafeRegexPattern(`(${escapeRegexSpecialChars(match.toLowerCase())})`) | ||
| ); |
There was a problem hiding this comment.
The filter operation here ensures that only safe regex patterns are used. This is a good step to prevent ReDoS attacks. However, if isSafeRegexPattern returns false, the match is filtered out, which might lead to unexpected behavior for legitimate user inputs that are deemed unsafe. Consider logging or handling such cases to provide a better user experience or to understand why certain inputs are being dropped.
| /** | ||
| * Copyright 2026 Google LLC | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ |
There was a problem hiding this comment.
The copyright year is set to 2026. This seems like a typo and should be updated to the current year.
| "@types/react-table": "^7.7.12", | ||
| "@types/reactstrap": "^8.7.1", | ||
| "@types/resize-observer-browser": "^0.1.6", | ||
| "@types/safe-regex": "^1.1.6", |
There was a problem hiding this comment.
Adding @types/safe-regex as a dev dependency is good for type safety when using the safe-regex library.
| "rehype-raw": "^7.0.0", | ||
| "remark-gfm": "^3.0.1", | ||
| "resize-observer-polyfill": "^1.5.1", | ||
| "safe-regex": "^2.1.1", |
There was a problem hiding this comment.
Adding safe-regex as a dependency is crucial for implementing the ReDoS protection. This library helps in identifying potentially vulnerable regular expressions.
| const safeQuery = isSafeRegexPattern(escapeRegexSpecialChars(query)) | ||
| ? query | ||
| : ""; |
There was a problem hiding this comment.
The safeQuery variable is correctly introduced to sanitize the query before it's used in the request object. This prevents potential injection vulnerabilities. However, if the query is deemed unsafe, it's replaced with an empty string. This might lead to an empty search result without clear indication to the user. Consider adding a warning or a more robust fallback mechanism.
1 participant
Fix for complex regular expressions with potential for catastrophic backtracking when processing user input.