OCPBUGS-85473: fix: escape regex variable values in regex matcher contexts#1010
OCPBUGS-85473: fix: escape regex variable values in regex matcher contexts#1010jgbernalp wants to merge 1 commit into
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@jgbernalp: This pull request references Jira Issue OCPBUGS-85473, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/invalid-bug
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@jgbernalp: This pull request references Jira Issue OCPBUGS-85473, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
removed
the
jira/invalid-bug
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jgbernalp The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
WalkthroughExtracts ChangesVariable Utils Extraction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Walkthrough
ChangesVariable Utils Extraction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/src/components/dashboards/legacy/variable-utils.ts (2)
44-47: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReturn type should be
string | undefined.The function returns
undefined(lines 46, 72) but is annotated: string, so callers can't see the nullable contract from the type.♻️ Proposed change
timespan: number, namespace: string, -): string => { +): string | undefined => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/dashboards/legacy/variable-utils.ts` around lines 44 - 47, The function in variable-utils.ts has a return type annotation of : string but it actually returns undefined in multiple places (line 46 shown in the diff, and line 72 mentioned in the comment). Update the function signature's return type annotation from : string to : string | undefined to correctly reflect that the function can return either a string or undefined, ensuring callers can see the nullable contract from the type signature.
68-69: 🩺 Stability & Availability | 🔵 TrivialEscape regex metacharacters in variable names for robustness.
The key
kinnew RegExp(\\$${k}`, 'g')should be escaped using_.escapeRegExp(k)to handle edge cases where dashboard variable names contain regex metacharacters like.,*,+, etc. While hardcoded interval variables and built-in variables are safe, user-defined variable names fromvariablesparameter are derived from dashboard templating configuration and could theoretically include special characters. Without escaping, a variable namedtest.varwould incorrectly match$test+ any character +var` instead of the literal pattern.const re = new RegExp(`\\$${_.escapeRegExp(k)}`, 'g');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/dashboards/legacy/variable-utils.ts` around lines 68 - 69, The regular expression pattern in the _.each loop is not escaping the variable name key, which could cause issues if dashboard variable names contain regex metacharacters like dots or asterisks. In the line where new RegExp is constructed with the template string containing the key k, wrap k with _.escapeRegExp() to properly escape any regex special characters before inserting it into the pattern. This ensures that variable names like "test.var" are treated as literal strings rather than regex patterns.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/components/dashboards/legacy/variable-utils.ts`:
- Around line 77-79: The condition at line 77 uses `v.name === 'namespace'` but
the Variable type does not have a name property; instead, `k` represents the
variable key in the _.each loop starting at line 68. Replace the check `v.name
=== 'namespace'` with `k === 'namespace'` to correctly identify when processing
the namespace variable. Additionally, the function return type is declared as
`string` on line 38 but the function can implicitly return `undefined` (at lines
46 and 72), so update the return type annotation to `string | undefined` to
match the actual return behavior.
---
Nitpick comments:
In `@web/src/components/dashboards/legacy/variable-utils.ts`:
- Around line 44-47: The function in variable-utils.ts has a return type
annotation of : string but it actually returns undefined in multiple places
(line 46 shown in the diff, and line 72 mentioned in the comment). Update the
function signature's return type annotation from : string to : string |
undefined to correctly reflect that the function can return either a string or
undefined, ensuring callers can see the nullable contract from the type
signature.
- Around line 68-69: The regular expression pattern in the _.each loop is not
escaping the variable name key, which could cause issues if dashboard variable
names contain regex metacharacters like dots or asterisks. In the line where new
RegExp is constructed with the template string containing the key k, wrap k with
_.escapeRegExp() to properly escape any regex special characters before
inserting it into the pattern. This ensures that variable names like "test.var"
are treated as literal strings rather than regex patterns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 006e25af-dfa0-4b45-a46d-abd4b071aa91
📒 Files selected for processing (3)
web/src/components/dashboards/legacy/legacy-variable-dropdowns.tsxweb/src/components/dashboards/legacy/variable-utils.spec.tsweb/src/components/dashboards/legacy/variable-utils.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/src/components/dashboards/legacy/variable-utils.spec.ts (1)
23-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for overlapping variable names (
$jobvs$job_name).Current suite is thorough, but this edge case is still untested and directly protects substitution correctness.
🧪 Proposed test addition
describe('evaluateVariableTemplate', () => { + it('does not partially replace overlapping variable names', () => { + const vars = makeVariables({ + job: { value: 'short' }, + job_name: { value: 'long' }, + }); + expect( + evaluateVariableTemplate('metric{a="$job",b="$job_name"}', vars, timespan, ''), + ).toBe('metric{a="short",b="long"}'); + }); + it('substitutes multiple variables', () => {Also applies to: 77-82
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/dashboards/legacy/variable-utils.spec.ts` around lines 23 - 31, Add a new regression test after the existing substitutes multiple variables test that specifically tests overlapping variable names to ensure correct substitution behavior. The new test should call makeVariables with overlapping variable names like job and job_name (or similar names where one is a prefix of another), then invoke evaluateVariableTemplate with a template string that uses both variables (for example, a metric with both $job and $job_name placeholders), and verify the output correctly substitutes each variable to its own value rather than incorrectly replacing the shorter variable name within the longer one.web/src/components/dashboards/legacy/variable-utils.ts (1)
18-20: 📐 Maintainability & Code Quality | 🔵 TrivialReplace
datasource?: anywithdatasource?: DataSource.The
Variabletype should use theDataSourcetype already defined intypes.tsinstead ofany. This aligns with how similar fields are typed inPanelandTemplateVariablein the same file. All existing usages access the.nameproperty, which is already part of theDataSourcestructure, so this change improves type safety without requiring code modifications elsewhere.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/dashboards/legacy/variable-utils.ts` around lines 18 - 20, In the Variable type definition, replace the datasource field type from any to DataSource to improve type safety and maintain consistency with similar fields in Panel and TemplateVariable types. Ensure that the DataSource type is imported from types.ts if it is not already imported, since all existing usages access the .name property which is part of the DataSource structure and the change requires no modifications to existing code.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/components/dashboards/legacy/variable-utils.ts`:
- Around line 67-69: The RegExp pattern constructed in the loop iterating
through allVariables is too permissive and matches variable name prefixes rather
than complete variable names. For example, when k is "job", the pattern matches
"$job" inside "$job_name", causing unintended substitutions. Modify the RegExp
pattern in the new RegExp call to use word boundaries (or another appropriate
delimiter pattern) to ensure only complete variable names are matched,
preventing overlap corruption when variable names share common prefixes.
---
Nitpick comments:
In `@web/src/components/dashboards/legacy/variable-utils.spec.ts`:
- Around line 23-31: Add a new regression test after the existing substitutes
multiple variables test that specifically tests overlapping variable names to
ensure correct substitution behavior. The new test should call makeVariables
with overlapping variable names like job and job_name (or similar names where
one is a prefix of another), then invoke evaluateVariableTemplate with a
template string that uses both variables (for example, a metric with both $job
and $job_name placeholders), and verify the output correctly substitutes each
variable to its own value rather than incorrectly replacing the shorter variable
name within the longer one.
In `@web/src/components/dashboards/legacy/variable-utils.ts`:
- Around line 18-20: In the Variable type definition, replace the datasource
field type from any to DataSource to improve type safety and maintain
consistency with similar fields in Panel and TemplateVariable types. Ensure that
the DataSource type is imported from types.ts if it is not already imported,
since all existing usages access the .name property which is part of the
DataSource structure and the change requires no modifications to existing code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a1e441a2-ac59-4d02-9e8b-7713d07862e9
📒 Files selected for processing (3)
web/src/components/dashboards/legacy/legacy-variable-dropdowns.tsxweb/src/components/dashboards/legacy/variable-utils.spec.tsweb/src/components/dashboards/legacy/variable-utils.ts
jgbernalp
force-pushed
the
escape-variables-for-regex-context
branch
from
0ab55ec to
704dc88
Compare
openshift-ci
Bot
added
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web/src/components/dashboards/legacy/variable-utils.ts (1)
67-69: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPrevent partial placeholder matches and escape variable keys during regex substitution.
The regex pattern at line 68 matches variable name prefixes rather than complete names. For example, when
kis"job", the pattern matches$jobinside$job_name, causing deterministic query corruption when variable names share prefixes. Additionally, the variable key is interpolated without escaping, which could allow regex special characters in variable names to alter pattern behavior.🔧 Proposed fix
_.each(allVariables, (v, k) => { - const re = new RegExp(`\\$${k}`, 'g'); + const escapedKey = _.escapeRegExp(k); + const re = new RegExp(`\\$${escapedKey}(?![A-Za-z0-9_])`, 'g'); if (result.match(re)) {The negative lookahead
(?![A-Za-z0-9_])ensures only complete variable names are matched, and_.escapeRegExp(k)prevents regex special characters in variable names from altering the pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/dashboards/legacy/variable-utils.ts` around lines 67 - 69, In the _.each loop that iterates through allVariables, modify the RegExp pattern creation to add a negative lookahead assertion `(?![A-Za-z0-9_])` after the variable key placeholder to ensure only complete variable names are matched (not prefixes). Additionally, wrap the variable key `k` with `_.escapeRegExp(k)` before interpolating it into the pattern string to prevent regex special characters in variable names from altering the pattern behavior.
🧹 Nitpick comments (1)
web/src/components/targets-page.tsx (1)
215-227: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider explicit handling for undefined health state.
The component now explicitly accepts
health?: 'up' | 'down', making it clear thathealthcan beundefined. However, whenhealthisundefined, the component renders the "Down" state (RedExclamationCircleIcon + "Down" text), which may be semantically misleading—a target with no health data is different from a target that is confirmed down.While this PR focuses on typing rather than functional changes, consider adding an explicit case for
undefinedto improve clarity:-return health === 'up' ? ( +return health === 'up' ? ( <> <GreenCheckCircleIcon /> {t('Up')} </> +) : health === 'down' ? ( + <> + <RedExclamationCircleIcon /> {t('Down')} + </> ) : ( <> - <RedExclamationCircleIcon /> {t('Down')} + <span>{t('Unknown')}</span> </> );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/targets-page.tsx` around lines 215 - 227, The Health component currently treats undefined health state the same as the 'down' state by rendering RedExclamationCircleIcon, which is semantically misleading since no health data is different from a confirmed down status. Add an explicit conditional case in the Health component to handle when health is undefined, rendering an appropriate indicator for the unknown or no-data state instead of falling through to the down case, while keeping the existing 'up' and 'down' cases intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/cypress/component/mocks/perses-dashboards.tsx`:
- Line 7: The mock in the panelOptions conditional is calling the extra function
with an unnecessary empty object argument, but the actual function signature in
ShowTimeseries.tsx defines extra as a zero-argument function. Update the call to
panelOptions.extra() on line 7 to remove the empty object argument {} so it
matches the actual function signature and improves consistency with the real
implementation.
---
Duplicate comments:
In `@web/src/components/dashboards/legacy/variable-utils.ts`:
- Around line 67-69: In the _.each loop that iterates through allVariables,
modify the RegExp pattern creation to add a negative lookahead assertion
`(?![A-Za-z0-9_])` after the variable key placeholder to ensure only complete
variable names are matched (not prefixes). Additionally, wrap the variable key
`k` with `_.escapeRegExp(k)` before interpolating it into the pattern string to
prevent regex special characters in variable names from altering the pattern
behavior.
---
Nitpick comments:
In `@web/src/components/targets-page.tsx`:
- Around line 215-227: The Health component currently treats undefined health
state the same as the 'down' state by rendering RedExclamationCircleIcon, which
is semantically misleading since no health data is different from a confirmed
down status. Add an explicit conditional case in the Health component to handle
when health is undefined, rendering an appropriate indicator for the unknown or
no-data state instead of falling through to the down case, while keeping the
existing 'up' and 'down' cases intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8c1d8792-378d-4b33-af73-0d505ef33588
📒 Files selected for processing (27)
web/cypress/component/mocks/AddToDashboardButton.tsxweb/cypress/component/mocks/OlsToolUIPersesWrapper.tsxweb/cypress/component/mocks/perses-dashboards.tsxweb/cypress/dummy.tsxweb/cypress/support/incidents_prometheus_query_mocks/mock-generators.tsweb/cypress/support/incidents_prometheus_query_mocks/prometheus-mocks.tsweb/cypress/support/incidents_prometheus_query_mocks/schema/validate-fixtures.tsweb/cypress/support/index.tsweb/eslint.config.tsweb/src/components/alerting/AlertList/AggregateAlertTableRow.tsxweb/src/components/alerting/AlertRulesDetailsPage.tsxweb/src/components/alerting/AlertUtils.tsxweb/src/components/alerting/SilencesUtils.tsxweb/src/components/console/console-shared/src/components/empty-state/EmptyBox.tsxweb/src/components/console/console-shared/src/components/loading/Loading.tsxweb/src/components/dashboards/legacy/dashboard-skeleton-legacy.tsxweb/src/components/dashboards/legacy/legacy-dashboard.tsxweb/src/components/dashboards/legacy/legacy-variable-dropdowns.tsxweb/src/components/dashboards/legacy/variable-utils.spec.tsweb/src/components/dashboards/legacy/variable-utils.tsweb/src/components/dashboards/perses/dashboard-header.tsxweb/src/components/dashboards/shared/dashboard-dropdown.tsxweb/src/components/labels.tsxweb/src/components/ols-tool-ui/helpers/AddToDashboardButton.tsxweb/src/components/ols-tool-ui/helpers/OlsToolUIPersesWrapper.tsxweb/src/components/query-browser.tsxweb/src/components/targets-page.tsx
💤 Files with no reviewable changes (4)
- web/cypress/support/index.ts
- web/cypress/dummy.tsx
- web/cypress/support/incidents_prometheus_query_mocks/mock-generators.ts
- web/cypress/support/incidents_prometheus_query_mocks/prometheus-mocks.ts
✅ Files skipped from review due to trivial changes (7)
- web/cypress/component/mocks/AddToDashboardButton.tsx
- web/src/components/alerting/AlertRulesDetailsPage.tsx
- web/src/components/console/console-shared/src/components/loading/Loading.tsx
- web/src/components/alerting/SilencesUtils.tsx
- web/src/components/dashboards/legacy/dashboard-skeleton-legacy.tsx
- web/src/components/query-browser.tsx
- web/cypress/support/incidents_prometheus_query_mocks/schema/validate-fixtures.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/components/dashboards/legacy/legacy-variable-dropdowns.tsx
- web/src/components/dashboards/legacy/variable-utils.spec.ts
Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
jgbernalp
force-pushed
the
escape-variables-for-regex-context
branch
from
704dc88 to
03bb196
Compare
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/src/components/dashboards/legacy/variable-utils.ts (1)
18-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
anyondatasourcewith a safer type.Using
anyhere drops type-safety in downstream usage. Preferunknown(with narrowing at use sites) or a concrete union/interface.Suggested minimal change
export type Variable = { isHidden?: boolean; isLoading?: boolean; includeAll?: boolean; options?: string[]; query?: string; value?: string; - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - datasource?: any; + datasource?: unknown; };As per coding guidelines, “Avoid using 'any' type; use proper type definitions instead.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/dashboards/legacy/variable-utils.ts` around lines 18 - 19, The datasource property in the interface/type is currently typed as any, which eliminates type-safety in downstream code. Replace the any type annotation for the datasource field with either unknown (and add appropriate type narrowing at usage sites) or a concrete type definition such as a union of specific data source types or a defined interface. Remove or adjust the eslint-disable comment accordingly once the type is properly defined, as it will no longer be necessary.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@web/src/components/dashboards/legacy/variable-utils.ts`:
- Around line 18-19: The datasource property in the interface/type is currently
typed as any, which eliminates type-safety in downstream code. Replace the any
type annotation for the datasource field with either unknown (and add
appropriate type narrowing at usage sites) or a concrete type definition such as
a union of specific data source types or a defined interface. Remove or adjust
the eslint-disable comment accordingly once the type is properly defined, as it
will no longer be necessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9f30df09-60bf-401d-b430-c25114ef02c3
📒 Files selected for processing (3)
web/src/components/dashboards/legacy/legacy-variable-dropdowns.tsxweb/src/components/dashboards/legacy/variable-utils.spec.tsweb/src/components/dashboards/legacy/variable-utils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/components/dashboards/legacy/variable-utils.spec.ts
- web/src/components/dashboards/legacy/legacy-variable-dropdowns.tsx
|
@jgbernalp: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
This PR moves the
evaluateVariableTemplateinto its own utility file and adds tests for the regex escape bug.Summary by CodeRabbit
Refactor
Tests