synshop / membership.synshop.org

External Membership Management System for SYN Shop
GNU General Public License v3.0
0 stars 0 forks source link

Fix JS bug that allows form submission when Full Name is empty #50

Closed munroebot closed 1 month ago

munroebot commented 4 months ago

Deployed to https://membership-dev.synshop.org/

munroebot commented 4 months ago

Capturing @mrjones-plip feedback:

I was able to sign up with email 4111111111111111@mailinator.com and full name of   (space)

----------

when I log back in my full name is now "None" which is not what I put (I put   (space)).

-----

It's kinda weird that I can delete my card, choose "Pause Membership" 
and it still requires me to re-enter my card.  I feel that "Pause" should 
require a valid card - I guess that's what "Delete" is for then?

-----

When you click "Submit" nothing happens. The page just hangs for like 3 seconds 
and then reloads with an message at the top that I didn't readily see (even though 
I think you added that message the top at my behest  )

The instant "Submit" is pressed, it  would be good to disable the "Submit" button 
and make a lil spinner show up . 

-----

We obviously don't handle javascript not being enabled very well, but I fully 
acknowledge that that's a very weird thing that I do.

-------

If I enter a name of                                     <script>alert(1)</script> it changes it to  &lt;script&gt;alert(1)&lt;/script&gt;.  Which if I then save again it turns into &amp;lt;script&amp;gt;alert(1)&amp;lt;/script&amp;gt;  which if I then save again it turns into &amp;amp;lt;script&amp;amp;gt;alert(1)&amp;amp;lt;/script&amp;amp;gt;

tl;dr 
* we turn all & to &amp; thus & are not allowed - this is for both Full Name and Discord.
* we also cleanse all < and > into &lt; and &gt; (and then into &amp;lt;foo&amp;gt; ; )
* we are cleansing a value before writing it to the database. instead we 
   should allow any value written to the database and then just cleanse it on display.

-------------

Setting a full name or discord name to (see attached) causes the value to 
not be accepted.  The form is submitted and the old values are shown
munroebot commented 4 months ago

Hi @mrjones-plip. I've updated the dev site with some of the changes you suggested:

  1. Full Name now detects white space only and fails form validation (and prevents submission)
  2. Made the Submit button become disabled and changed the text to "Updating..." when a user clicks it and the form has passed all validation.

I didn't do anything about javascript injection or escaping of characters. This data is being written to Stripe meta data and I expect them to handle things.

Please review when you have time. Thanks!

munroebot commented 1 month ago

Merged without feedback