Add a JS moq-token package.

[kixelated]

No description provided.

[claude]

Claude finished @kixelated's task —— View job

---

Code Review Complete

Todo List:

• Read project context and guidelines
• Review package structure and configuration
• Analyze TypeScript source files
• Check for security concerns
• Assess code quality and best practices
• Provide comprehensive feedback

Summary

This is a JavaScript/TypeScript port of the existing Rust moq-token package for JWT-based authentication in the MoQ project. The package provides functionality for signing and verifying JWT tokens with HMAC algorithms. Overall, the code is well-structured and follows modern TypeScript practices.

Issues Found

🐛 Critical Bug

Location: js/moq-token/src/claims.ts:12-16

.check(
    z.refine((data) => data.publish || data.subscribe, {
        message: "Either publish or subscribe must be specified",
    }),
);

Issue: Incorrect usage of Zod's check method. The check method doesn't accept a z.refine() call.

Fix: Should be either:

.refine((data) => data.publish || data.subscribe, {
    message: "Either publish or subscribe must be specified",
})

or use superRefine for more complex validation.

🔒 Security Concerns

1. Missing Secret Validation (js/moq-token/src/key.ts:34, 52)

   • The secret field from the JWK is used directly without validation
   • Should validate that the secret is properly base64url encoded
   • Should validate minimum secret length for security

2. JSON Parsing (js/moq-token/src/key.ts:24)

   • JSON.parse() without try-catch could expose sensitive error information
   • Should wrap in try-catch with sanitized error messages

3. Token Expiration (js/moq-token/src/key.ts:47-60)

   • The verification function doesn't explicitly handle or validate the expires field from claims
   • While Jose may handle standard JWT exp claims, the custom expires field is not validated

📋 Code Quality Issues

1. Inconsistent Import Style (js/moq-token/src/key.ts:3)

   • Uses .js extension in imports: "./algorithm.js"
   • Should be consistent with other imports that don't use extensions

2. Missing Error Context (js/moq-token/src/key.ts:31, 49)

   • Error messages don't include which operation was attempted
   • Could be more descriptive for debugging

3. Type Safety (js/moq-token/src/key.ts:57)

   • claimsSchema.parse(payload) could throw - should handle parsing errors gracefully

🏗️ Package Configuration

Good practices observed:

• ✅ Proper TypeScript configuration extending parent tsconfig
• ✅ Correct pnpm workspace integration
• ✅ Appropriate dependency choices (Jose for JWT, Zod for validation)
• ✅ Good package.json structure with proper exports

Areas for improvement:

• Missing repository field URL in package.json (has "repository": "github:kixelated/moq" but should be full URL)
• No keywords field for discoverability

📚 Documentation

Strengths:

• ✅ Comprehensive README with usage examples
• ✅ Clear API documentation
• ✅ Good TypeScript types with JSDoc comments

Missing:

• License information in README (only in package.json)
• No mention of compatibility with Rust implementation
• No troubleshooting section

🧪 Testing

Major Gap: No tests included in the package

• Should have unit tests for load(), sign(), and verify() functions
• Should test error conditions and edge cases
• Should verify interoperability with Rust implementation

🚀 Performance Considerations

• Uses jose library which is well-optimized
• No obvious performance bottlenecks
• Could consider caching parsed keys if used repeatedly

💡 Recommendations

1. Fix the critical Zod bug - this will cause runtime errors
2. Add comprehensive tests - essential for a security-focused package
3. Improve error handling - wrap JSON parsing and add better error messages
4. Add secret validation - validate format and minimum length
5. Consider adding key caching - for repeated use scenarios
6. Add integration tests - verify compatibility with Rust implementation

🎯 Security Best Practices

The package handles sensitive authentication tokens and should follow security best practices:

• ✅ Uses established JWT library (Jose)
• ✅ Supports only secure algorithms (HMAC-SHA)
• ❌ Missing input validation for secrets
• ❌ No explicit expiration handling
• ❌ Error messages could leak information

Overall Assessment

This is a solid foundation for a JWT authentication package. The code structure is clean, follows TypeScript best practices, and integrates well with the existing project. However, there are some critical issues that need to be addressed before merging, particularly the Zod validation bug and missing test coverage.

Recommendation: Request changes to fix the critical bug and add tests before merging.

[claude]

Claude finished @kixelated's task —— View job

---

Security and Bug Review

Todo List:

• Read project context and setup
• Review main authentication logic in key.ts
• Analyze claims validation in claims.ts
• Check algorithm handling
• Identify security vulnerabilities
• Document bug findings
• Provide final review summary

Critical Bug Found

🐛 IAT Claim Overwrite Bug (js/moq-token/src/key.ts:84)

The sign function incorrectly overwrites the iat (issued at) claim:

const jwt = await new jose.SignJWT(claims)
    .setProtectedHeader({
        alg: key.alg,
        typ: "JWT",
        ...(key.kid && { kid: key.kid }),
    })
    .setIssuedAt() // ← This overwrites any existing iat in claims
    .sign(secret);

Impact: When a caller provides an explicit iat value in the claims object, it gets overwritten by the current timestamp. Test js/moq-token/src/key.test.ts:190 expects the original iat to be preserved.

Fix: Only call setIssuedAt() if claims.iat is not provided:

const jwtBuilder = new jose.SignJWT(claims)
    .setProtectedHeader({
        alg: key.alg,
        typ: "JWT", 
        ...(key.kid && { kid: key.kid }),
    });

if (!claims.iat) {
    jwtBuilder.setIssuedAt();
}

const jwt = await jwtBuilder.sign(secret);

Security Assessment

✅ Security Strengths:

• Secret validation enforces minimum 32-byte length and base64url encoding
• Algorithm restriction to HMAC-only (secure for this use case)
• Comprehensive input validation with Zod schemas
• Proper path validation in verification
• Error messages don't leak sensitive information
• Uses secure Jose JWT library

No additional security vulnerabilities found. The implementation follows JWT security best practices.

Summary

One critical bug identified that breaks the expected API contract for iat claims. The test suite correctly validates this expectation, confirming this is a bug that needs fixing. All security aspects are properly implemented.

---

[claude]

Claude finished @kixelated's task —— View job

---

Bug and Security Review

Todo List:

• Read main source files for bugs and security issues
• Analyze JWT authentication implementation
• Check for security vulnerabilities
• Provide concise bug and security findings

Critical Bug Found

🐛 IAT Claim Overwrite Bug (js/moq-token/src/key.ts:84)

The sign function incorrectly overwrites the iat (issued at) claim:

const jwt = await new jose.SignJWT(claims)
    .setProtectedHeader({
        alg: key.alg,
        typ: "JWT",
        ...(key.kid && { kid: key.kid }),
    })
    .setIssuedAt() // ← This overwrites any existing iat in claims
    .sign(secret);

Impact: When a caller provides an explicit iat value in the claims object, it gets overwritten by the current timestamp. Test js/moq-token/src/key.test.ts:190 expects the original iat to be preserved.

Fix: Only call setIssuedAt() if claims.iat is not provided:

const jwtBuilder = new jose.SignJWT(claims)
    .setProtectedHeader({
        alg: key.alg,
        typ: "JWT", 
        ...(key.kid && { kid: key.kid }),
    });

if (!claims.iat) {
    jwtBuilder.setIssuedAt();
}

const jwt = await jwtBuilder.sign(secret);

Security Assessment

✅ Security Strengths:

• Secret validation enforces minimum 32-byte length and base64url encoding
• Algorithm restriction to HMAC-only (secure for this use case)
• Comprehensive input validation with Zod schemas
• Proper path validation in verification
• Error messages don't leak sensitive information
• Uses secure Jose JWT library

No additional security vulnerabilities found. The implementation follows JWT security best practices.

Summary

One critical bug identified that breaks the expected API contract for iat claims. The test suite correctly validates this expectation, confirming this is a bug that needs fixing. All security aspects are properly implemented.

---

[claude]

Claude finished @kixelated's task —— View job

---

Bug and Security Review

Todo List:

• Read main source files for bugs and security issues
• Analyze JWT authentication implementation
• Check for security vulnerabilities
• Provide concise bug and security findings

Critical Bug Found

🐛 IAT Claim Overwrite Bug (js/moq-token/src/key.ts:84)

The sign function incorrectly overwrites the iat (issued at) claim:

const jwt = await new jose.SignJWT(claims)
    .setProtectedHeader({
        alg: key.alg,
        typ: "JWT",
        ...(key.kid && { kid: key.kid }),
    })
    .setIssuedAt() // ← This overwrites any existing iat in claims
    .sign(secret);

Impact: When a caller provides an explicit iat value in the claims object, it gets overwritten by the current timestamp. Test js/moq-token/src/key.test.ts:190 expects the original iat to be preserved.

Fix: Only call setIssuedAt() if claims.iat is not provided:

const jwtBuilder = new jose.SignJWT(claims)
    .setProtectedHeader({
        alg: key.alg,
        typ: "JWT", 
        ...(key.kid && { kid: key.kid }),
    });

if (!claims.iat) {
    jwtBuilder.setIssuedAt();
}

const jwt = await jwtBuilder.sign(secret);

Security Assessment

✅ Security Strengths:

• Secret validation enforces minimum 32-byte length and base64url encoding
• Algorithm restriction to HMAC-only (secure for this use case)
• Comprehensive input validation with Zod schemas
• Proper path validation in verification
• Error messages don't leak sensitive information
• Uses secure Jose JWT library

No additional security vulnerabilities found. The implementation follows JWT security best practices.

Summary

One critical bug identified that breaks the expected API contract for iat claims. The test suite correctly validates this expectation, confirming this is a bug that needs fixing. All security aspects are properly implemented.

---

[claude]

Claude finished @kixelated's task —— View job

---

Bug and Security Review

Todo List:

• Read main source files for bugs and security issues
• Analyze JWT authentication implementation
• Check for security vulnerabilities
• Provide concise bug and security findings

Critical Bug Found

🐛 IAT Claim Overwrite Bug (js/moq-token/src/key.ts:84)

The sign function incorrectly overwrites the iat (issued at) claim:

const jwt = await new jose.SignJWT(claims)
    .setProtectedHeader({
        alg: key.alg,
        typ: "JWT",
        ...(key.kid && { kid: key.kid }),
    })
    .setIssuedAt() // ← This overwrites any existing iat in claims
    .sign(secret);

Impact: When a caller provides an explicit iat value in the claims object, it gets overwritten by the current timestamp. Test js/moq-token/src/key.test.ts:190 expects the original iat to be preserved.

Fix: Only call setIssuedAt() if claims.iat is not provided:

const jwtBuilder = new jose.SignJWT(claims)
    .setProtectedHeader({
        alg: key.alg,
        typ: "JWT", 
        ...(key.kid && { kid: key.kid }),
    });

if (!claims.iat) {
    jwtBuilder.setIssuedAt();
}

const jwt = await jwtBuilder.sign(secret);

Security Assessment

✅ Security Strengths:

• Secret validation enforces minimum 32-byte length and base64url encoding
• Algorithm restriction to HMAC-only (secure for this use case)
• Comprehensive input validation with Zod schemas
• Proper path validation in verification
• Error messages don't leak sensitive information
• Uses secure Jose JWT library

No additional security vulnerabilities found. The implementation follows JWT security best practices.

Summary

One critical bug identified that breaks the expected API contract for iat claims. The test suite correctly validates this expectation, confirming this is a bug that needs fixing. All security aspects are properly implemented.

---