Security Fix: Prevent OS Command Injection in Passbolt adapter (CWE-78)#1697
Merged
djmb merged 4 commits intobasecamp:mainfrom Nov 11, 2025
Merged
Security Fix: Prevent OS Command Injection in Passbolt adapter (CWE-78)#1697djmb merged 4 commits intobasecamp:mainfrom
djmb merged 4 commits intobasecamp:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR enhances security in the Passbolt adapter by adding proper shell escaping to folder IDs when constructing passbolt CLI commands. However, the changes introduce several code issues including duplicate lines, inconsistent indentation, and a reference to an undefined variable.
- Added
.to_s.shellescapeto folder IDs in the passbolt command for improved security - Introduced duplicate filter_condition assignment and inconsistent indentation
- Removed the
missing_secretsvariable definition without updating its usage
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| items = `passbolt list resources #{filter_condition} #{folders.map { |item| "--folder #{item["id"].to_s.shellescape}" }.join(" ")} --json` | ||
| raise RuntimeError, "Could not read #{secrets} from Passbolt" unless $?.success? | ||
| items = JSON.parse(items) | ||
| found_names = items.map { |item| item["name"] } |
There was a problem hiding this comment.
The variable missing_secrets is referenced on line 57 but is no longer defined. The line that calculated missing_secrets = secret_names - found_names was removed. Add this line back between lines 56 and 57.
Suggested change
| found_names = items.map { |item| item["name"] } | |
| found_names = items.map { |item| item["name"] } | |
| missing_secrets = secret_names - found_names |
| items = `passbolt list resources #{filter_condition} #{folders.map { |item| "--folder #{item["id"].to_s.shellescape}" }.join(" ")} --json` | ||
| raise RuntimeError, "Could not read #{secrets} from Passbolt" unless $?.success? | ||
| items = JSON.parse(items) | ||
| found_names = items.map { |item| item["name"] } |
There was a problem hiding this comment.
This assignment to found_names is useless, since its value is never read.
Suggested change
| found_names = items.map { |item| item["name"] } |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
djmb
reviewed
Nov 7, 2025
|
|
||
| items = JSON.parse(items) | ||
| found_names = items.map { |item| item["name"] } | ||
| missing_secrets = secret_names - found_names |
Contributor
Author
|
You can keep it but I would suggest you make it not vurenable
Donal McBreen ***@***.***> schrieb am Fr. 7. Nov. 2025 um
15:22:
… ***@***.**** commented on this pull request.
------------------------------
In lib/kamal/secrets/adapters/passbolt.rb
<#1697 (comment)>:
> items = JSON.parse(items)
found_names = items.map { |item| item["name"] }
- missing_secrets = secret_names - found_names
We need to keep this line.
—
Reply to this email directly, view it on GitHub
<#1697 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BX7NNO6LNLXYD5DXG277JET33STLJAVCNFSM6AAAAACLE2IAYKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTIMZUGI2TEOJUGU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi team,
This PR fixes a critical OS Command Injection vulnerability in the
Passboltadapter.The
item["id"]variable was interpolated directly into a shell command without escaping. An attacker who can control folder metadata in Passbolt could escalate this to RCE on the machine runningkamal secrets fetch.This patch adds the missing
.to_s.shellescapeto sanitize the input, fixing the vulnerability.