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:

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

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