# Coldbox Reviewer

> Use this skill when reviewing ColdBox application code for correctness, security, performance, testability, and adherence to ColdBox conventions. Covers handlers, services, models, interceptors, modules, routing, ORM/OBM usage, REST APIs, dependency injection patterns, and common anti-patterns to flag during pull request reviews.

- Skill: `coldbox/coldbox-reviewer` (Agent Skill)
- Install (CLI): `npx skillmds@latest add coldbox/coldbox-reviewer`
- Raw SKILL.md: https://api.skillmd.com/api/skills/coldbox/coldbox-reviewer/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Security
- Author: ColdBox (https://skillmd.com/u/coldbox)
- Updated: 2026-09-17
- Page: https://skillmd.com/skills/coldbox/coldbox-reviewer

---


# ColdBox Code Reviewer

## When to Use This Skill

Use this skill when:
- Reviewing pull requests for ColdBox (BoxLang or CFML) applications
- Auditing an existing codebase for quality and security issues
- Establishing a code review checklist for a development team
- Identifying anti-patterns or risky patterns in ColdBox code

---

## Language Mode Reference

Examples use **BoxLang (`.bx`)** syntax by default. Adapt for your target language:

| Concept | BoxLang (`.bx`) | CFML (`.cfc`) |
|---------|-----------------|---------------|
| Class declaration | `class [extends="..."] {` | `component [extends="..."] {` |
| DI annotation | `@inject` above `property name="svc";` | `property name="svc" inject="svc";` |
| View templates | `.bxm` suffix | `.cfm` / `.cfml` suffix |
| Tag prefix | `<bx:if>`, `<bx:output>`, `<bx:set>` | `<cfif>`, `<cfoutput>`, `<cfset>` |

> **CFML Compat Mode**: With BoxLang + CFML Compat module, `.bx` and `.cfc` files coexist freely. BoxLang-native classes use `class {}` (`.bx` files); CFML-compat classes use `component {}` (`.cfc` files).

## Review Checklist Overview

| Category | Key Questions |
|----------|--------------|
| Architecture | Does the code follow the ColdBox layers correctly? |
| Handlers | Are actions thin? No business logic leaking in? |
| Services | Single responsibility? Proper error handling? |
| Security | Input validated? SQL injection / XSS / CSRF covered? |
| Dependency Injection | WireBox used correctly? No `createObject()` anti-patterns? |
| ORM / Queries | N+1 problem? Unsafe concatenation in HQL/SQL? |
| REST APIs | Consistent HTTP verbs, status codes, and response shape? |
| Performance | Unnecessary DB hits? Missing caching? |
| Testability | Logic in handler actions? Hard-coded dependencies? |
| Documentation | Public API documented? Deprecated APIs marked? |

---

## Architecture & Layering

### ✅ Correct Layering

```
Handler → Service → DAO / ORM Entity
          ↕ (optional)
       Interceptor / Event
```

- **Handlers** — route events, populate prc, delegate to services
- **Services** — all business logic, emit interception points
- **DAOs / Repositories** — only data access
- **Models / Entities** — data + domain behavior; no framework calls

### 🚩 Anti-Patterns to Flag

```js
// ❌ Business logic inside a handler action
function create( event, rc, prc ) {
    var user = new User()
    user.setEmail( rc.email )
    // hashing inside the handler — belongs in a service
    user.setPassword( hash( rc.password, "bcrypt" ) )
    entitySave( user )
    event.setView( "users/created" )
}

// ✅ Delegate to service
function create( event, rc, prc ) {
    prc.user = userService.create( rc )
    event.setView( "users/created" )
}
```

---

## Handler Review

### Checklist

- [ ] Action is thin — only coordinates between service calls and view setup
- [ ] `rc` inputs are validated before use (or validated inside the service)
- [ ] No raw SQL or ORM queries directly in handler
- [ ] `allowedMethods` is declared to restrict HTTP verbs
- [ ] `event.paramValue()` used instead of `isDefined("rc.x")` checks
- [ ] `relocate()` used after successful POST (PRG pattern)
- [ ] No `cfoutput` / direct response writes in a non-REST handler

### Common Handler Issues

```js
// ❌ Missing allowedMethods guard — allows DELETE via GET
function delete( event, rc, prc ) {
    userService.delete( rc.userId )
    relocate( "users.index" )
}

// ✅ Always declare allowedMethods
this.allowedMethods = { delete: "DELETE,POST" }

// ❌ Direct array access without param check
function show( event, rc, prc ) {
    prc.user = userService.find( rc.userId ) // rc.userId may not exist
}

// ✅ Use paramValue() to provide a safe default
function show( event, rc, prc ) {
    event.paramValue( "userId", 0 )
    prc.user = userService.find( rc.userId )
}
```

---

## Security Review

### Input Validation & Sanitization

```js
// ❌ Using rc directly in a query without validation
var q = queryExecute( "SELECT * FROM users WHERE email = '#rc.email#'" )

// ✅ Use query params (always)
var q = queryExecute(
    "SELECT * FROM users WHERE email = :email",
    { email: { value: rc.email, cfsqltype: "cf_sql_varchar" } }
)
```

### Cross-Site Scripting (XSS)

```html
<!-- ❌ Unescaped output in views -->
<p>#prc.user.getBio()#</p>

<!-- ✅ Always encode output -->
<p>#encodeForHTML( prc.user.getBio() )#</p>
```

### Mass Assignment

```js
// ❌ Populating an entity directly from rc — exposes all fields
getBeanPopulator().populateFromStruct( user, rc )

// ✅ Allowlist only expected keys
getBeanPopulator().populateFromStruct(
    user,
    rc,
    include = "firstName,lastName,email"
)
```

### CSRF

- Verify that mutating actions (POST/PUT/DELETE) check a CSRF token if the app uses session-based auth
- JWT-based REST APIs are generally exempt (token in header, not cookie)

### Authentication / Authorization Checks

```js
// ❌ No authorization check before operating on another user's data
function update( event, rc, prc ) {
    userService.update( rc.userId, rc )
}

// ✅ Verify the current user owns the resource
function update( event, rc, prc ) {
    if ( !authService.canEdit( rc.userId ) ) {
        return event.renderData( type: "json", data: { error: "Forbidden" }, statusCode: 403 )
    }
    userService.update( rc.userId, rc )
}
```

---

## Dependency Injection (WireBox)

### Checklist

- [ ] All dependencies injected via `property name="..." inject="..."` — no `createObject()`
- [ ] Singletons (`singleton` scope) hold no mutable request-level state
- [ ] Request/transient objects use `transient` scope or `wirebox.getInstance()`
- [ ] No `application.wirebox` access patterns in business code

### Common DI Issues

```js
// ❌ Manual instantiation defeats WireBox
class {
    function init() {
        variables.userService = createObject( "component", "models.UserService" ).init()
    }
}

// ✅ Let WireBox inject it
class {
    property name="userService" inject="UserService";
}

// ❌ Singleton service holds request-scoped state — causes data leakage between requests
class singleton {
    property name="currentUser"; // ← DANGEROUS in a singleton
}

// ✅ Store per-request state in prc, not in a singleton service
```

---

## ORM / Query Review

### N+1 Query Problem

```js
// ❌ N+1: one query to load orders, then one per order for user
for ( var order in orders ) {
    print( order.getUser().getEmail() )  // triggers a SELECT for each order
}

// ✅ Use HQL with join fetch
var orders = ORMExecuteQuery(
    "FROM Order o JOIN FETCH o.user WHERE o.status = :status",
    { status: "active" }
)
```

### Unsafe HQL / SQL

```js
// ❌ String concatenation in HQL — SQL injection risk
var results = ORMExecuteQuery( "FROM User WHERE email = '#arguments.email#'" )

// ✅ Named parameters
var results = ORMExecuteQuery(
    "FROM User WHERE email = :email",
    false,
    { email: arguments.email }
)
```

### Query of Queries

```js
// ❌ QoQ on a large query is slow; loop aggregation in CFML is faster
var result = new Query( sql: "SELECT * FROM myBigQuery WHERE status = 'active'", dbtype: "query", myBigQuery: myBigQuery ).execute().getResult()

// ✅ For aggregation, use the database query or array functions instead
```

---

## REST API Review

### Checklist

- [ ] Correct HTTP verbs used: GET (read), POST (create), PUT/PATCH (update), DELETE (delete)
- [ ] Correct HTTP status codes returned: 200, 201, 204, 400, 401, 403, 404, 422, 500
- [ ] Error responses follow a consistent JSON shape: `{ "error": "message" }`
- [ ] No sensitive data (passwords, tokens, PII) in response bodies
- [ ] Pagination info included in list endpoints: `{ data: [], meta: { page, total } }`
- [ ] `Content-Type: application/json` set on all JSON responses

### Common REST Issues

```js
// ❌ Wrong status code for creation
function create( event, rc, prc ) {
    prc.user = userService.create( rc )
    event.renderData( type: "json", data: prc.user, statusCode: 200 ) // should be 201
}

// ✅
event.renderData( type: "json", data: prc.user, statusCode: 201 )

// ❌ Swallowing errors and returning 200
function update( event, rc, prc ) {
    try {
        userService.update( rc )
        event.renderData( type: "json", data: { success: true } )
    } catch( any e ) {
        event.renderData( type: "json", data: { success: false } ) // ← 200 status even on failure
    }
}

// ✅ Return appropriate 4xx/5xx codes
} catch( ValidationException e ) {
    event.renderData( type: "json", data: { error: e.message }, statusCode: 422 )
} catch( any e ) {
    event.renderData( type: "json", data: { error: "Internal error" }, statusCode: 500 )
}
```

---

## Performance Review

### Checklist

- [ ] No per-request database calls for data that rarely changes (use CacheBox)
- [ ] No wildcard `SELECT *` on wide tables
- [ ] Large result sets use pagination, not full loads
- [ ] View rendering does not execute queries (data loaded in action, not view)
- [ ] No expensive operations inside loops

### Caching Anti-Pattern

```js
// ❌ Hits the DB on every request for config that never changes
function getConfig( event, rc, prc ) {
    prc.config = configService.loadAllSettings()
}

// ✅ Cache it
function getConfig( event, rc, prc ) {
    prc.config = cache( "appConfig", 60, function() {
        return configService.loadAllSettings()
    } )
}
```

---

## Testability Review

### Checklist

- [ ] Business logic is in services — not in handlers or views — so it can be unit-tested
- [ ] Services accept dependencies via injection (not hard-coded `createObject()`)
- [ ] Methods return values rather than writing directly to `prc` or `application`
- [ ] Side effects (email, SMS, external calls) are behind an interface so they can be mocked
- [ ] No global state required to test a method in isolation

### Hard-to-Test Pattern

```js
// ❌ Hard to test — direct DB call + global scope write
function processOrder( required numeric orderId ) {
    var order = entityLoad( "Order", orderId, true )
    application.stats.orderCount++
    sendEmail( to: order.getUser().getEmail(), subject: "Order confirmed" )
}

// ✅ Testable — dependencies injected, side effects behind interfaces
function processOrder( required numeric orderId ) {
    var order = orderDAO.find( orderId )
    statsService.incrementOrders()
    emailService.sendOrderConfirmation( order )
    return order
}
```

---

## Interceptor Review

### Checklist

- [ ] Interceptor has a single, clearly stated responsibility
- [ ] Does not perform heavy work on high-frequency points (`preProcess`, `postRender`)
- [ ] Terminates the request with `event.noExecution()` correctly when intercepting
- [ ] Does not swallow exceptions silently

```js
// ❌ Heavy DB query on every single request
void function preProcess( event, interceptData, buffer ) {
    prc.menuItems = menuService.loadAll() // hits DB on every request
}

// ✅ Cache it
void function preProcess( event, interceptData, buffer ) {
    prc.menuItems = cache( "menuItems", 10, function() {
        return menuService.loadAll()
    } )
}
```

---

## Module Review

### Checklist

- [ ] `box.json` has accurate `version`, `name`, and ColdBox version constraint
- [ ] `ModuleConfig.cfc` declares all settings with sensible defaults
- [ ] Routes are prefixed with the module namespace to avoid conflicts
- [ ] Interceptors are registered in `interceptors` array, not hard-coded
- [ ] Module cleans up after itself in `onUnload()`

---

## Documentation Review

- [ ] Every public method has a `/** ... */` comment with description + `@return`
- [ ] Handler actions document `@rc` inputs and `@prc` outputs
- [ ] Deprecated APIs are marked `@deprecated` with a migration path
- [ ] New public APIs have `@since` set to the current version
- [ ] Comments are accurate (not stale from a previous implementation)

---

## Quick Reference: Red Flags

| Code Pattern | Issue |
|-------------|-------|
| `"SELECT ... WHERE x = '#var#'"` | SQL injection |
| `#prc.userInput#` in a view with no encoding | XSS |
| `populateFromStruct(entity, rc)` with no allowlist | Mass assignment |
| `createObject("component", "some.path").init()` | Bypasses WireBox |
| `application.scope` mutations in a handler | Global state; race conditions |
| `cfquery` inside a `cfloop` | N+1 / chatty DB pattern |
| Missing `allowedMethods` on mutating actions | CSRF / verb tampering |
| `catch(any e) { /* empty */ }` | Swallowed exceptions |
| Singleton with mutable instance state | Request data leakage |
| Business logic in a view template | Untestable, violates MVC |