JSON loading should follow OWASP reccomendation #257

Closed
ldusan84 opened this Issue Mar 16, 2013 · 7 comments

Comments

Projects
2 participants

@verklov verklov added major-accept and removed minor-check labels Apr 18, 2014

@verklov verklov self-assigned this Apr 18, 2014

@verklov

This comment has been minimized.

Show comment
Hide comment
@verklov

verklov Apr 18, 2014

Contributor

@ldusan84, thank you for the issue and sorry for the delay! There is a ticket in the backlog. We will notify you once the team resolves this issue.

Contributor

verklov commented Apr 18, 2014

@ldusan84, thank you for the issue and sorry for the delay! There is a ticket in the backlog. We will notify you once the team resolves this issue.

magento-team added a commit that referenced this issue Jul 4, 2014

2.0.0.0-dev85
* Service layer updates:
  * Implemented API for the CatalogInventory module
  * Refactored the external usages of the CatalogInventory module to service
* Fixed bugs:
  * Fixed an issue where a coupon usage option was not comprehensible enough
  * Fixed an issue where products selection for adding to a bundle option was lost when switching between pages with product grids
  * Fixed an issue where  Google Content was not sending the correct 'description' attribute
  * Fixed an issue where custom attributes were not displayed in layered navigation after a product import
  * Fixed an issue where the Category URL keys did not work correctly after saving
  * Fixed an issue where an admin could not create a Target rule with a certain Products to Display condition
  * Fixed a jQuery error on a product page in the Admin panel, which appeared when switching between product tabs
* Framework Improvements:
  * Created ProductsCustomOptions Service API for Catalog module
  * Created DownloadableLink Service API for Catalog module
* GitHub requests:
  * [#257] JSON loading should follow OWASP recommendation
@verklov

This comment has been minimized.

Show comment
Hide comment
@verklov

verklov Jul 21, 2014

Contributor

@ldusan84, we have fixed the issue that you reported and released the fix in dev85. Thank you again for contributing to Magento quality! We are closing this issue.

Contributor

verklov commented Jul 21, 2014

@ldusan84, we have fixed the issue that you reported and released the fix in dev85. Thank you again for contributing to Magento quality! We are closing this issue.

@verklov verklov closed this Jul 21, 2014

@verklov

This comment has been minimized.

Show comment
Hide comment
@verklov

verklov Jul 28, 2014

Contributor

@ldusan84, I saw your tweet that you did not find the code we released in dev85 in scope of this issue. I investigated to find out if there was a mistake and the code was not included, but nope, the developer confirmed his code changes are there.

He made an assumption that his implementation was different from what you might have expected. Here is what he suggested:

Look for Magento\Framework\App\Response\Http::representJson($content) method.
You can find its numerous usages in places that did require appropriate Content-Type header.

Please let us know if this explains everything or you would still like to get more information. If yes, please note what exactly leaves you puzzled here and I will try to connect you and the developer to make sure you receive all the answers.

Contributor

verklov commented Jul 28, 2014

@ldusan84, I saw your tweet that you did not find the code we released in dev85 in scope of this issue. I investigated to find out if there was a mistake and the code was not included, but nope, the developer confirmed his code changes are there.

He made an assumption that his implementation was different from what you might have expected. Here is what he suggested:

Look for Magento\Framework\App\Response\Http::representJson($content) method.
You can find its numerous usages in places that did require appropriate Content-Type header.

Please let us know if this explains everything or you would still like to get more information. If yes, please note what exactly leaves you puzzled here and I will try to connect you and the developer to make sure you receive all the answers.

@ldusan84

This comment has been minimized.

Show comment
Hide comment
@ldusan84

ldusan84 Jul 28, 2014

Collaborator

Hi @verklov

Thanks for your effort on this issue.

My concern was mainly regarding that the mime type on script tags that output json should be "application/json" and not "text/javascript". I realize it's a minor issue and that this vulnerability is not likely to be exploited, but I think it's a good practice to follow OWASP standards.

Let me know what you think.

Thanks
Dusan

Collaborator

ldusan84 commented Jul 28, 2014

Hi @verklov

Thanks for your effort on this issue.

My concern was mainly regarding that the mime type on script tags that output json should be "application/json" and not "text/javascript". I realize it's a minor issue and that this vulnerability is not likely to be exploited, but I think it's a good practice to follow OWASP standards.

Let me know what you think.

Thanks
Dusan

@verklov

This comment has been minimized.

Show comment
Hide comment
@verklov

verklov Jul 28, 2014

Contributor

Hi @ldusan84, I will let the developer know of your concerns tomorrow. If this requires some changes in the code to correspond to the OWASP standards, we will definitely initiate this change.

Let me get back to you once I have the decision made.

Best regards,
Sergey

Contributor

verklov commented Jul 28, 2014

Hi @ldusan84, I will let the developer know of your concerns tomorrow. If this requires some changes in the code to correspond to the OWASP standards, we will definitely initiate this change.

Let me get back to you once I have the decision made.

Best regards,
Sergey

@verklov

This comment has been minimized.

Show comment
Hide comment
@verklov

verklov Jul 30, 2014

Contributor

@ldusan84, I got a response from the developer to your latest comment in this thread:

Magento uses tags with type set to text/javascript for regular javascript code. Content type application/json has to be used when script tag contains only JSON. If you have found any such tags in Magento code, please let us know and we will fix it. Magento uses pure JSON mainly in AJAX requests, and as we mentioned in the initial post we have already fixed those cases (Magento now sets correct content type for JSON responses).

Once again, thank you for your input.

Contributor

verklov commented Jul 30, 2014

@ldusan84, I got a response from the developer to your latest comment in this thread:

Magento uses tags with type set to text/javascript for regular javascript code. Content type application/json has to be used when script tag contains only JSON. If you have found any such tags in Magento code, please let us know and we will fix it. Magento uses pure JSON mainly in AJAX requests, and as we mentioned in the initial post we have already fixed those cases (Magento now sets correct content type for JSON responses).

Once again, thank you for your input.

@ldusan84

This comment has been minimized.

Show comment
Hide comment
@ldusan84

ldusan84 Jul 30, 2014

Collaborator

Hi @verklov

Thanks for the response. In the meantime I have investigated this a bit and it seems that mime type on script tag is not really that important, so I think that's good the way it is.

I really like the way this issue has been resolved, thanks again.

Regards
Dusan

Collaborator

ldusan84 commented Jul 30, 2014

Hi @verklov

Thanks for the response. In the meantime I have investigated this a bit and it seems that mime type on script tag is not really that important, so I think that's good the way it is.

I really like the way this issue has been resolved, thanks again.

Regards
Dusan

vpelipenko added a commit that referenced this issue Apr 30, 2015

Merge pull request #257 from magento-goinc/MAGETWO-33527
[GoInc] MAGETWO-33527 M2 GitHub Update (version 0.74.0-beta6)

magento-team pushed a commit that referenced this issue Dec 23, 2015

mmansoorebay pushed a commit that referenced this issue Aug 18, 2016

Merge pull request #257 from magento-south/MAGETWO-24139
Stories:
* MAGETWO-24139: Resolve TODO's related to Customer Service or create stories to resolve them
* MAGETWO-56007: Initialize default values in customer custom attributes metadata
* MAGETWO-56008: Moving getStoreByWebsite to Store Module

@magento-engcom-team magento-engcom-team moved this from TODO to Done in branch [2.3-develop] Sep 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment