diff --git a/.claude/settings.local.json b/.claude/settings.local.json index 2efedac7..8b35c993 100644 --- a/.claude/settings.local.json +++ b/.claude/settings.local.json @@ -3,8 +3,11 @@ "allow": [ "Bash(mkdir:*)", "Bash(jq:*)", - "Bash(cat:*)" + "Bash(cat:*)", + "Bash(rustc:*)", + "Bash(git commit:*)", + "Bash(git push:*)" ], "deny": [] } -} \ No newline at end of file +} diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 9160edce..57c5e546 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -1,4 +1,4 @@ -# This file was autogenerated by dist: https://opensource.axo.dev/cargo-dist/ +# This file was autogenerated by dist: https://axodotdev.github.io/cargo-dist # # Copyright 2022-2024, axodotdev # SPDX-License-Identifier: MIT or Apache-2.0 @@ -63,7 +63,7 @@ jobs: # we specify bash to get pipefail; it guards against the `curl` command # failing. otherwise `sh` won't catch that `curl` returned non-0 shell: bash - run: "curl --proto '=https' --tlsv1.2 -LsSf https://github.com/axodotdev/cargo-dist/releases/download/v0.28.1-prerelease.2/cargo-dist-installer.sh | sh" + run: "curl --proto '=https' --tlsv1.2 -LsSf https://github.com/axodotdev/cargo-dist/releases/download/v0.28.2/cargo-dist-installer.sh | sh" - name: Cache dist uses: actions/upload-artifact@v4 with: diff --git a/.gitignore b/.gitignore index 5221cf9c..260d9187 100644 --- a/.gitignore +++ b/.gitignore @@ -1,5 +1,5 @@ .devenv* -.pre-commit* +.pre-commit-hooks.yaml .direnv target .DS_Store diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml new file mode 100644 index 00000000..a3a48895 --- /dev/null +++ b/.pre-commit-config.yaml @@ -0,0 +1,5 @@ +repos: + - repo: https://github.com/doublify/pre-commit-rust + rev: v1.0 + hooks: + - id: fmt \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index 84769d41..3cc123d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,10 +10,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Integrate `secrecy` crate for secure secret handling with automatic memory zeroing - Bitwarden provider supports Bitwarden & Bitwarden Secrets Manager via - `bitwarden://` & `bws://` URIs. + `bitwarden://` & `bws://` URIs with enhanced error message sanitization. +- Add `reflect()` method to Provider trait for provider introspection +- Export `Provider` trait from secretspec crate for use in derived code ### Changed - Made keyring provider optional via `keyring` feature flag (enabled by default) +- Unified provider parsing logic in init command to support all provider formats consistently +- Downgraded keyring dependency to 3.6.2 +- Updated `with_provider` in derive macro to accept `TryInto>` for consistent provider handling + +### Fixed +- Fixed secret optionality logic: having a default value no longer makes a secret optional in generated types ## [0.2.0] - 2025-07-17 diff --git a/Cargo.lock b/Cargo.lock index 92f752c5..ebb5fe3a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -17,17 +17,6 @@ version = "2.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "320119579fcad9c21884f5c4861d16174d0e06250625266f50fe6898340abefa" -[[package]] -name = "aes" -version = "0.8.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b169f7a6d4742236a0a00c541b845991d0ac43e546831af1249753ab4c3aa3a0" -dependencies = [ - "cfg-if", - "cipher", - "cpufeatures", -] - [[package]] name = "anstream" version = "0.6.19" @@ -126,24 +115,6 @@ version = "2.9.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1b8e56985ec62d17e9c1001dc89c88ecd7dc08e47eba5ec7c29c7b5eeecde967" -[[package]] -name = "block-buffer" -version = "0.10.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3078c7629b62d3f0439517fa394996acacc5cbc91c5a20d8c658e77abd503a71" -dependencies = [ - "generic-array", -] - -[[package]] -name = "block-padding" -version = "0.3.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a8894febbff9f758034a5b8e12d87918f56dfc64a8e1fe757d65e29041538d93" -dependencies = [ - "generic-array", -] - [[package]] name = "bumpalo" version = "3.19.0" @@ -162,31 +133,12 @@ version = "1.10.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d71b6127be86fdcfddb610f7182ac57211d4b18a3e9c82eb2d17662f2227ad6a" -[[package]] -name = "cbc" -version = "0.1.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "26b52a9543ae338f279b96b0b9fed9c8093744685043739079ce85cd58f289a6" -dependencies = [ - "cipher", -] - [[package]] name = "cfg-if" version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9555578bc9e57714c812a1f84e4fc5b4d21fcb063490c624de019f7464c91268" -[[package]] -name = "cipher" -version = "0.4.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "773f3b9af64447d2ce9850330c473515014aa235e6a783b02db81ff39e4a3dad" -dependencies = [ - "crypto-common", - "inout", -] - [[package]] name = "clap" version = "4.5.40" @@ -255,6 +207,16 @@ dependencies = [ "windows-sys 0.59.0", ] +[[package]] +name = "core-foundation" +version = "0.9.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "91e195e091a93c46f7102ec7818a2aa394e1e1771c3ab4825963fa03e45afb8f" +dependencies = [ + "core-foundation-sys", + "libc", +] + [[package]] name = "core-foundation" version = "0.10.1" @@ -271,15 +233,6 @@ version = "0.8.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "773648b94d0e5d620f64f280777445740e61fe701025087ec8b57f45c791888b" -[[package]] -name = "cpufeatures" -version = "0.2.17" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "59ed5838eebb26a2bb2e58f6d5b5316989ae9d08bab10e0e6d103e656d1b0280" -dependencies = [ - "libc", -] - [[package]] name = "crossterm" version = "0.25.0" @@ -305,16 +258,6 @@ dependencies = [ "winapi", ] -[[package]] -name = "crypto-common" -version = "0.1.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1bfb12502f3fc46cca1bb51ac28df9d618d813cdc3d2f25b9fe775a34af26bb3" -dependencies = [ - "generic-array", - "typenum", -] - [[package]] name = "dbus" version = "0.9.7" @@ -332,27 +275,11 @@ version = "4.0.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b42a16374481d92aed73ae45b1f120207d8e71d24fb89f357fadbd8f946fd84b" dependencies = [ - "aes", - "block-padding", - "cbc", "dbus", "futures-util", - "hkdf", "num", "once_cell", "rand", - "sha2", -] - -[[package]] -name = "digest" -version = "0.10.7" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9ed9a281f7bc9b7576e61468ba615a66a5c8cfdff42420a70aa82701a3b1e292" -dependencies = [ - "block-buffer", - "crypto-common", - "subtle", ] [[package]] @@ -488,16 +415,6 @@ dependencies = [ "slab", ] -[[package]] -name = "generic-array" -version = "0.14.7" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "85649ca51fd72272d7821adaf274ad91c288277713d9c18820d8499a7ff69e9a" -dependencies = [ - "typenum", - "version_check", -] - [[package]] name = "getrandom" version = "0.2.16" @@ -545,24 +462,6 @@ version = "0.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2304e00983f87ffb38b55b444b5e3b60a884b5d30c0fca7d82fe33449bbe55ea" -[[package]] -name = "hkdf" -version = "0.12.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7b5f8eb2ad728638ea2c7d47a21db23b7b58a72ed6a38256b8a1849f15fbbdf7" -dependencies = [ - "hmac", -] - -[[package]] -name = "hmac" -version = "0.12.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6c49c37c09c17a53d937dfbb742eb3a961d65a994e6bcdcf37e7399d0cc8ab5e" -dependencies = [ - "digest", -] - [[package]] name = "http" version = "1.3.1" @@ -691,16 +590,6 @@ dependencies = [ "hashbrown", ] -[[package]] -name = "inout" -version = "0.1.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "879f10e63c20629ecabbb64a8010319738c66a5cd0c29b02d63d272b03751d01" -dependencies = [ - "block-padding", - "generic-array", -] - [[package]] name = "inquire" version = "0.6.2" @@ -758,14 +647,16 @@ dependencies = [ [[package]] name = "keyring" -version = "4.0.0-rc.1" +version = "3.6.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bb06f73ca0ea1cbd3858e54404585e33dccb860cb4fc8a66ad5e75a5736f3f19" +checksum = "1961983669d57bdfe6c0f3ef8e4c229b5ef751afcc7d87e4271d2f71f6ccfa8b" dependencies = [ "byteorder", "dbus-secret-service", + "linux-keyutils", "log", - "security-framework", + "security-framework 2.11.1", + "security-framework 3.2.0", "windows-sys 0.59.0", ] @@ -820,6 +711,16 @@ dependencies = [ "syn", ] +[[package]] +name = "linux-keyutils" +version = "0.2.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "761e49ec5fd8a5a463f9b84e877c373d888935b71c6be78f3767fe2ae6bed18e" +dependencies = [ + "bitflags 2.9.1", + "libc", +] + [[package]] name = "linux-raw-sys" version = "0.9.4" @@ -1277,6 +1178,19 @@ dependencies = [ "url", ] +[[package]] +name = "security-framework" +version = "2.11.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "897b2245f0b511c87893af39b033e5ca9cce68824c4d7e7630b5a1d339658d02" +dependencies = [ + "bitflags 2.9.1", + "core-foundation 0.9.4", + "core-foundation-sys", + "libc", + "security-framework-sys", +] + [[package]] name = "security-framework" version = "3.2.0" @@ -1284,7 +1198,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "271720403f46ca04f7ba6f55d438f8bd878d6b8ca0a1046e8228c4145bcbb316" dependencies = [ "bitflags 2.9.1", - "core-foundation", + "core-foundation 0.10.1", "core-foundation-sys", "libc", "security-framework-sys", @@ -1354,17 +1268,6 @@ dependencies = [ "serde", ] -[[package]] -name = "sha2" -version = "0.10.9" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a7507d819769d01a365ab707794a4084392c824f54a7a6a7862f8c3d0892b283" -dependencies = [ - "cfg-if", - "cpufeatures", - "digest", -] - [[package]] name = "signal-hook" version = "0.3.18" @@ -1425,12 +1328,6 @@ version = "0.11.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7da8b5736845d9f2fcb837ea5d9e2628564b3b043a70948a3f0b778838c5fb4f" -[[package]] -name = "subtle" -version = "2.6.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "13c2bddecc57b384dee18652358fb23172facb8a2c51ccc10d74c157bdea3292" - [[package]] name = "supports-color" version = "3.0.2" @@ -1628,12 +1525,6 @@ dependencies = [ "toml", ] -[[package]] -name = "typenum" -version = "1.18.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1dccffe3ce07af9386bfd29e80c0ab1a8205a2fc34e4bcd40364df902cfa8f3f" - [[package]] name = "unicode-ident" version = "1.0.18" @@ -1687,12 +1578,6 @@ version = "0.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "06abde3611657adf66d383f00b093d7faecc7fa57071cce2578660c9f1010821" -[[package]] -name = "version_check" -version = "0.9.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0b928f33d975fc6ad9f86c8f283853ad26bdd5b10b7f1542aa2fa15e2289105a" - [[package]] name = "wasi" version = "0.11.1+wasi-snapshot-preview1" diff --git a/Cargo.toml b/Cargo.toml index fa968f14..4b453ac6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -12,7 +12,7 @@ edition = "2024" [workspace.dependencies] clap = { version = "4.0", features = ["derive", "env"] } -keyring = { version = "4.0.0-rc.1", features = ["encrypted"] } +keyring = { version = "3.6.2", features = ["linux-native", "sync-secret-service", "windows-native", "apple-native"] } serde = { version = "1.0", features = ["derive"] } toml = "0.8" thiserror = "1.0" diff --git a/devenv.lock b/devenv.lock index 9ec0bc51..9bef56f5 100644 --- a/devenv.lock +++ b/devenv.lock @@ -3,10 +3,10 @@ "devenv": { "locked": { "dir": "src/modules", - "lastModified": 1752692978, + "lastModified": 1753461480, "owner": "cachix", "repo": "devenv", - "rev": "b2281100077d641cbf135e8680e973c0c2270bc4", + "rev": "3344ada87942ace7f386cd15b7ad6991484bc3f0", "type": "github" }, "original": { @@ -74,15 +74,14 @@ }, "nixpkgs": { "locked": { - "lastModified": 1752708699, - "owner": "domenkozar", + "lastModified": 1753461381, + "owner": "NixOS", "repo": "nixpkgs", - "rev": "5bbf675572638dcf7ca694a7b95fe12a7e34bffb", + "rev": "ae938d58f85f768d2351068ef7a154a184fd6ab8", "type": "github" }, "original": { - "owner": "domenkozar", - "ref": "cargo-dist-bump", + "owner": "NixOS", "repo": "nixpkgs", "type": "github" } @@ -105,10 +104,10 @@ ] }, "locked": { - "lastModified": 1752633862, + "lastModified": 1753411536, "owner": "oxalica", "repo": "rust-overlay", - "rev": "8668ca94858206ac3db0860a9dec471de0d995f8", + "rev": "7ae12d14d6bb74acfadf31e17a204d928eaf77b8", "type": "github" }, "original": { diff --git a/devenv.yaml b/devenv.yaml index 3a57c755..eb3a0e96 100644 --- a/devenv.yaml +++ b/devenv.yaml @@ -1,6 +1,6 @@ inputs: nixpkgs: - url: github:domenkozar/nixpkgs/cargo-dist-bump + url: github:NixOS/nixpkgs rust-overlay: url: github:oxalica/rust-overlay inputs: diff --git a/dist-workspace.toml b/dist-workspace.toml index 9c7834fb..d101b4f0 100644 --- a/dist-workspace.toml +++ b/dist-workspace.toml @@ -4,24 +4,20 @@ members = ["cargo:."] # Config for 'dist' [dist] # The preferred dist version to use in CI (Cargo.toml SemVer syntax) -cargo-dist-version = "0.28.1-prerelease.2" +cargo-dist-version = "0.28.2" # CI backends to support ci = "github" # The installers to generate for each app installers = ["shell"] # Target platforms to build apps for (Rust target-triple syntax) -targets = [ - "aarch64-apple-darwin", - # broken due to pkg-config - #"aarch64-unknown-linux-gnu", - "x86_64-apple-darwin", - "x86_64-unknown-linux-gnu", - "x86_64-pc-windows-msvc", -] +targets = ["aarch64-apple-darwin", "aarch64-unknown-linux-gnu", "x86_64-apple-darwin", "x86_64-unknown-linux-gnu", "x86_64-pc-windows-msvc"] # Path that installers should place binaries in install-path = "CARGO_HOME" # Whether to install an updater program install-updater = true [dist.dependencies.apt] -libdbus-1-dev = '*' +libdbus-1-dev = { version = "*", targets = [ + "x86_64-unknown-linux-gnu", + "aarch64-unknown-linux-gnu", +] } diff --git a/docs/src/content/docs/providers/bitwarden.md b/docs/src/content/docs/providers/bitwarden.md index d27d13b5..c3e5b8a9 100644 --- a/docs/src/content/docs/providers/bitwarden.md +++ b/docs/src/content/docs/providers/bitwarden.md @@ -227,4 +227,103 @@ To install it: ### Item Access - Graceful handling of missing items - Field validation and suggestions -- Organization/collection permission guidance \ No newline at end of file +- Organization/collection permission guidance + +### Timeout Configuration +- Commands timeout after 30 seconds by default +- Configurable via `BITWARDEN_CLI_TIMEOUT` environment variable +- Prevents hanging on network issues or CLI problems + +## Performance Monitoring + +The Bitwarden provider includes detailed performance instrumentation to help identify bottlenecks and optimize operations. + +### Enable Performance Logging + +```bash +# Enable detailed timing for any SecretSpec operation +$ export SECRETSPEC_PERF_LOG=1 +$ secretspec get SECRET_NAME --provider bitwarden:// +``` + +### Performance Output + +When enabled, you'll see detailed timing breakdown: + +``` +[PERF] bw status took 1.911577208s (1911ms) +[PERF] auth check took 1915ms +[PERF] PM item search took 850ms +[PERF] PM JSON parse took 5ms, 42 items +[PERF] get('SECRET_NAME') took 1.916723292s (1916ms) +``` + +### Performance Analysis Scripts + +Two scripts are provided for comprehensive performance analysis: + +#### Basic Performance Test +```bash +# Test that performance logging works +$ ./tests/test_performance_logging.sh +``` + +#### Comprehensive Analysis +```bash +# Run detailed performance benchmarks (requires BW_SESSION) +$ ./tests/bitwarden_performance.sh [BW_SESSION] +``` + +The comprehensive script measures: +- CLI command execution times +- JSON parsing performance +- Vault size impact +- Different retrieval strategies +- Repeated operation overhead +- SecretSpec integration timing + +### Metrics Tracked + +- **CLI Command Execution**: Individual `bw`/`bws` command timing +- **Authentication Checks**: Vault status verification time +- **Item Search**: Time to find items in vault (`bw list --search` vs `bw get item`) +- **JSON Parsing**: Parse time and item counts for large vaults +- **Overall Operations**: Total time for get/set operations +- **Field Extraction**: Time to extract specific fields from items + +### Performance Insights + +The timing data helps identify optimization opportunities: + +- **CLI vs API**: Compare different Bitwarden CLI command strategies +- **Vault Size Impact**: How vault size affects search performance +- **Caching Benefits**: Measure repeated access patterns +- **Bottleneck Identification**: Pinpoint slowest operations + +### Usage Examples + +```bash +# Monitor a single operation +$ SECRETSPEC_PERF_LOG=1 secretspec get DATABASE_URL --provider bitwarden:// + +# Monitor multiple operations with defaults +$ export SECRETSPEC_PERF_LOG=1 +$ export BITWARDEN_DEFAULT_TYPE=login +$ secretspec get API_KEY --provider bitwarden:// +$ secretspec get DATABASE_PASSWORD --provider bitwarden:// + +# Run performance analysis +$ ./tests/bitwarden_performance.sh $BW_SESSION + +# Configure timeout (in seconds) +$ BITWARDEN_CLI_TIMEOUT=60 secretspec get SECRET --provider bitwarden:// +``` + +### Troubleshooting Performance + +Common performance patterns and solutions: + +- **Slow vault access**: Consider using `bw sync` before operations +- **Large vault impact**: Use collection scoping to reduce search space +- **Repeated access**: Performance logging shows caching opportunities +- **Network latency**: Self-hosted instances may have different timing characteristics \ No newline at end of file diff --git a/docs/src/content/docs/providers/bw_project_plan.md b/docs/src/content/docs/providers/bw_project_plan.md index 134b40f8..750ccada 100644 --- a/docs/src/content/docs/providers/bw_project_plan.md +++ b/docs/src/content/docs/providers/bw_project_plan.md @@ -766,4 +766,201 @@ bw list items --search "secretspec/test/" | jq -r '.[].id' | xargs -I {} bw dele - ✅ Comprehensive error handling - ✅ URI configuration support complete - ✅ Integration tests successful -- ✅ No known issues or limitations \ No newline at end of file +- ✅ No known issues or limitations + +--- + +# Code Review and Improvement Plan + +## Code Quality Assessment ⭐⭐⭐⭐⭐ + +### Code Quality and Best Practices + +**Strengths:** +- **Excellent architecture**: Clear separation between Password Manager and Secrets Manager with unified interface +- **Comprehensive error handling**: Detailed, actionable error messages for common failure scenarios +- **Strong type safety**: Well-designed enums for item types and services with proper serialization +- **Good documentation**: Extensive inline documentation with examples +- **Consistent patterns**: Follows established SecretSpec provider patterns exactly + +**Areas for improvement:** +- **String cloning**: Excessive use of `.clone()` in SecretString conversions (lines 1203-1208, 1222-1238). Consider using references where possible +- **Magic numbers**: Item type constants could be defined as associated constants rather than enum discriminants +- **Method length**: Some methods like `extract_field_from_item` are quite long and could benefit from decomposition + +### Potential Bugs or Issues ⚠️ + +**Critical Issues:** +1. **Memory safety with SecretString**: The implementation correctly uses SecretString but exposes secrets frequently with `expose_secret()` - consider minimizing exposure scope +2. **Command injection potential**: CLI arguments are not properly escaped, though risk is low since they come from configuration + +**Minor Issues:** +1. **Case sensitivity**: Item name matching appears case-sensitive, which could cause user confusion +2. **Error propagation**: Some errors are converted to strings too early, losing type information +3. **Timeout handling**: No timeout configuration for CLI commands, which could hang indefinitely + +### Performance Considerations 🚀 + +**Good aspects:** +- **Synchronous operations**: Avoids async complexity as intended +- **Direct CLI integration**: Leverages optimized Bitwarden CLI + +**Concerns:** +1. **Multiple CLI calls**: Each operation may trigger multiple `bw` commands (status check, then operation) +2. **JSON parsing overhead**: Large vault responses are fully parsed even when only one item is needed +3. **No caching**: Repeated calls for the same secret will hit the CLI each time +4. **Command spawning overhead**: Each CLI call spawns a new process + +**Recommendations:** +- Consider implementing a simple in-memory cache for frequently accessed items +- Batch operations where possible +- Add timeout configuration for CLI commands + +### Security Concerns 🔒 + +**Well-handled:** +- **SecretString integration**: Proper use of memory-safe secret handling +- **Environment variable handling**: Secure token management +- **CLI output sanitization**: Stderr is properly captured and filtered + +**Areas of concern:** +1. **Secret exposure in logs**: CLI error messages might contain sensitive data +2. **Temporary files**: No evidence of secure cleanup if temp files are used +3. **Process environment**: Environment variables are passed to child processes +4. **Command line visibility**: CLI arguments may be visible in process lists + +**Recommendations:** +- Audit CLI error message handling to ensure no secrets leak +- Consider using stdin for sensitive CLI arguments where possible +- Add explicit memory clearing for sensitive strings + +### Test Coverage ✅ + +**Excellent coverage:** +- **Integration tests**: Comprehensive real-world testing with actual Bitwarden CLI +- **Unit tests**: Good coverage of configuration parsing and type conversion +- **Error scenarios**: Tests for authentication failures and CLI errors +- **Edge cases**: Special characters, Unicode, multiple profiles + +**Missing areas:** +1. **Concurrency testing**: No tests for concurrent access patterns +2. **CLI timeout scenarios**: No tests for hanging CLI processes +3. **Performance bottleneck analysis**: No measurement of CLI vs JSON processing time +4. **Memory leak testing**: No tests for SecretString cleanup + +## Improvement Plan + +### Phase 1: Critical Security & Reliability (High Priority, 1-2 days) + +**1. Add CLI Command Timeouts** +- Add timeout configuration to `BitwardenConfig` (default: 30s) +- Implement timeout handling in `execute_bw_command()` and `execute_bws_command()` +- Add timeout tests for hanging CLI scenarios +- **Risk**: CLI commands can hang indefinitely +- **Files**: `bitwarden.rs` (command execution methods) + +**2. Audit Secret Leakage in Error Messages** +- Review all error message construction for potential secret exposure +- Sanitize CLI stderr output before including in error messages +- Add tests to verify no secrets appear in error messages +- **Risk**: Secrets could leak through error logs +- **Files**: `bitwarden.rs` (error handling in CLI methods) + +### Phase 2: Performance Optimizations (Medium Priority, 2-3 days) + +**3. Reduce String Cloning Overhead** ✅ **Complete** +- ✅ Replaced unnecessary `.clone()` calls in SecretString conversions +- ✅ Implemented `AsRef` helper functions for cleaner API +- ✅ Optimized field extraction methods to minimize allocations +- **Impact**: Reduced memory usage and improved code clarity +- **Files**: `bitwarden.rs` (field extraction methods) + +**4. Basic Caching Implementation** ⏸️ **Deprioritized** +- ~~Add optional in-memory cache for frequently accessed items~~ +- ~~Cache vault items by item name/ID with TTL (default: 5 minutes)~~ +- ~~Add cache invalidation and configuration options~~ +- **Status**: Deprioritized based on performance analysis findings +- **Reason**: Performance data shows 99.9% of execution time is CLI/network latency (2-4 seconds), while JSON processing is only 133μs (0.003%). Caching would provide minimal benefit for current usage patterns. +- **Alternative**: Consider batch secret retrieval for multi-secret workflows instead + +### Phase 3: Code Quality Improvements (Medium Priority, 1-2 days) + +**5. Decompose Long Methods** +- Split `extract_field_from_item()` into item-type-specific methods +- Extract field mapping logic into separate helper methods +- Improve readability and maintainability +- **Impact**: Better code organization and testability +- **Files**: `bitwarden.rs` (field extraction methods) + +**6. Add Better Type Safety** +- Define item type constants as associated constants +- Improve error types with more specific variants +- Add validation for configuration combinations +- **Impact**: Better compile-time safety and clearer errors +- **Files**: `bitwarden.rs` (type definitions and configuration) + +### Phase 4: Performance Analysis (Lower Priority, 2-3 days) + +**7. Add Concurrency Tests** +- Test concurrent access to same secrets +- Test provider thread safety +- Test CLI command queuing and resource contention +- **Impact**: Ensure production reliability under load +- **Files**: `tests.rs` (new test module) + +**8. Add Performance Bottleneck Analysis** +- Instrument CLI command execution times (separate timing for `bw`/`bws` vs `jq`) +- Measure `bw list` + `jq` filtering performance vs more targeted commands +- Add timing metrics to `bitwarden_integration.sh` script with aggregate reporting +- Identify optimization opportunities (e.g., `bw get item` vs `bw list | jq`) +- **Impact**: Identify and fix actual performance bottlenecks in CLI usage +- **Files**: `bitwarden.rs` (add timing instrumentation), `tests/bitwarden_integration.sh` + +**Specific measurements:** +- Time for `bw list items --search "term"` +- Time for `jq` processing of large JSON responses +- Time for `bw get item "specific-item"` as alternative +- Memory usage of JSON parsing with large responses +- Compare different CLI command strategies + +### Phase 5: Advanced Features (Optional, 3-4 days) + +**9. Enhanced Error Recovery** +- Add retry logic for transient CLI failures +- Implement exponential backoff for rate limiting +- Add circuit breaker pattern for CLI availability +- **Impact**: Better reliability in production environments + +**10. CLI Argument Security** +- Use stdin for sensitive CLI arguments where possible +- Minimize command line visibility of secrets +- Add secure temporary file handling if needed +- **Impact**: Reduce attack surface for process monitoring + +## Implementation Priority Matrix + +| Task | Priority | Risk | Effort | Dependencies | +|------|----------|------|--------|--------------| +| CLI Timeouts | High | High | Low | None | +| Secret Leakage Audit | High | High | Low | None | +| String Cloning | Medium | Low | Low | None | +| Basic Caching | Medium | Low | Medium | None | +| Method Decomposition | Medium | Low | Low | None | +| Type Safety | Medium | Low | Medium | None | +| Concurrency Tests | Medium | Medium | Medium | Phases 1-2 | +| Performance Analysis | Low | Low | Medium | Phase 2 | +| Error Recovery | Low | Low | High | Phases 1-3 | +| CLI Security | Low | Medium | High | All phases | + +## Overall Assessment 📊 + +**Grade: A- (Excellent with minor improvements needed)** + +This is a **production-ready, well-architected implementation** that demonstrates: +- Deep understanding of SecretSpec patterns +- Comprehensive error handling +- Strong security practices +- Excellent documentation +- Thorough testing with 4K+ entry real-world vault + +**Recommended approach**: Execute phases sequentially, with Phase 1 being mandatory before production deployment. The implementation is already suitable for production use, with these improvements enhancing reliability and performance. \ No newline at end of file diff --git a/secretspec-derive/src/lib.rs b/secretspec-derive/src/lib.rs index 0a8e570c..83367825 100644 --- a/secretspec-derive/src/lib.rs +++ b/secretspec-derive/src/lib.rs @@ -441,9 +441,10 @@ fn field_name_ident(name: &str) -> proc_macro2::Ident { /// Helper function to check if a secret is optional. /// -/// A secret is considered optional if: -/// - It has `required = false` in the config, OR -/// - It has a default value specified +/// A secret is considered optional only if: +/// - It has `required = false` in the config +/// +/// Having a default value does not make a secret optional. /// /// # Arguments /// @@ -453,7 +454,7 @@ fn field_name_ident(name: &str) -> proc_macro2::Ident { /// /// `true` if the secret is optional, `false` if required fn is_secret_optional(secret_config: &Secret) -> bool { - !secret_config.required || secret_config.default.is_some() + !secret_config.required } /// Determines if a field should be optional across all profiles. @@ -1098,14 +1099,14 @@ mod builder_generation { /// /// ```ignore /// pub struct SecretSpecBuilder { - /// provider: Option Result>>, + /// provider: Option Result, String>>>, /// profile: Option Result>>, /// } /// ``` pub fn generate_struct() -> proc_macro2::TokenStream { quote! { pub struct SecretSpecBuilder { - provider: Option Result>>, + provider: Option Result, String>>>, profile: Option Result>>, } } @@ -1148,18 +1149,13 @@ mod builder_generation { pub fn with_provider(mut self, provider: T) -> Self where - T: TryInto, - T::Error: std::fmt::Display, + T: TryInto> + 'static, + T::Error: std::fmt::Display + 'static, { - match provider.try_into() { - Ok(url) => { - self.provider = Some(Box::new(move || Ok(url))); - } - Err(e) => { - let error_msg = format!("Invalid provider URI: {}", e); - self.provider = Some(Box::new(move || Err(error_msg))); - } - } + self.provider = Some(Box::new(move || { + provider.try_into() + .map_err(|e| format!("Invalid provider: {}", e)) + })); self } @@ -1193,17 +1189,18 @@ mod builder_generation { /// /// # Generated Logic /// - /// 1. If provider is set, call the closure to get the URI + /// 1. If provider is set, call the closure to get the Provider instance /// 2. Convert any errors to SecretSpecError - /// 3. Convert URI to string for the loading system + /// 3. Extract the provider name to pass to the loading system fn generate_provider_resolution( provider_expr: proc_macro2::TokenStream, ) -> proc_macro2::TokenStream { quote! { let provider_str = if let Some(provider_fn) = #provider_expr { - let uri = provider_fn() + let provider_box = provider_fn() .map_err(|e| secretspec::SecretSpecError::ProviderOperationFailed(e))?; - Some(uri.to_string()) + // Get the provider name to pass as a string to set_provider + Some(provider_box.name().to_string()) } else { None }; diff --git a/secretspec-derive/src/tests.rs b/secretspec-derive/src/tests.rs index 5b4d8968..407e455a 100644 --- a/secretspec-derive/src/tests.rs +++ b/secretspec-derive/src/tests.rs @@ -531,13 +531,13 @@ HAS_DEFAULT = { description = "Secret with default", required = true, default = }; assert!(!is_secret_optional(&required_no_default)); - // Required with default (should be optional) + // Required with default (should NOT be optional) let required_with_default = Secret { description: Some("Required with default".to_string()), required: true, default: Some("default_value".to_string()), }; - assert!(is_secret_optional(&required_with_default)); + assert!(!is_secret_optional(&required_with_default)); // Not required let not_required = Secret { @@ -633,8 +633,8 @@ HAS_DEFAULT = { description = "Secret with default", required = true, default = profiles, }; - // API_KEY is optional because it has default in development - assert!(is_field_optional_across_profiles("API_KEY", &config)); + // API_KEY is NOT optional (required in all profiles, default doesn't make it optional) + assert!(!is_field_optional_across_profiles("API_KEY", &config)); // DATABASE_URL is optional because it's not required in default assert!(is_field_optional_across_profiles("DATABASE_URL", &config)); diff --git a/secretspec-derive/tests/integration_test.rs b/secretspec-derive/tests/integration_test.rs index 3ec30999..cf0b188f 100644 --- a/secretspec-derive/tests/integration_test.rs +++ b/secretspec-derive/tests/integration_test.rs @@ -42,7 +42,7 @@ mod profile_generation { redis_url, } => { let _: Option = api_key; // Optional in dev - let _: Option = database_url; // Required but has default + let _: String = database_url; // Required but has default let _: Option = redis_url; // Optional } _ => panic!("Expected Development variant"), @@ -70,7 +70,7 @@ mod profile_generation { // Verify the union struct has Option for fields that are optional in any profile fn _test_field_types(s: SecretSpec) { let _: Option = s.api_key; // Optional in development - let _: Option = s.database_url; // Has default in dev, so optional in union type + let _: String = s.database_url; // Has default in dev but still required let _: Option = s.redis_url; // Optional by default } } @@ -85,7 +85,7 @@ mod complex_generation { fn test_complex_field_types() { fn _test_field_types(s: SecretSpec) { let _: String = s.always_required; - let _: Option = s.required_with_default; // Has default + let _: String = s.required_with_default; // Has default but still required let _: Option = s.always_optional; let _: Option = s.complex_secret; // Optional in dev and test let _: Option = s.multi_profile; // Optional in base @@ -199,8 +199,8 @@ mod profile_inheritance { } => { let _: String = database_url; // Required let _: String = api_key; // Required - let _: Option = log_level; // Optional with default - let _: Option = cache_ttl; // Optional with default + let _: Option = log_level; // Optional (required=false) with default + let _: Option = cache_ttl; // Optional (required=false) with default } _ => panic!("Expected Default variant"), } @@ -212,8 +212,8 @@ mod profile_inheritance { database_url, debug_mode, } => { - let _: Option = database_url; // Override: not required with default - let _: Option = debug_mode; // New field in development + let _: Option = database_url; // Override: required=false with default + let _: Option = debug_mode; // New field in development (required=false) } _ => panic!("Expected Development variant"), } @@ -228,7 +228,7 @@ mod profile_inheritance { } => { let _: String = database_url; // Override: required let _: String = api_key; // Override: required - let _: Option = log_level; // Override: different default + let _: Option = log_level; // Override: required=false with different default } _ => panic!("Expected Production variant"), } @@ -242,8 +242,8 @@ mod profile_inheritance { enable_profiling, } => { let _: String = database_url; // Override: required - let _: Option = log_level; // Override: different default - let _: Option = enable_profiling; // New field in staging + let _: Option = log_level; // Override: required=false with different default + let _: Option = enable_profiling; // New field in staging (required=false) } _ => panic!("Expected Staging variant"), } diff --git a/secretspec-derive/tests/ui/file_not_found.rs b/secretspec-derive/tests/ui/file_not_found.rs index 97f905c4..2fd17728 100644 --- a/secretspec-derive/tests/ui/file_not_found.rs +++ b/secretspec-derive/tests/ui/file_not_found.rs @@ -3,4 +3,4 @@ use secretspec_derive::declare_secrets; // This should fail because the file doesn't exist declare_secrets!("this/file/does/not/exist.toml"); -fn main() {} \ No newline at end of file +fn main() {} diff --git a/secretspec-derive/tests/ui/invalid_toml.rs b/secretspec-derive/tests/ui/invalid_toml.rs index f6ba2cf8..58ae3e72 100644 --- a/secretspec-derive/tests/ui/invalid_toml.rs +++ b/secretspec-derive/tests/ui/invalid_toml.rs @@ -3,4 +3,4 @@ use secretspec_derive::declare_secrets; // This should fail because the TOML is invalid declare_secrets!("invalid_toml.txt"); -fn main() {} \ No newline at end of file +fn main() {} diff --git a/secretspec-derive/tests/ui/invalid_toml_embedded.rs b/secretspec-derive/tests/ui/invalid_toml_embedded.rs index d5c207c9..8bdf8384 100644 --- a/secretspec-derive/tests/ui/invalid_toml_embedded.rs +++ b/secretspec-derive/tests/ui/invalid_toml_embedded.rs @@ -3,4 +3,4 @@ use secretspec_derive::declare_secrets; // This should fail because the TOML is invalid declare_secrets!("invalid_toml_embedded.txt"); -fn main() {} \ No newline at end of file +fn main() {} diff --git a/secretspec.toml b/secretspec.toml new file mode 100644 index 00000000..3d7b199e --- /dev/null +++ b/secretspec.toml @@ -0,0 +1,16 @@ +[project] +name = "secretspec" +revision = "1.0" +# Extend configurations from subdirectories +# extends = [ "subdir1", "subdir2" ] + +[profiles.default] +# DATABASE_URL = { description = "Database connection string", required = true } + +[profiles.development] +# Development profile inherits all secrets from default profile +# Only define secrets here that need different values or settings than default +# DATABASE_URL = { default = "sqlite:///dev.db" } +# +# New secrets +# REDIS_URL = { description = "Redis connection URL for caching", required = false, default = "redis://localhost:6379" } diff --git a/secretspec/src/cli/mod.rs b/secretspec/src/cli/mod.rs index 9e69129e..602bcafa 100644 --- a/secretspec/src/cli/mod.rs +++ b/secretspec/src/cli/mod.rs @@ -1,4 +1,4 @@ -use crate::provider::{dotenv::DotEnvProvider, providers}; +use crate::provider::{Provider, providers}; use crate::{Config, GlobalConfig, GlobalDefaults, Profile, Project, Secrets}; use clap::{Parser, Subcommand}; use miette::{IntoDiagnostic, Result, WrapErr, miette}; @@ -206,26 +206,20 @@ pub fn main() -> Result<()> { } } - // Parse the provider URL - let uri = from - .parse::() - .map_err(|e| miette!("Invalid provider URL '{}': {}", from, e))?; + // Create provider from the specification string + // This handles various formats like "dotenv", "dotenv:.env", "dotenv://.env.production" + let provider: Box = from.as_str().try_into().into_diagnostic()?; - // Extract scheme from URI to validate provider - let scheme = uri.scheme(); - - // Currently only support dotenv provider - if scheme != "dotenv" { + // Check if it's a dotenv provider + if provider.name() != "dotenv" { return Err(miette!( - "Only 'dotenv://' provider URLs are currently supported for init --from. Got: {}", - from + "Only 'dotenv' provider is currently supported for init --from. Got provider: {}", + provider.name() )); } - // Create dotenv provider and reflect secrets - let dotenv_config = (&uri).try_into().into_diagnostic()?; - let dotenv_provider = DotEnvProvider::new(dotenv_config); - let secrets = dotenv_provider.reflect().into_diagnostic()?; + // Reflect secrets from the provider + let secrets = provider.reflect().into_diagnostic()?; // Create a new project config let mut profiles = HashMap::new(); @@ -267,6 +261,13 @@ pub fn main() -> Result<()> { .sum::(); println!("✓ Created secretspec.toml with {} secrets", secret_count); + // If we imported from a provider, suggest migration + if provider.name() == "dotenv" && secret_count > 0 { + println!("\nTo migrate your secrets from {}:", from); + println!(" 1. Review secretspec.toml and adjust as needed"); + println!(" 2. secretspec import {} # Import secret values", from); + } + println!("\nNext steps:"); println!(" 1. secretspec config init # Set up user configuration"); println!(" 2. secretspec check # Verify all secrets and set them"); diff --git a/secretspec/src/lib.rs b/secretspec/src/lib.rs index 8637969f..9e0c2bd2 100644 --- a/secretspec/src/lib.rs +++ b/secretspec/src/lib.rs @@ -65,6 +65,7 @@ pub use config::Secret; // Public API exports pub use error::{Result, SecretSpecError}; +pub use provider::Provider; pub use secrets::Secrets; pub use validation::ValidatedSecrets; diff --git a/secretspec/src/provider/bitwarden.rs b/secretspec/src/provider/bitwarden.rs index 88711924..86d61efc 100644 --- a/secretspec/src/provider/bitwarden.rs +++ b/secretspec/src/provider/bitwarden.rs @@ -3,6 +3,7 @@ use crate::{Result, SecretSpecError}; use secrecy::{ExposeSecret, SecretString}; use serde::{Deserialize, Serialize}; use std::process::Command; +use std::time::{Duration, Instant}; use url::Url; /// Bitwarden service type enum for distinguishing between Password Manager and Secrets Manager @@ -117,6 +118,7 @@ impl BitwardenItemType { } /// Get string representation + #[allow(dead_code)] pub fn as_str(&self) -> &'static str { match self { BitwardenItemType::Login => "login", @@ -175,6 +177,7 @@ impl BitwardenFieldType { } /// Get string representation + #[allow(dead_code)] pub fn as_str(&self) -> &'static str { match self { BitwardenFieldType::Text => "text", @@ -189,6 +192,7 @@ impl BitwardenFieldType { /// This struct deserializes the JSON output from the `bw get item` and `bw list items` commands. /// It supports all Bitwarden item types: Login, Secure Note, Card, Identity, etc. #[derive(Debug, Deserialize)] +#[allow(dead_code)] struct BitwardenItem { /// Unique identifier for the item. id: String, @@ -341,6 +345,7 @@ struct BitwardenSshKey { /// or boolean values. The field's name is used to identify specific /// data within an item. #[derive(Debug, Deserialize)] +#[allow(dead_code)] struct BitwardenField { /// The name/label of the field. name: Option, @@ -372,6 +377,7 @@ where /// using encoded JSON. It defines the structure and metadata for items that store secrets. /// Default item type is Login for better script compatibility. #[derive(Debug, Serialize)] +#[allow(dead_code)] struct BitwardenItemTemplate { /// The type of item (Login by default). #[serde(rename = "type", serialize_with = "serialize_item_type")] @@ -404,6 +410,7 @@ struct BitwardenItemTemplate { } /// Custom serializer for item type +#[allow(dead_code)] fn serialize_item_type( item_type: &BitwardenItemType, serializer: S, @@ -416,6 +423,7 @@ where /// Secure note configuration required for Bitwarden secure note items. #[derive(Debug, Serialize)] +#[allow(dead_code)] struct BitwardenSecureNote { /// Type of secure note. Always 0 for generic secure notes. #[serde(rename = "type")] @@ -427,6 +435,7 @@ struct BitwardenSecureNote { /// Each field represents a piece of data to store in the item. /// Used within BitwardenItemTemplate to define the item's content. #[derive(Debug, Serialize)] +#[allow(dead_code)] struct BitwardenFieldTemplate { /// The name/label of the field (e.g., "project", "key", "value"). name: String, @@ -438,6 +447,7 @@ struct BitwardenFieldTemplate { } /// Custom serializer for field type +#[allow(dead_code)] fn serialize_field_type( field_type: &BitwardenFieldType, serializer: S, @@ -571,6 +581,11 @@ pub struct BitwardenConfig { /// Default field name for storing values. /// Can be overridden by BITWARDEN_DEFAULT_FIELD environment variable. pub default_field: Option, + + /// Timeout in seconds for CLI commands (default: 30 seconds). + /// Prevents hanging CLI processes and provides better error handling. + /// Can be overridden by BITWARDEN_CLI_TIMEOUT environment variable. + pub cli_timeout: Option, } impl Default for BitwardenConfig { @@ -585,6 +600,7 @@ impl Default for BitwardenConfig { access_token: None, default_item_type: Some(BitwardenItemType::Login), // Login by default default_field: None, + cli_timeout: Some(30), // 30 seconds default timeout } } } @@ -642,6 +658,11 @@ impl TryFrom<&Url> for BitwardenConfig { } } "field" => config.default_field = Some(value.into_owned()), + "timeout" => { + if let Ok(timeout) = value.parse::() { + config.cli_timeout = Some(timeout); + } + } _ => {} // Ignore unknown parameters } } @@ -666,6 +687,11 @@ impl TryFrom<&Url> for BitwardenConfig { } } "field" => config.default_field = Some(value.into_owned()), + "timeout" => { + if let Ok(timeout) = value.parse::() { + config.cli_timeout = Some(timeout); + } + } _ => {} // Ignore unknown parameters } } @@ -749,6 +775,383 @@ impl BitwardenProvider { Self { config } } + /// Helper to convert any string-like type to SecretString. + /// + /// This centralizes the conversion logic and uses AsRef to accept + /// various string types (&str, String, &String, etc.) efficiently. + pub(crate) fn to_secret_string>(value: S) -> SecretString { + SecretString::new(value.as_ref().into()) + } + + /// Helper to convert Option to Option. + /// + /// This reduces the repetitive pattern of `.map(|s| SecretString::new(s.as_str().into()))` + /// to a more concise and efficient `.and_then(Self::option_to_secret_string)`. + pub(crate) fn option_to_secret_string>(opt: Option) -> Option { + opt.map(Self::to_secret_string) + } + + /// Gets the CLI timeout value from configuration or environment variable. + /// + /// Priority: environment variable > config value > default (30s) + pub(crate) fn get_cli_timeout(&self) -> Duration { + // Check environment variable first + if let Ok(timeout_str) = std::env::var("BITWARDEN_CLI_TIMEOUT") { + if let Ok(timeout_secs) = timeout_str.parse::() { + return Duration::from_secs(timeout_secs); + } + } + + // Use config value or default + let timeout_secs = self.config.cli_timeout.unwrap_or(30); + Duration::from_secs(timeout_secs) + } + + /// Sanitizes CLI error messages to prevent secret leakage. + /// + /// This method removes or redacts potential secrets from error messages while + /// preserving useful diagnostic information for users. + /// + /// # Security Considerations + /// + /// CLI error messages can sometimes contain: + /// - Access tokens or session keys in curl/HTTP error messages + /// - Secret values in JSON parsing errors + /// - File paths that might reveal sensitive information + /// - Command arguments that contain secrets + /// + /// # Arguments + /// + /// * `error_msg` - The raw error message from CLI stderr + /// + /// # Returns + /// + /// A sanitized error message safe for logging and user display + pub(crate) fn sanitize_error_message(&self, error_msg: &str) -> String { + let mut sanitized = error_msg.to_string(); + + // Redact file paths first to avoid false positives with secret patterns + sanitized = self.redact_file_paths(sanitized); + sanitized = self.redact_secret_patterns(sanitized); + sanitized = self.redact_bearer_tokens(sanitized); + sanitized = self.redact_base64_tokens(sanitized); + sanitized = self.truncate_long_message(sanitized); + + sanitized + } + + /// Redacts potential secret patterns in JSON/key-value formats. + pub(crate) fn redact_secret_patterns(&self, mut sanitized: String) -> String { + let secret_patterns = [ + // JSON patterns: "token": "value", "key": "value" + ("\"token\":", "\"[REDACTED]\""), + ("\"key\":", "\"[REDACTED]\""), + ("\"secret\":", "\"[REDACTED]\""), + ("\"password\":", "\"[REDACTED]\""), + ("\"session\":", "\"[REDACTED]\""), + ("\"access_token\":", "\"[REDACTED]\""), + ("\"api_key\":", "\"[REDACTED]\""), + ("\"authorization\":", "\"[REDACTED]\""), + ("\"bearer\":", "\"[REDACTED]\""), + ("\"jwt\":", "\"[REDACTED]\""), + ("\"refresh_token\":", "\"[REDACTED]\""), + ("\"client_secret\":", "\"[REDACTED]\""), + ("\"private_key\":", "\"[REDACTED]\""), + ("\"client_id\":", "\"[REDACTED]\""), + // URL/form patterns: token=value, key=value + ("token=", "token=[REDACTED]"), + ("key=", "key=[REDACTED]"), + ("secret=", "secret=[REDACTED]"), + ("password=", "password=[REDACTED]"), + ("session=", "session=[REDACTED]"), + ("access_token=", "access_token=[REDACTED]"), + ("api_key=", "api_key=[REDACTED]"), + ("authorization=", "authorization=[REDACTED]"), + ("bearer=", "bearer=[REDACTED]"), + ("jwt=", "jwt=[REDACTED]"), + ("refresh_token=", "refresh_token=[REDACTED]"), + ("client_secret=", "client_secret=[REDACTED]"), + ("private_key=", "private_key=[REDACTED]"), + ("client_id=", "client_id=[REDACTED]"), + // Error message patterns: "token: value", "key: value" + ("token: ", "token: [REDACTED]"), + ("key: ", "key: [REDACTED]"), + ("secret: ", "secret: [REDACTED]"), + ("password: ", "password: [REDACTED]"), + ("session: ", "session: [REDACTED]"), + ("access_token: ", "access_token: [REDACTED]"), + ("api_key: ", "api_key: [REDACTED]"), + ("authorization: ", "authorization: [REDACTED]"), + ("bearer: ", "bearer: [REDACTED]"), + ("jwt: ", "jwt: [REDACTED]"), + ("refresh_token: ", "refresh_token: [REDACTED]"), + ("client_secret: ", "client_secret: [REDACTED]"), + ("private_key: ", "private_key: [REDACTED]"), + ("client_id: ", "client_id: [REDACTED]"), + ]; + + for (pattern, replacement) in &secret_patterns { + if let Some(start) = sanitized.to_lowercase().find(&pattern.to_lowercase()) { + let value_start = start + pattern.len(); + if let Some(value_part) = sanitized.get(value_start..) { + // Skip whitespace and quotes to get to the actual value + let mut actual_value_start = 0; + for (i, ch) in value_part.char_indices() { + if ch != ' ' && ch != '"' { + actual_value_start = i; + break; + } + } + + if let Some(actual_value) = value_part.get(actual_value_start..) { + // Find end of value (quote, comma, newline, brace, bracket) + // Don't break on spaces for values like "Bearer token123" + let end_chars = ['"', ',', '\n', '\r', '}', ']']; + let mut end_pos = actual_value.len(); + + for &end_char in &end_chars { + if let Some(pos) = actual_value.find(end_char) { + if pos < end_pos { + end_pos = pos; + } + } + } + + // Only redact if value looks like a secret (>= 8 chars) + if end_pos >= 8 { + let before = &sanitized[..value_start + actual_value_start]; + let after = &sanitized[value_start + actual_value_start + end_pos..]; + sanitized = format!("{}{}{}", before, replacement, after); + } + } + } + } + } + + sanitized + } + + /// Redacts Bearer tokens from error messages. + pub(crate) fn redact_bearer_tokens(&self, mut sanitized: String) -> String { + if let Some(bearer_start) = sanitized.to_lowercase().find("bearer ") { + let token_start = bearer_start + 7; + if let Some(token_part) = sanitized.get(token_start..) { + let token_end = token_part.find(' ').unwrap_or(token_part.len().min(60)); + if token_end >= 20 { + // Typical token length + let before = &sanitized[..token_start]; + let after = &sanitized[token_start + token_end..]; + sanitized = format!("{}[REDACTED]{}", before, after); + } + } + } + sanitized + } + + /// Redacts long base64-like strings (potential tokens/keys). + pub(crate) fn redact_base64_tokens(&self, sanitized: String) -> String { + let words: Vec = sanitized + .split_whitespace() + .map(|word| { + // Check for JWT pattern (base64.base64.base64 or base64.base64) + if word.contains('.') && word.len() >= 20 { + let parts: Vec<&str> = word.split('.').collect(); + if parts.len() >= 2 && parts.iter().all(|part| { + part.len() >= 4 && part.chars().all(|c| c.is_alphanumeric() || c == '+' || c == '/' || c == '=' || c == '-' || c == '_') + }) { + return "[REDACTED]".to_string(); + } + } + + // Check for regular base64-like strings + if word.len() >= 20 + && word.chars().all(|c| c.is_alphanumeric() || c == '+' || c == '/' || c == '=' || c == '-' || c == '_') + && !word.chars().all(|c| c.is_ascii_digit()) // Don't redact pure numbers + && !word.chars().all(|c| c == word.chars().next().unwrap()) // Don't redact repeated chars + { + "[REDACTED]".to_string() + } else { + word.to_string() + } + }) + .collect(); + words.join(" ") + } + + /// Redacts sensitive file paths while preserving filenames for debugging. + pub(crate) fn redact_file_paths(&self, sanitized: String) -> String { + let words: Vec = sanitized + .split_whitespace() + .map(|word| { + // Unix paths: /path/to/file + if word.starts_with('/') && word.matches('/').count() >= 2 { + if let Some(filename) = word.split('/').last() { + if !filename.is_empty() { + format!(".../{}", filename) + } else { + "[PATH_REDACTED]".to_string() + } + } else { + "[PATH_REDACTED]".to_string() + } + } + // Windows paths: C:\path\to\file or \\server\share\file + else if (word.len() >= 3 + && word.chars().nth(1) == Some(':') + && word.chars().nth(2) == Some('\\')) + || word.starts_with("\\\\") + { + if let Some(filename) = word.split('\\').last() { + if !filename.is_empty() && filename != word { + format!("...\\{}", filename) + } else { + "[PATH_REDACTED]".to_string() + } + } else { + "[PATH_REDACTED]".to_string() + } + } else { + word.to_string() + } + }) + .collect(); + words.join(" ") + } + + /// Truncates overly long error messages for security and readability. + pub(crate) fn truncate_long_message(&self, mut sanitized: String) -> String { + if sanitized.len() > 500 { + sanitized.truncate(450); + sanitized.push_str("... [truncated for security]"); + } + sanitized + } + + /// Executes a command with timeout using cross-platform approach. + /// + /// This method implements proper timeout handling using threads and channels + /// to prevent CLI commands from hanging indefinitely. It attempts to minimize + /// resource leaks by using detached threads for command execution. + /// + /// # Arguments + /// + /// * `cmd` - The command to execute with configured arguments + /// + /// # Returns + /// + /// * `Result` - The command output or timeout error + /// + /// # Errors + /// + /// Returns errors for: + /// - Command timeout (based on configuration) + /// - CLI not installed + /// - Command execution failures + /// + /// # Resource Management + /// + /// When a timeout occurs, the spawned thread may continue running until the + /// CLI command completes. This is a known limitation of cross-platform timeout + /// implementation without external process management dependencies. + fn execute_command_with_timeout(&self, mut cmd: Command) -> Result { + use std::sync::{Arc, Mutex, mpsc}; + use std::thread; + use std::time::Instant; + + let timeout = self.get_cli_timeout(); + let (tx, rx) = mpsc::channel(); + + // Use Arc>> to potentially store process handle for future cleanup + let process_handle: Arc>> = Arc::new(Mutex::new(None)); + let process_handle_clone = Arc::clone(&process_handle); + + // Spawn command execution in separate thread + let handle = thread::spawn(move || { + // Configure command for potential process management + let child = match cmd.spawn() { + Ok(mut child) => { + // Store the child process handle for potential cleanup + if let Ok(mut handle_guard) = process_handle_clone.lock() { + *handle_guard = Some(child); + } else { + // If we can't store the handle, kill the child and return error + let _ = child.kill(); + let _ = tx.send(Err(std::io::Error::new( + std::io::ErrorKind::Other, + "Failed to manage process handle", + ))); + return; + } + + // Get the child from the mutex to wait for it + let child = process_handle_clone.lock().unwrap().take().unwrap(); + child + } + Err(e) => { + let _ = tx.send(Err(e)); + return; + } + }; + + // Wait for the process to complete and collect output + let result = child.wait_with_output(); + + // Send result through channel (ignore send errors if receiver dropped) + let _ = tx.send(result); + }); + + let _start_time = Instant::now(); + + // Wait for either completion or timeout + match rx.recv_timeout(timeout) { + Ok(Ok(output)) => { + // Command completed successfully, thread will naturally terminate + let _ = handle.join(); // Clean up the thread + Ok(output) + } + Ok(Err(e)) => { + // Command execution failed, thread will naturally terminate + let _ = handle.join(); // Clean up the thread + + if e.kind() == std::io::ErrorKind::NotFound { + Err(SecretSpecError::ProviderOperationFailed( + "Bitwarden CLI is not installed. Please install it and ensure it's in your PATH.".to_string(), + )) + } else { + Err(SecretSpecError::ProviderOperationFailed(format!( + "Command execution failed: {}", + e + ))) + } + } + Err(mpsc::RecvTimeoutError::Timeout) => { + // Attempt to kill the process if we still have the handle + if let Ok(mut handle_guard) = process_handle.lock() { + if let Some(ref mut child) = handle_guard.as_mut() { + let _ = child.kill(); // Best effort to stop the process + let _ = child.wait(); // Clean up zombie process + } + } + + // Note: The thread may still be running briefly, but the process should be killed + // We don't join the thread here to avoid blocking on the cleanup + Err(SecretSpecError::ProviderOperationFailed(format!( + "Bitwarden CLI command timed out after {} seconds. You can increase the timeout with BITWARDEN_CLI_TIMEOUT environment variable.", + timeout.as_secs() + ))) + } + Err(mpsc::RecvTimeoutError::Disconnected) => { + // Thread panicked or channel closed unexpectedly + // Try to clean up if possible + let _ = handle.join(); + + Err(SecretSpecError::ProviderOperationFailed( + "Command execution failed: internal error".to_string(), + )) + } + } + } + /// Executes a Bitwarden Password Manager CLI command with proper error handling. /// /// This method handles: @@ -772,6 +1175,13 @@ impl BitwardenProvider { /// - Authentication required (not logged in or unlocked) /// - Command execution failures fn execute_bw_command(&self, args: &[&str]) -> Result { + // Performance timing if enabled + let start_time = if std::env::var("SECRETSPEC_PERF_LOG").is_ok() { + Some(Instant::now()) + } else { + None + }; + let mut cmd = Command::new("bw"); // Configure server if specified @@ -781,15 +1191,15 @@ impl BitwardenProvider { cmd.args(args); - let output = match cmd.output() { - Ok(output) => output, - Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + let output = self.execute_command_with_timeout(cmd).or_else(|e| { + // Provide more specific error message for CLI not found + if e.to_string().contains("not installed") { return Err(SecretSpecError::ProviderOperationFailed( "Bitwarden CLI (bw) is not installed.\n\nTo install it:\n - npm: npm install -g @bitwarden/cli\n - Homebrew: brew install bitwarden-cli\n - Chocolatey: choco install bitwarden-cli\n - Download: https://bitwarden.com/help/cli/\n\nAfter installation, run 'bw login' and 'bw unlock' to authenticate.".to_string(), )); } - Err(e) => return Err(e.into()), - }; + Err(e) + })?; if !output.status.success() { let error_msg = String::from_utf8_lossy(&output.stderr); @@ -807,12 +1217,26 @@ impl BitwardenProvider { } return Err(SecretSpecError::ProviderOperationFailed( - error_msg.to_string(), + self.sanitize_error_message(&error_msg), )); } - String::from_utf8(output.stdout) - .map_err(|e| SecretSpecError::ProviderOperationFailed(e.to_string())) + let result = String::from_utf8(output.stdout).map_err(|e| { + SecretSpecError::ProviderOperationFailed(self.sanitize_error_message(&e.to_string())) + }); + + // Log performance timing if enabled + if let Some(start) = start_time { + let duration = start.elapsed(); + eprintln!( + "[PERF] bw {} took {:?} ({}ms)", + args.join(" "), + duration, + duration.as_millis() + ); + } + + result } /// Executes a Bitwarden Secrets Manager CLI command with proper error handling. @@ -840,6 +1264,13 @@ impl BitwardenProvider { /// - Rate limiting issues /// - Command execution failures fn execute_bws_command(&self, args: &[&str]) -> Result { + // Performance timing if enabled + let start_time = if std::env::var("SECRETSPEC_PERF_LOG").is_ok() { + Some(Instant::now()) + } else { + None + }; + let mut cmd = Command::new("bws"); // Configure access token - check config first, then environment variable @@ -851,15 +1282,15 @@ impl BitwardenProvider { cmd.args(args); - let output = match cmd.output() { - Ok(output) => output, - Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + let output = self.execute_command_with_timeout(cmd).or_else(|e| { + // Provide more specific error message for CLI not found + if e.to_string().contains("not installed") { return Err(SecretSpecError::ProviderOperationFailed( "Bitwarden Secrets Manager CLI (bws) is not installed.\n\nTo install it:\n - Cargo: cargo install bws\n - Script: curl -sSL https://bitwarden.com/secrets/install | sh\n - Download: https://github.com/bitwarden/sdk-sm/releases\n\nAfter installation, set BWS_ACCESS_TOKEN environment variable with your access token.".to_string(), )); } - Err(e) => return Err(e.into()), - }; + Err(e) + })?; if !output.status.success() { let error_msg = String::from_utf8_lossy(&output.stderr); @@ -887,12 +1318,26 @@ impl BitwardenProvider { return Err(SecretSpecError::ProviderOperationFailed(format!( "Bitwarden Secrets Manager CLI error: {}", - error_msg + self.sanitize_error_message(&error_msg) ))); } - String::from_utf8(output.stdout) - .map_err(|e| SecretSpecError::ProviderOperationFailed(e.to_string())) + let result = String::from_utf8(output.stdout).map_err(|e| { + SecretSpecError::ProviderOperationFailed(self.sanitize_error_message(&e.to_string())) + }); + + // Log performance timing if enabled + if let Some(start) = start_time { + let duration = start.elapsed(); + eprintln!( + "[PERF] bws {} took {:?} ({}ms)", + args.join(" "), + duration, + duration.as_millis() + ); + } + + result } /// Checks if the user is authenticated with Bitwarden. @@ -981,6 +1426,7 @@ impl BitwardenProvider { /// # Returns /// /// A BitwardenItemTemplate ready for serialization + #[allow(dead_code)] fn create_item_template( &self, _project: &str, @@ -1023,9 +1469,9 @@ impl BitwardenProvider { /// extracting values using smart field detection. fn get_from_password_manager( &self, - project: &str, + _project: &str, key: &str, - profile: &str, + _profile: &str, ) -> Result> { // Check authentication status first if !self.is_authenticated()? { @@ -1034,8 +1480,6 @@ impl BitwardenProvider { )); } - eprintln!("DEBUG: get_from_password_manager called for key='{}'", key); - // Use Bitwarden's built-in search to find items matching the key let mut list_args = vec!["list", "items", "--search", key]; @@ -1048,7 +1492,39 @@ impl BitwardenProvider { } let output = self.execute_bw_command(&list_args)?; - let items: Vec = serde_json::from_str(&output)?; + + // Handle empty output (no search results) + let items: Vec = if output.trim().is_empty() { + Vec::new() + } else { + // Performance timing for JSON parsing (equivalent to jq processing) + let parse_start = if std::env::var("SECRETSPEC_PERF_LOG").is_ok() { + Some(std::time::Instant::now()) + } else { + None + }; + + let items: Vec = serde_json::from_str(&output).map_err(|e| { + SecretSpecError::ProviderOperationFailed(format!( + "Failed to parse Bitwarden search results: {}. Output was: '{}'", + e, + output.chars().take(100).collect::() + )) + })?; + + // Log JSON parsing performance (equivalent to jq timing) + if let Some(start) = parse_start { + let duration = start.elapsed(); + eprintln!( + "[PERF] JSON parse took {}μs for {} items ({}B)", + duration.as_micros(), + items.len(), + output.len() + ); + } + + items + }; // If we found items, use the first one (Bitwarden's search is already good) if let Some(item) = items.first() { @@ -1103,13 +1579,19 @@ impl BitwardenProvider { // If specific field requested, try to find it if let Some(field_name) = requested_field { match field_name.to_lowercase().as_str() { - "password" => return Ok(login.password.as_ref().map(|p| SecretString::new(p.clone().into()))), - "username" => return Ok(login.username.as_ref().map(|u| SecretString::new(u.clone().into()))), - "totp" => return Ok(login.totp.as_ref().map(|t| SecretString::new(t.clone().into()))), + "password" => { + return Ok(Self::option_to_secret_string(login.password.as_deref())); + } + "username" => { + return Ok(Self::option_to_secret_string(login.username.as_deref())); + } + "totp" => { + return Ok(Self::option_to_secret_string(login.totp.as_deref())); + } _ => { // Check custom fields for requested field name if let Some(value) = self.extract_from_custom_fields(item, field_name)? { - return Ok(Some(SecretString::new(value.into()))); + return Ok(Some(Self::to_secret_string(value))); } else { return Ok(None); } @@ -1125,13 +1607,13 @@ impl BitwardenProvider { || hint_lower.contains("token") { if let Some(password) = &login.password { - return Ok(Some(SecretString::new(password.clone().into()))); + return Ok(Some(Self::to_secret_string(password))); } } if hint_lower.contains("user") || hint_lower.contains("login") { if let Some(username) = &login.username { - return Ok(Some(SecretString::new(username.clone().into()))); + return Ok(Some(Self::to_secret_string(username))); } } @@ -1140,16 +1622,16 @@ impl BitwardenProvider { || hint_lower.contains("mfa") { if let Some(totp) = &login.totp { - return Ok(Some(SecretString::new(totp.clone().into()))); + return Ok(Some(Self::to_secret_string(totp))); } } // Default: prefer password, then username if let Some(password) = &login.password { - return Ok(Some(SecretString::new(password.clone().into()))); + return Ok(Some(Self::to_secret_string(password))); } if let Some(username) = &login.username { - return Ok(Some(SecretString::new(username.clone().into()))); + return Ok(Some(Self::to_secret_string(username))); } } @@ -1171,22 +1653,22 @@ impl BitwardenProvider { // If specific field requested, check custom fields first if let Some(field_name) = requested_field { if let Some(value) = self.extract_from_custom_fields(item, field_name)? { - return Ok(Some(SecretString::new(value.into()))); + return Ok(Some(Self::to_secret_string(value))); } } // Look for legacy "value" field (backward compatibility) if let Some(value) = self.extract_from_custom_fields(item, "value")? { - return Ok(Some(SecretString::new(value.into()))); + return Ok(Some(Self::to_secret_string(value))); } // Look for field matching the hint if let Some(value) = self.extract_from_custom_fields(item, field_hint)? { - return Ok(Some(SecretString::new(value.into()))); + return Ok(Some(Self::to_secret_string(value))); } // Fallback: return notes content - Ok(item.notes.as_ref().map(|notes| SecretString::new(notes.clone().into()))) + Ok(Self::option_to_secret_string(item.notes.as_deref())) } /// Extracts value from Card item (type 3). @@ -1200,15 +1682,29 @@ impl BitwardenProvider { // If specific field requested if let Some(field_name) = requested_field { match field_name.to_lowercase().as_str() { - "number" => return Ok(card.number.as_ref().map(|n| SecretString::new(n.clone().into()))), - "code" | "cvv" | "cvc" => return Ok(card.code.as_ref().map(|c| SecretString::new(c.clone().into()))), - "cardholder" | "name" => return Ok(card.cardholder_name.as_ref().map(|n| SecretString::new(n.clone().into()))), - "brand" => return Ok(card.brand.as_ref().map(|b| SecretString::new(b.clone().into()))), - "expmonth" | "exp_month" => return Ok(card.exp_month.as_ref().map(|m| SecretString::new(m.clone().into()))), - "expyear" | "exp_year" => return Ok(card.exp_year.as_ref().map(|y| SecretString::new(y.clone().into()))), + "number" => { + return Ok(Self::option_to_secret_string(card.number.as_deref())); + } + "code" | "cvv" | "cvc" => { + return Ok(Self::option_to_secret_string(card.code.as_deref())); + } + "cardholder" | "name" => { + return Ok(Self::option_to_secret_string( + card.cardholder_name.as_deref(), + )); + } + "brand" => { + return Ok(Self::option_to_secret_string(card.brand.as_deref())); + } + "expmonth" | "exp_month" => { + return Ok(Self::option_to_secret_string(card.exp_month.as_deref())); + } + "expyear" | "exp_year" => { + return Ok(Self::option_to_secret_string(card.exp_year.as_deref())); + } _ => { if let Some(value) = self.extract_from_custom_fields(item, field_name)? { - return Ok(Some(SecretString::new(value.into()))); + return Ok(Some(Self::to_secret_string(value))); } else { return Ok(None); } @@ -1220,7 +1716,7 @@ impl BitwardenProvider { let hint_lower = field_hint.to_lowercase(); if hint_lower.contains("number") || hint_lower.contains("card") { if let Some(number) = &card.number { - return Ok(Some(SecretString::new(number.clone().into()))); + return Ok(Some(Self::to_secret_string(number))); } } @@ -1229,13 +1725,13 @@ impl BitwardenProvider { || hint_lower.contains("cvc") { if let Some(code) = &card.code { - return Ok(Some(SecretString::new(code.clone().into()))); + return Ok(Some(Self::to_secret_string(code))); } } // Default: return card number if let Some(number) = &card.number { - return Ok(Some(SecretString::new(number.clone().into()))); + return Ok(Some(Self::to_secret_string(number))); } } @@ -1258,15 +1754,32 @@ impl BitwardenProvider { // If specific field requested if let Some(field_name) = requested_field { match field_name.to_lowercase().as_str() { - "email" => return Ok(identity.email.as_ref().map(|e| SecretString::new(e.clone().into()))), - "username" => return Ok(identity.username.as_ref().map(|u| SecretString::new(u.clone().into()))), - "phone" => return Ok(identity.phone.as_ref().map(|p| SecretString::new(p.clone().into()))), - "firstname" | "first_name" => return Ok(identity.first_name.as_ref().map(|f| SecretString::new(f.clone().into()))), - "lastname" | "last_name" => return Ok(identity.last_name.as_ref().map(|l| SecretString::new(l.clone().into()))), - "company" => return Ok(identity.company.as_ref().map(|c| SecretString::new(c.clone().into()))), + "email" => { + return Ok(identity.email.as_ref().map(Self::to_secret_string)); + } + "username" => { + return Ok(identity.username.as_ref().map(Self::to_secret_string)); + } + "phone" => { + return Ok(identity + .phone + .as_ref() + .map(|p| SecretString::new(p.clone().into()))); + } + "firstname" | "first_name" => { + return Ok(Self::option_to_secret_string( + identity.first_name.as_deref(), + )); + } + "lastname" | "last_name" => { + return Ok(Self::option_to_secret_string(identity.last_name.as_deref())); + } + "company" => { + return Ok(Self::option_to_secret_string(identity.company.as_deref())); + } _ => { if let Some(value) = self.extract_from_custom_fields(item, field_name)? { - return Ok(Some(SecretString::new(value.into()))); + return Ok(Some(Self::to_secret_string(value))); } else { return Ok(None); } @@ -1278,28 +1791,28 @@ impl BitwardenProvider { let hint_lower = field_hint.to_lowercase(); if hint_lower.contains("email") || hint_lower.contains("mail") { if let Some(email) = &identity.email { - return Ok(Some(SecretString::new(email.clone().into()))); + return Ok(Some(Self::to_secret_string(email))); } } if hint_lower.contains("phone") || hint_lower.contains("tel") { if let Some(phone) = &identity.phone { - return Ok(Some(SecretString::new(phone.clone().into()))); + return Ok(Some(Self::to_secret_string(phone))); } } if hint_lower.contains("user") || hint_lower.contains("login") { if let Some(username) = &identity.username { - return Ok(Some(SecretString::new(username.clone().into()))); + return Ok(Some(Self::to_secret_string(username))); } } // Default: prefer email, then username if let Some(email) = &identity.email { - return Ok(Some(SecretString::new(email.clone().into()))); + return Ok(Some(Self::to_secret_string(email))); } if let Some(username) = &identity.username { - return Ok(Some(SecretString::new(username.clone().into()))); + return Ok(Some(Self::to_secret_string(username))); } } @@ -1323,15 +1836,21 @@ impl BitwardenProvider { if let Some(field_name) = requested_field { match field_name.to_lowercase().as_str() { "private_key" | "privatekey" | "private" => { - return Ok(ssh_key.private_key.as_ref().map(|k| SecretString::new(k.clone().into()))); + return Ok(Self::option_to_secret_string( + ssh_key.private_key.as_deref(), + )); + } + "public_key" | "publickey" | "public" => { + return Ok(Self::option_to_secret_string(ssh_key.public_key.as_deref())); } - "public_key" | "publickey" | "public" => return Ok(ssh_key.public_key.as_ref().map(|k| SecretString::new(k.clone().into()))), "fingerprint" | "key_fingerprint" => { - return Ok(ssh_key.key_fingerprint.as_ref().map(|f| SecretString::new(f.clone().into()))); + return Ok(Self::option_to_secret_string( + ssh_key.key_fingerprint.as_deref(), + )); } _ => { if let Some(value) = self.extract_from_custom_fields(item, field_name)? { - return Ok(Some(SecretString::new(value.into()))); + return Ok(Some(Self::to_secret_string(value))); } else { return Ok(None); } @@ -1343,19 +1862,19 @@ impl BitwardenProvider { let hint_lower = field_hint.to_lowercase(); if hint_lower.contains("public") || hint_lower.contains("pub") { if let Some(public_key) = &ssh_key.public_key { - return Ok(Some(SecretString::new(public_key.clone().into()))); + return Ok(Some(Self::to_secret_string(public_key))); } } if hint_lower.contains("fingerprint") || hint_lower.contains("finger") { if let Some(fingerprint) = &ssh_key.key_fingerprint { - return Ok(Some(SecretString::new(fingerprint.clone().into()))); + return Ok(Some(Self::to_secret_string(fingerprint))); } } // Default: return private key (most common use case for SSH keys) if let Some(private_key) = &ssh_key.private_key { - return Ok(Some(SecretString::new(private_key.clone().into()))); + return Ok(Some(Self::to_secret_string(private_key))); } } @@ -1403,6 +1922,8 @@ impl BitwardenProvider { key: &str, _profile: &str, ) -> Result> { + let perf_enabled = std::env::var("SECRETSPEC_PERF_LOG").is_ok(); + // For Secrets Manager, we create a secret name based on project and key // Profile is encoded in the secret name since SM doesn't have built-in profile support let secret_name = format!("{}_{}", project, key); @@ -1415,9 +1936,33 @@ impl BitwardenProvider { args.push(project_id); } + let list_start = if perf_enabled { + Some(Instant::now()) + } else { + None + }; match self.execute_bws_command(&args) { Ok(output) => { + if let Some(start) = list_start { + eprintln!( + "[PERF] BWS secret list took {}ms", + start.elapsed().as_millis() + ); + } + + let parse_start = if perf_enabled { + Some(Instant::now()) + } else { + None + }; let secrets: Vec = serde_json::from_str(&output)?; + if let Some(start) = parse_start { + eprintln!( + "[PERF] BWS JSON parse took {}μs, {} secrets", + start.elapsed().as_micros(), + secrets.len() + ); + } // Look for a secret with matching key name for secret in secrets { @@ -1466,7 +2011,19 @@ impl BitwardenProvider { } let output = self.execute_bw_command(&list_args)?; - let items: Vec = serde_json::from_str(&output)?; + + // Handle empty output (no search results) + let items: Vec = if output.trim().is_empty() { + Vec::new() + } else { + serde_json::from_str(&output).map_err(|e| { + SecretSpecError::ProviderOperationFailed(format!( + "Failed to parse Bitwarden item list: {}. Output was: '{}'", + e, + output.chars().take(100).collect::() + )) + })? + }; // Search strategies (same as get method): // 1. Exact name match with secretspec format (for compatibility) @@ -1756,7 +2313,7 @@ impl BitwardenProvider { "Bitwarden CLI (bw) is not installed.\n\nTo install it:\n - npm: npm install -g @bitwarden/cli\n - Homebrew: brew install bitwarden-cli\n - Chocolatey: choco install bitwarden-cli\n - Download: https://bitwarden.com/help/cli/".to_string(), ) } else { - SecretSpecError::ProviderOperationFailed(e.to_string()) + SecretSpecError::ProviderOperationFailed(self.sanitize_error_message(&e.to_string())) } })?; @@ -1775,7 +2332,7 @@ impl BitwardenProvider { if !output.status.success() { let error_msg = String::from_utf8_lossy(&output.stderr); return Err(SecretSpecError::ProviderOperationFailed( - error_msg.to_string(), + self.sanitize_error_message(&error_msg), )); } @@ -2044,7 +2601,7 @@ impl BitwardenProvider { "Bitwarden CLI (bw) is not installed.\n\nTo install it:\n - npm: npm install -g @bitwarden/cli\n - Homebrew: brew install bitwarden-cli\n - Chocolatey: choco install bitwarden-cli\n - Download: https://bitwarden.com/help/cli/".to_string(), ) } else { - SecretSpecError::ProviderOperationFailed(e.to_string()) + SecretSpecError::ProviderOperationFailed(self.sanitize_error_message(&e.to_string())) } })?; @@ -2063,7 +2620,7 @@ impl BitwardenProvider { if !output.status.success() { let error_msg = String::from_utf8_lossy(&output.stderr); return Err(SecretSpecError::ProviderOperationFailed( - error_msg.to_string(), + self.sanitize_error_message(&error_msg), )); } @@ -2176,20 +2733,33 @@ impl Provider for BitwardenProvider { /// - Item retrieval failures /// - JSON parsing errors fn get(&self, project: &str, key: &str, profile: &str) -> Result> { - eprintln!( - "DEBUG: BitwardenProvider.get() called with key='{}', service={:?}", - key, self.config.service - ); - match self.config.service { + let start_time = if std::env::var("SECRETSPEC_PERF_LOG").is_ok() { + Some(Instant::now()) + } else { + None + }; + + let result = match self.config.service { BitwardenService::PasswordManager => { - eprintln!("DEBUG: Calling get_from_password_manager"); self.get_from_password_manager(project, key, profile) } BitwardenService::SecretsManager => { - eprintln!("DEBUG: Calling get_from_secrets_manager"); self.get_from_secrets_manager(project, key, profile) } + }; + + // Log performance timing if enabled + if let Some(start) = start_time { + let duration = start.elapsed(); + eprintln!( + "[PERF] get('{}') took {:?} ({}ms)", + key, + duration, + duration.as_millis() + ); } + + result } /// Stores or updates a secret in Bitwarden. @@ -2215,14 +2785,33 @@ impl Provider for BitwardenProvider { /// - Item creation/update failures /// - Temporary file creation errors fn set(&self, project: &str, key: &str, value: &SecretString, profile: &str) -> Result<()> { - match self.config.service { + let start_time = if std::env::var("SECRETSPEC_PERF_LOG").is_ok() { + Some(Instant::now()) + } else { + None + }; + + let result = match self.config.service { BitwardenService::PasswordManager => { self.set_to_password_manager(project, key, value, profile) } BitwardenService::SecretsManager => { self.set_to_secrets_manager(project, key, value, profile) } + }; + + // Log performance timing if enabled + if let Some(start) = start_time { + let duration = start.elapsed(); + eprintln!( + "[PERF] set('{}') took {:?} ({}ms)", + key, + duration, + duration.as_millis() + ); } + + result } } diff --git a/secretspec/src/provider/dotenv.rs b/secretspec/src/provider/dotenv.rs index 47191bae..a8932600 100644 --- a/secretspec/src/provider/dotenv.rs +++ b/secretspec/src/provider/dotenv.rs @@ -156,63 +156,6 @@ impl DotEnvProvider { pub fn new(config: DotEnvConfig) -> Self { Self { config } } - - /// Reflects all secrets available in the .env file as Secret entries. - /// - /// This method reads the .env file and returns all environment variables - /// as Secret entries with default descriptions and all marked as required. - /// If the file doesn't exist, returns an empty HashMap. - /// - /// # Returns - /// - /// * `Ok(HashMap)` - All environment variables as Secret - /// * `Err(SecretSpecError)` - If reading the file fails - /// - /// # Examples - /// - /// ```ignore - /// use secretspec::provider::dotenv::{DotEnvProvider, DotEnvConfig}; - /// - /// let provider = DotEnvProvider::new(DotEnvConfig::default()); - /// let secrets = provider.reflect().unwrap(); - /// for (key, config) in secrets { - /// println!("Found secret: {} - {}", key, config.description); - /// } - /// ``` - pub fn reflect(&self) -> Result> { - use crate::config::Secret; - - if !self.config.path.exists() { - return Ok(HashMap::new()); - } - - // Check if path is a directory - if self.config.path.is_dir() { - return Err(SecretSpecError::Io(std::io::Error::new( - std::io::ErrorKind::IsADirectory, - format!( - "Expected file but found directory: {}", - self.config.path.display() - ), - ))); - } - - let mut secrets = HashMap::new(); - let env_vars = dotenvy::from_path_iter(&self.config.path)?; - for item in env_vars { - let (key, _value) = item?; - secrets.insert( - key.clone(), - Secret { - description: Some(format!("{} secret", key)), - required: true, - default: None, - }, - ); - } - - Ok(secrets) - } } impl Provider for DotEnvProvider { @@ -306,6 +249,41 @@ impl Provider for DotEnvProvider { fs::write(&self.config.path, content)?; Ok(()) } + + fn reflect(&self) -> Result> { + use crate::config::Secret; + + if !self.config.path.exists() { + return Ok(HashMap::new()); + } + + // Check if path is a directory + if self.config.path.is_dir() { + return Err(SecretSpecError::Io(std::io::Error::new( + std::io::ErrorKind::IsADirectory, + format!( + "Expected file but found directory: {}", + self.config.path.display() + ), + ))); + } + + let mut secrets = HashMap::new(); + let env_vars = dotenvy::from_path_iter(&self.config.path)?; + for item in env_vars { + let (key, _value) = item?; + secrets.insert( + key.clone(), + Secret { + description: Some(format!("{} secret", key)), + required: true, + default: None, + }, + ); + } + + Ok(secrets) + } } #[cfg(test)] diff --git a/secretspec/src/provider/mod.rs b/secretspec/src/provider/mod.rs index 882487c7..efdf9829 100644 --- a/secretspec/src/provider/mod.rs +++ b/secretspec/src/provider/mod.rs @@ -54,6 +54,7 @@ use crate::{Result, SecretSpecError}; use secrecy::SecretString; +use std::collections::HashMap; use std::convert::TryFrom; use url::Url; @@ -237,6 +238,32 @@ pub trait Provider: Send + Sync { /// /// This should match the name registered with the provider macro. fn name(&self) -> &'static str; + + /// Discovers and returns all secrets available in this provider. + /// + /// This method is used to introspect the provider and find all available secrets. + /// It's particularly useful for importing secrets from external sources. + /// + /// # Returns + /// + /// A HashMap where keys are secret names and values are `Secret` configurations. + /// The default implementation returns an empty map, indicating the provider + /// doesn't support reflection. + /// + /// # Example + /// + /// ```rust,ignore + /// let secrets = provider.reflect()?; + /// for (name, secret) in secrets { + /// println!("Found secret: {} = {:?}", name, secret); + /// } + /// ``` + fn reflect(&self) -> Result> { + Err(SecretSpecError::ProviderOperationFailed(format!( + "Provider '{}' does not support reflection", + self.name() + ))) + } } impl TryFrom for Box { diff --git a/secretspec/src/provider/tests.rs b/secretspec/src/provider/tests.rs index 44cebdf6..82f7b723 100644 --- a/secretspec/src/provider/tests.rs +++ b/secretspec/src/provider/tests.rs @@ -313,6 +313,26 @@ fn test_bitwarden_config_parsing() { assert_eq!(config.service, BitwardenService::SecretsManager); assert_eq!(config.default_item_type, Some(BitwardenItemType::Login)); assert_eq!(config.default_field, Some("password".to_string())); + + // Test timeout configuration + let url = Url::parse("bitwarden://?timeout=60").unwrap(); + let config = BitwardenConfig::try_from(&url).unwrap(); + assert_eq!(config.service, BitwardenService::PasswordManager); + assert_eq!(config.cli_timeout, Some(60)); + + // Test timeout configuration with other parameters + let url = Url::parse("bws://?project=test&timeout=45&field=password").unwrap(); + let config = BitwardenConfig::try_from(&url).unwrap(); + assert_eq!(config.service, BitwardenService::SecretsManager); + assert_eq!(config.project_id, Some("test".to_string())); + assert_eq!(config.cli_timeout, Some(45)); + assert_eq!(config.default_field, Some("password".to_string())); + + // Test invalid timeout value is ignored + let url = Url::parse("bitwarden://?timeout=invalid").unwrap(); + let config = BitwardenConfig::try_from(&url).unwrap(); + assert_eq!(config.service, BitwardenService::PasswordManager); + assert_eq!(config.cli_timeout, Some(30)); // Should use default } #[test] @@ -499,6 +519,102 @@ fn test_bitwarden_environment_variables() { } } +#[test] +fn test_bitwarden_timeout_configuration() { + use crate::provider::bitwarden::{BitwardenConfig, BitwardenProvider}; + use std::env; + use std::time::Duration; + + // Test default timeout + let config = BitwardenConfig::default(); + let provider = BitwardenProvider::new(config); + assert_eq!(provider.get_cli_timeout(), Duration::from_secs(30)); + + // Test timeout from configuration + let mut config = BitwardenConfig::default(); + config.cli_timeout = Some(45); + let provider = BitwardenProvider::new(config); + assert_eq!(provider.get_cli_timeout(), Duration::from_secs(45)); + + // Test environment variable override + unsafe { + env::set_var("BITWARDEN_CLI_TIMEOUT", "60"); + } + + let config = BitwardenConfig::default(); + let provider = BitwardenProvider::new(config); + assert_eq!(provider.get_cli_timeout(), Duration::from_secs(60)); + + // Clean up + unsafe { + env::remove_var("BITWARDEN_CLI_TIMEOUT"); + } +} + +#[test] +fn test_bitwarden_error_message_sanitization() { + use crate::provider::bitwarden::{BitwardenConfig, BitwardenProvider}; + + let config = BitwardenConfig::default(); + let provider = BitwardenProvider::new(config); + + // Test JSON token redaction + let error_with_token = r#"{"error": "authentication failed", "token": "test_secret_token_12345678901234567890", "code": 401}"#; + let sanitized = provider.sanitize_error_message(error_with_token); + assert!(!sanitized.contains("test_secret_token_12345678901234567890")); + assert!(sanitized.contains("\"[REDACTED]\"")); + + // Test Bearer token redaction + let error_with_bearer = "HTTP 401: Bearer eyJ0eXAiOiJKV1QiLnothinghere invalid or expired"; + let sanitized = provider.sanitize_error_message(error_with_bearer); + assert!(!sanitized.contains("eyJ0eXAiOiJKV1QiLnothinghere")); + assert!(sanitized.contains("Bearer [REDACTED]")); + + // Test password redaction + let error_with_password = r#"{"password": "supersecretpassword123", "username": "user"}"#; + let sanitized = provider.sanitize_error_message(error_with_password); + assert!(!sanitized.contains("supersecretpassword123")); + assert!(sanitized.contains("\"[REDACTED]\"")); + + // Test URL parameter redaction + let error_with_url_params = "Failed to authenticate: token=abc123def456ghi789jkl012 expired"; + let sanitized = provider.sanitize_error_message(error_with_url_params); + assert!(!sanitized.contains("abc123def456ghi789jkl012")); + assert!(sanitized.contains("token=[REDACTED]")); + + // Test long base64-like string redaction + let error_with_base64 = + "Session YWJjZGVmZ2hpamtsbW5vcHFyc3R1dnd4eXphYmNkZWZnaGlqa2xtbm9wcXJzdHV2d3h5eg== expired"; + let sanitized = provider.sanitize_error_message(error_with_base64); + assert!( + !sanitized + .contains("YWJjZGVmZ2hpamtsbW5vcHFyc3R1dnd4eXphYmNkZWZnaGlqa2xtbm9wcXJzdHV2d3h5eg==") + ); + assert!(sanitized.contains("[REDACTED]")); + + // Test file path redaction + let error_with_path = "Cannot read /home/user/.config/bitwarden/session.json"; + let sanitized = provider.sanitize_error_message(error_with_path); + assert!(!sanitized.contains("/home/user/.config/bitwarden/session.json")); + assert!(sanitized.contains(".../session.json")); + + // Test short values are NOT redacted (to avoid false positives) + let error_with_short_values = r#"{"key": "short", "status": "ok"}"#; + let sanitized = provider.sanitize_error_message(error_with_short_values); + assert!(sanitized.contains("short")); // Should not be redacted + + // Test normal error messages are preserved + let normal_error = "Vault is locked. Please unlock with bw unlock."; + let sanitized = provider.sanitize_error_message(normal_error); + assert_eq!(sanitized, normal_error); + + // Test message truncation for very long messages + let long_message = "A".repeat(600); + let sanitized = provider.sanitize_error_message(&long_message); + assert!(sanitized.len() <= 500); + assert!(sanitized.ends_with("... [truncated for security]")); +} + // Integration tests for all providers #[cfg(test)] mod integration_tests { @@ -719,6 +835,13 @@ mod integration_tests { #[test] fn test_bitwarden_authentication_states() { + // Only run this test if SECRETSPEC_TEST_PROVIDERS includes bitwarden + let providers = get_test_providers(); + if !providers.contains(&"bitwarden".to_string()) { + println!("Skipping bitwarden authentication test - not in SECRETSPEC_TEST_PROVIDERS"); + return; + } + // Test that we get proper error messages for different authentication states let provider = Box::::try_from("bitwarden://") .expect("Should create bitwarden provider"); @@ -757,6 +880,13 @@ mod integration_tests { #[test] fn test_bitwarden_error_messages() { + // Only run this test if SECRETSPEC_TEST_PROVIDERS includes bitwarden + let providers = get_test_providers(); + if !providers.contains(&"bitwarden".to_string()) { + println!("Skipping bitwarden error messages test - not in SECRETSPEC_TEST_PROVIDERS"); + return; + } + use crate::provider::bitwarden::BitwardenProvider; // Test that we get helpful error messages @@ -850,4 +980,686 @@ mod integration_tests { } } } + + #[test] + fn test_concurrent_access_to_mock_provider() { + use std::sync::Arc; + use std::thread; + use std::time::Duration; + + let provider = Arc::new(MockProvider::new()); + let num_threads = 10; + let operations_per_thread = 50; + + // Set up initial data + let project = "concurrent-test"; + let profile = "default"; + + // Create threads that perform concurrent read/write operations + let mut handles = vec![]; + + for thread_id in 0..num_threads { + let provider = Arc::clone(&provider); + + let handle = thread::spawn(move || { + for op_id in 0..operations_per_thread { + let key = format!("key-{}-{}", thread_id, op_id); + let value = format!("value-{}-{}", thread_id, op_id); + + // Set the value + let secret = SecretString::new(value.clone().into()); + provider.set(project, &key, &secret, profile).unwrap(); + + // Small delay to increase chance of race conditions + thread::sleep(Duration::from_millis(1)); + + // Get the value back + let retrieved = provider.get(project, &key, profile).unwrap(); + assert!(retrieved.is_some(), "Key {} should exist", key); + assert_eq!( + retrieved.unwrap().expose_secret(), + &value, + "Value mismatch for key {}", + key + ); + } + }); + + handles.push(handle); + } + + // Wait for all threads to complete + for handle in handles { + handle.join().unwrap(); + } + + // Verify final state - should have all keys + let expected_total = num_threads * operations_per_thread; + let storage = provider.storage.lock().unwrap(); + let actual_count = storage.len(); + + assert_eq!( + actual_count, expected_total, + "Expected {} keys but found {}", + expected_total, actual_count + ); + + println!( + "✓ Concurrent access test passed: {} threads × {} operations = {} total keys", + num_threads, operations_per_thread, actual_count + ); + } + + #[test] + fn test_concurrent_read_heavy_workload() { + use std::sync::Arc; + use std::thread; + use std::time::Instant; + + let provider = Arc::new(MockProvider::new()); + let project = "read-heavy-test"; + let profile = "default"; + + // Pre-populate with test data + let num_keys = 100; + for i in 0..num_keys { + let key = format!("key-{}", i); + let value = format!("value-{}", i); + let secret = SecretString::new(value.into()); + provider.set(project, &key, &secret, profile).unwrap(); + } + + let num_reader_threads = 20; + let reads_per_thread = 200; + let start_time = Instant::now(); + + let mut handles = vec![]; + + for thread_id in 0..num_reader_threads { + let provider = Arc::clone(&provider); + + let handle = thread::spawn(move || { + for read_id in 0..reads_per_thread { + let key_index = (thread_id + read_id) % num_keys; + let key = format!("key-{}", key_index); + let expected_value = format!("value-{}", key_index); + + let result = provider.get(project, &key, profile).unwrap(); + assert!(result.is_some(), "Key {} should exist", key); + assert_eq!( + result.unwrap().expose_secret(), + &expected_value, + "Value mismatch for key {}", + key + ); + } + }); + + handles.push(handle); + } + + // Wait for all readers to complete + for handle in handles { + handle.join().unwrap(); + } + + let elapsed = start_time.elapsed(); + let total_reads = num_reader_threads * reads_per_thread; + let reads_per_second = total_reads as f64 / elapsed.as_secs_f64(); + + println!( + "✓ Read-heavy test: {} reads in {:?} ({:.0} reads/sec)", + total_reads, elapsed, reads_per_second + ); + + // Performance assertion - should handle at least 1000 reads/sec + assert!( + reads_per_second > 1000.0, + "Performance too slow: {:.0} reads/sec (expected > 1000)", + reads_per_second + ); + } + + #[test] + fn test_mixed_concurrent_workload() { + use std::sync::Arc; + use std::thread; + use std::time::{Duration, Instant}; + + let provider = Arc::new(MockProvider::new()); + let project = "mixed-workload-test"; + let profile = "default"; + + // Pre-populate with some initial data + for i in 0..50 { + let key = format!("initial-key-{}", i); + let value = format!("initial-value-{}", i); + let secret = SecretString::new(value.into()); + provider.set(project, &key, &secret, profile).unwrap(); + } + + let num_writer_threads = 5; + let num_reader_threads = 15; + let operations_per_thread = 100; + let start_time = Instant::now(); + + let mut handles = vec![]; + + // Writer threads + for thread_id in 0..num_writer_threads { + let provider = Arc::clone(&provider); + + let handle = thread::spawn(move || { + for op_id in 0..operations_per_thread { + let key = format!("writer-{}-key-{}", thread_id, op_id); + let value = format!("writer-{}-value-{}", thread_id, op_id); + let secret = SecretString::new(value.into()); + + provider.set(project, &key, &secret, profile).unwrap(); + + // Simulate some processing time + thread::sleep(Duration::from_millis(2)); + } + }); + + handles.push(handle); + } + + // Reader threads + for thread_id in 0..num_reader_threads { + let provider = Arc::clone(&provider); + + let handle = thread::spawn(move || { + for op_id in 0..operations_per_thread { + // Try to read existing keys + let key_index = (thread_id + op_id) % 50; + let key = format!("initial-key-{}", key_index); + + let result = provider.get(project, &key, profile).unwrap(); + assert!(result.is_some(), "Initial key {} should exist", key); + + // Small delay to allow more interleaving + thread::sleep(Duration::from_millis(1)); + } + }); + + handles.push(handle); + } + + // Wait for all threads to complete + for handle in handles { + handle.join().unwrap(); + } + + let elapsed = start_time.elapsed(); + let total_operations = (num_writer_threads + num_reader_threads) * operations_per_thread; + let ops_per_second = total_operations as f64 / elapsed.as_secs_f64(); + + // Verify we have the expected number of keys + let storage = provider.storage.lock().unwrap(); + let expected_keys = 50 + (num_writer_threads * operations_per_thread); // initial + written + assert_eq!( + storage.len(), + expected_keys, + "Expected {} keys but found {}", + expected_keys, + storage.len() + ); + + println!( + "✓ Mixed workload test: {} operations in {:?} ({:.0} ops/sec)", + total_operations, elapsed, ops_per_second + ); + + // Performance assertion - should handle reasonable throughput + assert!( + ops_per_second > 500.0, + "Performance too slow: {:.0} ops/sec (expected > 500)", + ops_per_second + ); + } + + #[test] + fn test_provider_thread_safety() { + use std::sync::Arc; + use std::thread; + + let provider = Arc::new(MockProvider::new()); + let project = "thread-safety-test"; + let profile = "default"; + + // Test that the same provider instance can be safely shared across threads + let num_threads = 8; + let mut handles = vec![]; + + for thread_id in 0..num_threads { + let provider = Arc::clone(&provider); + + let handle = thread::spawn(move || { + // Each thread sets and gets its own unique key + let key = format!("thread-{}-key", thread_id); + let value = format!("thread-{}-value", thread_id); + let secret = SecretString::new(value.clone().into()); + + // Set the value + provider.set(project, &key, &secret, profile).unwrap(); + + // Immediately try to get it back + let result = provider.get(project, &key, profile).unwrap(); + assert!( + result.is_some(), + "Key {} should exist immediately after setting", + key + ); + assert_eq!( + result.unwrap().expose_secret(), + &value, + "Value should match for key {}", + key + ); + + // Try multiple get operations + for _ in 0..10 { + let result = provider.get(project, &key, profile).unwrap(); + assert!(result.is_some(), "Key {} should remain accessible", key); + assert_eq!( + result.unwrap().expose_secret(), + &value, + "Value should remain consistent for key {}", + key + ); + } + }); + + handles.push(handle); + } + + // Wait for all threads to complete + for handle in handles { + handle.join().unwrap(); + } + + // Verify all keys are present and correct + for thread_id in 0..num_threads { + let key = format!("thread-{}-key", thread_id); + let expected_value = format!("thread-{}-value", thread_id); + + let result = provider.get(project, &key, profile).unwrap(); + assert!( + result.is_some(), + "Key {} should exist after all threads complete", + key + ); + assert_eq!( + result.unwrap().expose_secret(), + &expected_value, + "Final value should be correct for key {}", + key + ); + } + + println!( + "✓ Thread safety test passed: {} threads completed successfully", + num_threads + ); + } + + #[test] + fn test_performance_baseline_measurements() { + use std::time::Instant; + + let provider = MockProvider::new(); + let project = "perf-baseline"; + let profile = "default"; + + // Test single operation performance + let key = "perf-test-key"; + let value = "perf-test-value"; + let secret = SecretString::new(value.into()); + + // Measure set operation + let start = Instant::now(); + provider.set(project, key, &secret, profile).unwrap(); + let set_duration = start.elapsed(); + + // Measure get operation + let start = Instant::now(); + let result = provider.get(project, key, profile).unwrap(); + let get_duration = start.elapsed(); + + assert!(result.is_some()); + assert_eq!(result.unwrap().expose_secret(), value); + + // Measure batch operations + let batch_size = 1000; + let start = Instant::now(); + + for i in 0..batch_size { + let batch_key = format!("batch-key-{}", i); + let batch_value = format!("batch-value-{}", i); + let batch_secret = SecretString::new(batch_value.into()); + provider + .set(project, &batch_key, &batch_secret, profile) + .unwrap(); + } + + let batch_set_duration = start.elapsed(); + let avg_set_time = batch_set_duration / batch_size; + + // Measure batch gets + let start = Instant::now(); + + for i in 0..batch_size { + let batch_key = format!("batch-key-{}", i); + let result = provider.get(project, &batch_key, profile).unwrap(); + assert!(result.is_some()); + } + + let batch_get_duration = start.elapsed(); + let avg_get_time = batch_get_duration / batch_size; + + println!("✓ Performance baseline measurements:"); + println!(" Single set: {:?}", set_duration); + println!(" Single get: {:?}", get_duration); + println!(" Average set ({}): {:?}", batch_size, avg_set_time); + println!(" Average get ({}): {:?}", batch_size, avg_get_time); + println!( + " Batch set throughput: {:.0} ops/sec", + batch_size as f64 / batch_set_duration.as_secs_f64() + ); + println!( + " Batch get throughput: {:.0} ops/sec", + batch_size as f64 / batch_get_duration.as_secs_f64() + ); + + // Performance assertions - should be very fast for in-memory operations + assert!( + set_duration.as_micros() < 1000, + "Set operation too slow: {:?}", + set_duration + ); + assert!( + get_duration.as_micros() < 1000, + "Get operation too slow: {:?}", + get_duration + ); + assert!( + avg_set_time.as_micros() < 100, + "Average set too slow: {:?}", + avg_set_time + ); + assert!( + avg_get_time.as_micros() < 100, + "Average get too slow: {:?}", + avg_get_time + ); + } + + // Unit tests for individual BitwardenProvider methods + #[test] + fn test_bitwarden_redact_secret_patterns() { + use crate::provider::bitwarden::{BitwardenConfig, BitwardenProvider}; + + let provider = BitwardenProvider::new(BitwardenConfig::default()); + + // Test JSON token pattern redaction - just verify method works + let input = r#"{"token": "secret123", "other": "value"}"#; + let result = provider.redact_secret_patterns(input.to_string()); + println!("redact_secret_patterns: '{}' -> '{}'", input, result); + assert!(!result.is_empty(), "Should produce output"); + + // Test that method works with various inputs including new field names + let test_cases = vec![ + (r#"{"key": "mysecretkey12345", "data": "public"}"#, true), + (r#"{"secret": "topsecret12345", "id": 123}"#, true), + (r#"{"password": "mypassword123", "username": "user"}"#, true), + ( + r#"{"authorization": "Bearer abc123456789", "type": "auth"}"#, + true, + ), + ( + r#"{"jwt": "eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0", "id": 1}"#, + true, + ), + ( + r#"{"client_secret": "verysecret123456", "client_id": "public123"}"#, + true, + ), + (r#"{"data": "public", "status": "ok"}"#, false), + ]; + + for (input, should_redact) in test_cases { + let result = provider.redact_secret_patterns(input.to_string()); + println!("Enhanced pattern test: '{}' -> '{}'", input, result); + assert!(!result.is_empty(), "Should produce output for: {}", input); + if should_redact { + assert!( + result.contains("[REDACTED]"), + "Should redact secrets in: {}", + input + ); + } + } + } + + #[test] + fn test_bitwarden_redact_bearer_tokens() { + use crate::provider::bitwarden::{BitwardenConfig, BitwardenProvider}; + + let provider = BitwardenProvider::new(BitwardenConfig::default()); + + // Test method exists and processes inputs without crashing + let test_cases = vec![ + "Authorization: Bearer eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0", + "Bearer abc123456789012345678901234567890", // Long bearer token + "No tokens here, just regular text", + "Bearer short", // Short bearer token + ]; + + for input in test_cases { + let result = provider.redact_bearer_tokens(input.to_string()); + assert!(!result.is_empty(), "Should produce output for: {}", input); + // Method should not crash and should produce reasonable output + } + } + + #[test] + fn test_bitwarden_redact_base64_tokens() { + use crate::provider::bitwarden::{BitwardenConfig, BitwardenProvider}; + + let provider = BitwardenProvider::new(BitwardenConfig::default()); + + // Test method exists and processes inputs without crashing + let test_cases = vec![ + "Token: eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0", + "short123", // Should not be redacted + "123456789012345678901234567890", // Pure numbers + "aaaaaaaaaaaaaaaaaaaaaaaaa", // Repeated chars + "abc123def456ghi789jkl012mno345", // Mixed alphanumeric + ]; + + for input in test_cases { + let result = provider.redact_base64_tokens(input.to_string()); + println!("base64 test: '{}' -> '{}'", input, result); + assert!(!result.is_empty(), "Should produce output for: {}", input); + // Method should not crash - actual behavior may vary + } + } + + #[test] + fn test_bitwarden_redact_file_paths() { + use crate::provider::bitwarden::{BitwardenConfig, BitwardenProvider}; + + let provider = BitwardenProvider::new(BitwardenConfig::default()); + + // Test method exists and processes inputs without crashing + let test_cases = vec![ + "Error in /home/user/secrets/config.json at line 5", + "Failed to read C:\\Users\\Admin\\Documents\\secret.txt", + "Copy from /tmp/source.dat to /home/dest.dat", + "No file paths in this message", + "File: ./config.json or ../data.txt", + ]; + + for input in test_cases { + let result = provider.redact_file_paths(input.to_string()); + println!("file path test: '{}' -> '{}'", input, result); + assert!(!result.is_empty(), "Should produce output for: {}", input); + // Method should not crash - actual behavior may vary + } + } + + #[test] + fn test_bitwarden_truncate_long_message() { + use crate::provider::bitwarden::{BitwardenConfig, BitwardenProvider}; + + let provider = BitwardenProvider::new(BitwardenConfig::default()); + + // Test short message (should remain unchanged) + let short_msg = "This is a short error message"; + let result = provider.truncate_long_message(short_msg.to_string()); + assert_eq!(result, short_msg); + + // Test long message (should be truncated) + let long_msg = "A".repeat(600); // 600 characters + let result = provider.truncate_long_message(long_msg); + assert_eq!(result.len(), 450 + "... [truncated for security]".len()); + assert!(result.ends_with("... [truncated for security]")); + assert!(result.starts_with("AAA")); // Should start with original content + + // Test exactly 500 characters (should not be truncated) + let exact_msg = "B".repeat(500); + let result = provider.truncate_long_message(exact_msg.clone()); + assert_eq!(result, exact_msg); + + // Test 501 characters (should be truncated) + let just_over_msg = "C".repeat(501); + let result = provider.truncate_long_message(just_over_msg); + assert!(result.ends_with("... [truncated for security]")); + assert_eq!(result.len(), 450 + "... [truncated for security]".len()); + } + + #[test] + fn test_bitwarden_sanitize_integration() { + use crate::provider::bitwarden::{BitwardenConfig, BitwardenProvider}; + + let provider = BitwardenProvider::new(BitwardenConfig::default()); + + // Test comprehensive sanitization with complex input + let complex_error = "Authentication failed: {\"token\": \"longsecret123456789\", \"bearer\": \"Bearer eyJ0eXAiOiJKV1QiLnothinghere\"} File: /home/user/.secrets/vault.json Error: ".to_string() + &"Additional context: ".repeat(50); + + let result = provider.sanitize_error_message(&complex_error); + + // Just verify the method works and produces output + assert!(!result.is_empty(), "Should produce output"); + + // Should contain some form of redaction indicators + assert!( + result.contains("[REDACTED]") || result.contains("...") || result.contains("truncated"), + "Should show some sanitization occurred: {}", + result + ); + } + + #[test] + fn test_bitwarden_comprehensive_error_sanitization() { + use crate::provider::bitwarden::{BitwardenConfig, BitwardenProvider}; + + let provider = BitwardenProvider::new(BitwardenConfig::default()); + + // Test that all error paths through sanitize_error_message produce clean output + let test_cases = vec![ + "Command failed with token: abc123456789012345", + "Authentication failed: Bearer eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.secret", + "Error reading /home/user/.secrets/vault.json", + "Failed to parse: {\"password\": \"verysecret12345678\", \"status\": \"error\"}", + "Windows path error: C:\\Users\\Admin\\AppData\\Local\\secret.dat", + ]; + + for input in test_cases { + let result = provider.sanitize_error_message(input); + println!("Comprehensive sanitization: '{}' -> '{}'", input, result); + + // Verify no raw secrets remain + assert!( + !result.contains("abc123456789012345"), + "Should redact long tokens" + ); + assert!( + !result.contains("verysecret12345678"), + "Should redact password values" + ); + assert!( + !result.contains("eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.secret"), + "Should redact JWT" + ); + + // Verify file paths are sanitized + if input.contains("/home/user/.secrets") { + assert!( + result.contains(".../vault.json"), + "Should preserve filename in Unix paths" + ); + } + if input.contains("C:\\Users\\Admin") { + assert!( + result.contains("...\\secret.dat"), + "Should preserve filename in Windows paths" + ); + } + + assert!(!result.is_empty(), "Should produce output"); + } + } + + #[test] + fn test_bitwarden_string_helper_functions() { + use crate::provider::bitwarden::BitwardenProvider; + use secrecy::ExposeSecret; + + // Test to_secret_string helper + let test_string = "test_value"; + let secret = BitwardenProvider::to_secret_string(test_string); + assert_eq!(secret.expose_secret(), test_string); + + // Test with String type + let test_string = String::from("test_value_2"); + let secret = BitwardenProvider::to_secret_string(&test_string); + assert_eq!(secret.expose_secret(), "test_value_2"); + + // Test option_to_secret_string helper with Some + let some_value = Some("optional_test"); + let result = BitwardenProvider::option_to_secret_string(some_value); + assert!(result.is_some()); + assert_eq!(result.unwrap().expose_secret(), "optional_test"); + + // Test option_to_secret_string helper with None + let none_value: Option<&str> = None; + let result = BitwardenProvider::option_to_secret_string(none_value); + assert!(result.is_none()); + + // Test with Option + let some_string = Some(String::from("string_test")); + let result = BitwardenProvider::option_to_secret_string(some_string.as_deref()); + assert!(result.is_some()); + assert_eq!(result.unwrap().expose_secret(), "string_test"); + } + + #[test] + fn test_default_reflect_returns_error() { + // Test that the default reflect implementation returns an error + let provider = MockProvider::new(); + let result = provider.reflect(); + assert!( + result.is_err(), + "Default reflect implementation should return an error" + ); + + let error = result.unwrap_err(); + let error_msg = error.to_string(); + assert!( + error_msg.contains("does not support reflection"), + "Error message should indicate reflection is not supported" + ); + } } diff --git a/tests/bitwarden_performance.sh b/tests/bitwarden_performance.sh new file mode 100755 index 00000000..322cbd31 --- /dev/null +++ b/tests/bitwarden_performance.sh @@ -0,0 +1,346 @@ +#!/bin/bash + +# SecretSpec Bitwarden Performance Analysis Script +# Measures timing of different Bitwarden CLI operations +# Usage: ./bitwarden_performance.sh [BW_SESSION] + +set -e # Exit on any error + +# Get BW_SESSION from command line or environment +if [ $# -gt 0 ]; then + BW_SESSION="$1" + echo "Using BW_SESSION from command line argument" +elif [ -n "$BW_SESSION" ]; then + echo "Using BW_SESSION from environment variable" +else + echo "ERROR: BW_SESSION is required either as argument or environment variable" + echo "Usage: $0 [BW_SESSION]" + echo "Or: BW_SESSION=your_session $0" + exit 1 +fi + +echo "🔬 SecretSpec Bitwarden Performance Analysis" +echo "===========================================" + +# Colors for output +GREEN='\033[0;32m' +RED='\033[0;31m' +YELLOW='\033[1;33m' +BLUE='\033[0;34m' +MAGENTA='\033[0;35m' +NC='\033[0m' # No Color + +# Timing variables +declare -a TIMING_LABELS +declare -a TIMING_VALUES +TIMING_COUNT=0 + +# Function to measure command execution time +measure_time() { + local label="$1" + local command="$2" + + echo -e "\n${BLUE}Measuring: $label${NC}" + echo "Command: $command" + + # Measure execution time using time command + local start_time=$(date +%s%N) + + if output=$(eval "$command" 2>&1); then + local end_time=$(date +%s%N) + local duration_ns=$((end_time - start_time)) + local duration_ms=$((duration_ns / 1000000)) + + echo -e "${GREEN}✓ Success${NC} - Duration: ${duration_ms}ms" + + # Store timing data + TIMING_LABELS[$TIMING_COUNT]="$label" + TIMING_VALUES[$TIMING_COUNT]=$duration_ms + TIMING_COUNT=$((TIMING_COUNT + 1)) + + # Show output size + local output_size=${#output} + echo "Output size: $output_size bytes" + else + echo -e "${RED}✗ Failed${NC}: $output" + fi +} + +# Function to measure repeated operations +measure_repeated() { + local label="$1" + local command="$2" + local count="${3:-5}" # Default to 5 iterations + + echo -e "\n${MAGENTA}Measuring repeated: $label (${count}x)${NC}" + echo "Command: $command" + + local total_time=0 + local min_time=999999 + local max_time=0 + + for i in $(seq 1 $count); do + local start_time=$(date +%s%N) + + if eval "$command" >/dev/null 2>&1; then + local end_time=$(date +%s%N) + local duration_ns=$((end_time - start_time)) + local duration_ms=$((duration_ns / 1000000)) + + total_time=$((total_time + duration_ms)) + + if [ $duration_ms -lt $min_time ]; then + min_time=$duration_ms + fi + + if [ $duration_ms -gt $max_time ]; then + max_time=$duration_ms + fi + + echo -n "." + else + echo -e "\n${RED}✗ Failed on iteration $i${NC}" + return 1 + fi + done + + local avg_time=$((total_time / count)) + + echo -e "\n${GREEN}✓ Completed${NC}" + echo "Average: ${avg_time}ms, Min: ${min_time}ms, Max: ${max_time}ms" + + # Store average timing data + TIMING_LABELS[$TIMING_COUNT]="$label (avg)" + TIMING_VALUES[$TIMING_COUNT]=$avg_time + TIMING_COUNT=$((TIMING_COUNT + 1)) +} + +echo -e "\n${YELLOW}Prerequisites Check${NC}" +echo "Checking Bitwarden CLI authentication..." + +# Check BW authentication +if ! BW_SESSION="$BW_SESSION" bw status | grep -q "unlocked"; then + echo -e "${RED}ERROR: Bitwarden vault is not unlocked with provided session!${NC}" + exit 1 +fi + +echo -e "${GREEN}✓ Bitwarden CLI is authenticated and unlocked${NC}" + +# Get vault size for context +echo -e "\n${YELLOW}Vault Statistics${NC}" +item_count=$(BW_SESSION="$BW_SESSION" bw list items | jq 'length') +echo "Total items in vault: $item_count" + +# Create test items if needed +echo -e "\n${YELLOW}Setting up test data${NC}" +TEST_ITEM_NAME="secretspec-perf-test-$(date +%s)" +echo "Creating test item: $TEST_ITEM_NAME" + +# Create a test item +create_json=$(cat </dev/null +echo -e "${GREEN}✓ Test item created${NC}" + +echo -e "\n${YELLOW}=== BITWARDEN CLI PERFORMANCE TESTS ===${NC}" + +# Test 1: Basic status check +measure_time "bw status" \ + "BW_SESSION='$BW_SESSION' bw status" + +# Test 2: List all items (full vault scan) +measure_time "bw list items (full vault)" \ + "BW_SESSION='$BW_SESSION' bw list items" + +# Test 3: List items with search filter +measure_time "bw list items --search (filtered)" \ + "BW_SESSION='$BW_SESSION' bw list items --search '$TEST_ITEM_NAME'" + +# Test 4: Get specific item by exact name +measure_time "bw get item (by name)" \ + "BW_SESSION='$BW_SESSION' bw get item '$TEST_ITEM_NAME'" + +# Test 5: List + jq filtering (current implementation) +measure_time "bw list + jq filter" \ + "BW_SESSION='$BW_SESSION' bw list items | jq -r '.[] | select(.name == \"$TEST_ITEM_NAME\")'" + +# Test 6: List with search + jq (optimized) +measure_time "bw list --search + jq" \ + "BW_SESSION='$BW_SESSION' bw list items --search '$TEST_ITEM_NAME' | jq -r '.[0]'" + +echo -e "\n${YELLOW}=== REPEATED OPERATION TESTS ===${NC}" + +# Test 7: Repeated item retrieval (cache effectiveness) +measure_repeated "Repeated bw get item" \ + "BW_SESSION='$BW_SESSION' bw get item '$TEST_ITEM_NAME'" \ + 10 + +# Test 8: Repeated list + search +measure_repeated "Repeated bw list --search" \ + "BW_SESSION='$BW_SESSION' bw list items --search '$TEST_ITEM_NAME'" \ + 10 + +echo -e "\n${YELLOW}=== FIELD EXTRACTION PERFORMANCE ===${NC}" + +# Test 9: Extract password field with jq +measure_time "Extract password with jq" \ + "BW_SESSION='$BW_SESSION' bw get item '$TEST_ITEM_NAME' | jq -r '.login.password'" + +# Test 10: Extract custom field with jq +measure_time "Extract custom field with jq" \ + "BW_SESSION='$BW_SESSION' bw get item '$TEST_ITEM_NAME' | jq -r '.fields[] | select(.name == \"api_key\") | .value'" + +echo -e "\n${YELLOW}=== LARGE RESPONSE TESTS ===${NC}" + +# Test 11: Search with common prefix (potentially many results) +measure_time "Search common prefix" \ + "BW_SESSION='$BW_SESSION' bw list items --search 'test'" + +# Test 12: Process large JSON response +measure_time "Process vault JSON size" \ + "BW_SESSION='$BW_SESSION' bw list items | wc -c" + +echo -e "\n${YELLOW}=== SECRETSPEC INTEGRATION PERFORMANCE ===${NC}" + +# Enable performance logging for detailed timing +export SECRETSPEC_PERF_LOG=1 +echo "Performance logging enabled - will show detailed timing breakdown" + +# Create a test secretspec.toml +cat > secretspec.toml << EOF +[project] +name = "perf-test" +revision = "1.0" + +[profiles.default] +"$TEST_ITEM_NAME" = { required = true } +TEST_FIELD = { required = false } +EOF + +# Build if needed +if [ ! -f "./target/debug/secretspec" ]; then + echo "Building secretspec..." + cargo build --bin secretspec --quiet +fi + +# Test 13: SecretSpec get (password field) - with detailed logging +echo -e "\n${BLUE}Detailed timing for secretspec get operation:${NC}" +echo "This will show breakdown of CLI vs JSON processing time:" +measure_time "secretspec get (password)" \ + "BW_SESSION='$BW_SESSION' SECRETSPEC_PERF_LOG=1 ./target/debug/secretspec get '$TEST_ITEM_NAME' --provider 'bitwarden://'" | tee /tmp/secretspec_timing.log + +# Test 14: SecretSpec get (custom field) +measure_time "secretspec get (custom field)" \ + "BW_SESSION='$BW_SESSION' ./target/debug/secretspec get '$TEST_ITEM_NAME' --provider 'bitwarden://?field=api_key'" + +# Test 15: Repeated SecretSpec operations +measure_repeated "Repeated secretspec get" \ + "BW_SESSION='$BW_SESSION' ./target/debug/secretspec get '$TEST_ITEM_NAME' --provider 'bitwarden://'" \ + 10 + +echo -e "\n${YELLOW}=== PERFORMANCE SUMMARY ===${NC}" +echo "==========================================" + +# Find slowest and fastest operations +slowest_idx=0 +fastest_idx=0 +slowest_time=0 +fastest_time=999999 + +for i in $(seq 0 $((TIMING_COUNT - 1))); do + if [ ${TIMING_VALUES[$i]} -gt $slowest_time ]; then + slowest_time=${TIMING_VALUES[$i]} + slowest_idx=$i + fi + + if [ ${TIMING_VALUES[$i]} -lt $fastest_time ]; then + fastest_time=${TIMING_VALUES[$i]} + fastest_idx=$i + fi +done + +echo -e "\n${GREEN}Fastest operation:${NC}" +printf "%-50s %6dms\n" "${TIMING_LABELS[$fastest_idx]}" "${TIMING_VALUES[$fastest_idx]}" + +echo -e "\n${RED}Slowest operation:${NC}" +printf "%-50s %6dms\n" "${TIMING_LABELS[$slowest_idx]}" "${TIMING_VALUES[$slowest_idx]}" + +echo -e "\n${BLUE}All timings:${NC}" +for i in $(seq 0 $((TIMING_COUNT - 1))); do + printf "%-50s %6dms\n" "${TIMING_LABELS[$i]}" "${TIMING_VALUES[$i]}" +done | sort -k2 -n + +# Calculate potential savings +echo -e "\n${MAGENTA}Performance Insights:${NC}" + +# Analyze secretspec timing breakdown +if [ -f /tmp/secretspec_timing.log ]; then + echo -e "\n${BLUE}SecretSpec Operation Breakdown:${NC}" + grep "\[PERF\]" /tmp/secretspec_timing.log | sed 's/^/ /' || echo " No detailed timing found" + rm -f /tmp/secretspec_timing.log +fi + +# Compare different retrieval methods +list_time=0 +get_time=0 +search_time=0 + +for i in $(seq 0 $((TIMING_COUNT - 1))); do + case "${TIMING_LABELS[$i]}" in + *"list items (full"*) + list_time=${TIMING_VALUES[$i]} + ;; + *"get item (by name)"*) + get_time=${TIMING_VALUES[$i]} + ;; + *"list --search + jq"*) + search_time=${TIMING_VALUES[$i]} + ;; + esac +done + +if [ $list_time -gt 0 ] && [ $get_time -gt 0 ]; then + savings=$((list_time - get_time)) + percent=$((savings * 100 / list_time)) + echo "- Using 'bw get item' instead of 'bw list items' saves: ${savings}ms (${percent}%)" +fi + +if [ $list_time -gt 0 ] && [ $search_time -gt 0 ]; then + savings=$((list_time - search_time)) + percent=$((savings * 100 / list_time)) + echo "- Using 'bw list --search' instead of full list saves: ${savings}ms (${percent}%)" +fi + +# Cleanup +echo -e "\n${YELLOW}Cleaning up...${NC}" +BW_SESSION="$BW_SESSION" bw delete item $(BW_SESSION="$BW_SESSION" bw get item "$TEST_ITEM_NAME" | jq -r '.id') --noconfirm >/dev/null 2>&1 || true +rm -f secretspec.toml + +echo -e "\n${YELLOW}Performance Environment Variables:${NC}" +echo "To enable detailed timing for any SecretSpec operation:" +echo " export SECRETSPEC_PERF_LOG=1" +echo " secretspec get SECRET_NAME --provider bitwarden://" +echo "" +echo "This will show timing for:" +echo " - Authentication checks" +echo " - CLI command execution" +echo " - JSON parsing" +echo " - Field extraction" +echo " - Overall operation time" + +echo -e "\n${GREEN}Performance analysis complete!${NC}" \ No newline at end of file diff --git a/tests/cli-integration.sh b/tests/cli-integration.sh index ef8c961b..0077196b 100755 --- a/tests/cli-integration.sh +++ b/tests/cli-integration.sh @@ -149,7 +149,33 @@ check_success "Set secret in production profile" secretspec config show > /dev/null check_success "Config show command works" -# Test 11: Default value handling +# Test 11: Init from provider +# Create a .env file to import from +cat > .env.source << EOF +API_KEY=test-api-key +DATABASE_URL=postgres://localhost/test +EOF + +# Test init with bare provider name +rm -f secretspec.toml +secretspec init --from dotenv:.env.source +check_success "Init from dotenv provider with path" + +# Verify secrets were imported +grep -q "API_KEY" secretspec.toml && grep -q "DATABASE_URL" secretspec.toml +check_success "Init imported secrets from .env file" + +# Test init with bare provider name (should use default .env) +echo "DEFAULT_KEY=default-value" > .env +rm -f secretspec.toml +secretspec init --from dotenv +check_success "Init from dotenv provider (bare name)" + +# Verify it found the default .env +grep -q "DEFAULT_KEY" secretspec.toml +check_success "Init found default .env file" + +# Test 12: Default value handling cat > secretspec.toml << EOF [project] name = "test-app" diff --git a/tests/test_performance_logging.sh b/tests/test_performance_logging.sh new file mode 100755 index 00000000..ed036321 --- /dev/null +++ b/tests/test_performance_logging.sh @@ -0,0 +1,61 @@ +#!/bin/bash + +# Test script to verify performance logging works +# This script doesn't require a real Bitwarden vault + +set -e + +echo "🔬 Testing Performance Logging" +echo "==============================" + +# Build the binary +echo "Building secretspec..." +cargo build --bin secretspec --quiet + +# Create a minimal test config +cat > secretspec.toml << 'EOF' +[project] +name = "perf-test" +revision = "1.0" + +[profiles.default] +TEST_KEY = { required = false } +EOF + +echo "✓ Created test config" + +# Test 1: Run without performance logging (should have no [PERF] output) +echo -e "\n📊 Test 1: Normal operation (no performance logging)" +if ./target/debug/secretspec get TEST_KEY --provider bitwarden:// 2>&1 | grep -q "\[PERF\]"; then + echo "❌ FAILED: Found [PERF] output when not enabled" + exit 1 +else + echo "✅ PASSED: No performance output when disabled" +fi + +# Test 2: Run with performance logging enabled (should have [PERF] output) +echo -e "\n📊 Test 2: With performance logging enabled" +SECRETSPEC_PERF_LOG=1 ./target/debug/secretspec get TEST_KEY --provider bitwarden:// 2>&1 | grep "\[PERF\]" > /tmp/perf_output.txt || true + +if [ -s /tmp/perf_output.txt ]; then + echo "✅ PASSED: Performance logging enabled" + echo "Performance output:" + cat /tmp/perf_output.txt | sed 's/^/ /' +else + echo "⚠️ WARNING: No performance output found (this is expected if no Bitwarden CLI is available)" +fi + +# Test 3: Test with environment variable +echo -e "\n📊 Test 3: Testing timing granularity" +echo "Running with performance logging to see timing breakdown..." + +SECRETSPEC_PERF_LOG=1 ./target/debug/secretspec get NONEXISTENT_KEY --provider bitwarden:// 2>&1 | \ + grep "\[PERF\]" | head -5 | sed 's/^/ /' || echo " (No output - likely no Bitwarden CLI available)" + +# Cleanup +rm -f secretspec.toml /tmp/perf_output.txt + +echo -e "\n✅ Performance logging tests completed" +echo "To use performance logging:" +echo " export SECRETSPEC_PERF_LOG=1" +echo " secretspec get KEY --provider bitwarden://" \ No newline at end of file diff --git a/tests/test_provider_not_found.rs b/tests/test_provider_not_found.rs index 82cc13c5..22a445e7 100644 --- a/tests/test_provider_not_found.rs +++ b/tests/test_provider_not_found.rs @@ -7,7 +7,7 @@ mod test_provider_not_found { fn test_keyring_provider_when_feature_disabled() { // This test checks what error we get when trying to use keyring provider // when the keyring feature is disabled - + #[cfg(not(feature = "keyring"))] { match Box::::try_from("keyring") { @@ -24,18 +24,21 @@ mod test_provider_not_found { } } } - + #[cfg(feature = "keyring")] { // When feature is enabled, keyring should work match Box::::try_from("keyring") { Ok(provider) => assert_eq!(provider.name(), "keyring"), - Err(e) => panic!("Should create keyring provider when feature is enabled: {}", e), + Err(e) => panic!( + "Should create keyring provider when feature is enabled: {}", + e + ), } } } - - #[test] + + #[test] fn test_truly_unknown_provider() { // Test a provider that really doesn't exist match Box::::try_from("nonexistent_provider") { @@ -51,4 +54,4 @@ mod test_provider_not_found { } } } -} \ No newline at end of file +}