Skip to content

Fix newline handling in JSON data for keyring#4

Open
antoniogfs wants to merge 3 commits into
ProtonVPN:stablefrom
antoniogfs:stable
Open

Fix newline handling in JSON data for keyring#4
antoniogfs wants to merge 3 commits into
ProtonVPN:stablefrom
antoniogfs:stable

Conversation

@antoniogfs
Copy link
Copy Markdown

This patch fixes a critical issue where the keyring parser fails to properly handle certificate data from ProtonVPN that contains newline characters. The fix escapes literal newlines in JSON-serialized data before storing it in the system keyring, ensuring the data can be reliably parsed back on retrieval.


What This Patch Solves

The Problem:
ProtonVPN certificates embedded in keyring data contain actual newline characters that break the keyring's ability to parse the stored JSON. When the system attempts to retrieve and deserialize this data, the JSON decoder fails because the literal newlines in the certificate strings corrupt the JSON structure, causing a JSONDecodeError that cascades into a KeyError.

The Solution:
The patch modifies the _set_item() method in proton/keyring_linux/core/keyring_linux.py to escape all literal newline characters (\n) to their escaped string representation (\\n) before storing the JSON in the keyring. This ensures:

  1. Data Integrity: Certificate strings with newlines are safely stored without corrupting the JSON structure
  2. Reliable Parsing: When data is retrieved via _get_item(), the JSON decoder successfully deserializes it without errors
  3. Backward Compatibility: The escaping happens at storage time, making it transparent to the caller

Technical Details

Aspect Details
Files Changed 1 file: proton/keyring_linux/core/keyring_linux.py
Lines Modified Lines 84-86 in the _set_item() method
Change Type Bug fix (data serialization)
Commits 1 commit
Diff Summary +2 lines, -1 line

The Code Change:

# Before (line 84):
def _set_item(self, key, value):
    json_data = json.dumps(value)
    
# After (lines 84-86):
def _set_item(self, key, value):
    value = json.dumps(value).replace("\n", "\\n")
    json_data = value

The fix intercepts the JSON string immediately after serialization and replaces all unescaped newlines with their escaped equivalents before passing the data to the keyring backend.


Why This Matters

Impact Scope: This bug affects any ProtonVPN credential or certificate data that contains multiline content (e.g., X.509 certificates commonly have newlines for readability). Without this fix:

  • ❌ Credentials cannot be stored in the keyring
  • ❌ Stored credentials cannot be retrieved
  • ❌ The application fails to authenticate

With This Patch:

  • ✅ ProtonVPN certificates with newlines are stored correctly
  • ✅ Data is reliably retrieved without JSON parsing errors
  • ✅ Users can seamlessly use ProtonVPN credentials on Linux systems

Risk Assessment

Risk Level: LOW

  • The change is minimal and surgical—only affects JSON serialization at storage time
  • No API changes or breaking modifications
  • The escaping is transparent to callers
  • Existing data retrieval logic (_get_item()) remains unchanged

This is a stable, focused bug fix with high confidence in its correctness.

@antoniogfs
Copy link
Copy Markdown
Author

I just realized that although this solution works, there is an important caveat to consider.

The keyring is also accessible by other applications, which may suffer from the same issue, potentially making this fix ineffective.

A more robust approach would be to Base64-encode the certificate values instead

Update storage format to base64 encode JSON data for keyring.
@antoniogfs
Copy link
Copy Markdown
Author

The solution provided, although working, should be considered a temporary workaround.

A better and more stable approach would be to Base64-encode only the certificate value, but that would need to be implemented elsewhere.

Decode stored data from base64 before loading JSON.
@jllaneras
Copy link
Copy Markdown
Contributor

jllaneras commented May 22, 2026

hi @antoniogfs , why do you say the following?

A better and more stable approach would be to Base64-encode only the certificate value

I think storing all keyring entries encoded in base64 is more stable because it guarantees new lines will never end up in keyring entries, to avoid triggering the gnome-keyring bug. The main downside is that it makes debugging the keyring a bit more complicated but it's something we can deal with.

However, many users will have entries in the keyring that were not encoded to base64 and this would break the keyring for those users. We can't introduce a breaking change. We could introduce a base64: prefix in entries to then know which ones were encoded and which ones were stored without encoding. Remember this bug only manifests when the gnome keyring is stored unencrypted.

@antoniogfs
Copy link
Copy Markdown
Author

Hi @jllaneras,

IMHO, this does not change much from a debugging perspective. The certificate is already in PEM format, so encoding it in Base64 does not really make it less readable. In any case, it still needs to be handled programmatically.

Regarding the breaking change, I think you are partially right. However, I also believe the number of impacted users could be quite large. For example, users relying on other tools such as Chrome, which use the same keyring, may already be affected by this issue. Moreover, there may be possible workarounds that could preserve backward compatibility and support both cases.

So, the question seems more about finding the right trade-off between:

  • long-term, repeatedly frustrated users (like me), and
  • users who may be temporarily inconvenienced by a one-time breaking change.

Of course, you probably have much more context and information than I do, so I’ll leave the difficult decision to you. I just wanted to contribute to the discussion and hopefully help.

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.

2 participants