Skip to content

[DETAIL] DGS field transformation can return numbers, crashing UI components that expect strings #1954

@adriangohjw

Description

@adriangohjw

Note

From: https://app.detail.dev/oss/opengovsg/isomer/bug_f8d4b734-d643-47e8-8deb-a9debbe57e66
I have not validated this but it seems legit. Can test with mock response using Storybook to verify.

Summary

  • Context: transformDgsField transforms DGS field references (e.g., [dgs:column_name]) by extracting values from DGS API records for use in UI components.
  • Bug: The function returns number values when DGS records contain numeric columns, but its type signature promises T extends string | undefined | null.
  • Actual vs. expected: Returns number (e.g., 2023) when the DGS column is numeric, but TypeScript expects string | undefined | null.
  • Impact: Runtime crash with TypeError: content.trim is not a function when numeric values flow to BaseParagraph which calls .trim() on line 43.

Code with Bug

// packages/components/src/hooks/useDgsData/transformDgsField.ts
export const transformDgsField = <T extends string | undefined | null>(
  field: T,
  record: Record<string, unknown>,
): T => {
  try {
    if (!field || !isStringDgs(field)) {
      return field
    }
    return record[extractDgsFieldKey(field)] as T  // <-- BUG 🔴 casts unknown as T without validation
  } catch {
    return field
  }
}

Explanation

  • DGS records intentionally support Record<string, string | number>, so record[extractDgsFieldKey(field)] can legitimately be a number.
  • transformDgsField uses as T to force the return type to string | undefined | null without validating/coercing the runtime value.
  • Call sites (e.g., DgsContactInformation) pass title/description through unvalidated, and downstream BaseParagraph calls content.trim(), which throws if content is a number.

Codebase Inconsistency

// packages/components/src/hooks/useDgsData/safeJsonParse.ts
// For other types (number, boolean, etc.), return undefined
return undefined

sageJsonParse explicitly treats number as invalid and returns undefined, but transformDgsField provides no equivalent protection for string fields like title/description.

Recommended Fix

export const transformDgsField = <T extends string | undefined | null>(
  field: T,
  record: Record<string, unknown>,
): T => {
  try {
    if (!field || !isStringDgs(field)) {
      return field
    }
    const value = record[extractDgsFieldKey(field)]
    if (value === undefined || value === null) {
      return value as T
    }
    // Convert to string to honor type contract
    return String(value) as T
  } catch {
    return field
  }
}

History

This bug was introduced in commit af4bf87. The function was created to transform DGS field references like [dgs:column_name] into their actual values from DGS records. The bug slipped in because the developer used a type assertion (as T) to cast the returned value, trusting that DGS column values would always be strings—however, the DGS API intentionally returns both string and number types, and no type checking or conversion was performed before returning the value.

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type
No fields configured for issues without a type.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions