Skip to content

fix: restrict OTC bridge CORS origins#5063

Closed
Asti1982 wants to merge 1 commit into
Scottcjn:mainfrom
Asti1982:nomad-fix-otc-cors-5054
Closed

fix: restrict OTC bridge CORS origins#5063
Asti1982 wants to merge 1 commit into
Scottcjn:mainfrom
Asti1982:nomad-fix-otc-cors-5054

Conversation

@Asti1982
Copy link
Copy Markdown
Contributor

Summary

Fixes #5054 by removing wildcard CORS from the OTC bridge.

What changed

  • Added restricted default OTC bridge CORS origins: https://bottube.ai and https://rustchain.org.
  • Added OTC_CORS_ORIGINS as a comma-separated allowlist for deployments that need additional trusted origins.
  • Ignored wildcard * entries and fell back to restricted defaults if the env value is empty or wildcard-only.
  • Added regression coverage proving the default is not wildcard and wildcard env entries are not accepted.

Why

CORS(app) allowed any website origin to read OTC bridge API responses. The bridge exposes order/trade APIs, so keeping cross-origin access to trusted frontends only reduces CSRF-style and data-exposure risk for browser users.

Validation

  • python -m pytest .\tests\test_otc_bridge_query_validation.py -q -> 7 passed
  • python -m pytest .\tests\test_otc_bridge_query_validation.py -q -k cors -> 2 passed, 5 deselected
  • python -m py_compile .\otc-bridge\otc_bridge.py .\tests\test_otc_bridge_query_validation.py -> passed
  • git diff --check -> passed
  • python .\tools\bcos_spdx_check.py --base-ref origin/main -> BCOS SPDX check: OK
  • Direct Flask-CORS header check in a temporary venv with otc-bridge/requirements.txt: allowed origin got Access-Control-Allow-Origin; unlisted origin got no CORS allow header.

Note: the older otc-bridge/test_otc_bridge.py suite still has a Windows tempfile/SQLite teardown issue in this environment (PermissionError deleting the shared temp DB), which then causes cascading rate-limit failures. That appears independent of this CORS change.

No production OTC bridge, live wallet, private key, token transfer, or destructive request was used.

Bounty #71 payout wallet if eligible: RTCda4841be5b2d109da5d995fb864c09676bb5b7c7

@github-actions
Copy link
Copy Markdown
Contributor

Welcome to RustChain! Thanks for your first pull request.

Before we review, please make sure:

  • Your PR has a BCOS-L1 or BCOS-L2 label
  • 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) tests Test suite changes size/S PR: 11-50 lines labels May 13, 2026
@Asti1982 Asti1982 force-pushed the nomad-fix-otc-cors-5054 branch from 965fde8 to 0ad68ee Compare May 13, 2026 09:43
@github-actions github-actions Bot added size/M PR: 51-200 lines and removed size/S PR: 11-50 lines labels May 13, 2026
@Asti1982
Copy link
Copy Markdown
Contributor Author

Follow-up: the first CI run exposed a test-harness collision, not a production failure. Another test module had left a flask_cors.CORS stub in sys.modules that did not accept keyword args. The OTC bridge query-validation loader now normalizes that stub to CORS(app, **kwargs) before importing the bridge.

Current head: 0ad68ee

Validation after the fix:

  • python -m pytest .\tests\test_otc_bridge_query_validation.py -q -> 7 passed
  • preloaded old flask_cors.CORS = lambda app: app stub + same pytest target -> 7 passed
  • python -m py_compile .\otc-bridge\otc_bridge.py .\tests\test_otc_bridge_query_validation.py -> passed
  • git diff --check -> passed
  • python .\tools\bcos_spdx_check.py --base-ref origin/main -> OK

GitHub checks are now green on the updated head.

Copy link
Copy Markdown
Contributor

@loganoe loganoe left a comment

Choose a reason for hiding this comment

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

Approved. The change replaces the blanket CORS(app) with a restricted default allowlist, supports OTC_CORS_ORIGINS, and deliberately drops * entries so an accidental wildcard does not reopen the bridge.

Validation performed:

  • git diff --check origin/main...HEAD passed
  • python3 -m py_compile otc-bridge/otc_bridge.py tests/test_otc_bridge_query_validation.py passed
  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 /tmp/review-5058-venv/bin/python -m pytest tests/test_otc_bridge_query_validation.py -q passed (7 passed)
  • Loaded the app with real Flask-CORS and confirmed https://bottube.ai / configured trusted origins receive Access-Control-Allow-Origin, while https://evil.example receives no CORS allow-origin header.

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.

Approved after a focused review of this PR's diff against origin/main.

Scope checked:

  • PR title: fix: restrict OTC bridge CORS origins
  • Changed files: otc-bridge/otc_bridge.py, tests/test_otc_bridge_query_validation.py
  • Review finding: Reviewed otc-bridge/otc_bridge.py, tests/test_otc_bridge_query_validation.py; diff adds regression test coverage.

Validation run locally from a clean checkout of pull/5063/head:

  • git diff --check origin/main...HEAD → exit 0 (no output)
  • python3 tools/bcos_spdx_check.py --base-ref origin/main → exit 0 (BCOS SPDX check: OK)
  • python3 -B -m py_compile otc-bridge/otc_bridge.py tests/test_otc_bridge_query_validation.py → exit 0 (no output)
  • python3 -B -m pytest tests/test_otc_bridge_query_validation.py -q → exit 0 (7 passed, 1 warning in 1.34s)
  • Added-line secret-pattern scan → pass

I did not find a merge-blocking issue in this focused pass. The change is small enough to review directly, and the targeted gates above passed.

Copy link
Copy Markdown
Contributor

@himanalot himanalot left a comment

Choose a reason for hiding this comment

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

Reviewed the OTC bridge CORS restriction.

The PR replaces the wildcard CORS(app) with a parsed allowlist, defaults to the two expected public origins, and filters out * entries from OTC_CORS_ORIGINS so wildcard-only or empty configuration falls back to restricted defaults. The tests cover the default and wildcard-env parsing behavior without depending on a real Flask-CORS installation.

I do not see a blocking issue in this PR.

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.

PR Review

Approved

  • Code is correct
  • No obvious issues
  • Good contribution

Thanks! 🙏

Reviewed by jaxint

@Scottcjn
Copy link
Copy Markdown
Owner

Closing per codex audit — superseded by #5061, which is the cleaner implementation. Bounty credit goes to the winning PR.

Reason: Real OTC CORS hardening, but galpetame #5061 is the cleaner strict-superset duplicate with broader tests.

@Scottcjn Scottcjn closed this May 15, 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! Thanks for contributing!

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) size/M PR: 51-200 lines tests Test suite changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: OTC bridge uses wildcard CORS — any website can call trading API

7 participants