Validate and escape all inputs #22

z38 · 2018-03-02T21:09:00Z

This patch ensures all user inputs are either validated against a regexp (e.g. IBAN, ISR references) or are a subset of the officially supported character set.

In addition, the strings are escaped properly (where necessary). Previously, we relied on DOMDocument::createElement($tag, $content) which does no escaping at all.

sdespont · 2018-03-03T11:57:53Z

@z38 Assert all user's inputs is a good idea. Some inputs must throw exception like "identifiers" or strings which must respect TEXT_SWIFT format, postal accounts, ISR reference number ,... . Therefore, your Text class is very useful.

But in my point of view, this class should sanitize TEXT_CH inputs and not throwing an exception. These strings are present to eases the review of the debtors and creditors names and addresses. Misspelling these strings or missing characters in theses strings must not be a showstopper to the XML creation.

As a user of this library, I don't want to catch an exception if my user want to create a payment which involve a company name like "360° S.A". I would like that the library sanitize the name in "360 S.A" for example. Same example with a company with a long name like "VAUDOISE GENERALE, Compagnie d'Assurances SA", I would like that the end of the string would be removed to fit the 35 chars of the spec.

Otherwise, the sanitation must be done by all the users of the library before calling it.

z38 · 2018-03-09T10:50:45Z

Thank you for your feedback. I agree that sanitizing all strings manually definitely is cumbersome. On the other hand, I can see that one might want an exception (e.g. to show a validation error to the user).

To satisfy both needs, I added two methods to sanitize user inputs. In addition, there is a special constructor for postal addresses. With these additions, it should be possible to sanitize most inputs for a message without much additional effort. I'd be happy to hear your thoughts on the proposed approach.

sdespont · 2018-03-10T08:47:28Z

@z38 Thanks, your implementation is well done 👍

Just some points to discuss directly in the code itself.

sdespont

Good job BTW.

sdespont · 2018-03-10T08:28:25Z

src/Z38/SwissPayment/StructuredPostalAddress.php

+     * @param string      $town       Town name
+     * @param string      $country    Country code (ISO 3166-1 alpha-2)
+     */
+    public static function sanitize($street, $buildingNo, $postCode, $town, $country = 'CH')


I don't think it is really clear for (new) lib users to have specific static sanitize method as second constructor. I would suggest to implement a boolean parameter to the __constuctor in order to specify if the inputs must be sanitized or not. This parameter would be false by default to avoid BC break.

I think we will keep it separate to ensure people are aware of the lossy sanitization process (and avoid further complication of the constructor).

sdespont · 2018-03-10T08:32:38Z

src/Z38/SwissPayment/StructuredPostalAddress.php

@@ -43,11 +43,31 @@ class StructuredPostalAddress implements PostalAddressInterface
     */
    public function __construct($street, $buildingNo, $postCode, $town, $country = 'CH')


Is it really a good idea to let the $country with a default param? I mean, if someone needs to create a payment, he knows for which country his address is. I know that would include a BC break, but as the version is still 0.*, users must pay attention to the release note before updating.

As the library is targeted towards Switzerland, I will keep the default argument for now. I will keep your point in mind in case we extend the scope of the library.

sdespont · 2018-03-10T08:40:56Z

src/Z38/SwissPayment/Text.php

