Skip to content

Refs #576: Reject control chars in login CSV config#628

Open
xingxi0614-cpu wants to merge 1 commit into
ramimbo:mainfrom
xingxi0614-cpu:codex/b576-config-login-controls
Open

Refs #576: Reject control chars in login CSV config#628
xingxi0614-cpu wants to merge 1 commit into
ramimbo:mainfrom
xingxi0614-cpu:codex/b576-config-login-controls

Conversation

@xingxi0614-cpu
Copy link
Copy Markdown

@xingxi0614-cpu xingxi0614-cpu commented May 29, 2026

Refs #576

Summary

Reject raw control characters in deploy GitHub-login CSV entries before trimming them into clean admin or accepted-labeler logins.

This keeps values such as these from being accepted as alice during deploy readiness checks:

  • MERGEWORK_ADMIN_LOGINS=<tab>alice
  • MERGEWORK_GITHUB_ACCEPTED_LABELERS=alice\x85

Normal clean entries and ordinary comma-separated login behavior remain unchanged.

Distinctness

This is limited to the admin/accepted-labeler GitHub-login CSV parsing path. It does not overlap the existing #576 slices for deploy secret/database/base URL C1 controls, webhook identities, wallet material, account/wallet filters, bounty filters, MCP arguments, JSON integers, MRWK amounts, or attempt source URLs.

Validation

  • focused CSV-login regression: 1 passed
  • tests/test_deploy_readiness.py: 28 passed
  • full test suite: 495 passed, 1 warning
  • ruff check on changed files: passed
  • ruff format-check on changed files: passed
  • mypy app/config.py: success
  • docs smoke: ok
  • git diff --check: clean
  • clean merge-tree against current origin/main and PR Refs #576: Reject C1 controls in deploy settings #623
  • submission quality gate: PASS

Safety

No private data, secrets, admin token values, wallet material, production mutation, price/liquidity/exchange/bridge/off-ramp claims, or fabricated payout claims are included.

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced validation of environment configuration values. The system now properly detects control characters in comma-separated configuration entries and applies appropriate handling rules to ensure stricter validation of login credentials and labeler settings.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 29, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: e5a14f62-126b-4bb9-aac4-5347cb684f67

📥 Commits

Reviewing files that changed from the base of the PR and between 0708f8a and d9189b3.

📒 Files selected for processing (2)
  • app/config.py
  • tests/test_deploy_readiness.py

📝 Walkthrough

Walkthrough

CSV environment variable parsing now detects control characters in individual items and adjusts how they are normalized before validation. When control characters are present, items are lowercased without trimming; otherwise items are trimmed and lowercased. A test validates that admin-login and labeler CSV entries with control characters are rejected by deploy readiness validation.

Changes

CSV Control Character Detection

Layer / File(s) Summary
Control character detection and CSV normalization
app/config.py, tests/test_deploy_readiness.py
_csv_env adds a _contains_control_character helper to detect control characters in each comma-separated item. Items with control characters are lowercased without trimming; items without control characters are trimmed and lowercased. A new test validates that admin-login and labeler CSV entries containing control characters are rejected with validation errors.
🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Title check ✅ Passed Title clearly names the changed surface and describes the main change: rejecting control characters in login CSV configuration.
Description check ✅ Passed Description includes all required sections: summary, evidence (distinctness and validation details), and test evidence checklist with results.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Mergework Public Artifact Hygiene ✅ Passed PR modifies only internal config code and tests; no public artifacts (README, docs, PR descriptions) changed. No investment, price, cash-out, or fabricated payout claims present.
Bounty Pr Focus ✅ Passed PR diff matches stated files, changes focused to login CSV parsing, test validates control character rejection, no unrelated scope.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@barnacleagent-svg barnacleagent-svg left a comment

Choose a reason for hiding this comment

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

PR Review — #628

PR: #628
Author: @xingxi0614-cpu
Head SHA: d9189b3
Branch: codex/b576-config-login-controls

Files Changed

  • (+7 lines): Added helper, applied to for admin/login CSV parsing
  • (+21 lines): Added test covering tab and � control chars in login env vars

CI Status

Quality, readiness, docs, and image checks: completed/success

Code Review

The change is straightforward and well-scoped:

  • Helper function correctly catches C0 (U+0000–U+001F) and C1 (U+007F–U+009F) control ranges
  • Applied specifically to GitHub-login CSV env vars where control chars would be invisible but corrupt logins
  • Test covers both and cases with proper deploy env setup
  • No side effects on normal logins

Verdict

APPROVE — Clean, focused, well-tested. Ready for merge.

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.

2 participants