SAK-50599 Samigo fix SEB return behavior #14313
Conversation
WalkthroughAdds UI fragment population when secure delivery START validation fails, conditions secure delivery initialization in DeliveryBean, improves locale-aware fragment rendering, adjusts secure delivery authoring inputs and JS handling, introduces a SEB return URL flow and propagation, and removes an accesskey from a secure delivery error button. Changes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (8)**/*.{html,jsp,vm,xhtml,xml}📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
Files:
**/*.{html,jsp,vm,xhtml,xml,css,scss}📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
Files:
**/*.{js,html,jsp,vm,xhtml,xml}📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
Files:
**/*.{html,jsp,jspx,xml,ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.js📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
Files:
**/*.{js,ts,jsx,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*/*.{js,ts}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.java📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
Files:
🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-10-07T15:11:27.298ZApplied to files:
📚 Learning: 2025-12-08T23:22:13.173ZApplied to files:
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (7)
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
🤖 Fix all issues with AI Agents
In @samigo/samigo-app/src/webapp/js/deliverySafeExamBrowser.js:
- Around line 32-38: The getSelectURL() server-side accessor in
DeliveryBean.java currently returns the LESSONBUILDER_RETURNURL_SAMIGO session
attribute unvalidated; update getSelectURL() to validate the session value
against an allowlist before returning it—accept only same-origin or whitelisted
paths (e.g., relative paths starting with "/" but not protocol-relative "//", or
full URLs matching the server's host/origin or explicit trusted paths); if
validation fails, return a safe default (e.g., application context path or null)
and log the rejection. Use URI/URL parsing to detect host/port/scheme and
normalize input, reference getSelectURL(), DeliveryBean.java and
LESSONBUILDER_RETURNURL_SAMIGO when making the change.
🧹 Nitpick comments (2)
samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/delivery/LinearAccessDeliveryActionListener.java (1)
129-132: Consider extracting ResourceLoader to a static field or injecting it as a dependency.This inline instantiation pattern (
new ResourceLoader().getLocale()) is used throughout the samigo module. While this code path executes only on delivery validation failure (a non-frequent scenario), extracting a static ResourceLoader instance would improve code clarity and avoid repeated service lookups within the error handling path.samigo/samigo-app/src/webapp/js/deliverySafeExamBrowser.js (1)
148-162: Consider error handling for URL construction and document the timing assumption.Two observations:
URL construction (lines 148-150): If
getLaunchSebLink()returns an invalid URL, thenew URL()constructor will throw. Consider wrapping in try-catch for robustness.Arbitrary timeout (lines 158-161): The 500ms delay before redirecting back is a timing assumption that may be fragile across different browsers or system loads. While this is a common pattern for protocol handler launches, consider:
- Documenting why 500ms was chosen
- Whether browser events (like
beforeunload) could provide more reliable timing- Testing across browsers to validate this duration
🔎 Proposed error handling for URL construction
document.addEventListener("DOMContentLoaded", () => { - const launchUrl = new URL(getLaunchSebLink()); - launchUrl.searchParams.set("return", getReturnUrl()); - configureLink(launchSebLinkId, launchUrl.toString()); + try { + const launchUrl = new URL(getLaunchSebLink()); + launchUrl.searchParams.set("return", getReturnUrl()); + configureLink(launchSebLinkId, launchUrl.toString()); + } catch (error) { + console.error("Failed to construct SEB launch URL:", error); + } configureLink(downloadSebLinkId, downloadSebLink); configureLink(downloadConfigLinkId, getDownloadConfigLink());
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/delivery/DeliveryActionListener.javasamigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/delivery/LinearAccessDeliveryActionListener.javasamigo/samigo-app/src/webapp/js/authoringSecureDeliverySettings.jssamigo/samigo-app/src/webapp/js/deliverySafeExamBrowser.jssamigo/samigo-app/src/webapp/jsf/delivery/beginTakingAssessment.jspsamigo/samigo-app/src/webapp/jsf/delivery/secureDeliveryError.jsp
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{html,jsp,vm,xhtml,xml}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
**/*.{html,jsp,vm,xhtml,xml}: Use Bootstrap 5.2 as the preferred UI framework for styling in Sakai frontends
Leverage Bootstrap 5 components for consistent UI/UX
Files:
samigo/samigo-app/src/webapp/jsf/delivery/secureDeliveryError.jspsamigo/samigo-app/src/webapp/jsf/delivery/beginTakingAssessment.jsp
**/*.{html,jsp,vm,xhtml,xml,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
Ensure all UI components work across different screen sizes (Responsive Design)
Files:
samigo/samigo-app/src/webapp/jsf/delivery/secureDeliveryError.jspsamigo/samigo-app/src/webapp/jsf/delivery/beginTakingAssessment.jsp
**/*.{js,html,jsp,vm,xhtml,xml}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
Use the web components in the webcomponents/ directory when possible in Sakai frontends
Files:
samigo/samigo-app/src/webapp/jsf/delivery/secureDeliveryError.jspsamigo/samigo-app/src/webapp/js/authoringSecureDeliverySettings.jssamigo/samigo-app/src/webapp/jsf/delivery/beginTakingAssessment.jspsamigo/samigo-app/src/webapp/js/deliverySafeExamBrowser.js
**/*.{html,jsp,jspx,xml,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer kebab-case for values of HTML class and id attributes
Files:
samigo/samigo-app/src/webapp/jsf/delivery/secureDeliveryError.jspsamigo/samigo-app/src/webapp/js/authoringSecureDeliverySettings.jssamigo/samigo-app/src/webapp/jsf/delivery/beginTakingAssessment.jspsamigo/samigo-app/src/webapp/js/deliverySafeExamBrowser.js
**/*.js
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
**/*.js: Use clean, standard modern JavaScript in Sakai frontends
Update jQuery code to modern JavaScript when making changes, if the changes are minimal
Prefer ES6+ features (arrow functions, template literals, destructuring, etc.) in JavaScript
Write modular, reusable JavaScript components in Sakai frontends
Minimize use of global variables and functions (Avoid Global Scope) in JavaScript
Files:
samigo/samigo-app/src/webapp/js/authoringSecureDeliverySettings.jssamigo/samigo-app/src/webapp/js/deliverySafeExamBrowser.js
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Target evergreen browsers; assume ES2022+ features and browser APIs likefetchkeepalive are present; avoid legacy branches, UA sniffing, or fallbacks unless a specific evergreen gap is documented
Replace jQuery with modern DOM APIs when touching code; new work should not add jQuery dependencies
Compose Lit components, ES modules, and encapsulated helpers; keep state local and explicit with modular code
Prefer module scope or class fields; expose intentional APIs instead of incidental globals; avoid global side channels
Files:
samigo/samigo-app/src/webapp/js/authoringSecureDeliverySettings.jssamigo/samigo-app/src/webapp/js/deliverySafeExamBrowser.js
**/*/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Internal reactive state in Lit components should stay prefixed with
_and is only surfaced through getters/setters when required
Files:
samigo/samigo-app/src/webapp/js/authoringSecureDeliverySettings.jssamigo/samigo-app/src/webapp/js/deliverySafeExamBrowser.js
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/delivery/DeliveryActionListener.javasamigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/delivery/LinearAccessDeliveryActionListener.java
🧠 Learnings (3)
📓 Common learnings
Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 0
File: :0-0
Timestamp: 2025-10-07T15:11:27.298Z
Learning: In samigo’s Total Scores view (samigo/samigo-app/src/webapp/jsf/evaluation/totalScores.jsp), mailto links were hidden after commit dee05746 (PR #12312, SAK-49674) added a render check requiring email.fromEmailAddress to be non-empty; PR #14154 (SAK-52058) restores visibility by checking only description.email.
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Commit messages should follow: <issue key> <component> <brief description> (e.g., SAK-12345 Assignments add option x)
📚 Learning: 2025-10-07T15:11:27.298Z
Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 0
File: :0-0
Timestamp: 2025-10-07T15:11:27.298Z
Learning: In samigo’s Total Scores view (samigo/samigo-app/src/webapp/jsf/evaluation/totalScores.jsp), mailto links were hidden after commit dee05746 (PR #12312, SAK-49674) added a render check requiring email.fromEmailAddress to be non-empty; PR #14154 (SAK-52058) restores visibility by checking only description.email.
Applied to files:
samigo/samigo-app/src/webapp/jsf/delivery/secureDeliveryError.jspsamigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/delivery/DeliveryActionListener.javasamigo/samigo-app/src/webapp/jsf/delivery/beginTakingAssessment.jsp
📚 Learning: 2025-12-08T23:22:13.173Z
Learnt from: hornersa
Repo: sakaiproject/sakai PR: 14282
File: samigo/samigo-app/src/webapp/jsf/author/item/timed.jsp:22-28
Timestamp: 2025-12-08T23:22:13.173Z
Learning: In JSF, h:outputLabel's for attribute is resolved to the actual clientId at render time. You can safely use a shortened value (e.g., for="timedHours") when the rendered output will map to a full clientId (like itemForm:timed:timedHours). This is valid for accessibility and keeps markup concise. Apply this guidance to JSP files under the samigo-app JSF views; ensure the rendered label correctly associates with the corresponding input for assistive technologies.
Applied to files:
samigo/samigo-app/src/webapp/jsf/delivery/secureDeliveryError.jspsamigo/samigo-app/src/webapp/jsf/delivery/beginTakingAssessment.jsp
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: sakai-deploy
- GitHub Check: maven-build
- GitHub Check: maven-build
🔇 Additional comments (9)
samigo/samigo-app/src/webapp/jsf/delivery/beginTakingAssessment.jsp (1)
50-50: LGTM! SEB return URL propagation implemented correctly.The hidden input field properly binds to
delivery.selectURLand is conditionally rendered only whendelivery.sebSetupis true, ensuring it participates in SEB-enabled assessment flows without affecting other delivery modes.samigo/samigo-app/src/webapp/jsf/delivery/secureDeliveryError.jsp (1)
49-54: LGTM! Accesskey removal improves accessibility.Removing the unused accesskey attribute prevents potential keyboard shortcut conflicts and aligns with modern accessibility best practices. The button retains all necessary functional attributes.
samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/delivery/LinearAccessDeliveryActionListener.java (1)
61-61: Import added for locale support.The ResourceLoader import enables locale-aware HTML fragment rendering for secure delivery failures, supporting proper internationalization.
samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/delivery/DeliveryActionListener.java (1)
371-374: LGTM! Locale-aware fragment rendering added.The change correctly populates the secure delivery HTML fragment with locale information on START phase failure, ensuring error messages are properly internationalized. This is consistent with the parallel change in LinearAccessDeliveryActionListener.
samigo/samigo-app/src/webapp/js/authoringSecureDeliverySettings.js (3)
52-59: LGTM! Improved radio input handling.The change from a single-element pattern to
querySelectorAllwithforEachcorrectly handles all radio inputs sharing the same name attribute, ensuring the enable operation applies to all related inputs. This is more robust and idiomatic.
61-68: LGTM! Consistent disable logic.The disable method mirrors the enable method's improved pattern, ensuring all radio inputs with the given name are properly disabled.
296-312: LGTM! Enhanced UI state management for secure delivery settings.The additions correctly manage visibility of help texts and the exit password field across different module selections:
- NONE case (line 304): hides all help texts since no module is active
- Default case (lines 307-309): cleans up help texts and ensures exit password is visible for non-SEB modules
This prevents UI state inconsistencies and ensures users see only relevant configuration options.
samigo/samigo-app/src/webapp/js/deliverySafeExamBrowser.js (2)
59-59: Good defensive coding improvement.Adding the
linkcheck before callingremove()prevents potential runtime errors when the element doesn't exist.
132-136: Critical fix: corrected key validation flow.The swapped logic now correctly handles the SEB key lifecycle:
- When keys are empty:
updateKeys()is called to fetch them asynchronously, withonSebKeysPresentas the callback- When keys are present:
onSebKeysPresent()is invoked immediatelyThe previous logic was inverted and would have attempted validation before keys were available.
| function getReturnUrl() { | ||
| const target = document.getElementById(returnUrlId)?.value; | ||
| if (target && target.trim() !== "") { | ||
| return target; | ||
| } | ||
| return window.location.href; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where sebReturnUrl is populated and check for validation
# Search for sebReturnUrl in JSP/Java files across the repo
rg -n 'sebReturnUrl' -t java -t jsp --max-count=20Repository: sakaiproject/sakai
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Search for sebReturnUrl across all files without strict type filtering
rg -n 'sebReturnUrl' -i --max-count=50
# Also search for the file itself to understand context
fd 'deliverySafeExamBrowser' -type fRepository: sakaiproject/sakai
Length of output: 527
🏁 Script executed:
#!/bin/bash
# Find the delivery bean class
fd -e 'java' | xargs rg -l 'class.*Delivery.*Bean|class.*Delivery.*' | head -10
# Search for selectURL property
rg -n 'selectURL' --max-count=30Repository: sakaiproject/sakai
Length of output: 2913
🏁 Script executed:
#!/bin/bash
# Read DeliveryBean.java and find selectURL property
cat samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/bean/delivery/DeliveryBean.java | head -100
# Also search for selectURL definition
rg -n 'selectURL' samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/bean/delivery/DeliveryBean.java -A 5 -B 2Repository: sakaiproject/sakai
Length of output: 5026
🏁 Script executed:
#!/bin/bash
# Get file size first
wc -l samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/bean/delivery/DeliveryBean.java
# Search for selectURL with context
rg -n 'selectURL' samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/bean/delivery/DeliveryBean.java -B 5 -A 10Repository: sakaiproject/sakai
Length of output: 161
🏁 Script executed:
#!/bin/bash
# Case-insensitive search for selectURL
rg -i 'selecturl' samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/bean/delivery/ -B 3 -A 5
# Also check other delivery bean files
rg -i 'selecturl' samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/bean/delivery/ --files-with-matchesRepository: sakaiproject/sakai
Length of output: 1401
🏁 Script executed:
#!/bin/bash
# Get the complete getSelectURL method
rg -n 'public String getSelectURL' samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/bean/delivery/DeliveryBean.java -A 25Repository: sakaiproject/sakai
Length of output: 1185
🏁 Script executed:
#!/bin/bash
# Search for validation of LESSONBUILDER_RETURNURL_SAMIGO
rg -i 'LESSONBUILDER_RETURNURL_SAMIGO' -B 3 -A 3
# Check if there's any URL validation utility used elsewhere
rg -i 'validateurl|validateredirect|allowlist' samigo/ --max-count=20Repository: sakaiproject/sakai
Length of output: 7601
Add server-side validation in getSelectURL() to prevent open redirect vulnerability.
The getSelectURL() method in DeliveryBean.java returns the LESSONBUILDER_RETURNURL_SAMIGO session attribute without validation. Although the JavaScript code in deliverySafeExamBrowser.js reads this value safely, the server-side value should be validated against an allowlist (same-origin URLs or specific known paths) to prevent open redirect attacks, even if LessonBuilder is expected to set safe values.
🤖 Prompt for AI Agents
In @samigo/samigo-app/src/webapp/js/deliverySafeExamBrowser.js around lines 32 -
38, The getSelectURL() server-side accessor in DeliveryBean.java currently
returns the LESSONBUILDER_RETURNURL_SAMIGO session attribute unvalidated; update
getSelectURL() to validate the session value against an allowlist before
returning it—accept only same-origin or whitelisted paths (e.g., relative paths
starting with "/" but not protocol-relative "//", or full URLs matching the
server's host/origin or explicit trusted paths); if validation fails, return a
safe default (e.g., application context path or null) and log the rejection. Use
URI/URL parsing to detect host/port/scheme and normalize input, reference
getSelectURL(), DeliveryBean.java and LESSONBUILDER_RETURNURL_SAMIGO when making
the change.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.