Repository navigation
fix: add GET-only guards and align api-local with deployment - #1090
Conversation
|
@AgentX010 is attempting to deploy a commit to the Collins' projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@AgentX010 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
collinsezedike
left a comment
There was a problem hiding this comment.
@AgentX010, thank you for your time on this. I have to request changes, because the branch does not build as it stands.
The handlers import requireGetMethod from api/_lib/middleware.ts, but that module neither defines nor exports it, and apps/api-local/src/routes/vaults.ts imports KNOWN_VAULT_IDS from ../lib/known-vault-ids, which does not exist. Both are unresolved imports, which is why Lint & Typecheck, Test, and E2E fail on the current head.
Beyond the build:
- The GET guard is used with opposite polarity in two places.
api/v1/admin/[resource].ts:33,41negate the call, whileapi/v1/positions/[publicKey].ts:11andapi/v1/vaults/[vaultId].ts:19do not. Pick one return contract and apply it consistently. - The api-local rate-limit change from #1054 is not present.
apps/api-local/src/routes/tx.ts:12,24still apply the per-routemax: 10limit to deposit and withdraw; thekeyGeneratorchange inindex.tsdoes not remove them. apps/api-local/src/index.ts:31narrows CORS toGET, OPTIONS, but the deployedapplyCorsadvertisesGET, POST, OPTIONSand this server has POST routes. That is new drift.- #1054 task 2 is not done.
api/v1/keepers/[action].tsis unchanged, so the comment still omitssnapshotand line 67 still says the keepers are three. pnpm format:checkfails on the extra blank line inapi/v1/admin/[resource].tsand the missing trailing newline inapi/v1/vaults/[vaultId].ts.- The Commit Messages check is failing on the head commit; make the message conform to CONTRIBUTING's rules.
Once the helper is added and the above are addressed, please request a re-review.
| import { | ||
| applyCors, | ||
| checkRateLimit, | ||
| requireGetMethod, |
There was a problem hiding this comment.
requireGetMethod is imported from ../../_lib/middleware.js, but that module neither defines nor exports it (git grep finds only this import and the call sites). Add the helper to api/_lib/middleware.ts, or this file will not compile.
| @@ -1,5 +1,6 @@ | |||
| import type { FastifyPluginAsync } from "fastify"; | |||
| import { handleGetVaults, handleGetVaultById } from "@meridian/api-core"; | |||
| import { KNOWN_VAULT_IDS } from "../lib/known-vault-ids"; | |||
There was a problem hiding this comment.
../lib/known-vault-ids does not exist in apps/api-local/src; there is no lib/ directory. Either add the module, or build the set from KNOWN_POOLS the way api/v1/vaults/[vaultId].ts does.
| const { resource } = req.query; | ||
|
|
||
| if (resource === "vault-state") { | ||
| if (!requireGetMethod(req, res)) return; |
There was a problem hiding this comment.
Guard polarity differs from the other two handlers here it is negated, while api/v1/positions/[publicKey].ts:11 and api/v1/vaults/[vaultId].ts:19 are not. Both cannot be correct. Define the helper's return contract and apply it the same way everywhere.
|
|
||
| await app.register(cors, { | ||
| origin: process.env.ALLOWED_ORIGIN ?? DEFAULT_ALLOWED_ORIGIN, | ||
| methods: ["GET", "OPTIONS"], |
There was a problem hiding this comment.
Restricting CORS to GET, OPTIONS drops POST, but the deployed applyCors advertises GET, POST, OPTIONS and this server serves POST routes under /api/v1/tx and /api/v1/keepers. This adds drift rather than removing it.
collinsezedike
left a comment
There was a problem hiding this comment.
@AgentX010 the build still fails at ee2985ba: requireGetMethod is imported but not defined, and the api-local route imports a module that does not exist. The admin guards also use the opposite polarity from the other handlers, and the CORS change drops POST. api/v1/vaults/index.ts still has no method guard, which task 1 of the issue also names.
| import { | ||
| applyCors, | ||
| checkRateLimit, | ||
| requireGetMethod, |
There was a problem hiding this comment.
@AgentX010 api/_lib/middleware.ts does not export requireGetMethod at this head, so this import and both admin calls fail to build. Add the helper there.
| @@ -1,5 +1,6 @@ | |||
| import type { FastifyPluginAsync } from "fastify"; | |||
| import { handleGetVaults, handleGetVaultById } from "@meridian/api-core"; | |||
| import { KNOWN_VAULT_IDS } from "../lib/known-vault-ids"; | |||
There was a problem hiding this comment.
@AgentX010 apps/api-local/src/lib/known-vault-ids.ts does not exist, so this import fails with ERR_MODULE_NOT_FOUND and the api-local tests cannot start. Build the set from KNOWN_POOLS, as the deployed handler does at api/v1/vaults/[vaultId].ts:8-12, or add the module.
| const { resource } = req.query; | ||
|
|
||
| if (resource === "vault-state") { | ||
| if (!requireGetMethod(req, res)) return; |
There was a problem hiding this comment.
@AgentX010 This is inverted relative to positions/[publicKey].ts:11 and vaults/[vaultId].ts:17, which use if (requireGetMethod(req, res)) return;. With the helper returning true when it has already sent 405, this returns before handling a valid GET. Use the same polarity at lines 33 and 41.
|
|
||
| await app.register(cors, { | ||
| origin: process.env.ALLOWED_ORIGIN ?? DEFAULT_ALLOWED_ORIGIN, | ||
| methods: ["GET", "OPTIONS"], |
There was a problem hiding this comment.
@AgentX010 Restricting CORS to GET, OPTIONS drops POST, but the local server serves the POST /api/v1/tx/* routes, so preflight now fails. Keep POST in the allowed methods.
ee2985b to
d218b22
Compare
collinsezedike
left a comment
There was a problem hiding this comment.
@AgentX010 The obfuscated payload has been removed. The prior blockers remain. requireGetMethod is still absent from api/_lib/middleware.ts. apps/api-local still imports a nonexistent known-vault-ids module. The admin guards still use the opposite polarity. The local CORS config still drops POST.
a14d500 to
8092425
Compare
8092425 to
925e007
Compare
collinsezedike
left a comment
There was a problem hiding this comment.
@AgentX010 thank you for your contribution. If you would like to pick up another open issue, the issue list has more work available.
|
@AgentX010 if Meridian is useful to you, would you consider starring the repository? |
Summary
Read-only API routes reject non-GET methods with 405 and an
Allow: GETheader. The local Fastify mirror now matches the deployed handlers for CORS methods, rate limiting, vault lookup, admin history, and admin method guards.Changes
requireGetMethodinapi/_lib/middleware.tsand applied it to positions, the vault list, vault detail, and both admin read branches.snapshot.KNOWN_POOLS, non-GET vault and admin routes return 405, and/admin/historymirrors the deployed route.branch_structure.json.Verification
pnpm --filter @meridian/api test: 96 passedpnpm --filter @meridian/api-local test: 45 passedpnpm --filter @meridian/api typecheckpnpm --filter @meridian/api-local typecheckpnpm --filter @meridian/api lintpnpm --filter @meridian/api-local lintCloses #1054