Add minor improvements#14164
Open
O-sura wants to merge 2 commits into
Open
Conversation
O-sura
commented
Apr 20, 2026
O-sura
commented
Contributor
- This PR adds some minor improvements to the validation logics
O-sura
requested review from
AnuGayan,
Arshardh,
chamilaadhi and
tharindu1st
as code owners
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughServer-side validation for the request ChangesCallback validation & rendering across authentication JSPs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@all-in-one-apim/modules/distribution/product/src/main/extensions/password-recovery.jsp`:
- Around line 213-217: The callback value is not being normalized before
rendering the hidden field: ensure empty string and the literal "null" are
treated as null. In the JSP blocks that check callback (the if using
StringUtils.isBlank, StringUtils.equalsIgnoreCase(callback, "null"), and
AuthenticationEndpointUtil.isValidMultiOptionURI), change the logic to first
normalize callback by setting callback = null when StringUtils.isBlank(callback)
OR StringUtils.equalsIgnoreCase(callback, "null") OR
!AuthenticationEndpointUtil.isValidMultiOptionURI(callback); update both
occurrences (the current block around the first check and the similar block at
lines 219-223) so that only a non-blank, non-"null", valid multi-option URI
remains and the hidden field is not rendered with "" or "null".
In
`@all-in-one-apim/modules/distribution/product/src/main/extensions/self-registration-username-request.jsp`:
- Around line 51-63: The code currently validates the callback into the local
variable callback (using AuthenticationEndpointUtil.isValidMultiOptionURI) but
later replays the original request parameter into the signup.do form, defeating
validation; update the form population logic so it does not write the raw
request parameter back — either omit replaying the "callback" parameter entirely
when generating hidden inputs (like reCaptcha is skipped) or always emit the
already-validated callback variable (and treat null/blank as absent). Locate the
form/output generation that iterates request parameters (the block that emits
inputs for signup.do) and change it to skip "callback" or substitute
request.getParameter("callback") with the validated callback variable and ensure
isCallBackUrlEmpty is respected. Ensure no other replay paths emit the
unvalidated callback.
In
`@all-in-one-apim/modules/distribution/product/src/main/extensions/self-registration-with-verification.jsp`:
- Around line 196-201: Normalize the callback string by treating the literal
"null" (case-insensitive) as null first, then validate the raw callback value
with AuthenticationEndpointUtil.isValidMultiOptionURI(callback) before
URL-encoding; only call
IdentityManagementEndpointUtil.getURLEncodedCallback(callback) after the raw
callback passes validation. In practice, update the block that currently uses
StringUtils.isBlank(callback), StringUtils.equalsIgnoreCase(callback, "null"),
IdentityManagementEndpointUtil.getURLEncodedCallback(callback) and
AuthenticationEndpointUtil.isValidMultiOptionURI(encodedCallback) so it first
sets callback = null when equalsIgnoreCase("null"), then calls
isValidMultiOptionURI(callback) on the raw value and only encodes with
getURLEncodedCallback(callback) if validation succeeds.
In
`@api-control-plane/modules/distribution/product/src/main/extensions/password-recovery.jsp`:
- Around line 213-217: The callback variable can be the empty string or the
literal "null" and still pass the subsequent null check, so before rendering the
hidden field ensure such values are normalized to null: update the code that
validates/uses callback (the callback variable, the block using StringUtils and
AuthenticationEndpointUtil.isValidMultiOptionURI) to first do if
(StringUtils.isBlank(callback) || StringUtils.equalsIgnoreCase(callback,
"null")) { callback = null; } and only then call
AuthenticationEndpointUtil.isValidMultiOptionURI(callback) (or null-check
afterwards) so blank/"null" values are not preserved; apply the same
normalization wherever callback is later used (including the block around the
hidden field rendering).
In
`@api-control-plane/modules/distribution/product/src/main/extensions/self-registration-username-request.jsp`:
- Around line 51-63: The sanitized callback handling is being bypassed because
the original request parameter "callback" is re-added later; fix by normalizing
blank or "null" values to null (as done in the existing validation using
callback and isCallBackUrlEmpty), then when rebuilding parameters for
replay/skipping the parameter-map loop and when rendering the hidden field for
"callback" ensure you only output the sanitized callback (the validated callback
variable) and skip any replay of request.getParameter("callback"); modify the
parameter replay loop (the code that iterates request.getParameterMap() and
re-emits fields) to exclude the "callback" key and ensure the hidden input for
signup.do uses the validated callback value and is omitted when callback ==
null.
In
`@api-control-plane/modules/distribution/product/src/main/extensions/self-registration-with-verification.jsp`:
- Around line 196-201: Normalize and validate the raw callback value (not the
URL-encoded one) and treat the literal string "null" as blank: update the
condition around the callback handling to use a normalization like
StringUtils.trimToEmpty(callback) and check
StringUtils.isBlank(normalizedCallback) (or
StringUtils.equalsIgnoreCase(normalizedCallback, "null")) so the block runs only
for a meaningful value; then call
AuthenticationEndpointUtil.isValidMultiOptionURI(normalizedCallback) to validate
the raw callback and only after validation call
IdentityManagementEndpointUtil.getURLEncodedCallback(callback) to produce
encodedCallback; adjust assignments so invalid or blank/"null" callbacks are set
to null consistently.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: afa66cb3-d16c-4af3-9a81-b5ffb1b11b56
📒 Files selected for processing (6)
all-in-one-apim/modules/distribution/product/src/main/extensions/password-recovery.jspall-in-one-apim/modules/distribution/product/src/main/extensions/self-registration-username-request.jspall-in-one-apim/modules/distribution/product/src/main/extensions/self-registration-with-verification.jspapi-control-plane/modules/distribution/product/src/main/extensions/password-recovery.jspapi-control-plane/modules/distribution/product/src/main/extensions/self-registration-username-request.jspapi-control-plane/modules/distribution/product/src/main/extensions/self-registration-with-verification.jsp
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api-control-plane/modules/distribution/product/src/main/extensions/self-registration-with-verification.jsp (1)
177-200:⚠️ Potential issue | 🟡 MinorInconsistent handling between the two callback validation layers.
The callback is validated twice with different failure semantics: lines 178-185 forward to
error.jspwhenUtils.validateCallbackURLfails, whereas lines 196-200 silently nullify onisValidMultiOptionURIfailure (falling back to the user portal URL at line 206). A callback that passes the first check but fails the second is therefore quietly rewritten rather than rejected. Consider either aligning both on the same outcome, or documenting why the second check is intentionally softer. Also, since the first block already guaranteescallbackis either blank or structurally valid, the"null"literal check is the primary new behavior added here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api-control-plane/modules/distribution/product/src/main/extensions/self-registration-with-verification.jsp` around lines 177 - 200, The code inconsistently handles callback validation: after Utils.validateCallbackURL(...) rejects the request by forwarding to "error.jsp", the later AuthenticationEndpointUtil.isValidMultiOptionURI(callback) check silently nulls the callback; change this to the same failure flow as the first check — when callback is non-blank (and not the literal "null") but AuthenticationEndpointUtil.isValidMultiOptionURI(callback) returns false, set request attributes (error=true and errorMsg using IdentityManagementEndpointUtil.i18n(...) with an appropriate message) and forward to "error.jsp" instead of nullifying callback so Utils.validateCallbackURL and isValidMultiOptionURI have consistent semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@api-control-plane/modules/distribution/product/src/main/extensions/self-registration-with-verification.jsp`:
- Around line 177-200: The code inconsistently handles callback validation:
after Utils.validateCallbackURL(...) rejects the request by forwarding to
"error.jsp", the later
AuthenticationEndpointUtil.isValidMultiOptionURI(callback) check silently nulls
the callback; change this to the same failure flow as the first check — when
callback is non-blank (and not the literal "null") but
AuthenticationEndpointUtil.isValidMultiOptionURI(callback) returns false, set
request attributes (error=true and errorMsg using
IdentityManagementEndpointUtil.i18n(...) with an appropriate message) and
forward to "error.jsp" instead of nullifying callback so
Utils.validateCallbackURL and isValidMultiOptionURI have consistent semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82b9f88f-d1ab-4e27-b318-a7cd116c47a6
📒 Files selected for processing (3)
api-control-plane/modules/distribution/product/src/main/extensions/password-recovery.jspapi-control-plane/modules/distribution/product/src/main/extensions/self-registration-username-request.jspapi-control-plane/modules/distribution/product/src/main/extensions/self-registration-with-verification.jsp
🚧 Files skipped from review as they are similar to previous changes (1)
- api-control-plane/modules/distribution/product/src/main/extensions/password-recovery.jsp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
all-in-one-apim/modules/distribution/product/src/main/extensions/self-registration-username-request.jsp (1)
57-64:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the sanitized callback authoritative in this form.
This all-in-one page still validates
getURLEncodedCallback(callback)but submits the rawcallback, and the hidden field is always emitted. So the value posted tosignup.dois not the same value that was validated, and invalid/null callbacks are still replayed instead of being omitted like in theapi-control-planeversion.Suggested fix
- } else { - String encodedCallback = IdentityManagementEndpointUtil.getURLEncodedCallback(callback); - if (!AuthenticationEndpointUtil.isValidMultiOptionURI(encodedCallback)) { - callback = null; - } + } else if (!AuthenticationEndpointUtil.isValidMultiOptionURI(callback)) { + callback = null; } @@ - <input id="callback" name="callback" type="hidden" - value="<%=Encode.forHtmlAttribute(callback)%>" class="form-control"> + <% if (StringUtils.isNotBlank(callback)) { %> + <input id="callback" name="callback" type="hidden" + value="<%=Encode.forHtmlAttribute(callback)%>" class="form-control"> + <% } %>#!/bin/bash # Compare the all-in-one and api-control-plane username-request pages. rg -n -C3 'getURLEncodedCallback|isValidMultiOptionURI|id="callback"|name="callback"' \ "all-in-one-apim/modules/distribution/product/src/main/extensions/self-registration-username-request.jsp" \ "api-control-plane/modules/distribution/product/src/main/extensions/self-registration-username-request.jsp"Also applies to: 195-197
🤖 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 `@all-in-one-apim/modules/distribution/product/src/main/extensions/self-registration-username-request.jsp` around lines 57 - 64, The form is validating the URL-encoded value via IdentityManagementEndpointUtil.getURLEncodedCallback(callback) and AuthenticationEndpointUtil.isValidMultiOptionURI(encodedCallback) but still emits and posts the original raw callback (hidden field id="callback" name="callback"), causing invalid callbacks to be replayed to signup.do; change the template so the sanitized/encoded callback is authoritative: compute encodedCallback via getURLEncodedCallback(callback), only render the hidden input when encodedCallback passes isValidMultiOptionURI(encodedCallback) and set its value to encodedCallback (or set callback=null/omit input when invalid), ensuring the value posted to signup.do matches what was validated.all-in-one-apim/modules/distribution/product/src/main/extensions/self-registration-with-verification.jsp (1)
193-203:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the raw callback in the all-in-one verification flow too.
This path still percent-encodes
callbackbefore callingisValidMultiOptionURI(), but later reuses the raw callback. Theapi-control-planecounterpart already validates the raw value, so this version can still approve a callback that was never checked in the same form it is propagated.Suggested fix
- } else { - String encodedCallback = IdentityManagementEndpointUtil.getURLEncodedCallback(callback); - if (!AuthenticationEndpointUtil.isValidMultiOptionURI(encodedCallback)) { - callback = null; - } + } else if (!AuthenticationEndpointUtil.isValidMultiOptionURI(callback)) { + callback = null; }#!/bin/bash # Compare the all-in-one and api-control-plane self-registration-with-verification pages. rg -n -C3 'getURLEncodedCallback|isValidMultiOptionURI|name="callback"' \ "all-in-one-apim/modules/distribution/product/src/main/extensions/self-registration-with-verification.jsp" \ "api-control-plane/modules/distribution/product/src/main/extensions/self-registration-with-verification.jsp"🤖 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 `@all-in-one-apim/modules/distribution/product/src/main/extensions/self-registration-with-verification.jsp` around lines 193 - 203, The code percent-encodes callback via IdentityManagementEndpointUtil.getURLEncodedCallback(callback) then validates the encoded value with AuthenticationEndpointUtil.isValidMultiOptionURI(encodedCallback) but later reuses the original raw callback, leaving an unvalidated raw value; change the logic in the block that handles callback to first call AuthenticationEndpointUtil.isValidMultiOptionURI(callback) on the raw callback and set callback = null if that fails, and only then produce/use an encodedCallback (via getURLEncodedCallback) if needed; update references so that AuthenticationEndpointUtil.isValidMultiOptionURI is invoked on the raw callback variable and callback is nullified on failure to prevent propagating an unchecked raw URL.
🤖 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
`@all-in-one-apim/modules/distribution/product/src/main/extensions/password-recovery.jsp`:
- Around line 212-219: The code validates the encoded callback (encodedCallback)
but later uses the original raw callback, allowing mismatches; change the flow
in the block that uses AuthenticationEndpointUtil.isValidMultiOptionURI and
IdentityManagementEndpointUtil.getURLEncodedCallback so you validate the raw
callback string first by calling
AuthenticationEndpointUtil.isValidMultiOptionURI(callback) (or a raw-URI
validation helper) and only if that returns true produce and use
IdentityManagementEndpointUtil.getURLEncodedCallback(callback); ensure the
variable callback that is propagated is the validated/encoded value (or set to
null on failed validation) so validation and the propagated value match.
---
Duplicate comments:
In
`@all-in-one-apim/modules/distribution/product/src/main/extensions/self-registration-username-request.jsp`:
- Around line 57-64: The form is validating the URL-encoded value via
IdentityManagementEndpointUtil.getURLEncodedCallback(callback) and
AuthenticationEndpointUtil.isValidMultiOptionURI(encodedCallback) but still
emits and posts the original raw callback (hidden field id="callback"
name="callback"), causing invalid callbacks to be replayed to signup.do; change
the template so the sanitized/encoded callback is authoritative: compute
encodedCallback via getURLEncodedCallback(callback), only render the hidden
input when encodedCallback passes isValidMultiOptionURI(encodedCallback) and set
its value to encodedCallback (or set callback=null/omit input when invalid),
ensuring the value posted to signup.do matches what was validated.
In
`@all-in-one-apim/modules/distribution/product/src/main/extensions/self-registration-with-verification.jsp`:
- Around line 193-203: The code percent-encodes callback via
IdentityManagementEndpointUtil.getURLEncodedCallback(callback) then validates
the encoded value with
AuthenticationEndpointUtil.isValidMultiOptionURI(encodedCallback) but later
reuses the original raw callback, leaving an unvalidated raw value; change the
logic in the block that handles callback to first call
AuthenticationEndpointUtil.isValidMultiOptionURI(callback) on the raw callback
and set callback = null if that fails, and only then produce/use an
encodedCallback (via getURLEncodedCallback) if needed; update references so that
AuthenticationEndpointUtil.isValidMultiOptionURI is invoked on the raw callback
variable and callback is nullified on failure to prevent propagating an
unchecked raw URL.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6861b730-445f-42e2-adc4-aff9d9d76e57
📒 Files selected for processing (6)
all-in-one-apim/modules/distribution/product/src/main/extensions/password-recovery.jspall-in-one-apim/modules/distribution/product/src/main/extensions/self-registration-username-request.jspall-in-one-apim/modules/distribution/product/src/main/extensions/self-registration-with-verification.jspapi-control-plane/modules/distribution/product/src/main/extensions/password-recovery.jspapi-control-plane/modules/distribution/product/src/main/extensions/self-registration-username-request.jspapi-control-plane/modules/distribution/product/src/main/extensions/self-registration-with-verification.jsp
O-sura
force-pushed
the
master
branch
2 times, most recently
from
6ade548 to
795c086
Compare
CrowleyRajapakse
approved these changes
Jun 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.