Skip to content

Ord/retry and error helpers#39

Open
Neurvinch wants to merge 2 commits intoOWASP-BLT:mainfrom
Neurvinch:ord/retry-and-error-helpers
Open

Ord/retry and error helpers#39
Neurvinch wants to merge 2 commits intoOWASP-BLT:mainfrom
Neurvinch:ord/retry-and-error-helpers

Conversation

@Neurvinch
Copy link
Copy Markdown
Contributor

@Neurvinch Neurvinch commented Mar 17, 2026

Summary

  • run_ord_command() — exponential-backoff retry on transient errors (connection refused, timeout, etc.)
  • sanitize_error() — strips RPC passwords from stderr before returning to caller
  • write_temp_yaml() — UUID-suffixed temp files to prevent race conditions between concurrent requests

Changes

  • ord-server/ord-api.py — 3 new helper functions + _TRANSIENT_ERROR_KEYWORDS list

Test plan

  • Transient errors (connection refused) trigger retry with backoff
  • Deterministic errors (bad args) fail immediately without retry
  • Temp YAML files use unique names (no collision between concurrent requests)
  • RPC passwords never appear in error responses

Summary by CodeRabbit

  • New Features

    • Webhook requests now require signature verification; valid requests are proxied to a configurable backend.
    • Requests under /mainnet/ and /regtest/ are routed to the backend; other paths serve the static site.
  • Chores

    • Improved resilience with automatic retries for transient ORD failures and sanitized error reporting.
    • Added configurable ORD backend URL via deployment configuration.

@owasp-blt
Copy link
Copy Markdown

owasp-blt bot commented Mar 17, 2026

📊 Monthly Leaderboard

Hi @Neurvinch! Here's how you rank for March 2026:

Rank User Open PRs PRs (merged) PRs (closed) Reviews Comments Total
32 xovishnukosuri @xovishnukosuri 2 2 0 0 1 24
33 Neurvinch @Neurvinch 10 1 0 0 2 24
34 Varshith2403315 @Varshith2403315 2 1 0 1 3 23

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 /leaderboard on any issue or PR to see your rank!

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 17, 2026

Walkthrough

Adds transient-error detection, retry/backoff, error sanitization, and temporary YAML helpers to ord-server; introduces HMAC-SHA256 webhook signature verification and request proxying to an ORD backend in the Worker entrypoint; and adds an ORD_BACKEND_URL configuration variable to wrangler.toml.

Changes

Cohort / File(s) Summary
ORD server helpers
ord-server/ord-api.py
Added _TRANSIENT_ERROR_KEYWORDS, _is_transient_error(), run_ord_command() (exponential-backoff retries), sanitize_error(), and write_temp_yaml() utilities. These add retry/sanitization and temp file creation for ORD command workflows.
Cloudflare Worker — request auth & proxy
src/index.py
Reworked on_fetch flow: added _verify_signature() (HMAC-SHA256), _json_error() helper, env checks for WEBHOOK_SECRET and ORD_BACKEND_URL, and proxy logic that validates signatures then forwards requests to the configured ORD backend. Static-site serving preserved for non-/mainnet/ and non-/regtest/ paths.
Configuration
wrangler.toml
Added ORD_BACKEND_URL variable to [vars] with placeholder value and explanatory comment.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Client
participant Worker
participant Backend
Client->>Worker: HTTP request (webhook) with body & X-Signature-256
Worker->>Worker: _verify_signature(body, header, WEBHOOK_SECRET)
alt signature valid
Worker->>Backend: fetch(ORD_BACKEND_URL, method/headers/body)
Backend-->>Worker: HTTP response
Worker-->>Client: proxied response
else signature invalid
Worker-->>Client: 401 JSON error
end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested labels

quality: medium

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Ord/retry and error helpers' directly and concisely summarizes the main change: adding retry logic and error handling helper functions to the ORD API.
Docstring Coverage ✅ Passed Docstring coverage is 87.50% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

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.

@owasp-blt
Copy link
Copy Markdown

owasp-blt bot commented Mar 17, 2026

👋 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:

  • The PR author
  • coderabbitai
  • copilot

Once a valid peer review is submitted, this check will pass automatically. Thank you!

⚠️ Peer review enforcement is active.

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

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)

