shipperhq / module-shipper

Base ShipperHQ Repo
Open Software License 3.0
21 stars 21 forks source link

More than 255 Characters In Customer Address Street Field Causes a 503 error #52

Closed nikolazic closed 5 years ago

nikolazic commented 6 years ago

Steps to replicate

  1. Login as an existing customer
  2. Go to My Account > Address Book > Add New Address (or Edit Address under an existing address)
  3. Fill out the form
  4. Enter a 256-character string in any of the street fields.
  5. Click Save Address

Actual A 503 error

Expected The form should reload with an error message:

"Street Address" length must be equal or less than 255 characters.

The issue is in the ShipperHQ_Shipper module observer: ShipperHQ\Shipper\Observer\CustomerAddressEdit

Line 80:

$this->addressRepository->save($existingAddress);

This save throw an exception if address has more than 256 characters. This is normally caught in the controller and the error is saved in the session. However, this observer is in post-dispatch and it does not catch this error, it let's it bubble up and cause a 503.

Line 80 should be in a try/catch and an exception should caught, logged, or the observer should not fire if there are issues found to begin with (consider either inspecting the message storage or observing session_abstract_add_message. See \Magento\Framework\Message\Manager::addMessage)

wsajosh commented 6 years ago

Hi @nikolazic

Thanks for raising this issue. I'm going to investigate it and will release a extension update if required. I'm assuming this is a edge case and not causing you massive issues. I'm aiming to investigate this early next week.

wsajosh commented 6 years ago

Hi @nikolazic

I've not been able to reproduce this. I'm running the latest ShipperHQ code on Magento 2.2.4. I only have ShipperHQ installed.

I've tried shipping to both US and non US addresses with a 256 character street line 1 and I see the error message that I'd expect to see.

Please let me know if you have any further information on this issue.

gorillagroup commented 6 years ago

@wsajosh ,

We are running 2.2.3. We will try reproducing this again with the latest module and 2.2.3 and let you know what we find.

vkalchenko commented 6 years ago

@wsajosh We were able to reproduce the bug on a clean magento 2.2.3 installation. Please see detailed steps to reproduce below:

  1. Install clean Magento 2.2.3
  2. Register new customer
  3. Create new address
  4. Edit this address and enter 256 character street line 1
  5. Press Save Address and confirm that you see error message "Street Address" length must be equal or less than 255 characters.

  6. Install module ShipperHQ_Shipper composer require shipperhq/module-shipper
  7. Refresh Magento modules php bin/magento module:enable --all php bin/magento setup:upgrade --keep-generated php bin/magento cache:clean
  8. Edit customer address and enter 256 character street line 1
  9. Press Save Address and confirm that you see error 500
vkalchenko commented 6 years ago

@wsajosh Proposed solution is here - https://github.com/shipperhq/module-shipper/pull/54

wsajosh commented 6 years ago

@vkalchenko Thanks! I can see the error now. I've tested it with your proposed fix and it does stop the server error but I'm seeing the error message show up twice. I don't think we need to add to the message queue in the catch as its been added already. Are you seeing the same error message twice in your tests?

vkalchenko commented 6 years ago

@wsajosh Yes, I've also seen the duplicate error. You're right, no need to add this message to the message queue, adding just try...catch should be enough. See updated pull request - https://github.com/shipperhq/module-shipper/pull/55

wsajosh commented 6 years ago

@vkalchenko Thanks! Looks good. I've got our lead development reviewing this so it can be merged in

wsajosh commented 5 years ago

This has been implemented in our module and will be in the next release. Thanks all involved for the detailed steps and fix