Skip to content

Roadmap: Production Readiness for v0.1 Release - Code Quality & Cleanup #100

@melvincarvalho

Description

@melvincarvalho

Summary

This issue tracks all code quality improvements, refactoring, security fixes, and cleanup needed to achieve a production-ready v0.1 release of JavaScriptSolidServer. Based on a comprehensive deep-dive analysis of 47+ source files.

Current Version: 0.0.81
Target Version: 0.1.0
Overall Readiness: ~65%


Executive Summary

Category Critical High Medium Low
Security 1 1 5 0
Error Handling 0 1 3 1
Code Quality 0 0 5 4
Performance 0 1 2 2
Testing 0 0 3 1
Documentation 0 0 3 1
Architecture 0 0 3 1
Total 1 3 24 10

🚨 CRITICAL - Must Fix (Blockers)

1. Path Traversal Vulnerability in Git Handler

File: src/handlers/git.js lines 56-61
Severity: CRITICAL
Status: UNFIXED

// VULNERABLE: Uses path.resolve without urlToPath sanitization
const repoAbs = resolve(dataRoot, repoRelative);

Issue: Git handler bypasses path traversal protection used elsewhere. An attacker could potentially access files outside the data directory.

Fix: Use urlToPath() from /src/utils/url.js which includes traversal protection:

import { urlToPath } from '../utils/url.js';
const repoAbs = urlToPath(repoRelative);

2. Unimplemented Agent Group Authorization

File: src/wac/checker.js line 182
Severity: CRITICAL
Status: UNFIXED

// TODO: Check agent groups

Issue: WAC agentGroup authorization is silently ignored. Users expecting group-based access control will have security issues.

Options:

  1. Implement agent group checking (proper fix)
  2. Return 501 Not Implemented when agentGroup encountered (temporary)
  3. Log warning when agentGroup ignored (minimum)

3. WebSocket Subscription Memory Leak

File: src/notifications/websocket.js lines 28-48
Severity: HIGH
Status: UNFIXED

const subscriptions = new Map();  // WebSocket -> Set<url>
const subscribers = new Map();    // url -> Set<WebSocket>

Issue: No cleanup when WebSocket closes unexpectedly. Maps grow unbounded if close events don't fire properly.

Fix: Add explicit cleanup in close handler and periodic garbage collection:

socket.on('close', () => {
  const urls = subscriptions.get(socket);
  if (urls) {
    for (const url of urls) {
      subscribers.get(url)?.delete(socket);
      if (subscribers.get(url)?.size === 0) {
        subscribers.delete(url);
      }
    }
    subscriptions.delete(socket);
  }
});

4. JSON.parse DoS Vulnerabilities

Files: Multiple (5 locations)
Severity: HIGH
Status: PARTIAL

safeJsonParse() exists in src/utils/json.js but not used consistently:

File Line Status
src/wac/parser.js 40 ❌ Raw JSON.parse
src/auth/did-nostr.js 129, 141 ❌ Raw JSON.parse
src/nostr/relay.js 242 ❌ Raw JSON.parse
src/auth/token.js 105 ❌ Raw JSON.parse
src/ap/routes/inbox.js 126 ❌ Raw JSON.parse

Fix: Replace all JSON.parse() with safeJsonParse() that has size limits and try/catch.


⚠️ HIGH PRIORITY - Should Fix Before v0.1

5. Silent Error Swallowing in Storage

File: src/storage/filesystem.js lines 34, 49, 50+
Severity: MEDIUM

export async function read(urlPath) {
  try {
    return await fs.readFile(filePath);
  } catch {
    return null;  // Can't distinguish "not found" from "permission error"
  }
}

Issue: Callers cannot distinguish between:

  • File not found (404)
  • Permission denied (403)
  • Disk error (500)

Fix: Return error objects or throw typed errors:

export async function read(urlPath) {
  try {
    return { data: await fs.readFile(filePath) };
  } catch (err) {
    if (err.code === 'ENOENT') return { error: 'not_found' };
    if (err.code === 'EACCES') return { error: 'permission_denied' };
    return { error: 'system_error', details: err.message };
  }
}

6. Inconsistent Error Response Format

Files: Multiple handlers
Severity: MEDIUM

Current formats vary:

// Format A - Minimal
{ error: 'Not Found' }

// Format B - With message
{ error: 'Forbidden', message: 'Dotfile access is not allowed' }

// Format C - Extended
{ error: 'Too Many Requests', message: '...', retryAfter: 60 }

Fix: Standardize all error responses:

{
  error: 'ErrorCode',
  message: 'Human readable description',
  statusCode: 404,
  details: { /* optional context */ }
}

Create src/utils/errors.js:

export function errorResponse(reply, code, error, message, details = {}) {
  return reply.code(code).send({
    error,
    message,
    statusCode: code,
    ...details
  });
}

7. Missing Global Security Headers

File: src/server.js
Severity: MEDIUM

Security headers only applied to mashlib responses, not globally.

Fix: Add global onSend hook:

fastify.addHook('onSend', async (request, reply) => {
  reply.header('X-Content-Type-Options', 'nosniff');
  reply.header('X-Frame-Options', 'DENY');
  reply.header('X-XSS-Protection', '1; mode=block');
  reply.header('Referrer-Policy', 'strict-origin-when-cross-origin');
});

8. Request Correlation IDs

Files: All handlers
Severity: MEDIUM

No request IDs for tracing requests through logs.

Fix: Add request ID generation:

fastify.addHook('onRequest', async (request) => {
  request.id = request.headers['x-request-id'] || crypto.randomUUID();
  request.log = request.log.child({ requestId: request.id });
});

fastify.addHook('onSend', async (request, reply) => {
  reply.header('X-Request-ID', request.id);
});

🔧 MEDIUM PRIORITY - Nice to Fix

9. Code Duplication - getRequestPaths()

Files: src/handlers/resource.js:24-31, src/handlers/container.js:15-22
Severity: LOW

Nearly identical implementations.

Fix: Extract to src/utils/paths.js:

export function getRequestPaths(request, dataRoot) {
  const urlPath = decodeURIComponent(request.url.split('?')[0]);
  const fullPath = urlToPath(urlPath);
  return { urlPath, fullPath };
}

10. Magic Numbers/Strings

Files: Multiple
Severity: LOW

Create src/constants.js:

export const LIMITS = {
  SLUG_MAX_LENGTH: 255,
  BODY_MAX_SIZE: 10 * 1024 * 1024,  // 10MB
  WS_MAX_SUBSCRIPTIONS: 100,
  WS_MAX_URL_LENGTH: 2048,
};

export const CACHE = {
  JTI_CLEANUP_INTERVAL: 60 * 1000,
  JTI_TTL: 15 * 60 * 1000,
  DPOP_MAX_AGE: 5 * 60,
};

export const RATE_LIMITS = {
  GLOBAL_PER_MINUTE: 100,
  WRITE_PER_MINUTE: 60,
  POD_CREATION_PER_DAY: 1,
};

11. Complex Functions Need Refactoring

Severity: MEDIUM

Function File Lines Issue
createServer() src/server.js 540 God function, too many responsibilities
handleGet() src/handlers/resource.js 150+ Multiple concerns mixed
authorize() src/auth/middleware.js 80+ Complex nested conditionals

Recommendation: Break into smaller focused functions. Example for createServer():

export async function createServer(options) {
  const fastify = createFastifyInstance(options);
  configureRequestContext(fastify, options);
  registerSecurityPlugins(fastify, options);
  registerFeaturePlugins(fastify, options);
  registerRoutes(fastify, options);
  return fastify;
}

12. Dead Code Removal

File: src/utils/url.js line 11

export let DATA_ROOT = './data';  // Legacy, superseded by getDataRoot()

Fix: Remove legacy export, use only getDataRoot() function.


13. ACL Lookup Caching

File: src/wac/checker.js lines 74-98
Severity: LOW (Performance)

while (currentStoragePath && currentStoragePath !== '/') {
  // Linear walk up hierarchy for every request - no caching
}

Fix: Add LRU cache for parsed ACL rules:

import { LRUCache } from 'lru-cache';

const aclCache = new LRUCache({ max: 500, ttl: 5 * 60 * 1000 });

async function getAcl(path) {
  const cached = aclCache.get(path);
  if (cached) return cached;
  const acl = await loadAndParseAcl(path);
  aclCache.set(path, acl);
  return acl;
}

📝 DOCUMENTATION

14. Missing JSDoc Comments

Priority: MEDIUM

File Status
src/storage/filesystem.js ❌ Most functions undocumented
src/handlers/resource.js ⚠️ Partial
src/wac/parser.js ❌ Complex ACL parsing undocumented
src/ap/routes/inbox.js ❌ ActivityPub flow undocumented
src/auth/nostr.js ❌ Schnorr verification undocumented

15. Undocumented API Behaviors

Priority: MEDIUM

Document in README or API docs:

  • POST /.pods endpoint request/response format
  • IdP issuer URL trailing slash handling
  • Subdomain mode pod URL structure
  • WebSocket protocol version (solid-0.1) compatibility
  • Rate limiting headers and retry behavior

🧪 TESTING

16. Test Coverage Gaps

Priority: MEDIUM

Area Status Priority
Git handler (src/handlers/git.js) ❌ No tests HIGH
Range requests (streaming) ❌ No tests MEDIUM
ActivityPub federation ❌ Limited MEDIUM
Nostr relay stress tests ❌ None LOW
Concurrent request handling ❌ None MEDIUM
WebID-TLS auth ❌ No tests LOW

17. Missing Integration Tests

Priority: MEDIUM

  • Auth + WAC + Resource access combined
  • Quota enforcement during multi-file operations
  • IdP registration + pod access flow
  • Concurrent modification conflicts

🏗️ ARCHITECTURE (Post-v0.1)

18. Storage Abstraction

Priority: LOW (v0.2)

Currently hardcoded to filesystem. Abstract for future backends:

// src/storage/interface.js
export interface StorageAdapter {
  read(path: string): Promise<Buffer | null>;
  write(path: string, data: Buffer): Promise<void>;
  delete(path: string): Promise<void>;
  exists(path: string): Promise<boolean>;
  stat(path: string): Promise<Stats>;
  list(path: string): Promise<string[]>;
}

// src/storage/filesystem.js
export class FilesystemAdapter implements StorageAdapter { ... }

// Future: src/storage/s3.js, src/storage/sqlite.js

19. Request Context Object

Priority: LOW (v0.2)

Current approach decorates Fastify request with 15+ properties. Create proper context:

// Instead of request.connegEnabled, request.mashlibEnabled, etc.
class RequestContext {
  constructor(request, config) {
    this.config = config;
    this.features = {
      conneg: config.conneg,
      notifications: config.notifications,
      // ...
    };
    this.pod = this.extractPodFromRequest(request);
  }
}

20. Consolidate Logging

Priority: MEDIUM

Replace mixed console.log/warn/error with Fastify logger:

// Before
console.error('Config error:', err);

// After
fastify.log.error({ err }, 'Config error');

📋 IMPLEMENTATION CHECKLIST

Phase 1: Security Fixes (BLOCKER for v0.1)

  • Fix path traversal in git handler
  • Implement or warn on agent group authorization
  • Fix WebSocket subscription cleanup
  • Use safeJsonParse() consistently (5 files)
  • Add global security headers

Phase 2: Error Handling (HIGH for v0.1)

  • Create standardized error response helper
  • Update all handlers to use standard format
  • Replace silent catches with proper error returns
  • Add request correlation IDs

Phase 3: Code Quality (MEDIUM)

  • Extract shared utilities (getRequestPaths, etc.)
  • Create constants.js for magic numbers
  • Remove dead code (legacy DATA_ROOT export)
  • Add JSDoc to undocumented functions

Phase 4: Testing (MEDIUM)

  • Add git handler tests
  • Add integration tests for auth + WAC flow
  • Add range request tests
  • Add concurrent modification tests

Phase 5: Documentation (MEDIUM)

  • Document .pods endpoint
  • Document rate limiting behavior
  • Document WebSocket protocol
  • Add API reference for IdP endpoints

Phase 6: Performance (LOW - can defer)

  • Add ACL lookup caching
  • Cache etag calculations
  • Profile and optimize hot paths

Phase 7: Architecture (v0.2)

  • Abstract storage interface
  • Create request context objects
  • Refactor createServer() into smaller functions
  • Add observability/metrics framework

EFFORT ESTIMATES

Phase Difficulty Days Priority
Phase 1: Security 35/100 2-3 P0 BLOCKER
Phase 2: Error Handling 25/100 2 P0
Phase 3: Code Quality 20/100 1-2 P1
Phase 4: Testing 30/100 2-3 P1
Phase 5: Documentation 15/100 1 P1
Phase 6: Performance 25/100 1-2 P2
Phase 7: Architecture 50/100 3-5 v0.2

Total for v0.1: ~10-13 days of focused work


DEPENDENCIES

This issue relates to:

Recommended order:

  1. This issue (v0.1 readiness) - Phases 1-5
  2. Feature: Docker Support - Dockerfile, Compose, and Container Registry #99 Docker support
  3. Feature: Configuration Presets for Common Use Cases #97 Presets
  4. Feature: Enhanced CLI Setup Wizard & Developer Experience #98 CLI Wizard
  5. Feature: Admin Onboarding Wizard & Configuration Improvements #95 Web Onboarding
  6. Feature: Admin Control Panel for Server Management #96 Admin Panel

SUCCESS CRITERIA FOR v0.1

  • All CRITICAL and HIGH issues resolved
  • No known security vulnerabilities
  • Consistent error response format
  • Request tracing via correlation IDs
  • Core functionality has test coverage
  • API behaviors documented
  • Clean npm audit (0 vulnerabilities)
  • Docker image published to ghcr.io

References

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions