fix(admin): avoid encoded slash admin URLs#310
Merged
SantiagoDePolonia merged 2 commits intomainfrom May 8, 2026
Merged
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Contributor
📝 WalkthroughWalkthroughThis PR consolidates five admin resource endpoints (budgets, model overrides, model pricing overrides, aliases, guardrails) from path-parameterized routes to collection-style base routes that accept identifiers in JSON request bodies. The changes span API contracts, backend handlers, frontend client code, and comprehensive test coverage across unit, integration, and E2E layers. ChangesAdmin API Endpoint Consolidation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR migrates admin write/delete endpoints for budgets, aliases, guardrails, model overrides, and model pricing overrides from URL path parameters to JSON request bodies, solving the encoded-slash problem for identifiers like
Confidence Score: 4/5The refactor is mechanically correct and well-covered by tests, but the budget handlers do not guard against an empty user_path, which silently routes write and delete operations to the root '/' budget — a data-integrity risk for any caller that accidentally omits the field. The alias, guardrail, and model-override handlers all explicitly reject an empty identifier after the refactor, but budgetRequestKey (used by UpsertBudget and DeleteBudget) and the inline call in ResetBudget skip that check. An omitted user_path will silently affect the global root budget rather than returning 400, which can corrupt budget state for deployments that rely on the root limit. The rest of the change is straightforward and the JS clients validate inputs client-side before sending. internal/admin/handler_budgets.go — the budgetRequestKey helper and the ResetBudget handler both need an explicit user_path emptiness check. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as Dashboard or API Client
participant Echo as Echo Router
participant Handler as Admin Handler
participant Store as Budget or Override Store
Note over Client,Store: Before this PR — identifier in URL path
Client->>Echo: PUT /admin/api/v1/budgets/teams-acme/daily
Echo->>Handler: Param(user_path), Param(period)
Handler->>Store: Upsert(userPath, period, amount)
Note over Client,Store: After this PR — identifier in JSON body
Client->>Echo: PUT /admin/api/v1/budgets with body
Echo->>Handler: Bind(req)
Handler->>Handler: budgetRequestKey(req.UserPath, req.BudgetKey)
alt user_path is omitted or empty
Handler-->>Client: 200 OK silently targets root budget
else user_path provided
Handler->>Store: Upsert(userPath, periodSeconds, amount)
Store-->>Client: 200 budgetListResponse
end
Reviews (2): Last reviewed commit: "fix(admin): align request schemas with h..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@internal/admin/handler_budgets.go`:
- Around line 243-254: Change the body contract to include an explicit budget
key object and enforce its either/or requirement: add a new field BudgetKey
`json:"budget_key" binding:"required"` (type struct { Period string
`json:"period,omitempty"`; PeriodSeconds int64 `json:"period_seconds,omitempty"`
}) to both upsertBudgetRequest and deleteBudgetRequest so the generated OpenAPI
publishes the key object, then add validation in the corresponding handlers
(e.g., upsertBudgetHandler and deleteBudgetHandler) to return 400 if neither
BudgetKey.Period nor BudgetKey.PeriodSeconds is provided (or if both are
provided, if you want exclusive) — do not rely on implicit tags alone so the
runtime and schema agree.
In `@internal/admin/handler_guardrails.go`:
- Around line 15-25: Remove the incorrect Gin `binding:"required"` tags from the
request structs so they don't mislead readers: edit the upsertGuardrailRequest
and deleteGuardrailRequest definitions to drop `binding:"required"` from the
Name fields (and any other `binding` tags), leaving only valid JSON tags; keep
the existing manual validation in the handler functions (the
strings.TrimSpace/empty checks in the upsert/delete handlers) or replace them
with Echo-compatible validation (register a custom Validator and use `validate`
tags) if you prefer automatic validation.
In `@internal/admin/handler_model_pricing_overrides.go`:
- Around line 45-59: The OpenAPI comments for the admin model-pricing overrides
endpoints (e.g., the UpsertModelPricingOverride handler documented at the top of
internal/admin/handler_model_pricing_overrides.go and the other operation around
lines 91-104) are missing a 502 Bad Gateway response even though
pricingOverrideWriteError returns http.StatusBadGateway for non-validation write
failures; update both Swagger comment blocks to add a `@Failure` 502 {object}
core.GatewayError (or equivalent) entry so the generated spec documents the Bad
Gateway error path produced by pricingOverrideWriteError.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 733bc42f-261d-4a66-807d-a66d1352ee1c
📒 Files selected for processing (31)
README.mdcmd/gomodel/docs/docs.godocs/openapi.jsoninternal/admin/dashboard/dashboard.gointernal/admin/dashboard/dashboard_test.gointernal/admin/dashboard/static/js/modules/aliases.jsinternal/admin/dashboard/static/js/modules/aliases.test.cjsinternal/admin/dashboard/static/js/modules/budgets.jsinternal/admin/dashboard/static/js/modules/budgets.test.cjsinternal/admin/dashboard/static/js/modules/guardrails.jsinternal/admin/dashboard/static/js/modules/guardrails.test.cjsinternal/admin/dashboard/static/js/modules/model-pricing-overrides.jsinternal/admin/dashboard/static/js/modules/model-pricing-overrides.test.cjsinternal/admin/errors.gointernal/admin/handler_aliases.gointernal/admin/handler_aliases_test.gointernal/admin/handler_budgets.gointernal/admin/handler_budgets_test.gointernal/admin/handler_guardrails.gointernal/admin/handler_guardrails_test.gointernal/admin/handler_model_overrides.gointernal/admin/handler_model_overrides_test.gointernal/admin/handler_model_pricing_overrides.gointernal/admin/handler_model_pricing_overrides_test.gointernal/admin/handler_test.gointernal/admin/routes.gointernal/admin/routes_test.gointernal/server/http_test.gotests/e2e/budget_test.gotests/e2e/release-e2e-scenarios.mdtests/integration/workflows_guardrails_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/openapi.json (1)
233-1210:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the alias admin mutations to the generated OpenAPI too.
This refreshes the body-based contracts for budgets and model overrides, but the generated spec still has no
/admin/api/v1/aliasesentries at all. The release E2E matrix in this PR already switched alias create/delete toPUT/DELETE /admin/api/v1/aliaseswithnamein the JSON body, so SDK/docs consumers will still see an incomplete admin API after merge.🤖 Prompt for 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. In `@docs/openapi.json` around lines 233 - 1210, The OpenAPI spec is missing the admin alias mutation endpoints; add a new path entry for "/admin/api/v1/aliases" implementing at minimum PUT and DELETE operations that mirror other admin mutation patterns: use requestBody JSON schemas (e.g. admin.upsertAliasRequest for PUT and admin.deleteAliasRequest for DELETE), include appropriate responses (200 or 204 and standard error responses referencing core.GatewayError), and include the security section with BearerAuth; follow the existing style of "/admin/api/v1/budgets" and "/admin/api/v1/model-overrides" for tags ("admin"), summaries, descriptions, and response blocks so SDK/docs consumers see the create/delete alias mutations (PUT/DELETE with name in the body).
🤖 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 `@docs/openapi.json`:
- Around line 412-421: Update the components schema admin.resetBudgetsRequest so
it requires the reset confirmation token (the exact property name used by the
handler / TestResetBudgetsEndpoint) instead of allowing an empty object or
arbitrary string: add a properties block with the confirmation field (string),
include that field in the required array, and set additionalProperties to false
so generated clients cannot send {} or other shapes the server will reject.
In `@tools/openapi-postprocess.mjs`:
- Around line 185-193: The budget schemas are missing a required "user_path"
property causing client requests to be rejected at runtime; update the
postprocess to call ensureRequiredProperty("admin.upsertBudgetRequest",
"user_path") and ensureRequiredProperty("admin.deleteBudgetRequest",
"user_path") (in the same block alongside the existing ensureRequiredProperty
calls and before applyBudgetKeySchemaConstraints()) so the generated OpenAPI
schemas mark user_path as required for upsert and delete handlers.
---
Outside diff comments:
In `@docs/openapi.json`:
- Around line 233-1210: The OpenAPI spec is missing the admin alias mutation
endpoints; add a new path entry for "/admin/api/v1/aliases" implementing at
minimum PUT and DELETE operations that mirror other admin mutation patterns: use
requestBody JSON schemas (e.g. admin.upsertAliasRequest for PUT and
admin.deleteAliasRequest for DELETE), include appropriate responses (200 or 204
and standard error responses referencing core.GatewayError), and include the
security section with BearerAuth; follow the existing style of
"/admin/api/v1/budgets" and "/admin/api/v1/model-overrides" for tags ("admin"),
summaries, descriptions, and response blocks so SDK/docs consumers see the
create/delete alias mutations (PUT/DELETE with name in the body).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 6b7b6db9-6df2-43df-8c44-964aeb06e529
📒 Files selected for processing (13)
cmd/gomodel/docs/docs.godocs/openapi.jsoninternal/admin/dashboard/static/js/modules/budgets.jsinternal/admin/dashboard/static/js/modules/budgets.test.cjsinternal/admin/handler_aliases.gointernal/admin/handler_budgets.gointernal/admin/handler_budgets_test.gointernal/admin/handler_guardrails.gointernal/admin/handler_model_overrides.gointernal/admin/handler_model_pricing_overrides.gotests/e2e/budget_test.gotests/e2e/release-e2e-scenarios.mdtools/openapi-postprocess.mjs
Comment on lines
+412
to
+421
| "requestBody": { | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/admin.resetBudgetsRequest" | ||
| } | ||
| } | ||
| }, | ||
| "description": "Reset confirmation", | ||
| "required": true |
Contributor
There was a problem hiding this comment.
Tighten the reset-all schema to require the confirmation token.
admin.resetBudgetsRequest currently allows {} and arbitrary strings here, but the handler contract is stricter: TestResetBudgetsEndpoint shows the endpoint returns 400 unless the caller sends the reset confirmation token. Please encode that in the schema as well, otherwise generated clients will advertise requests the server rejects.
🤖 Prompt for 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.
In `@docs/openapi.json` around lines 412 - 421, Update the components schema
admin.resetBudgetsRequest so it requires the reset confirmation token (the exact
property name used by the handler / TestResetBudgetsEndpoint) instead of
allowing an empty object or arbitrary string: add a properties block with the
confirmation field (string), include that field in the required array, and set
additionalProperties to false so generated clients cannot send {} or other
shapes the server will reject.
Comment on lines
+185
to
+193
| ensureRequiredProperty("admin.upsertBudgetRequest", "amount"); | ||
| ensureRequiredProperty("admin.upsertBudgetRequest", "budget_key"); | ||
| ensureRequiredProperty("admin.deleteBudgetRequest", "budget_key"); | ||
| ensureRequiredProperty("admin.upsertModelOverrideRequest", "selector"); | ||
| ensureRequiredProperty("admin.deleteModelOverrideRequest", "selector"); | ||
| ensureRequiredProperty("admin.upsertModelPricingOverrideRequest", "selector"); | ||
| ensureRequiredProperty("admin.upsertModelPricingOverrideRequest", "pricing"); | ||
| ensureRequiredProperty("admin.deleteModelPricingOverrideRequest", "selector"); | ||
| applyBudgetKeySchemaConstraints(); |
Contributor
There was a problem hiding this comment.
user_path is still optional in budget schemas, but runtime requires it.
The post-process step enforces budget_key (and amount for upsert), but not user_path. Handlers validate user_path, so generated clients can produce requests that always fail with 400.
💡 Proposed fix
ensureRequiredProperty("admin.upsertBudgetRequest", "amount");
ensureRequiredProperty("admin.upsertBudgetRequest", "budget_key");
+ensureRequiredProperty("admin.upsertBudgetRequest", "user_path");
ensureRequiredProperty("admin.deleteBudgetRequest", "budget_key");
+ensureRequiredProperty("admin.deleteBudgetRequest", "user_path");
ensureRequiredProperty("admin.upsertModelOverrideRequest", "selector");
ensureRequiredProperty("admin.deleteModelOverrideRequest", "selector");
ensureRequiredProperty("admin.upsertModelPricingOverrideRequest", "selector");🤖 Prompt for 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.
In `@tools/openapi-postprocess.mjs` around lines 185 - 193, The budget schemas are
missing a required "user_path" property causing client requests to be rejected
at runtime; update the postprocess to call
ensureRequiredProperty("admin.upsertBudgetRequest", "user_path") and
ensureRequiredProperty("admin.deleteBudgetRequest", "user_path") (in the same
block alongside the existing ensureRequiredProperty calls and before
applyBudgetKeySchemaConstraints()) so the generated OpenAPI schemas mark
user_path as required for upsert and delete handlers.
Comment on lines
+357
to
361
| func budgetRequestKey(rawUserPath string, key *budgetKeyRequest) (string, int64, error) { | ||
| userPath, err := budget.NormalizeUserPath(rawUserPath) | ||
| if err != nil { | ||
| return "", 0, fmt.Errorf("invalid user_path path parameter: %w", err) | ||
| return "", 0, err | ||
| } |
There was a problem hiding this comment.
user_path guard — when the caller omits or sends an empty user_path, budget.NormalizeUserPath("") returns "/" without an error, so both UpsertBudget and DeleteBudget will silently operate on the global root budget instead of returning 400. Every other refactored handler (aliases, guardrails, model-overrides) rejects an empty identifier explicitly. The same issue exists in ResetBudget at line 181 where budget.NormalizeUserPath(req.UserPath) is called directly with no prior emptiness check.
Suggested change
| func budgetRequestKey(rawUserPath string, key *budgetKeyRequest) (string, int64, error) { | |
| userPath, err := budget.NormalizeUserPath(rawUserPath) | |
| if err != nil { | |
| return "", 0, fmt.Errorf("invalid user_path path parameter: %w", err) | |
| return "", 0, err | |
| } | |
| func budgetRequestKey(rawUserPath string, key *budgetKeyRequest) (string, int64, error) { | |
| if strings.TrimSpace(rawUserPath) == "" { | |
| return "", 0, errors.New("user_path is required") | |
| } | |
| userPath, err := budget.NormalizeUserPath(rawUserPath) | |
| if err != nil { | |
| return "", 0, err | |
| } |
Contributor
Author
There was a problem hiding this comment.
It's intentional
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.
Summary
Tests
make test-race, dashboard JavaScript unit tests, hot-path performance guard,make lint,mint validatego test ./internal/admin ./internal/server ./cmd/gomodelgo test -tags e2e ./tests/e2e -run TestBudgetAdminEndpointsSQLite_E2Ego test -tags integration ./tests/integration -run '^$'node --test internal/admin/dashboard/static/js/modules/aliases.test.cjs internal/admin/dashboard/static/js/modules/budgets.test.cjs internal/admin/dashboard/static/js/modules/guardrails.test.cjs internal/admin/dashboard/static/js/modules/model-pricing-overrides.test.cjsnode --test internal/admin/dashboard/static/js/modules/dashboard-layout.test.cjs internal/admin/dashboard/static/js/modules/timezone-layout.test.cjsSummary by CodeRabbit
Refactor
Documentation
Tests