Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix wrong target for submit buttons in js s3file.js #228

Merged
merged 1 commit into from
Aug 30, 2022
Merged

Fix wrong target for submit buttons in js s3file.js #228

merged 1 commit into from
Aug 30, 2022

Conversation

anneFly
Copy link
Contributor

@anneFly anneFly commented Aug 30, 2022

Hey @codingjoe 👋
I believe I found a small bug inside the js part of this library.

Description:

Inside the s3file.js there is an event handler for the click event on all submit buttons:

var submitButtons = form.querySelectorAll('input[type=submit], button[type=submit]')
Array.from(submitButtons).forEach(function (submitButton) {
submitButton.addEventListener('click', clickSubmit)
})

When handling this event, the code is trying to copy the name and value form the clicked submit button to a new element. However, the code is using event.target:
var submitButton = e.target

This is a problem because the clicked submit button might have child elements like spans or images. If the user's mouse pointer is on a child element of the button, then event.target will point to that child element. And since this child most likely does not have a name attribute, the new input element will have the name undefined and the data that gets posted to the server will contain undefined=1 instead of the name of the actual submit button.

How to reproduce:

On the main branch you can reproduce it with a submit button that has a child and then you need to click on that child.
E.g. here is a button with a 10 pixels image.

<button type="submit" name="asdf">
  <img src="data:image/jpeg;base64,/9j/4AAQSkZJRgABAQEBLAEsAAD//gATQ3JlYXRlZCB3aXRoIEdJTVD/4gKwSUNDX1BST0ZJTEUAAQEAAAKgbGNtcwQwAABtbnRyUkdCIFhZWiAH5gAIAB4ACQAgAAFhY3NwQVBQTAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA9tYAAQAAAADTLWxjbXMAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA1kZXNjAAABIAAAAEBjcHJ0AAABYAAAADZ3dHB0AAABmAAAABRjaGFkAAABrAAAACxyWFlaAAAB2AAAABRiWFlaAAAB7AAAABRnWFlaAAACAAAAABRyVFJDAAACFAAAACBnVFJDAAACFAAAACBiVFJDAAACFAAAACBjaHJtAAACNAAAACRkbW5kAAACWAAAACRkbWRkAAACfAAAACRtbHVjAAAAAAAAAAEAAAAMZW5VUwAAACQAAAAcAEcASQBNAFAAIABiAHUAaQBsAHQALQBpAG4AIABzAFIARwBCbWx1YwAAAAAAAAABAAAADGVuVVMAAAAaAAAAHABQAHUAYgBsAGkAYwAgAEQAbwBtAGEAaQBuAABYWVogAAAAAAAA9tYAAQAAAADTLXNmMzIAAAAAAAEMQgAABd7///MlAAAHkwAA/ZD///uh///9ogAAA9wAAMBuWFlaIAAAAAAAAG+gAAA49QAAA5BYWVogAAAAAAAAJJ8AAA+EAAC2xFhZWiAAAAAAAABilwAAt4cAABjZcGFyYQAAAAAAAwAAAAJmZgAA8qcAAA1ZAAAT0AAACltjaHJtAAAAAAADAAAAAKPXAABUfAAATM0AAJmaAAAmZwAAD1xtbHVjAAAAAAAAAAEAAAAMZW5VUwAAAAgAAAAcAEcASQBNAFBtbHVjAAAAAAAAAAEAAAAMZW5VUwAAAAgAAAAcAHMAUgBHAEL/2wBDAAMCAgMCAgMDAwMEAwMEBQgFBQQEBQoHBwYIDAoMDAsKCwsNDhIQDQ4RDgsLEBYQERMUFRUVDA8XGBYUGBIUFRT/2wBDAQMEBAUEBQkFBQkUDQsNFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBT/wgARCAAKAAoDAREAAhEBAxEB/8QAFQABAQAAAAAAAAAAAAAAAAAAAAj/xAAUAQEAAAAAAAAAAAAAAAAAAAAA/9oADAMBAAIQAxAAAAGVAAf/xAAUEAEAAAAAAAAAAAAAAAAAAAAg/9oACAEBAAEFAh//xAAUEQEAAAAAAAAAAAAAAAAAAAAg/9oACAEDAQE/AR//xAAUEQEAAAAAAAAAAAAAAAAAAAAg/9oACAECAQE/AR//xAAUEAEAAAAAAAAAAAAAAAAAAAAg/9oACAEBAAY/Ah//xAAUEAEAAAAAAAAAAAAAAAAAAAAg/9oACAEBAAE/IR//2gAMAwEAAgADAAAAEJJP/8QAFBEBAAAAAAAAAAAAAAAAAAAAIP/aAAgBAwEBPxAf/8QAFBEBAAAAAAAAAAAAAAAAAAAAIP/aAAgBAgEBPxAf/8QAFBABAAAAAAAAAAAAAAAAAAAAIP/aAAgBAQABPxAf/9k=" />
</button>

If you click directly on that button, you'll see that the data that gets posted to the server will contain undefined=1

Proposed solution in this PR:

I've changed event.target to event.currentTarget. That should always give you the button and not one of its children.
If you try to reproduce the bug on this branch, you should see the expected result asdf=1 in the request data instead of undefined=1

@codecov
Copy link

codecov bot commented Aug 30, 2022

Codecov Report

Merging #228 (07fd7d0) into main (94ee141) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #228   +/-   ##
=======================================
  Coverage   98.02%   98.02%           
=======================================
  Files           8        8           
  Lines         203      203           
=======================================
  Hits          199      199           
  Misses          4        4           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@codingjoe
Copy link
Owner

codingjoe commented Aug 30, 2022

Hi @anneFly,

Long time no see. I hope you are well. Thanks for spotting the error and the fix. That reminds me that this package is overdue for some JS tests.

@SebastianKapunkt would you be interested in setting up jest + coverage here?

Thanks again, I'll release the patch right away.

Best Johannes!

@codingjoe codingjoe merged commit b2f8577 into codingjoe:main Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants