mirror of
https://github.com/cheat/cheat.git
synced 2026-03-07 03:03:32 +01:00
docs: move ADRs to project root, remove boilerplate README
Move `doc/adr/` to `adr/` for discoverability. Remove the generic ADR README — `ls adr/` serves the same purpose. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -1,169 +0,0 @@
|
||||
# ADR-001: Path Traversal Protection for Cheatsheet Names
|
||||
|
||||
Date: 2025-01-21
|
||||
|
||||
## Status
|
||||
|
||||
Accepted
|
||||
|
||||
## Context
|
||||
|
||||
The `cheat` tool allows users to create, edit, and remove cheatsheets using commands like:
|
||||
- `cheat --edit <name>`
|
||||
- `cheat --rm <name>`
|
||||
|
||||
Without validation, a user could potentially provide malicious names like:
|
||||
- `../../../etc/passwd` (directory traversal)
|
||||
- `/etc/passwd` (absolute path)
|
||||
- `~/.ssh/authorized_keys` (home directory expansion)
|
||||
|
||||
While `cheat` is a local tool run by the user themselves (not a network service), path traversal could still lead to:
|
||||
1. Accidental file overwrites outside cheatsheet directories
|
||||
2. Confusion about where files are being created
|
||||
3. Potential security issues in shared environments
|
||||
|
||||
## Decision
|
||||
|
||||
We implemented input validation for cheatsheet names to prevent directory traversal attacks. The validation rejects names that:
|
||||
|
||||
1. Contain `..` (parent directory references)
|
||||
2. Are absolute paths (start with `/` on Unix)
|
||||
3. Start with `~` (home directory expansion)
|
||||
4. Are empty
|
||||
5. Start with `.` (hidden files - these are not displayed by cheat)
|
||||
|
||||
The validation is performed at the application layer before any file operations occur.
|
||||
|
||||
## Implementation Details
|
||||
|
||||
### Validation Function
|
||||
|
||||
The validation is implemented in `internal/cheatpath/validate.go`:
|
||||
|
||||
```go
|
||||
func ValidateSheetName(name string) error {
|
||||
// Reject empty names
|
||||
if name == "" {
|
||||
return fmt.Errorf("cheatsheet name cannot be empty")
|
||||
}
|
||||
|
||||
// Reject names containing directory traversal
|
||||
if strings.Contains(name, "..") {
|
||||
return fmt.Errorf("cheatsheet name cannot contain '..'")
|
||||
}
|
||||
|
||||
// Reject absolute paths
|
||||
if filepath.IsAbs(name) {
|
||||
return fmt.Errorf("cheatsheet name cannot be an absolute path")
|
||||
}
|
||||
|
||||
// Reject names that start with ~ (home directory expansion)
|
||||
if strings.HasPrefix(name, "~") {
|
||||
return fmt.Errorf("cheatsheet name cannot start with '~'")
|
||||
}
|
||||
|
||||
// Reject hidden files (files that start with a dot)
|
||||
filename := filepath.Base(name)
|
||||
if strings.HasPrefix(filename, ".") {
|
||||
return fmt.Errorf("cheatsheet name cannot start with '.' (hidden files are not supported)")
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
```
|
||||
|
||||
### Integration Points
|
||||
|
||||
The validation is called in:
|
||||
- `cmd/cheat/cmd_edit.go` - before creating or editing a cheatsheet
|
||||
- `cmd/cheat/cmd_remove.go` - before removing a cheatsheet
|
||||
|
||||
### Allowed Patterns
|
||||
|
||||
The following patterns are explicitly allowed:
|
||||
- Simple names: `docker`, `git`
|
||||
- Nested paths: `docker/compose`, `lang/go/slice`
|
||||
- Current directory references: `./mysheet`
|
||||
|
||||
## Consequences
|
||||
|
||||
### Positive
|
||||
|
||||
1. **Safety**: Prevents accidental or intentional file operations outside cheatsheet directories
|
||||
2. **Simplicity**: Validation happens early, before any file operations
|
||||
3. **User-friendly**: Clear error messages explain why a name was rejected
|
||||
4. **Performance**: Minimal overhead - simple string checks
|
||||
5. **Compatibility**: Doesn't break existing valid cheatsheet names
|
||||
|
||||
### Negative
|
||||
|
||||
1. **Limitation**: Users cannot use `..` in cheatsheet names even if legitimate
|
||||
2. **No symlink support**: Cannot create cheatsheets through symlinks outside the cheatpath
|
||||
|
||||
### Neutral
|
||||
|
||||
1. Uses Go's `filepath.IsAbs()` which handles platform differences (Windows vs Unix)
|
||||
2. No attempt to resolve or canonicalize paths - validation is purely syntactic
|
||||
|
||||
## Security Considerations
|
||||
|
||||
### Threat Model
|
||||
|
||||
`cheat` is a local command-line tool, not a network service. The primary threats are:
|
||||
- User error (accidentally overwriting important files)
|
||||
- Malicious scripts that invoke `cheat` with crafted arguments
|
||||
- Shared system scenarios where cheatsheets might be shared
|
||||
|
||||
### What This Protects Against
|
||||
|
||||
- Directory traversal using `../`
|
||||
- Absolute path access to system files
|
||||
- Shell expansion of `~` to home directory
|
||||
- Empty names that might cause unexpected behavior
|
||||
- Hidden files that wouldn't be displayed anyway
|
||||
|
||||
### What This Does NOT Protect Against
|
||||
|
||||
- Users with filesystem permissions can still directly edit any file
|
||||
- Symbolic links within the cheatpath pointing outside
|
||||
- Race conditions (TOCTOU) - though minimal risk for a local tool
|
||||
- Malicious content within cheatsheets themselves
|
||||
|
||||
## Testing
|
||||
|
||||
Comprehensive tests ensure the validation works correctly:
|
||||
|
||||
1. **Unit tests** (`internal/cheatpath/validate_test.go`) verify the validation logic
|
||||
2. **Integration tests** verify the actual binary blocks malicious inputs
|
||||
3. **No system files are accessed** during testing - all tests use isolated directories
|
||||
|
||||
Example test cases:
|
||||
```bash
|
||||
# These are blocked:
|
||||
cheat --edit "../../../etc/passwd"
|
||||
cheat --edit "/etc/passwd"
|
||||
cheat --edit "~/.ssh/config"
|
||||
cheat --rm ".."
|
||||
|
||||
# These are allowed:
|
||||
cheat --edit "docker"
|
||||
cheat --edit "docker/compose"
|
||||
cheat --edit "./local"
|
||||
```
|
||||
|
||||
## Alternative Approaches Considered
|
||||
|
||||
1. **Path resolution and verification**: Resolve the final path and check if it's within the cheatpath
|
||||
- Rejected: More complex, potential race conditions, platform-specific edge cases
|
||||
|
||||
2. **Chroot/sandbox**: Run file operations in a restricted environment
|
||||
- Rejected: Overkill for a local tool, platform compatibility issues
|
||||
|
||||
3. **Filename allowlist**: Only allow alphanumeric characters and specific symbols
|
||||
- Rejected: Too restrictive, would break existing cheatsheets with valid special characters
|
||||
|
||||
## References
|
||||
|
||||
- OWASP Path Traversal: https://owasp.org/www-community/attacks/Path_Traversal
|
||||
- CWE-22: Improper Limitation of a Pathname to a Restricted Directory
|
||||
- Go filepath package documentation: https://pkg.go.dev/path/filepath
|
||||
@@ -1,100 +0,0 @@
|
||||
# ADR-002: No Defensive Checks for Environment Variable Parsing
|
||||
|
||||
Date: 2025-01-21
|
||||
|
||||
## Status
|
||||
|
||||
Accepted
|
||||
|
||||
## Context
|
||||
|
||||
In `cmd/cheat/main.go` lines 47-52, the code parses environment variables assuming they all contain an equals sign:
|
||||
|
||||
```go
|
||||
for _, e := range os.Environ() {
|
||||
pair := strings.SplitN(e, "=", 2)
|
||||
if runtime.GOOS == "windows" {
|
||||
pair[0] = strings.ToUpper(pair[0])
|
||||
}
|
||||
envvars[pair[0]] = pair[1] // Could panic if pair has < 2 elements
|
||||
}
|
||||
```
|
||||
|
||||
If `os.Environ()` returned a string without an equals sign, `strings.SplitN` would return a slice with only one element, causing a panic when accessing `pair[1]`.
|
||||
|
||||
## Decision
|
||||
|
||||
We will **not** add defensive checks for this condition. The current code that assumes all environment strings contain "=" will remain unchanged.
|
||||
|
||||
## Rationale
|
||||
|
||||
### Go Runtime Guarantees
|
||||
|
||||
Go's official documentation guarantees that `os.Environ()` returns environment variables in the form "key=value". This is a documented contract of the Go runtime that has been stable since Go 1.0.
|
||||
|
||||
### Empirical Evidence
|
||||
|
||||
Testing across platforms confirms:
|
||||
- All environment variables returned by `os.Environ()` contain at least one "="
|
||||
- Empty environment variables appear as "KEY=" (with an empty value)
|
||||
- Even Windows special variables like "=C:=C:\path" maintain the format
|
||||
|
||||
### Cost-Benefit Analysis
|
||||
|
||||
Adding defensive code would:
|
||||
- **Cost**: Add complexity and cognitive overhead
|
||||
- **Cost**: Suggest uncertainty about Go's documented behavior
|
||||
- **Cost**: Create dead code that can never execute under normal conditions
|
||||
- **Benefit**: Protect against a theoretical scenario that violates Go's guarantees
|
||||
|
||||
The only scenarios where this could panic are:
|
||||
1. A bug in Go's runtime (extremely unlikely, would affect all Go programs)
|
||||
2. Corrupted OS-level environment (would cause broader system issues)
|
||||
3. Breaking change in future Go version (would break many programs, unlikely)
|
||||
|
||||
## Consequences
|
||||
|
||||
### Positive
|
||||
- Simpler, more readable code
|
||||
- Trust in platform guarantees reduces unnecessary defensive programming
|
||||
- No performance overhead from unnecessary checks
|
||||
|
||||
### Negative
|
||||
- Theoretical panic if Go's guarantees are violated
|
||||
|
||||
### Neutral
|
||||
- Follows Go community standards of trusting standard library contracts
|
||||
|
||||
## Alternatives Considered
|
||||
|
||||
### 1. Add Defensive Check
|
||||
```go
|
||||
if len(pair) < 2 {
|
||||
continue // or pair[1] = ""
|
||||
}
|
||||
```
|
||||
**Rejected**: Adds complexity for a condition that should never occur.
|
||||
|
||||
### 2. Add Panic with Clear Message
|
||||
```go
|
||||
if len(pair) < 2 {
|
||||
panic("os.Environ() contract violation: " + e)
|
||||
}
|
||||
```
|
||||
**Rejected**: Would crash the program for the same theoretical issue.
|
||||
|
||||
### 3. Add Comment Documenting Assumption
|
||||
```go
|
||||
// os.Environ() guarantees "key=value" format, so pair[1] is safe
|
||||
envvars[pair[0]] = pair[1]
|
||||
```
|
||||
**Rejected**: While documentation is good, this particular guarantee is fundamental to Go.
|
||||
|
||||
## Notes
|
||||
|
||||
If Go ever changes this behavior (extremely unlikely as it would break compatibility), it would be caught immediately in testing as the program would panic on startup. This would be a clear signal to revisit this decision.
|
||||
|
||||
## References
|
||||
|
||||
- Go os.Environ() documentation: https://pkg.go.dev/os#Environ
|
||||
- Go os.Environ() source code and tests
|
||||
@@ -1,104 +0,0 @@
|
||||
# ADR-003: No Parallelization for Search Operations
|
||||
|
||||
Date: 2025-01-22
|
||||
|
||||
## Status
|
||||
|
||||
Accepted
|
||||
|
||||
## Context
|
||||
|
||||
We investigated optimizing cheat's search performance through parallelization. Initial assumptions suggested that I/O operations (reading multiple cheatsheet files) would be the primary bottleneck, making parallel processing beneficial.
|
||||
|
||||
Performance benchmarks were implemented to measure search operations, and a parallel search implementation using goroutines was created and tested.
|
||||
|
||||
## Decision
|
||||
|
||||
We will **not** implement parallel search. The sequential implementation will remain unchanged.
|
||||
|
||||
## Rationale
|
||||
|
||||
### Performance Profile Analysis
|
||||
|
||||
CPU profiling revealed that search performance is dominated by:
|
||||
- **Process creation overhead** (~30% in `os/exec.(*Cmd).Run`)
|
||||
- **System calls** (~30% in `syscall.Syscall6`)
|
||||
- **Process management** (fork, exec, pipe setup)
|
||||
|
||||
The actual search logic (regex matching, file reading) was negligible in the profile, indicating our optimization efforts were targeting the wrong bottleneck.
|
||||
|
||||
### Benchmark Results
|
||||
|
||||
Parallel implementation showed minimal improvements:
|
||||
- Simple search: 17ms → 15.3ms (10% improvement)
|
||||
- Regex search: 15ms → 14.9ms (minimal improvement)
|
||||
- Colorized search: 19.5ms → 16.8ms (14% improvement)
|
||||
- Complex regex: 20ms → 15.3ms (24% improvement)
|
||||
|
||||
The best case saved only ~5ms in absolute terms.
|
||||
|
||||
### Cost-Benefit Analysis
|
||||
|
||||
**Costs of parallelization:**
|
||||
- Added complexity with goroutines, channels, and synchronization
|
||||
- Increased maintenance burden
|
||||
- More difficult debugging and testing
|
||||
- Potential race conditions
|
||||
|
||||
**Benefits:**
|
||||
- 5-15% performance improvement (5ms in real terms)
|
||||
- Imperceptible to users in interactive use
|
||||
|
||||
### User Experience Perspective
|
||||
|
||||
For a command-line tool:
|
||||
- Current 15-20ms response time is excellent
|
||||
- Users cannot perceive 5ms differences
|
||||
- Sub-50ms is considered "instant" in HCI research
|
||||
|
||||
## Consequences
|
||||
|
||||
### Positive
|
||||
- Simpler, more maintainable codebase
|
||||
- Easier to debug and reason about
|
||||
- No synchronization bugs or race conditions
|
||||
- Focus remains on code clarity
|
||||
|
||||
### Negative
|
||||
- Missed opportunity for ~5ms performance gain
|
||||
- Search remains single-threaded
|
||||
|
||||
### Neutral
|
||||
- Performance remains excellent for intended use case
|
||||
- Follows Go philosophy of preferring simplicity
|
||||
|
||||
## Alternatives Considered
|
||||
|
||||
### 1. Keep Parallel Implementation
|
||||
**Rejected**: Complexity outweighs negligible performance gains.
|
||||
|
||||
### 2. Optimize Process Startup
|
||||
**Rejected**: Process creation overhead is inherent to CLI tools and cannot be avoided without fundamental architecture changes.
|
||||
|
||||
### 3. Future Optimizations
|
||||
If performance becomes critical, consider:
|
||||
- **Long-running daemon**: Eliminate process startup overhead entirely
|
||||
- **Shell function**: Reduce fork/exec overhead
|
||||
- **Compiled-in cheatsheets**: Eliminate file I/O
|
||||
|
||||
However, these would fundamentally change the tool's architecture and usage model.
|
||||
|
||||
## Notes
|
||||
|
||||
This decision reinforces important principles:
|
||||
1. Always profile before optimizing
|
||||
2. Consider the full execution context
|
||||
3. Measure what matters to users
|
||||
4. Complexity has a real cost
|
||||
|
||||
The parallelization attempt was valuable as a learning exercise and definitively answered whether this optimization path was worthwhile.
|
||||
|
||||
## References
|
||||
|
||||
- Benchmark implementation: cmd/cheat/search_bench_test.go
|
||||
- Reverted parallel implementation: see git history (commit 82eb918)
|
||||
@@ -1,38 +0,0 @@
|
||||
# Architecture Decision Records
|
||||
|
||||
This directory contains Architecture Decision Records (ADRs) for the cheat project.
|
||||
|
||||
## What is an ADR?
|
||||
|
||||
An Architecture Decision Record captures an important architectural decision made along with its context and consequences. ADRs help us:
|
||||
|
||||
- Document why decisions were made
|
||||
- Understand the context and trade-offs
|
||||
- Review decisions when requirements change
|
||||
- Onboard new contributors
|
||||
|
||||
## ADR Format
|
||||
|
||||
Each ADR follows this template:
|
||||
|
||||
1. **Title**: ADR-NNN: Brief description
|
||||
2. **Date**: When the decision was made
|
||||
3. **Status**: Proposed, Accepted, Deprecated, Superseded
|
||||
4. **Context**: What prompted this decision?
|
||||
5. **Decision**: What did we decide to do?
|
||||
6. **Consequences**: What are the positive, negative, and neutral outcomes?
|
||||
|
||||
## Index of ADRs
|
||||
|
||||
| ADR | Title | Status | Date |
|
||||
|-----|-------|--------|------|
|
||||
| [001](001-path-traversal-protection.md) | Path Traversal Protection for Cheatsheet Names | Accepted | 2025-01-21 |
|
||||
| [002](002-environment-variable-parsing.md) | No Defensive Checks for Environment Variable Parsing | Accepted | 2025-01-21 |
|
||||
| [003](003-search-parallelization.md) | No Parallelization for Search Operations | Accepted | 2025-01-22 |
|
||||
|
||||
## Creating a New ADR
|
||||
|
||||
1. Copy the template from an existing ADR
|
||||
2. Use the next sequential number
|
||||
3. Fill in all sections
|
||||
4. Include the ADR alongside the commit implementing the decision
|
||||
Reference in New Issue
Block a user