Security fixes from audit v0.0.71 by jjohare · Pull Request #57 · JavaScriptSolidServer/JavaScriptSolidServer

jjohare · 2026-01-05T15:14:18Z

Summary

This PR addresses security findings from a comprehensive multi-agent security audit of JavaScriptSolidServer v0.0.71.

Security Fixes (6 commits)

Severity	Issue	Fix
HIGH	Path traversal in git handler	Added iterative `..` removal and boundary validation in `src/handlers/git.js`
MEDIUM	DNS failure SSRF bypass	Changed to block requests when DNS resolution fails in `src/utils/ssrf.js`
MEDIUM	DPoP jti replay attacks	Added time-bounded jti cache in `src/auth/solid-oidc.js`
MEDIUM	Unsafe JSON parsing in WAC	Use `safeJsonParse()` with 10MB limit in `src/wac/parser.js`
LOW	podName single-pass sanitization	Iterative `..` removal in `src/utils/url.js`
LOW	No password strength validation	Minimum 8-char requirement in `src/idp/interactions.js`

Audit Report

Includes updated SECURITY-AUDIT-2026-01-05.md with comprehensive findings
Removes invalid future-dated audit file

Testing

Code review of all changes for correctness
Follows existing patterns in codebase
No breaking changes to public API

Test plan

Run existing test suite
Manual verification of path traversal protection
Test DPoP authentication with replay attempt
Test registration with weak password (should fail)

🤖 Generated with Claude Code

The git HTTP handler was vulnerable to path traversal attacks because it did not sanitize user input before passing it to path.resolve(). Attack vector: An attacker could craft URLs like: GET /../../etc/passwd/info/refs?service=git-upload-pack This could potentially allow reading files outside DATA_ROOT when git endpoints are enabled. Fix: - Add iterative '..' removal in extractRepoPath() (matches urlToPath pattern) - Add isPathWithinDataRoot() validation after path.resolve() - Import getDataRoot() for consistent data root access - Return 403 Forbidden when path traversal is detected Severity: HIGH CVSS: 7.5 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Previously, when DNS resolution failed during SSRF validation, the request was allowed to proceed. This could be exploited by an attacker using DNS manipulation or timing attacks to bypass SSRF protection. Fix: - Return { valid: false } when DNS resolution fails - Log a warning for security monitoring - Provide clear error message indicating DNS failure This is a breaking change for edge cases where legitimate external services have temporary DNS issues, but security takes precedence. Severity: MEDIUM CVSS: 4.9 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Add time-bounded jti cache to prevent DPoP proof replay attacks within the 5-minute validity window. This addresses MEDIUM severity finding from security audit. Changes: - Add dpopJtiCache Map to store jti->timestamp mappings - Add periodic cleanup (every 60s) to prevent memory growth - Add isJtiUsed() check before accepting DPoP proof - Record jti after successful verification with recordJti() The jti (JWT ID) is a unique identifier in DPoP proofs. Without tracking, the same proof could be replayed multiple times within its validity window. This fix ensures each DPoP proof can only be used once. CVSS: 5.3 (Medium) - Token replay within short window 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Change podName sanitization from single-pass to iterative removal of '..' sequences, matching the approach used for urlPath. Before: `podName.replace(/\.\./g, '')` After: Loop until no more '..' sequences remain This prevents bypass attempts like '....' being reduced to '..' in a single pass. While the subsequent boundary check provides defense in depth, consistent sanitization across all inputs is best practice. CVSS: 3.1 (Low) - Defense hardening, mitigated by existing boundary check 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Replace raw JSON.parse() with safeJsonParse() which enforces a 10MB size limit before parsing. This prevents memory exhaustion attacks via maliciously large ACL documents. The safeJsonParse utility was already available in utils/url.js but wasn't being used consistently across the codebase. This addresses the audit finding about inconsistent JSON parsing protection. CVSS: 5.3 (Medium) - DoS via large JSON payloads 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Enforce minimum password length of 8 characters during registration. Previously only password confirmation matching was validated. This is a baseline security requirement that prevents trivially weak passwords while remaining user-friendly. Future enhancements could include complexity requirements or breach database checking. CVSS: 3.1 (Low) - Weak password prevention 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

- Create comprehensive security audit for v0.0.71 - Remove invalid future-dated audit file (2026-01-15) - Document all findings from 5-agent parallel security review - Track remediation status from previous audits (v0.0.48-0.0.51) This audit covers: - Authentication and token security - Path traversal and SSRF protection - DoS resistance and input validation - WAC authorization implementation - Dependency security (npm audit: 0 vulnerabilities) Overall posture: GOOD - All critical issues from previous audits fixed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Copilot

Pull request overview

This PR implements security fixes for JavaScriptSolidServer v0.0.71 based on findings from a comprehensive security audit. The changes address 6 vulnerabilities ranging from HIGH to LOW severity, focusing on path traversal protection, SSRF prevention, DPoP replay attack mitigation, and input validation.

Key changes include:

Enhanced path traversal protection in the git handler with iterative sanitization and boundary validation
Stricter SSRF protection that blocks requests when DNS resolution fails
DPoP jti tracking with time-bounded caching to prevent replay attacks within the 5-minute validity window
Consistent use of safe JSON parsing with size limits in the WAC parser
Iterative podName sanitization to prevent multi-pass traversal bypasses
Minimum 8-character password requirement for new account registration

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/handlers/git.js	Adds path traversal protection to git handler including iterative `..` removal, boundary validation function, and data root verification
src/utils/ssrf.js	Changes DNS resolution failure handling from allowing with warning to blocking the request for better SSRF protection
src/auth/solid-oidc.js	Implements DPoP jti replay attack prevention with time-bounded cache, cleanup interval, and validation functions
src/wac/parser.js	Replaces unsafe `JSON.parse()` with `safeJsonParse()` that enforces 10MB size limit for DoS protection
src/utils/url.js	Enhances podName sanitization from single-pass to iterative `..` removal to prevent bypass attempts
src/idp/interactions.js	Adds minimum 8-character password length validation during user registration
SECURITY-AUDIT-2026-01-05.md	Adds comprehensive security audit report documenting vulnerabilities, fixes, and recommendations
SECURITY-AUDIT-2026-01-15.md	Removes invalid future-dated audit file

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-05T16:22:09Z

+  // Security: remove path traversal attempts (multiple passes for ....// bypass)
+  let previous;
+  do {
+    previous = cleanPath;
+    cleanPath = cleanPath.replace(/\.\./g, '');
+  } while (cleanPath !== previous);


The path sanitization removes all occurrences of ".." which could result in unintended modifications to legitimate paths. For example, a repository path like "my..repo" or "node..modules" would become "myrepo" or "nodemodules", potentially breaking legitimate use cases. Consider using a more precise approach that only removes path traversal sequences (e.g., "../" or "..") rather than all occurrences of two consecutive dots.

Suggested change

// Security: remove path traversal attempts (multiple passes for ....// bypass)

let previous;

do {

previous = cleanPath;

cleanPath = cleanPath.replace(/\.\./g, '');

} while (cleanPath !== previous);

// Security: normalize path and remove traversal segments while preserving legitimate names

const parts = cleanPath.split('/');

const safeParts = [];

for (const part of parts) {

if (part === '' || part === '.') {

continue;

}

if (part === '..') {

if (safeParts.length > 0) {

safeParts.pop();

}

continue;

}

safeParts.push(part);

}

cleanPath = safeParts.join('/');

Copilot · 2026-01-05T16:22:10Z

+}
+
+// Start periodic cleanup
+setInterval(cleanupJtiCache, JTI_CACHE_CLEANUP_INTERVAL);


The setInterval at module scope will prevent the Node.js process from cleanly exiting because it creates an active timer reference. This is particularly problematic in test environments or during graceful shutdowns. Consider using setInterval(...).unref() to allow the process to exit when there are no other active operations, or provide a cleanup function to clear the interval during shutdown.

Suggested change

setInterval(cleanupJtiCache, JTI_CACHE_CLEANUP_INTERVAL);

const jtiCleanupInterval = setInterval(cleanupJtiCache, JTI_CACHE_CLEANUP_INTERVAL);

jtiCleanupInterval.unref();

Copilot · 2026-01-05T16:22:10Z


+    // Check for replay attack
+    if (isJtiUsed(payload.jti)) {
+      return { thumbprint: null, error: 'DPoP proof jti already used (replay attack prevented)' };


The error message reveals internal implementation details by explicitly mentioning "replay attack prevented". From a security perspective, it's generally better to use a more generic error message like "Invalid or expired DPoP proof" to avoid leaking information about security mechanisms to potential attackers. This makes it harder for attackers to understand why their attack failed and refine their approach.

Suggested change

return { thumbprint: null, error: 'DPoP proof jti already used (replay attack prevented)' };

return { thumbprint: null, error: 'Invalid or expired DPoP proof' };

Copilot · 2026-01-05T16:22:11Z

+function isPathWithinDataRoot(resolvedPath, dataRoot) {
+  const normalizedRoot = resolve(dataRoot);
+  const normalizedPath = resolve(resolvedPath);
+  return normalizedPath.startsWith(normalizedRoot + '/') || normalizedPath === normalizedRoot;


The path validation logic has a potential flaw on Windows systems. The check uses a hardcoded forward slash separator, but Windows uses backslashes. This means on Windows, a path like C:\data\root\subdir would not match C:\data\root/ and would incorrectly fail validation even for legitimate paths within the data root. Consider using path.sep for cross-platform compatibility.

melvincarvalho

Review: LGTM - Approve for merge

Reviewed each change for correctness and risk:

1. DPoP jti replay prevention (`src/auth/solid-oidc.js`) ✅

Sound implementation with dpopJtiCache Map
Periodic cleanup prevents memory growth
Follows the 5-min DPOP_MAX_AGE window
Low risk

2. Git handler path traversal fix (`src/handlers/git.js`) ✅

Iterative .. removal matches existing urlToPath() pattern
Adds isPathWithinDataRoot() boundary check (defense-in-depth)
Returns 403 on traversal detection
Low risk

3. Password length validation (`src/idp/interactions.js`) ✅

Simple 8-char minimum - reasonable baseline
Low risk, good UX

4. DNS failure SSRF blocking (`src/utils/ssrf.js`) ✅

More secure than allowing on DNS failure
Could have false positives for legitimate services with DNS issues, but security takes precedence
Low risk

5. podName iterative sanitization (`src/utils/url.js`) ✅

Makes podName match urlPath pattern (consistency)
Already had boundary check as defense-in-depth
Low risk

6. safeJsonParse in WAC parser (`src/wac/parser.js`) ✅

Uses existing utility, adds DoS protection
Low risk

7. Audit files ✅

New comprehensive audit for v0.0.71
Removes invalid future-dated file (2026-01-15)

All changes follow existing codebase patterns, are security-focused, and have low breakage risk. Good contribution! 🎉

melvincarvalho · 2026-01-05T16:30:21Z

Responses to Copilot review comments

1. `git.js:46` - `..` removal breaking legitimate paths like `my..repo`

Valid edge case, but repository names containing .. are extremely rare in practice. The existing urlToPath() function uses the same pattern for consistency. Security takes precedence here - we can add an allowlist for specific patterns if real-world use cases emerge.

2. `solid-oidc.js:60` - `setInterval` preventing clean exit

Fair point for test environments. Using .unref() would be cleaner. For a long-running Solid server this isn't an issue, but worth considering for future improvement. Not a blocker.

3. `solid-oidc.js:226` - Error message leaks "replay attack"

Mildly valid, but DPoP errors are already implementation-specific per the spec. Attackers already know DPoP exists and how it works. Low security risk. Could be genericized in a follow-up if desired.

4. `git.js:61` - Windows path separator

Valid concern for Windows. JSS is primarily Linux-targeted (Solid pods typically run on Linux servers). The existing urlToPath() also uses /. Could be improved with path.sep in a future cross-platform effort, but low risk for current deployment scenarios.

Overall: These are good observations for future hardening, but none are blockers for this security-focused PR. The changes follow existing patterns and address real vulnerabilities.

@jjohare

Merged PR #57 (thanks @jjohare!): - DPoP jti replay attack prevention - Git handler path traversal fix - DNS failure blocks SSRF (fail-secure) - Password minimum length (8 chars) - podName iterative sanitization - safeJsonParse in WAC parser

jjohare and others added 7 commits January 5, 2026 15:00

melvincarvalho requested a review from Copilot January 5, 2026 16:18

Copilot started reviewing on behalf of melvincarvalho January 5, 2026 16:18 View session

Copilot AI reviewed Jan 5, 2026

View reviewed changes

melvincarvalho approved these changes Jan 5, 2026

View reviewed changes

melvincarvalho merged commit b2f3f54 into JavaScriptSolidServer:gh-pages Jan 5, 2026
6 checks passed

Navigation Menu

Uh oh!

Security fixes from audit v0.0.71 - #57

Security fixes from audit v0.0.71#57
melvincarvalho merged 7 commits into
JavaScriptSolidServer:gh-pagesfrom
jjohare:gh-pages

jjohare commented Jan 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 5, 2026

Uh oh!

Copilot AI Jan 5, 2026

Uh oh!

Copilot AI Jan 5, 2026

Uh oh!

Copilot AI Jan 5, 2026

Uh oh!

melvincarvalho left a comment

Uh oh!

melvincarvalho commented Jan 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-  // Security: remove path traversal attempts (multiple passes for ....// bypass)
-  let previous;
-  do {
-    previous = cleanPath;
-    cleanPath = cleanPath.replace(/\.\./g, '');
-  } while (cleanPath !== previous);
+  // Security: normalize path and remove traversal segments while preserving legitimate names
+  const parts = cleanPath.split('/');
+  const safeParts = [];
+  for (const part of parts) {
+    if (part === '' || part === '.') {
+      continue;
+    }
+    if (part === '..') {
+      if (safeParts.length > 0) {
+        safeParts.pop();
+      }
+      continue;
+    }
+    safeParts.push(part);
+  }
+  cleanPath = safeParts.join('/');

	setInterval(cleanupJtiCache, JTI_CACHE_CLEANUP_INTERVAL);
	const jtiCleanupInterval = setInterval(cleanupJtiCache, JTI_CACHE_CLEANUP_INTERVAL);
	jtiCleanupInterval.unref();

	return { thumbprint: null, error: 'DPoP proof jti already used (replay attack prevented)' };
	return { thumbprint: null, error: 'Invalid or expired DPoP proof' };

Uh oh!

Conversation

jjohare commented Jan 5, 2026

Summary

Security Fixes (6 commits)

Audit Report

Testing

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

melvincarvalho left a comment

Choose a reason for hiding this comment

Review: LGTM - Approve for merge

1. DPoP jti replay prevention (src/auth/solid-oidc.js) ✅

2. Git handler path traversal fix (src/handlers/git.js) ✅

3. Password length validation (src/idp/interactions.js) ✅

4. DNS failure SSRF blocking (src/utils/ssrf.js) ✅

5. podName iterative sanitization (src/utils/url.js) ✅

6. safeJsonParse in WAC parser (src/wac/parser.js) ✅

7. Audit files ✅

Uh oh!

melvincarvalho commented Jan 5, 2026

Responses to Copilot review comments

1. git.js:46 - .. removal breaking legitimate paths like my..repo

2. solid-oidc.js:60 - setInterval preventing clean exit

3. solid-oidc.js:226 - Error message leaks "replay attack"

4. git.js:61 - Windows path separator

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

1. DPoP jti replay prevention (`src/auth/solid-oidc.js`) ✅

2. Git handler path traversal fix (`src/handlers/git.js`) ✅

3. Password length validation (`src/idp/interactions.js`) ✅

4. DNS failure SSRF blocking (`src/utils/ssrf.js`) ✅

5. podName iterative sanitization (`src/utils/url.js`) ✅

6. safeJsonParse in WAC parser (`src/wac/parser.js`) ✅

1. `git.js:46` - `..` removal breaking legitimate paths like `my..repo`

2. `solid-oidc.js:60` - `setInterval` preventing clean exit

3. `solid-oidc.js:226` - Error message leaks "replay attack"

4. `git.js:61` - Windows path separator