Stop refusing _manifest.json, and test the rule instead of guessing it

[openipc-ai]

Found while verifying the host after the block volume was detached: the cron log was filling with

refusing "_manifest.json" from nightly-20260702-db82859: not a plain filename
refusing "_manifest.json" from nightly-20260628-220abe6: not a plain filename
...

_manifest.json is a real release asset — it has been in /srv/github-releases since 25 July. The filename guard added in #67 for the path-traversal finding spelled the rule as a character class, /\A[A-Za-z0-9][A-Za-z0-9._+-]*\z/, which requires the first character to be alphanumeric and so refused it from the very first run.

Guessing at the character set upstream is allowed to use was the mistake. The rule is structural now — a name is a plain filename when:

• basename leaves it unchanged, which rules out a/b, ../../etc/cron.d/evil, .. and .
• it does not start with a dot, which keeps an asset from colliding with .mirror-state.json or the size manifests

Nothing is assumed about which characters upstream may use, so the guard defends the actual threat without refusing files we meant to keep.

Verification

Table-tested against real asset names and every traversal case from the #67 review:

name	verdict
openipc.gk7605v100-nor-lite.tgz	accept
_manifest.json	accept
sizes.t31x-lite.json	accept
u-boot-t40xp-universal.bin	accept
../../etc/cron.d/evil	refuse
a/b	refuse
.. / .	refuse
.rootfs.sizes / .mirror-state.json / .tmp-mirror-x	refuse
(empty)	refuse

And on the host: 495 assets published rather than 494, _manifest.json fetched and tracked in the state file again, no refusing lines left in the log.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MFwmYHCbci2esgc8AMmzJR

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Accept underscore-prefixed release assets by using structural filename validation

🐞 Bug fix 🕐 10-20 Minutes

AI Description

• Replace restrictive asset-name regex with a structural “plain filename” check.
• Allow _manifest.json and other legitimate names while still blocking traversal and dotfiles.
• Keep mirror state/manifests protected from asset-name collisions.

Diagram

graph TD
  GH{{"GitHub Releases API"}} --> MR["mirror-releases.rb"] --> VF["plain_filename? guard"] --> FS["/srv/github-releases"]
  MR --> ST[".mirror-state.json"]

  VF -->|"refuse non-plain"| LG["cron log"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Loosen the existing regex allowlist (e.g., permit leading underscore)

• ➕ Minimal code churn and preserves current style
• ➕ Easy to reason about what characters are allowed
• ➖ Still guesses upstream naming rules and risks future false refusals
• ➖ Harder to ensure the regex captures the actual threat model (path semantics) rather than character aesthetics

2. Sanitize names (e.g., replace slashes / strip dots) instead of refusing

• ➕ Maximizes mirroring success even with “weird” upstream names
• ➕ Avoids operational churn from new asset naming patterns
• ➖ Can create ambiguity/collisions (two different names mapping to one sanitized name)
• ➖ Riskier from a security perspective because it transforms attacker-controlled input into filesystem paths

3. Use a stricter path containment check (join + realpath) instead of basename equality

• ➕ Very robust against traversal variants when combined with a fixed root directory check
• ➕ Can be extended to protect against symlink edge cases if needed
• ➖ More complex and can be trickier to implement correctly without TOCTOU issues
• ➖ Requires careful handling when the target path does not yet exist

Recommendation: Keep the PR’s structural validation approach. It directly enforces the security property needed (no path components / no dotfile collisions) without over-constraining legitimate upstream filenames, avoiding the class of false negatives that broke _manifest.json.

Files changed (1) +14 / -6

Bug fix (1) +14 / -6

mirror-releases.rbReplace SAFE_NAME regex with structural plain filename validation +14/-6

Replace SAFE_NAME regex with structural plain filename validation

• Removes the character-class-based SAFE_NAME guard and replaces it with a 'plain_filename?(name)' predicate based on 'File.basename' equality, non-empty names, and a no-leading-dot rule. Updates asset filtering to use the new predicate and expands comments to explain the threat model and the prior '_manifest.json' false refusal.

deploy/mirror-releases.rb

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Asset name log forging ✓ Resolved 🐞 Bug ⛨ Security

Description

After switching from a character allowlist to plain_filename?, asset names containing control
characters (e.g., newlines) are no longer rejected and can be interpolated into log lines unescaped,
enabling log forging and confusing monitoring/debugging output. This is activated by the PR because
plain_filename? only checks basename-unchanged and leading-dot, not printable/log-safe characters.

Code

deploy/mirror-releases.rb[R58-60]

+def plain_filename?(name)
+  !name.empty? && name == File.basename(name) && !name.start_with?('.')
+end

Evidence

plain_filename? no longer constrains characters beyond structural checks, so filenames with
embedded newlines/control characters can pass; multiple log lines interpolate the raw name without
.inspect, allowing injected line breaks to alter the log stream.

deploy/mirror-releases.rb[58-60]
deploy/mirror-releases.rb[172-173]
deploy/mirror-releases.rb[180-182]
deploy/mirror-releases.rb[241-243]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new `plain_filename?(name)` check accepts any characters other than slashes/dotfiles, so filenames with control characters (notably `\n`, `\r`, tabs) can now be mirrored. Several log lines interpolate `asset[:name]` / `a[:name]` without escaping, which allows log forging (multi-line injection) and can break log parsing.
### Issue Context
This PR intentionally stopped guessing an upstream character set and now only enforces structural safety. To keep that goal while preventing log injection, logging should escape/quote externally-sourced names (e.g., via `.inspect`), rather than reintroducing a restrictive allowlist.
### Fix Focus Areas
- deploy/mirror-releases.rb[58-60]
- deploy/mirror-releases.rb[172-173]
- deploy/mirror-releases.rb[180-182]
- deploy/mirror-releases.rb[241-243]
### Suggested changes
- Update log messages that include asset names to use `name.inspect` (or a small helper like `log_name(name) = name.to_s.inspect`) so control characters are escaped.
- Example: `log "  FAILED download #{asset[:name].inspect} (#{asset[:release]})"`
- Example: `log "  would fetch #{a[:name].inspect} (#{a[:size]} bytes, #{a[:release]})"`
- Keep the structural filename test as-is (to avoid regressing `_manifest.json`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can turn these tips off under Display preferences

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