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

fix: bash avoiding newline when Base64 encoding a long string #2437

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

VietND96
Copy link
Member

Thanks for contributing to the Docker-Selenium project!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines, applied for this repository.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Fixes #2435

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅

2435 - Fully compliant

Fully compliant requirements:

  • Add -w 0 option to base64 to override the max column
  • Fix the issue with longer passwords and usernames causing problems in the graphql health check
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Potential Compatibility Issue
The -w0 option for base64 might not be supported in all environments. Verify compatibility across different systems.

Possible Security Concern
Ensure that the SE_ROUTER_USERNAME and SE_ROUTER_PASSWORD environment variables are properly secured and not exposed in logs or to unauthorized users.

Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Best practice
Use printf instead of echo for more consistent handling of special characters

Consider using printf instead of echo for more consistent behavior across different
systems when dealing with special characters or escape sequences.

Base/check-grid.sh [8]

-BASIC_AUTH="$(echo -en "${SE_ROUTER_USERNAME}:${SE_ROUTER_PASSWORD}" | base64 -w0)"
+BASIC_AUTH="$(printf '%s:%s' "${SE_ROUTER_USERNAME}" "${SE_ROUTER_PASSWORD}" | base64 -w0)"
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7
Maintainability
Improve code organization by grouping variable declarations together

Move the BASIC_AUTH variable declaration to the beginning of the script, near other
variable declarations, to improve code organization and readability.

Video/video.sh [31-32]

-/opt/bin/validate_endpoint.sh "${NODE_STATUS_ENDPOINT}"
 BASIC_AUTH="$(echo -en "${SE_ROUTER_USERNAME}:${SE_ROUTER_PASSWORD}" | base64 -w0)"
 
+/opt/bin/validate_endpoint.sh "${NODE_STATUS_ENDPOINT}"
+
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7
Enhancement
Improve variable scope and reduce potential code duplication

Consider moving the BASIC_AUTH variable declaration outside the if block to avoid
potential repetition if the variable is used in multiple places.

charts/selenium-grid/configs/node/nodeProbe.sh [39-42]

+BASIC_AUTH="$(echo -en "${SE_ROUTER_USERNAME}:${SE_ROUTER_PASSWORD}" | base64 -w0)"
+
 return_list=($(bash ${NODE_CONFIG_DIRECTORY}/nodeGridUrl.sh))
 grid_url=${return_list[0]}
 grid_check=${return_list[1]}
-BASIC_AUTH="$(echo -en "${SE_ROUTER_USERNAME}:${SE_ROUTER_PASSWORD}" | base64 -w0)"
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7

💡 Need additional feedback ? start a PR chat

@VietND96 VietND96 merged commit ec01d2f into trunk Oct 22, 2024
53 checks passed
@VietND96 VietND96 deleted the base64-newline branch October 22, 2024 04:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: Base64 adds new line for username and password
1 participant