fix: add createHttpClient back to app instance #5383
Conversation
security plugin need it to create a new httpClient for SSRF /~https://github.com/eggjs/egg/blob/a612e806019402aa217a1562b5ad847a308e843b/lib/egg.js#L293 /~https://github.com/eggjs/security/blob/e3408408adec5f8d009d37f75126ed082481d0ac/lib/extend/safe_curl.js#L21
WalkthroughThis pull request introduces enhancements to TypeScript type definitions and configuration options within the Egg.js framework. Key changes involve extending the Changes
Possibly related issues
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms (13)
🔇 Additional comments (5)
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/app/extend/context.ts(1 hunks)src/app/extend/context.types.ts(0 hunks)src/app/extend/request.ts(1 hunks)src/app/extend/request.types.ts(0 hunks)src/app/extend/response.ts(1 hunks)src/app/extend/response.types.ts(0 hunks)src/lib/core/httpclient.ts(2 hunks)src/lib/egg.ts(3 hunks)src/lib/egg.types.ts(0 hunks)src/lib/types.ts(2 hunks)test/lib/core/httpclient.test.ts(1 hunks)
💤 Files with no reviewable changes (4)
- src/app/extend/request.types.ts
- src/app/extend/response.types.ts
- src/lib/egg.types.ts
- src/app/extend/context.types.ts
🔇 Additional comments (10)
src/app/extend/response.ts (1)
37-44: LGTM! Type declarations look good.The TypeScript declarations for
realStatusgetter/setter methods are correctly defined in the module augmentation.src/lib/core/httpclient.ts (2)
24-34: LGTM! Constructor changes enhance configurability.The addition of the
optionsparameter with proper merging of configuration allows for customized HTTP client instances, which is essential for the security plugin's SSRF protection.
5-5: LGTM! Type exports are properly defined.The
HttpClientOptionstype is correctly exported both in the imports and exports sections.Also applies to: 13-13
src/app/extend/request.ts (1)
267-276: LGTM! Request interface types are well-defined.The TypeScript declarations for the Request interface methods and properties are correctly defined in the module augmentation.
src/app/extend/context.ts (1)
308-322: LGTM! Context interface types are comprehensive.The TypeScript declarations properly include all necessary methods and properties, particularly the HTTP client-related ones needed for SSRF protection:
curlmethodhttpclientandhttpClientgetterssrc/lib/types.ts (2)
69-78: Well-structured interface definition with proper documentation.The
HttpClientConfiginterface is well-defined with clear property types and documentation. The deprecated flag foruseHttpClientNextis properly marked.
155-155: Clean refactoring of httpclient configuration.Good refactoring to use the new
HttpClientConfiginterface, improving type safety and maintainability.src/lib/egg.ts (3)
34-36: Clean type imports with proper organization.The HttpClient-related type imports are well-organized and properly scoped.
692-706: Comprehensive type declarations for EggCore interface.The type declarations in the
@eggjs/coremodule are well-structured and maintain type safety.
390-397: Implementation aligns with PR objectives.The
createHttpClientmethod is properly implemented to support SSRF protection by allowing custom HTTP client instances.Let's verify the SSRF protection integration:
✅ Verification successful
SSRF protection is properly integrated.
The codebase shows proper SSRF protection implementation through:
- Built-in security configuration interface with IP blacklisting and custom validation
- Integration with egg-security plugin
- Support for custom HttpClient options
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify SSRF protection usage in security plugin # Test: Search for createHttpClient usage in security plugin rg -A 5 'createHttpClient' | grep -C 5 'ssrf'Length of output: 43
Script:
#!/bin/bash # Find HttpClient class definition and implementation ast-grep --pattern 'class HttpClient { $$$ }' # Find HttpClientOptions interface ast-grep --pattern 'interface HttpClientOptions { $$$ }' # Search for security-related configurations rg -l "HttpClientOptions|HttpClient" | xargs rg -A 5 -B 5 "(security|protect|validate|whitelist|blacklist|allowlist|blocklist|host|domain|url)"Length of output: 83710
Script:
#!/bin/bash # Search for SSRF implementation and integration rg -A 5 -B 5 "ssrf" --type ts # Look for security plugin integration with HttpClient ast-grep --pattern 'class HttpClient { $$$ request($_, $_) { $$$ } $$$ }'Length of output: 1743
| describe('app.createHttpClient(options)', () => { | ||
| let app: MockApplication; | ||
| before(() => { | ||
| app = createApp('apps/httpclient-retry'); | ||
| return app.ready(); | ||
| }); | ||
| after(() => app.close()); | ||
|
|
||
| it('should work', async () => { | ||
| const client1 = app.createHttpClient(); | ||
| const client2 = app.createHttpClient(); | ||
| assert.notEqual(client1, client2); | ||
| const res = await client1.request(url, { | ||
| method: 'GET', | ||
| }); | ||
| assert.equal(res.status, 200); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for createHttpClient.
While the current tests verify basic functionality, consider adding tests for:
- Custom options handling
- Error scenarios
- SSRF protection functionality (main PR objective)
Example test cases:
it('should apply custom options', () => {
const client = app.createHttpClient({ timeout: 1000 });
assert.equal(client.options.timeout, 1000);
});
it('should handle SSRF protection', async () => {
const client = app.createHttpClient();
await assert.rejects(
() => client.request('http://internal-network'),
/SSRF protection/
);
});
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #5383 +/- ##
==========================================
- Coverage 95.25% 95.17% -0.08%
==========================================
Files 45 41 -4
Lines 3980 3915 -65
Branches 433 433
==========================================
- Hits 3791 3726 -65
Misses 189 189 ☔ View full report in Codecov by Sentry. |
[skip ci] ## [4.0.1](v4.0.0...v4.0.1) (2025-01-14) ### Bug Fixes * add createHttpClient back to app instance ([#5383](#5383)) ([e5a697e](e5a697e)), closes [/github.com/eggjs/egg/blob/a612e806019402aa217a1562b5ad847a308e843b/lib/egg.js#L293](/~https://github.com/eggjs//github.com/eggjs/egg/blob/a612e806019402aa217a1562b5ad847a308e843b/lib/egg.js/issues/L293) [/github.com/eggjs/security/blob/e3408408adec5f8d009d37f75126ed082481d0ac/lib/extend/safe_curl.js#L21](/~https://github.com/eggjs//github.com/eggjs/security/blob/e3408408adec5f8d009d37f75126ed082481d0ac/lib/extend/safe_curl.js/issues/L21)
security plugin need it to create a new httpClient for SSRF
egg/lib/egg.js
Line 293 in a612e80
Summary by CodeRabbit
New Features
createHttpClientmethod to create HTTP client instances with custom options.Improvements
Testing