feat(env): add environment variable manager with encryption, templates, and CLI support #338
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR introduces a comprehensive environment variable management system for Cortex Linux, including a new EnvManager class with per-application isolation, optional encryption, templates, and validation rules. It integrates an "env" CLI command with subcommands (set, get, list, delete, export, import, template, clear), along with documentation, examples, tests, and dependency updates. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as CortexCLI
participant EM as EnvManager
participant Crypto as Encryption
participant FS as File System
participant OS as os.environ
User->>CLI: cortex env set app API_KEY secret123 --encrypt
activate CLI
CLI->>EM: set(app, API_KEY, secret123, encrypt=True)
activate EM
EM->>EM: validate(API_KEY, secret123)
alt Encryption Enabled
EM->>Crypto: encrypt(secret123)
activate Crypto
Crypto-->>EM: encrypted_value
deactivate Crypto
EM->>EM: create EnvVariable(encrypted=True)
else Plain Text
EM->>EM: create EnvVariable(encrypted=False)
end
EM->>FS: _save_app_env(app, variables)
activate FS
FS->>FS: write JSON to ~/.cortex/env/app.json (perms 600)
FS-->>EM: success
deactivate FS
EM-->>CLI: True
deactivate EM
CLI-->>User: ✓ Variable set
deactivate CLI
User->>CLI: cortex env get app API_KEY
activate CLI
CLI->>EM: get(app, API_KEY, decrypt=True)
activate EM
EM->>FS: _load_app_env(app)
activate FS
FS-->>EM: variables JSON
deactivate FS
EM->>EM: retrieve EnvVariable(encrypted=True)
alt Decryption Requested
EM->>Crypto: decrypt(encrypted_value)
activate Crypto
Crypto-->>EM: secret123
deactivate Crypto
end
EM-->>CLI: secret123
deactivate EM
CLI-->>User: secret123
deactivate CLI
User->>CLI: cortex env load app
activate CLI
CLI->>EM: load_env_to_os(app)
activate EM
EM->>EM: get_env_dict(app, decrypt=True)
EM->>OS: os.environ.update(env_dict)
EM-->>CLI: success
deactivate EM
CLI-->>User: ✓ Loaded to environment
deactivate CLI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (14)
.gitignore (1)
208-208: Consider using a pattern instead of a specific filename.Adding
myapp.envas a specific entry is unusual. The existing.envpatterns on lines 140 and 159 should already ignore files named.env, but they won't matchmyapp.env. If the intent is to ignore app-specific environment files generated by the EnvManager, consider using a more general pattern like*.env(if appropriate) or placing these files in a dedicated ignored directory.If this was added to ignore a test artifact, ensure it's cleaned up or the pattern is generalized.
cortex/cli.py (2)
811-813: Handle missing subcommand more gracefully.When users run
cortex envwithout a subcommand,args.env_actionisNone, which falls through to the else branch. The message "Unknown env command" is slightly misleading. Consider checking forNoneexplicitly and providing usage guidance.🔎 Proposed fix
+ elif args.env_action is None: + self._print_error("Please specify a subcommand. Use: set, get, list, delete, export, import, template, clear") + return 1 + else: self._print_error("Unknown env command. Use: set, get, list, delete, export, import, template, clear") return 1
1049-1054: Consider using nested subparsers for template commands.The current design uses
nargs="?"forappandtemplate_name, which are only required for theapplyaction. This works but makes the CLI interface less self-documenting. Users runningcortex env template --helpwon't see thatapplyrequires additional arguments.A cleaner approach would be to use nested subparsers for
template listandtemplate apply, but the current implementation with runtime validation (lines 760-762) is functional.tests/test_env_manager.py (2)
150-155: Clarify the test intent for None handling.The test includes
Nonein the test values but then skips it with a comment about type checks. If theRequiredValidator.validate()method expects astrandNonewould fail type checking, then including it in this test is misleading. Either test thatNoneraises aTypeErroror remove it from the test values.🔎 Proposed fix
# Invalid - for value in ["", " ", None]: - if value is None: - continue # Skip None as it would fail type check + for value in ["", " "]: is_valid, error = validator.validate("KEY", value) self.assertFalse(is_valid)
444-457: UseaddCleanuportry/finallyfor os.environ restoration.If an assertion fails before lines 456-457, the test environment variables will leak into
os.environfor subsequent tests. Consider usingself.addCleanup()ortry/finallyto ensure cleanup always runs.🔎 Proposed fix
def test_load_env_to_os(self): """Test loading environment to os.environ""" self.manager.set("myapp", "TEST_VAR_1", "test_value_1") self.manager.set("myapp", "TEST_VAR_2", "test_value_2") + + # Register cleanup before loading to ensure it runs even if assertions fail + self.addCleanup(lambda: os.environ.pop("TEST_VAR_1", None)) + self.addCleanup(lambda: os.environ.pop("TEST_VAR_2", None)) # Load to os.environ self.manager.load_env_to_os("myapp") self.assertEqual(os.environ.get("TEST_VAR_1"), "test_value_1") self.assertEqual(os.environ.get("TEST_VAR_2"), "test_value_2") - - # Clean up - del os.environ["TEST_VAR_1"] - del os.environ["TEST_VAR_2"]requirements.txt (1)
14-15: Consider upgrading to a more recent cryptography version constraint.The
cryptography>=41.0.0dependency is correct for Fernet encryption support inenv_manager.py, but the version constraint is outdated. The latest stable version is 46.0.3. Update the constraint tocryptography>=45.0.0or higher to ensure users receive security patches and recent improvements while maintaining compatibility.examples/env_demo.py (2)
7-19: Moveshutilimport to top-level with other imports.The
shutilmodule is imported inside each demo function (lines 64, 103, 145, 198, 235). Per PEP 8, imports should be at the top of the file.🔎 Proposed fix
import os import sys +import shutil import tempfile from pathlib import PathThen remove the
import shutilstatements from lines 64, 103, 145, 198, and 235.
23-24: Add type hints to function signatures.Per coding guidelines, type hints are required in Python code. All demo functions are missing return type annotations.
🔎 Proposed fix for all demo functions
-def demo_basic_operations(): +def demo_basic_operations() -> None: """Demonstrate basic environment variable operations"""Apply similar changes to
demo_templates,demo_import_export,demo_validation,demo_encryption, andmain.cortex/env_manager.py (6)
93-96: Consider usingfullmatch()instead ofmatch()for complete string validation.
match()only checks if the pattern matches at the beginning of the string. For validation purposes,fullmatch()ensures the entire value conforms to the pattern.🔎 Proposed fix
def validate(self, key: str, value: str) -> tuple[bool, Optional[str]]: - if self.pattern.match(value): + if self.pattern.fullmatch(value): return True, None return False, f"{key}: {self.error_message}"
229-232: Consider atomic file writes to prevent data corruption.Writing directly to the app file could result in partial writes or corruption if the process is interrupted. Using a temporary file with atomic rename is safer.
🔎 Proposed fix
def _save_app_env(self, app_name: str, env_vars: Dict[str, EnvVariable]): """Save environment variables for an application.""" app_file = self._get_app_file(app_name) data = {key: var.to_dict() for key, var in env_vars.items()} - with open(app_file, "w") as f: - json.dump(data, f, indent=2) - - app_file.chmod(0o600) + # Write to temp file first, then atomically rename + temp_file = app_file.with_suffix(".json.tmp") + with open(temp_file, "w") as f: + json.dump(data, f, indent=2) + temp_file.chmod(0o600) + temp_file.rename(app_file)
373-383: YAML export is naive and may produce invalid YAML for complex values.The hand-rolled YAML export only escapes double quotes. Values containing colons, special YAML characters, or multiline content may produce invalid YAML. Consider using the
yamllibrary or documenting this limitation.For a quick improvement without adding a dependency:
🔎 Proposed fix to document limitation
elif format == "yaml": + # Note: Simple YAML format - complex values may not serialize correctly lines = []
513-520: Add error handling for malformed template files.
list_templates()will raise an exception if any template JSON file is malformed, preventing enumeration of valid templates.🔎 Proposed fix
def list_templates(self) -> List[EnvTemplate]: """List all available templates.""" templates = [] for template_file in self.templates_dir.glob("*.json"): - with open(template_file) as f: - data = json.load(f) - templates.append(EnvTemplate.from_dict(data)) + try: + with open(template_file) as f: + data = json.load(f) + templates.append(EnvTemplate.from_dict(data)) + except (json.JSONDecodeError, TypeError, KeyError) as e: + # Skip malformed templates, optionally log warning + pass return templates
649-657: Consider documenting thatload_env_to_osoverwrites existing environment variables.The method updates
os.environdirectly, which will overwrite any existing variables with the same keys. This may be intentional, but documenting the behavior helps prevent surprises.🔎 Proposed docstring update
def load_env_to_os(self, app_name: str): """ Load environment variables into os.environ. + + Note: This will overwrite any existing environment variables + with the same keys. Args: app_name: Application name """
36-37: Replacedatetime.utcnow()withdatetime.now(timezone.utc)to address Python 3.12+ deprecation.datetime.utcnow() is deprecated in Python 3.12+ and the recommended way to create an object representing the current time in UTC is by calling datetime.now(timezone.utc).
Update the import:
-from datetime import datetime +from datetime import datetime, timezoneThen replace all
datetime.utcnow().isoformat()calls at lines 36, 37, 265, 418, and 457 withdatetime.now(timezone.utc).isoformat().
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.gitignorecortex/cli.pycortex/env_manager.pydocs/ENV_MANAGEMENT.mdexamples/env_demo.pyrequirements.txttests/test_env_manager.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/cli.pytests/test_env_manager.pyexamples/env_demo.pycortex/env_manager.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_env_manager.py
🧬 Code graph analysis (3)
cortex/cli.py (1)
cortex/env_manager.py (8)
EnvManager(132-674)get(288-307)delete(309-326)export(351-395)import_env(397-469)list_templates(513-520)apply_template(471-494)delete_app(659-674)
tests/test_env_manager.py (1)
cortex/env_manager.py (20)
EnvTemplate(51-67)EnvVariable(29-47)RegexValidator(86-96)RequiredValidator(120-129)URLValidator(99-117)to_dict(40-42)to_dict(60-62)from_dict(45-47)from_dict(65-67)validate(76-83)validate(93-96)validate(114-117)validate(126-129)validate(610-627)get(288-307)delete(309-326)get_env_dict(629-647)load_env_to_os(649-657)delete_app(659-674)_get_app_file(209-211)
examples/env_demo.py (1)
cortex/env_manager.py (5)
get(288-307)export(351-395)list_templates(513-520)apply_template(471-494)import_env(397-469)
🪛 markdownlint-cli2 (0.18.1)
docs/ENV_MANAGEMENT.md
42-42: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
107-107: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
478-478: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (8)
cortex/cli.py (2)
893-894: Help table entries are correctly added.The new
envandnotifycommands are properly documented in the help table, consistent with the existing command entries.
1098-1099: Command dispatch is correctly integrated.The
envcommand is properly wired into the main command dispatcher, following the established pattern.tests/test_env_manager.py (1)
1-26: Test suite provides good coverage for the EnvManager feature.The test file covers data models, validators, core EnvManager operations, encryption, templates, import/export, and an end-to-end integration workflow. The test isolation using temporary directories is properly implemented. As per coding guidelines, maintaining >80% coverage for PRs appears achievable with this suite.
docs/ENV_MANAGEMENT.md (1)
1-12: Documentation is comprehensive and well-structured.The ENV_MANAGEMENT.md provides thorough coverage of all features including CLI commands, templates, validation, encryption, and integration patterns. The examples are practical and cover common use cases (Django, Node.js, microservices, Docker).
examples/env_demo.py (1)
239-258: LGTM!The
main()function correctly orchestrates all demos with clear visual separation. The overall structure of the demo script effectively showcases the EnvManager capabilities.cortex/env_manager.py (3)
169-199: Key generation approach stores derived key directly—verify this meets security requirements.The implementation generates a key using PBKDF2 with a random salt and machine-id, then stores the derived Fernet key directly. This works correctly but differs from typical patterns where salt is stored and key is re-derived from a password. Since this is machine-local encryption without user password input, the approach is reasonable.
However, note that:
- If
/etc/machine-idis unavailable and random bytes are used, the key becomes non-reproducible (which is fine since the key is stored).- The comment on lines 175-177 suggests future user-provided password support which would require storing the salt.
596-608: URL and DATABASE patterns may cause redundant validation.A variable like
DATABASE_URLmatches both.*_URL$(URL validator) and.*DATABASE.*(Required validator). While not incorrect, this may cause confusion if validation messages overlap.The validation framework is well-designed. The port regex correctly validates the 1-65535 range, and the pattern-based rule system is flexible.
1-12: Well-structured module with comprehensive documentation.The module docstring clearly describes all features. The overall architecture with separate data classes, validators, and the main manager class provides good separation of concerns. Security practices like 0o600 file permissions are appropriately applied.
| try: | ||
| with open(args.file) as f: | ||
| env_data = f.read() |
There was a problem hiding this comment.
Specify encoding when reading the import file.
The file is opened without specifying encoding, which defaults to the system locale. This could cause issues with environment files containing non-ASCII characters on systems with different default encodings.
🔎 Proposed fix
try:
- with open(args.file) as f:
+ with open(args.file, encoding="utf-8") as f:
env_data = f.read()🤖 Prompt for AI Agents
In cortex/cli.py around lines 728-730, the import file is opened without an
explicit encoding which can break on non-ASCII content; change the open call to
specify an encoding (e.g., UTF-8) when reading the file (and optionally add
errors='strict' or errors='replace' if you want fallback behavior) so the
env_data read is deterministic across systems.
| else: # env format (default) | ||
| lines = [] | ||
| for key, var in env_vars.items(): | ||
| if var.encrypted: | ||
| value = "[encrypted]" | ||
| else: | ||
| value = var.value | ||
| # Escape quotes and special chars for shell | ||
| value = value.replace('"', '\\"') | ||
| lines.append(f'export {key}="{value}"') | ||
| return "\n".join(lines) |
There was a problem hiding this comment.
Shell export doesn't escape all special characters.
Values containing $, backticks, or newlines can cause shell injection or parsing issues. Consider escaping these or using single quotes.
🔎 Proposed fix
else: # env format (default)
lines = []
for key, var in env_vars.items():
if var.encrypted:
value = "[encrypted]"
else:
value = var.value
- # Escape quotes and special chars for shell
- value = value.replace('"', '\\"')
- lines.append(f'export {key}="{value}"')
+ # Use single quotes to prevent shell expansion
+ # Escape single quotes by ending quote, adding escaped quote, resuming
+ value = value.replace("'", "'\"'\"'")
+ lines.append(f"export {key}='{value}'")
return "\n".join(lines)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cortex/env_manager.py around lines 385-395, the current code only escapes
double quotes which leaves values with $, backticks, newlines or single quotes
vulnerable to shell parsing/injection; replace the manual replace with proper
shell quoting by using Python's shlex.quote (import shlex) to wrap the value for
safe shell export (e.g. export KEY=<shlex.quote(value)>), apply this for both
encrypted placeholder and real values, and ensure the returned lines join
remains the same.
| if format == "json": | ||
| data = json.loads(env_data) | ||
| for key, value in data.items(): | ||
| if value != "[encrypted]": | ||
| # Set directly to new env_vars | ||
| now = datetime.utcnow().isoformat() | ||
| env_vars[key] = EnvVariable( | ||
| key=key, | ||
| value=value, | ||
| encrypted=False, | ||
| description="", | ||
| created_at=now, | ||
| updated_at=now, | ||
| tags=[], | ||
| ) | ||
|
|
||
| elif format == "env": | ||
| for line in env_data.strip().split("\n"): | ||
| line = line.strip() | ||
| if not line or line.startswith("#"): | ||
| continue | ||
|
|
||
| # Parse export KEY="value" or KEY=value | ||
| if line.startswith("export "): | ||
| line = line[7:] # Remove "export " | ||
|
|
||
| if "=" not in line: | ||
| continue | ||
|
|
||
| key, _, value = line.partition("=") | ||
| key = key.strip() | ||
| value = value.strip() | ||
|
|
||
| # Remove quotes | ||
| if value.startswith('"') and value.endswith('"'): | ||
| value = value[1:-1] | ||
| elif value.startswith("'") and value.endswith("'"): | ||
| value = value[1:-1] | ||
|
|
||
| # Unescape | ||
| value = value.replace('\\"', '"') | ||
|
|
||
| if value != "[encrypted]": | ||
| # Set directly to new env_vars | ||
| now = datetime.utcnow().isoformat() | ||
| env_vars[key] = EnvVariable( | ||
| key=key, | ||
| value=value, | ||
| encrypted=False, | ||
| description="", | ||
| created_at=now, | ||
| updated_at=now, | ||
| tags=[], | ||
| ) |
There was a problem hiding this comment.
Imported variables bypass validation rules.
The import_env method creates EnvVariable objects directly without calling validate(). This allows importing values that would be rejected by set(), potentially introducing invalid data.
🔎 Proposed fix - validate during import
if value != "[encrypted]":
+ # Validate imported value
+ is_valid, error = self.validate(key, value)
+ if not is_valid:
+ # Skip invalid values or raise error
+ continue # or: raise ValueError(f"Import failed: {error}")
+
# Set directly to new env_vars
now = datetime.utcnow().isoformat()Apply this fix to both the JSON import block (around line 416) and the env format import block (around line 455).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cortex/env_manager.py around lines 413 to 466, imported variables are
instantiated directly which bypasses the model's validation; update both the
JSON import block (~416) and the env-format import block (~455) to validate each
value before inserting into env_vars by either (1) constructing the EnvVariable
then calling its validate() method and only adding it if validate passes
(raising or skipping/logging on validation errors), or (2) delegating to the
existing set() path that performs validation; preserve the existing
"[encrypted]" skip logic and ensure timestamps/tags are still set when
validation succeeds.
| ``` | ||
| 🔧 Environment variables for myapp: | ||
| DATABASE_URL: postgres://localhost/mydb | ||
| API_KEY: [encrypted] 🔐 | ||
| Production API key | ||
| NODE_ENV: production | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to code block.
Per static analysis (MD040), fenced code blocks should have a language specified. This output block could use text or plaintext as the language specifier.
🔎 Proposed fix
-```
+```text
🔧 Environment variables for myapp:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| 🔧 Environment variables for myapp: | |
| DATABASE_URL: postgres://localhost/mydb | |
| API_KEY: [encrypted] 🔐 | |
| Production API key | |
| NODE_ENV: production | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
42-42: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In docs/ENV_MANAGEMENT.md around lines 42 to 49, the fenced code block lacks a
language specifier which violates MD040; update the opening triple-backtick to
include a language such as text or plaintext (e.g., ```text) so the block
becomes fenced with the specified language and leave the block contents
unchanged.
| ``` | ||
| 📋 Available templates: | ||
| nodejs | ||
| Node.js application environment | ||
| Variables: NODE_ENV, PORT, DATABASE_URL, REDIS_URL, LOG_LEVEL | ||
| Required: DATABASE_URL | ||
| python | ||
| Python application environment | ||
| Variables: PYTHON_ENV, DATABASE_URL, REDIS_URL, LOG_LEVEL, DEBUG | ||
| Required: DATABASE_URL | ||
| django | ||
| Django application environment | ||
| Variables: DJANGO_SETTINGS_MODULE, SECRET_KEY, DEBUG, DATABASE_URL, ALLOWED_HOSTS | ||
| Required: SECRET_KEY, DATABASE_URL | ||
| docker | ||
| Docker environment variables | ||
| Variables: DOCKER_HOST, COMPOSE_PROJECT_NAME, COMPOSE_FILE | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to code block.
Per static analysis (MD040), this output example should have a language specifier like text.
🔎 Proposed fix
-```
+```text
📋 Available templates:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| 📋 Available templates: | |
| nodejs | |
| Node.js application environment | |
| Variables: NODE_ENV, PORT, DATABASE_URL, REDIS_URL, LOG_LEVEL | |
| Required: DATABASE_URL | |
| python | |
| Python application environment | |
| Variables: PYTHON_ENV, DATABASE_URL, REDIS_URL, LOG_LEVEL, DEBUG | |
| Required: DATABASE_URL | |
| django | |
| Django application environment | |
| Variables: DJANGO_SETTINGS_MODULE, SECRET_KEY, DEBUG, DATABASE_URL, ALLOWED_HOSTS | |
| Required: SECRET_KEY, DATABASE_URL | |
| docker | |
| Docker environment variables | |
| Variables: DOCKER_HOST, COMPOSE_PROJECT_NAME, COMPOSE_FILE | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
107-107: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In docs/ENV_MANAGEMENT.md around lines 107 to 128 the fenced code block showing
"Available templates" lacks a language specifier which triggers MD040; update
the opening triple-backtick to include a language (e.g., text) so the block
becomes ```text, leaving the block contents unchanged, then save the file.
| | `--encrypt` | Encrypt the value | | ||
| | `--description <text>` | Add description | | ||
| | `--tags <tag1> <tag2>` | Add tags | | ||
| | `--format <format>` | Export/import format (env, json, yaml) | |
There was a problem hiding this comment.
Documentation/code mismatch for import formats.
The options table shows --format supports "env, json, yaml" for both export and import. However, the CLI parser at cortex/cli.py line 1046 only allows ["env", "json"] for import:
import_parser.add_argument("--format", choices=["env", "json"], default="env", ...)Update the documentation to reflect that import only supports env and json formats, while export supports all three.
🔎 Proposed fix
| `--format <format>` | Export/import format (env, json, yaml) |
+
+Note: Import only supports `env` and `json` formats. Export supports `env`, `json`, and `yaml`.🤖 Prompt for AI Agents
In docs/ENV_MANAGEMENT.md around line 471 the table entry for `--format`
incorrectly states import supports "env, json, yaml"; update the documentation
to state import supports only "env, json" while export supports "env, json,
yaml". Edit the options table and any surrounding explanatory text to
distinguish import vs export formats, e.g., change the `--format` description to
indicate export: (env, json, yaml) and import: (env, json), and ensure no other
lines still claim YAML is supported for import.
| ``` | ||
| ~/.cortex/environments/ # Per-app environment files | ||
| ~/.cortex/env_templates/ # Template definitions | ||
| ~/.cortex/.env_key # Encryption key (600 permissions) | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to code block.
Per static analysis (MD040), this path listing should have a language specifier like text or plaintext.
🔎 Proposed fix
-```
+```text
~/.cortex/environments/ # Per-app environment files📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| ~/.cortex/environments/ # Per-app environment files | |
| ~/.cortex/env_templates/ # Template definitions | |
| ~/.cortex/.env_key # Encryption key (600 permissions) | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
478-478: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In docs/ENV_MANAGEMENT.md around lines 478-482, the fenced code block showing
paths lacks a language specifier (MD040); update the opening fence to include a
plain text language label (e.g., ```text or ```plaintext) so the block becomes a
text code block, leaving the contents unchanged and preserving indentation and
comments.
| encrypted = manager.get("app", "API_KEY", decrypt=False) | ||
| console.print(f" [dim]{encrypted[:50]}...[/dim]") |
There was a problem hiding this comment.
Handle potential None return from manager.get().
manager.get() returns Optional[str]. If the variable wasn't found, slicing encrypted[:50] would raise a TypeError. While unlikely in this controlled demo, adding a guard improves robustness.
🔎 Proposed fix
# Get encrypted (raw)
console.print("\n[bold]Encrypted value (base64)...[/bold]")
encrypted = manager.get("app", "API_KEY", decrypt=False)
- console.print(f" [dim]{encrypted[:50]}...[/dim]")
+ if encrypted:
+ console.print(f" [dim]{encrypted[:50]}...[/dim]")
+ else:
+ console.print(" [dim]Variable not found[/dim]")🤖 Prompt for AI Agents
In examples/env_demo.py around lines 220-221, manager.get(...) returns
Optional[str] and the code slices encrypted[:50] without checking for None; add
a guard to handle None before slicing (e.g., check if encrypted is None and
print a placeholder or an explicit "not found" message), or coerce to an empty
string before slicing, so the demo does not raise TypeError when the key is
missing.
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive environment variable management system for Cortex Linux, providing encrypted storage, per-application isolation, validation, templates, and CLI integration for managing environment variables.
Key Changes:
- New EnvManager class with encryption support using Fernet (AES-based symmetric encryption)
- CLI commands for setting, getting, listing, importing, exporting, and managing environment variables
- Built-in templates for common frameworks (Node.js, Python, Django, Docker)
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| cortex/env_manager.py | Core implementation of environment variable manager with encryption, validation, and template support |
| cortex/cli.py | CLI integration adding env command with multiple subcommands for variable management |
| tests/test_env_manager.py | Comprehensive test suite covering EnvManager functionality, encryption, validation, and templates |
| docs/ENV_MANAGEMENT.md | Complete documentation with usage examples, best practices, and API reference |
| examples/env_demo.py | Demo script showcasing environment manager features using rich console output |
| requirements.txt | Added cryptography>=41.0.0 dependency for encryption support |
| .gitignore | Added myapp.env to prevent committing example environment files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Escape quotes and special chars for shell | ||
| value = value.replace('"', '\\"') | ||
| lines.append(f'export {key}="{value}"') |
There was a problem hiding this comment.
The export function's shell escaping for .env format only escapes double quotes but doesn't handle other shell special characters like backticks, dollar signs, or newlines. A value containing these characters could cause issues or security vulnerabilities when the exported file is sourced in a shell. Consider using a more robust escaping mechanism or warning users about potential security implications when exporting to shell format.
| # Escape quotes and special chars for shell | |
| value = value.replace('"', '\\"') | |
| lines.append(f'export {key}="{value}"') | |
| # Safely single-quote for shell: escape embedded single quotes | |
| safe_value = value.replace("'", "'\"'\"'") | |
| lines.append(f"export {key}='{safe_value}'") |
| self.add_validation_rule( | ||
| r".*PORT.*", | ||
| RegexValidator(r"^([1-9][0-9]{0,3}|[1-5][0-9]{4}|6[0-4][0-9]{3}|65[0-4][0-9]{2}|655[0-2][0-9]|6553[0-5])$", "Must be a valid port number (1-65535)"), | ||
| ) |
There was a problem hiding this comment.
The port validation regex pattern has a potential issue. The pattern ^([1-9][0-9]{0,3}|[1-5][0-9]{4}|6[0-4][0-9]{3}|65[0-4][0-9]{2}|655[0-2][0-9]|6553[0-5])$ should correctly validate ports 1-65535, but it's overly complex and difficult to maintain. A simpler approach would be to convert the string to an integer and check if it's in the valid range (1-65535). This would be more readable, maintainable, and less error-prone.
|
|
||
| ### How It Works | ||
|
|
||
| Cortex uses Fernet encryption (symmetric encryption based on AES-128) to protect sensitive values: |
There was a problem hiding this comment.
The documentation states that Fernet uses "AES-128", but Fernet actually uses AES-128 in CBC mode with PKCS7 padding for the encryption. However, the key derivation using PBKDF2 with SHA256 and 32-byte length in the code (line 181 of env_manager.py) creates a 256-bit key, which means Fernet will use AES-128-CBC with the first 128 bits. The documentation should be more precise about the encryption algorithm details or simply state "AES encryption" without specifying the bit length to avoid confusion.
| Cortex uses Fernet encryption (symmetric encryption based on AES-128) to protect sensitive values: | |
| Cortex uses Fernet encryption (symmetric encryption based on AES in CBC mode with PKCS7 padding) to protect sensitive values: |
| console.print(f"\n⚠️ About to delete {len(env_vars)} variables from '{args.app}'") | ||
| confirm = input("Continue? [y/N]: ") | ||
| if confirm.lower() != "y": | ||
| cx_print("Cancelled", "info") | ||
| return 0 |
There was a problem hiding this comment.
The clear command uses input() for confirmation which could cause issues in non-interactive environments or automated scripts. Consider adding a --yes or --force flag to allow non-interactive clearing, or detecting if stdin is not a TTY and handling that case appropriately. This would improve usability in CI/CD pipelines and automated deployments.
| env_vars[key] = EnvVariable( | ||
| key=key, | ||
| value=value, | ||
| encrypted=False, | ||
| description="", | ||
| created_at=now, | ||
| updated_at=now, | ||
| tags=[], | ||
| ) | ||
|
|
||
| elif format == "env": | ||
| for line in env_data.strip().split("\n"): | ||
| line = line.strip() | ||
| if not line or line.startswith("#"): | ||
| continue | ||
|
|
||
| # Parse export KEY="value" or KEY=value | ||
| if line.startswith("export "): | ||
| line = line[7:] # Remove "export " | ||
|
|
||
| if "=" not in line: | ||
| continue | ||
|
|
||
| key, _, value = line.partition("=") | ||
| key = key.strip() | ||
| value = value.strip() | ||
|
|
||
| # Remove quotes | ||
| if value.startswith('"') and value.endswith('"'): | ||
| value = value[1:-1] | ||
| elif value.startswith("'") and value.endswith("'"): | ||
| value = value[1:-1] | ||
|
|
||
| # Unescape | ||
| value = value.replace('\\"', '"') | ||
|
|
||
| if value != "[encrypted]": | ||
| # Set directly to new env_vars | ||
| now = datetime.utcnow().isoformat() | ||
| env_vars[key] = EnvVariable( | ||
| key=key, | ||
| value=value, | ||
| encrypted=False, | ||
| description="", | ||
| created_at=now, | ||
| updated_at=now, | ||
| tags=[], | ||
| ) |
There was a problem hiding this comment.
The import_env method bypasses validation when directly creating EnvVariable objects. Variables imported from files don't go through the validate() method that's called in the set() method. This means invalid URLs, ports, or empty required fields could be imported without any validation errors. Imported variables should be validated to maintain data integrity and consistency with manually set variables.
| if value != "[encrypted]": | ||
| # Set directly to new env_vars | ||
| now = datetime.utcnow().isoformat() | ||
| env_vars[key] = EnvVariable( | ||
| key=key, | ||
| value=value, | ||
| encrypted=False, | ||
| description="", | ||
| created_at=now, | ||
| updated_at=now, | ||
| tags=[], | ||
| ) |
There was a problem hiding this comment.
The import_env method skips variables with value "[encrypted]" during import, but doesn't provide any feedback to the user that these variables were skipped. This could lead to confusion when users import files that contain encrypted value placeholders. Consider logging a warning or returning information about skipped variables so users understand that encrypted values cannot be imported from exported files.
| # Apply template variables | ||
| for key, default_value in template.variables.items(): | ||
| value = variables.get(key, default_value) | ||
| self.set(app_name, key, value, description=f"From template: {template_name}") | ||
|
|
There was a problem hiding this comment.
The validation is performed before saving variables, but when applying templates with the apply_template method, validation errors could occur after some template variables have already been set. This means a failed template application could leave the environment in a partially configured state. Consider validating all template variables before setting any of them, or implementing a transaction-like rollback mechanism to ensure atomic template application.
| # Apply template variables | |
| for key, default_value in template.variables.items(): | |
| value = variables.get(key, default_value) | |
| self.set(app_name, key, value, description=f"From template: {template_name}") | |
| # Snapshot current environment so we can roll back on failure | |
| # This relies on the existing export/import functionality described in the module docstring. | |
| original_env = self.export_env(app_name) | |
| # Apply template variables atomically | |
| try: | |
| for key, default_value in template.variables.items(): | |
| value = variables.get(key, default_value) | |
| self.set(app_name, key, value, description=f"From template: {template_name}") | |
| except Exception: | |
| # Roll back to the original environment to avoid partial template application | |
| self.import_env(app_name, original_env, format="env", merge=False) | |
| raise |
| # Generate new key using PBKDF2 | ||
| # In production, this should use a user-provided password | ||
| # For now, we'll use machine-specific data | ||
| salt = os.urandom(16) | ||
| kdf = PBKDF2HMAC( | ||
| algorithm=hashes.SHA256(), | ||
| length=32, | ||
| salt=salt, | ||
| iterations=100000, | ||
| ) | ||
| # Use machine ID or generate random key | ||
| try: | ||
| with open("/etc/machine-id", "rb") as f: | ||
| machine_id = f.read().strip() | ||
| except FileNotFoundError: | ||
| machine_id = os.urandom(32) | ||
|
|
||
| key = kdf.derive(machine_id) | ||
| self.cipher_key = base64.urlsafe_b64encode(key) | ||
|
|
There was a problem hiding this comment.
The encryption key generation uses a random salt (os.urandom(16)) but doesn't store the salt anywhere. This salt is needed to regenerate the same key from the machine ID, but since it's not saved, it's effectively lost after key generation. This means the salt serves no purpose in the current implementation. Either the salt should be stored alongside the key (or derived deterministically), or the implementation should be changed to generate a completely random key using Fernet.generate_key() instead of using PBKDF2 with a discarded salt.
| # Generate new key using PBKDF2 | |
| # In production, this should use a user-provided password | |
| # For now, we'll use machine-specific data | |
| salt = os.urandom(16) | |
| kdf = PBKDF2HMAC( | |
| algorithm=hashes.SHA256(), | |
| length=32, | |
| salt=salt, | |
| iterations=100000, | |
| ) | |
| # Use machine ID or generate random key | |
| try: | |
| with open("/etc/machine-id", "rb") as f: | |
| machine_id = f.read().strip() | |
| except FileNotFoundError: | |
| machine_id = os.urandom(32) | |
| key = kdf.derive(machine_id) | |
| self.cipher_key = base64.urlsafe_b64encode(key) | |
| # Generate a new random Fernet key and store it | |
| self.cipher_key = Fernet.generate_key() |
| try: | ||
| with open("/etc/machine-id", "rb") as f: | ||
| machine_id = f.read().strip() | ||
| except FileNotFoundError: | ||
| machine_id = os.urandom(32) |
There was a problem hiding this comment.
The /etc/machine-id file is specific to Linux systems using systemd. On macOS, BSD, or Windows systems, this file won't exist, causing the fallback to os.urandom(32) every time. This means encryption keys won't be consistent across EnvManager instantiations on non-Linux systems, making encrypted values unreadable after the first session. Consider using a cross-platform approach such as the uuid.getnode() function to get a machine identifier, or always use a randomly generated key that's persisted to the key file.
| # Get encrypted value (raw) | ||
| encrypted = self.manager.get("myapp", "API_KEY", decrypt=False) | ||
| self.assertNotEqual(encrypted, original) | ||
| self.assertTrue(len(encrypted) > len(original)) |
There was a problem hiding this comment.
assertTrue(a > b) cannot provide an informative message. Using assertGreater(a, b) instead will give more informative messages.
| self.assertTrue(len(encrypted) > len(original)) | |
| self.assertGreater(len(encrypted), len(original)) |
1 participant
This PR introduces a complete environment variable management system for Cortex.
It adds per-application environment isolation with support for encrypted secrets, validation rules, and reusable environment templates. The feature includes CLI commands for setting, getting, listing, importing, exporting, and clearing environment variables.
Key additions:
This improves security, developer experience, and configuration consistency across applications.
Summary by CodeRabbit
New Features
envcommand with subcommands for setting, getting, listing, deleting, importing, and exporting variables.Documentation
Chores