Skip to content

Fix env file quote stripping for mixed-quote values#138

Merged
dcellison merged 1 commit intomainfrom
fix/env-quote-stripping
Mar 23, 2026
Merged

Fix env file quote stripping for mixed-quote values#138
dcellison merged 1 commit intomainfrom
fix/env-quote-stripping

Conversation

@dcellison
Copy link
Copy Markdown
Owner

Summary

  • Replace .strip("\"'") with matched-pair quote detection in both env file parsing paths
  • str.strip(chars) treats its argument as a set of characters, removing any from both ends independently - so KEY='he said "hello"' loses the trailing " because it is in the strip set, yielding a corrupted value
  • New _strip_quotes() helper only strips when the first and last characters are the same quote type
  • Applied to both parse_env_file() and the protected env parser in load_config()
  • The python-dotenv code path already handles this correctly; only the manual fallback parsers were affected

Test plan

  • _strip_quotes unit tests: matched double, matched single, mismatched, no quotes, empty string, single char quote, empty quoted string, inner quotes preserved (8 tests)
  • parse_env_file integration tests: single quotes containing double, double containing single, mismatched not stripped, unquoted unchanged, single quote char unchanged (5 tests)
  • Existing test_quoted_values (simple "hello" and 'world') still passes
  • All 1062 tests pass, make check clean

Fixes #131

Replace .strip("\"'") with matched-pair quote detection in both
parse_env_file() and the protected env parser in load_config().

str.strip(chars) removes any character in the set from both ends
independently, so KEY='he said "hello"' would lose the trailing
double quote, yielding a corrupted value. The new _strip_quotes()
helper only strips when the first and last characters are the same
quote type.

Fixes #131
@dcellison
Copy link
Copy Markdown
Owner Author

Review by Kai

PR Review: Fix env file quote stripping for mixed-quote values

Overall: Clean fix. No bugs, no security issues. Minor suggestions only.


Logic / Correctness

The _strip_quotes() helper (config.py:321-330) is correct. The len(value) >= 2 guard prevents index errors on empty and single-character inputs, and the value[0] == value[-1] check enforces matched pairs. All edge cases are handled.

The fix is applied consistently to both manual parse paths (parse_env_file line 364, load_config line 723), which is the right scope per the PR description.


Security

No concerns. If anything, the old behavior was the mild hazard — silently corrupting credential values containing embedded quotes.


Edge cases not covered (suggestions only)

Suggestion — Escape sequences inside quoted values (e.g. KEY="it's a \"test\"") are not handled — backslashes will be preserved literally. This is not a regression (the old code didn't handle it either), but it's a known limitation worth a comment, since the docstring says "inner quotes of the opposite type are preserved" without mentioning same-type escaped quotes.

Suggestion_strip_quotes is now part of the public test surface (_strip_quotes is imported in test_workspace_config.py). Since it's a private helper (underscore prefix), that's fine, but it could be noted in the docstring that it's intentionally exported for testing.


Style

Tests are well-named and cover the decision boundaries cleanly. The TestStripQuotes unit tests and TestParseEnvFile integration tests together give good confidence. No style issues.


Verdict: Approve. The fix is minimal, correct, and the test coverage is thorough.

@dcellison dcellison merged commit c0d549d into main Mar 23, 2026
1 check passed
@dcellison dcellison deleted the fix/env-quote-stripping branch March 23, 2026 22:19
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.

parse_env_file quote stripping corrupts mixed-quote values

2 participants