Encoding issues with product names

[Amit Gadaley]

Hello All,

Recently working for a client I encountered a weird issue related to
special characters encodings. We have product names containing special
characters like ' (apostrophes). When we create orders for it, an encoded
value for it is stored in OrderItem.itemDescription. The same encoded value
also copied for invoice and return. When I checked the Product entity
record, the original value (name without encoding) was stored there. I
debugged the issue at code level and found that the system does encoding
(string or html) at the time of order creation.

I understand that for security reasons (and I want to know more about it),
the system does the encodings. My concerns are related to not using
encoding when we create products. And it is not good UI experience to
display encoded forms of values to screens.

I suggest we should use some methods to display encoded values properly on
screens or remove the encoding at the very first place.

Please feel free to provide any suggestions or inputs.

--
Kind Regards,
Amit Gadaley
*Technical Consultant*
*HotWax Systems*
*Enterprise open source experts*
cell: [phone]
office: 0731-409-3684
http://www.hotwaxsystems.com

[Jacques Le Roux]

Hi Amit,

It's better to encode to prevent XSS. Then of course we need to decode when showing those values.
Actually in this case it's automatically encoded by Freemarker as explained in this old but still good reference:
https://ofbiz.markmail.org/thread/e2iznsqhhxxdplxh

So we can do the same, ie using StringUtil.wrapString(), like

<input size="60" type="text" name="description_${cartLineIndex}" value="${StringUtil.wrapString(cartLine.getName(dispatcher))?default("")}"/><br />

This should be done everywhere it's needed in FTL files.

I have added a patch for similar "cartLine.get...()" cases at OFBIZ-12029.
Of course other cases like that can pop out anytime; eg, I'll also fix a long awaiting one at  OFBIZ-7343...

We could think that using Freemarker autoescaping as suggested in OFBIZ-7675 would be better.
But escaping is not encoding. You can check by using ?html (local autoescaping ) instead of StringUtil.wrapString(). You get "tes&amp;&#x23;39&#x3b;t"

For widgets forms, there is a problem currently investigated with OFBIZ-12026...

HTH

Jacques

Le 26/09/2020 à 11:00, Amit Gadaley a écrit :

> Hello All,
>
> Recently working for a client I encountered a weird issue related to
> special characters encodings. We have product names containing special
> characters like ' (apostrophes). When we create orders for it, an encoded
> value for it is stored in OrderItem.itemDescription. The same encoded value
> also copied for invoice and return. When I checked the Product entity
> record, the original value (name without encoding) was stored there. I
> debugged the issue at code level and found that the system does encoding
> (string or html) at the time of order creation.
>
> I understand that for security reasons (and I want to know more about it),
> the system does the encodings. My concerns are related to not using
> encoding when we create products. And it is not good UI experience to
> display encoded forms of values to screens.
>
> I suggest we should use some methods to display encoded values properly on
> screens or remove the encoding at the very first place.
>
> Please feel free to provide any suggestions or inputs.
>

[Mridul Pathak-5]

Hi Amit,

I agree with Jacques. Though I see that in shopping cart implementation
when copying product name to order item name it uses string encoding vs
html encoding, I think this could be fixed to use html encoding for
product/item name like it's done for product/item description in the same
method.

Thanks.
Mridul Pathak

On Mon, Sep 28, 2020 at 2:21 PM Jacques Le Roux <
[hidden email]> wrote:

> Hi Amit,
>
> It's better to encode to prevent XSS. Then of course we need to decode
> when showing those values.
> Actually in this case it's automatically encoded by Freemarker as
> explained in this old but still good reference:
> https://ofbiz.markmail.org/thread/e2iznsqhhxxdplxh
>
> So we can do the same, ie using StringUtil.wrapString(), like
>
> <input size="60" type="text" name="description_${cartLineIndex}"
> value="${StringUtil.wrapString(cartLine.getName(dispatcher))?default("")}"/><br
> />
>
> This should be done everywhere it's needed in FTL files.
>
> I have added a patch for similar "cartLine.get...()" cases at OFBIZ-12029.
> Of course other cases like that can pop out anytime; eg, I'll also fix a
> long awaiting one at  OFBIZ-7343...
>
> We could think that using Freemarker autoescaping as suggested in
> OFBIZ-7675 would be better.
> But escaping is not encoding. You can check by using ?html (local
> autoescaping ) instead of StringUtil.wrapString(). You get
> "tes&amp;&#x23;39&#x3b;t"
>
> For widgets forms, there is a problem currently investigated with
> OFBIZ-12026...
>
> HTH
>
> Jacques
>
> Le 26/09/2020 à 11:00, Amit Gadaley a écrit :
> > Hello All,
> >
> > Recently working for a client I encountered a weird issue related to
> > special characters encodings. We have product names containing special
> > characters like ' (apostrophes). When we create orders for it, an encoded
> > value for it is stored in OrderItem.itemDescription. The same encoded
> value
> > also copied for invoice and return. When I checked the Product entity
> > record, the original value (name without encoding) was stored there. I
> > debugged the issue at code level and found that the system does encoding
> > (string or html) at the time of order creation.
> >
> > I understand that for security reasons (and I want to know more about
> it),
> > the system does the encodings. My concerns are related to not using
> > encoding when we create products. And it is not good UI experience to
> > display encoded forms of values to screens.
> >
> > I suggest we should use some methods to display encoded values properly
> on
> > screens or remove the encoding at the very first place.
> >
> > Please feel free to provide any suggestions or inputs.
> >
>

