Skip to content

Add function to process the passcmd for API key retrieval. #866

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add function to process the passcmd for API key retrieval. #866

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion zulip/zulip/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
import os
import platform
import random
import shlex
import subprocess
import sys
import time
import traceback
Expand Down Expand Up @@ -366,12 +368,15 @@ class MissingURLError(ZulipError):
class UnrecoverableNetworkError(ZulipError):
pass

class APIKeyRetrievalError(ZulipError):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: This seems used only for the passcmd case, not all API key retrieval? If so, the name could be improved.

pass

class Client:
def __init__(
self,
email: Optional[str] = None,
api_key: Optional[str] = None,
passcmd: Optional[str] = None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll defer to Tim as to if we'll want a passcmd option to the initializer.

However, with the current code below, the behavior appears incorrect, if we intend to support this behavior.

config_file: Optional[str] = None,
verbose: bool = False,
retry_on_errors: bool = True,
Expand Down Expand Up @@ -424,8 +429,29 @@ def __init__(
config = ConfigParser()
with open(config_file) as f:
config.read_file(f, config_file)
if api_key is None:
if api_key is None and config.has_option("api", "key"):
api_key = config.get("api", "key")
if passcmd is None and config.has_option("api", "passcmd"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for later review: I'm unsure what behavior we want here, ie. whether we should allow passcmd to override key in the config file.

Copy link
Author

@Gopinath-Mahendiran Gopinath-Mahendiran Apr 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have a look at this zulip/zulip-terminal#1581 (comment) ,If either passcmd or api is present in the configuration file, the application will use whichever is available. This line specifically checks whether passcmd is present in the configuration.

passcmd = config.get("api", "passcmd")
malicious_chars = {";", "|", "&", ">", "<", "`", "$", "\\", "\n", "\r"}
if any(char in passcmd for char in malicious_chars):
raise APIKeyRetrievalError(
f"Invalid characters detected in passcmd: {passcmd!r}"
)
Comment on lines +436 to +440
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the basis for this approach? Do you have a reference?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If passcmd contains any of these characters, it may lead to command injection, potentially resulting in unexpected behavior.

try:
cmd_parts = shlex.split(passcmd)
except ValueError as err:
raise APIKeyRetrievalError(
f"Failed to parse passcmd '{passcmd}': {err!s}"
) from err
try:
result = subprocess.run(cmd_parts, capture_output=True, check=True)
api_key = result.stdout.decode().strip()
except subprocess.CalledProcessError as err:
raise APIKeyRetrievalError(
f'Failed to retrieve API key using passcmd "{passcmd}".'
f"Command exited with return code {err.returncode}."
Comment on lines +452 to +453
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This formatting looks strange and long.

Copy link
Author

@Gopinath-Mahendiran Gopinath-Mahendiran Apr 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the first block handles parsing the passcmd using shlex.split(), raising an APIKeyRetrievalError if parsing fails.

  • It then runs the parsed command securely using subprocess.run() with error checking.
  • If the command fails (non-zero return code), a descriptive APIKeyRetrievalError is raised with details.

) from err
if email is None:
email = config.get("api", "email")
if site is None and config.has_option("api", "site"):
Expand Down
Loading