Fix case-sensitive section/URL label matching in config_helper (#99)#104
Merged
JillRegan merged 1 commit intoMay 22, 2026
Merged
Conversation
|
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
043be38 to
8dcd7f5
Compare
…word#99) Per @JillRegan's comment on 1Password#99: swap `name == strings.ToLower(s.Label)` for `strings.EqualFold(name, s.Label)` in `sectionIDForName`. While implementing, noticed the same case-sensitive comparison pattern in 4 other sites in the same file (`sectionLabelForName`, `urlPrimaryForName`, `urlLabelForName`, `urlURLForName`) and in the field-label comparison inside `setValuesForTag`. All five comparison sites now use `strings.EqualFold`. Adds table-driven unit tests for all five helpers plus an end-to-end test exercising mixed-case tags through `setValuesForTag`. `go test ./...`, `go test -race ./...`, `gofmt`, `go vet` all clean.
8dcd7f5 to
878d24d
Compare
JillRegan
approved these changes
May 22, 2026
JillRegan
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the fix on this @jasondillingham 🙌 Approved!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #99.
Per @JillRegan's comment on #99, the fix is to swap
name == strings.ToLower(s.Label)forstrings.EqualFold(name, s.Label)insectionIDForName. While implementing, I noticed the same case-sensitive comparison pattern in four other sites in the same file plus one inline comparison insetValuesForTag. They're all the same bug class — user-supplied struct tag values matched case-sensitively against 1Password-stored labels — so I included them all. Happy to split into a smaller PR if you'd prefer to land just the section helpers.Bug
In
connect/config_helper.go, comparisons of struct-tag values against 1Password labels lowercase only one side, so any mixed-case label silently fails to match:A user with the struct:
— and a section actually named
"Details"in 1Password — getsHost = ""silently. The reporter noticed this foropsection; the same class of bug exists foropurl(mirror direction) and foropfield(no lowercasing at all).Fix
All five comparison sites now use
strings.EqualFold(tag, label):strings.ToLowerwhich allocates a new string on each call)Tests
connect/config_helper_test.go— table-driven unit tests for all five helpers (sectionIDForName,sectionLabelForName,urlPrimaryForName,urlLabelForName,urlURLForName). Covers exact-case, lowercase, uppercase, mixed-case, no-match, nil, and empty cases. 32 sub-tests total.Test_restClient_loadStructFromItem_caseInsensitiveTagsinconnect/client_test.go— end-to-end test exercisingsetValuesForTagwithUPPERCASEand mixed-case tags against the existinggetComplexItemfixture (which uses lowercase labels). Catches the field-label fix insetValuesForTagthat the unit tests can't reach.Pre-flight checks
go test ./...— passgo test -race ./...— passgofmt -l ./...— cleango vet ./...— clean