Fixed missing totp parameter #86
Conversation
WalkthroughThe changes modify the Changes
Sequence Diagram(s)sequenceDiagram
participant S as steam_deploy.sh
participant C as steamcmd
S->>S: Check steam_totp validity
alt If steam_totp is valid
S->>C: Execute steamcmd with Steam Guard code
else
S->>C: Execute steamcmd without Steam Guard code
end
C-->>S: Return login result
S->>S: Log error details if login fails
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
steam_deploy.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
steam_deploy.sh
[warning] 167-167: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
[warning] 169-169: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
steam_deploy.sh
Outdated
| steam_login_args="+login "$steam_username"" | ||
| if [ "$steam_totp" != "INVALID" ]; then | ||
| steam_login_args="+set_steam_guard_code "$steam_totp" $steam_login_args" | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix quoting issues in dynamic login argument construction.
The concatenation of strings for steam_login_args on lines 167 and 169 is problematic. The embedded double quotes are not escaped, which causes the shell to unquote parts of the command and can lead to syntax errors or unexpected behavior. For example, the assignment in line 167:
steam_login_args="+login "$steam_username""should properly escape the inner quotes. A suggested fix is:
- steam_login_args="+login "$steam_username""
+ steam_login_args="+login \"$steam_username\""Similarly, on line 169:
steam_login_args="+set_steam_guard_code "$steam_totp" $steam_login_args"should be updated to:
- steam_login_args="+set_steam_guard_code "$steam_totp" $steam_login_args"
+ steam_login_args="+set_steam_guard_code \"$steam_totp\" $steam_login_args"This ensures that any spaces or special characters in the parameters are correctly managed and that the static analysis warnings (SC2027) are resolved.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 167-167: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
[warning] 169-169: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
There was a problem hiding this comment.
Since every other parameter is passed the same way in this script, this should be fine
There was a problem hiding this comment.
@JanWerder You're absolutely right. I've checked the file and confirmed that this parameter passing style (using "$variable" without escaping the quotes) is consistently used throughout the script, such as in the test login command on line 97. Maintaining consistency in coding style across the codebase is important. Thanks for the clarification!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
steam_deploy.sh (3)
135-160: Remove Obsolete Test Login CodeThese lines contain a commented-out test login block that is no longer active. To improve clarity and reduce clutter, consider removing these unused code segments if they aren’t intended for future reference or debugging.
167-215: TOTP Login Branch: Validate and Refactor Error HandlingThe updated TOTP branch now correctly includes the
+set_steam_guard_code "$steam_totp"parameter in the upload command. However, there are two points worth noting:
- Variable Assignment Check:
The static analysis warning (SC2154) indicates thatsteam_usernameis referenced but not assigned. Ensure that this variable is initialized (or provided via the environment) before usage to avoid runtime errors.- Error Logging Duplication:
The inline error-handling block within the command’s subshell is highly detailed but duplicated in the alternate branch. For maintainability and to avoid code duplication, consider extracting this error logging into a dedicated function, which can then be called from both branches.Example diff for extracting error logging into a function:
+log_errors() { + echo "" + echo "#################################" + echo "# Errors #" + echo "#################################" + echo "" + echo "Listing current folder and rootpath" + echo "" + ls -alh + echo "" + ls -alh "$rootPath" || true + echo "" + echo "Listing logs folder:" + echo "" + ls -Ralph "$steamdir/logs/" + for f in "$steamdir"/logs/*; do + if [ -e "$f" ]; then + echo "######## $f" + cat "$f" + echo + fi + done + echo "" + echo "Displaying error log" + echo "" + cat "$steamdir/logs/stderr.txt" + echo "" + echo "Displaying bootstrapper log" + echo "" + cat "$steamdir/logs/bootstrap_log.txt" + echo "" + echo "#################################" + echo "# Output #" + echo "#################################" + echo "" + ls -Ralph BuildOutput + for f in BuildOutput/*.log; do + echo "######## $f" + cat "$f" + echo + done + exit 1 +}Then, you could update the TOTP branch error handling like so:
- steamcmd +set_steam_guard_code "$steam_totp" +login "$steam_username" +run_app_build "$manifest_path" +quit || ( - ... # inline error logging block - exit 1 - ) + steamcmd +set_steam_guard_code "$steam_totp" +login "$steam_username" +run_app_build "$manifest_path" +quit || log_errorsPlease verify that this refactor aligns with your design intentions.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 169-169: steam_username is referenced but not assigned.
(SC2154)
216-264: Standard Login Branch: Refactor Duplicate Error HandlingIn the else branch for the standard login (lines 216–264), the error-handling code is essentially a duplicate of the one used in the TOTP branch. For better maintainability, consider refactoring this block into the same
log_errorsfunction suggested earlier. This will ensure consistency and simplify any future modifications to error reporting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
steam_deploy.sh(2 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
steam_deploy.sh
[warning] 169-169: steam_username is referenced but not assigned.
(SC2154)
🔇 Additional comments (1)
steam_deploy.sh (1)
267-267: Ensure Robust Output RedirectionThe final line appends the manifest path to the
$GITHUB_OUTPUTfile. Consider quoting the variable in the redirection to protect against potential word splitting or unexpected behavior if the variable contains spaces.Example diff:
-echo "manifest=${manifest_path}" >> $GITHUB_OUTPUT +echo "manifest=${manifest_path}" >> "$GITHUB_OUTPUT"
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
steam_deploy.sh (2)
170-218: TOTP Login Block: Correct Integration of TOTP with Room for Refactoring
This section improves the upload process by incorporating the TOTP parameter via+set_steam_guard_code "$steam_totp"whensteam_totpis valid. The detailed error logging ensures issues are traceable on failure. However, the error-handling logic here is nearly identical to that in the standard login branch (lines 219-268). Consider refactoring the duplicated error-handling routines into a common helper function to adhere to DRY principles and ease future maintenance.
219-268: Standard Login Block: Consistency and DRY Opportunity
In this branch the login proceeds without the TOTP requirement, as expected. The error-handling approach (with extensive logging and exit on failure) replicates the structure used when TOTP is provided. Moving these common error handling routines to a shared function could reduce duplication and simplify future modifications.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
steam_deploy.sh(3 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
steam_deploy.sh
[warning] 143-143: steam_username is referenced but not assigned.
(SC2154)
🔇 Additional comments (2)
steam_deploy.sh (2)
135-162: Test Login Block: Validate Behavior and Environment Variables
This block triggers a test login whensteam_totpequals"INVALID". Ensure that using the"INVALID"placeholder is intentional for test scenarios. Also, note thatsteam_usernameis referenced in the login command but is not explicitly assigned within this script; please verify that it is reliably set in the environment to avoid runtime errors.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 143-143: steam_username is referenced but not assigned.
(SC2154)
270-270: Manifest Output: Verify Environment Dependency
The command appending the manifest path to$GITHUB_OUTPUTassumes that this environment variable is set. Please verify that$GITHUB_OUTPUTis defined in the execution context to prevent potential issues in downstream processes.
|
I've had a closer look and my inital commit didn't do the job proper. The problem seems to be that the verifying the TOTP with a test login takes too longl, so that the code cycles once the real upload step comes around. I've mitigated that by excluding the test-login step for TOTP. |
When using TOTP, a password is required to login. While steamcmd is perfectly fine with accepting the password as part of the username input (separated with a space), the way this action is set up, the second steamcmd invocation where the upload happens would then also use the password, causing steamcmd to error because the password was provided without the TOTP code. Rather than use the TOTP code there again (as proposed in game-ci#86), add the password in the test step (which will cache the credentials), and retain it from the upload step. In the configVdf case, the password will be empty, and steamcmd will ignore it, keeping the previous mechanics.
Changes
steam-deploy supports totp, but the script only checks for the test login. The actual upload command then doesn't utilize the neccessary +set_steam_guard_code parameter. This change adds that.
Checklist
Summary by CodeRabbit