coldbox-reviewer

name	coldbox-reviewer
description	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.
applyTo	**/*.{bx,bxm,cfc,cfm,cfml}

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

// ❌ 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

// ❌ 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

// ❌ 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)

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

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

Mass Assignment

// ❌ 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

// ❌ 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

// ❌ 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

// ❌ 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

// ❌ 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

// ❌ 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

// ❌ 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

// ❌ 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

// ❌ 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

// ❌ 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