Improved: Enhance RequestHandler view state persistence and screen location resolution - #1624
Open
Krishnauprit18 wants to merge 1 commit into
Open
Conversation
…cation resolution
Krishnauprit18
changed the title
There was a problem hiding this comment.
Pull request overview
This PR strengthens OFBiz widget and request handling security by hardening how screen locations are resolved and how “last view” state is persisted/restored from the session, aiming to reduce injection and unsafe resource/location override vectors.
Changes:
- Introduces
WidgetSecureLocationand uses it fromScreenFactoryto sanitizecomponent://screen locations before resolution. - Enhances
RequestHandlerto validate_LAST_VIEW_NAME_against controller policy (including auth requirements) and to filter out sensitive*Location/*Screen/*Template/*Uriparameters from session-persisted view state. - Tightens authentication requirements on specific view-maps in controller XMLs.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| framework/widget/src/main/java/org/apache/ofbiz/widget/model/WidgetSecureLocation.java | Adds a centralized sanitizer for widget resource locations. |
| framework/widget/src/main/java/org/apache/ofbiz/widget/model/ScreenFactory.java | Applies location sanitization before loading referenced screens. |
| framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/RequestHandler.java | Validates/persists _LAST_VIEW_NAME_ and filters persisted/restored view parameters. |
| framework/common/webcommon/WEB-INF/portal-controller.xml | Changes showPortalPage view-map to require auth. |
| applications/product/webapp/facility/WEB-INF/controller.xml | Changes EditShipmentRouteSegments view-map to require auth. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+37
to
+43
| if (location.startsWith(COMPO_TYPE) && location.length() > 12) { | ||
| if (location.indexOf("..") > 0) { | ||
| Debug.logWarning(String.format("For security reason traversal sequence '..' is not allowed: [%s]", location), MODULE); | ||
| return null; | ||
| } | ||
| return COMPO_TYPE + Paths.get(location.substring(12)).normalize(); | ||
| } |
Comment on lines
+202
to
205
| if (sanitizedLocation == null) { | ||
| Debug.logWarning("The location of screen [%s] isn't an allowed Path. Abort rendering. Raw location [%s]", MODULE, name, location); | ||
| throw new IllegalArgumentException("Abort screen rendering due to unallowed screen location"); | ||
| } |
Comment on lines
+1209
to
+1215
| Map<String, Object> sanitizedParamMap = new HashMap<>(paramMap); | ||
| sanitizedParamMap.keySet().removeIf(key -> key != null && ( | ||
| key.endsWith("Location") | ||
| || key.endsWith("Screen") | ||
| || key.endsWith("Template") | ||
| || key.endsWith("Uri") | ||
| )); |
Comment on lines
+972
to
980
| if (key != null && !key.startsWith("_") | ||
| && !key.endsWith("Location") | ||
| && !key.endsWith("Screen") | ||
| && !key.endsWith("Template") | ||
| && !key.endsWith("Uri") | ||
| && !("_EVENT_MESSAGE_".equals(key) || "_ERROR_MESSAGE_".equals(key) | ||
| || "_EVENT_MESSAGE_LIST_".equals(key) || "_ERROR_MESSAGE_LIST_".equals(key))) { | ||
| request.setAttribute(key, urlParamEntry.getValue()); | ||
| } |
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
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.
This PR builds upon PR #1586 by preserving
WidgetSecureLocationarchitecture and introducing two Defense-in-Depth security add-ons inRequestHandler:View State Persistence Validation (
_LAST_VIEW_NAME_): Validates candidate last view names againstControllerConfigpolicies before persisting them to the session, ensuring clean view navigation fallback.Parameter-Level Attribute Filtering (
_LAST_VIEW_PARAMS_): Filters dynamic resource and location parameters (*Location,*Screen,*Template,*Uri) during session persistence and view-last attribute restoration.