Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe update adds a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Main
participant Server
participant Status
User->>Main: Launch with -p <port>
Main->>Server: New(core, ..., httpPort)
Server->>Status: ServeStatus(router, core, ..., httpPort)
Server->>Status: ServeStatusRedirect(router, httpPort)
Server->>Server: Start HTTP server on :<httpPort>
User->>Server: HTTP request to :<httpPort>
Server->>Status: Handle status/redirect with dynamic port
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate Unit Tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 7
🔭 Outside diff range comments (1)
onekey.go (1)
105-115: Validate user-supplied port before use
flag.IntVar(&httpPort, …)allows negative numbers and values > 65535.
A quick range check prevents accidental crashes or privilege issues.flag.Parse() +if httpPort <= 0 || httpPort > 65535 { + log.Fatalf("invalid HTTP port: %d (must be 1-65535)", httpPort) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
README.md(3 hunks)VERSION(1 hunks)go.mod(1 hunks)onekey.go(3 hunks)server/http.go(3 hunks)server/status/status.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
onekey.go (1)
server/http.go (1)
New(28-75)
server/http.go (2)
server/status/status.go (2)
ServeStatus(39-55)ServeStatusRedirect(26-33)server/api/api.go (1)
ServeAPI(26-54)
server/status/status.go (3)
server/status/origincheck.go (1)
OriginCheck(30-38)core/core.go (1)
Core(98-133)memorywriter/memorywriter.go (1)
MemoryWriter(22-32)
🪛 LanguageTool
README.md
[uncategorized] ~17-~17: Possible missing comma found.
Context: ...n OneKey bridge runs an HTTP server on port 21320 by default. You can change this p...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~35-~35: Consider a more concise word here.
Context: ..../onekeyd -e 54935` To disable all USB in order to run on some virtual environments,launch...
(IN_ORDER_TO_PREMIUM)
[misspelling] ~54-~54: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...I documentation onekey-bridge starts a HTTP server on http://localhost:21320...
(EN_A_VS_AN)
🪛 markdownlint-cli2 (0.17.2)
README.md
20-20: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
25-25: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
38-38: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
🪛 Gitleaks (8.26.0)
server/status/status.go
24-24: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (4)
go.mod (1)
14-14: Make surego.sumis regenerated after the TOML bumpUpdating an indirect dependency should be followed by
go mod tidyso thatgo.sumreflects the new hashes.
Otherwise CI may fail on checksum mismatch in clean clones.Run:
#!/bin/bash go mod tidy git diff --exit-code go.sumVERSION (1)
1-2: Version bump looks good
No concerns.server/http.go (1)
38-40: Bind onlocalhostonly — intentional?Hard-coding
127.0.0.1prevents remote access even in trusted LAN setups.
If that is not intentional, accept0.0.0.0when the user passes
-pbut also wants exposure.-Addr: fmt.Sprintf("127.0.0.1:%d", httpPort), +Addr: fmt.Sprintf(":%d", httpPort), // listen on all interfacesserver/status/status.go (1)
21-22:portfield is written but never read
status.portgets set inServeStatus(line 45) yet nothing in this file reads it. Unused data adds noise and may rot.Either remove the field or actually use it (e.g. expose it in templates or helpers).
| s, err := server.New(c, stderrWriter, shortMemoryWriter, longMemoryWriter, version, httpPort) | ||
|
|
||
| if err != nil { | ||
| stderrLogger.Fatalf("https: %s", err) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Handle “address already in use” gracefully
Right now server.New returns an http.Server that will fail only when
Run() is called. Consider binding early so you can surface a clearer
error (e.g., “port 21320 busy”) before the heavy initialisation work.
🤖 Prompt for AI Agents
In onekey.go around lines 173 to 176, the current code initializes the server
but only detects port conflicts when Run() is called, which delays error
reporting. Modify the server.New function or its usage to perform an early bind
to the specified port before full initialization, so that if the port is already
in use, the error can be caught and reported immediately with a clear message
like "port 21320 busy." This will require attempting to listen on the port early
and returning an error if binding fails, allowing the caller to handle it
gracefully before proceeding with heavy setup.
| writer io.Writer | ||
| port int | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
port field is never read
Server.port is assigned but not used elsewhere. Either:
- Remove the field to keep the struct lean, or
- Expose a getter so callers can ask the server which port it bound.
Leaning toward option 1 for now.
🤖 Prompt for AI Agents
In server/http.go around lines 24 to 26, the port field in the Server struct is
assigned but never read or used. To fix this, remove the port field entirely
from the Server struct to keep it lean and avoid unused code.
| OneKey bridge runs an HTTP server on port 21320 by default. You can change this port using the `-p` parameter. | ||
|
|
||
| **Examples:** | ||
| - `./onekeyd` - Uses default port 21320 | ||
| - `./onekeyd -p 21325` - Uses port 21325 | ||
| - `./onekeyd -p 21321` - Uses port 21321 | ||
|
|
||
| This is useful when: |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix article and punctuation
-OneKey bridge runs an HTTP server on port 21320 by default. You can change this port using the `-p` parameter.
+OneKey Bridge runs an HTTP server on port 21320 by default; you can change the port with the `-p` parameter.📝 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.
| OneKey bridge runs an HTTP server on port 21320 by default. You can change this port using the `-p` parameter. | |
| **Examples:** | |
| - `./onekeyd` - Uses default port 21320 | |
| - `./onekeyd -p 21325` - Uses port 21325 | |
| - `./onekeyd -p 21321` - Uses port 21321 | |
| This is useful when: | |
| OneKey Bridge runs an HTTP server on port 21320 by default; you can change the port with the `-p` parameter. | |
| **Examples:** | |
| - `./onekeyd` - Uses default port 21320 | |
| - `./onekeyd -p 21325` - Uses port 21325 | |
| - `./onekeyd -p 21321` - Uses port 21321 | |
| This is useful when: |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~17-~17: Possible missing comma found.
Context: ...n OneKey bridge runs an HTTP server on port 21320 by default. You can change this p...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
20-20: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
🤖 Prompt for AI Agents
In README.md around lines 17 to 24, correct the article usage and punctuation in
the description about the OneKey bridge HTTP server port. Change "an HTTP
server" to "a HTTP server" if following the pronunciation rule or keep "an HTTP
server" if considering the 'H' sound, and ensure punctuation such as colons and
periods are properly placed after the examples and sentences for clarity and
grammatical correctness.
| **Examples:** | ||
| - `./onekeyd` - Uses default port 21320 | ||
| - `./onekeyd -p 21325` - Uses port 21325 | ||
| - `./onekeyd -p 21321` - Uses port 21321 | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Blank line before list to satisfy Markdownlint (MD032)
-**Examples:**
+**Examples:**
+📝 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.
| **Examples:** | |
| - `./onekeyd` - Uses default port 21320 | |
| - `./onekeyd -p 21325` - Uses port 21325 | |
| - `./onekeyd -p 21321` - Uses port 21321 | |
| **Examples:** | |
| - `./onekeyd` - Uses default port 21320 | |
| - `./onekeyd -p 21325` - Uses port 21325 | |
| - `./onekeyd -p 21321` - Uses port 21321 |
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
20-20: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
🤖 Prompt for AI Agents
In README.md around lines 19 to 23, add a blank line before the list of examples
to satisfy Markdownlint rule MD032. This means inserting an empty line above the
line starting with "**Examples:**" to separate the preceding paragraph or
content from the list.
| "/status/": "", | ||
| "/status/log.gz": "http://127.0.0.1:21320", | ||
| "/status/log.gz": fmt.Sprintf("http://127.0.0.1:%d", httpPort), | ||
| })) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Origin map can be built once
fmt.Sprintf runs on every request middleware chain build. Not critical, but you could pre-compute the map for clarity:
allowed := map[string]string{
"/status/": "",
"/status/log.gz": fmt.Sprintf("http://127.0.0.1:%d", httpPort),
}
r.Use(OriginCheck(allowed))🤖 Prompt for AI Agents
In server/status/status.go around lines 52 to 54, the origin map is being built
with fmt.Sprintf inside the middleware setup, causing it to be recomputed on
every request. To fix this, precompute the allowed origins map once before
setting up the middleware by creating the map with the formatted string assigned
to "/status/log.gz" and then pass this precomputed map to the OriginCheck
middleware. This improves clarity and avoids unnecessary repeated computation.
| func ServeStatusRedirect(r *mux.Router, httpPort int) { | ||
| r.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { | ||
| redirect(w, r, httpPort) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sanity-check httpPort before using it
ServeStatusRedirect trusts the incoming httpPort. If the caller passes 0 or a negative/invalid value you’ll generate a broken URL like http://127.0.0.1:0/status/.
Add a quick guard:
+if httpPort <= 0 || httpPort > 65535 {
+ httpPort = 21320 // fallback or return an error
+}🤖 Prompt for AI Agents
In server/status/status.go around lines 26 to 29, the ServeStatusRedirect
function uses the httpPort parameter without validation, which can lead to
invalid URLs if httpPort is zero or negative. Add a guard clause at the start of
ServeStatusRedirect to check if httpPort is a positive integer; if not, set it
to a default valid port number or return early to avoid creating broken redirect
URLs.
| func redirect(w http.ResponseWriter, r *http.Request, httpPort int) { | ||
| http.Redirect(w, r, fmt.Sprintf("http://127.0.0.1:%d/status/", httpPort), http.StatusMovedPermanently) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use 307 or 308 for redirects that preserve method
For local tooling a permanent 301 works, but if clients might POST to / you could break them. Consider a 307/308 which keeps the HTTP method intact:
-http.Redirect(w, r, fmt.Sprintf("http://127.0.0.1:%d/status/", httpPort), http.StatusMovedPermanently)
+http.Redirect(w, r, fmt.Sprintf("http://127.0.0.1:%d/status/", httpPort), http.StatusTemporaryRedirect)🤖 Prompt for AI Agents
In server/status/status.go around lines 35 to 37, the redirect uses HTTP status
301 which does not preserve the HTTP method and can break POST requests. Change
the status code in the http.Redirect call to either 307 or 308 to ensure the
HTTP method is preserved during the redirect.
1 participant
This PR merges the changes from the emulator branch into the onekey branch.
Summary by CodeRabbit