fix: allow legitimate instanceName input on instance create and fetch routes - #2735
Open
maonamassaaceleradora wants to merge 2 commits into
Conversation
The cross-instance auth bypass fix (7a55a2b) strips instanceName and instanceId from any untrusted body or query via sanitizeUntrustedInput(). Every other instance route carries ":instanceName" in its path, so the name is read from request.params and stripping the body copy is correct. But /instance/create has no path parameter — the name can only ever come from the body, so the sanitizer removed the one required field of the request. That surfaced first as a Prisma 500 ("Argument `name` is missing") and, after the name was defaulted, as a 400 "The instanceName cannot be empty". Creating an instance was impossible. Parameterize the protected field list so each call site declares what it needs, and pass ['instanceId'] on /instance/create. This does not reopen the bypass: instanceId is server-generated and stays protected, and every other route keeps the full default list.
…ances GET /instance/fetchInstances?instanceName=X is a lookup route: like /instance/create it has no ":instanceName" in its path, so its filter can only arrive in the query string. sanitizeUntrustedInput() stripped both instanceName and instanceId from the query, and did so silently from the caller's point of view — the request still returned 200, just with every instance on the server instead of the one that was asked for. A filtered lookup silently turning into a full listing is worse than an error. Allow both fields through for this route only; every other route keeps the default protected list.
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThe PR parameterizes untrusted-input sanitization to account for routes where instance identity legitimately comes from the request body or query string: instance creation accepts instanceName but still protects server-generated instanceId, while fetchInstances preserves query filters. The sanitizer’s default remains unchanged, so routes with :instanceName continue rejecting body/query identity overrides. Sequence diagram for route-specific instance input sanitizationsequenceDiagram
participant Client
participant RouterBroker
participant Sanitizer
participant InstanceService
Client->>RouterBroker: POST /instance/create with instanceName
RouterBroker->>Sanitizer: sanitizeUntrustedInput(body, ['instanceId'])
Sanitizer-->>RouterBroker: instanceName retained, instanceId rejected
RouterBroker->>InstanceService: Create instance with instanceName
InstanceService-->>Client: Created instance
Client->>RouterBroker: GET /instance/fetchInstances?instanceName=X
RouterBroker->>Sanitizer: sanitizeUntrustedInput(query, [])
Sanitizer-->>RouterBroker: instanceName filter retained
RouterBroker->>InstanceService: Fetch instances filtered by instanceName
InstanceService-->>Client: Matching instances
Client->>RouterBroker: Request route with :instanceName and query override
RouterBroker->>Sanitizer: sanitizeUntrustedInput(query)
Sanitizer-->>RouterBroker: instanceName and instanceId rejected
Flow diagram for protected instance fields by routeflowchart TD
A[Untrusted body or query input] --> B{Route has special input rules?}
B -->|POST /instance/create| C[sanitizeUntrustedInput with instanceId protected]
C --> D[Allow instanceName; reject instanceId]
B -->|GET /instance/fetchInstances| E[sanitizeUntrustedInput with no protected fields]
E --> F[Preserve instanceName or instanceId filters]
B -->|Other routes| G[sanitizeUntrustedInput with default protected fields]
G --> H[Reject instanceName and instanceId overrides]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've reviewed your changes and they look great!
Sourcery assessment
Needs a human reviewer. This changes which untrusted query and body fields can determine the target instance, so an incorrect protection decision could select or create the wrong instance and potentially expose or persist data under the wrong identity. Reverting stops future effects but does not automatically undo instances or records already created.
This branch has not been deployed
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
1 participant
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
The cross-instance auth bypass fix in
7a55a2bfaddedsanitizeUntrustedInput(), which stripsinstanceNameandinstanceIdfrom any untrusted body or query string. That is the right default for the vast majority of routes, which carry:instanceNamein their path and read the instance fromrequest.params— there, a body or query copy of those fields can only ever be an override attempt.Two instance routes have no path parameter, and for them the sanitizer removes legitimate, required input.
1.
POST /instance/create— creating an instance became impossibleThere is no
:instanceNamein this route's path, so the name can only come from the body. The sanitizer stripped it, leaving the create request with no name at all:500—Argument 'name' is missing400—The instanceName cannot be empty2.
GET /instance/fetchInstances?instanceName=X— filter silently ignoredSame root cause, but the failure mode is quieter and arguably worse. The filter was stripped from the query string, and the request still returned
200— just with every instance on the server rather than the one that was requested. There is no error for a caller to notice; a filtered lookup silently became a full listing.The fix
sanitizeUntrustedInput()takes the protected field list as a parameter, defaulting to the existingPROTECTED_INSTANCE_FIELDS, so each call site can declare what it actually needs:/instance/create→['instanceId']/instance/fetchInstances→[](query string only)This does not reopen the
7a55a2bfbypassWorth being explicit, since this PR relaxes a security fix:
/instance/createstill protectsinstanceId. That is the field that matters there — it is server-generated, and letting a caller choose it is what would allow colliding with or hijacking an existing instance.instanceNameon this route is not an override of a trusted path parameter; it is the sole source of the value, and it is validated and uniqueness-checked downstream as before./instance/fetchInstancesis a read-only lookup with no:instanceNameto override. The bypass7a55a2bfclosed was one where a body/query field overrode an instance identity already established by the path — which cannot happen on a route that has no such parameter.:instanceNamein its path had its protection changed. The default is untouched, so every route that was hardened by7a55a2bfstays hardened.🔗 Related Issue
No issue filed. Follow-up to
7a55a2bf; found on one of our production instances immediately after picking up that commit.🧪 Type of Change
🧪 Testing
Verified against one of our production instances:
POST /instance/create— before:500from Prisma /400 The instanceName cannot be empty. After: the instance is created with the requested name.GET /instance/fetchInstances?instanceName=X— before:200with every instance on the server. After:200with only the requested instance.GET /instance/fetchInstanceswith no filter still returns the full listing.instanceIdin the body of/instance/createis still rejected and still logs the "Ignoring attempt to override protected field" warning.:instanceNameto confirm body/query overrides are still stripped.npm run lint:checkandnpx tsc --noEmitboth pass with no errors.✅ Checklist
📝 Additional Notes
The two commits are split by route so they can be reviewed independently — the
/instance/createbreakage is the louder one, but thefetchInstancesfilter being dropped silently is the one more likely to have gone unnoticed in the wild.If you would rather see the allowed fields expressed as an explicit per-route map instead of the optional parameter used here, I'm happy to restructure it that way.
🤖 Generated with Claude Code
Summary by Sourcery
Restore legitimate instance creation and filtered lookup inputs without weakening protection for routes that establish instance identity through the path.
Bug Fixes:
Enhancements: