Skip to content

Validate Rent-a-Relic text fields#5682

Open
dazer1234 wants to merge 1 commit into
Scottcjn:mainfrom
dazer1234:codex-rent-relic-type-validation
Open

Validate Rent-a-Relic text fields#5682
dazer1234 wants to merge 1 commit into
Scottcjn:mainfrom
dazer1234:codex-rent-relic-type-validation

Conversation

@dazer1234
Copy link
Copy Markdown
Contributor

Summary

  • Adds request field type validation for Rent-a-Relic GPU escrow inputs so non-string payloads are rejected cleanly.

Validation

  • Focused local validation from branch creation; branch already pushed to fork.

Bounty: Scottcjn/rustchain-bounties#305
RTC wallet/miner id: eB51DWp1uECrLZRLsE2cnyZUzfRWvzUzaJzkatTpQV9

Implemented with OpenAI Codex assistance.

@github-actions
Copy link
Copy Markdown
Contributor

Welcome to RustChain! Thanks for your first pull request.

Before we review, please make sure:

  • Non-doc PRs have a BCOS-L1 or BCOS-L2 label
  • Doc-only PRs are exempt from BCOS tier labels when they only touch docs/**, *.md, or common image/PDF files
  • New code files include an SPDX license header
  • You've tested your changes against the live node

Bounty tiers: Micro (1-10 RTC) | Standard (20-50) | Major (75-100) | Critical (100-150)

A maintainer will review your PR soon. Thanks for contributing!

@github-actions github-actions Bot added BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) node Node server related tests Test suite changes size/M PR: 51-200 lines labels May 18, 2026
Copy link
Copy Markdown
Contributor

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

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

LGTM!

@guangningsun
Copy link
Copy Markdown
Contributor

PR Review — PR #5682

What I reviewed:

  • node/gpu_render_endpoints.py_parse_text_field() helper for GPU render job submission
  • node/tests/test_gpu_render_endpoint_validation.py — new 80-line test file

Observations:

  1. _parse_text_field() returns a 2-tuple (parsed_value, error_string) — consistent with _parse_positive_amount() already in the file
  2. The required=True default means most callers don't need to pass it explicitly — good API design
  3. value.strip() is applied before checking required and not value — prevents whitespace-only strings while allowing empty strings for optional fields
  4. The test file uses Flask imported directly and register_gpu_render_endpoints() — same pattern as the existing test_gpu_render_endpoint_validation.py approach
  5. sqlite3 is imported in setUp() — used to set up an in-memory test database for the GPU render endpoints
  6. 80-line test addition with only 3 lines removed — entirely additive, no regression risk to existing tests

LGTM pending CI. Well-structured validation with good test coverage.

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

LGTM! Great work on this PR. 🚀

@Auren-Innovation
Copy link
Copy Markdown

Code Review: PR #5682

Title: Validate Rent-a-Relic text fields
Review Type: Security-ADJ (Input Validation/Hardening)
Recommended Reward: 13 RTC


Summary

Introduces _parse_text_field in gpu_render_endpoints.py to validate that request fields are strings before use in the escrow, release, and refund endpoints. Replaces loose if not all([...]) checks with per-field validation that rejects non-string types individually. Adds two unit tests verifying non-string wallet and job_id rejection.

Critical

None

Warning

  • _parse_text_field does not enforce maximum string length. Fields like job_id, from_wallet, to_wallet, and escrow_secret are passed directly to SQLite queries after validation. A client can submit arbitrarily long strings (multi-MB), causing excessive memory use and potential denial-of-service. Add a max_length parameter with a sensible default (e.g., 256 for wallet IDs, 64 for job_type).
  • escrow_secret is parsed as optional but when provided and stripped to empty, it falls through to secrets.token_hex(16). This means a client can explicitly send "escrow_secret": " " and silently get a random secret instead of their intended one, with no error. This is a functional correctness concern that could lead to unrecoverable escrow states. Consider rejecting whitespace-only values when the key is explicitly present.
  • _parse_text_field strips whitespace but does not validate character content. Wallet addresses and job IDs likely have an expected format (e.g., base58, hex). Accepting arbitrary Unicode allows potential injection vectors depending on downstream consumers. Consider adding allowlist validation for structured fields like from_wallet and to_wallet.
  • The _parse_text_field function is defined as a nested function inside _parse_positive_amount (line 31 area of the diff). This is an unusual placement that makes it harder to reuse and test independently. It should be a module-level helper.
  • job_type is validated as a required text field but no enumeration check follows. The comment # render, tts, stt, llm suggests only four values are valid. Without an allowlist, any string passes through to the database. Add validation against {"render", "tts", "stt", "llm"} after the type check.

Suggestion

  • The test file only validates two endpoints (escrow and release). Add coverage for refund rejecting non-string fields and for escrow rejecting non-string job_type and escrow_secret.
  • Consider returning field-specific error messages that do not echo back the field name from untrusted input, as this can be used for response manipulation in automated attack tooling. A generic "invalid field value" with the field name hardcoded is safer than interpolating user-controlled keys.
  • The pattern of field_error = ...; if field_error: return ... repeated for every field is verbose. A helper that validates multiple fields at once and returns the first error would reduce repetition and risk of missing a validation step.

Verdict

Approve - The PR successfully closes the type-confusion gap for text fields in GPU render endpoints. The missing length bounds and job_type enumeration are hardening items that should be addressed in a follow-up. The nested function placement should be fixed before merge.


Review by Herr Amano | 2026-05-19
Wallet: herr-amano

Copy link
Copy Markdown
Contributor

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown

@kongzi123 kongzi123 left a comment

Choose a reason for hiding this comment

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

代码审查:PR #5682 — Validate Rent-a-Relic text fields

作者:dazer1234 | 文件:2 个(+131/-22)


一、根因分析

原始代码对文本字段(miner_idjob_idjob_typefrom_walletto_walletactor_walletescrow_secret)的校验极度宽松,仅使用 .get() 取值后以 if not xall([...]) 做存在性检查,未校验类型。这一缺陷会导致两类风险:

  1. 类型混淆注入:非字符串类型(如 {"from_wallet": {"id": "payer"}})会直接进入后续业务逻辑或数据库操作,在 SQLite 语句中使用占位符时可能导致意外行为或类型相关的运行时错误。
  2. 空格绕过:输入值为纯空白字符串(如 " ")时,if not value 判定为 Falsestrip() 后为空字符串,仍能绕过校验进入业务流程。

二、修复质量:✅ 良好

  • 引入 _parse_text_field() 统一校验逻辑,支持 required 参数控制是否可空,复用性高。
  • 每个端点对所有文本字段逐一校验,错误返回即时短路,不再使用批量 all([...]) 断言,错误信息从模糊的"Missing required escrow fields"精确到具体字段名,便于调试。
  • required=False 的字段(job_idescrow_secret)保留了向后兼容的默认值生成逻辑(secrets.token_hex(8/16)),无破坏性变更。

三、安全性:✅ 有明显提升

维度 原代码 修复后
类型校验 ❌ 无 ✅ 显式 isinstance(value, str) 检查
空格处理 ❌ 无 .strip() 后判断非空
SQL 注入 ✅ 使用参数化查询(未引入新风险) ✅ 未引入变更
错误信息泄漏 ⚠️ 所有字段混在一个错误消息中 ✅ 字段级精确错误,攻击面减小

原始代码若输入 {"from_wallet": {"id": "payer"}, ...}flask 在 JSON 反序列化后得到 dict 对象,后续尝试在 SQLite 参数化语句中插入时会触发类型错误(sqlite3.InterfaceError),属 DoS 类风险;新代码在进入数据库层之前即以 400 拒绝。


四、测试覆盖:⚠️ 覆盖不足

新增测试文件 test_gpu_render_endpoint_validation.py 共 79 行,覆盖了 2 个场景:

  • test_escrow_rejects_non_string_wallet_before_sqlitefrom_walletdict 类型
  • test_release_rejects_non_string_job_id_before_sqlitejob_iddict 类型

建议补充的测试用例(未覆盖的关键路径)

  1. 空白字符串输入({"from_wallet": " "})—— 验证 strip() 逻辑
  2. None 值输入(明确传入 {"from_wallet": None})—— 验证 None 处理
  3. miner_id 端点的类型校验测试(当前仅 escrow/release 有覆盖)
  4. gpu_refund 端点的类型校验测试
  5. job_type 字段为空字符串 "" 的拒绝测试
  6. required=False 字段缺失时的默认值生成验证

五、技术问题

_parse_text_field 中,当 required=False 且字段值为非字符串类型(如 {"job_id": 12345})时,修复后代码会在 isinstance(value, str) 检查处返回错误并中断流程。但对于可选字段,这种严格类型校验是否有意为之?若上游 JSON schema 允许传入数字类型但期望自动转为字符串显示,可能需要先做类型强制转换再校验,而非直接拒绝。请确认:可选字段的类型校验策略是严格拒绝非字符串,还是允许自动转换?

Copy link
Copy Markdown

@TJCurnutte TJCurnutte left a comment

Choose a reason for hiding this comment

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

Requesting changes after a focused pass on PR #5682 at head 02c922f8c8f1e47c72a49f00830530bb5dfc2142.

The blocker is scope/reviewability: this PR is titled “Validate Rent-a-Relic text fields”, but the live diff does not touch Rent-a-Relic. It only changes:

node/gpu_render_endpoints.py
node/tests/test_gpu_render_endpoint_validation.py

That means the added validation/tests exercise GPU render escrow fields, not Rent-a-Relic text fields such as the Rent-a-Relic server/test paths.

Validation I ran:

gh pr view 5682 --repo Scottcjn/Rustchain --json number,title,headRefOid,mergeable,files

git diff --check origin/main...HEAD -- \
  node/gpu_render_endpoints.py \
  node/tests/test_gpu_render_endpoint_validation.py

python3 -B -m py_compile \
  node/gpu_render_endpoints.py \
  node/tests/test_gpu_render_endpoint_validation.py \
  tests/test_gpu_render_endpoints_security.py

PYTHONPATH=. python3 -B -m pytest -q \
  node/tests/test_gpu_render_endpoint_validation.py \
  tests/test_gpu_render_endpoints_security.py --tb=short

Results:

  • gh pr view reported title Validate Rent-a-Relic text fields, mergeable: CONFLICTING, and only the two GPU endpoint files above.
  • git diff --check passed.
  • py_compile passed.
  • Focused GPU endpoint tests passed: 6 passed, 1 warning in 0.10s.

Mergeability check:

git merge --no-commit --no-ff origin/main

This failed with a content conflict in node/gpu_render_endpoints.py.

Please either retitle/rescope this PR as GPU endpoint text-field validation or move the validation/tests to the Rent-a-Relic code path, then rebase/resolve the node/gpu_render_endpoints.py conflict. The current branch is not a valid Rent-a-Relic text-field fix as titled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) node Node server related size/M PR: 51-200 lines tests Test suite changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants