search

kevinschaich / mintable

🍃 Automate your personal finances – for free, with no ads, and no data collection.
https://kevinschaich.io/mintable/
MIT License
1.54k stars 204 forks source link

401 error caused by whitespace in Google Client ID / Secret input #42

Closed blimmer closed 4 years ago

blimmer commented 4 years ago

During the setup process, I kept getting a 401 error when trying to connect my Google Sheet. After further inspection, I noticed that the issue was due to whitespace from the copy/paste from the linked QuickStart page. When I removed the whitespace, everything worked. I'd be happy to provide a PR to improve this UX.

I was thinking about a few ways of modifying ConfigPropertyInputGroup. We could:

  1. Always disallow leading/trailing whitespace. This could be unnecessarily limiting for some configuration properties where we'd want users to be able to. However, it would be pretty simple to implement.
  2. Introduce an allowWhitespace passed as configuration. This could be more backward compatible and we could introduce it per-property (defaulting to false). For instance, I'm thinking we could configure it here, adding an allowWhitespace: false parameter to this config hash: https://github.com/kevinschaich/mintable/blob/52a8a2f0a8af4fd49c348a046afc22ddebb2c927/src/pages/sheet-provider-setup.jsx#L16-L19

Alternatively, we could sanitize the input on the backend. I was leaning away from this strategy because the issue I experienced is going to primarily happen when dealing with front-end user input, but that's a path forward as well.

What are your thoughts?

PS: Thank you for this project! I've only been using it for a few days and it has already been incredibly useful.

kevinschaich commented 4 years ago

Hey @blimmer – assume you were configuring this in the front end GUI? Should be easy enough to trim these inputs and reduce the possibility someone else encounters this.

I don't see the allowWhitespace config being a commonly used feature and I'd pitch for just disallowing it – but if people complain about it I'd be happy to add it in.

blimmer commented 4 years ago

Hey @kevinschaich - yep, exactly. I was configuring it via the front-end GUI. My gut reaction was always to trim leading and trailing whitespace, but since I'm a new user, I didn't know if there were use cases where it would cause people trouble.

I'd be happy to put something together in the next week or so as time permits. Thanks again!

kevinschaich commented 4 years ago

Sounds good @blimmer – a good place to do this might be in lib/common.js

https://github.com/kevinschaich/mintable/blob/52a8a2f0a8af4fd49c348a046afc22ddebb2c927/src/lib/common.js#L104-L111

That way you won't have to sanitize each input on the frontend.