[Jacques Le Roux]

Hi Mridul,

Maybe I miss what you mean, because everywhere, not only in ShoppingCartItem.java, "html" encoding is used not "string" encoding. Again, the reason is
to prevent XSS attacks.

I understand that you suggest that, eg:
String productName = ProductContentWrapper.getProductContentAsText(product, "PRODUCT_NAME", this.locale, dispatcher, "string");
instead of currently
String productName = ProductContentWrapper.getProductContentAsText(product, "PRODUCT_NAME", this.locale, dispatcher, "html");

Do you mean something else?

If it's what you mean then we should not change. Because else it's easy to replace a product/item name at
https://localhost:8443/ordermgr/control/orderentry
by <script>alert('xss')</script> <https://localhost:8443/ordermgr/control/product?product_id=testQuote>

Then if you finalize the order and get to https://localhost:8443/ordermgr/control/processorder
the new product/item name is showing and it you click on it you get to
https://localhost:8443/ordermgr/control/product?product_id=yourProductId
ans get an "XSS" attack.

If you mean something else, please explain

Thanks

Jacques

Le 28/09/2020 à 16:44, Mridul Pathak a écrit :

> Hi Amit,
>
> I agree with Jacques. Though I see that in shopping cart implementation
> when copying product name to order item name it uses string encoding vs
> html encoding, I think this could be fixed to use html encoding for
> product/item name like it's done for product/item description in the same
> method.
>
> Thanks.
> Mridul Pathak
>
> On Mon, Sep 28, 2020 at 2:21 PM Jacques Le Roux <
> [hidden email]> wrote:
>
>> Hi Amit,
>>
>> It's better to encode to prevent XSS. Then of course we need to decode
>> when showing those values.
>> Actually in this case it's automatically encoded by Freemarker as
>> explained in this old but still good reference:
>> https://ofbiz.markmail.org/thread/e2iznsqhhxxdplxh
>>
>> So we can do the same, ie using StringUtil.wrapString(), like
>>
>> <input size="60" type="text" name="description_${cartLineIndex}"
>> value="${StringUtil.wrapString(cartLine.getName(dispatcher))?default("")}"/><br
>> />
>>
>> This should be done everywhere it's needed in FTL files.
>>
>> I have added a patch for similar "cartLine.get...()" cases at OFBIZ-12029.
>> Of course other cases like that can pop out anytime; eg, I'll also fix a
>> long awaiting one at  OFBIZ-7343...
>>
>> We could think that using Freemarker autoescaping as suggested in
>> OFBIZ-7675 would be better.
>> But escaping is not encoding. You can check by using ?html (local
>> autoescaping ) instead of StringUtil.wrapString(). You get
>> "tes&amp;&#x23;39&#x3b;t"
>>
>> For widgets forms, there is a problem currently investigated with
>> OFBIZ-12026...
>>
>> HTH
>>
>> Jacques
>>
>> Le 26/09/2020 à 11:00, Amit Gadaley a écrit :
>>> Hello All,
>>>
>>> Recently working for a client I encountered a weird issue related to
>>> special characters encodings. We have product names containing special
>>> characters like ' (apostrophes). When we create orders for it, an encoded
>>> value for it is stored in OrderItem.itemDescription. The same encoded
>> value
>>> also copied for invoice and return. When I checked the Product entity
>>> record, the original value (name without encoding) was stored there. I
>>> debugged the issue at code level and found that the system does encoding
>>> (string or html) at the time of order creation.
>>>
>>> I understand that for security reasons (and I want to know more about
>> it),
>>> the system does the encodings. My concerns are related to not using
>>> encoding when we create products. And it is not good UI experience to
>>> display encoded forms of values to screens.
>>>
>>> I suggest we should use some methods to display encoded values properly
>> on
>>> screens or remove the encoding at the very first place.
>>>
>>> Please feel free to provide any suggestions or inputs.
>>>

[Jacques Le Roux]

Le 01/10/2020 à 08:42, Jacques Le Roux a écrit :
> by <script>alert('xss')</script> <https://localhost:8443/ordermgr/control/product?product_id=testQuote>
This has been inadvertently copied there (actually copied from HTML browser source page when copying <script>alert('xss')</script>, I'm a leazy
type-writer)