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 incorrect salt when partially staged files are commited #119

Merged
merged 2 commits into from
Apr 26, 2021

Conversation

Aramgutang
Copy link
Contributor

See ticket #118.

@jmurty
Copy link
Collaborator

jmurty commented Mar 31, 2021

@Aramgutang Can you think of any way we could add unit test coverage for partial commits of encrypted files?

Since git-p refuses to work with transcrypt-encrypted files it might be tough to simulate the behaviour.

@Aramgutang
Copy link
Contributor Author

Yeah, I read through the git add/apply/am documentation, and couldn't find anything useable. Basically the scenario to reproduce requires invoking the clean filter for a file, while feeding it input that isn't actually the contents of the file, all within the context of generating a blob object for the change.

The various GUIs probably have their own implementations of this, which is why some seem to be able to handle staging partial changes on encrypted files, and some aren't.

@Aramgutang
Copy link
Contributor Author

Aramgutang commented Mar 31, 2021

I read through the code for git-cola, and it appears that the way it stages hunks/lines is by generating patch files, writing them to a tmpfile, and passing them to git apply:

https://github.com/git-cola/git-cola/blob/b65aced9c089183eb008cd8c276b3c1941ee1f87/cola/diffparse.py#L261
https://github.com/git-cola/git-cola/blob/ac7e533541b46f7de5e778132be11227639ba0a7/cola/cmds.py#L287

But then again, staging partial changes to encrypted files in git-cola doesn't actually work, and it's probably because git apply doesn't pass the content through the filters, and neither does git-cola before writing the patchfile (which was the code I was hoping to find).

I reckon the only viable way to approach it would be along the lines of:

  • Commit encrypted file.txt with content "AA"
  • Save its encrypted content for reference later
  • Modify file.txt to contain "BB"
  • Call the clean filter script directly, passing "BA" into its stdin, and "file.txt" as the filename argument
  • Generate a patch file using the received output and the encrypted content of the committed version we saved from before
  • git apply the patch to the index, but not the working tree (using --cached)
  • Modify file.txt to contain "BA"
  • Verify that git status lists file.txt as having unstaged changes (prefix MM when run with -s), but git diff outputs nothing
  • With the fix in place, git status should not list file.txt as having unstaged changes (prefix M ), and the diff should still be empty

Seems like a lot of work...

@jmurty
Copy link
Collaborator

jmurty commented Mar 31, 2021

Whew, yeah, definitely a lot of work. For this one, knowing that the change hasn’t broken anything already tested (most features) is probably good enough.

Thanks for the explanation @Aramgutang

@jmurty jmurty merged commit 588f1bc into elasticdog:master Apr 26, 2021
@jmurty
Copy link
Collaborator

jmurty commented Apr 26, 2021

Thanks for this fix @Aramgutang!

I tested it manually with VSCode pre- and post-fix and found the behaviour exactly as you described.

It's weird that VSCode allows partial commits when other tools refuse to, but that's a pretty important Git "client" to work with.

jmurty added a commit that referenced this pull request Apr 26, 2021
* master:
  Fix incorrect salt when partially staged files are commited (#119)
  Use shorthand for grep options for broader compatibility (#121)
jmurty added a commit that referenced this pull request Jan 15, 2022
* main:
  Improve command hint to fix secret files not encrypted in index (#120) (#130)
  Remove Ubuntu 16.04 LTS from test matrix (#123)
  Configure default Git branch name for macOS tests in GitHub
  Handle rename of primary branch from "master" to "main"
  Ensure Git index is up-to-date before dirty repo  check #37 (#109)
  Fix incorrect salt when partially staged files are commited (#119)
  Use shorthand for grep options for broader compatibility (#121)
Erotemic added a commit to Erotemic/transcrypt that referenced this pull request Jul 11, 2022
jmurty added a commit that referenced this pull request Oct 15, 2022
# By James Murty (18) and others
# Via GitHub (1) and James Murty (1)
* main: (26 commits)
  Centralise load and save of password into functions #141
  Fix date of 2.2.0 release
  Ensure tests use "main" as default branch name #143
  Use OpenSSL for B64 encoding not `base64` which differs between Linux and Mac #140
  Use core attributesFile from worktree (#137)
  Document `xxd` requirement, and make optional with OpenSSL < 3 (#138)
  Prepare for 2.2.0 release
  Fix when using OpenSSL 3 which no longer embeds salt in output (#135)
  Consolidate all git operation scripts into a single transcrypt script
  Fix handling of small files and files with null in first 8 bytes (#116)
  Improve command hint to fix secret files not encrypted in index (#120) (#130)
  Remove Ubuntu 16.04 LTS from test matrix (#123)
  Configure default Git branch name for macOS tests in GitHub
  Handle rename of primary branch from "master" to "main"
  Ensure Git index is up-to-date before dirty repo  check #37 (#109)
  Fix incorrect salt when partially staged files are commited (#119)
  Use shorthand for grep options for broader compatibility (#121)
  Let user set a custom path to openssl #108
  Install entire transcrypt script into repository
  Change version to indicate development "pre-release" status
  ...

# Conflicts:
#	README.md
#	tests/_test_helper.bash
#	tests/test_cleanup.bats
#	tests/test_crypt.bats
#	tests/test_init.bats
#	tests/test_not_inited.bats
#	transcrypt
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