chore: add access-control-max-age header to cors #654
Conversation
WalkthroughCORS configuration updates in HTTP dispatch layer extending allowed HTTP methods from POST and OPTIONS to include GET, adding preflight cache duration header with 86400-second TTL, and updating associated header allowlists. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (2)
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 |
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
magicblock-aperture/src/server/http/dispatch.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Run CI - Format
magicblock-aperture/src/server/http/dispatch.rs
[error] 176-176: rustfmt check failed. Formatting issue detected in the imports: add trailing comma as required. Command: cargo +nightly fmt --check -- --config-path rustfmt-nightly.toml
[error] 1-1: Makefile: cargo fmt check failed with exit code 2.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_make_ci_lint
- GitHub Check: run_make_ci_test
- GitHub Check: Build Project
🔇 Additional comments (1)
magicblock-aperture/src/server/http/dispatch.rs (1)
186-186: LGTM! Appropriate preflight cache duration.The
Access-Control-Max-Ageheader with a value of 86400 seconds (24 hours) is a reasonable and commonly used duration for caching CORS preflight responses. This reduces the number of preflight requests while allowing timely policy updates.
| let response = Response::builder() | ||
| .header(ACCESS_CONTROL_ALLOW_ORIGIN, "*") | ||
| .header(ACCESS_CONTROL_ALLOW_METHODS, "POST, OPTIONS") | ||
| .header(ACCESS_CONTROL_ALLOW_METHODS, "POST, OPTIONS, GET") |
There was a problem hiding this comment.
🧩 Analysis chain
Verify GET method support and alignment with PR scope.
Adding GET to the allowed methods is not mentioned in the PR title or objectives, which only reference adding the max-age header. This appears to be scope creep.
Additionally, the JSON-RPC specification typically uses POST requests. Allowing GET requests could have security implications (e.g., sensitive data in query strings, browser caching) and may not be properly handled by the downstream RPC method handlers.
Run the following script to verify if GET requests are actually used or handled elsewhere in the codebase:
🏁 Script executed:
#!/bin/bash
# Description: Search for GET method handling or usage in RPC context
echo "=== Searching for GET method handling in HTTP/RPC code ==="
rg -n -C3 'Method::GET|method.*==.*GET|GET.*request' --type=rust
echo -e "\n=== Searching for HTTP method routing logic ==="
rg -n -C3 'Method::(POST|GET|PUT|DELETE)' --type=rust -g '!target/**'
echo -e "\n=== Checking if JSON-RPC handlers expect specific methods ==="
rg -n -C3 'json.*rpc|rpc.*method|sendTransaction|getAccountInfo' --type=rust -g '**/http/**' -g '!target/**'Length of output: 3972
I need to examine the dispatch.rs file more closely to understand the actual method handling and confirm whether GET requests would be properly handled.
Let me examine the dispatch.rs file directly to understand the method handling logic:
Based on my verification through web searches and script execution, the original review comment is substantively correct. The change to add GET to Access-Control-Allow-Methods is indeed problematic and represents scope creep.
Verify and remove the GET method from Access-Control-Allow-Methods header.
Solana JSON-RPC requires HTTP POST requests with a Content-Type: application/json header, and HTTP GET is not recommended for JSON-RPC, and JSON-RPC servers usually don't implement it. The codebase search found no GET method handling in the RPC dispatch logic—GET is only used in the separate metrics service for the /metrics endpoint.
Adding GET to the allowed methods without corresponding handler logic creates a mismatch: browsers or clients might attempt GET requests that won't be properly processed by the JSON-RPC handler. Additionally, HTTP GET requests may be cached and may not even reach the RPC-server, which is unsafe for RPC operations.
This change is also scope creep—the PR's stated objective is adding the max-age header, not modifying allowed HTTP methods. Remove "GET" from the header value, keeping only "POST, OPTIONS".
🤖 Prompt for AI Agents
magicblock-aperture/src/server/http/dispatch.rs around line 184: the
Access-Control-Allow-Methods header was incorrectly changed to include "GET",
which the RPC dispatch does not handle and is out of scope for this change;
revert the header value to only allow "POST, OPTIONS" (remove "GET"), ensuring
the CORS header matches actual supported methods for the JSON-RPC handler and
leaving the separate metrics GET endpoint unchanged.
lucacillario
deleted the
chore/add-access-control-max-age-header-to-cors
branch
* master: feat: use latest svm version (#657) chore: update solana account (#660) fix: better transaction diagnostics & rent exemption check (#642) chore: add access-control-max-age header to cors (#654) fix(aperture): prevent racy getLatestBlockhash (#649) fix: await until sub is established and perform them in parallel (#650) feat: persist all accounts (#648)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.