Skip to content

Replace eval in CsvEntry#object_metadata with a non-executing parser #1196

Description

@laritakr

Summary

Bulkrax::CsvEntry#object_metadata calls eval on each persisted entry that arrives as a String when generating CSV exports. This is a server-side code-execution risk on the export path for ActiveFedora-backed resources whose object:-mapped fields contain attacker-controlled content.

Background

The legacy ActiveFedora persistence form for object:-mapped fields stores entries as the result of Hash#to_s — a string like "[{\"first_name\"=>\"Fake\"}]". To rehydrate the data on export, object_metadata calls eval(d) on each entry. The original author flagged this as a known concern in the comment at app/models/bulkrax/csv_entry.rb (around line 332-339):

Using eval is a very bad idea as it will execute the value of d within the full Ruby interpreter context.
TODO: Would it be possible to store this as a non-string? Maybe the actual Ruby Array and Hash?

#1193 narrowed the surface by skipping eval when the entry is already a Hash (the modern Valkyrie/JSONB shape), but the legacy String-input path was left in place for backward compatibility. The risk on the legacy path is unchanged.

Scope of the risk

The eval runs only during export, only via object_metadata, and only on entries that arrive as Strings — meaning only on records persisted via ActiveFedora-backed models with object:-mapped multi-value attributes. Adopters running Valkyrie/JSONB-backed resources are not affected (after #1193's Hash guard, those skip eval entirely).

For an attacker payload to reach eval, the attacker needs:

  1. A way to get a value into a field that maps via object: to a multi-value attribute on an AF-backed model. Typical paths: CSV import cell, form field submission.
  2. The persisted value must escape the string-quoting that wraps it in the Hash#to_s output. Whether this is achievable depends on how the AF setter sanitizes input — non-trivial but not proven impossible.
  3. Any subsequent export that includes the affected record runs eval on the payload, executing Ruby server-side.

The exploit difficulty is gated on step 2. The risk class is real even if a specific exploit hasn't been demonstrated.

Proposal

Replace eval with a non-executing parser for the legacy stringified form. Two reasonable approaches:

  1. Strict literal-only parser via RubyVM::AbstractSyntaxTree. Walk the AST; accept a whitelist of literal node types (HASH, ARRAY, STR, LIT, TRUE, FALSE, NIL, SYM); reject anything else. Reconstruct the value from the AST nodes without invoking Ruby execution.

  2. Migrate legacy persistence to JSON/YAML at write time. Resources currently persisting via Hash#to_s write JSON instead; reads of pre-migration entries fall back to a strict parser, but the long-term storage format is clean. More invasive than option 1 but durably eliminates the unsafe format.

Either way, when an entry doesn't conform, log a warning and skip it rather than aborting the export.

Acceptance criteria

  • object_metadata no longer calls eval.
  • Legacy stringified entries continue to round-trip through export when their values are simple literals (strings, numbers, symbols, booleans, nil, nested arrays/hashes of the same).
  • Entries containing Ruby method calls, constant references, interpolation, or any non-literal node are rejected and logged; the export continues with remaining valid entries.
  • Spec coverage for both happy-path legacy parsing and rejection of executable content.
  • Plain-Hash-input path (already in place from Add nested_attributes flag for object: mappings #1193) is unchanged.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions