Skip to content

Fix TypeScript build errors for CitationComponent and Radix UI types (Vibe Kanban)#26

Merged
bensonwong merged 1 commit intomainfrom
d5d1-fix-citation-and
Jan 16, 2026
Merged

Fix TypeScript build errors for CitationComponent and Radix UI types (Vibe Kanban)#26
bensonwong merged 1 commit intomainfrom
d5d1-fix-citation-and

Conversation

@bensonwong
Copy link
Collaborator

@bensonwong bensonwong commented Jan 16, 2026 •
edited
Loading

Summary

This PR fixes TypeScript build errors that occurred after integrating the Radix UI Popover component into CitationComponent.

Changes Made

1. Fixed implicit any type errors in src/react/CitationComponent.tsx

Added explicit Event type annotations to the Radix Popover event handlers:

onPointerDownOutside={(e: Event) => e.preventDefault()}
onInteractOutside={(e: Event) => e.preventDefault()}

These handlers prevent the popover from closing when interacting with its content, which is important for the hover-to-show behavior.

2. Updated TypeScript configuration in tsconfig.json

Extended the types array to include React type definitions:

"types": ["node", "react", "react-dom"]

The previous configuration only included "node" types, which prevented TypeScript from resolving the @radix-ui/react-popover module types. Adding "react" and "react-dom" allows the Radix UI component types to be properly resolved.

Why These Changes Were Needed

The CitationComponent was recently refactored to use shadcn/Radix Popover (PR #24) for improved tooltip/popover functionality. However, the TypeScript configuration wasn't updated to support the new Radix UI dependencies, causing the following build errors:

  • TS7006: Parameter 'e' implicitly has an 'any' type (lines 820-821)
  • TS2307: Cannot find module '@radix-ui/react-popover'

Testing

  • Build passes successfully (npm run build)
  • All 339 unit tests pass

This PR was written using Vibe Kanban

@bensonwong
I've fixed the build errors. Here's a summary of the changes made:
d270421 
## Changes Made

### 1. `tsconfig.json`
Added `"react"` and `"react-dom"` to the types array so TypeScript can find the Radix UI types:
```json
"types": ["node", "react", "react-dom"],
```

### 2. `src/react/CitationComponent.tsx` (lines 820-821)
Added explicit `Event` type annotations to the popover event handlers to fix the implicit `any` type errors:
```typescript
onPointerDownOutside={(e: Event) => e.preventDefault()}
onInteractOutside={(e: Event) => e.preventDefault()}
```

## Results

- **Build**: ✅ Passes successfully
- **Unit Tests**: ✅ 339 tests pass
- **Note**: 3 Playwright `.spec.tsx` files fail because they're being incorrectly picked up by `bun test` instead of the Playwright component test runner (`npm run test:ct`). This is a pre-existing test configuration issue unrelated to the PRD implementation.

The CitationComponent is properly implemented according to the PRD with:
- Radix UI Popover integration
- All display variants (brackets, text, minimal, indicator)
- Status indicators (pending, verified, partial, not found)
- Image overlay with zoom functionality
- Diff highlighting using the `diff` library
- Custom behavior configuration
@bensonwong bensonwong changed the title fix citation and build (vibe-kanban) Fix TypeScript build errors for CitationComponent and Radix UI types (Vibe Kanban) Jan 16, 2026
@bensonwong bensonwong merged commit 39e04fd into main Jan 16, 2026
@bensonwong bensonwong deleted the d5d1-fix-citation-and branch January 16, 2026 02:59
@claude claude bot mentioned this pull request Jan 31, 2026
Merged
5 tasks
bensonwong added a commit that referenced this pull request Feb 15, 2026
@bensonwong @claude
fix: address remaining CodeQL security alerts
b004a6a 
This commit fixes the remaining critical security issues flagged by CodeQL:

**Prototype Pollution Fixes:**

1. parseCitation.ts:698 (Alert #33)
   - Fixed groupCitationsByAttachmentIdObject() prototype pollution
   - Changed from Record<string, unknown> to createSafeObject()
   - Added isSafeKey() validation before assigning citation keys
   - Prevents "__proto__" and other dangerous keys from polluting prototype

2. citationParser.ts:85 (Alert #46)
   - Fixed expandCompactKeys() remote property injection
   - Changed from Record<string, unknown> to createSafeObject()
   - Added isSafeKey() check before assigning fullKey values
   - Prevents malicious keys in user-provided citation data

**URL Sanitization Fixes:**

3. SourcesListComponent.utils.tsx (Alerts #26-27)
   - Fixed remaining .includes() substring matching vulnerabilities
   - Changed merriam-webster.com to use isDomainMatch()
   - Changed dictionary.com to use isDomainMatch()
   - Prevents domain spoofing attacks (e.g., dictionary.com.evil.com)

4. Commerce Domain Detection (Alerts #20-30 partial)
   - Improved amazon/ebay domain detection for regional TLDs
   - Now uses extractDomain() + startsWith() for proper validation
   - Prevents false positives from amazon.evil.com
   - Handles legitimate regional domains (amazon.co.uk, ebay.com.au)

**Security Impact:**
- 3 critical prototype pollution vulnerabilities fixed
- 2 URL spoofing vulnerabilities fixed
- Reduces CodeQL alert count by 5 issues
- All security fixes use our safe utility wrappers

**Testing:**
- All 1170 tests pass ✓
- Build succeeds without errors
- Lint passes cleanly
- No breaking changes to public API

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
bensonwong added a commit that referenced this pull request Feb 15, 2026
@bensonwong
security: add comprehensive safety utilities and upgrade dependencies (…
dc0f71f 
…#237)

* chore: upgrade type packages and document dependency strategy

Upgrade safe packages with no breaking changes:
- @types/jest 29.5.14 → 30.0.0 (type definitions only)
- @types/node 24.10.13 → 25.2.3 (type definitions only)
- @vitejs/plugin-react 4.7.0 → 5.1.4 (no breaking changes)

Add comprehensive upgrade documentation:
- DEPENDENCY_UPGRADE_STATUS.md: Status of all 9 PRs from Dependabot
- UPGRADE_RESEARCH.md: Detailed technical analysis per package
- UPGRADE_QUICK_REFERENCE.md: Quick status table and decisions
- UPGRADE_README.md: Navigation guide for upgrade docs
- UPGRADE_CODE_PATTERNS.md: Grep commands and code examples
- UPGRADE_COMMANDS.md: Step-by-step terminal commands
- RESEARCH_SUMMARY.txt: Executive summary

Verified:
✅ npm run build passes
✅ npm run lint passes
✅ No code changes required for these upgrades

Skipped unsafe upgrades:
❌ rimraf 5.0.10 → 6.1.2 (requires Node >=20, conflicts with >=18 policy)

Pending for future PRs (require testing):
⚠️ jest 29.7.0 → 30.2.0 (major version)
⚠️ jest-environment-jsdom 29.7.0 → 30.2.0 (major version)
⚠️ @jest/globals 29.7.0 → 30.2.0 (major version)
⚠️ size-limit 11.2.0 → 12.0.0 (major version)
⚠️ @size-limit/preset-small-lib 11.2.0 → 12.0.0 (major version)

* chore: upgrade Node.js minimum to >=20 and upgrade rimraf to 6.1.2

Upgrade Node.js engine requirement:
- Old: >=18
- New: >=20

This enables support for the latest Node.js features and allows upgrading
dependencies that require >=20.

Upgrade rimraf:
- rimraf 5.0.10 → 6.1.2
- Now compatible with Node.js >=20 requirement
- No API changes (CLI only, used in build scripts)

Verified:
✅ npm run build passes
✅ npm run lint passes
✅ No code changes required

* docs: update dependency upgrade status after rimraf upgrade

* security: add comprehensive safety utilities and test suite

Add security utilities to prevent common web vulnerabilities:

1. regexSafety.ts - Prevent ReDoS (Regular Expression Denial of Service)
   - validateRegexInput(): Input length validation before regex operations
   - safeMatch/safeExec/safeReplace/safeReplaceAll/safeSplit/safeSearch/safeTest()
   - Prevents catastrophic backtracking attacks on polynomial regex patterns
   - Max input: 100KB (prevents abuse while allowing legitimate use)

2. objectSafety.ts - Prevent prototype pollution attacks
   - isSafeKey(): Reject dangerous keys (__proto__, constructor, prototype)
   - createSafeObject(): Create null-prototype objects
   - safeAssign/safeAssignBulk/safeMerge(): Safe property assignment with validation
   - Blocks prototype chain attacks from untrusted input

3. urlSafety.ts - Prevent URL spoofing and domain attacks
   - extractDomain(): Safe URL parsing with normalization
   - isDomainMatch(): Exact domain matching (not substring matching)
   - detectSourceType(): Safe platform detection
   - isApprovedDomain/isSafeDomain(): Whitelist and blacklist validation
   - Prevents subdomain spoofing (twitter.com.evil.com) and homograph attacks

4. logSafety.ts - Prevent log injection attacks
   - sanitizeForLog(): Remove control chars, ANSI codes, newlines
   - createLogEntry/safeLog/sanitizeJsonForLog(): Structured logging
   - Prevents attackers from injecting fake log entries

Security Test Suite (66 tests):
✓ ReDoS prevention: Input validation, regex wrappers
✓ Prototype pollution: Safe keys, safe objects, safe assignment
✓ URL sanitization: Domain extraction, subdomain attacks, spoofing
✓ Log injection: Newline/tab/ANSI removal, injection attempts

Verified:
✅ npm run build passes
✅ npm run test:jest -- src/__tests__/security.test.ts: 66/66 passing
✅ npm run lint passes

All utilities are:
- Backwards-compatible (new exports only)
- Well-documented with examples
- Production-ready with comprehensive test coverage
- No performance impact on legitimate use

* docs: add implementation summary for security utilities

* chore: remove upgrade documentation files (move to PR descriptions)

* chore: remove implementation summary (keep codebase clean)

* Potential fix for pull request finding 'Unused variable, import, function or class'

* fix: address all code review feedback from PR #237

Critical fixes:
- Export all security utilities from src/index.ts for public API access
- Fix isDomainMatch() to correctly handle multi-part TLDs (co.uk, co.nz, etc.)
- Fix depth tracking bug in sanitizeJsonForLog() using recursive depth tracking
- Make console.warn() configurable in objectSafety.ts via setObjectSafetyWarning()

Improvements:
- Export MAX_REGEX_INPUT_LENGTH constant for external use
- Replace TypeScript `as any` with proper type annotations
- Remove unused COMPLEX_REGEX constant from test suite
- Fix control character issue in regex patterns using RegExp constructor
- Replace any type with unknown in generic defaults

Testing:
- All 66 security tests pass
- No linting errors (3 warnings in unrelated files)
- All security utilities properly exported and tested

* fix: resolve TypeScript build and type errors

- Fix stringify function return type to always return string (undefined → 'null')
- Simplify safeReplace and safeReplaceAll type casting using any for DOM API compatibility
- All 66 security tests still passing
- Build now succeeds with no TypeScript errors
- Linter warnings reduced to acceptable levels (control chars intentional, any necessary)

* fix: integrate security utilities and address code review feedback

This commit addresses all proactive security improvements:

MUST FIX:
- Fix JSON.stringify(undefined) bug in logSafety.ts
  - Objects now correctly omit undefined values (matching native behavior)
  - Arrays still convert undefined to null (as per JSON spec)

- Integrate security utilities into vulnerable code
  - SourcesListComponent.utils.tsx: Replace 30+ vulnerable .includes()
    domain checks with safe isDomainMatch() to prevent subdomain spoofing
  - normalizeCitation.ts: Use createSafeObject() and isSafeKey() to
    prevent prototype pollution in citation attribute parsing

SHOULD FIX:
- Simplify root domain extraction in urlSafety.ts
  - Clearer TLD extraction logic (slice(-2) instead of complex indexOf)

- Optimize detectSourceType() performance
  - Parse URL once instead of 30+ times (extract domain upfront)
  - Create matcher closure to avoid repeated URL parsing

- Document UTF-16 code unit limitation in regexSafety.ts
  - Explain difference between code units, bytes, and Unicode code points
  - Clarify that emoji/rare chars use 2 code units (surrogate pairs)

- Improve ANSI escape sequence removal in logSafety.ts
  - Comprehensive pattern now catches all common ANSI sequences:
    color codes, OSC sequences, charset selection, private modes
  - Replaced incomplete pattern that only caught sequences with non-word chars

All changes are backwards-compatible with no breaking API changes.

* fix: address lint warnings and code review feedback

This commit fixes all remaining lint errors and addresses code review
feedback from Claude code reviews on PR #237:

**Lint Fixes:**
- Remove unused `showCopyButton` parameter from StatusHeader component
  - Removed from both function signature and StatusHeaderProps interface
  - Parameter was defined but never used in the function body

- Add biome-ignore directives for intentional control characters
  - logSafety.ts: ANSI escape sequence regex intentionally uses \x1b, \x07
  - These hex escapes are required to match terminal control codes

- Add biome-ignore directives for necessary 'any' type usage
  - regexSafety.ts: Replacer function signatures must match built-in
    String.replace() and String.replaceAll() overloads
  - Type casts are required due to TypeScript's complex union handling

**Impact:**
- All lint checks now pass cleanly (bun run lint ✓)
- All tests still pass (1170 tests ✓)
- Build succeeds without errors
- No breaking changes to public API

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* fix: address critical code review findings

This commit addresses all 8 critical and high-priority issues identified
in the comprehensive Claude code review:

**Critical Fixes:**

1. Error Message Accuracy (regexSafety.ts:45)
   - Changed "bytes" → "characters" in error message
   - JavaScript string.length measures UTF-16 code units, not bytes
   - More accurate error reporting for ReDoS prevention

2. Multi-Part TLD Handling (urlSafety.ts:207-214, 245-252)
   - Fixed isApprovedDomain() and isSafeDomain() to use extractRootDomain()
   - Previously duplicated logic with parts.slice(-2) broke for .co.uk domains
   - Now correctly handles bbc.co.uk, com.au, co.kr, etc.

3. Expanded Multi-Part TLD List (urlSafety.ts:41)
   - Added co.kr (South Korea)
   - Added com.cn (China)
   - Added com.sg (Singapore)
   - Added net.au, edu.au (Australia)
   - Added ac.jp (Japan academic)
   - Total: 23 multi-part TLDs now supported

**High-Priority Fixes:**

4. Circular Reference Bug (logSafety.ts:28-46)
   - Fixed JSON.stringify() throwing on circular refs
   - Now catches error and falls back to String(value)
   - Prevents runtime crashes on complex object graphs

5. Log Truncation Indicator (logSafety.ts:44-46)
   - Added "... [TRUNCATED]" suffix when logs are truncated
   - Prevents silent payload hiding in security contexts
   - Updated test expectations (100 → 115 chars with suffix)

6. Regex State Bug (regexSafety.ts:92)
   - Reset regex.lastIndex = 0 in safeExec()
   - Prevents stateful behavior with global regex reuse
   - Ensures consistent execution across multiple calls

7. Domain Validation (urlSafety.ts:120-123)
   - Added empty domain parameter check in isDomainMatch()
   - Returns false immediately if domain is empty string
   - Prevents edge case where invalid URL + empty domain = true

8. Expanded Dangerous Keys (objectSafety.ts:15-21)
   - Added toString (breaks string coercion)
   - Added valueOf (breaks primitive coercion)
   - Added hasOwnProperty (used in property checks)
   - Added isPrototypeOf (prototype chain manipulation)
   - Total: 7 dangerous keys now blocked

**Testing:**
- All 1170 tests pass ✓
- Updated security test for truncation indicator
- Build succeeds without errors
- Lint passes cleanly

**Impact:**
- No breaking changes to public API
- Enhanced security posture across all utility modules
- Better error messages for debugging
- More robust edge case handling

* fix: address remaining CodeQL security alerts

This commit fixes the remaining critical security issues flagged by CodeQL:

**Prototype Pollution Fixes:**

1. parseCitation.ts:698 (Alert #33)
   - Fixed groupCitationsByAttachmentIdObject() prototype pollution
   - Changed from Record<string, unknown> to createSafeObject()
   - Added isSafeKey() validation before assigning citation keys
   - Prevents "__proto__" and other dangerous keys from polluting prototype

2. citationParser.ts:85 (Alert #46)
   - Fixed expandCompactKeys() remote property injection
   - Changed from Record<string, unknown> to createSafeObject()
   - Added isSafeKey() check before assigning fullKey values
   - Prevents malicious keys in user-provided citation data

**URL Sanitization Fixes:**

3. SourcesListComponent.utils.tsx (Alerts #26-27)
   - Fixed remaining .includes() substring matching vulnerabilities
   - Changed merriam-webster.com to use isDomainMatch()
   - Changed dictionary.com to use isDomainMatch()
   - Prevents domain spoofing attacks (e.g., dictionary.com.evil.com)

4. Commerce Domain Detection (Alerts #20-30 partial)
   - Improved amazon/ebay domain detection for regional TLDs
   - Now uses extractDomain() + startsWith() for proper validation
   - Prevents false positives from amazon.evil.com
   - Handles legitimate regional domains (amazon.co.uk, ebay.com.au)

**Security Impact:**
- 3 critical prototype pollution vulnerabilities fixed
- 2 URL spoofing vulnerabilities fixed
- Reduces CodeQL alert count by 5 issues
- All security fixes use our safe utility wrappers

**Testing:**
- All 1170 tests pass ✓
- Build succeeds without errors
- Lint passes cleanly
- No breaking changes to public API

* refactor: focus DANGEROUS_KEYS on core prototype pollution vectors

This commit refines the security implementation based on code review feedback.

**Change:**
Reduced DANGEROUS_KEYS from 7 to 3 keys, focusing on core prototype pollution vectors:
- __proto__: Direct access to object's internal prototype
- constructor: Indirect access to constructor.prototype
- prototype: Modifies constructor's prototype for all instances

**Rationale:**
The additional keys (toString, valueOf, hasOwnProperty, isPrototypeOf) are primarily
for method hijacking, not prototype pollution. Including them over-blocks legitimate
use cases. Applications requiring defense against method hijacking can build on top
of this foundation using similar patterns.

**Documentation:**
Added detailed comments explaining:
- What each dangerous key does
- Why we focus on core vectors
- How to extend for additional defense

**Impact:**
- More focused security posture (protects against actual pollution)
- Reduces false positives and legitimate use case blocking
- All 1170 tests pass
- Build succeeds without errors
- No breaking changes to API

This maintains security while being pragmatic about what constitutes a "dangerous key"
in the context of prototype pollution specifically.

* docs: add security utilities migration guide

Add comprehensive documentation for integrating security utilities into the codebase.

This guide covers:

1. **ReDoS Prevention (regexSafety)**
   - When to use safe regex wrappers
   - Files that need migration (11 locations)
   - Examples and patterns

2. **Prototype Pollution Prevention (objectSafety)**
   - How to use createSafeObject() and isSafeKey()
   - Already fixed locations in parsing layer
   - Attack vectors explained

3. **URL Domain Verification (urlSafety)**
   - Domain matching best practices
   - Difference between exact and substring matching
   - Already fixed locations
   - Multi-part TLD support

4. **Log Injection Prevention (logSafety)**
   - Safe logging patterns
   - Configuration options
   - Circular reference handling

5. **Migration Phases**
   - Phase 1: Critical security (current - done)
   - Phase 2: Comprehensive coverage (future PR)
   - Phase 3: Advanced defense (ESLint, branded types)

6. **Testing and Verification**
   - How to test security utilities
   - Verification patterns for each defense

7. **Security Limits**
   - Rationale for default limits
   - How to customize if needed
   - Performance considerations

This document serves as:
- Implementation guide for developers
- Security best practices reference
- Migration checklist for future work

* style: format DANGEROUS_KEYS for consistency

Apply biome formatting to DANGEROUS_KEYS constant to maintain consistent code style across the security utilities module.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@bensonwong