Add Improvement#14187
Conversation
e19166
requested review from
AnuGayan,
Arshardh,
chamilaadhi and
tharindu1st
as code owners
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughTwo product modules update their ChangesRegistration URL helper encoding
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 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: 2
🧹 Nitpick comments (1)
api-control-plane/modules/distribution/product/src/main/extensions/basicauth.jsp (1)
542-542: ⚡ Quick winRedundant
StringEscapeUtils.escapeHtml4()around a JS-encoded valueThe return value of
getRegistrationUrlis nowEncode.forJavaScript(rawUrl).Encode.forJavaScriptis already "safe for use in HTML script attributes (such as onclick)" and "the caller MUST provide the surrounding quotation characters." Wrapping the result inStringEscapeUtils.escapeHtml4()adds a redundant HTML-encoding pass. It doesn't corrupt the output in practice (sinceforJavaScriptalready encodes<,>,&as\uXXXX), but it creates a misleading layered-encoding pattern that suggests the JS-encoded value is not already safe for the HTMLonclickattribute context.Per the OWASP example:
<button onclick="alert('<%=Encode.forJavaScript(data)%>');">— no additional HTML escaping is needed.🔧 Proposed fix
-onclick="window.location.href='<%=StringEscapeUtils.escapeHtml4(getRegistrationUrl(accountRegistrationEndpointURL, urlEncodedURL, urlParameters))%>';" +onclick="window.location.href='<%=getRegistrationUrl(accountRegistrationEndpointURL, urlEncodedURL, urlParameters)%>';"🤖 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 `@api-control-plane/modules/distribution/product/src/main/extensions/basicauth.jsp` at line 542, The onclick attribute is double-encoding a JS-safe value: remove the redundant StringEscapeUtils.escapeHtml4() wrapper around the getRegistrationUrl(...) call (which now returns Encode.forJavaScript(rawUrl)) and output the getRegistrationUrl result directly inside the existing surrounding quotes so the JS-encoded string is used as-is; locate the call to StringEscapeUtils.escapeHtml4(getRegistrationUrl(accountRegistrationEndpointURL, urlEncodedURL, urlParameters)) in the onclick attribute and replace it with the direct getRegistrationUrl(...) result while keeping the surrounding onclick="... '...' ..." quotation characters 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
`@api-control-plane/modules/distribution/product/src/main/extensions/basicauth.jsp`:
- Around line 588-593: The code incorrectly applies Encode.forHtmlAttribute to
urlEncodedURL inside getRegistrationUrl; remove that intermediate HTML encoding
so urlEncodedURL (which is already percent-encoded via URLEncoder.encode(...))
is concatenated directly into rawUrl, and keep a single
Encode.forJavaScript(rawUrl) at the end; specifically, update getRegistrationUrl
to stop calling Encode.forHtmlAttribute(urlEncodedURL) and use urlEncodedURL
as-is when building rawUrl before returning Encode.forJavaScript(rawUrl).
- Around line 588-593: The getRegistrationUrl method currently returns
Encode.forJavaScript(rawUrl) which is unsafe for use inside an onclick
attribute; change the encoding to Encode.forJavaScriptAttribute(rawUrl) so the
value is safely encoded for JavaScript attribute context (update the return in
getRegistrationUrl and remove any extra HTML-escaping wrappers at call sites
such as in basicauth.jsp that were compensating incorrectly); ensure the method
signature getRegistrationUrl(...) still builds rawUrl the same way and only the
final Encode.* call is replaced with Encode.forJavaScriptAttribute.
---
Nitpick comments:
In
`@api-control-plane/modules/distribution/product/src/main/extensions/basicauth.jsp`:
- Line 542: The onclick attribute is double-encoding a JS-safe value: remove the
redundant StringEscapeUtils.escapeHtml4() wrapper around the
getRegistrationUrl(...) call (which now returns Encode.forJavaScript(rawUrl))
and output the getRegistrationUrl result directly inside the existing
surrounding quotes so the JS-encoded string is used as-is; locate the call to
StringEscapeUtils.escapeHtml4(getRegistrationUrl(accountRegistrationEndpointURL,
urlEncodedURL, urlParameters)) in the onclick attribute and replace it with the
direct getRegistrationUrl(...) result while keeping the surrounding onclick="...
'...' ..." quotation characters 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 80c0034f-5b40-4153-bfd7-727aaccc58a8
📒 Files selected for processing (2)
all-in-one-apim/modules/distribution/product/src/main/extensions/basicauth.jspapi-control-plane/modules/distribution/product/src/main/extensions/basicauth.jsp
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)
all-in-one-apim/modules/distribution/product/src/main/extensions/login.jsp (1)
547-554:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
domainis appended to the URL withoutencodeURIComponent().The raw text-field value is concatenated directly into the redirect URL. A user who types e.g.
corp.example.com&sessionDataKey=attacker_keywill inject an extra query parameter, potentially overridingsessionDataKeyor other sensitive params. Since this PR is already hardening the call-site arguments on this exact code path, encodingdomainhere is the natural completion of that work.🛡️ Proposed fix
if (domain != "") { document.location = "<%=commonauthURL%>?idp=" + key + "&authenticator=" + value + - "&sessionDataKey=<%=Encode.forUriComponent(request.getParameter("sessionDataKey"))%>&domain=" + - domain; + "&sessionDataKey=<%=Encode.forUriComponent(request.getParameter("sessionDataKey"))%>&domain=" + + encodeURIComponent(domain); }🤖 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/login.jsp` around lines 547 - 554, The redirect builds document.location in login.jsp by concatenating domain directly into the query string (when setting document.location = "<%=commonauthURL%>?...&domain=" + domain); fix it by applying JavaScript encoding to the domain value (use encodeURIComponent(domain)) before concatenation so the domain cannot inject extra query parameters; update both branches that set document.location (the branch that appends &domain= and the else branch if relevant) so all user-controlled pieces are encoded similarly, keeping commonauthURL and existing Encode.forUriComponent usage unchanged.
🤖 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.
Outside diff comments:
In `@all-in-one-apim/modules/distribution/product/src/main/extensions/login.jsp`:
- Around line 547-554: The redirect builds document.location in login.jsp by
concatenating domain directly into the query string (when setting
document.location = "<%=commonauthURL%>?...&domain=" + domain); fix it by
applying JavaScript encoding to the domain value (use
encodeURIComponent(domain)) before concatenation so the domain cannot inject
extra query parameters; update both branches that set document.location (the
branch that appends &domain= and the else branch if relevant) so all
user-controlled pieces are encoded similarly, keeping commonauthURL and existing
Encode.forUriComponent usage unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8367ad1a-b039-4de6-b840-43800e854b7d
📒 Files selected for processing (1)
all-in-one-apim/modules/distribution/product/src/main/extensions/login.jsp
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #14187 +/- ##
============================================
+ Coverage 11.25% 19.05% +7.80%
- Complexity 827 1419 +592
============================================
Files 361 361
Lines 17719 17719
Branches 1897 1897
============================================
+ Hits 1994 3377 +1383
+ Misses 15691 14301 -1390
- Partials 34 41 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
e19166
force-pushed
the
master
branch
2 times, most recently
from
19e93de to
84fcc7b
Compare
3 participants
Add improvement