[
https://issues.apache.org/jira/browse/OFBIZ-11306?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17018938#comment-17018938
]
Jacques Le Roux commented on OFBIZ-11306:
-----------------------------------------
Hi James,
h3. What I agree about
I agree about the rules. Pagination, finding, listing should also be exempted
from CSRF token check, I guess there are other kinds. So there would not be
issues for back and forth.
I agree that it's better to put csrf-token information in the controllers
(Request Map ). Also as the possibility already exists.
h3. Some questions and "answers"
bq. Explanation for tokens in navigation In OFBiz, there are forms that use the
same uri for getting the form and posting the changes.
Have you few examples of that (one would be sufficient)? We need to be sure
that we are not missing anything.
bq. While there is CSRF token in the form URL, the token is invalidated during
form submission.
Could you please explain where/how is that done? Is that depending on being a
POST method as in {{tokenMap.remove(requestUri);}} in CsrfUtil::checkToken?
bq. Explanation for tokens in navigation. This is the reason why token was
invalidated each time it was used in a GET request. However, to allow back
forward browser actions to work, tokens aren't invalidated for GET request as a
compromise between usability and security.
I'd prefer that we change all the "same uri for getting the form and posting
the changes.". Somehow what you did for processorder in OFBIZ-11319
bq. Explanation for tokens in form URL (defined in FTL files). In order to
manually add a CSRF token field, user need to ensure the uri used is the same
in both the form action attribute and the CSRF token field.
Though I'd add preferred rather to add the token in a hidden field. I
understand it's an easy way to automatically do it, and seems safe. As with the
previous point we need to be sure that all forms use the POST method. Also we
need to do it for at least ofbizContentUrl and check no others would miss it.
h3. Suggestions:
* I sugget we make {{return size() > 100;}} in CsrfUtil::getTokenMap a
properties to allow users to adjust in function of their needs.
* Some recommend to encrypt IP and "Timeout" in the CSRF token and check. We
could do that by using a JWT token rather than a random value. We could then
check both IP and "Timeout" to increase safety.
h3. We have few small problems
# with Time limitation due to session timeout in ecommerce (maybe the price to
pay), eg add to compare after session ended for an anymous user.
# SetTimeZoneFromBrowser when starting:
org.apache.ofbiz.webapp.control.RequestHandlerException: Invalid or missing
CSRF token for AJAX call to path '/SetTimeZoneFromBrowser'. Also not only when
starting.
h3. We have several issues (at least) in Webtools. All work on trunk demo.
* Entity Reference - Interactive Version (also it's duplicated, we should keep
only one entry, the one in Entity Engine Tools section)
* Service Reference (also it's diplicated, we should keep only one entry, the
one in Service Engine section)
* Entity Reference - Static Version
* Entity Reference - PDF
* Induce Model XML from Database
* (most?) options in Check/Update Database
I guess the last (and maybe others) are related to nested request. Like with
checkLogin that you handled in CsrfUtil::generateTokenForNonAjax, not sure why
it does not workfor those. More on that below...
h3. Issues in Entity Data Maintenance related to new REST work at OFBIZ-11007:
Open an entity like
webtools/control/ViewGeneric?entityName=Agreement&agreementId=8000
Edit does not work, nor create, nor delete. They work if you revert
6e1c7b5958cc1a80be5746bfd177136a1feabe0d commit (last one for OFBIZ-11007)
It's because we have nested requests with this special syntax:
{code:xml}
<!-- === Rest Entity Mapping === -->
<!-- form for creating a record -->
<request-map uri="entity/list" method="get"><security https="true"
auth="true"/><response name="success" type="view" value="entitymaint"
/></request-map>
<!-- form for creating a record -->
<request-map uri="entity/create/{entityName}" method="get"><security
https="true" auth="true"/><response name="success" type="view"
value="EditGeneric"/></request-map>
<!-- form for modifying a record -->
<request-map uri="entity/edit/{entityName}/{pkValues: .*}"
method="get"><security https="true" auth="true"/><response name="success"
type="view" value="EditGeneric" /></request-map>
<!--view relations for a given entity -->
<request-map uri="entity/relations/{entityName}" method="get"><security
https="true" auth="true"/><response name="success" type="view"
value="ViewRelations" /></request-map>
<!-- getting the view of records -->
<request-map uri="entity/find/{entityName}" method="get"><security
https="true" auth="true"/><response name="success" type="view"
value="FindGeneric" /></request-map>
<!-- adding new record (from submitted form) -->
<request-map uri="entity/change/{entityName}" method="post">
<security https="true" auth="true"/>
<event type="java" path="org.apache.ofbiz.webtools.GenericWebEvent"
invoke="updateGeneric"/>
<response name="success" type="view" value="FindGeneric"/>
<response name="error" type="view" value="ViewGeneric"/>
</request-map>
<!-- view entity records -->
<request-map uri="entity/find/{entityName}/{pkValues: .*}"
method="get"><security https="true" auth="true"/><response name="success"
type="view" value="ViewGeneric" /></request-map>
<!-- modifying existing record -->
<request-map uri="entity/change/{entityName}/{pkValues: .*}" method="put">
<security https="true" auth="true"/>
<event type="java" path="org.apache.ofbiz.webtools.GenericWebEvent"
invoke="updateGeneric"/>
<response name="success" type="view" value="ViewGeneric"/>
<response name="error" type="view" value="ViewGeneric"/>
</request-map>
<!-- deleting existing record -->
<request-map uri="entity/change/{entityName}/{pkValues: .*}"
method="delete">
<security https="true" auth="true"/>
<event type="java" path="org.apache.ofbiz.webtools.GenericWebEvent"
invoke="updateGeneric"/>
<response name="success" type="view" value="ViewGeneric"/>
<response name="error" type="view" value="ViewGeneric"/>
</request-map>
{code}
So we get errors like
2020-01-16 12:41:00,998 |jsse-nio-8443-exec-6 |CsrfUtil
|E| request map is null for path entity/find
2020-01-16 12:41:49,544 |jsse-nio-8443-exec-6 |CsrfUtil
|E| request map is null for path entity/edit
2020-01-16 12:27:45,222 |jsse-nio-8443-exec-4 |CsrfUtil
|E| request map is null for path entity/create
2020-01-16 12:28:48,342 |jsse-nio-8443-exec-4 |CsrfUtil
|E| request map is null for path entity/relations
I tried to cope with it using
{noformat}
@@ -798,7 +827,11 @@ public class RequestHandler {
if (pathInfo.get(0).indexOf('?') > -1) {
return pathInfo.get(0).substring(0, pathInfo.get(0).indexOf('?'));
} else {
- return pathInfo.get(0);
+ if (1 < StringUtils.countMatches(path, "/")) {
+ return pathInfo.get(0) + "/" + pathInfo.get(1);
+ } else {
+ return pathInfo.get(0);
+ }
{noformat}
and reusing RequestHandler::getRequestUri in several places notably in CsrfUtil
class. It helps but seems not to be enough.
It seems the same(?) problem exist for searchorders when using a serarch
parameter (OK w/o, ie empty by default). We get this message:
bq. Invalid or missing CSRF token to path '/orderview'. Click here to continue
Maybe related to tokenMap creation at {{tokenMap = getTokenMap(request,
"/"+RequestHandler.getRequestUri(pathOrRequestUri));}}
Also I tried to cope with Rest links containing {noformat}"/"{noformat}
using:
{noformat}
diff --git framework/webtools/template/entity/ViewGeneric.ftl
framework/webtools/template/entity/ViewGeneric.ftl
index 216eb63aaa..60c432d9e4 100644
--- framework/webtools/template/entity/ViewGeneric.ftl
+++ framework/webtools/template/entity/ViewGeneric.ftl
@@ -53,6 +53,7 @@ function ShowTab(lname) {
<a href='<@ofbizUrl>entity/find/${entityName}</@ofbizUrl>'
class="buttontext">${uiLabelMap.WebtoolsBackToFindScreen}</a>
<#if enableEdit = "false">
<#if hasCreatePermission>
+ <#assign currentFindString = currentFindString?replace("/", "/")!>
<form
action="<@ofbizUrl>entity/edit/${currentFindString}</@ofbizUrl>" method="get">
<input type="submit" value="${uiLabelMap.CommonEdit}" />
</form>
{noformat}
But not sure it was a good way, maybe the link issue does not come from that...
h3. Lastly about formatting
Please check
https://cwiki.apache.org/confluence/display/OFBIZ/Coding+Conventions. For
instance:
{code:java}if (partyTokenMap==null){{code}
should be
{code:java}if (partyTokenMap == null) {{code}
and
{code:java}controlPath +
(requestURI.startsWith("/")?requestURI:"/"+requestURI));{code}
should be
{code:java}controlPath + (requestURI.startsWith("/") ? requestUri : "/" +
requestUri);{code}
etc.
> POC for CSRF Token
> ------------------
>
> Key: OFBIZ-11306
> URL: https://issues.apache.org/jira/browse/OFBIZ-11306
> Project: OFBiz
> Issue Type: Improvement
> Components: ALL APPLICATIONS
> Affects Versions: Upcoming Branch
> Reporter: James Yong
> Assignee: Jacques Le Roux
> Priority: Minor
> Labels: CSRF
> Fix For: Upcoming Branch
>
> Attachments: OFBIZ-11306-v2.patch, OFBIZ-11306.patch,
> OFBIZ-11306.patch, OFBIZ-11306.patch, OFBIZ-11306.patch, OFBIZ-11306.patch,
> OFBIZ-11306.patch, OFBIZ-11306.patch, OFBIZ-11306.patch, OFBIZ-11306.patch,
> OFBIZ-11306.patch, OFBIZ-11306.patch, OFBIZ-11306.patch,
> OFBIZ-11306_Plugins.patch, OFBIZ-11306_Plugins.patch,
> OFBIZ-11306_Plugins.patch, OFBIZ-11306_Plugins.patch,
> OFBIZ-11306_Plugins.patch
>
>
> CRSF tokens are generated using SecureRandom class.
> 1) In widget form where a hidden token field is auto-generated.
> 2) In FTL form where a <@csrfTokenField> macro is used to generate the csrf
> token field.
> 3) In Ajax call where a <@csrfTokenAjax> macro is used to assign csrf token
> to X-CSRF-Token in request header.
> CSRF tokens are stored in the user sessions, and verified during POST request.
> A new attribute i.e. csrf-token is added to the security tag to exempt CSRF
> token check.
> Certain request path, like LookupPartyName, can be exempt from CSRF token
> check during Ajax POST call.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
