fix: P0/P1 review findings + credential hygiene#121
Open
tiantt wants to merge 2 commits into
Open
Conversation
- cli_exec: lazy-import termios/tty so the CLI no longer crashes at import time on Windows; interactive exec/shell fails with a clear message instead. - ve_sign: sign X-Security-Token into the canonical headers when an STS session_token is present (mirrors auth/_sigv4); thread the token through ve_request and the VeCR/VeCodePipeline call sites. Static ak/sk callers keep the original four-header form (byte-identical signature). - identity.auth: forward the real STS session_token and stop sending the hard-coded 'identity_token' placeholder; IdentityToken is now an opt-in arg. - agent_server origin: disable allow_credentials when CORS origins are a wildcard (Starlette would otherwise echo any Origin with credentials). - ve_pipeline: honor config.build_timeout instead of the hard-coded 900s. - cli delete credential/harness: add --force and an interactive-only confirmation prompt; non-interactive callers keep deleting without a prompt (no behavior change for CI/scripts). - Tests: STS signing golden vector, CORS credential gating, delete confirmation (interactive decline + non-interactive proceed).
- redact: add secret_key, security_token and signature to the field list so the repo's own config key names and presigned-URL HMACs are masked; add regression tests (secrets masked, trace-id/git-sha/uuid preserved). - harness deploy: write harness.json (which stores the runtime API key in plaintext) with mode 0600 instead of default umask. - cli deploy: mask the API key in terminal output (full value stays in harness.json) and note the file must not be committed. - gitignore: ignore harness.json.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up remediation from a fresh full-repo review. Stacked on top of
fix/nfr-scan-findings; contains only the changes below.P0
agentkit/toolkit/cli/sandbox/cli_exec.py:termios/ttywere imported at module top level, so everyagentkitcommand (even--help) crashed at import time on Windows. Now imported lazily; interactive exec/shell exits with a clear message on platforms without a POSIX terminal.P1
agentkit/utils/ve_sign.py: signX-Security-Tokeninto the canonical headers when an STSsession_tokenis present (mirrorsagentkit/auth/_sigv4.py). Threaded throughve_requestand everyVeCR/VeCodePipelinecall site (agentkit/toolkit/volcengine/cr.py,code_pipeline.py,services/cr_service.py). Static ak/sk callers pass no token and keep the original four-header form (byte-identical signature — pinned by a golden vector).agentkit/sdk/identity/auth.py: forward the real STSsession_token; stop sending the hard-coded"identity_token"placeholder (IdentityTokenis now an opt-in argument).agentkit/apps/agent_server_app/origin.py: disableallow_credentialswhen CORS origins are a wildcard — Starlette otherwise echoes any Origin back with credentials, allowing any site to make credentialed cross-origin calls. Explicit allowlists keep credentials enabled.agentkit/toolkit/builders/ve_pipeline.py: honorconfig.build_timeout(default 3600s) instead of the hard-coded 900s cap.agentkit/toolkit/cli/cli_delete.py:delete credential/delete harnessgain--forceand an interactive-only confirmation prompt. Non-interactive callers (CI, pipes) keep deleting without a prompt, so this is not a breaking change for automation.Credential hygiene
agentkit/utils/redact.py: addsecret_key,security_token,signatureto the redaction field list (the repo's own config key names and presigned-URL HMACs were slipping through).agentkit/toolkit/harness/deploy.py: writeharness.json(stores the runtime API key in plaintext) with mode0600.agentkit/toolkit/cli/cli_deploy.py: mask the API key in deploy output (full value stays inharness.json)..gitignore: ignoreharness.json.Compatibility
No public API signatures removed; all new parameters are optional with defaults preserving prior behavior. The delete confirmation is gated on an interactive TTY, so automation is unaffected.
Tests
Full suite green (938 passed). New coverage:
tests/utils/test_redact.py, an STS golden-vector case intests/utils/test_ve_sign_signing.py, CORS credential gating intests/apps/test_agent_server_origin.py, and delete-confirmation cases intests/toolkit/cli/test_cli_credential.py.