Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces a new "GET Method Guard" functionality into the project. It updates documentation by adding a sidebar entry and new pages explaining the plugin, along with enhancements to related security topics. In the server package, a new plugin is implemented to restrict GET requests based on procedure configuration and is accompanied by tests to validate its behavior. Additionally, exports are updated to include the new plugin, ensuring it is accessible from the module index. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant GETGuard as GetMethodGuardPlugin
Client->>Server: Send HTTP Request (GET/POST)
Server->>GETGuard: Invoke interceptors
alt GET request for non-GET endpoint
GETGuard-->>Server: Reject with error (405)
Server-->>Client: Respond 405 Method Not Allowed
else GET request for GET endpoint
GETGuard-->>Server: Validate and allow request
Server-->>Client: Respond 200 OK
else Non-GET request
GETGuard-->>Server: Bypass GET check
Server-->>Client: Respond 200 OK
end
Possibly related PRs
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
More templates
@orpc/arktype
@orpc/client
@orpc/contract
@orpc/openapi
@orpc/openapi-client
@orpc/react
@orpc/react-query
@orpc/shared
@orpc/server
@orpc/solid-query
@orpc/standard-server
@orpc/standard-server-fetch
@orpc/standard-server-node
@orpc/svelte-query
@orpc/valibot
@orpc/vue-colada
@orpc/vue-query
@orpc/zod
commit: |
![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
🧹 Nitpick comments (6)
apps/content/docs/plugins/get-method-guard.md (2)
18-24: Consider adding explicit import forosThe code example correctly demonstrates how to mark a procedure to accept GET requests, but it doesn't show where
oscomes from. Consider adding an explicit import statement to make the example more complete.-import { os } from '@orpc/server' +// Import the route builder +import { os } from '@orpc/server'
26-39: Add documentation about the customizable error optionThe setup example is clear and demonstrates how to integrate the plugin. However, the documentation doesn't mention that the error thrown can be customized, which is a feature provided by the implementation.
Consider adding a section about customizing the error:
## Customizing the Error You can customize the error thrown when a GET request is made to a procedure that doesn't allow it: ```ts import { GetMethodGuardPlugin } from '@orpc/server/plugins' import { ORPCError } from '@orpc/contract' const handler = new RPCHandler(router, { plugins: [ new GetMethodGuardPlugin({ error: new ORPCError('FORBIDDEN', 'GET method is not allowed for this procedure') }) ], })packages/server/src/plugins/get-method-guard.test.ts (1)
1-58: Consider adding a test for custom error optionsThe tests cover the basic functionality well but don't test the ability to customize the error via the options passed to the plugin constructor. Consider adding a test case that verifies custom error options work correctly.
it('should use custom error when provided', async () => { const customErrorHandler = new RPCHandler({ ping: os.handler(() => 'pong'), }, { plugins: [ new GetMethodGuardPlugin({ error: new ORPCError('FORBIDDEN', 'Custom error message') }), ], }) const response = await customErrorHandler.handle( new Request('http://localhost/ping?data=%7B%7D') ) expect(response).toEqual({ matched: true, response: expect.objectContaining({ status: 403, // FORBIDDEN status }) }) const responseBody = await response.response.json() expect(responseBody.error.message).toBe('Custom error message') })packages/server/src/plugins/get-method-guard.ts (3)
20-20: Consider documenting the high order valueThe order value is set very high (7,000,000) without explanation. While this likely ensures the plugin runs after most others, it would be helpful to document why this specific value was chosen.
- order = 7_000_000 + /** + * High order value ensures this plugin runs after most others, + * allowing other plugins to modify the request/context first. + */ + order = 7_000_000
43-45: Consider consistent error type usageThe plugin uses
TypeErrorfor context corruption butORPCErrorfor method violations. Consider usingORPCErrorconsistently for both cases to provide a uniform error handling experience.- throw new TypeError('[GetMethodGuardPlugin] GET method guard context has been corrupted or modified by another plugin or interceptor') + throw new ORPCError('INTERNAL_SERVER_ERROR', '[GetMethodGuardPlugin] GET method guard context has been corrupted or modified by another plugin or interceptor')
26-29: Add array type check before using unshiftThe code initializes
rootInterceptorsandclientInterceptorsif they're undefined, but doesn't verify they're arrays before callingunshift. While standard handler options should ensure they're arrays, a defensive check would make the code more robust.options.rootInterceptors ??= [] options.clientInterceptors ??= [] + + if (!Array.isArray(options.rootInterceptors) || !Array.isArray(options.clientInterceptors)) { + throw new TypeError('[GetMethodGuardPlugin] rootInterceptors and clientInterceptors must be arrays') + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/content/.vitepress/config.ts(1 hunks)apps/content/docs/advanced/rpc-protocol.md(1 hunks)apps/content/docs/plugins/get-method-guard.md(1 hunks)apps/content/docs/plugins/simple-csrf-protection.md(1 hunks)packages/server/src/plugins/get-method-guard.test.ts(1 hunks)packages/server/src/plugins/get-method-guard.ts(1 hunks)packages/server/src/plugins/index.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/server/src/plugins/get-method-guard.test.ts (2)
packages/server/src/builder.ts (1)
os(245-261)packages/server/src/plugins/get-method-guard.ts (1)
GetMethodGuardPlugin(17-56)
packages/server/src/plugins/get-method-guard.ts (4)
packages/server/src/context.ts (1)
Context(1-1)packages/server/src/adapters/standard/plugin.ts (1)
StandardHandlerPlugin(4-7)packages/server/src/adapters/standard/handler.ts (1)
StandardHandlerOptions(25-47)packages/contract/src/config.ts (1)
fallbackContractConfig(20-26)
🪛 GitHub Check: codecov/patch
packages/server/src/plugins/index.ts
[warning] 3-3: packages/server/src/plugins/index.ts#L3
Added line #L3 was not covered by tests
🔇 Additional comments (18)
packages/server/src/plugins/index.ts (1)
3-3: LGTM: New export added correctly.The export for the new GET Method Guard plugin has been properly added to the plugins index file, maintaining the alphabetical ordering of exports.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 3-3: packages/server/src/plugins/index.ts#L3
Added line #L3 was not covered by testsapps/content/docs/advanced/rpc-protocol.md (1)
32-34: Well-placed security information.This info block provides valuable security guidance in the appropriate context. The CSRF prevention link to MDN documentation and reference to the GET Method Guard plugin enhance the security consciousness of developers using the RPC protocol.
apps/content/.vitepress/config.ts (1)
113-113: LGTM: Sidebar navigation entry added correctly.The navigation entry for the GET Method Guard plugin is properly positioned in the Plugins section, following the existing pattern of other plugin entries.
apps/content/docs/plugins/simple-csrf-protection.md (3)
3-3: Improved description with clearer security context.The enhanced description provides better context about the plugin's purpose in preventing CSRF attacks.
6-6: Title updated for consistency.Adding "Plugin" to the title maintains consistency with other plugin documentation.
8-8: Enhanced introduction with educational link.The expanded introduction paragraph provides a clearer explanation of CSRF protection with a helpful link to MDN documentation.
apps/content/docs/plugins/get-method-guard.md (4)
1-4: Good choice of title and meta description!The title and description clearly communicate the purpose of the plugin. The description effectively summarizes the security benefits and the specific problem it addresses (CSRF risks).
6-8: Introduction establishes the security context wellThe introduction effectively explains the plugin's purpose and links to relevant documentation about RPC Protocol and CSRF prevention, which helps developers understand the security context.
10-12: "When to Use" section provides clear guidanceThis section properly explains the specific scenarios where the plugin is beneficial, focusing on applications that store sensitive data in cookies with certain SameSite settings.
14-16: "How it works" section is straightforward and preciseThe explanation of the plugin's behavior is concise and clear, making it easy for developers to understand the core functionality.
packages/server/src/plugins/get-method-guard.test.ts (4)
1-16: Test setup is comprehensiveThe test setup creates a handler with:
- A procedure not marked as GET-allowed ('ping')
- A procedure explicitly marked as GET-allowed ('pong')
- The GetMethodGuardPlugin properly configured
- A test interceptor
This covers the necessary scenarios for testing the plugin's functionality.
18-32: Tests correctly verify GET method restrictionsThe test properly verifies:
- GET requests to non-GET procedures return 405
- POST requests to the same procedure return 200
This confirms the core functionality of the plugin.
34-38: Test confirms GET method is allowed for marked proceduresThis test properly verifies that a GET request to a procedure explicitly marked as GET returns a 200 OK response, confirming the plugin allows proper GET requests.
40-57: Test for context corruption is thoroughThe test correctly verifies that when an interceptor modifies the context in a way that removes the plugin's context symbol, the handler responds with a 500 error, ensuring robustness against potential interceptor conflicts.
packages/server/src/plugins/get-method-guard.ts (4)
15-15: Symbol naming is clear and follows conventionsThe symbol name clearly indicates its purpose and follows standard naming conventions with uppercase and underscores, making it easy to identify in debugging contexts.
17-24: Class implementation follows good practicesThe class correctly:
- Implements the StandardHandlerPlugin interface
- Uses generics to maintain type safety
- Provides a default error while allowing customization
- Has a high order value to ensure it runs after other plugins
No issues with the implementation.
26-40: Root interceptor implementation is robustThe root interceptor correctly:
- Checks if the request method is GET
- Preserves the original context while adding the method information
- Uses a symbol to avoid name collisions
This implementation ensures the context is properly augmented without losing existing information.
42-54: Client interceptor implementation has good error handlingThe client interceptor:
- Verifies the context hasn't been corrupted
- Retrieves the procedure method with a fallback mechanism
- Throws the appropriate error when GET requests are made to non-GET procedures
- Only throws errors when necessary (doesn't affect other methods)
The implementation is secure and follows good practices.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/server/src/plugins/get-method-guard.ts (1)
5-13: Consider simplifying theerrorproperty type.You could directly type
errorasORPCErrorrather thanInstanceType<typeof ORPCError>to avoid any potential confusion.export interface GetMethodGuardPluginOptions { - error?: InstanceType<typeof ORPCError> + error?: ORPCError }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/server/src/plugins/get-method-guard.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/server/src/plugins/get-method-guard.ts (4)
packages/server/src/context.ts (1)
Context(1-1)packages/server/src/adapters/standard/plugin.ts (1)
StandardHandlerPlugin(4-7)packages/server/src/adapters/standard/handler.ts (1)
StandardHandlerOptions(25-47)packages/contract/src/config.ts (1)
fallbackContractConfig(20-26)
🔇 Additional comments (4)
packages/server/src/plugins/get-method-guard.ts (4)
1-3: Module imports appear correct and consistent.The imports for types and utilities are well-structured. No further issues are noted here.
26-40: Valid approach to setting GET context in the handler.Unshifting an interceptor that injects the
isGetMethodflag into the context is a clean way to handle method checks early in the pipeline. This looks good.
42-55: Logic for enforcing GET restrictions is well-structured.The second interceptor correctly checks for a GET request and ensures the procedure is allowed to use GET. Throwing the configured
erroris appropriate here.
20-20:Details
✅ Verification successful
Confirm plugin execution priority.
The order is set to a large value (
7_000_000). Make sure it is intentional so that this plugin runs at the desired point in the pipeline.Please verify that the plugin’s priority aligns with your intended execution order. If it’s misplaced, you may need to adjust this value.
🏁 Script executed:
#!/bin/bash # Search for other plugin `order` properties to ensure the chosen priority does not conflict. rg --multiline 'order\s*=\s*\d+'Length of output: 420
Plugin execution priority verified
The
get-method-guard.tsplugin's order is set to7_000_000, which correctly positions it after the batch plugins (order5_000_000) and before the CSRF and CORS plugins (orders8_000_000and9_000_000respectively). This ordering appears intentional and aligns with the desired execution sequence for your pipeline. Please confirm once more that this sequence is aligned with the overall plugin dependency and execution strategy.
1 participant
Summary by CodeRabbit