Open sedkis opened 1 month ago
⏱️ Estimated effort to review [1-5] | 3 |
🧪 Relevant tests | Yes |
🔒 Security concerns | No |
⚡ Key issues to review |
Error Handling Consistency: The error message in mw_jwt.go for the RSAPSS check could be more consistent with other error messages in the same function. Consider using a similar format for clarity and uniformity.
|
API Changes
no api changes detected
Category | Suggestion | Score |
Possible bug |
Add nil check for token.Header to prevent runtime errors___ **To avoid potential nil pointer dereference, add a nil check fortoken.Header before accessing its "alg" key in the error message formatting.** [gateway/mw_jwt.go [845]](https://github.com/TykTechnologies/tyk/pull/6368/files#diff-e8bce0f6790c8c56b30e24dbeebb0fc4aa0879ab5ea5f6ef6dbe68931410e237R845-R845) ```diff +if token.Header == nil { + return fmt.Errorf("token header is nil") +} return fmt.Errorf("%v: %v and not RSA or RSAPSS signature", UnexpectedSigningMethod, token.Header["alg"]) ``` Suggestion importance[1-10]: 9Why: Adding a nil check for `token.Header` prevents potential runtime errors, which is crucial for code reliability and stability. | 9 |
Maintainability |
Consolidate conditional checks into a single statement for clarity___ **To improve the readability and maintainability of the JWT signing method assertion,consider consolidating the checks for jwt.SigningMethodRSA and jwt.SigningMethodRSAPSS into a single conditional statement using logical OR.** [gateway/mw_jwt.go [843-845]](https://github.com/TykTechnologies/tyk/pull/6368/files#diff-e8bce0f6790c8c56b30e24dbeebb0fc4aa0879ab5ea5f6ef6dbe68931410e237R843-R845) ```diff -if _, ok := token.Method.(*jwt.SigningMethodRSA); !ok { - if _, ok := token.Method.(*jwt.SigningMethodRSAPSS); !ok { - return fmt.Errorf("%v: %v and not RSA or RSAPSS signature", UnexpectedSigningMethod, token.Header["alg"]) - } +if _, ok := token.Method.(*jwt.SigningMethodRSA); !ok && _, ok := token.Method.(*jwt.SigningMethodRSAPSS); !ok { + return fmt.Errorf("%v: %v and not RSA or RSAPSS signature", UnexpectedSigningMethod, token.Header["alg"]) } ``` Suggestion importance[1-10]: 8Why: Consolidating the checks into a single conditional statement improves code readability and maintainability without altering functionality. | 8 |
Enhancement |
Add a test case for RSAPSS signing method___ **To improve the test coverage and ensure the RSAPSS signing method is correctly handled,add a specific test case for RSAPSS similar to the existing RS256 test.** [gateway/mw_jwt_test.go [1628-1657]](https://github.com/TykTechnologies/tyk/pull/6368/files#diff-406bf8fdb6c0cc77f04c6245c70abfc592ddb1525aa843200d850e14d135ebfcR1628-R1657) ```diff -func TestAssertRS256JWT(t *testing.T) { +func TestAssertRSAPSSJWT(t *testing.T) { ... } ``` Suggestion importance[1-10]: 8Why: Adding a specific test case for RSAPSS improves test coverage and ensures that the RSAPSS signing method is correctly handled. | 8 |
Add assertions to check JWT contents in tests___ **To ensure the robustness of the JWT parsing tests, add assertions to verify the contentsof the parsed JWT, such as checking the claims or the header, instead of only checking for errors.** [gateway/mw_jwt_test.go [1614-1625]](https://github.com/TykTechnologies/tyk/pull/6368/files#diff-406bf8fdb6c0cc77f04c6245c70abfc592ddb1525aa843200d850e14d135ebfcR1614-R1625) ```diff -_, err := parser.Parse(rawJWT, func(token *jwt.Token) (interface{}, error) { +token, err := parser.Parse(rawJWT, func(token *jwt.Token) (interface{}, error) { if err := assertSigningMethod(signingMethod, token); err != nil { return nil, err } return parseJWTKey(signingMethod, pubKey) }) if err != nil { t.Error("Could not validate RS256 key") } +assert.Equal(t, "John Doe", token.Claims.(jwt.MapClaims)["name"]) ``` Suggestion importance[1-10]: 7Why: Adding assertions to verify the contents of the parsed JWT enhances test robustness and ensures the JWT is parsed correctly. | 7 |
Issues
0 New issues
0 Accepted issues
Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code
Adding support for the more secure RSA-PSS signed JWTS.
Description
allows for the use of the RSA-PSS signature algorithm commonly referred to as PS256, PS384, PS512. The change is invisible to existing RSA Public Keyuse cases. Simply - by using "RSA Public Key" signing algorithm, Tyk will now validate JWTs signed by both RS & PS Class algorithms using Public Keys.
Motivation and Context
RSA-PSS is considered more secure than PKCS1 v1.5 due to its probabilistic nature, which helps mitigate certain attacks (e.g., padding oracle attacks).
RS256: Commonly used in legacy systems, JWT (JSON Web Tokens), and many existing protocols where backward compatibility is important. PS256: Recommended for new applications where higher security is desired. It is becoming more widely adopted in modern security protocols.
How This Has Been Tested
Unit tests have been added. Both positive + negative tests that test both RS class JWTs and PS class JWTs.
Checklist