Conversation
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.
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GitHubAction
participant DockerContainer
participant SteamCMD
User->>GitHubAction: Provide username, totp, (optional) password
GitHubAction->>DockerContainer: Set env vars (username, totp, password)
DockerContainer->>SteamCMD: Run steamcmd +set_steam_guard_code +login username password
SteamCMD-->>DockerContainer: Authentication result
DockerContainer-->>GitHubAction: Continue deployment flow
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
🧹 Nitpick comments (1)
README.md (1)
84-87: Add explicit guidance on storing the password securelyThe new
passwordinput is inherently sensitive but the docs don’t explicitly call out that it must be provided via a GitHub Secret (similar to the many other secret parameters mentioned). Consider adding a short sentence such as “Store this value in a GitHub Secret (e.g.STEAM_PASSWORD) and reference it via${{ secrets.STEAM_PASSWORD }}.”
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)action.yml(2 hunks)steam_deploy.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
steam_deploy.sh
[warning] 141-141: steam_username is referenced but not assigned.
(SC2154)
[warning] 141-141: steam_password is referenced but not assigned.
(SC2154)
🔇 Additional comments (2)
action.yml (2)
9-12: Enforce “password required whentotpis set” at action levelDeclaring
passwordasrequired: falseis fine, but users can still providetotpwithoutpassword, leading to a runtime failure.
Consider adding a lightweight validation step (composite run or early check insteam_deploy.sh) that aborts with a clear message whentotpis supplied andpasswordis empty.
100-101: Consistent env-var naming conventions
steam_username/steam_totpuse snake-case, so doessteam_password– great. Just flagging that sticking to this pattern avoids surprises in the shell script; no action needed.
| steamcmd +set_steam_guard_code "$steam_totp" +login "$steam_username" "$steam_password" +quit; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid passing an empty string as the password & silence set -u pitfalls
With set -u enabled, "$steam_password" is fine if the variable is defined (empty is okay) – but SteamCMD treats +login user "" differently from +login user.
A safer pattern is to only append the password argument when it is non-empty:
-if [ -n "$steam_password" ]; then
- steamcmd +set_steam_guard_code "$steam_totp" +login "$steam_username" "$steam_password" +quit
-else
- steamcmd +set_steam_guard_code "$steam_totp" +login "$steam_username" +quit
-fi
+if [ -n "$steam_password" ]; then
+ steamcmd +set_steam_guard_code "$steam_totp" +login "$steam_username" "$steam_password" +quit
+else
+ steamcmd +set_steam_guard_code "$steam_totp" +login "$steam_username" +quit
+fiThis prevents accidental “blank-password” logins and keeps the script robust even if callers omit the new input.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 141-141: steam_username is referenced but not assigned.
(SC2154)
[warning] 141-141: steam_password is referenced but not assigned.
(SC2154)
🤖 Prompt for AI Agents
In steam_deploy.sh around lines 141 to 142, the script currently passes the
password argument to steamcmd even if it is an empty string, which SteamCMD
treats differently than omitting the password. To fix this, modify the script to
conditionally append the password argument only if the variable is non-empty,
ensuring no empty password is passed. This will prevent accidental
blank-password logins and avoid issues with set -u when the variable is
undefined.
|
Thanks, I just spent a few hours wondering about this as well and ended up here with the same conclusion. Baffling how this isn't mentioned anywhere... |
davidmfinol
left a comment
davidmfinol
left a comment
There was a problem hiding this comment.
I somehow didn't see this PR when it was first raised, but this approach looks good to me!
Apologies for the late reply, and thanks for the contribution!
|
Looks like the |
|
Updated and released in https://github.com/game-ci/steam-deploy/releases/tag/v3.2.0 |
3 participants
Changes
Unless I am missing something, when using TOTP, a password is required to login. It could be that both steamcmd and this action are missing some details in the documentation, but I can find no way to login to steamcmd with only a username and a TOTP code.
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 #86), add the password in the test step (which will cache the credentials), and withhold it from the upload step.
In the configVdf case, the password will be empty, and steamcmd will ignore it, keeping the previous mechanics.
This is a much simpler solution than #86 to the same problem.
Checklist
Summary by CodeRabbit