Skip to content

bug(cli): preserve base64 padding in environment variable parsing #882

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

Merged
merged 4 commits into from
Jun 19, 2025

Conversation

vadim-kharin-codefresh
Copy link
Contributor

What

Why

Notes

pgaijin66 and others added 2 commits June 19, 2025 14:21
## Summary

This PR fixes a regression introduced when the Codefresh CLI switched
from `prepareKeyValueFromCLIEnvOption` to
`prepareKeyValueObjectsFromCLIEnvOption` for parsing CLI variables and
secrets. The new parser broke support for values containing `=`
characters, such as base64-encoded secrets. This update restores safe
parsing by preserving everything after the first `=` in key=value pairs.

## Description

While using the latest version of the Codefresh CLI (`:latest`), we
recently observed failures in pipelines using environment variables that
contain base64-encoded secrets. The errors surfaced as base64 decoding
failures, even though the same values decoded correctly outside the
pipeline.

With no changes made to our pipeline definitions or application code, we
traced the issue to a change in how the CLI parses secrets passed via
command-line flags.

When parsing `key=value` strings, the helper function
`prepareKeyValueObjectsFromCLIEnvOption` split the string on all `=`
characters and used only the second segment as the value. This caused
any additional `=` characters (common in base64 padding) to be
discarded, resulting in corrupted secrets.

## Root Cause

A recent CLI update replaced the original parser
`prepareKeyValueFromCLIEnvOption` with a new function
`prepareKeyValueObjectsFromCLIEnvOption` to handle secrets and variables
passed via CLI flags.

The **original function** correctly preserved the full value by
splitting only on the first `=`:

```js
const key = vars.split('=', 1)[0];
const value = vars.split('=').slice(1).join('=');
```

The new function, however, introduced a bug by splitting on all =
characters and using only the second token:

```
const fields = vars.split('=');
const key = fields[0];
const value = fields[1];
```

This logic causes values to be truncated if they contain additional =
characters, as is common with base64-encoded secrets (which often end in
= or == for padding).

```
# Input
SECRET=YWJjZA==   # base64 for "abcd"

# Incorrect parsing
key: SECRET
value: YWJjZA    #  padding lost

# Expected
key: SECRET
value: YWJjZA==   #  full value preserved

```

## Relevant code diff
```
- const variables = prepareKeyValueFromCLIEnvOption(this.argv.variable);
+ const variables = prepareKeyValueObjectsFromCLIEnvOption(this.argv.variable);
+ const secrets = prepareKeyValueObjectsFromCLIEnvOption(this.argv.secret).map((secret) => ({
+   ...secret, encrypted: true
+ }));
...
- request.options.variables = variables;
+ request.options.variables = variables.concat(secrets);
```

This change introduced the regression by altering how key=value pairs
are parsed, causing silent corruption of any values containing multiple
= characters.

## Fix

The updated logic:

1. Splits only on the first =
2. Preserves all remaining characters (including =) in the value
3. Avoids corrupting secrets that use encoding formats requiring padding

## Impact 

1. Prevents silent corruption of base64-encoded secret values.
4. Restores pipeline reliability when using secrets in key=value CLI
input.
5. Aligns behavior with the more robust prepareKeyValueFromCLIEnvOption
function.
6. Adds defensive validation against malformed or partial inputs.
@vadim-kharin-codefresh vadim-kharin-codefresh merged commit 822dd03 into master Jun 19, 2025
1 check passed
@vadim-kharin-codefresh vadim-kharin-codefresh deleted the CR-28628-base64-padding-fix branch June 19, 2025 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants