Repository: lestrrat-go/jwx
Pull request #1017 (2023-11-18 04:49:14)

Remove signer instance upon call to jws.UnregisterSigner

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/lestrrat-go/jwx/pull/1017?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=lestrrat-go) Report
Attention: `3 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`0a15b4d`)](https://app.codecov.io/gh/lestrrat-go/jwx/commit/0a15b4d9a180b06c6924c4a03b39ae70d05514f4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=lestrrat-go) 72.00% compared to head [(`12ae40f`)](https://app.codecov.io/gh/lestrrat-go/jwx/pull/1017?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=lestrrat-go) 71.99%.
> Report is 3 commits behind head on develop/v2.

| [Files](https://app.codecov.io/gh/lestrrat-go/jwx/pull/1017?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=lestrrat-go) | Patch % | Lines |
|---|---|---|
| [jwe/jwe.go](https://app.codecov.io/gh/lestrrat-go/jwx/pull/1017?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=lestrrat-go#diff-andlL2p3ZS5nbw==) | 85.00% | [2 Missing and 1 partial :warning: ](https://app.codecov.io/gh/lestrrat-go/jwx/pull/1017?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=lestrrat-go) |

<details><summary>Additional details and impacted files</summary>


```diff
@@              Coverage Diff               @@
##           develop/v2    #1017      +/-   ##
==============================================
- Coverage       72.00%   71.99%   -0.01%     
==============================================
  Files              93       89       -4     
  Lines           13795    13801       +6     
==============================================
+ Hits             9933     9936       +3     
- Misses           3044     3047       +3     
  Partials          818      818              
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/lestrrat-go/jwx/pull/1017?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=lestrrat-go).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=lestrrat-go).


Patch:
diff --git a/Changes b/Changes
index 26dabe5c..14d26bd7 100644
--- a/Changes
+++ b/Changes
@@ -5,6 +5,10 @@ v2 has many incompatibilities with v1. To see the full list of differences betwe
 v1 and v2, please read the Changes-v2.md file (https://github.com/lestrrat-go/jwx/blob/develop/v2/Changes-v2.md)
 
 v2.0.17 UNRELEASED
+[Bug Fixes]
+  * [jws] Previously, `jws.UnregisterSigner` did not remove the previous signer instance when
+    the signer was registered and unregistered multiple times. This has been fixed.
+
 [New Features]
   * [jwe] (EXPERIMENTAL) `jwe.WithCEK` has been added to extract the content encryption key (CEK) from the Decrypt operation.
   * [jwe] (EXPERIMENTAL) `jwe.EncryptStatic` has been added to encrypt content using a static CEK.
diff --git a/jws/jws.go b/jws/jws.go
index a348c618..4cf2c317 100644
--- a/jws/jws.go
+++ b/jws/jws.go
@@ -74,6 +74,12 @@ func (s *payloadSigner) PublicHeader() Headers {
 var signers = make(map[jwa.SignatureAlgorithm]Signer)
 var muSigner = &sync.Mutex{}
 
+func removeSigner(alg jwa.SignatureAlgorithm) {
+	muSigner.Lock()
+	defer muSigner.Unlock()
+	delete(signers, alg)
+}
+
 func makeSigner(alg jwa.SignatureAlgorithm, key interface{}, public, protected Headers) (*payloadSigner, error) {
 	muSigner.Lock()
 	signer, ok := signers[alg]
diff --git a/jws/signer.go b/jws/signer.go
index 44c8bfb7..39da7286 100644
--- a/jws/signer.go
+++ b/jws/signer.go
@@ -34,6 +34,9 @@ func RegisterSigner(alg jwa.SignatureAlgorithm, f SignerFactory) {
 	muSignerDB.Lock()
 	signerDB[alg] = f
 	muSignerDB.Unlock()
+
+	// Remove previous signer, if there was one
+	removeSigner(alg)
 }
 
 // UnregisterSigner removes the signer factory associated with
@@ -49,6 +52,8 @@ func UnregisterSigner(alg jwa.SignatureAlgorithm) {
 	muSignerDB.Lock()
 	delete(signerDB, alg)
 	muSignerDB.Unlock()
+	// Remove previous signer
+	removeSigner(alg)
 }
 
 func init() {


Test patch:
diff --git a/jws/jws_test.go b/jws/jws_test.go
index bf29b5a8..d0a31105 100644
--- a/jws/jws_test.go
+++ b/jws/jws_test.go
@@ -2044,6 +2044,19 @@ func TestGH910(t *testing.T) {
 	require.NoError(t, err, `jws.Verify should succeed`)
 
 	require.Equal(t, src, string(verified), `verified payload should match`)
+
+	jws.UnregisterSigner(sha256Algo)
+
+	// Now try after unregistering the signer for the algorithm
+	_, err = jws.Sign([]byte(src), jws.WithKey(sha256Algo, nil))
+	require.Error(t, err, `jws.Sign should succeed`)
+
+	jws.RegisterSigner(sha256Algo, jws.SignerFactoryFn(func() (jws.Signer, error) {
+		return s256SignerVerifier{}, nil
+	}))
+
+	_, err = jws.Sign([]byte(src), jws.WithKey(sha256Algo, nil))
+	require.NoError(t, err, `jws.Sign should succeed`)
 }
 
 func TestUnpaddedSignatureR(t *testing.T) {