Add length check for Docker Registry authentication strings #480
Conversation
|
/review |
|
|
Preparing review... |
Code Review Analysis
Code Review FeedbackGeneral suggestions: The PR effectively addresses the need for validating Docker Registry authentication strings, which is crucial for maintaining security and adherence to Docker's standards. It's important to ensure that error messages are clear and actionable, providing users with a clear understanding of the constraints. Additionally, considering the future maintainability of the code, it might be beneficial to externalize string length limits and regex patterns to a configuration file or constants, making it easier to update these values if Docker's requirements change.
User guidelines:
|
| @@ -28,6 +28,10 @@ var composeImagesRemoveAll = dockercompose.ImageRemoveAll | |||
| var composePull = dockercompose.Pull | |||
| var composePullWithFile = dockercompose.PullWithFile | |||
|
|
|||
| // Maximum lengths as defined by Docker | |||
| var dockerRegistryUserNameLengthLimit = 30 | |||
There was a problem hiding this comment.
$\color{#FFA500}Improvements: Code Maintainability$
Consider externalizing dockerRegistryUserNameLengthLimit and dockerRegistryServerNameLengthLimit to a configuration file or at least to a more central location within the codebase. This would make it easier to update these values without having to dive deep into the business logic code.
| func is_username_registry_safe(username string, serverName string) bool { | ||
| func isRegistryCredentialsSafe(username string, serverName string) error { | ||
| if len(username) > dockerRegistryUserNameLengthLimit { | ||
| return fmt.Errorf("error: docker registry username can not exceed %d characters", dockerRegistryUserNameLengthLimit) |
There was a problem hiding this comment.
$\color{#FFA500}Improvements: Code Maintainability$
The error messages returned from isRegistryCredentialsSafe could be more descriptive by including the invalid input value. This would help users quickly identify and correct the issue.
| func TestReturnErrorUserNameTooLong(t *testing.T) { | ||
| expected := errors.New("error: docker registry username can not exceed 30 characters") | ||
| actual := isRegistryCredentialsSafe("usernameusernameusernameusername", "servername") | ||
| if actual.Error() != expected.Error() { |
There was a problem hiding this comment.
$\color{#FFA500}Improvements: Code Maintainability$
In your tests, instead of comparing error strings, consider using errors.Is for a more robust error comparison. This approach is less fragile and more idiomatic.
| func TestReturnNilRegistrySafe(t *testing.T) { | ||
| assert.Equal(t, isRegistryCredentialsSafe("username", "servername"), nil)} | ||
|
|
||
| func TestReturnErrorUserNameTooLong(t *testing.T) { |
There was a problem hiding this comment.
$\color{#FFD700}Coding Style$
The test function names could be more descriptive to reflect the specific scenario being tested, such as TestRegistryCredentialsSafe_UserNameTooLong instead of TestReturnErrorUserNameTooLong. This provides clearer intent and readability.
| func (compose *ComposeInfo) List(instanceName string) { | ||
| if err := composeList(util.ExecCommandWrap{}, instanceName); err != nil { | ||
| osExit(1) | ||
| } | ||
| } | ||
|
|
||
| // Check if docker username or docker registry strings are safe with good characters | ||
| func is_username_registry_safe(username string, serverName string) bool { | ||
| func isRegistryCredentialsSafe(username string, serverName string) error { |
There was a problem hiding this comment.
$\color{#FFA500}Improvements: Code Robustness$
For the isRegistryCredentialsSafe function, consider returning a custom error type that includes fields for the specific validation failure (e.g., ErrLengthExceeded, ErrInvalidCharacters). This would allow for more nuanced error handling and user feedback.
| if !is_username_registry_safe(username, serverName) { | ||
| fmt.Fprintf(os.Stderr, "Error: No special characters allowed in username/registry. List of good characters include: [a-z], [A-Z], [0-9], . , - , _, :") | ||
| if err := isRegistryCredentialsSafe(username, serverName); err != nil { | ||
| fmt.Fprintf(os.Stderr, err.Error()) |
There was a problem hiding this comment.
$\color{#FFA500}Improvements: Code Refactoring$
The fmt.Fprintf(os.Stderr, err.Error()) pattern is used multiple times. Consider creating a utility function for error reporting to standardize error handling and make the code cleaner.
|
Preparing code suggestions... |
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
$\color{#FFA500}Improvements: Code Robustness$
Suggestion: Refactor the error handling to return errors instead of exiting the process directly. This allows for more granular error handling and recovery by the caller.
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| } | |
| } | |
| if err := composeDown(util.ExecCommandWrap{}, instanceName); err != nil { | |
| return err | |
| } | |
| return nil |
| func (compose *ComposeInfo) DownWithFile(instanceName string, fileName string) { | ||
| if err := composeDownWithFile(util.ExecCommandWrap{}, instanceName, fileName); err != nil { | ||
| osExit(1) | ||
| } |
There was a problem hiding this comment.
$\color{#FFA500}Improvements: Code Robustness$
Suggestion: Validate the instanceName and fileName parameters to ensure they are not empty before proceeding with operations, enhancing error handling and user feedback.
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| func (compose *ComposeInfo) DownWithFile(instanceName string, fileName string) { | |
| if err := composeDownWithFile(util.ExecCommandWrap{}, instanceName, fileName); err != nil { | |
| osExit(1) | |
| } | |
| func (compose *ComposeInfo) DownWithFile(instanceName string, fileName string) { | |
| if instanceName == "" || fileName == "" { | |
| return fmt.Errorf("instanceName and fileName cannot be empty") | |
| } | |
| if err := composeDownWithFile(util.ExecCommandWrap{}, instanceName, fileName); err != nil { | |
| return err | |
| } | |
| return nil | |
| } |
| if actual.Error() != expected.Error() { | ||
| t.Errorf("wrong error: %v", actual) | ||
| } |
There was a problem hiding this comment.
$\color{#FFA500}Improvements: Code Robustness$
Suggestion: Replace direct string comparison for errors with type assertions or specific error type checks where possible.
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| if actual.Error() != expected.Error() { | |
| t.Errorf("wrong error: %v", actual) | |
| } | |
| var err *MyCustomError | |
| if errors.As(actual, &err) { | |
| assert.Equal(t, expected.Error(), err.Error()) | |
| } else { | |
| t.Errorf("expected error of type MyCustomError, got %T", actual) | |
| } |
| expected := errors.New("error: docker registry username can not exceed 30 characters") | ||
| actual := isRegistryCredentialsSafe("usernameusernameusernameusername", "servername") | ||
| if actual.Error() != expected.Error() { | ||
| t.Errorf("wrong error: %v", actual) | ||
| } | ||
| } | ||
|
|
||
| func Test_is_username_registry_safe_fail(t *testing.T) { | ||
| result :=is_username_registry_safe("username*", "servername") | ||
| assert.Equal(t, false, result) | ||
|
|
||
| func TestReturnErrorServerNameTooLong(t *testing.T) { | ||
| expected := errors.New("error: docker registry servername can not exceed 253 characters") |
There was a problem hiding this comment.
$\color{#FFA500}Improvements: Code Maintainability$
Suggestion: Avoid using magic numbers directly in tests by defining them as constants or using descriptive variables.
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| expected := errors.New("error: docker registry username can not exceed 30 characters") | |
| actual := isRegistryCredentialsSafe("usernameusernameusernameusername", "servername") | |
| if actual.Error() != expected.Error() { | |
| t.Errorf("wrong error: %v", actual) | |
| } | |
| } | |
| func Test_is_username_registry_safe_fail(t *testing.T) { | |
| result :=is_username_registry_safe("username*", "servername") | |
| assert.Equal(t, false, result) | |
| func TestReturnErrorServerNameTooLong(t *testing.T) { | |
| expected := errors.New("error: docker registry servername can not exceed 253 characters") | |
| const maxUsernameLength = 30 | |
| const maxServerNameLength = 253 | |
| expectedUsernameError := fmt.Errorf("error: docker registry username can not exceed %d characters", maxUsernameLength) | |
| expectedServernameError := fmt.Errorf("error: docker registry servername can not exceed %d characters", maxServerNameLength) |
| assert.Equal(t, isRegistryCredentialsSafe("username", "servername"), nil)} | ||
|
|
||
| func TestReturnErrorUserNameTooLong(t *testing.T) { | ||
| expected := errors.New("error: docker registry username can not exceed 30 characters") |
There was a problem hiding this comment.
$\color{#FFD700}Coding Style$
Suggestion: Ensure consistent use of error messages, specifically regarding the character limit error message.
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| expected := errors.New("error: docker registry username can not exceed 30 characters") | |
| expected := errors.New("error: docker registry username cannot exceed 30 characters") |
| actual := isRegistryCredentialsSafe("username", | ||
| "servernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservername") |
There was a problem hiding this comment.
$\color{#FFA500}Improvements: Code Maintainability$
Suggestion: Implement a helper function for creating long strings to avoid hardcoding long strings directly in test cases.
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| actual := isRegistryCredentialsSafe("username", | |
| "servernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservernameservername") | |
| longServerName := strings.Repeat("servername", 26) // Creates a long server name for testing | |
| actual := isRegistryCredentialsSafe("username", longServerName) |
| if actual.Error() != expected.Error() { | ||
| t.Errorf("wrong error: %v", actual) | ||
| } |
There was a problem hiding this comment.
$\color{#FFA500}Improvements: Code Refactoring$
Suggestion: Refactor error comparison in tests to use assert.Error for consistency and clarity.
NOTE: Ensure that the suggested code is comprehensive. This serves as an example of the code you could use. The user should verify it after accepting the suggestion.
| if actual.Error() != expected.Error() { | |
| t.Errorf("wrong error: %v", actual) | |
| } | |
| assert.Error(t, actual, expected.Error()) |
|
The PR review is to check for sustainability and correctness. Sustainability is actually more business critical as correctness is largely tested into the code over time. Its useful to keep in mind that SW often outlives the HW it was written for and engineers move from job to job so it is critical that code developed for Intel be supportable across many years. It is up to the submitter and reviewer to look at the code from a perspective of what if we have to debug this 3 years from now after the author is no longer available and defect databases have been lost. Yes, that happens all the time when we are working with time scales of more than 2 years. When reviewing your code it is important to look at it from this perspective.
Author Mandatory (to be filled by PR Author/Submitter)
PULL DESCRIPTION
Provide a 1-2 line brief overview of the changes submitted through the Pull Request...
REFERENCES
Reference URL for issue tracking (JIRA/HSD/Github): <URL to be filled>
Note-1: Depending on complexity of code changes, use the suitable word for complexity: Low/Medium/High
Example: PR for Slim boot loader project with medium complexity can have the label as: ISDM_Medium
CODE MAINTAINABILITY
APPLICATION SPECIFIC
Maintainer Mandatory (to be filled by PR Reviewer/Approving Maintainer)
QUALITY CHECKS
CODE REVIEW IMPACT
Note P1/P2/P3/P4 denotes severity of defects found (Showstopper/High/Medium/Low) and xx denotes number of defects found
SECURITY CHECKS
Please check if your PR fulfills the following requirements:
Code must act as a teacher for future developers