Improved: Enhance RequestHandler view state persistence and screen location resolution - #1624
Merged
ashishvijaywargiya merged 1 commit intoAug 14, 2026
Conversation
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()); | ||
| } |
Krishnauprit18
force-pushed
the
improvement/view-rendering-optimization
branch
from
August 14, 2026 05:03
b2d3970 to
e51fe34
Compare
…cation resolution
Krishnauprit18
force-pushed
the
improvement/view-rendering-optimization
branch
from
August 14, 2026 05:56
e51fe34 to
5c68185
Compare
ashishvijaywargiya
pushed a commit
to ashishvijaywargiya/ofbiz-framework
that referenced
this pull request
Aug 14, 2026
…cation resolution (apache#1624) This PR builds on [PR apache#1586](Krishnauprit18:secure-widget-resource-loading) and keeps the WidgetSecureLocation approach, adding two related changes in RequestHandler: 1. View state persistence check (_LAST_VIEW_NAME_): Validates candidate last view names against ControllerConfig before they are stored in the session, so view navigation falls back cleanly. 2. 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. Thank you Krishna for the contribution. (cherry picked from commit 9ca27f4)
ashishvijaywargiya
pushed a commit
to ashishvijaywargiya/ofbiz-framework
that referenced
this pull request
Aug 14, 2026
…cation resolution (apache#1624) This PR builds on [PR apache#1586](Krishnauprit18:secure-widget-resource-loading) and keeps the WidgetSecureLocation approach, adding two related changes in RequestHandler: 1. View state persistence check (_LAST_VIEW_NAME_): Validates candidate last view names against ControllerConfig before they are stored in the session, so view navigation falls back cleanly. 2. 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. Thank you Krishna for the contribution. (cherry picked from commit 9ca27f4)
ashishvijaywargiya
added a commit
that referenced
this pull request
Aug 14, 2026
…cation resolution (#1624) (#1653) This PR builds on [PR #1586](Krishnauprit18:secure-widget-resource-loading) and keeps the WidgetSecureLocation approach, adding two related changes in RequestHandler: 1. View state persistence check (_LAST_VIEW_NAME_): Validates candidate last view names against ControllerConfig before they are stored in the session, so view navigation falls back cleanly. 2. 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. Thank you Krishna for the contribution. (cherry picked from commit 9ca27f4) Co-authored-by: Krishna Uprit <125099508+Krishnauprit18@users.noreply.github.com>
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 on PR #1586 and keeps the WidgetSecureLocation approach, adding two related changes in RequestHandler:
View state persistence check (LAST_VIEW_NAME): Validates candidate last view names against ControllerConfig before they are stored in the session, so view navigation falls back cleanly.
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.