181-194: ⚠️ Potential issue | 🟠 Major

New helpers are not integrated into existing endpoints.

The PR adds run_ord_command, sanitize_error, and write_temp_yaml but none of the endpoints use them:

  1. Race condition: Lines 161-162 and 221-222 still write to shared YAML_FILE_PATH instead of using write_temp_yaml
  2. No retry: Lines 182, 242, 267 use raw subprocess.run instead of run_ord_command
  3. Password exposure: Lines 192, 246, 276 return e.stderr directly without sanitize_error

This leaves the helpers as dead code and doesn't achieve the PR objectives.

🔧 Example integration for this endpoint
-    # Write YAML to a temporary file
-    with open(YAML_FILE_PATH, "w") as file:
-        file.write(yaml_content)
+    # Write YAML to a unique temporary file to avoid race conditions
+    tmp_yaml_path = write_temp_yaml(yaml_content)

     command = [
         "sudo",
         ORD_PATH,
         f"--bitcoin-rpc-username={BITCOIN_RPC_USER_MAINNET}",
         f"--bitcoin-rpc-password={BITCOIN_RPC_PASSWORD_MAINNET}",
         f"--bitcoin-rpc-url={BITCOIN_RPC_URL_MAINNET}",
         "wallet",
         f"--server-url={ORD_SERVER_URL_MAINNET}",
         f"--name={WALLET_NAME_MAINNET}",
         "split",
-        f"--splits={YAML_FILE_PATH}",
+        f"--splits={tmp_yaml_path}",
         f"--fee-rate={fee_rate}",
     ]

     if is_dry_run:
         command.append("--dry-run")

     try:
-        result = subprocess.run(command, capture_output=True, text=True, check=True)
+        result = run_ord_command(command)
         txid = result.stdout.strip()
         return jsonify({
             "success": True,
             "txid": txid,
             "dry_run": is_dry_run
         })
     except subprocess.CalledProcessError as e:
         return jsonify({
             "success": False,
-            "error": e.stderr,
+            "error": sanitize_error(e.stderr or ""),
             "dry_run": is_dry_run
         })
🤖 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 181 - 194, The endpoint is still using
the shared YAML_FILE_PATH, raw subprocess.run calls, and returning e.stderr
directly; update the handler to create per-request temp YAML via write_temp_yaml
(instead of writing YAML_FILE_PATH), invoke ord using the run_ord_command helper
(replace subprocess.run usage and its check logic), and when catching
subprocess.CalledProcessError pass the stderr through sanitize_error before
including it in the JSON response so passwords are not leaked; ensure you
reference and remove direct uses of YAML_FILE_PATH, subprocess.run, and
CalledProcessError in this handler and use write_temp_yaml, run_ord_command, and
sanitize_error respectively.
🧹 Nitpick comments (1)
ord-server/ord-api.py (1)

128-138: Missing cleanup mechanism for temporary files.

The function creates uniquely named temp files but never deletes them. Over time, these will accumulate and consume disk space. Consider either:

  1. Returning a context manager that cleans up on exit
  2. Using Python's tempfile module with automatic cleanup
  3. Documenting that callers must delete the file after use

The /tmp fallback flagged by static analysis (S108) is acceptable here given the UUID-based naming and controlled content.

♻️ Option: Use tempfile for automatic cleanup
+import tempfile
+from contextlib import contextmanager
+
+@contextmanager
+def write_temp_yaml(content: str):
+    """Write content to a temp file, yielding its path, then clean up."""
+    base_dir = os.path.dirname(YAML_FILE_PATH) or None
+    with tempfile.NamedTemporaryFile(
+        mode="w", suffix=".yaml", prefix="batch-", dir=base_dir, delete=False
+    ) as f:
+        f.write(content)
+        tmp_path = f.name
+    try:
+        yield tmp_path
+    finally:
+        os.unlink(tmp_path)

Usage would then be:

with write_temp_yaml(yaml_content) as tmp_path:
    command = [..., f"--splits={tmp_path}", ...]
    run_ord_command(command)
🤖 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 128 - 138, The write_temp_yaml function
creates temp files (tmp_path) under YAML_FILE_PATH (or /tmp) but never deletes
them; change it to return a context manager that yields the temp file path and
removes the file on exit (use contextlib.contextmanager or implement a small
class with __enter__/__exit__), or alternatively switch to
tempfile.NamedTemporaryFile/TemporaryDirectory patterns that guarantee cleanup;
update callers to use "with write_temp_yaml(...)" so the temporary file used by
write_temp_yaml is automatically deleted when the context exits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@ord-server/ord-api.py`:
- Around line 97-113: Validate max_retries at the start of run_ord_command
(raise ValueError if max_retries < 1) so the loop always has a meaningful
contract, and ensure you never raise a None exception; alternatively enforce
max_retries = max(1, int(max_retries)). Also defensively handle e.stderr being
None by calling _is_transient_error(e.stderr or "") (or checking for None before
accessing .lower() inside _is_transient_error) so transient-checks don't crash;
keep raising the captured CalledProcessError (last_exc) on final failure.

---

Outside diff comments:
In `@ord-server/ord-api.py`:
- Around line 181-194: The endpoint is still using the shared YAML_FILE_PATH,
raw subprocess.run calls, and returning e.stderr directly; update the handler to
create per-request temp YAML via write_temp_yaml (instead of writing
YAML_FILE_PATH), invoke ord using the run_ord_command helper (replace
subprocess.run usage and its check logic), and when catching
subprocess.CalledProcessError pass the stderr through sanitize_error before
including it in the JSON response so passwords are not leaked; ensure you
reference and remove direct uses of YAML_FILE_PATH, subprocess.run, and
CalledProcessError in this handler and use write_temp_yaml, run_ord_command, and
sanitize_error respectively.

---

Nitpick comments:
In `@ord-server/ord-api.py`:
- Around line 128-138: The write_temp_yaml function creates temp files
(tmp_path) under YAML_FILE_PATH (or /tmp) but never deletes them; change it to
return a context manager that yields the temp file path and removes the file on
exit (use contextlib.contextmanager or implement a small class with
__enter__/__exit__), or alternatively switch to
tempfile.NamedTemporaryFile/TemporaryDirectory patterns that guarantee cleanup;
update callers to use "with write_temp_yaml(...)" so the temporary file used by
write_temp_yaml is automatically deleted when the context exits.
🪄 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: 4261cc94-fa30-46ab-bbc5-2fe1709c56ba

📥 Commits

Reviewing files that changed from the base of the PR and between 9a1b356 and 7679aed.

📒 Files selected for processing (1)
  • ord-server/ord-api.py

Comment on lines +97 to +113
def run_ord_command(command: list, max_retries: int = 3, retry_delay: float = 2.0):
"""Run an ord CLI command with exponential-backoff retry on transient errors.

Returns the CompletedProcess on success; raises subprocess.CalledProcessError
on final failure.
"""
last_exc = None
for attempt in range(max_retries):
try:
return subprocess.run(command, capture_output=True, text=True, check=True)
except subprocess.CalledProcessError as e:
last_exc = e
if attempt < max_retries - 1 and _is_transient_error(e.stderr):
time.sleep(retry_delay * (2 ** attempt))
continue
break
raise last_exc
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Edge case: max_retries <= 0 causes TypeError.

If max_retries is 0 or negative, the loop never executes and last_exc remains None, causing raise last_exc to raise TypeError instead of a meaningful error.

Additionally, e.stderr could potentially be None in certain subprocess failure modes, which would cause _is_transient_error to fail on .lower().

🛡️ Proposed fix with defensive checks
 def run_ord_command(command: list, max_retries: int = 3, retry_delay: float = 2.0):
     """Run an ord CLI command with exponential-backoff retry on transient errors.
 
     Returns the CompletedProcess on success; raises subprocess.CalledProcessError
     on final failure.
     """
+    if max_retries < 1:
+        max_retries = 1
     last_exc = None
     for attempt in range(max_retries):
         try:
             return subprocess.run(command, capture_output=True, text=True, check=True)
         except subprocess.CalledProcessError as e:
             last_exc = e
-            if attempt < max_retries - 1 and _is_transient_error(e.stderr):
+            if attempt < max_retries - 1 and _is_transient_error(e.stderr or ""):
                 time.sleep(retry_delay * (2 ** attempt))
                 continue
             break
     raise last_exc
🧰 Tools
🪛 Ruff (0.15.6)

[error] 106-106: subprocess call: check for execution of untrusted input

(S603)

🤖 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 97 - 113, Validate max_retries at the
start of run_ord_command (raise ValueError if max_retries < 1) so the loop
always has a meaningful contract, and ensure you never raise a None exception;
alternatively enforce max_retries = max(1, int(max_retries)). Also defensively
handle e.stderr being None by calling _is_transient_error(e.stderr or "") (or
checking for None before accessing .lower() inside _is_transient_error) so
transient-checks don't crash; keep raising the captured CalledProcessError
(last_exc) on final failure.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds three utility helpers to ord-api.py: retry logic for transient CLI errors, error message sanitization, and race-condition-safe temp file creation. Also removes some inline comments and adds section headers.

Changes:

  • Added run_ord_command() with exponential-backoff retry, sanitize_error() for credential redaction, and write_temp_yaml() for unique temp files
  • Reorganized file with section comment headers and trimmed some docstring/comment verbosity

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +128 to +138
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)
return tmp_path
Neurvinch and others added 2 commits March 17, 2026 12:39
…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>
@Neurvinch Neurvinch force-pushed the ord/retry-and-error-helpers branch from 7679aed to 9b8131a Compare March 17, 2026 07:10
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: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@ord-server/ord-api.py`:
- Around line 101-142: The live route handlers still call subprocess.run, write
to the shared YAML_FILE_PATH, and return raw stderr; update each place that
directly invokes subprocess.run to call run_ord_command(...) instead (so
retries/backoff are used), replace any direct file writes that use
YAML_FILE_PATH with write_temp_yaml(content) to get an isolated temp path, and
ensure any exception handlers that return or propagate e.stderr instead call
sanitize_error(e.stderr) (or sanitize_error(stderr)) so secrets are redacted;
search for direct uses of subprocess.run, YAML_FILE_PATH, and e.stderr in the
handlers and swap them to run_ord_command, write_temp_yaml, and sanitize_error
respectively.

In `@src/index.py`:
- Around line 58-62: The proxied fetch drops the X-Signature-256 header,
duplicates the query prefix, and always includes a body regardless of
request.method; update the fetch call in src/index.py so it forwards the
incoming "X-Signature-256" header (and preserve other original headers as
appropriate), use url.search directly instead of f"?{url.search}" to avoid a
double "?", and only include body_text in the fetch options for methods that
allow a body (i.e., not for GET or HEAD). Ensure you reference the existing
variables backend_url, path, url.search, request.method, body_text and include
the "X-Signature-256" header in the headers object passed to fetch.

In `@wrangler.toml`:
- Around line 8-10: The wrangler.toml currently contains a truthy placeholder
value for ORD_BACKEND_URL which makes the Worker treat it as configured; remove
the placeholder (leave ORD_BACKEND_URL unset or empty) and/or update the runtime
validation in src/index.py where ORD_BACKEND_URL is read (the request handler or
init logic that checks its truthiness) to explicitly reject the placeholder
string "https://your-ord-backend.example.com" and any empty/placeholder values
by returning a 500 (or failing fast) with a clear error message; ensure the
validation references the ORD_BACKEND_URL environment variable and treats the
placeholder as invalid so deployments without a real secret won’t silently proxy
to the dummy host.
🪄 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: 9dc686be-7447-44bb-a429-008610cf4d59

📥 Commits

Reviewing files that changed from the base of the PR and between 7679aed and 9b8131a.

📒 Files selected for processing (3)
  • ord-server/ord-api.py
  • src/index.py
  • wrangler.toml

Comment on lines +101 to +142
def run_ord_command(command: list, max_retries: int = 3, retry_delay: float = 2.0):
"""Run an ord CLI command with exponential-backoff retry on transient errors.

Returns the CompletedProcess on success; raises subprocess.CalledProcessError
on final failure.
"""
last_exc = None
for attempt in range(max_retries):
try:
return subprocess.run(command, capture_output=True, text=True, check=True)
except subprocess.CalledProcessError as e:
last_exc = e
if attempt < max_retries - 1 and _is_transient_error(e.stderr):
time.sleep(retry_delay * (2 ** attempt))
continue
break
raise last_exc


def sanitize_error(stderr: str) -> str:
"""Strip sensitive values from stderr before sending it to the caller."""
for secret in [
BITCOIN_RPC_PASSWORD_MAINNET,
BITCOIN_RPC_PASSWORD_REGTEST,
os.getenv("WALLET_API_PASSWORD", ""),
]:
if secret:
stderr = stderr.replace(secret, "[REDACTED]")
return stderr.strip()


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)
return tmp_path
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

The new ORD helpers are not wired into any live endpoint yet.

Lines 186, 246, and 271 still call subprocess.run directly, Lines 165-166 and 225-226 still write the shared YAML_FILE_PATH, and Lines 196, 250, and 280 still return raw e.stderr. As written, the live routes still miss retry/backoff, temp-file isolation, and password redaction.

🧰 Tools
🪛 Ruff (0.15.6)

[error] 110-110: subprocess call: check for execution of untrusted input

(S603)


[error] 138-138: Probable insecure usage of temporary file or directory: "/tmp"

(S108)

🤖 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 101 - 142, The live route handlers still
call subprocess.run, write to the shared YAML_FILE_PATH, and return raw stderr;
update each place that directly invokes subprocess.run to call
run_ord_command(...) instead (so retries/backoff are used), replace any direct
file writes that use YAML_FILE_PATH with write_temp_yaml(content) to get an
isolated temp path, and ensure any exception handlers that return or propagate
e.stderr instead call sanitize_error(e.stderr) (or sanitize_error(stderr)) so
secrets are redacted; search for direct uses of subprocess.run, YAML_FILE_PATH,
and e.stderr in the handlers and swap them to run_ord_command, write_temp_yaml,
and sanitize_error respectively.

Comment on lines +58 to +62
query = f"?{url.search}" if url.search else ""
return await fetch(
f"{backend_url}{path}{query}",
{"method": request.method, "headers": {"Content-Type": "application/json"}, "body": body_text},
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, let's look at the actual code in src/index.py around lines 58-62
head -n 75 src/index.py | tail -n 30

Repository: OWASP-BLT/BLT-Rewards

Length of output: 1380


🏁 Script executed:

# Let's also see a broader context of the function containing lines 58-62
head -n 100 src/index.py | tail -n 50

Repository: OWASP-BLT/BLT-Rewards

Length of output: 2075


🏁 Script executed:

# Check if ord-server/ord-api.py exists and examine the verification logic
test -f ord-server/ord-api.py && head -n 60 ord-server/ord-api.py || echo "File not found"

Repository: OWASP-BLT/BLT-Rewards

Length of output: 1747


🏁 Script executed:

# Look for where signature or X-Signature-256 is used in the codebase
rg "X-Signature-256|signature" src/ -A 2 -B 2

Repository: OWASP-BLT/BLT-Rewards

Length of output: 1243


🏁 Script executed:

# Verify WHATWG URL.search behavior - does it include the leading ?
python3 << 'EOF'
from urllib.parse import urlparse, parse_qs

# Test URL parsing to understand search behavior
test_urls = [
    "https://example.com/path?key=value",
    "https://example.com/path",
    "https://example.com/path?a=1&b=2"
]

for url_str in test_urls:
    parsed = urlparse(url_str)
    query_string = parsed.query
    # In WHATWG standard, URLSearchParams is accessed via url.search which includes the ?
    # But urllib.parse.query doesn't include it
    print(f"URL: {url_str}")
    print(f"  query (no ?): {query_string}")
    print(f"  reconstructed (with ?): ?{query_string if query_string else ''}")
    print()
EOF

Repository: OWASP-BLT/BLT-Rewards

Length of output: 342


🏁 Script executed:

# Let's check the JavaScript/WHATWG URL behavior more directly
# Looking at what url.search should return in the Cloudflare Workers context
# The code uses "URL.new(request.url)" which is the WHATWG URL standard
# In WHATWG standard, url.search includes the leading ? character if a query exists
python3 << 'EOF'
# Simulating WHATWG URL.search behavior
# According to WHATWG URL spec, url.search returns:
# - empty string "" if no query
# - "?querystring" (WITH the leading ?) if query exists

test_cases = [
    ("https://example.com/path?key=value", "?key=value"),
    ("https://example.com/path", ""),
    ("https://example.com/path?", "?"),
]

for url, expected_search in test_cases:
    # The code does: f"?{url.search}" 
    # If url.search is already "?key=value", this becomes "??key=value"
    if expected_search:
        constructed = f"?{expected_search}"
        print(f"URL: {url}")
        print(f"  url.search should be: {expected_search}")
        print(f"  Code constructs: {constructed}")
        print(f"  ISSUE: Double question mark!" if constructed.startswith("??") else "")
    else:
        print(f"URL: {url} -> url.search is empty, so no query in output")
    print()
EOF

Repository: OWASP-BLT/BLT-Rewards

Length of output: 390


🏁 Script executed:

# Confirm the backend Flask app uses the verify_webhook_signature check
grep -n "verify_webhook_signature" ord-server/ord-api.py | head -20

Repository: OWASP-BLT/BLT-Rewards

Length of output: 252


🏁 Script executed:

# Check if any routes in the backend skip signature verification
grep -B5 -A5 "@app.route" ord-server/ord-api.py | head -50

Repository: OWASP-BLT/BLT-Rewards

Length of output: 1337


Forward the X-Signature-256 header and preserve request semantics in the proxied call.

The outbound proxy at lines 58–62 drops the X-Signature-256 header that was just verified, so every proxied call to the backend will be rejected with 401. The Flask backend at ord-server/ord-api.py (lines 147, 202, 254) calls verify_webhook_signature(request) on every protected route—including GET endpoints like /mainnet/wallet-balance—and rejects requests without the header.

Additionally, line 58 duplicates the query string prefix: url.search in the WHATWG standard already includes the leading ?, so f"?{url.search}" becomes ??key=value. The body is also unconditionally included in the fetch options, which violates HTTP semantics for GET and HEAD methods.

Suggested fix
-    query = f"?{url.search}" if url.search else ""
-    return await fetch(
-        f"{backend_url}{path}{query}",
-        {"method": request.method, "headers": {"Content-Type": "application/json"}, "body": body_text},
-    )
+    method = request.method.upper()
+    headers = {"X-Signature-256": signature}
+    if method not in ("GET", "HEAD"):
+        headers["Content-Type"] = "application/json"
+
+    init = {"method": method, "headers": headers}
+    if method not in ("GET", "HEAD"):
+        init["body"] = body_text
+
+    return await fetch(f"{backend_url}{path}{url.search}", init)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/index.py` around lines 58 - 62, The proxied fetch drops the
X-Signature-256 header, duplicates the query prefix, and always includes a body
regardless of request.method; update the fetch call in src/index.py so it
forwards the incoming "X-Signature-256" header (and preserve other original
headers as appropriate), use url.search directly instead of f"?{url.search}" to
avoid a double "?", and only include body_text in the fetch options for methods
that allow a body (i.e., not for GET or HEAD). Ensure you reference the existing
variables backend_url, path, url.search, request.method, body_text and include
the "X-Signature-256" header in the headers object passed to fetch.

Comment on lines +8 to +10
# URL of the private ord backend service (Flask server on your ord node)
# Set via: npx wrangler secret put ORD_BACKEND_URL
ORD_BACKEND_URL = "https://your-ord-backend.example.com"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Don't keep a truthy placeholder for ORD_BACKEND_URL.

Line 10 makes the backend look configured even when deployment forgot the real value. Because src/index.py Lines 47-50 only check truthiness, the Worker will proxy to the dummy host instead of failing fast with the intended 500. If this is meant to come from wrangler secret put, leave it unset here or reject the placeholder explicitly.

🤖 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 contains a
truthy placeholder value for ORD_BACKEND_URL which makes the Worker treat it as
configured; remove the placeholder (leave ORD_BACKEND_URL unset or empty) and/or
update the runtime validation in src/index.py where ORD_BACKEND_URL is read (the
request handler or init logic that checks its truthiness) to explicitly reject
the placeholder string "https://your-ord-backend.example.com" and any
empty/placeholder values by returning a 500 (or failing fast) with a clear error
message; ensure the validation references the ORD_BACKEND_URL environment
variable and treats the placeholder as invalid so deployments without a real
secret won’t silently proxy to the dummy host.

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

Projects

Status: Ready

Development

Successfully merging this pull request may close these issues.

2 participants