-
Notifications
You must be signed in to change notification settings - Fork 178
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
Trim spaces from SSID #728
base: master
Are you sure you want to change the base?
Conversation
Thanks for looking into this -- I have been wanting this one for a while! Curious, is it necessary to trim strings in C, or would just on the frontend be enough? |
LGTM |
@skot my usual motto/advice/ethos/whatever is that validation and sanitation have to happen on the backend API level. it's optional but suggested to also at least validate, if not also sanitize, values on the frontend as well. the frontend can be bypassed by just using the API without the frontend. that's why it's most important there. having it also occur in the browser is just good UX so things that happen on the backend are consistent with what the user sees when they submit the form. @WantClue I'll open a new PR to expand this sanitation to the the other form fields once this is merged in. unless you'd prefer it just be done in this pr. |
I agree that sanitizing inputs on the backend is a good idea. I think in this case though, we're just cleaning up a UI/Autocorrect quirk. If a user submits a SSID with a trailing space over the API, we should prolly assume they meant it, and allow it. |
i would usually consider this an anti-pattern. however, if we wanna support that case, that's fine. i can take that part out of the api portion and leave in the frontend trimming. that's a smaller piece of code anyways. |
Frontend trimming should be the only goal here. Still it might be confusing to the user. |
2b150fc
to
e340953
Compare
ok. i've updated the PR to only trim on the frontend. |
I noticed a while ago that the string inputs are not trimmed of spaces. This can cause, in my use-case, issues with the SSID not matching when typing the SSID in manually from a phone when the word Bitcoin is auto-completed. This PR simply trims the SSID input on the API and client side. I only did this for the SSID for now to simplify the initial PR, since this concept could, and perhaps should, be expanded to the other string inputs. This expansion would probably also help with #565.