improvement(tools): prevent internal request self-hops - #7190
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThis PR prevents integration tools from routing execution back through Sim’s own HTTP API and moves internal work behind registered in-process operations.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/tools/index.ts | Adds the authoritative runtime self-origin guard, redirect checks, and loopback-alias matching; the previously reported alias bypass is fixed. |
| scripts/check-tool-request-boundary.ts | Expands the static audit to resolve indirect request policies and URL expressions while failing closed on unresolved prohibited request forms. |
| apps/sim/tools/types.ts | Defines the mutually exclusive external-request and registered internal-operation tool boundaries. |
| apps/sim/lib/internal/principals/executor.ts | Centralizes executor-principal binding and supports deriving operation delegations from trusted execution deadlines. |
| apps/sim/lib/function-execution/execute-request.ts | Replaces internal HTTP-oriented execution handoffs with scoped in-process execution and workspace-file delegated principals. |
| apps/sim/lib/core/security/input-validation.server.ts | Supports the runtime network boundary with shared URL and loopback validation behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Tool[Tool invocation] --> Boundary{Execution boundary}
Boundary -->|InternalToolConfig.operation| Registry[Internal operation registry]
Registry --> Handler[In-process authorized handler]
Boundary -->|External ToolConfig.request| Guard[Self-origin and SSRF guards]
Guard -->|External origin| Provider[External provider]
Guard -->|Sim origin| Reject[Reject self-hop]
Reviews (12): Last reviewed commit: "fix(tools): block self-hosted loopback a..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
@cubic-dev-ai review this PR |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 65 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
@cubic-dev-ai review this PR |
|
@greptile-apps review this PR |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 72 files
Confidence score: 3/5
- In
apps/sim/lib/internal/file/parser.ts, actorless executions can no longer parse publicprofile-pictures,og-images, orworkspace-logosfiles becauseassertUserFileContentAccessrequiresuserIdfirst; preserve the legacy world-readable access path for these public files. - In
apps/sim/lib/internal/file/operations.test.ts, the mock and assertion target an authorization function thatoperations.tsno longer calls, leaving the in-processassertOperationFileAccessgate effectively unverified; update the test to mock and assert the current access check.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/lib/internal/file/parser.ts">
<violation number="1" location="apps/sim/lib/internal/file/parser.ts:128">
P2: Actorless executions can no longer parse public `profile-pictures`, `og-images`, or `workspace-logos` files. `assertUserFileContentAccess` requires `userId` before reaching the legacy world-readable check; let public reads short-circuit without a user identity.</violation>
</file>
<file name="apps/sim/lib/internal/file/operations.test.ts">
<violation number="1" location="apps/sim/lib/internal/file/operations.test.ts:815">
P2: The mock module and its assertion point at a function the code under test never calls. operations.ts no longer imports @/app/api/files/authorization — its file-access gate is the in-process assertOperationFileAccess -> assertUserFileContentAccess from materialization.server — so expect(mockVerifyFileAccess).not.toHaveBeenCalled() is trivially true and provides no self-hop regression coverage (the PR's stated purpose). Either drop the dead @/app/api/files/authorization mock and its assertion, or assert against the actual in-process gate that materialization.server now uses; without that the test gives false assurance that the actorless path skips authorization.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
@greptile-apps review this PR |
|
@cubic-dev-ai review this PR |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 73 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
@cubic-dev-ai review this PR |
|
@greptile-apps review this PR |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 76 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
You’re at about 94% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Re-trigger cubic
|
@cubic-dev-ai review this PR |
|
@greptile-apps review this PR |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 79 files
You’re at about 96% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Re-trigger cubic
|
@cubic-dev-ai review this PR |
|
@greptile-apps review this PR |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 81 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
You’re at about 97% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Re-trigger cubic
|
@cubic-dev-ai review this PR |
|
@greptile-apps review this PR |
|
Greptile loopback-alias finding was valid and is fixed in f1d95a6. The runtime guard now treats loopback names/IPs as the same self-hosted origin only when protocol and effective port match, covering initial requests and every redirect hop. Different loopback ports remain valid local-provider targets, while the explicit http_request/webhook_request same-origin capability is unchanged. Regressions cover localhost, 127.0.0.1, another 127/8 alias, IPv6 ::1, redirects, different ports, and the generic HTTP exception. |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 81 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
You’re at about 98% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Re-trigger cubic
1 participant
Summary
Type of Change
Testing
bun run lintbun run type-checkbun run check:auditsChecklist