chore: fix namespace check #196
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideAdds an SSR static file route for serving built assets from the dist directory, adjusts dist path resolution for dev vs prod, tightens publish namespace editability rules, and updates CI integration tests to run against the Bun-based SSR server instead of the Vite preview server. Sequence diagram for SSR static file handling routesequenceDiagram
actor Browser
participant SSRServer
participant staticRoute
participant marketingRoute
participant publishRoute
participant FileSystem as FileSystem
Browser->>SSRServer: HTTP GET /static/app.js
SSRServer->>staticRoute: handle(RequestContext)
alt request_method_not_GET
staticRoute-->>SSRServer: undefined
SSRServer->>marketingRoute: handle(RequestContext)
else not_static_path_or_file
staticRoute-->>SSRServer: undefined
SSRServer->>marketingRoute: handle(RequestContext)
else static_path_or_file
staticRoute->>FileSystem: resolve(distDir, pathname)
alt path_traversal_detected
staticRoute-->>SSRServer: Response 403 Forbidden
SSRServer-->>Browser: 403 Forbidden
else path_valid
FileSystem-->>staticRoute: file bytes or error
alt file_found
staticRoute-->>SSRServer: Response 200 with ContentType
SSRServer-->>Browser: 200 OK (static asset)
else file_not_found
staticRoute-->>SSRServer: undefined
SSRServer->>marketingRoute: handle(RequestContext)
end
end
end
opt marketing_route_does_not_handle
SSRServer->>publishRoute: handle(RequestContext)
end
Class diagram for updated publish namespace and homepage settingsclassDiagram
class PublishManage {
+onClose() void
-isOwner boolean
-activeSubscription SubscriptionPlan|null
-namespace string
-handleRemoveHomePage() void
-handleUpdateHomePage() void
-render() void
}
class HomePageSetting {
+spaceId string
+namespace string
+siteId string
+homePage View|null
+publishViews View[]
+isOwner boolean
+activePlan SubscriptionPlan|null
+canEdit boolean
-removeLoading boolean
-updateLoading boolean
-onRemoveHomePage() void
-onUpdateHomePage() void
-render() void
}
class SubscriptionPlan {
}
class View {
}
PublishManage o-- HomePageSetting : uses
HomePageSetting --> SubscriptionPlan : activePlan
HomePageSetting --> View : publishViews
class NamespaceEditGuard {
+canEditNamespace(namespace string) boolean
+hasEditPermission(isOwner boolean, isOfficialHost boolean, activeSubscription SubscriptionPlan|null) boolean
}
PublishManage ..> NamespaceEditGuard : computes_canEdit
class TooltipBehavior {
+getTooltipTitle(isOwner boolean, isOfficialHost boolean, activeSubscription SubscriptionPlan|null) string|null
+isEditBlocked(isOwner boolean, isOfficialHost boolean, activeSubscription SubscriptionPlan|null) boolean
}
PublishManage ..> TooltipBehavior : uses
class UuidValidator {
+uuidValidate(value string) boolean
}
PublishManage ..> UuidValidator : determines_canEdit
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- The
staticRoutehandler usesfs.readFileSync, which will block the event loop for larger files; consider switching tofs.promises.readFileor a streaming approach to avoid synchronous disk I/O in the request path. HomePageSettingstill accepts anisOwnerprop that is no longer used (other than in auseEffectdependency), which can cause confusion; either remove the prop entirely or wire it back into the logic where it meaningfully affects behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `staticRoute` handler uses `fs.readFileSync`, which will block the event loop for larger files; consider switching to `fs.promises.readFile` or a streaming approach to avoid synchronous disk I/O in the request path.
- `HomePageSetting` still accepts an `isOwner` prop that is no longer used (other than in a `useEffect` dependency), which can cause confusion; either remove the prop entirely or wire it back into the logic where it meaningfully affects behavior.
## Individual Comments
### Comment 1
<location> `deploy/routes.ts:59` </location>
<code_context>
+ }
+
+ try {
+ const file = fs.readFileSync(filePath);
+ const ext = path.extname(filePath);
+ const contentType = MIME_TYPES[ext] || 'application/octet-stream';
</code_context>
<issue_to_address>
**suggestion (performance):** Using synchronous fs.readFileSync in an async route handler can block the event loop.
Since this handler is async, using `fs.readFileSync` will block the event loop during disk I/O and can hurt throughput under load or with larger files. Prefer an async alternative like `await fs.promises.readFile(filePath)` (or Bun’s async API) to keep this path non-blocking.
Suggested implementation:
```typescript
try {
const file = await fs.promises.readFile(filePath);
const ext = path.extname(filePath);
const contentType = MIME_TYPES[ext] || 'application/octet-stream';
return new Response(file, {
headers: { 'Content-Type': contentType },
});
} catch {
logger.warn(`Static file not found: ${filePath}`);
return;
}
};
```
1. Ensure that the enclosing route handler function is declared as `async` (e.g., `export default async function ...`), otherwise `await` will not be allowed.
2. Confirm that `fs` is imported from Node's `fs` module in a way that supports `fs.promises`, e.g.: `import fs from 'fs';` or `import * as fs from 'fs';`. If it is currently destructured (e.g., `import { readFileSync } from 'fs';`), adjust the import to allow `fs.promises.readFile`.
</issue_to_address>
### Comment 2
<location> `deploy/routes.ts:53` </location>
<code_context>
+ // Prevent path traversal attacks - ensure resolved path stays within distDir
+ const normalizedDistDir = path.resolve(distDir);
+
+ if (!filePath.startsWith(normalizedDistDir + path.sep) && filePath !== normalizedDistDir) {
+ logger.warn(`Path traversal attempt blocked: ${url.pathname}`);
+ return new Response('Forbidden', { status: 403 });
</code_context>
<issue_to_address>
**🚨 issue (security):** Path traversal protection based on string startsWith can be brittle across platforms.
`startsWith` assumes consistent casing and path semantics, which can be bypassed on case‑insensitive filesystems or via symlinks. A safer pattern is to compare normalized, relative paths instead of raw prefixes, e.g.:
```ts
const rel = path.relative(normalizedDistDir, filePath);
if (rel.startsWith('..') || path.isAbsolute(rel)) {
logger.warn(`Path traversal attempt blocked: ${url.pathname}`);
return new Response('Forbidden', { status: 403 });
}
```
This enforces containment within `distDir` in a more portable way.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // Prevent path traversal attacks - ensure resolved path stays within distDir | ||
| const normalizedDistDir = path.resolve(distDir); | ||
|
|
||
| if (!filePath.startsWith(normalizedDistDir + path.sep) && filePath !== normalizedDistDir) { |
There was a problem hiding this comment.
🚨 issue (security): Path traversal protection based on string startsWith can be brittle across platforms.
startsWith assumes consistent casing and path semantics, which can be bypassed on case‑insensitive filesystems or via symlinks. A safer pattern is to compare normalized, relative paths instead of raw prefixes, e.g.:
const rel = path.relative(normalizedDistDir, filePath);
if (rel.startsWith('..') || path.isAbsolute(rel)) {
logger.warn(`Path traversal attempt blocked: ${url.pathname}`);
return new Response('Forbidden', { status: 403 });
}This enforces containment within distDir in a more portable way.
josue693
pushed a commit
to josue693/AppFlowy-Web
that referenced
this pull request
Dec 21, 2025
* chore: fix namespace check * chore: lint * chore: add test
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Checklist
General
Testing
Feature-Specific
Summary by Sourcery
Serve static assets from the built dist directory in the SSR server, adjust publish namespace editability rules, and update local/CI dev server configuration.
New Features:
Bug Fixes:
Enhancements:
Build:
CI: