Read regex mobile regex through html dataset #9413
Conversation
WalkthroughTwo distinct changes: a UI text label update in the OIDC form component and a refactoring of mobile number validation logic to use dynamic regex pattern construction from a hidden DOM element instead of inline pattern matching. Changes
Sequence DiagramsequenceDiagram
participant User
participant Form as Mobile Validation
participant DOM as Hidden Element
rect rgb(200, 220, 255)
Note over Form,DOM: Old Flow
User->>Form: Enter mobile number
Form->>Form: Apply inline encoded regex
Form->>User: Validate result
end
rect rgb(220, 255, 200)
Note over Form,DOM: New Flow
User->>Form: Enter mobile number
Form->>DOM: Read data-regex attribute
DOM-->>Form: Return encoded pattern
Form->>Form: Construct RegExp from pattern
Form->>Form: test() against mobile number
Form->>User: Validate result
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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: 2
🧹 Nitpick comments (1)
identity-apps-core/apps/authentication-portal/src/main/webapp/mobile.jsp (1)
190-191: Render the hidden regex only when policy is enabledAlways rendering
data-regexrisks"null"/empty values and downstream surprises. Wrap the element so it only appears when a pattern is provided.- <div id="regexData" data-regex="<%=Encode.forHtmlAttribute(mobileRegex)%>" style="display:none;"></div> + <% if (validateMobileNumberFormat && StringUtils.isNotBlank(mobileRegex)) { %> + <div id="regexData" + data-regex="<%= Encode.forHtmlAttribute(mobileRegex) %>" + style="display:none;"></div> + <% } %>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
features/admin.applications.v1/components/forms/inbound-oidc-form.tsx(1 hunks)identity-apps-core/apps/authentication-portal/src/main/webapp/mobile.jsp(1 hunks)
| </Grid.Column> | ||
| <Grid.Column mobile={ 16 } tablet={ 16 } computer={ 16 }> | ||
| <Heading as="h4">Logout URLs</Heading> | ||
| <Heading as="h4">Logout URL</Heading> |
There was a problem hiding this comment.
🧩 Analysis chain
Avoid hardcoded text; use i18n key for the section heading
This file consistently uses t(...) for labels. Hardcoding "Logout URL" breaks i18n/l10n. Prefer the existing translations (e.g., a logoutURLs.heading key) or add a new singular key if needed.
- <Heading as="h4">Logout URL</Heading>
+ <Heading as="h4">
+ { t("applications:forms.inboundOIDC.sections.logoutURLs.heading") }
+ </Heading>Run to confirm an existing heading key (adjust paths as needed):
🏁 Script executed:
#!/bin/bash
rg -nP --type=json -C2 'logoutURLs.*heading' i18n|head -n 50Length of output: 44
---
🏁 Script executed:
```shell
#!/bin/bash
# Find i18n or translation-related directories/files
fd -t f -e json -e ts -e tsx | grep -iE '(i18n|translation|locale|lang)' | head -20
Length of output: 1119
🏁 Script executed:
#!/bin/bash
# Check the structure near the file under review to understand the project layout
fd -t d -path '*/admin.applications*' | head -10Length of output: 168
🏁 Script executed:
#!/bin/bash
# Search for "Logout URL" or similar logout-related i18n keys anywhere in the repo
rg -n --type=json 'logout' | head -30Length of output: 1063
🏁 Script executed:
#!/bin/bash
# Check the actual file to see how other headings use i18n
rg -nP --type=tsx 'Heading as="h4"' features/admin.applications.v1/components/forms/inbound-oidc-form.tsx -C2 | head -40Length of output: 27
---
🏁 Script executed:
```shell
#!/bin/bash
# Find translation/locale JSON files - likely in public folder or modules
fd -t f -e json | grep -iE '(locale|translation|messages|en|lang)' | grep -v node_modules | grep -v deployment.config | head -30
Length of output: 2887
🏁 Script executed:
#!/bin/bash
# Check the actual file around line 3912
sed -n '3910,3915p' features/admin.applications.v1/components/forms/inbound-oidc-form.tsxLength of output: 329
🏁 Script executed:
#!/bin/bash
# Search for how other Heading h4 elements use i18n in this file
rg -n --type=ts 'Heading as="h4"' features/admin.applications.v1/components/forms/inbound-oidc-form.tsx -B1 -A1 | head -50Length of output: 2772
🏁 Script executed:
#!/bin/bash
# Look for t(...) usage patterns in the file to understand i18n key structure
rg -n 't\(' features/admin.applications.v1/components/forms/inbound-oidc-form.tsx | head -20Length of output: 1443
---
🏁 Script executed:
```shell
#!/bin/bash
# Find where translation keys are defined - check public folder
find . -type f -name "*.json" -path "*/public/*" | grep -iE '(en|locale|translation|messages)' | grep -v node_modules | head -20
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Search for existing logout-related i18n keys in the codebase
rg -n 'logoutURL' --type=ts --type=tsx features/admin.applications.v1/ -B1 -A1 | head -30Length of output: 27
🏁 Script executed:
#!/bin/bash
# Look for similar section keys patterns to understand structure
rg -n 'sections\.' features/admin.applications.v1/components/forms/inbound-oidc-form.tsx | grep -i 'heading' | head -10Length of output: 110
Use i18n translation key for "Logout URL" heading
Line 3912 is the only h4 heading in this file that is hardcoded; all others consistently use t(...) for translations following the pattern applications:forms.inboundOIDC.sections.*. This breaks i18n/l10n consistency.
- <Heading as="h4">Logout URL</Heading>
+ <Heading as="h4">
+ { t("applications:forms.inboundOIDC.sections.logoutURLs.heading") }
+ </Heading>📝 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.
| <Heading as="h4">Logout URL</Heading> | |
| <Heading as="h4"> | |
| { t("applications:forms.inboundOIDC.sections.logoutURLs.heading") } | |
| </Heading> |
🤖 Prompt for AI Agents
In features/admin.applications.v1/components/forms/inbound-oidc-form.tsx around
line 3912, the h4 heading "Logout URL" is hardcoded while other headings use the
i18n helper; replace the hardcoded string by using the translation key following
the existing pattern, e.g. call
t('applications:forms.inboundOIDC.sections.logoutUrl') (or the exact key name
matching your locale files) so the heading is internationalized and consistent
with the rest of the file.
| var regexPattern = document.getElementById('regexData').dataset.regex; | ||
| // decode HTML-encoded backslashes for use in RegExp constructor | ||
| regexPattern = regexPattern.replace(/\\\\/g, "\\"); | ||
| var regexObj = new RegExp(regexPattern); | ||
| if (mobileNumber == "") { |
There was a problem hiding this comment.
Harden regex construction and validation; avoid runtime breaks and over-unescaping
- Construct
RegExponly if validation is enabled and a pattern exists. - Wrap in try/catch to avoid a hard failure on invalid patterns.
- Only unescape double backslashes if present; unconditional replacement can change semantics for patterns that need literal backslashes.
- Use the compiled regex only when available.
- var regexPattern = document.getElementById('regexData').dataset.regex;
- // decode HTML-encoded backslashes for use in RegExp constructor
- regexPattern = regexPattern.replace(/\\\\/g, "\\");
- var regexObj = new RegExp(regexPattern);
+ var regexObj = null;
+ var regexHolder = document.getElementById('regexData');
+ var regexPattern = regexHolder ? regexHolder.dataset.regex : "";
+ // Build regex only when policy is enabled and a non-empty pattern exists.
+ if (<%=validateMobileNumberFormat%> && regexPattern) {
+ try {
+ // Unescape only if we actually see double backslashes.
+ var pattern = regexPattern.indexOf("\\\\") >= 0
+ ? regexPattern.replace(/\\\\/g, "\\")
+ : regexPattern;
+ regexObj = new RegExp(pattern);
+ } catch (e) {
+ console.error("Invalid mobile regex pattern supplied:", e);
+ regexObj = null; // Fail open on client; server should still validate.
+ }
+ }
@@
- } else if (<%=validateMobileNumberFormat%> && !(regexObj.test(mobileNumber))) {
+ } else if (<%=validateMobileNumberFormat%> && regexObj && !(regexObj.test(mobileNumber.trim()))) {Additional minor tweak (outside the selected lines): consider var mobileNumber = document.getElementById("MOBILE_NUMBER").value.trim(); to avoid whitespace false negatives.
Also applies to: 204-205
🤖 Prompt for AI Agents
In identity-apps-core/apps/authentication-portal/src/main/webapp/mobile.jsp
around lines 196-200 (and similarly 204-205), only build a RegExp when
validation is enabled and a pattern exists: check the flag and presence of
dataset.regex before attempting construction; if present, conditionally unescape
double backslashes (perform replace only when /\\\\/ is found) rather than
unconditionally altering the pattern, then wrap new RegExp(...) in try/catch and
proceed to use the compiled regex only if construction succeeds; additionally,
trim the mobile number by replacing the mobileNumber assignment with a .trim()
call to avoid whitespace false negatives.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9413 +/- ##
==========================================
+ Coverage 48.75% 55.88% +7.12%
==========================================
Files 42 42
Lines 1003 1020 +17
Branches 250 254 +4
==========================================
+ Hits 489 570 +81
+ Misses 514 416 -98
- Partials 0 34 +34
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
1 participant
Purpose
Currently the mobile regex is being read via forJavaScript encoding. But this could cause issues since it escapes certain characters. eg: backslash
This PR improves this by reading the value to the dataset of a html component and then reading it via javascript. Then the double escaped values are fixed manually via javascript.
Related Issues
Checklist
Security checks
Developer Checklist (Mandatory)
product-isissue to track any behavioral change or migration impact.Summary by CodeRabbit
Style
Refactor