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

[ENHANCEMENT] Allow for whitespace-trailing passwords (#2873) #2954

Merged
merged 2 commits into from
Oct 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions pkg/gopass/secrets/akv.go
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ func ParseAKV(in []byte) *AKV {

// handle the password that must be in the very first line
if first {
a.password = strings.TrimSpace(line)
a.password = line
first = false

continue
Expand All @@ -260,15 +260,13 @@ func ParseAKV(in []byte) *AKV {
continue
}

line = strings.TrimSpace(line)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The components of the line were going to be trimmed anyway.


key, val, found := strings.Cut(line, kvSep)
if !found {
continue
}

key = strings.TrimSpace(key)
val = strings.TrimSpace(val)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I add a more detailed comment of why val wasn't Trimmed? It would seem arbitrary otherwise.

Do you normally reference GitHub issues on comments?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the dead code and add a reference to the GH Issue, please.

// val is not Trimmed. See https://github.com/gopasspw/gopass/issues/2873
// we only store lower case keys for KV
a.kvp[key] = append(a.kvp[key], val)
}
Expand Down
69 changes: 69 additions & 0 deletions pkg/gopass/secrets/akv_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,75 @@ func TestSetKeyValuePairToEmptyAKV(t *testing.T) {
assert.Equal(t, "bar", v)
}

func TestAKVTrailingWhitespace(t *testing.T) {
t.Parallel()
// Expected behaviour is KEY: VALUE, with one space.
// Fallback should exist for KEY:VALUE, with no spaces.
mlValue := `foobar
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the issue initially was about passwords surrounded by spaces, no just key-values pairs, no?
How does that work for passwords now? A test would be nice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, I think the issue was about whitespace in passwords.
Being able to handle them in KV pairs is very nice as well, but either we need to change the description of the PR to match the implementation or add support for whitespace in passwords as well.

Copy link
Contributor Author

@lhardt lhardt Oct 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I had a misunderstanding between KV pairs and the password itself. But it should also be simple to not Trim the password. But I also see no harm in leaving values trimmed as well. Do you mind if I do both on the same PR?

defaultBehaviour: cd
sorroundedBySpace: cd
withoutSpace:cd`
s := ParseAKV([]byte(mlValue))
assert.NotNil(t, s)
v1, _ := s.Get("defaultBehaviour")
assert.Equal(t, "cd", v1)
v2, _ := s.Get("sorroundedBySpace")
assert.Equal(t, " cd \t ", v2)
v3, _ := s.Get("defaultBehaviour")
assert.Equal(t, "cd", v3)
}

func TestAKVPasswordWhitespace(t *testing.T) {
t.Parallel()

helloIsWorldStr := "\nhello: world\n"
helloIsWorldPair := map[string][]string{
"hello": {"world"},
}

for _, tc := range []struct {
name string
in string
pw string
kvp map[string][]string
}{
{
name: "justpassword",
in: `this is a password.` + helloIsWorldStr,
pw: "this is a password.",
kvp: helloIsWorldPair,
},
{
name: "spaceonly",
in: " " + helloIsWorldStr,
pw: " ",
kvp: helloIsWorldPair,
},
{
name: "tab",
in: "\t tab padded password \t" + helloIsWorldStr,
pw: "\t tab padded password \t",
kvp: helloIsWorldPair,
},
} {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()

a := ParseAKV([]byte(tc.in))

assert.Equal(t, tc.pw, a.password, tc.name)
for k, vs := range tc.kvp {
sort.Strings(vs)
gvs := a.kvp[k]
sort.Strings(gvs)
assert.Equal(t, vs, gvs, k)
}

assert.Equal(t, tc.in, string(a.Bytes()), tc.name)
})
}
}

func TestParseAKV(t *testing.T) {
t.Parallel()

Expand Down
Loading