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

Improve password handling (#1) #3

Merged

Conversation

EddMCambs
Copy link
Contributor

While testing for writing my documentation at the weekend, I got incredibly jumbled up in how passwords and their escaping were being handled, and how it kept breaking on my test cases. This meant I wasn't happy the documentation I wrote around that was correct. I came at it again fresh at lunch time today, and went through the code to work out what was happening, and found I'd massively over complicated matters, by going around things the wrong way, and mixing myself up.

I started this PR as a plain documentation update, to simplify where I'd gone wrong, but in doing so I spotted a way to reduce the amount of escaping needed to the bare minimum. I've added a new function to backup.sh called prepare_login_json, that constructs the json we're sending to the server, and writing it to a file. curl has the ability to read the data it is POST-ing from a file, which skips the need to manually create the json on the command line, with all the quote escaping that requires.

The function to create the json file should also be safe and not need any escaping, because first it uses printf to create the two keyvalue pairs we care about (loginMethod: password, and password: $ACTUAL_BUDGET_PASSWORD) concatenated with the NUL character. This is then piped to jq, which splits the strings on NULs, and then encodes each pair itself into valid json, that gets redirected to a temp file. Because NUL can never exist in a password, this always ends up with the full password being written, no matter what symbols are in it.

This is a little overkill, as writing it to a file without it needing to pass it on the command line would probably be easy enough to escape manually, but letting jq handle correctly encoding the password is much safer IMO, because as a dedicated json tool, it's got way more cover for edge cases than we could ever write.

The only other change was to fix a typo in include.sh, and remove an errant exclamation mark from the zero-length string test. That was breaking my build while testing, and doesn't appear to be present in the image on Dockerhub, so I'm not sure if something just didn't get built and pushed, but the code here works now, with all the test cases I could throw at it this evening.

* updates to allow passwords to work

* Update documentation now passwords are fixed.

* Fix typo

* Revert testing Dockerfile changes

* Fixed Typo

---------

Authored-by: Edd Miles <[email protected]>
@rodriguestiago0 rodriguestiago0 merged commit 7472623 into rodriguestiago0:main Feb 10, 2025
1 check failed
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.

3 participants