USPS Integration Upgrade to REST API#5258
Open
IbrahimS2 wants to merge 8 commits intoOpenMage:mainfrom
Open
Conversation
github-actions
bot
added
Template : admin
Contributor
There was a problem hiding this comment.
Pull request overview
This PR upgrades the Magento USPS carrier integration from the legacy USPS Web Tools/XML API to a USPS REST (OAuth2) based implementation. It introduces new REST configuration options in admin, plus additional optional features like address verification, delivery estimates, and label generation.
Changes:
- Adds USPS REST OAuth2 credentials + environment configuration and admin “test connection / test rate quote” tools.
- Introduces REST client/service classes for auth, rates/tracking, service standards (delivery estimates), address verification, and label creation.
- Adds a frontend USPS address verification modal and wiring to inject it into checkout/customer address pages.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 23 comments.
Show a summary per file
| File | Description |
|---|---|
| js/mage/adminhtml/usps_config.js | Admin JS intended to auto-populate USPS REST gateway URL based on environment selection. |
| app/design/frontend/base/default/template/usa/checkout/address/verification.phtml | Frontend modal + JS/CSS for USPS address verification during checkout. |
| app/design/frontend/base/default/layout/usa.xml | Adds layout handles to inject the USPS address verification modal block on frontend pages. |
| app/design/adminhtml/default/default/template/usa/dhl/unitofmeasure.phtml | Admin template (duplicate) for DHL unit-of-measure UI behavior. |
| app/design/adminhtml/default/default/layout/usa.xml | Admin layout intended to load USPS config JS and add an extra block on system config. |
| app/code/core/Mage/Usa/etc/system.xml | Reworks USPS carrier configuration fields for REST (environment, OAuth2 creds, feature toggles). |
| app/code/core/Mage/Usa/etc/config.xml | Adds routers, observers, layout updates (frontend), and new USPS defaults for REST. |
| app/code/core/Mage/Usa/controllers/Adminhtml/UspsController.php | Admin controller endpoints for “test connection”, “test rate quote”, and “create dimensions”. |
| app/code/core/Mage/Usa/controllers/AddressController.php | Frontend controller for AJAX address verify/apply actions. |
| app/code/core/Mage/Usa/Model/Shipping/Carrier/UspsAuth.php | OAuth2 client-credentials token retrieval + caching for USPS REST API. |
| app/code/core/Mage/Usa/Model/Shipping/Carrier/Usps/Tracking/Service.php | REST tracking lookup implementation. |
| app/code/core/Mage/Usa/Model/Shipping/Carrier/Usps/Source/Pricetype.php | Source model for USPS REST pricing type selection. |
| app/code/core/Mage/Usa/Model/Shipping/Carrier/Usps/Source/Method.php | Updates method source model to REST method codes + labels. |
| app/code/core/Mage/Usa/Model/Shipping/Carrier/Usps/Source/Freemethod.php | Extends method source model to add “None” option for free method. |
| app/code/core/Mage/Usa/Model/Shipping/Carrier/Usps/Source/Environment.php | Source model for sandbox vs production environment. |
| app/code/core/Mage/Usa/Model/Shipping/Carrier/Usps/Source/Accounttype.php | Source model for USPS account type selection. |
| app/code/core/Mage/Usa/Model/Shipping/Carrier/Usps/Service/Standards.php | Service standards (delivery estimates) client + caching. |
| app/code/core/Mage/Usa/Model/Shipping/Carrier/Usps/Rest/Client.php | Generic REST client wrapper with retry support. |
| app/code/core/Mage/Usa/Model/Shipping/Carrier/Usps/Label/Service.php | USPS label generation service via REST endpoints. |
| app/code/core/Mage/Usa/Model/Shipping/Carrier/Usps/Error/Dictionary.php | Error mapping/translation for REST API responses. |
| app/code/core/Mage/Usa/Model/Shipping/Carrier/Usps/Backend/Testconnection.php | Duplicate/unused config field renderer (misplaced under Model). |
| app/code/core/Mage/Usa/Model/Shipping/Carrier/Usps/Backend/Createdimensions.php | Duplicate/unused config field renderer (misplaced under Model). |
| app/code/core/Mage/Usa/Model/Shipping/Carrier/Usps/Address/Service.php | Address verification service (REST calls + correction application). |
| app/code/core/Mage/Usa/Model/Observer.php | Observers for order address validation + label/shipment prep. |
| app/code/core/Mage/Usa/Helper/Data.php | Replaces unit conversion implementation (PhpUnitsOfMeasure → Zend_Measure) + adds USPS helper methods. |
| app/code/core/Mage/Usa/Block/Checkout/Address/Verification.php | Frontend block used to render/address verification URLs and enablement checks. |
| app/code/core/Mage/Usa/Block/Adminhtml/System/Config/Form/Field/Usps/Testratequote.php | System config field renderer: “Test Rate Quote” button + JS. |
| app/code/core/Mage/Usa/Block/Adminhtml/System/Config/Form/Field/Usps/Testconnection.php | System config field renderer: “Test Connection” button + JS. |
| app/code/core/Mage/Usa/Block/Adminhtml/System/Config/Form/Field/Usps/Createdimensions.php | System config field renderer: “Create Dimension Attributes” button + JS + status. |
Comment on lines
359
to
364
| // Wait for shipping object to be available | ||
| var checkForShipping = function() { | ||
| if (typeof window.shipping !== 'undefined' && window.shipping.save) { | ||
| this._hookShippingSave(); | ||
| } else { | ||
| // Retry after a short delay |
There was a problem hiding this comment.
hookCheckout() polls every 100ms until window.shipping.save exists, but this template is also injected into non-onepage pages (multishipping/customer address form). On those pages, window.shipping never exists, so the polling loop will run indefinitely and waste CPU. Add a max-attempt timeout or gate hookCheckout() behind a onepage-checkout detection.
Suggested change
| // Wait for shipping object to be available | |
| var checkForShipping = function() { | |
| if (typeof window.shipping !== 'undefined' && window.shipping.save) { | |
| this._hookShippingSave(); | |
| } else { | |
| // Retry after a short delay | |
| // Limit the number of attempts to avoid infinite polling on non-onepage pages | |
| var attempts = 0; | |
| var maxAttempts = 100; // 100 * 100ms = 10 seconds | |
| // Wait for shipping object to be available | |
| var checkForShipping = function() { | |
| attempts++; | |
| if (typeof window.shipping !== 'undefined' && window.shipping.save) { | |
| this._hookShippingSave(); | |
| } else if (attempts < maxAttempts) { | |
| // Retry after a short delay while under the max attempt limit |
Comment on lines
13
to
91
| * Renders a button in system configuration to create dimension EAV attributes | ||
| * Creates package_length, package_width, package_height attributes for products | ||
| * | ||
| * @package Mage_Usa | ||
| */ | ||
| class Mage_Usa_Model_Shipping_Carrier_Usps_Backend_Createdimensions | ||
| extends Mage_Adminhtml_Block_System_Config_Form_Field | ||
| { | ||
| /** | ||
| * Generate button HTML for creating dimension attributes | ||
| * | ||
| * @param Varien_Data_Form_Element_Abstract $element | ||
| * @return string | ||
| */ | ||
| protected function _getElementHtml(Varien_Data_Form_Element_Abstract $element) | ||
| { | ||
| $buttonLabel = Mage::helper('usa')->__('Create Dimension Attributes'); | ||
| $ajaxUrl = Mage::helper('adminhtml')->getUrl('adminhtml/usps/createdimensions'); | ||
|
|
||
| $html = '<button type="button" id="usps-create-dimensions-button" onclick="createUspsAttributes(\'' . $ajaxUrl . '\')" class="scalable">' | ||
| . '<span>' . $buttonLabel . '</span></button>'; | ||
| $html .= '<div id="usps-attr-result" style="margin-top:10px; font-weight:bold;"></div>'; | ||
| $html .= '<p class="note"><span>' . Mage::helper('usa')->__('Creates product attributes: package_length, package_width, package_height (in inches). These attributes are used for accurate dimensional shipping rates.') . '</span></p>'; | ||
|
|
||
| // JavaScript for AJAX call | ||
| $html .= <<<'JAVASCRIPT' | ||
| <script type="text/javascript"> | ||
| //<![CDATA[ | ||
| function createUspsAttributes(url) { | ||
| // Show loading message | ||
| var resultDiv = document.getElementById('usps-attr-result'); | ||
| resultDiv.innerHTML = '<span style="color:gray;">Creating attributes...</span>'; | ||
|
|
||
| // Disable button during request | ||
| var button = document.getElementById('usps-create-dimensions-button'); | ||
| button.disabled = true; | ||
|
|
||
| // Make AJAX request | ||
| new Ajax.Request(url, { | ||
| parameters: { | ||
| form_key: FORM_KEY | ||
| }, | ||
| onSuccess: function(response) { | ||
| button.disabled = false; | ||
| try { | ||
| var result = JSON.parse(response.responseText); | ||
| if (result.success) { | ||
| resultDiv.innerHTML = '<span style="color:green;">✓ ' + result.message + '</span>'; | ||
| } else { | ||
| resultDiv.innerHTML = '<span style="color:red;">✗ ' + result.message + '</span>'; | ||
| } | ||
| } catch(e) { | ||
| resultDiv.innerHTML = '<span style="color:red;">✗ Error parsing response: ' + e.message + '</span>'; | ||
| } | ||
| }, | ||
| onFailure: function(response) { | ||
| button.disabled = false; | ||
| resultDiv.innerHTML = '<span style="color:red;">✗ Failed to create attributes. Check server logs for details.</span>'; | ||
| } | ||
| }); | ||
| } | ||
| //]]> | ||
| </script> | ||
| JAVASCRIPT; | ||
|
|
||
| return $html; | ||
| } | ||
|
|
||
| /** | ||
| * Remove scope info from field (not needed for button) | ||
| * | ||
| * @param Varien_Data_Form_Element_Abstract $element | ||
| * @return string | ||
| */ | ||
| protected function _renderScopeLabel(Varien_Data_Form_Element_Abstract $element) | ||
| { | ||
| return ''; | ||
| } | ||
| } |
There was a problem hiding this comment.
Same issue as the testconnection backend file: this class lives under Model/.../Backend but extends an adminhtml block and appears unused (system.xml references the block class usa/adminhtml_system_config_form_field_usps_createdimensions). Please remove the duplicate or consolidate to a single implementation/location.
Suggested change
| * Renders a button in system configuration to create dimension EAV attributes | |
| * Creates package_length, package_width, package_height attributes for products | |
| * | |
| * @package Mage_Usa | |
| */ | |
| class Mage_Usa_Model_Shipping_Carrier_Usps_Backend_Createdimensions | |
| extends Mage_Adminhtml_Block_System_Config_Form_Field | |
| { | |
| /** | |
| * Generate button HTML for creating dimension attributes | |
| * | |
| * @param Varien_Data_Form_Element_Abstract $element | |
| * @return string | |
| */ | |
| protected function _getElementHtml(Varien_Data_Form_Element_Abstract $element) | |
| { | |
| $buttonLabel = Mage::helper('usa')->__('Create Dimension Attributes'); | |
| $ajaxUrl = Mage::helper('adminhtml')->getUrl('adminhtml/usps/createdimensions'); | |
| $html = '<button type="button" id="usps-create-dimensions-button" onclick="createUspsAttributes(\'' . $ajaxUrl . '\')" class="scalable">' | |
| . '<span>' . $buttonLabel . '</span></button>'; | |
| $html .= '<div id="usps-attr-result" style="margin-top:10px; font-weight:bold;"></div>'; | |
| $html .= '<p class="note"><span>' . Mage::helper('usa')->__('Creates product attributes: package_length, package_width, package_height (in inches). These attributes are used for accurate dimensional shipping rates.') . '</span></p>'; | |
| // JavaScript for AJAX call | |
| $html .= <<<'JAVASCRIPT' | |
| <script type="text/javascript"> | |
| //<![CDATA[ | |
| function createUspsAttributes(url) { | |
| // Show loading message | |
| var resultDiv = document.getElementById('usps-attr-result'); | |
| resultDiv.innerHTML = '<span style="color:gray;">Creating attributes...</span>'; | |
| // Disable button during request | |
| var button = document.getElementById('usps-create-dimensions-button'); | |
| button.disabled = true; | |
| // Make AJAX request | |
| new Ajax.Request(url, { | |
| parameters: { | |
| form_key: FORM_KEY | |
| }, | |
| onSuccess: function(response) { | |
| button.disabled = false; | |
| try { | |
| var result = JSON.parse(response.responseText); | |
| if (result.success) { | |
| resultDiv.innerHTML = '<span style="color:green;">✓ ' + result.message + '</span>'; | |
| } else { | |
| resultDiv.innerHTML = '<span style="color:red;">✗ ' + result.message + '</span>'; | |
| } | |
| } catch(e) { | |
| resultDiv.innerHTML = '<span style="color:red;">✗ Error parsing response: ' + e.message + '</span>'; | |
| } | |
| }, | |
| onFailure: function(response) { | |
| button.disabled = false; | |
| resultDiv.innerHTML = '<span style="color:red;">✗ Failed to create attributes. Check server logs for details.</span>'; | |
| } | |
| }); | |
| } | |
| //]]> | |
| </script> | |
| JAVASCRIPT; | |
| return $html; | |
| } | |
| /** | |
| * Remove scope info from field (not needed for button) | |
| * | |
| * @param Varien_Data_Form_Element_Abstract $element | |
| * @return string | |
| */ | |
| protected function _renderScopeLabel(Varien_Data_Form_Element_Abstract $element) | |
| { | |
| return ''; | |
| } | |
| } | |
| * This class is kept as a backend model placeholder to avoid misusing | |
| * adminhtml block logic under the Model namespace. | |
| * | |
| * The actual button rendering logic is implemented in the | |
| * usa/adminhtml_system_config_form_field_usps_createdimensions block. | |
| * | |
| * @deprecated Use the usa/adminhtml_system_config_form_field_usps_createdimensions | |
| * block class for rendering the configuration button. | |
| * @package Mage_Usa | |
| */ | |
| class Mage_Usa_Model_Shipping_Carrier_Usps_Backend_Createdimensions | |
| extends Mage_Core_Model_Config_Data | |
| { | |
| } |
| <cache_ttl>3600</cache_ttl> | ||
| <crid></crid> | ||
| <mid></mid> | ||
| <manifest_mid></manifest_mid> |
There was a problem hiding this comment.
The default config uses <manifest_mid>, but system.xml and the label service read carriers/usps/mmid (Manifest MID). With the current mismatch, the configured value won’t be read consistently. Please standardize on one config key across system.xml, config.xml defaults, and the PHP code.
Suggested change
| <manifest_mid></manifest_mid> | |
| <mmid></mmid> |
Comment on lines
+20
to
33
| * @param mixed $value | ||
| * @param string $sourceWeightMeasure | ||
| * @param string $toWeightMeasure | ||
| * @return int|null|string | ||
| */ | ||
| public function convertMeasureWeight($value, $sourceWeightMeasure, $toWeightMeasure) | ||
| { | ||
| if ($value) { | ||
| $unitWeight = new Mass($value, $sourceWeightMeasure); | ||
| return $unitWeight->toUnit($toWeightMeasure); | ||
| $locale = Mage::app()->getLocale()->getLocale(); | ||
| $unitWeight = new Zend_Measure_Weight($value, $sourceWeightMeasure, $locale); | ||
| $unitWeight->setType($toWeightMeasure); | ||
| return $unitWeight->getValue(); | ||
| } | ||
|
|
There was a problem hiding this comment.
This helper changed from PhpUnitsOfMeasure to Zend_Measure, but there are existing unit tests for Mage_Usa_Helper_Data (e.g., tests/unit/Mage/Usa/Helper/DataTest.php) that expect PhpUnitsOfMeasure exceptions/behavior. Please update/add tests to reflect the new conversion implementation and return types so CI stays green.
Comment on lines
11
to
120
| * USPS Test Connection Button Backend Model | ||
| * | ||
| * Renders a button in system configuration to test USPS REST API connectivity | ||
| * Tests OAuth2 authentication with provided credentials | ||
| * | ||
| * @package Mage_Usa | ||
| */ | ||
| class Mage_Usa_Model_Shipping_Carrier_Usps_Backend_Testconnection | ||
| extends Mage_Adminhtml_Block_System_Config_Form_Field | ||
| { | ||
| /** | ||
| * Generate button HTML for testing USPS API connection | ||
| * | ||
| * @param Varien_Data_Form_Element_Abstract $element | ||
| * @return string | ||
| */ | ||
| protected function _getElementHtml(Varien_Data_Form_Element_Abstract $element) | ||
| { | ||
| $buttonLabel = Mage::helper('usa')->__('Test Connection'); | ||
| $ajaxUrl = Mage::helper('adminhtml')->getUrl('adminhtml/usps/testconnection'); | ||
|
|
||
| $html = '<button type="button" id="usps-test-connection-button" onclick="testUspsConnection(\'' . $ajaxUrl . '\')" class="scalable">' | ||
| . '<span>' . $buttonLabel . '</span></button>'; | ||
| $html .= '<div id="usps-test-result" style="margin-top:10px; font-weight:bold;"></div>'; | ||
|
|
||
| // JavaScript for AJAX call | ||
| $html .= <<<'JAVASCRIPT' | ||
| <script type="text/javascript"> | ||
| //<![CDATA[ | ||
| function testUspsConnection(url) { | ||
| // Get credentials from form | ||
| var clientId = ''; | ||
| var clientSecret = ''; | ||
| var environment = ''; | ||
|
|
||
| // Try to get values from obscure inputs (encrypted fields) | ||
| var clientIdField = document.getElementById('carriers_usps_client_id'); | ||
| var clientSecretField = document.getElementById('carriers_usps_client_secret'); | ||
| var environmentField = document.getElementById('carriers_usps_environment'); | ||
|
|
||
| if (clientIdField) { | ||
| clientId = clientIdField.value; | ||
| } | ||
| if (clientSecretField) { | ||
| clientSecret = clientSecretField.value; | ||
| } | ||
| if (environmentField) { | ||
| environment = environmentField.value; | ||
| } | ||
|
|
||
| // Validate required fields | ||
| if (!clientId || !clientSecret || !environment) { | ||
| document.getElementById('usps-test-result').innerHTML = | ||
| '<span style="color:red;">✗ Please fill in Client ID, Client Secret, and Environment before testing.</span>'; | ||
| return; | ||
| } | ||
|
|
||
| // Show loading message | ||
| var resultDiv = document.getElementById('usps-test-result'); | ||
| resultDiv.innerHTML = '<span style="color:gray;">Testing connection...</span>'; | ||
|
|
||
| // Disable button during request | ||
| var button = document.getElementById('usps-test-connection-button'); | ||
| button.disabled = true; | ||
|
|
||
| // Make AJAX request | ||
| new Ajax.Request(url, { | ||
| parameters: { | ||
| client_id: clientId, | ||
| client_secret: clientSecret, | ||
| environment: environment, | ||
| form_key: FORM_KEY | ||
| }, | ||
| onSuccess: function(response) { | ||
| button.disabled = false; | ||
| try { | ||
| var result = JSON.parse(response.responseText); | ||
| if (result.success) { | ||
| resultDiv.innerHTML = '<span style="color:green;">✓ ' + result.message + '</span>'; | ||
| } else { | ||
| resultDiv.innerHTML = '<span style="color:red;">✗ ' + result.message + '</span>'; | ||
| } | ||
| } catch(e) { | ||
| resultDiv.innerHTML = '<span style="color:red;">✗ Error parsing response: ' + e.message + '</span>'; | ||
| } | ||
| }, | ||
| onFailure: function(response) { | ||
| button.disabled = false; | ||
| resultDiv.innerHTML = '<span style="color:red;">✗ Connection failed. Check server logs for details.</span>'; | ||
| } | ||
| }); | ||
| } | ||
| //]]> | ||
| </script> | ||
| JAVASCRIPT; | ||
|
|
||
| return $html; | ||
| } | ||
|
|
||
| /** | ||
| * Remove scope info from field (not needed for button) | ||
| * | ||
| * @param Varien_Data_Form_Element_Abstract $element | ||
| * @return string | ||
| */ | ||
| protected function _renderScopeLabel(Varien_Data_Form_Element_Abstract $element) | ||
| { | ||
| return ''; | ||
| } | ||
| } |
There was a problem hiding this comment.
This class is placed under Model/.../Backend but extends Mage_Adminhtml_Block_System_Config_Form_Field (a block) and is not referenced by system.xml (which uses usa/adminhtml_system_config_form_field_usps_testconnection). Keeping both implementations is confusing; please remove the unused one or wire it up consistently.
Suggested change
| * USPS Test Connection Button Backend Model | |
| * | |
| * Renders a button in system configuration to test USPS REST API connectivity | |
| * Tests OAuth2 authentication with provided credentials | |
| * | |
| * @package Mage_Usa | |
| */ | |
| class Mage_Usa_Model_Shipping_Carrier_Usps_Backend_Testconnection | |
| extends Mage_Adminhtml_Block_System_Config_Form_Field | |
| { | |
| /** | |
| * Generate button HTML for testing USPS API connection | |
| * | |
| * @param Varien_Data_Form_Element_Abstract $element | |
| * @return string | |
| */ | |
| protected function _getElementHtml(Varien_Data_Form_Element_Abstract $element) | |
| { | |
| $buttonLabel = Mage::helper('usa')->__('Test Connection'); | |
| $ajaxUrl = Mage::helper('adminhtml')->getUrl('adminhtml/usps/testconnection'); | |
| $html = '<button type="button" id="usps-test-connection-button" onclick="testUspsConnection(\'' . $ajaxUrl . '\')" class="scalable">' | |
| . '<span>' . $buttonLabel . '</span></button>'; | |
| $html .= '<div id="usps-test-result" style="margin-top:10px; font-weight:bold;"></div>'; | |
| // JavaScript for AJAX call | |
| $html .= <<<'JAVASCRIPT' | |
| <script type="text/javascript"> | |
| //<![CDATA[ | |
| function testUspsConnection(url) { | |
| // Get credentials from form | |
| var clientId = ''; | |
| var clientSecret = ''; | |
| var environment = ''; | |
| // Try to get values from obscure inputs (encrypted fields) | |
| var clientIdField = document.getElementById('carriers_usps_client_id'); | |
| var clientSecretField = document.getElementById('carriers_usps_client_secret'); | |
| var environmentField = document.getElementById('carriers_usps_environment'); | |
| if (clientIdField) { | |
| clientId = clientIdField.value; | |
| } | |
| if (clientSecretField) { | |
| clientSecret = clientSecretField.value; | |
| } | |
| if (environmentField) { | |
| environment = environmentField.value; | |
| } | |
| // Validate required fields | |
| if (!clientId || !clientSecret || !environment) { | |
| document.getElementById('usps-test-result').innerHTML = | |
| '<span style="color:red;">✗ Please fill in Client ID, Client Secret, and Environment before testing.</span>'; | |
| return; | |
| } | |
| // Show loading message | |
| var resultDiv = document.getElementById('usps-test-result'); | |
| resultDiv.innerHTML = '<span style="color:gray;">Testing connection...</span>'; | |
| // Disable button during request | |
| var button = document.getElementById('usps-test-connection-button'); | |
| button.disabled = true; | |
| // Make AJAX request | |
| new Ajax.Request(url, { | |
| parameters: { | |
| client_id: clientId, | |
| client_secret: clientSecret, | |
| environment: environment, | |
| form_key: FORM_KEY | |
| }, | |
| onSuccess: function(response) { | |
| button.disabled = false; | |
| try { | |
| var result = JSON.parse(response.responseText); | |
| if (result.success) { | |
| resultDiv.innerHTML = '<span style="color:green;">✓ ' + result.message + '</span>'; | |
| } else { | |
| resultDiv.innerHTML = '<span style="color:red;">✗ ' + result.message + '</span>'; | |
| } | |
| } catch(e) { | |
| resultDiv.innerHTML = '<span style="color:red;">✗ Error parsing response: ' + e.message + '</span>'; | |
| } | |
| }, | |
| onFailure: function(response) { | |
| button.disabled = false; | |
| resultDiv.innerHTML = '<span style="color:red;">✗ Connection failed. Check server logs for details.</span>'; | |
| } | |
| }); | |
| } | |
| //]]> | |
| </script> | |
| JAVASCRIPT; | |
| return $html; | |
| } | |
| /** | |
| * Remove scope info from field (not needed for button) | |
| * | |
| * @param Varien_Data_Form_Element_Abstract $element | |
| * @return string | |
| */ | |
| protected function _renderScopeLabel(Varien_Data_Form_Element_Abstract $element) | |
| { | |
| return ''; | |
| } | |
| } | |
| * USPS Test Connection Backend Model | |
| * | |
| * This class is kept as a backend model stub to avoid confusion with the | |
| * system configuration field block implementation, which is provided by | |
| * the alias `usa/adminhtml_system_config_form_field_usps_testconnection`. | |
| * | |
| * @package Mage_Usa | |
| */ | |
| class Mage_Usa_Model_Shipping_Carrier_Usps_Backend_Testconnection | |
| extends Mage_Core_Model_Config_Data | |
| { | |
| } |
Comment on lines
+14
to
+35
| <script type="text/javascript"> | ||
| //<![CDATA[ | ||
| function changeDimensions() | ||
| { | ||
| var dimensionUnit = '(<?php echo $this->getInch(); ?>)'; | ||
| var dhlintUnitOfMeasureNote = '<?php echo $this->getDivideOrderWeightNoteLbp(); ?>'; | ||
| if ($("carriers_dhlint_unit_of_measure").value == "K") { | ||
| dimensionUnit = '(<?php echo $this->getCm(); ?>)'; | ||
| dhlintUnitOfMeasureNote = '<?php echo $this->getDivideOrderWeightNoteKg(); ?>'; | ||
| } | ||
| $$('[for="carriers_dhlint_height"]')[0].innerHTML = '<?php echo $this->getHeight(); ?> ' + dimensionUnit; | ||
| $$('[for="carriers_dhlint_depth"]')[0].innerHTML = '<?php echo $this->getDepth(); ?> ' + dimensionUnit; | ||
| $$('[for="carriers_dhlint_width"]')[0].innerHTML = '<?php echo $this->getWidth(); ?> ' + dimensionUnit; | ||
|
|
||
| $('carriers_dhlint_divide_order_weight').next().down().innerHTML = dhlintUnitOfMeasureNote; | ||
| } | ||
| document.observe("dom:loaded", function() { | ||
| $("carriers_dhlint_unit_of_measure").observe("change", changeDimensions); | ||
| changeDimensions(); | ||
| }); | ||
| //]]> | ||
| </script> |
There was a problem hiding this comment.
This file duplicates the existing template at app/design/adminhtml/base/default/template/usa/dhl/unitofmeasure.phtml verbatim. That increases maintenance cost and risks divergence. Prefer updating the base/default template (or explain why an override is required here).
| { | ||
| var dimensionUnit = '(<?php echo $this->getInch(); ?>)'; | ||
| var dhlintUnitOfMeasureNote = '<?php echo $this->getDivideOrderWeightNoteLbp(); ?>'; | ||
| if ($("carriers_dhlint_unit_of_measure").value == "K") { |
There was a problem hiding this comment.
Use strict comparison in new JS (===) instead of == when comparing unit codes to avoid unintended truthy coercions.
Suggested change
| if ($("carriers_dhlint_unit_of_measure").value == "K") { | |
| if ($("carriers_dhlint_unit_of_measure").value === "K") { |
Comment on lines
115
to
121
| // Direct diagnostic logging of response | ||
|
|
||
| $debugData = [ | ||
| 'result' => $responseData, | ||
| '__pid' => getmypid(), | ||
| ]; | ||
| $this->_debug($debugData); |
There was a problem hiding this comment.
This debug logging stores the full OAuth response (including the USPS access_token) in the shipping debug logs. Because the access token is a bearer credential, anyone with access to these logs (e.g., via log files, backups, or support dumps) could reuse it to call USPS APIs on behalf of the store, leading to account misuse and exposure of shipping data. Avoid logging the raw OAuth response here and ensure sensitive fields like access_token are redacted or omitted before calling _debug.
Suggested change
| // Direct diagnostic logging of response | |
| $debugData = [ | |
| 'result' => $responseData, | |
| '__pid' => getmypid(), | |
| ]; | |
| $this->_debug($debugData); | |
| // Diagnostic logging without including sensitive OAuth response content | |
| $this->_debug([ | |
| 'result' => 'OAuth response received (content redacted)', | |
| '__pid' => getmypid(), | |
| ]); |
Comment on lines
554
to
563
| $debugData = [ | ||
| 'request' => [ | ||
| 'endpoint' => $endpoint, | ||
| 'payload' => $requestPayload, | ||
| ], | ||
| 'cache_key' => $cacheKey, | ||
| '__pid' => getmypid(), | ||
| ]; | ||
|
|
||
| // Log request payload for debugging |
There was a problem hiding this comment.
When USPS rate debug logging is enabled, this code logs the entire REST rate request payload, which includes the USPS accountNumber used for commercial pricing. Storing this account identifier in application logs increases the risk that someone with log access (or leaked backups/support bundles) can abuse the USPS billing account or correlate it with other secrets. Remove or redact sensitive fields such as accountNumber from the data passed to _debug, logging only non-sensitive metadata needed for troubleshooting.
Suggested change
| $debugData = [ | |
| 'request' => [ | |
| 'endpoint' => $endpoint, | |
| 'payload' => $requestPayload, | |
| ], | |
| 'cache_key' => $cacheKey, | |
| '__pid' => getmypid(), | |
| ]; | |
| // Log request payload for debugging | |
| // Create a sanitized copy of the request payload for logging and remove sensitive fields. | |
| $sanitizedPayload = $requestPayload; | |
| if (is_array($sanitizedPayload) && array_key_exists('accountNumber', $sanitizedPayload)) { | |
| unset($sanitizedPayload['accountNumber']); | |
| } | |
| $debugData = [ | |
| 'request' => [ | |
| 'endpoint' => $endpoint, | |
| 'payload' => $sanitizedPayload, | |
| ], | |
| 'cache_key' => $cacheKey, | |
| '__pid' => getmypid(), | |
| ]; | |
| // Log sanitized request payload for debugging |
js/mage/adminhtml/usps_config.js
Outdated
Comment on lines
20
to
21
| var envField = document.getElementById('carriers_usps_rest_environment'); | ||
| var urlField = document.getElementById('carriers_usps_gateway_rest_url'); |
There was a problem hiding this comment.
The DOM ids used here ("carriers_usps_rest_environment" and "carriers_usps_gateway_rest_url") don’t match the ids generated by system.xml for the USPS fields (e.g., "carriers_usps_environment" and "carriers_usps_gateway_url"). As a result, the script will exit early and never auto-populate the URL.
Suggested change
| var envField = document.getElementById('carriers_usps_rest_environment'); | |
| var urlField = document.getElementById('carriers_usps_gateway_rest_url'); | |
| var envField = document.getElementById('carriers_usps_environment'); | |
| var urlField = document.getElementById('carriers_usps_gateway_url'); |
Comment on lines
+1101
to
+1116
| <environment translate="label comment tooltip"> | ||
| <label>API Environment</label> | ||
| <comment><![CDATA[<strong>Production:</strong> https://apis.usps.com/ (Live transactions)<br/><strong>Sandbox:</strong> https://apis-tem.usps.com/ (Testing only)]]></comment> | ||
| <tooltip>Select Sandbox for testing with test credentials. Switch to Production for live customer transactions. URLs are automatically configured.</tooltip> | ||
| <frontend_type>select</frontend_type> | ||
| <source_model>usa/shipping_carrier_usps_source_environment</source_model> | ||
| <sort_order>20</sort_order> | ||
| <show_in_default>1</show_in_default> | ||
| <show_in_website>1</show_in_website> | ||
| <show_in_store>0</show_in_store> | ||
| <depends><active>1</active></depends> | ||
| </gateway_url> | ||
| <gateway_secure_url translate="label"> | ||
| <label>Secure Gateway URL</label> | ||
| </environment> | ||
| <gateway_url translate="label comment tooltip"> | ||
| <label>Gateway URL</label> | ||
| <tooltip>This field auto-populates when you select an environment above. You can manually edit it if you need to use a custom USPS API endpoint.</tooltip> | ||
| <frontend_type>text</frontend_type> |
There was a problem hiding this comment.
The new config field ids/paths are "environment" and "gateway_url", but the USPS carrier code reads "rest_environment" and "gateway_rest_url" when determining the REST gateway. With this mismatch, saved config values won’t be picked up by the carrier, and environment-based URL selection won’t work. Either keep the legacy keys or update the carrier to read the new keys consistently.
Comment on lines
+234
to
240
| <allowed_methods>USPS_GROUND_ADVANTAGE_SP,PRIORITY_MAIL_SP,PRIORITY_MAIL_FE,PRIORITY_MAIL_FB,PRIORITY_MAIL_PL,PRIORITY_MAIL_FS,PRIORITY_MAIL_FP,PRIORITY_MAIL_FA,PRIORITY_MAIL_EXPRESS_SP,PRIORITY_MAIL_EXPRESS_FE,PRIORITY_MAIL_EXPRESS_FA,FIRST_CLASS_PACKAGE_INTERNATIONAL_SERVICE_SP,PRIORITY_MAIL_INTERNATIONAL_SP,PRIORITY_MAIL_INTERNATIONAL_FE,PRIORITY_MAIL_INTERNATIONAL_FS,PRIORITY_MAIL_INTERNATIONAL_FB,PRIORITY_MAIL_INTERNATIONAL_PL,PRIORITY_MAIL_EXPRESS_INTERNATIONAL_SP,PRIORITY_MAIL_EXPRESS_INTERNATIONAL_FE</allowed_methods> | ||
| <container>VARIABLE</container> | ||
| <cutoff_cost/> | ||
| <free_method/> | ||
| <gateway_url>https://production.shippingapis.com/ShippingAPI.dll</gateway_url> | ||
| <gateway_secure_url>https://secure.shippingapis.com/ShippingAPI.dll</gateway_secure_url> | ||
| <shipment_requesttype>0</shipment_requesttype> | ||
| <environment>production</environment> | ||
| <gateway_url>https://api.usps.com/</gateway_url> | ||
| <handling/> |
There was a problem hiding this comment.
Default USPS settings are inconsistent with the rest of the implementation: (1) gateway_url is set to "https://api.usps.com/" while other code/constants use "https://apis.usps.com/", and (2) allowed_methods includes codes like "PRIORITY_MAIL_EXPRESS_INTERNATIONAL_SP/FE" that aren’t present in the USPS method source model. This will lead to invalid defaults and missing/unchecked options in admin.
Comment on lines
12
to
31
| public function testconnectionAction() | ||
| { | ||
| $request = $this->getRequest(); | ||
| $environment = $request->getParam('environment'); | ||
| $clientId = $request->getParam('client_id'); | ||
| $clientSecret = $request->getParam('client_secret'); | ||
| $websiteCode = $request->getParam('website'); | ||
| $storeCode = $request->getParam('store'); | ||
|
|
||
| if ($clientId === '******') { | ||
| $clientId = $this->_getConfig('carriers/usps/client_id', $websiteCode, $storeCode); | ||
| } | ||
| if ($clientSecret === '******') { | ||
| $clientSecret = $this->_getConfig('carriers/usps/client_secret', $websiteCode, $storeCode); | ||
| } | ||
|
|
||
| try { | ||
| if ($clientId === '' || $clientId === null || $clientSecret === '' || $clientSecret === null || $environment === '' || $environment === null) { | ||
| throw new Exception('Client ID, Client Secret, and Environment are required.'); | ||
| } |
There was a problem hiding this comment.
These admin actions perform privileged operations (API credential testing, rate quote calls) but do not validate the admin form_key. Since the JS sends form_key, the controller should call _validateFormKey() and reject requests when invalid to prevent CSRF.
| public function getCacheTtl($store = null) | ||
| { | ||
| $ttl = Mage::getStoreConfig('carriers/usps/cache_ttl', $store); | ||
| return $ttl ? (int)$ttl : 3600; |
There was a problem hiding this comment.
getCacheTtl() treats a configured TTL of "0" as falsy and returns 3600, but the system config describes 0 as "disable caching". Preserve 0 by checking for null/empty string instead of truthiness.
Suggested change
| return $ttl ? (int)$ttl : 3600; | |
| if ($ttl === null || $ttl === '') { | |
| return 3600; | |
| } | |
| return (int)$ttl; |
| $cached = Mage::app()->getCache()->load($cacheKey); | ||
|
|
||
| if ($cached !== false) { | ||
| $estimates[$mailClass] = unserialize($cached); |
There was a problem hiding this comment.
This unserialize() reads from cache without restricting allowed_classes. For consistency with other core code and to avoid object injection risks, pass allowed_classes=false (or store JSON instead of PHP serialization).
Suggested change
| $estimates[$mailClass] = unserialize($cached); | |
| $estimates[$mailClass] = unserialize($cached, ['allowed_classes' => false]); |
|
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
1 participant
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.
Description (*)
Replace the legacy USPS XML/web-tools transport with a REST-only USPS integration. This is a breaking change: stores must supply USPS REST credentials (OAuth2 Client ID/Secret) and select the appropriate API environment after upgrade. There is no legacy fallback.
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)