+    public static function sanitize($input, $maxLength)
+    {
+        $input = preg_replace('/\s+/', ' ', (string) $input);
+        $input = trim(preg_replace(self::TEXT_NOT_CH, '', $input));


I would replace "non-ch" compliant characters with a substitution char like a space, a dot or a dash because otherwise it could render 2 words stuck together making the reading difficult. But, because "non-ch" compliant characters should be quiet rare, it should be ok to just remove it like you did. So, I let you chose.

sdespont · 2018-03-10T08:43:58Z

src/Z38/SwissPayment/Text.php

+    /**
+     * @internal
+     */
+    public static function sanitizeCountryCode($input)


This method must not exists. Only assertCountryCode method must be used.

👍 Removed, this method is not very helpful indeed.

sdespont · 2018-03-10T09:07:33Z

src/Z38/SwissPayment/Text.php

+    /**
+     * @internal
+     */
+    public static function assertCountryCode($input)


In this assertion, it would be great to check that the country code is valid isn't it?
The full ISO3166 country code list can be retrieved here : https://gist.github.com/vxnick/380904

As the developers need to present a list of valid countries the end users anyway, I will leave that open for now.

sdespont · 2018-03-11T08:01:13Z

@z38 Also, do not forget to escape CreditTransfer/setRemittanceInformation method.

swiss-payment/src/Z38/SwissPayment/TransactionInformation/CreditTransfer.php

Lines 112 to 124 in bd3fe38

    
               /** 
        
                * Sets the unstructured remittance information 
        
                * 
        
                * @param string|null $remittanceInformation 
        
                * 
        
                * @return CreditTransfer This credit transfer 
        
                */ 
        
               public function setRemittanceInformation($remittanceInformation) 
        
               { 
        
                   $this->remittanceInformation = $remittanceInformation; 
        
                   return $this; 
        
               }

sdespont · 2018-03-18T08:38:12Z

@z38 Did you find the time to check my review?

z38 · 2018-03-20T12:50:34Z

@sdespont Thank you for your feedback! I'm really glad you took the time to review the patch thoroughly. I'm sorry it took me so long to respond. Feel free to comment if you found any issues or something needs some rethinking.

sdespont · 2018-03-20T20:37:59Z

@z38 I think that we are ready for a merge 👍

sdespont · 2018-03-23T10:20:38Z

@z38 It would be great if you find the time to merge and tag a 0.6.0 version these next days.

z38 force-pushed the escaping branch from ef94f51 to 9366c15 Compare March 2, 2018 21:14

z38 mentioned this pull request Mar 2, 2018

Sanitize strings to never render invalid XML regarding the XSD #21

Closed

z38 force-pushed the escaping branch from 9366c15 to 4299fb7 Compare March 3, 2018 07:14

sdespont reviewed Mar 10, 2018

View reviewed changes

z38 force-pushed the escaping branch 2 times, most recently from b6ac491 to a80b702 Compare March 20, 2018 12:34

z38 added 3 commits March 25, 2018 13:15

Validate and escape all inputs

0956b42

Add methods to sanitize inputs

01e4b06

Show usage of sanitizing constructor

bb7dbed

z38 force-pushed the escaping branch from a80b702 to bb7dbed Compare March 25, 2018 11:17

z38 merged commit 3eefd9f into master Mar 25, 2018

z38 deleted the escaping branch March 25, 2018 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate and escape all inputs #22

Validate and escape all inputs #22

z38 commented Mar 2, 2018

sdespont commented Mar 3, 2018

z38 commented Mar 9, 2018

sdespont commented Mar 10, 2018

sdespont left a comment

sdespont Mar 10, 2018 •

edited

Loading

z38 Mar 20, 2018

sdespont Mar 10, 2018 •

edited

Loading

z38 Mar 20, 2018

sdespont Mar 10, 2018 •

edited

Loading

sdespont Mar 10, 2018

z38 Mar 20, 2018

sdespont Mar 10, 2018

z38 Mar 20, 2018

sdespont commented Mar 11, 2018

sdespont commented Mar 18, 2018

z38 commented Mar 20, 2018 •

edited

Loading

sdespont commented Mar 20, 2018

sdespont commented Mar 23, 2018

		@@ -43,11 +43,31 @@ class StructuredPostalAddress implements PostalAddressInterface
		*/
		public function __construct($street, $buildingNo, $postCode, $town, $country = 'CH')

Validate and escape all inputs #22

Validate and escape all inputs #22

Conversation

z38 commented Mar 2, 2018

sdespont commented Mar 3, 2018

z38 commented Mar 9, 2018

sdespont commented Mar 10, 2018

sdespont left a comment

Choose a reason for hiding this comment

sdespont Mar 10, 2018 • edited Loading

Choose a reason for hiding this comment

z38 Mar 20, 2018

Choose a reason for hiding this comment

sdespont Mar 10, 2018 • edited Loading

Choose a reason for hiding this comment

z38 Mar 20, 2018

Choose a reason for hiding this comment

sdespont Mar 10, 2018 • edited Loading

Choose a reason for hiding this comment

sdespont Mar 10, 2018

Choose a reason for hiding this comment

z38 Mar 20, 2018

Choose a reason for hiding this comment

sdespont Mar 10, 2018

Choose a reason for hiding this comment

z38 Mar 20, 2018

Choose a reason for hiding this comment

sdespont commented Mar 11, 2018

sdespont commented Mar 18, 2018

z38 commented Mar 20, 2018 • edited Loading

sdespont commented Mar 20, 2018

sdespont commented Mar 23, 2018

sdespont Mar 10, 2018 •

edited

Loading

sdespont Mar 10, 2018 •

edited

Loading

sdespont Mar 10, 2018 •

edited

Loading

z38 commented Mar 20, 2018 •

edited

Loading