feat: support SSRF check on useHttpClientNext = true #96
Conversation
WalkthroughThe project update involves enhancing configurations and functionalities. Node.js version specifications in workflows are updated, and a new workflow for publishing commits is introduced. The README files are revised to provide better license and contributor information. In core application files, several functions related to safe HTTP requests are renamed and restructured. Furthermore, Node.js version requirements are tightened, and the SSRF protection mechanism is refined. Finally, additional security configurations and test cases are added to improve the application robustness. Changes
Poem
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
Removed dependencies detected. Learn more about Socket for GitHub ↗︎ 🚮 Removed packages: npm/git-contributor@2.1.5 |
|
commit: egg-security
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #96 +/- ##
==========================================
+ Coverage 97.23% 97.25% +0.02%
==========================================
Files 32 32
Lines 1338 1349 +11
Branches 337 340 +3
==========================================
+ Hits 1301 1312 +11
Misses 37 37 ☔ View full report in Codecov by Sentry. |
![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
Outside diff range and nitpick comments (17)
app/extend/context.js (2)
Line range hint
1-1: Remove the redundant 'use strict' directive.The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
- 'use strict';
Line range hint
116-116: Use optional chaining.Change to an optional chain for better readability and safety.
- const token = findToken(this.query, queryName) || findToken(this.request.body, bodyName) || - (headerName && this.get(headerName)); + const token = findToken(this.query, queryName) || findToken(this.request.body, bodyName) || + this.get?.(headerName);README.zh-CN.md (2)
Line range hint
92-92: Avoid using emphasis instead of a heading.Use proper headings instead of emphasis for better readability.
- __使用__ + ### 使用Also applies to: 118-118, 124-124, 331-331
Line range hint
257-257: Use underscores for strong style.Use underscores instead of asterisks for strong style.
- **注意** + __注意__README.md (13)
Line range hint
99-99: Fix grammar: Use base form of the verbThe modal verb ‘will’ requires the verb’s base form. Change ‘will overrides’ to ‘will override’.
- the current request configuration will overrides the default configuration + the current request configuration will override the default configuration
Line range hint
140-140: Fix grammar: Use base form of the verbThe modal verb ‘will’ requires the verb’s base form. Change ‘will overrides’ to ‘will override’.
- the current request configuration will overrides the default configuration + the current request configuration will override the default configuration
Line range hint
146-146: Simplify phrase: Use ‘whether’The phrase ‘whether or not’ can be shortened to ‘whether’.
- Whether or not the domain is in the whitelist of the configuration. + Whether the domain is in the whitelist of the configuration.
Line range hint
149-149: Simplify phrase: Use ‘whether’The phrase ‘whether or not’ can be shortened to ‘whether’.
- whether or not send back an `Access-Control-Allow-Origin` response header + whether to send back an `Access-Control-Allow-Origin` response header
Line range hint
248-248: Fix misspelling: Use hyphenated formThe word ‘case insensitive’ should be hyphenated to ‘case-insensitive’.
- `domainWhiteList` and `url` are case insensitive. + `domainWhiteList` and `url` are case-insensitive.
Line range hint
347-347: Fix misspelling: Use ‘affect’The word ‘effect’ should be ‘affect’ as it means to have an effect upon.
- it will effect server performance + it will affect server performance
Line range hint
381-381: Fix typographical error: Add commaA comma is usually used after the phrase ‘for example’.
- For example `html` tag is not in the whitelist. + For example, `html` tag is not in the whitelist.
Line range hint
424-424: Fix grammar: Use base form of the verbThe modal verb ‘will’ requires the verb’s base form. Change ‘will lost’ to ‘will lose’.
- it will lost performance + it will lose performance
Line range hint
436-436: Fix grammar: Use third-person verb formThe verb form should match the singular noun ‘user’. Change ‘user submit’ to ‘user submits’.
- when user submit the implementation + when user submits the implementation
Line range hint
503-503: Simplify phrase: Remove ‘by mistake’The phrase ‘by mistake’ might be wordy. Consider removing it.
- take `text/plain` as `text/html` by mistake and render it + take `text/plain` as `text/html` and render it
Line range hint
161-161: Fix markdownlint issue: Use heading instead of emphasisEmphasis is used instead of a heading. Change the line to use a heading.
- __mention：`match` has higher priority than `ignore`__ + ### Mention + `match` has higher priority than `ignore`
Line range hint
424-424: Fix markdownlint issue: Use heading instead of emphasisEmphasis is used instead of a heading. Change the line to use a heading.
- __it has a very complex process and will lost performance, so avoid the use as far as possible__ + ### Note + It has a very complex process and will lose performance, so avoid the use as far as possible.
Line range hint
286-286: Fix markdownlint issue: Use underscores for strong styleStrong style should use underscores instead of asterisks.
- **Mention: Particular attention, if you need to resolve URL use `surl`，`surl` need warpped in quotes, Otherwise will lead to XSS vulnerability.** + __Mention: Particular attention, if you need to resolve URL use `surl`，`surl` need warpped in quotes, Otherwise will lead to XSS vulnerability.__
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- .github/workflows/nodejs.yml (1 hunks)
- .github/workflows/pkg.pr.new.yml (1 hunks)
- README.md (1 hunks)
- README.zh-CN.md (1 hunks)
- app/extend/agent.js (1 hunks)
- app/extend/application.js (2 hunks)
- app/extend/context.js (2 hunks)
- lib/extend/safe_curl.js (1 hunks)
- package.json (2 hunks)
- test/fixtures/apps/ssrf-check-address-useHttpClientNext/config/config.default.js (1 hunks)
- test/fixtures/apps/ssrf-check-address-useHttpClientNext/package.json (1 hunks)
- test/ssrf.test.js (2 hunks)
Files skipped from review due to trivial changes (3)
- .github/workflows/nodejs.yml
- .github/workflows/pkg.pr.new.yml
- test/fixtures/apps/ssrf-check-address-useHttpClientNext/package.json
Additional context used
Biome
lib/extend/safe_curl.js
[error] 10-10: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
app/extend/context.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
[error] 116-116: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Markdownlint
README.zh-CN.md
92-92: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
118-118: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
124-124: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
331-331: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
257-257: Expected: underscore; Actual: asterisk
Strong style(MD050, strong-style)
257-257: Expected: underscore; Actual: asterisk
Strong style(MD050, strong-style)
README.md
161-161: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
424-424: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
286-286: Expected: underscore; Actual: asterisk
Strong style(MD050, strong-style)
286-286: Expected: underscore; Actual: asterisk
Strong style(MD050, strong-style)
347-347: Expected: underscore; Actual: asterisk
Strong style(MD050, strong-style)
347-347: Expected: underscore; Actual: asterisk
Strong style(MD050, strong-style)
LanguageTool
README.md
[style] ~56-~56: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ... }, }; ``` ### match & ignore If you want to set security config open for a certain ...(REP_WANT_TO_VB)
[style] ~74-~74: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ... //... }, }, }; ``` If you want to set security config disable for a certa...(REP_WANT_TO_VB)
[grammar] ~99-~99: The modal verb ‘will’ requires the verb’s base form.
Context: ... the current request configuration will overrides the default configuration (new configur...(MD_BASEFORM)
[grammar] ~140-~140: The modal verb ‘will’ requires the verb’s base form.
Context: ... the current request configuration will overrides the default configuration, but it does ...(MD_BASEFORM)
[style] ~146-~146: Consider shortening this phrase to just ‘whether’, unless you mean ‘regardless of whether’.
Context: ... ## API ### ctx.isSafeDomain(domain) Whether or not the domain is in the whitelist of the c...(WHETHER)
[style] ~149-~149: Consider shortening this phrase to just ‘whether’, unless you mean ‘regardless of whether’.
Context: ...s this function internally to determine whether or not send back an `Access-Control-Allow-Orig...(WHETHER)
[misspelling] ~248-~248: This word is normally spelled with a hyphen.
Context: ...(url).domainWhiteListandurl` are case insensitive. ### jsonp Based on [jsonp-body](http...(EN_COMPOUNDS_CASE_INSENSITIVE)
[misspelling] ~347-~347: Did you mean “affect” (have an effect upon)?
Context: ...html is a very complex process, it will effect server performance, so if you do not ne...(AFFECT_EFFECT)
[typographical] ~381-~381: After the expression ‘for example’ a comma is usually used.
Context: .../lib/default.js) will be filtered. For examplehtmltag is not in the whitelist. ``...(COMMA_FOR_EXAMPLE)
[grammar] ~424-~424: The modal verb ‘will’ requires the verb’s base form.
Context: ..._it has a very complex process and will lost performance, so avoid the use as far as...(MD_BASEFORM)
[grammar] ~436-~436: Consider using third-person verb forms for singular and mass nouns.
Context: ...mand execution vulnerability, when user submit the implementation of the command by br...(MASS_AGREEMENT)
[style] ~503-~503: ‘by mistake’ might be wordy. Consider a shorter alternative.
Context: .... E.g: taketext/plainastext/htmlby mistake and render it, especially when there's ...(EN_WORDINESS_PREMIUM_BY_MISTAKE)
Additional comments not posted (12)
app/extend/agent.js (1)
1-4: LGTM! The function renaming aligns with the PR objectives.The renaming of
safeCurltosafeCurlForApplicationis consistent with the new SSRF protection logic.test/fixtures/apps/ssrf-check-address-useHttpClientNext/config/config.default.js (1)
1-16: LGTM! The SSRF protection configurations are well-defined.The IP blacklist and custom
checkAddressfunction are appropriate for SSRF protection. EnablinguseHttpClientNextaligns with the PR objectives.lib/extend/safe_curl.js (1)
9-29: LGTM! The SSRF protection logic is well-implemented.The
safeCurlForApplicationfunction effectively integrates SSRF protection. ThesafeCurlForContextfunction correctly delegates tosafeCurlForApplication.Tools
Biome
[error] 10-10: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
app/extend/application.js (1)
Line range hint
1-34: LGTM! The function renaming aligns with the PR objectives.The renaming of
safeCurltosafeCurlForApplicationis consistent with the new SSRF protection logic.package.json (1)
36-36: Dependency update approved.The
eggdependency has been updated to^3.26.0.However, ensure that this update does not introduce any compatibility issues.
test/ssrf.test.js (1)
91-110: New test case for SSRF protection approved.The new test case for
checkAddresswithuseHttpClientNext = trueensures that the SSRF protection works correctly with the new HTTP client.However, ensure that this test case covers all necessary scenarios.
Verification successful
New test case for SSRF protection approved.
The new test case for
checkAddresswithuseHttpClientNext = trueensures that the SSRF protection works correctly with the new HTTP client. ThecheckIllegalAddressErrorfunction includes the necessary assertions to validate the SSRF protection.
- The assertions within
checkIllegalAddressErrorconfirm that the SSRF protection is functioning as expected.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new test case covers all necessary scenarios. # Test: Run the tests to ensure everything works correctly. npm testLength of output: 81
Script:
#!/bin/bash # Search for test cases related to `checkAddress` with `useHttpClientNext = true` # and list the assertions within those test cases. # Find the test file test_file=$(fd ssrf.test.js) # Print the test cases related to `checkAddress` with `useHttpClientNext = true` rg -A 10 "describe\('checkAddress with useHttpClientNext = true'" $test_file # List the assertions within those test cases rg "assert" $test_fileLength of output: 1113
Script:
#!/bin/bash # List the lines within the `checkAddress with useHttpClientNext = true` block to verify the presence of assertions. # Find the test file test_file=$(fd ssrf.test.js) # Print the lines within the `checkAddress with useHttpClientNext = true` block rg -A 50 "describe\('checkAddress with useHttpClientNext = true'" $test_fileLength of output: 1849
Script:
#!/bin/bash # Search for the definition of the `checkIllegalAddressError` function and inspect its contents. # Find the definition of the `checkIllegalAddressError` function rg "function checkIllegalAddressError" -A 20Length of output: 475
app/extend/context.js (1)
231-231: LGTM!The
safeCurlfunction has been correctly assigned tosafeCurlForContext.README.zh-CN.md (2)
429-431: Update approved.The license section has been updated to include a link to the MIT license.
433-437: Update approved.The contributors section has been updated to include a link to contributor statistics.
README.md (3)
556-556: LGTM: License section updateThe change to include a link to the MIT license on GitHub enhances the documentation.
560-560: LGTM: Contributors section updateThe change to include a link to view contributors visually using
contributors-imgenhances the documentation.
562-562: LGTM: Contributors section additionThe new line indicating that the contributors section is made with
contributors-imgprovides additional context.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- .github/workflows/nodejs.yml (1 hunks)
- package.json (2 hunks)
Files skipped from review due to trivial changes (1)
- .github/workflows/nodejs.yml
Additional comments not posted (3)
package.json (3)
53-53: LGTM! Theciscript update aligns with best practices.The update ensures that linting and coverage checks are performed in the CI pipeline.
39-39: Verify compatibility witheggversion ^3.26.0.Ensure that the codebase is compatible with the updated
eggversion and run the tests to confirm this.
5-5: Verify compatibility with Node.js >=14.20.0.Ensure that all dependencies and the codebase are compatible with the updated minimum Node.js version.
[skip ci] ## [3.4.0](v3.3.1...v3.4.0) (2024-07-01) ### Features * support SSRF check on useHttpClientNext = true ([#96](#96)) ([1d6bfff](1d6bfff))
Summary by CodeRabbit
New Features
config.default.js.Bug Fixes
>=14.20.0in workflow andpackage.json.Documentation
README.mdandREADME.zh-CN.md.Refactor
safeCurlfunction tosafeCurlForApplicationandsafeCurlForContext.Tests
ssrf.test.js.Chores
eggdependency to version^3.26.0.git-contributordependency.