Enable setting the port and allowPrivateAddress through environment vars #53
Conversation
|
Warning Rate limit exceeded@dahlia has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce a new property Changes
Possibly related PRs
Poem
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: 2
🧹 Outside diff range and nitpick comments (1)
src/index.tsx (1)
25-25: Document the conditional export behavior and consider type safety.The export behavior changes based on BEHIND_PROXY, which could lead to type-related issues in consuming code.
Consider adding JSDoc documentation and type definitions:
/** * @returns When BEHIND_PROXY is true, returns an object with: * - fetch: Proxy-aware fetch function * - port: Configured server port * @returns When BEHIND_PROXY is false, returns the Hono app instance */ export default BEHIND_PROXY ? { fetch: behindProxy(app.fetch.bind(app)), port: HOLLO_PORT } : app;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/federation/index.ts (1 hunks)
- src/index.tsx (1 hunks)
🧰 Additional context used
🪛 Biome
src/federation/index.ts
[error] 112-112: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
src/index.tsx
[error] 23-23: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
🔇 Additional comments (2)
src/index.tsx (2)
25-25: Verify the impact of conditional exports on TypeScript consumers.The conditional export pattern might affect type inference in consuming TypeScript code.
Let's check for potential type-related issues:
✅ Verification successful
No type-related issues found with conditional exports
The conditional export in
src/index.tsxis safe from a TypeScript perspective because:
- The TypeScript configuration is properly set up with strict type checking
- Both branches of the conditional return compatible types (either the full app or an object with a subset of app's properties)
- No direct type imports or dependencies on this module were found in the codebase
- The exported types are properly handled by TypeScript's type system which can deal with conditional types and unions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for type-related imports of this module # Expected: Find places where the exported type might need to be handled conditionally # Search for imports of this module rg -l "from ['\"].*src/index['\"]" # Search for type definitions related to this module fd -e d.ts -x cat {} \;Length of output: 425
Script:
#!/bin/bash # Let's check for type definitions and usage patterns more thoroughly # Check for type declarations or interfaces in the index file ast-grep --pattern 'type $_ = $_' ast-grep --pattern 'interface $_ $_' # Look for direct imports of the index file rg "from ['\"]\.\.?/index['\"]" -A 2 # Check if there are any TypeScript configuration files that might affect type inference fd tsconfig.json -x cat {}Length of output: 6459
Line range hint
1-25: Verify ALLOW_PRIVATE_ADDRESS implementation.The PR objectives mention ALLOW_PRIVATE_ADDRESS configuration, but it's not visible in this file.
Let's verify the implementation:
✅ Verification successful
ALLOW_PRIVATE_ADDRESS is properly implemented
The environment variable is correctly implemented in the federation configuration at
src/federation/index.ts. It's used to configure theallowPrivateAddressoption when creating the federation instance:export const federation = createFederation<void>({ kv, queue, allowPrivateAddress: process.env["ALLOW_PRIVATE_ADDRESS"] === "true" });🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify ALLOW_PRIVATE_ADDRESS implementation # Expected: Find where ALLOW_PRIVATE_ADDRESS is used # Search for ALLOW_PRIVATE_ADDRESS usage rg "ALLOW_PRIVATE_ADDRESS" # Search for federation configuration ast-grep --pattern 'federation($_, $_)'Length of output: 425
Script:
#!/bin/bash # Let's check the federation configuration in src/federation/index.ts cat src/federation/index.ts # Also check if there are any other files that might be using allowPrivateAddress rg "allowPrivateAddress"Length of output: 30932
🧰 Tools
🪛 Biome
[error] 23-23: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
src/index.tsx
Outdated
| @@ -20,5 +20,6 @@ app.get("/nodeinfo/2.0", (c) => c.redirect("/nodeinfo/2.1")); | |||
|
|
|||
| // biome-ignore lint/complexity/useLiteralKeys: tsc complains about this (TS4111) | |||
| const BEHIND_PROXY = process.env["BEHIND_PROXY"] === "true"; | |||
| const HOLLO_PORT = process.env["HOLLO_PORT"] ?? 3000; | |||
There was a problem hiding this comment.
Add type safety and validation for HOLLO_PORT.
The port value should be properly typed and validated to ensure it's a valid port number.
Apply this diff to add proper type conversion and validation:
-const HOLLO_PORT = process.env["HOLLO_PORT"] ?? 3000;
+const HOLLO_PORT = (() => {
+ const port = parseInt(process.env["HOLLO_PORT"] ?? "3000", 10);
+ if (isNaN(port) || port < 0 || port > 65535) {
+ throw new Error("HOLLO_PORT must be a valid port number between 0 and 65535");
+ }
+ return port;
+})();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const HOLLO_PORT = process.env["HOLLO_PORT"] ?? 3000; | |
| const HOLLO_PORT = (() => { | |
| const port = parseInt(process.env["HOLLO_PORT"] ?? "3000", 10); | |
| if (isNaN(port) || port < 0 || port > 65535) { | |
| throw new Error("HOLLO_PORT must be a valid port number between 0 and 65535"); | |
| } | |
| return port; | |
| })(); |
🧰 Tools
🪛 Biome
[error] 23-23: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
There was a problem hiding this comment.
Environment variables are untyped and all strings. It should be parsed into a number using Number.parseInt() function.
| const HOLLO_PORT = process.env["HOLLO_PORT"] ?? 3000; | |
| const HOLLO_PORT = Number.parseInt(process.env["HOLLO_PORT"] ?? "3000"); |
There was a problem hiding this comment.
By the way, as the existing configurations do not have HOLLO_ prefix, it should be just PORT or LISTEN_PORT for consistency.
There was a problem hiding this comment.
@dahlia, understood. I'll update the environment variable to PORT or LISTEN_PORT for consistency with the existing configurations.
(^・ω・^ )
Thanks for the clarification!
✏️ Learnings added
Learnt from: dahlia
PR: dahlia/hollo#53
File: src/index.tsx:23-23
Timestamp: 2024-10-27T15:51:16.533Z
Learning: In this codebase, environment variables do not have the `HOLLO_` prefix. Future environment variables should be named consistently without the `HOLLO_` prefix, e.g., use `PORT` or `LISTEN_PORT` instead of `HOLLO_PORT`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
@dahlia, thank you for the clarification. You're absolutely right that environment variables are untyped strings and should be parsed using Number.parseInt() when expecting a number. I've noted this information for future reviews.
(_/)
(•_•)💡
/ \
✏️ Learnings added
Learnt from: dahlia
PR: dahlia/hollo#53
File: src/index.tsx:23-23
Timestamp: 2024-10-27T15:51:24.473Z
Learning: In JavaScript, environment variables are untyped strings and should be parsed using `Number.parseInt()` when expecting numeric values.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
src/federation/index.ts
Outdated
| @@ -109,6 +109,7 @@ if (getRedisUrl() == null) { | |||
| export const federation = createFederation<void>({ | |||
| kv, | |||
| queue, | |||
| allowPrivateAddress: process.env["ALLOW_PRIVATE_ADDRESS"] === "true" | |||
There was a problem hiding this comment.
💡 Codebase verification
Document security implications and add environment variable configuration
The environment variable ALLOW_PRIVATE_ADDRESS is currently undocumented. Please:
- Add documentation for
ALLOW_PRIVATE_ADDRESSin appropriate configuration files (e.g.,.env.example,README.md). - Add the following security notice above the configuration:
// SECURITY: Enabling private addresses allows federation with internal network endpoints. // Only enable this in trusted environments where internal federation is required.
- Consider simplifying the computed expression to
process.env.ALLOW_PRIVATE_ADDRESS === "true".
🔗 Analysis chain
Consider security implications and improve code style.
While the implementation aligns with the PR objectives, consider the following improvements:
- The computed expression can be simplified.
- Adding private address support has security implications that should be documented.
Apply this diff to simplify the computed expression:
- allowPrivateAddress: process.env["ALLOW_PRIVATE_ADDRESS"] === "true"
+ allowPrivateAddress: process.env.ALLOW_PRIVATE_ADDRESS === "true"Consider adding a comment documenting the security implications of enabling private addresses, such as:
// SECURITY: Enabling private addresses allows federation with internal network endpoints.
// Only enable this in trusted environments where internal federation is required.Let's verify if this environment variable is documented:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if ALLOW_PRIVATE_ADDRESS is documented in README or similar files
fd -e md -e txt -e env.example | xargs rg "ALLOW_PRIVATE_ADDRESS"
Length of output: 65
Script:
#!/bin/bash
# Let's try a broader search for any documentation or configuration files
fd -t f | rg -l ".(md|txt|env|config|json|ya?ml)$" | xargs rg -i "private.*address|allow.*private"
# Also search for environment variable usage in the codebase
rg "ALLOW_PRIVATE_ADDRESS" -A 2 -B 2
Length of output: 396
🧰 Tools
🪛 Biome
[error] 112-112: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
There was a problem hiding this comment.
It's turned off to prevent SSRF attacks. Why do you need this option?
There was a problem hiding this comment.
@dahlia, thank you for clarifying that ALLOW_PRIVATE_ADDRESS is turned off by default to prevent SSRF attacks. I've noted this information for future reviews.
(•̀o•́)ง
✏️ Learnings added
Learnt from: dahlia
PR: dahlia/hollo#53
File: src/federation/index.ts:112-112
Timestamp: 2024-10-27T15:51:25.648Z
Learning: In the Hollo application, `ALLOW_PRIVATE_ADDRESS` is turned off by default to prevent SSRF attacks.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
It's turned off to prevent SSRF attacks. Why do you need this option?
To build https://data.funfedi.dev/, I run all the Fediverse applications in a local docker network. It uses ips starting with 178.3.x.x by default, I think. These fall into the private addresses. So I need a way to disable this.
I'm not sure if I know how to check for an IP range here. Is it fine if I just add the variable to .env.sample?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Okay, fair enough. Please add it to the .env.sample.
src/federation/index.ts
Outdated
| @@ -109,6 +109,7 @@ if (getRedisUrl() == null) { | |||
| export const federation = createFederation<void>({ | |||
| kv, | |||
| queue, | |||
| allowPrivateAddress: process.env["ALLOW_PRIVATE_ADDRESS"] === "true" | |||
There was a problem hiding this comment.
It's turned off to prevent SSRF attacks. Why do you need this option?
src/index.tsx
Outdated
| @@ -20,5 +20,6 @@ app.get("/nodeinfo/2.0", (c) => c.redirect("/nodeinfo/2.1")); | |||
|
|
|||
| // biome-ignore lint/complexity/useLiteralKeys: tsc complains about this (TS4111) | |||
| const BEHIND_PROXY = process.env["BEHIND_PROXY"] === "true"; | |||
| const HOLLO_PORT = process.env["HOLLO_PORT"] ?? 3000; | |||
There was a problem hiding this comment.
Environment variables are untyped and all strings. It should be parsed into a number using Number.parseInt() function.
| const HOLLO_PORT = process.env["HOLLO_PORT"] ?? 3000; | |
| const HOLLO_PORT = Number.parseInt(process.env["HOLLO_PORT"] ?? "3000"); |
src/index.tsx
Outdated
| @@ -20,5 +20,6 @@ app.get("/nodeinfo/2.0", (c) => c.redirect("/nodeinfo/2.1")); | |||
|
|
|||
| // biome-ignore lint/complexity/useLiteralKeys: tsc complains about this (TS4111) | |||
| const BEHIND_PROXY = process.env["BEHIND_PROXY"] === "true"; | |||
| const HOLLO_PORT = process.env["HOLLO_PORT"] ?? 3000; | |||
There was a problem hiding this comment.
By the way, as the existing configurations do not have HOLLO_ prefix, it should be just PORT or LISTEN_PORT for consistency.
|
An option like this does make sense, however, it should be a comma separated string of private IP addresses or ranges not to enforce checking on, not a simple Boolean value. e.g., https://docs.joinmastodon.org/admin/config/#allowed_private_addresses |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
src/federation/index.ts (1)
112-112: Simplify the computed expressionThe computed expression can be simplified by using dot notation instead of string literal notation for accessing environment variables.
- allowPrivateAddress: process.env["ALLOW_PRIVATE_ADDRESS"] === "true" + allowPrivateAddress: process.env.ALLOW_PRIVATE_ADDRESS === "true"🧰 Tools
🪛 Biome
[error] 112-112: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .env.sample (1 hunks)
- src/federation/index.ts (1 hunks)
- src/index.tsx (1 hunks)
🧰 Additional context used
📓 Learnings (2)
src/federation/index.ts (1)
Learnt from: dahlia PR: dahlia/hollo#53 File: src/federation/index.ts:112-112 Timestamp: 2024-10-27T15:51:25.872Z Learning: In the Hollo application, `ALLOW_PRIVATE_ADDRESS` is turned off by default to prevent SSRF attacks.src/index.tsx (1)
Learnt from: dahlia PR: dahlia/hollo#53 File: src/index.tsx:23-23 Timestamp: 2024-10-27T15:51:16.761Z Learning: In this codebase, environment variables do not have the `HOLLO_` prefix. Future environment variables should be named consistently without the `HOLLO_` prefix, e.g., use `PORT` or `LISTEN_PORT` instead of `HOLLO_PORT`.
🪛 Biome
src/federation/index.ts
[error] 112-112: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
src/index.tsx
[error] 23-23: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
🔇 Additional comments (1)
src/federation/index.ts (1)
112-112: Verify the security implications of enabling private addressesThe code allows federation with private addresses when
ALLOW_PRIVATE_ADDRESSis set to "true". This configuration should be used with caution as it could potentially lead to Server-Side Request Forgery (SSRF) attacks.Let's check if there are any security-related comments or documentation:
✅ Verification successful
Security measures are in place for private address handling
The codebase has proper security measures in place:
- Uses
@fedify/fedifylibrary (v1.1.2) for federation implementation- Includes
ssrfcheck(v1.1.1) as a dependency, which is a dedicated SSRF prevention library- The
allowPrivateAddressflag is part of thecreateFederationconfiguration, indicating it's a controlled feature of the federation libraryThe security concern is adequately addressed through these built-in protections, making this a safe and intentional configuration option.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for security documentation related to private addresses echo "Searching for security documentation..." rg -i "private.*address|ssrf|security" -g "*.{md,txt}" # Check if the environment variable is documented echo "Checking environment variable documentation..." fd -e md -e env.example | xargs rg "ALLOW_PRIVATE_ADDRESS"Length of output: 307
Script:
#!/bin/bash # Let's check the context around the usage of ALLOW_PRIVATE_ADDRESS echo "Checking usage context of ALLOW_PRIVATE_ADDRESS..." rg -B 5 -A 5 "ALLOW_PRIVATE_ADDRESS" # Let's also check for any validation or security checks around private addresses echo "Checking for private address validation..." ast-grep --pattern 'isPrivateAddress($$$)' # Look for any network or URL validation related code echo "Checking for URL/network validation..." rg -B 3 -A 3 "isPrivate|privateAddress|validateUrl|checkUrl"Length of output: 985
Script:
#!/bin/bash # Let's examine the federation module implementation to understand how this flag is used echo "Checking federation module implementation..." rg -B 10 -A 10 "createFederation" # Let's also check if there are any tests related to private address handling echo "Checking related tests..." fd -e test.ts -e spec.ts | xargs rg -l "ALLOW_PRIVATE_ADDRESS|privateAddress" # Check the dependencies to see if there's a federation library being used echo "Checking package.json for federation-related dependencies..." fd package.json | xargs catLength of output: 4814
🧰 Tools
🪛 Biome
[error] 112-112: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
src/index.tsx
Outdated
| @@ -20,5 +20,6 @@ app.get("/nodeinfo/2.0", (c) => c.redirect("/nodeinfo/2.1")); | |||
|
|
|||
| // biome-ignore lint/complexity/useLiteralKeys: tsc complains about this (TS4111) | |||
| const BEHIND_PROXY = process.env["BEHIND_PROXY"] === "true"; | |||
| const HOLLO_PORT = Number.parseInt(process.env["LISTEN_PORT"] ?? 3000, 10); | |||
There was a problem hiding this comment.
Enhance port configuration robustness and naming.
- The environment variable name should be consistent with existing conventions (avoid HOLLO_ prefix).
- Add validation for port number range and proper error handling.
Apply this diff to address these concerns:
-const HOLLO_PORT = Number.parseInt(process.env["LISTEN_PORT"] ?? 3000, 10);
+const port = (() => {
+ const rawPort = Number.parseInt(process.env["LISTEN_PORT"] ?? "3000", 10);
+ if (isNaN(rawPort) || rawPort < 0 || rawPort > 65535) {
+ throw new Error("LISTEN_PORT must be a valid port number between 0 and 65535");
+ }
+ return rawPort;
+})();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const HOLLO_PORT = Number.parseInt(process.env["LISTEN_PORT"] ?? 3000, 10); | |
| const port = (() => { | |
| const rawPort = Number.parseInt(process.env["LISTEN_PORT"] ?? "3000", 10); | |
| if (isNaN(rawPort) || rawPort < 0 || rawPort > 65535) { | |
| throw new Error("LISTEN_PORT must be a valid port number between 0 and 65535"); | |
| } | |
| return rawPort; | |
| })(); |
🧰 Tools
🪛 Biome
[error] 23-23: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
src/federation/index.ts
Outdated
| @@ -109,6 +109,7 @@ if (getRedisUrl() == null) { | |||
| export const federation = createFederation<void>({ | |||
| kv, | |||
| queue, | |||
| allowPrivateAddress: process.env["ALLOW_PRIVATE_ADDRESS"] === "true" | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing IP range configuration for private addresses
Based on the PR feedback, instead of a simple boolean flag, consider implementing a more flexible configuration that accepts comma-separated IP ranges. This approach:
- Provides granular control over allowed private addresses
- Aligns with Mastodon's implementation
- Reduces security risks by limiting the scope of allowed private addresses
Here's a suggested implementation:
- allowPrivateAddress: process.env["ALLOW_PRIVATE_ADDRESS"] === "true"
+ allowPrivateAddresses: process.env.ALLOW_PRIVATE_ADDRESSES?.split(",").map(range => range.trim()).filter(Boolean) ?? []Usage example in .env:
ALLOW_PRIVATE_ADDRESSES=192.168.1.0/24,10.0.0.0/8
Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 Biome
[error] 112-112: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
There was a problem hiding this comment.
@HelgeKrueger this above would be the better implementation ^, but it may be required to make changes in Fedify first.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@HelgeKrueger yeah, needs a change in Fedify here: https://github.com/dahlia/fedify/blob/main/src/runtime/docloader.ts#L221
There was a problem hiding this comment.
On a side note, I've added this feature to https://socialhub.activitypub.rocks/t/things-a-fediverse-server-must-implement/4668/3?u=helge
|
Added change to .env.sample I also created dahlia/fedify#157 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
src/federation/index.ts (1)
112-112: Simplify environment variable access.The environment variable access can be simplified by using dot notation.
- allowPrivateAddress: process.env["ALLOW_PRIVATE_ADDRESS"] === "true" + allowPrivateAddress: process.env.ALLOW_PRIVATE_ADDRESS === "true"🧰 Tools
🪛 Biome
[error] 112-112: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .env.sample (1 hunks)
- src/federation/index.ts (1 hunks)
- src/index.tsx (1 hunks)
🧰 Additional context used
📓 Learnings (2)
src/federation/index.ts (1)
Learnt from: dahlia PR: dahlia/hollo#53 File: src/federation/index.ts:112-112 Timestamp: 2024-10-27T15:51:25.872Z Learning: In the Hollo application, `ALLOW_PRIVATE_ADDRESS` is turned off by default to prevent SSRF attacks.src/index.tsx (1)
Learnt from: dahlia PR: dahlia/hollo#53 File: src/index.tsx:23-23 Timestamp: 2024-10-27T15:51:16.761Z Learning: In this codebase, environment variables do not have the `HOLLO_` prefix. Future environment variables should be named consistently without the `HOLLO_` prefix, e.g., use `PORT` or `LISTEN_PORT` instead of `HOLLO_PORT`.
🪛 Biome
src/federation/index.ts
[error] 112-112: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
src/index.tsx
[error] 23-23: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
🔇 Additional comments (3)
src/index.tsx (3)
25-25: LGTM with previous suggestions.The export logic is correct, implementing the proxy configuration as intended. Please refer to the previous review comment about adding type declarations for improved type safety.
23-25: Verify environment variable documentation.Let's ensure the new environment variable is properly documented.
✅ Verification successful
Let me check the content of .env.sample to verify if LISTEN_PORT is properly documented there.
Environment variable is properly documented
The
LISTEN_PORTenvironment variable is well documented in.env.samplewith a clear description indicating it's used as the server port whenBEHIND_PROXY=true. The default value of 3000 matches the code implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if LISTEN_PORT is documented in relevant files echo "Checking documentation for LISTEN_PORT configuration:" rg -l "LISTEN_PORT" --type md rg -l "LISTEN_PORT" -g ".env*"Length of output: 189
Script:
#!/bin/bash # Check the documentation of LISTEN_PORT in .env.sample echo "Content of .env.sample related to LISTEN_PORT:" rg "LISTEN_PORT" .env.sample -C 2Length of output: 373
🧰 Tools
🪛 Biome
[error] 23-23: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
23-23:⚠️ Potential issueImprove port configuration implementation.
The current implementation has a few issues:
- The variable name uses
HOLLO_prefix which is inconsistent with codebase conventions- The default value should be a string when using with
??operator since environment variables are stringsApply this diff to address these concerns:
-const HOLLO_PORT = Number.parseInt(process.env["LISTEN_PORT"] ?? 3000, 10); +const port = Number.parseInt(process.env["LISTEN_PORT"] ?? "3000", 10);Likely invalid or redundant comment.
🧰 Tools
🪛 Biome
[error] 23-23: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .env.sample (1 hunks)
- src/federation/index.ts (1 hunks)
- src/index.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .env.sample
🧰 Additional context used
📓 Learnings (1)
src/federation/index.ts (1)
Learnt from: dahlia
PR: dahlia/hollo#53
File: src/federation/index.ts:112-112
Timestamp: 2024-10-27T15:51:25.872Z
Learning: In the Hollo application, `ALLOW_PRIVATE_ADDRESS` is turned off by default to prevent SSRF attacks.
🪛 Biome
src/federation/index.ts
[error] 112-112: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
src/index.tsx
[error] 23-23: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
src/index.tsx
Outdated
|
|
||
| export default BEHIND_PROXY ? { fetch: behindProxy(app.fetch.bind(app)) } : app; | ||
| export default BEHIND_PROXY ? { fetch: behindProxy(app.fetch.bind(app)), port: HOLLO_PORT } : app; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type declarations for improved type safety.
The conditional export logic correctly exposes the port configuration when behind a proxy. However, adding explicit type declarations would improve type safety and maintainability.
Consider adding type declarations:
type AppExport = {
fetch: typeof app.fetch;
port?: number;
};
export default BEHIND_PROXY
? { fetch: behindProxy(app.fetch.bind(app)), port: listenPort } as AppExport
: app;
src/federation/index.ts
Outdated
| @@ -109,6 +109,7 @@ if (getRedisUrl() == null) { | |||
| export const federation = createFederation<void>({ | |||
| kv, | |||
| queue, | |||
| allowPrivateAddress: process.env["ALLOW_PRIVATE_ADDRESS"] === "true" | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing IP range configuration.
Based on the PR feedback from ThisIsMissEm, consider implementing a more granular approach that accepts comma-separated IP ranges instead of a boolean flag. This would provide better control over which private addresses are allowed.
Example configuration:
ALLOW_PRIVATE_ADDRESSES=192.168.1.0/24,10.0.0.0/8This approach:
- Provides granular control over allowed private addresses
- Aligns with Mastodon's implementation
- Reduces security risks by limiting the scope
Would you like assistance in implementing this feature?
🧰 Tools
🪛 Biome
[error] 112-112: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
Document security implications and simplify environment variable access.
The ALLOW_PRIVATE_ADDRESS environment variable controls SSRF protection. Consider:
- Adding a security notice in the code.
- Simplifying the environment variable access.
Apply this diff:
- allowPrivateAddress: process.env["ALLOW_PRIVATE_ADDRESS"] === "true"
+ // SECURITY: Enabling private addresses allows federation with internal network endpoints.
+ // Only enable this in trusted environments where internal federation is required.
+ allowPrivateAddress: process.env.ALLOW_PRIVATE_ADDRESS === "true"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| allowPrivateAddress: process.env["ALLOW_PRIVATE_ADDRESS"] === "true" | |
| // SECURITY: Enabling private addresses allows federation with internal network endpoints. | |
| // Only enable this in trusted environments where internal federation is required. | |
| allowPrivateAddress: process.env.ALLOW_PRIVATE_ADDRESS === "true" |
🧰 Tools
🪛 Biome
[error] 112-112: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
💡 Codebase verification
Environment variable ALLOW_PRIVATE_ADDRESS requires documentation
The ALLOW_PRIVATE_ADDRESS environment variable is used in federation configuration but lacks documentation about its security implications. This variable appears to control whether private addresses are allowed in federation, which could have significant security ramifications.
Required actions:
- Add documentation for
ALLOW_PRIVATE_ADDRESSin the environment variables documentation atdocs.hollo.social/install/env/ - Include security implications and recommended settings
- Consider adding a warning if enabling private addresses in federation context
🔗 Analysis chain
Verify environment variable documentation.
Let's ensure the security implications and configuration options are properly documented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if ALLOW_PRIVATE_ADDRESS is documented
echo "Checking documentation for ALLOW_PRIVATE_ADDRESS..."
fd -e md -e env.example -e env.sample | xargs rg -i "ALLOW_PRIVATE_ADDRESS"
# Check for any security documentation
echo -e "\nChecking for security documentation..."
fd -e md | xargs rg -i "security|ssrf|private.*address"
Length of output: 339
Script:
#!/bin/bash
# Let's check for any environment variable documentation in common locations
echo "Checking for env documentation in README and docs..."
fd "README|env|configuration" -e md | xargs cat
# Check for .env files or examples
echo -e "\nChecking for .env files or examples..."
fd -e env -e env.example -e env.sample -e env.template | xargs cat
# Check the actual usage context
echo -e "\nChecking usage context..."
rg -B 3 -A 3 "ALLOW_PRIVATE_ADDRESS"
Length of output: 2568
🧰 Tools
🪛 Biome
[error] 112-112: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
|
It is strange when I run biome, there is no erro rrmessage ... |
|
These errors came from TypeScript ( |
|
Sorry, for needing so many tries to get everything working. I still cannot get biome working in the docker environment. But it works on the host. Not sure what is going on. As far as tsc, I get the errors. Are they expected? |
|
Did you run |
By setting
BEHIND_PROXY: "true"
HOLLO_PORT: 80
ALLOW_PRIVATE_ADDRESS: "true"
one allows private addresses and runs on port 80.
By setting
one allows private addresses and runs on port 80.
Summary by CodeRabbit
New Features
Documentation
.env.sampleto include the newLISTEN_PORTandALLOW_PRIVATE_ADDRESSvariables for server configuration.