Skip to content

security fixes#351

Merged
nahimterrazas merged 3 commits into
mainfrom
deepsec-security-fixes
May 18, 2026
Merged

security fixes#351
nahimterrazas merged 3 commits into
mainfrom
deepsec-security-fixes

Conversation

@nahimterrazas
Copy link
Copy Markdown
Collaborator

@nahimterrazas nahimterrazas commented May 16, 2026

Summary

Testing Process

Checklist

  • Add a reference to related issues in the PR description.
  • Add unit tests if applicable.

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced scope validation during client registration with explicit allowlists and clearer error messages.
    • Improved validation of configuration parameters to prevent out-of-range values.
  • Documentation

    • Added security guidelines for off-chain discovery endpoint configuration and access control.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 16, 2026

📝 Walkthrough

Walkthrough

This pull request hardens the solver-service against two categories of security vulnerabilities: scope privilege escalation during public registration and potential discovery endpoint bypass of signature validation. Code changes add explicit input validation and parameter bounds checking; documentation pages guide operators on secure configuration and explain the attack surface.

Changes

Security Hardening and Validation

Layer / File(s) Summary
Public registration scope allowlist validation
crates/solver-service/src/apis/auth.rs
parse_public_scopes replaces a single contains(AdminAll) check with an allowlist-based validation loop, rejecting each disallowed scope with a specific error message. Tests verify admin-read is rejected and /auth/register returns 400 BAD_REQUEST with the expected error text.
Rebalance parameter bounds checking
crates/solver-service/src/apis/rebalance.rs
handle_update_rebalance_config uses checked u32::try_from to validate max_pending_transfers against overflow. handle_update_rebalance_threshold adds checked conversion and business constraint validation for deviation_band_bps (must be ≤ 10,000), producing distinct error messages for each failure mode. Tests cover overflow and range-violation scenarios.
Public registration admin-read vulnerability documentation
docs/public-registration-admin-read.html
New HTML documentation page explaining the admin-read scope escalation attack, with threat model, step-by-step exploitation flow, malicious request example, current vs. safe scope policy comparison, recommended mitigations (positive allowlist, separated admin issuance, tests), pseudocode solution, operational safeguards, and code reference pointers.
Discovery endpoint security guidance
docs/direct-discovery-bypass.html, docs/offchain-discovery-security.md
Two-server request flow documentation clarifying that /intent is an internal endpoint that must remain on private networks (localhost or mTLS), detailing the bypass attack surface when exposed, and recommending defense-in-depth via network policy and redundant ResourceLock signature validation in the discovery handler.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

  • shahnami
  • pepebndc
  • tirumerla

Poem

🐰 A rabbit guards the gates with care,
Allowlists checked with thorough flair,
No overflow shall pass this way,
Discovery paths kept tucked away,
Secure by bounds and lists so rare! 🔐

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description contains only the template structure with empty sections and unchecked checklist items, providing no actual content about the changes. Fill in the Summary section with details about the security fixes, document the testing approach, and complete the checklist items including issue references and confirmation of added unit tests.
Title check ❓ Inconclusive The title 'security fixes' is vague and generic, lacking specific details about which security vulnerabilities are being addressed or their nature. Use a more descriptive title that specifies the security issues being fixed, such as 'Fix scope validation and range checks in auth and rebalance endpoints'.
✅ Passed checks (3 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 92.86% which is sufficient. The required threshold is 80.00%.
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.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch deepsec-security-fixes

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/public-registration-admin-read.html`:
- Around line 322-325: Update the vulnerable-document wording to make clear this
describes pre-fix behavior: change present-tense phrases that say the parser
“still accepts” or “only rejects” to explicit pre-fix language such as “before
the fix, the parser accepted ‘admin-read’ but rejected ‘admin-all’” (referencing
the heading "Public Registration Can Mint Admin-Read JWTs" and the tokens
`admin-read` and `admin-all`), and append a short note like “Fixed in PR `#351`”
to the three affected sections (the paragraphs currently using present tense
around those tokens) so readers know the issue is resolved.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0ddb45f3-c7be-4e9f-9443-5733b097618c

📥 Commits

Reviewing files that changed from the base of the PR and between b7057e2 and a418dd3.

📒 Files selected for processing (5)
  • crates/solver-service/src/apis/auth.rs
  • crates/solver-service/src/apis/rebalance.rs
  • docs/direct-discovery-bypass.html
  • docs/offchain-discovery-security.md
  • docs/public-registration-admin-read.html

Comment thread docs/public-registration-admin-read.html Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented May 16, 2026

Codecov Report

❌ Patch coverage is 96.85039% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/solver-service/src/apis/rebalance.rs 95.8% 4 Missing ⚠️

📢 Thoughts on this report? Let us know!

@nahimterrazas nahimterrazas merged commit a3a7b15 into main May 18, 2026
18 checks passed
@nahimterrazas nahimterrazas deleted the deepsec-security-fixes branch May 18, 2026 20:09
nahimterrazas added a commit that referenced this pull request May 19, 2026
security fixes (#351)
nahimterrazas added a commit that referenced this pull request May 19, 2026
security fixes (#351)
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