fix(evm): remove unauthenticated NFT metadata endpoint (SSRF, FOX-168)#1280
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
💤 Files with no reviewable changes (9)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR removes the token metadata feature from the EVM coinstack: types and API contract, controller route, service implementations (Blockbook/Moralis), and the per-chain OpenAPI token-metadata schemas and endpoint. ChangesEVM Token Metadata Feature Removal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
kaladinlight
force-pushed
the
fix/evm-metadata-url-validation
branch
from
901afb5 to
0930a21
Compare
kaladinlight
changed the title
![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
🤖 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 `@node/coinstacks/common/api/src/evm/controller.ts`:
- Line 12: The checked-in OpenAPI contract still advertises the removed endpoint
/api/v1/metadata/token and the TokenMetadata schema; regenerate or prune the
swagger.json to remove the stale path and schema so docs/clients no longer
advertise a dead route. Specifically, run the swagger/OpenAPI generation used by
this project (or manually edit the spec) to delete the /api/v1/metadata/token
path and the TokenMetadata component, ensuring the updated contract matches the
actual controller surface (controller.ts imports/models) and commit the
regenerated swagger.json.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c453dc94-4f86-4f43-91ed-61b189ac28ca
📒 Files selected for processing (4)
node/coinstacks/common/api/src/evm/blockbookService.tsnode/coinstacks/common/api/src/evm/controller.tsnode/coinstacks/common/api/src/evm/models.tsnode/coinstacks/common/api/src/evm/moralisService.ts
💤 Files with no reviewable changes (1)
- node/coinstacks/common/api/src/evm/models.ts
Remove `GET /api/v1/metadata/token` and all of its supporting code (controller route, TokenMetadata/TokenType models, and the moralis + blockbook getTokenMetadata implementations). The endpoint made unauthenticated server-side HTTP requests to URLs derived from attacker-controlled NFT metadata (on-chain `tokenURI()`, `image`, `animation_url`), following redirects with no host allowlist or private-IP filtering. This is a blind SSRF (FOX-168) whose only reflected signal is the fetched `Content-Type` (image/video). The endpoint is unused by the web app, so removing the feature eliminates the attack surface outright rather than mitigating it with an outbound-URL allowlist (which remains exposed to DNS-rebinding). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kaladinlight
force-pushed
the
fix/evm-metadata-url-validation
branch
from
0930a21 to
401f544
Compare
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
2 participants
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.
Description
Removes
GET /api/v1/metadata/tokenand all of its supporting code instead of trying to sandbox the outbound fetch.The endpoint made an unauthenticated server-side HTTP request to a URL taken straight from attacker-controlled NFT metadata (on-chain
tokenURI(), plusimage/animation_url), and followed redirects with no host allowlist or private-IP filtering. That's a blind SSRF (HackenProof FOX-168) — the only signal reflected to the client is the fetchedContent-Typemapped toimage/video.Since the endpoint is unused by the web app, deleting it eliminates the attack surface entirely rather than mitigating it with an outbound-URL allowlist (which stays exposed to DNS-rebinding / TOCTOU).
Changes
evm/controller.ts— remove the/metadata/tokenroute handler.evm/models.ts— removeTokenMetadata,TokenType, andgetTokenMetadatafrom theAPIinterface.evm/moralisService.ts/evm/blockbookService.ts— remove thegetTokenMetadataimplementations and now-unused imports.routes.ts/swagger.jsonare gitignored tsoa build artifacts and regenerate from the controller, so they don't need touching.Severity note
Assessed Low: blind SSRF with a 1-bit content-type oracle, no response-body reflection, and no demonstrated credential path. These APIs run in Railway's isolated/containerized environment with no cloud IMDS reachable, so the usual SSRF → credential-theft escalation doesn't apply. Reported as High; removing the feature outright is the cleanest remediation regardless.
Breaking change
/api/v1/metadata/tokenis a public endpoint onapi.<chain>.shapeshift.com. Any third-party consumer relying on it will 404 after this. Confirmed unused by the ShapeShift web app.Verification
Didn't run build/tests locally per repo CLAUDE.md. Change is a pure deletion of a self-contained feature; remaining
TokenMetadata/TokenType/getTokenMetadatareferences are zero across source.Summary by CodeRabbit