Skip to content

Treat targets_modified overlay as data, not code#3488

Merged
jmthomas merged 8 commits into
mainfrom
exec
Jul 2, 2026
Merged

Treat targets_modified overlay as data, not code#3488
jmthomas merged 8 commits into
mainfrom
exec

Conversation

@jmthomas

Copy link
Copy Markdown
Member

The user-writable targets_modified/ overlay was executed as code via three routes reachable by non-admin users: table definitions (ERB + GENERIC eval), cmd/tlm definitions (overlaid by setup_targets), and Script Runner suite analysis. Read table/cmd-tlm definitions from the read-only targets/ tree only, require admin to write the cmd_tlm overlay (rejecting non-canonical keys), and gate suite analysis at the script_run tier.

🤖 Generated with Claude Code

The user-writable targets_modified/ overlay was executed as code via three
routes reachable by non-admin users: table definitions (ERB + GENERIC eval),
cmd/tlm definitions (overlaid by setup_targets), and Script Runner suite
analysis. Read table/cmd-tlm definitions from the read-only targets/ tree
only, require admin to write the cmd_tlm overlay (rejecting non-canonical
keys), and gate suite analysis at the script_run tier.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@jmthomas jmthomas requested review from clayandgen and ryanmelt June 18, 2026 02:35
@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.33%. Comparing base (5b2f6ed) to head (ba31a0d).

Files with missing lines Patch % Lines
...r-api/app/controllers/running_script_controller.rb 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3488      +/-   ##
==========================================
+ Coverage   78.72%   79.33%   +0.61%     
==========================================
  Files         483      690     +207     
  Lines       35619    57508   +21889     
  Branches      728      728              
==========================================
+ Hits        28041    45624   +17583     
- Misses       7500    11806    +4306     
  Partials       78       78              
Flag Coverage Δ
python 80.42% <ø> (-15.58%) ⬇️
ruby-api 81.19% <91.11%> (+0.23%) ⬆️
ruby-backend 83.16% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread openc3-cosmos-script-runner-api/app/controllers/running_script_controller.rb Dismissed
Comment thread openc3-cosmos-cmd-tlm-api/app/controllers/storage_controller.rb Outdated
Comment thread openc3-cosmos-cmd-tlm-api/app/controllers/storage_controller.rb Outdated
Comment thread openc3-cosmos-script-runner-api/app/controllers/scripts_controller.rb Outdated
Comment thread openc3/lib/openc3/utilities/target_file.rb Outdated
Comment thread openc3/python/openc3/utilities/target_file.py Outdated
@jmthomas jmthomas requested a review from clayandgen June 18, 2026 15:47
jmthomas and others added 2 commits June 18, 2026 09:56
PR #3488 gated only the presigned-upload writer. Table.save/save_as/
generate/destroy reach the same targets_modified/<TARGET>/cmd_tlm/...
overlay via TargetFile.create, gated only by authorization('system'),
which under Enterprise RBAC every role down to Viewer holds. That overlay
is ERB-rendered (and eval'd via GENERIC_*_CONVERSION) as code at load, so
a non-admin could inject code. Add a shared authorize_overlay_write choke
point requiring admin for cmd_tlm paths, failing closed on non-canonical
names, mirroring storage_controller#non_admin_config_overlay_write?.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
clayandgen
clayandgen previously approved these changes Jun 23, 2026

@ryanmelt ryanmelt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This isn't the right fix. ERB rendering should only be in plugin install, and removed everywhere else.

jmthomas and others added 2 commits June 30, 2026 14:05
Config files are ERB-rendered once at plugin install (TargetModel#deploy)
and stored rendered in the targets bucket. Runtime parse paths
(PacketConfig, Target, TableConfig) now pass run_erb=false so the
user-writable targets_modified overlay is treated as data, never executed
as code. Back out the read-path original: gating in favor of this approach;
retain cmd_tlm overlay write gating, which also covers the separate
GenericConversion eval-at-decom vector.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

@jmthomas

Copy link
Copy Markdown
Member Author

This isn't the right fix. ERB rendering should only be in plugin install, and removed everywhere else.

Note that we still have to handle GenericConversion#call for GENERIC_*_CONVERSION blocks so we still need the existing logic to handle that. Removed the places where we render ERB at runtime and backed out some of the handling for tables.

@jmthomas jmthomas requested review from clayandgen and ryanmelt June 30, 2026 20:27
@jmthomas jmthomas merged commit 9a24ab8 into main Jul 2, 2026
37 checks passed
@jmthomas jmthomas deleted the exec branch July 2, 2026 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants