[wms-88o] Credential store path remains stable across plugin updates

[mrdavidlaing]

Outcome

Credential store path remains stable across plugin updates

Approach

This implementation ensures credentials persist across plugin updates by:

1. Auditing installation processes - Verified that neither install-beads-bridge.sh nor build.ts set CREDENTIAL_STORE_PATH
2. Adding path validation - CredentialStore constructor now validates paths and rejects plugin/temporary directories
3. Comprehensive testing - Added tests for path validation covering multiple dangerous patterns and safe paths
4. Documentation - Created CREDENTIAL_STORAGE.md documenting the credential persistence design and policies

Acceptance Criteria

• CREDENTIAL_STORE_PATH is never set to plugin directory during installation - Verified by auditing install and build scripts
• Default credential path (~/.config/beads-bridge/credentials.json) is used consistently - Verified in code and tests
• Plugin updates do not modify existing credential storage locations - Verified that install only touches binary, not config
• [N/A] Migration logic handles any existing plugin-directory credential stores - Skipped per user decision; validation prevents future issues

Changes

New Files

• src/beads-bridge/docs/CREDENTIAL_STORAGE.md - Comprehensive documentation of credential storage design and policies

Modified Files

• src/beads-bridge/src/auth/credential-store.ts - Added validateCredentialPath() method to reject dangerous paths
• src/beads-bridge/tests/auth/credential-store.test.ts - Added 8 new path validation tests

Testing

All auth and backend tests pass (16/16):

cd src/beads-bridge
bun test tests/auth/ tests/backends/
# 16 pass, 0 fail

New path validation tests verify:

• ✅ Rejection of .claude/skills paths
• ✅ Rejection of node_modules paths
• ✅ Rejection of /tmp and temp directory paths
• ✅ Acceptance of .config directory paths
• ✅ Acceptance of user home directory paths
• ✅ Acceptance of Windows AppData/Roaming paths
• ✅ Default path usage when no override provided

Bead

Implements: wms-88o
View bead: bd show wms-88o

---

🤖 Generated with Claude Code

[claude]

Code Review - PR #24: Credential Store Path Stability

Thank you for this well-documented PR! This is a solid implementation that addresses credential persistence across plugin updates. Here's my review:

---

✅ Strengths

1. Excellent Documentation

• CREDENTIAL_STORAGE.md is comprehensive and well-structured
• Clear warnings about not setting CREDENTIAL_STORE_PATH
• Helpful troubleshooting section
• Developer guidelines and code review checklist

2. Robust Path Validation

• validateCredentialPath() correctly identifies dangerous patterns:
  • Plugin directories (.claude/skills, node_modules)
  • Temporary directories (/tmp, Windows temp)
• Case-insensitive matching handles edge cases
• Clear error messages guide users to the correct path

3. Comprehensive Test Coverage

• 8 new path validation tests cover both rejection and acceptance cases
• Tests verify default path behavior
• skipValidation option allows tests to use /tmp without triggering validation
• All existing tests pass (16/16)

4. Security-First Design

• Default path ~/.config/beads-bridge/credentials.json follows XDG standards
• Installation script correctly doesn't touch credential files
• Build script doesn't set environment variables

---

🔍 Issues & Suggestions

1. Path Validation Could Be More Robust (Minor)

The current validation uses simple string matching which has some edge cases:

// credential-store.ts:50-52
private validateCredentialPath(path: string): void {
  const normalizedPath = path.toLowerCase();
  // ...
}

Issues:

• Relative paths aren't resolved before validation (e.g., ../../../tmp/creds.json)
• Symlinks aren't followed
• Case-insensitive matching on case-sensitive filesystems may miss some dangerous paths

Suggestion:

import { resolve } from 'path';

private validateCredentialPath(path: string): void {
  // Resolve to absolute path to handle relative paths and symlinks
  const normalizedPath = resolve(path).toLowerCase();
  
  const dangerousPatterns = [
    '.claude/skills',
    'node_modules',
    '/tmp/',
    '/temp/',
    'appdata/local/temp',
    'appdata\\local\\temp',
  ];
  
  for (const pattern of dangerousPatterns) {
    if (normalizedPath.includes(pattern.toLowerCase())) {
      throw new Error(
        `Invalid credential path: "${path}" appears to be in a plugin or temporary directory. ` +
        `Credentials must be stored in a persistent location like ~/.config/beads-bridge/credentials.json`
      );
    }
  }
}

2. Test Coverage Gap (Minor)

The tests don't verify that relative paths are handled correctly:

// Add to credential-store.test.ts
it('should reject relative paths that resolve to dangerous directories', () => {
  // Assuming we're in a project with node_modules
  expect(() => {
    new CredentialStore('./node_modules/credentials.json');
  }).toThrow(/Invalid credential path.*plugin or temporary directory/);
});

3. Documentation Accuracy (Very Minor)

In CREDENTIAL_STORAGE.md:98-106, the "bad" example suggests environment variables affect global state, but the test at line 113 actually demonstrates proper cleanup. Consider clarifying:

# ❌ BAD - affects process environment, harder to isolate in parallel tests
export CREDENTIAL_STORE_PATH=/tmp/test-creds.json
beads-bridge auth github

# ✅ GOOD - use constructor injection for test isolation
const store = new CredentialStore('/tmp/test-creds.json', { skipValidation: true });

4. Type Safety Opportunity (Nice-to-have)

The options parameter could use a defined interface:

export interface CredentialStoreOptions {
  skipValidation?: boolean;
}

constructor(filePath?: string, options?: CredentialStoreOptions) {
  // ...
}

---

🎯 Security Considerations

No security concerns identified. The implementation:

• ✅ Uses AES-256-GCM encryption
• ✅ Prevents credentials in ephemeral locations
• ✅ Doesn't log sensitive paths
• ✅ Installation scripts don't set dangerous environment variables

---

🧪 Testing Notes

Per the PR description, all tests pass:

• ✅ 16/16 auth and backend tests pass
• ✅ Path validation tests cover expected scenarios
• ✅ Default path behavior verified

Recommendation: Consider adding integration tests that verify credentials actually persist across a simulated plugin update scenario.

---

📋 Best Practices Assessment

Code Quality: ✅ Excellent

• Clear naming conventions
• Proper error handling
• Good use of TypeScript types
• Following project conventions (using bun)

Documentation: ✅ Excellent

• Comprehensive CREDENTIAL_STORAGE.md
• Clear acceptance criteria in PR description
• Helpful troubleshooting guide

Testing: ✅ Good (with minor suggestions above)

• Good test coverage for path validation
• Proper use of skipValidation for test fixtures

---

✅ Approval Recommendation

APPROVED with minor suggestions for improvement.

This PR successfully addresses the credential persistence issue. The implementation is solid, well-tested, and thoroughly documented. The minor suggestions above are optional enhancements that could be addressed in a follow-up PR if desired.

Suggested merge approach:

1. Merge as-is if you want to ship this improvement quickly
2. OR address the path resolution enhancement for more robust validation

Great work on the comprehensive documentation and test coverage! 🎉

---

Review conducted using project guidelines from CLAUDE.md

• ✅ Uses bun for JavaScript operations
• ✅ No markdown TODOs (uses bd)
• ✅ Follows beads workflow