-
Notifications
You must be signed in to change notification settings - Fork 21
fix: add crypto.randomUUID polyfill and SPA routing support #97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add polyfills.ts for crypto.randomUUID in unsecured contexts (HTTP) - Import polyfills at top of main.tsx entry point - Create server.py with SPA routing support (serves index.html for non-asset routes) - Update systemd service to use new SPA-aware server Fixes: ErikBjare/bob#69 (blank page with crypto.randomUUID error) Fixes: SPA routing issue (routes like /chat now work) Co-authored-by: Bob <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to 59e7c40 in 1 minute and 44 seconds. Click for details.
- Reviewed
89lines of code in3files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. server.py:44
- Draft comment:
Consider using a threaded server (e.g. socketserver.ThreadingTCPServer) for better concurrency. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. server.py:31
- Draft comment:
Extract asset file extensions to a constant for easier maintainability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. src/main.tsx:7
- Draft comment:
Ensure asynchronous logging setup doesn't interfere with app initialization if logging is required early. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. src/polyfills.ts:4
- Draft comment:
Note that the polyfill uses Math.random(), which isn't cryptographically secure; ensure this meets your development needs. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is stating something that the code author has already explicitly acknowledged in line 1 with "development/unsecure contexts". This is not actionable - it's not asking for a change, just noting a fact that's already documented in the code itself. The comment starts with "Note that..." and ends with "ensure this meets your development needs" which is asking the author to confirm/verify something, which violates the rules. This is exactly the type of comment that should be removed - it's informative but not actionable, and the concern is already addressed in the code comments. Perhaps the comment is valuable because it makes the security limitation more explicit for future maintainers who might not understand what "unsecure contexts" means? Maybe the inline comment on line 1 isn't sufficient warning? The code already has a clear comment on line 1 stating this is for "development/unsecure contexts" which directly addresses the security concern. Adding a PR comment that restates this doesn't provide actionable feedback - it's just redundant information. The comment also asks the author to "ensure this meets your development needs" which is asking for confirmation, violating the rules. This comment should be deleted. It's purely informative, restates what's already in the code comments (line 1 mentions "unsecure contexts"), and asks the author to verify/ensure something rather than suggesting a concrete code change.
5. src/polyfills.ts:1
- Draft comment:
Typographical suggestion: In the comment on line 1, consider replacing "unsecure" with "insecure" for clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a typographical correction comment. Looking at the rules: it's not about obvious issues, it's not speculative, it's not asking for confirmation, and it's a clear actionable change. However, I need to consider if this falls under "Do NOT comment unless there is clearly a code change required" - this is just a comment typo, not actual code logic. The rules say "Do NOT make comments that are obvious or unimportant." A typo in a comment could be considered minor/unimportant. But it's also a legitimate correction that improves code quality. The rules favor code quality refactors that are "actionable and clear" - this is actionable and clear, though it's just a comment fix, not a code refactor. This is just a typo in a comment, not in actual code. While "insecure" is more correct than "unsecure", this might be considered too minor or unimportant to warrant a review comment. The code functionality is completely unaffected, and the meaning is still clear even with "unsecure". While it's true this doesn't affect functionality, the rules do allow for code quality improvements that are actionable and clear. However, the first rule emphasizes "Do NOT comment unless there is clearly a code change required" - a comment typo doesn't require a code change in the functional sense. This seems too minor. This comment is about a minor typo in a code comment that doesn't affect functionality. While technically correct, it falls under "obvious or unimportant" comments that should not be made according to the rules. The comment should be deleted.
Workflow ID: wflow_XAjdx3RRYMjClBgj
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR adds critical fixes for HTTP deployment and SPA routing:
- SPA Server (
server.py): Simple Python HTTP server that servesindex.htmlfor all non-asset routes, enabling client-side routing to work properly (fixes routes like/chat) - UUID Polyfill (
src/polyfills.ts): Addscrypto.randomUUIDfallback usingMath.random()for unsecured contexts where native implementation is unavailable - Early Initialization (
src/main.tsx): Imports polyfills before other modules to ensure availability
Key Findings
- The UUID polyfill uses
Math.random()which is not cryptographically secure and could lead to collisions in high-volume scenarios - The server's asset detection is incomplete (missing font formats and other common types)
- Default binding to
0.0.0.0may be too permissive for local development
The changes successfully address the immediate issues of blank pages and broken routing, though the UUID generation should ideally use crypto.getRandomValues() for better security.
Confidence Score: 3/5
- This PR is safe to merge with awareness of the cryptographic weakness in UUID generation
- Score reflects a critical logical issue with non-cryptographic UUID generation using
Math.random(). While this fixes the immediate blank page problem, the polyfill should usecrypto.getRandomValues()for secure random number generation. The SPA routing implementation is straightforward and functional. - Pay close attention to
src/polyfills.ts- the UUID generation usesMath.random()which is not cryptographically secure
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| server.py | 4/5 | Adds simple HTTP server with SPA routing fallback to index.html for non-asset routes |
| src/polyfills.ts | 3/5 | Implements crypto.randomUUID polyfill using Math.random() for unsecured contexts |
Sequence Diagram
sequenceDiagram
participant Browser
participant server.py
participant index.html
participant main.tsx
participant polyfills.ts
participant ApiClient
Browser->>server.py: GET /chat/123
server.py->>server.py: Check if path exists
server.py->>server.py: Not asset, not exists
server.py->>Browser: Serve index.html
Browser->>index.html: Load HTML
index.html->>main.tsx: Load main.tsx
main.tsx->>polyfills.ts: Import polyfills first
polyfills.ts->>polyfills.ts: Check if crypto.randomUUID exists
alt crypto.randomUUID missing
polyfills.ts->>polyfills.ts: Add Math.random() based polyfill
end
main.tsx->>Browser: Initialize React app
Browser->>ApiClient: Create instance
ApiClient->>polyfills.ts: Call crypto.randomUUID()
polyfills.ts->>ApiClient: Return UUID
ApiClient->>Browser: Ready with identifier
3 files reviewed, 3 comments
| return "xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx".replace(/[xy]/g, function(c) { | ||
| const r = Math.random() * 16 | 0; | ||
| const v = c === "x" ? r : (r & 0x3 | 0x8); | ||
| return v.toString(16); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Math.random() is not cryptographically secure. UUIDs generated this way could have collisions or be predictable. Consider using crypto.getRandomValues() if available:
| return "xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx".replace(/[xy]/g, function(c) { | |
| const r = Math.random() * 16 | 0; | |
| const v = c === "x" ? r : (r & 0x3 | 0x8); | |
| return v.toString(16); | |
| const bytes = new Uint8Array(16); | |
| crypto.getRandomValues(bytes); | |
| bytes[6] = (bytes[6] & 0x0f) | 0x40; // version 4 | |
| bytes[8] = (bytes[8] & 0x3f) | 0x80; // variant | |
| const hex = Array.from(bytes).map(b => b.toString(16).padStart(2, '0')).join(''); | |
| return `${hex.slice(0,8)}-${hex.slice(8,12)}-${hex.slice(12,16)}-${hex.slice(16,20)}-${hex.slice(20,32)}` as `${string}-${string}-${string}-${string}-${string}`; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/polyfills.ts
Line: 6:9
Comment:
**logic:** `Math.random()` is not cryptographically secure. UUIDs generated this way could have collisions or be predictable. Consider using `crypto.getRandomValues()` if available:
```suggestion
const bytes = new Uint8Array(16);
crypto.getRandomValues(bytes);
bytes[6] = (bytes[6] & 0x0f) | 0x40; // version 4
bytes[8] = (bytes[8] & 0x3f) | 0x80; // variant
const hex = Array.from(bytes).map(b => b.toString(16).padStart(2, '0')).join('');
return `${hex.slice(0,8)}-${hex.slice(8,12)}-${hex.slice(12,16)}-${hex.slice(16,20)}-${hex.slice(20,32)}` as `${string}-${string}-${string}-${string}-${string}`;
```
How can I resolve this? If you propose a fix, please make it concise.| return http.server.SimpleHTTPRequestHandler.do_GET(self) | ||
|
|
||
|
|
||
| def run_server(port=5701, bind='0.0.0.0'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider defaulting to 127.0.0.1 instead of 0.0.0.0 for local development security
Prompt To Fix With AI
This is a comment left during a code review.
Path: server.py
Line: 42:42
Comment:
**style:** Consider defaulting to `127.0.0.1` instead of `0.0.0.0` for local development security
How can I resolve this? If you propose a fix, please make it concise.Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Extracted from PR #95 as suggested by @ErikBjare.
This PR adds:
Fixes:
Files changed:
This is an independent fix that can be merged immediately.
Important
Adds
crypto.randomUUIDpolyfill and SPA routing support viaserver.pyto fix routing and UUID issues.crypto.randomUUIDpolyfill insrc/polyfills.tsfor unsecured contexts.src/main.tsx.server.pyto support SPA routing by servingindex.htmlfor non-asset routes.crypto.randomUUID./chat.This description was created by
for 59e7c40. You can customize this summary. It will automatically update as commits are pushed.