Open
Conversation
…roxy Replace placeholder worker with a real Cloudflare Worker that: - Routes /mainnet/* and /regtest/* as API calls, everything else as static site - Validates X-Signature-256 HMAC-SHA256 signature before forwarding - Proxies authenticated API requests to the private ord backend (ORD_BACKEND_URL) - Returns JSON errors for missing config or invalid signatures - Add ORD_BACKEND_URL to wrangler.toml [vars] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…helpers - run_ord_command: exponential-backoff retry on transient errors (connection refused, timeout, etc.) - sanitize_error: strip RPC passwords from stderr before returning - write_temp_yaml: UUID-suffixed temp files to avoid race conditions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- make_base_command: build ord CLI prefix for mainnet/regtest - make_wallet_args: build wallet sub-command args per network - validate_fee_rate: range check (1-10000 sat/vbyte) - Fix mainnet RPC URL default port (8332, not 18443) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- validate_live_auth: constant-time password check via hmac.compare_digest (prevents timing side-channel attacks on the wallet password) - Refactor all 3 endpoints to use make_base_command, make_wallet_args, run_ord_command, sanitize_error, write_temp_yaml, validate_fee_rate - Temp YAML files are cleaned up after each request - Add requests to requirements.txt Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📊 Monthly LeaderboardHi @Neurvinch! Here's how you rank for March 2026:
Scoring this month (across OWASP-BLT org): Open PRs (+1 each), Merged PRs (+10), Closed (not merged) (−2), Reviews (+5; first two per PR in-month), Comments (+2, excludes CodeRabbit). Run |
owasp-blt
bot
added
unresolved-conversations: 0
needs-peer-review
|
👋 Hi @Neurvinch! This pull request needs a peer review before it can be merged. Please request a review from a team member who is not:
Once a valid peer review is submitted, this check will pass automatically. Thank you!
|
WalkthroughThe changes introduce robust retry logic with transient error detection for ord CLI commands, add signature-based authentication to the API gateway for backend proxying, implement validation helpers for inputs, and add backend URL configuration for the API gateway routing. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OrdAPI as Ord API Server
participant OrdCLI as Ord CLI
participant ErrorDetector
Client->>OrdAPI: POST /send-bacon-tokens (with payload)
OrdAPI->>OrdAPI: Validate inputs (fee_rate, auth)
OrdAPI->>OrdAPI: Write YAML to temp file
OrdAPI->>OrdCLI: Execute command (attempt 1)
OrdCLI-->>OrdAPI: stderr with transient error
OrdAPI->>ErrorDetector: Classify error
ErrorDetector-->>OrdAPI: Is transient? Yes
OrdAPI->>OrdAPI: Wait (exponential backoff)
OrdAPI->>OrdCLI: Execute command (attempt 2)
OrdCLI-->>OrdAPI: Success/Permanent error
OrdAPI->>OrdAPI: Sanitize error if needed
OrdAPI->>OrdAPI: Cleanup temp files
OrdAPI-->>Client: JSON response
sequenceDiagram
participant Client
participant Gateway as API Gateway
participant Verifier as Signature Verifier
participant Backend as Backend Server
Client->>Gateway: POST /api/... (with X-Webhook-Signature header)
Gateway->>Verifier: Verify signature (HMAC-SHA256)
Verifier-->>Gateway: Valid? Yes/No
alt Signature valid
Gateway->>Backend: Proxy request (forward body + headers)
Backend-->>Gateway: Response
Gateway-->>Client: JSON response
else Signature invalid
Gateway-->>Client: 401 JSON error (Unauthorized)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ord-server/ord-api.py (1)
5-9: 🛠️ Refactor suggestion | 🟠 MajorRemove duplicate imports.
hmacandhashlibare imported twice (lines 5-6 and lines 8-9).♻️ Proposed fix
import uuid import time import hmac import hashlib import yaml -import hmac -import hashlib from flask import Flask, jsonify, request🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ord-server/ord-api.py` around lines 5 - 9, Remove the duplicate imports of hmac and hashlib in ord-api.py; keep a single import statement for each module (e.g., import hmac and import hashlib) and delete the repeated lines so there is only one import occurrence per module (verify any other duplicate import lines in ord_api.py and clean them similarly).
🧹 Nitpick comments (5)
wrangler.toml (1)
8-10: Clarify configuration approach: secret vs. plain var.The comment suggests setting
ORD_BACKEND_URLviawrangler secret put, but the value is defined under[vars]which stores plaintext variables, not secrets. Secrets set viawrangler secret putoverride vars at runtime but aren't defined inwrangler.toml.If this URL should be treated as sensitive, remove the default value and rely solely on the secret. Otherwise, update the comment to reflect that this is a plain configuration variable with an optional secret override.
📝 Suggested comment clarification
[vars] ENVIRONMENT = "production" -# URL of the private ord backend service (Flask server on your ord node) -# Set via: npx wrangler secret put ORD_BACKEND_URL +# URL of the private ord backend service (Flask server on your ord node). +# Override in production via: npx wrangler secret put ORD_BACKEND_URL ORD_BACKEND_URL = "https://your-ord-backend.example.com"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wrangler.toml` around lines 8 - 10, The wrangler.toml currently defines ORD_BACKEND_URL under [vars] but the comment instructs using `npx wrangler secret put` which is misleading; either remove the plaintext default and treat ORD_BACKEND_URL exclusively as a secret (delete the value from wrangler.toml and rely on the secret set at runtime via `wrangler secret put ORD_BACKEND_URL`) or keep the plaintext var and update the comment to state this is a non-sensitive [vars] entry with an optional secret override, making sure the ORD_BACKEND_URL key name is referenced correctly in the chosen approach.ord-server/ord-api.py (2)
132-142: Consider usingtempfilemodule for safer temp file handling.While the UUID suffix provides good uniqueness, using Python's
tempfilemodule would provide additional safety guarantees (secure permissions, atomic creation) and is more idiomatic.♻️ Suggested improvement
+import tempfile + def write_temp_yaml(content: str) -> str: """Write content to a uniquely named temp file and return its path. Using a UUID suffix avoids race conditions when concurrent requests share the same YAML_FILE_PATH directory. """ base_dir = os.path.dirname(YAML_FILE_PATH) or "/tmp" - tmp_path = os.path.join(base_dir, f"batch-{uuid.uuid4().hex}.yaml") - with open(tmp_path, "w") as f: - f.write(content) + fd, tmp_path = tempfile.mkstemp(suffix=".yaml", prefix="batch-", dir=base_dir) + try: + with os.fdopen(fd, "w") as f: + f.write(content) + except: + os.close(fd) + raise return tmp_path🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ord-server/ord-api.py` around lines 132 - 142, The write_temp_yaml function currently creates temp files by composing paths with UUIDs; replace this with the tempfile API to ensure secure, atomic creation and correct permissions: use tempfile.NamedTemporaryFile or tempfile.mkstemp with dir derived from os.path.dirname(YAML_FILE_PATH) (fallback to "/tmp"), create the file with delete=False so it persists, write the provided content into that temporary file, close it, and return its path; keep the function name write_temp_yaml and preserve behavior of returning the filepath for downstream use.
145-160: Credentials in command-line arguments are visible in process listings.Passing
--bitcoin-rpc-passwordon the command line exposes credentials to other users on the system viaps auxor/proc. Whilesanitize_errorredacts these from error output, the credentials remain visible during command execution.The
ordCLI help output does not indicate support for passing credentials via environment variables or configuration files. If such an alternative exists inord, consider using it instead of embedding credentials in command-line arguments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ord-server/ord-api.py` around lines 145 - 160, make_base_command currently embeds credentials (e.g. --bitcoin-rpc-password) into the argv passed to ORD_PATH which exposes secrets in process listings; remove the password flags from the returned lists in make_base_command (keep ORD_PATH, username and url flags or remove username too if ord supports env-based auth) and change callers that invoke subprocess to set the RPC password via the environment (e.g. pass an env dict with BITCOIN_RPC_PASSWORD_REGTEST or BITCOIN_RPC_PASSWORD_MAINNET) instead of including --bitcoin-rpc-password on the command line; reference make_base_command, ORD_PATH and the --bitcoin-rpc-password flag when locating the changes and ensure sanitize_error handling stays if needed.src/index.py (1)
1-1: Nit: Replace en-dash with hyphen-minus in docstring.The docstring uses an en-dash (
–) which may cause issues with some tools. Use a standard hyphen-minus (-) instead.📝 Suggested fix
-"""BLT-Rewards Cloudflare Worker – public API gateway + static site host.""" +"""BLT-Rewards Cloudflare Worker - public API gateway + static site host."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.py` at line 1, The module docstring in src/index.py uses an en-dash character in the string """BLT-Rewards Cloudflare Worker – public API gateway + static site host."""; replace the en-dash with a standard hyphen-minus so the docstring reads """BLT-Rewards Cloudflare Worker - public API gateway + static site host.""" ensuring the module-level string is updated accordingly.ord-server/requirements.txt (1)
1-5: Consider pinning dependency versions for reproducibility.None of the dependencies have pinned versions. This can lead to inconsistent builds and potential breakage when upstream packages release incompatible updates. Consider pinning to specific versions or version ranges.
♻️ Example with pinned versions
-Flask -python-dotenv -PyYAML -gunicorn -requests +Flask>=3.0,<4.0 +python-dotenv>=1.0,<2.0 +PyYAML>=6.0,<7.0 +gunicorn>=21.0,<23.0 +requests>=2.31,<3.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ord-server/requirements.txt` around lines 1 - 5, The requirements.txt currently lists unpinned packages (Flask, python-dotenv, PyYAML, gunicorn, requests); update it to pin each dependency to a specific stable version or an acceptable version range (e.g., == or ~= operators) to ensure reproducible builds, for example replace each bare name with a pinned version (Flask, python-dotenv, PyYAML, gunicorn, requests) and commit the change; optionally include a comment at the top explaining the chosen pinning strategy and add a periodic review note to upgrade versions deliberately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/index.py`:
- Around line 59-62: The proxy currently only forwards Content-Type when calling
fetch and thus drops the X-Signature-256 header required by backend
verify_webhook_signature; update the headers passed to fetch in the call inside
src/index.py (the fetch invocation) to include the incoming request's
X-Signature-256 (e.g., read request.headers.get("X-Signature-256") or
equivalent) and add it to the headers dict sent to the backend so the backend's
verify_webhook_signature can validate the request; alternatively, if you intend
the gateway to be sole authenticator, remove or skip backend
verify_webhook_signature instead (prefer forwarding the header for
compatibility).
---
Outside diff comments:
In `@ord-server/ord-api.py`:
- Around line 5-9: Remove the duplicate imports of hmac and hashlib in
ord-api.py; keep a single import statement for each module (e.g., import hmac
and import hashlib) and delete the repeated lines so there is only one import
occurrence per module (verify any other duplicate import lines in ord_api.py and
clean them similarly).
---
Nitpick comments:
In `@ord-server/ord-api.py`:
- Around line 132-142: The write_temp_yaml function currently creates temp files
by composing paths with UUIDs; replace this with the tempfile API to ensure
secure, atomic creation and correct permissions: use tempfile.NamedTemporaryFile
or tempfile.mkstemp with dir derived from os.path.dirname(YAML_FILE_PATH)
(fallback to "/tmp"), create the file with delete=False so it persists, write
the provided content into that temporary file, close it, and return its path;
keep the function name write_temp_yaml and preserve behavior of returning the
filepath for downstream use.
- Around line 145-160: make_base_command currently embeds credentials (e.g.
--bitcoin-rpc-password) into the argv passed to ORD_PATH which exposes secrets
in process listings; remove the password flags from the returned lists in
make_base_command (keep ORD_PATH, username and url flags or remove username too
if ord supports env-based auth) and change callers that invoke subprocess to set
the RPC password via the environment (e.g. pass an env dict with
BITCOIN_RPC_PASSWORD_REGTEST or BITCOIN_RPC_PASSWORD_MAINNET) instead of
including --bitcoin-rpc-password on the command line; reference
make_base_command, ORD_PATH and the --bitcoin-rpc-password flag when locating
the changes and ensure sanitize_error handling stays if needed.
In `@ord-server/requirements.txt`:
- Around line 1-5: The requirements.txt currently lists unpinned packages
(Flask, python-dotenv, PyYAML, gunicorn, requests); update it to pin each
dependency to a specific stable version or an acceptable version range (e.g., ==
or ~= operators) to ensure reproducible builds, for example replace each bare
name with a pinned version (Flask, python-dotenv, PyYAML, gunicorn, requests)
and commit the change; optionally include a comment at the top explaining the
chosen pinning strategy and add a periodic review note to upgrade versions
deliberately.
In `@src/index.py`:
- Line 1: The module docstring in src/index.py uses an en-dash character in the
string """BLT-Rewards Cloudflare Worker – public API gateway + static site
host."""; replace the en-dash with a standard hyphen-minus so the docstring
reads """BLT-Rewards Cloudflare Worker - public API gateway + static site
host.""" ensuring the module-level string is updated accordingly.
In `@wrangler.toml`:
- Around line 8-10: The wrangler.toml currently defines ORD_BACKEND_URL under
[vars] but the comment instructs using `npx wrangler secret put` which is
misleading; either remove the plaintext default and treat ORD_BACKEND_URL
exclusively as a secret (delete the value from wrangler.toml and rely on the
secret set at runtime via `wrangler secret put ORD_BACKEND_URL`) or keep the
plaintext var and update the comment to state this is a non-sensitive [vars]
entry with an optional secret override, making sure the ORD_BACKEND_URL key name
is referenced correctly in the chosen approach.
🪄 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: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 1d47a1da-00b2-487e-ab85-b5f7dc13065e
📒 Files selected for processing (5)
ord-server/.env.exampleord-server/ord-api.pyord-server/requirements.txtsrc/index.pywrangler.toml
Comment on lines
+59
to
+62
| return await fetch( | ||
| f"{backend_url}{path}{query}", | ||
| {"method": request.method, "headers": {"Content-Type": "application/json"}, "body": body_text}, | ||
| ) |
There was a problem hiding this comment.
Backend signature verification will fail: X-Signature-256 header not forwarded.
The backend endpoints (e.g., /mainnet/send-bacon-tokens at ord-server/ord-api.py:206-209) also call verify_webhook_signature() which requires the X-Signature-256 header. The proxy only forwards Content-Type, so all proxied requests will receive a 401 from the backend.
Either forward the signature header to the backend, or remove the duplicate verification from the backend if the gateway is the sole authenticator.
🐛 Option 1: Forward the signature header
return await fetch(
f"{backend_url}{path}{query}",
- {"method": request.method, "headers": {"Content-Type": "application/json"}, "body": body_text},
+ {
+ "method": request.method,
+ "headers": {
+ "Content-Type": "application/json",
+ "X-Signature-256": signature,
+ },
+ "body": body_text,
+ },
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.py` around lines 59 - 62, The proxy currently only forwards
Content-Type when calling fetch and thus drops the X-Signature-256 header
required by backend verify_webhook_signature; update the headers passed to fetch
in the call inside src/index.py (the fetch invocation) to include the incoming
request's X-Signature-256 (e.g., read request.headers.get("X-Signature-256") or
equivalent) and add it to the headers dict sent to the backend so the backend's
verify_webhook_signature can validate the request; alternatively, if you intend
the gateway to be sole authenticator, remove or skip backend
verify_webhook_signature instead (prefer forwarding the header for
compatibility).
github-project-automation
bot
moved this from Backlog
to Ready
in 📌 OWASP BLT Project Board
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
validate_live_auth()— constant-time password check viahmac.compare_digest(prevents timing side-channel attacks on the wallet API password)WALLET_API_PASSWORDenv var rejects all live transactions with 503make_base_command,make_wallet_args,run_ord_command,sanitize_error,write_temp_yaml,validate_fee_rate)finallyblocks after each requestrequeststorequirements.txtPROJECT_INFO.mdtoord-server/.gitignoreSecurity
The previous
password != os.getenv("WALLET_API_PASSWORD")comparison was vulnerable to timing side-channel attacks. Replaced withhmac.compare_digest()which runs in constant time regardless of where the mismatch occurs.Changes
ord-server/ord-api.py—validate_live_auth()+ endpoint refactoringord-server/.env.example— updated YAML_FILE_PATH commentord-server/.gitignore— addPROJECT_INFO.mdord-server/requirements.txt— addrequestsTest plan
WALLET_API_PASSWORDenv var → 503