🤖 Code Audit: 17 potential issue(s) found

[asmit25805]

Code Audit Report

All findings are reviewed for confidence before posting.
Please verify each finding before acting on it.

Repository: Tong89/smartNode
Findings: 17 issue(s) found — 🔴 1 critical · 🟠 10 high · 🟡 5 medium · 🔵 1 low

---

1. 🐛 Missing required arguments when calling link_budget_relay and link_budget_direct

Field	Details
Severity	🔴 Critical
Type	Bug
File	backend/orbit.py
Location	calculate_multi_hop_relay_rate
Confidence	94%

Problem:
The functions link_budget_relay and link_budget_direct are invoked with only a subset of the parameters they require (e.g., elevation_deg, rainfall_rate_mm_h, gs_altitude_km). This will raise a TypeError at runtime, breaking the multi‑hop relay rate calculation.

Suggested Fix:
Supply all mandatory arguments when calling these functions, mirroring the calls in calculate_relay_rate and calculate_direct_rate (e.g., include elevation_deg, rainfall_rate_mm_h, and gs_altitude_km).

---

2. 🔒 Open mode grants admin role when API key is unset

Field	Details
Severity	🟠 High
Type	Security
File	backend/auth.py
Location	init_auth
Confidence	96%

Problem:
If the environment variable SMARTNODE_API_KEY is not set, the middleware falls back to an "open" mode that assigns every request an identity with role "admin". This effectively gives full administrative privileges to unauthenticated users, which is a severe security risk if the application is deployed without the API key configured.

Suggested Fix:
Require an explicit configuration flag to enable open mode, and assign a least‑privilege role (e.g., "viewer") instead of "admin". Alternatively, abort start‑up when SMARTNODE_API_KEY is missing in production environments.

---

3. 🐛 Uncaught TypeError when JSON config file is not a dict

Field	Details
Severity	🟠 High
Type	Bug
File	backend/config.py
Location	LayeredSettings.load_file
Confidence	96%

Problem:
load_file reads a JSON file and unconditionally calls self._values.update(json.load(f) or {}), assuming the parsed JSON is a mapping. If the file contains a valid JSON array or other non-dict type, json.load returns that type and dict.update raises a TypeError, which is not caught, causing the application to crash during configuration loading.

Suggested Fix:
Validate that the parsed JSON is a dict before updating, e.g.,

data = json.load(f)
if isinstance(data, dict):
    self._values.update(data)
else:
    # optionally log a warning or ignore the file
    pass

---

4. 🔒 Default JWT secret is insecure in production if validation is bypassed

Field	Details
Severity	🟠 High
Type	Security
File	backend/config.py
Location	get_jwt_secret
Confidence	94%

Problem:
get_jwt_secret returns a hard‑coded placeholder secret ('dev-insecure-secret-change-me') when the environment variable SMARTNODE_JWT_SECRET is absent. If the application starts without calling validate_config (or if validate_config is ignored), a production deployment could use this predictable secret, allowing attackers to forge JWTs and gain unauthorized access.

Suggested Fix:
Remove the default placeholder and require the environment variable, e.g.,

def get_jwt_secret():
    secret = os.environ.get("SMARTNODE_JWT_SECRET")
    if not secret:
        raise RuntimeError("SMARTNODE_JWT_SECRET must be set in production")
    return secret

---

5. 🐛 Duplicate ground station IDs

Field	Details
Severity	🟠 High
Type	Bug
File	backend/constellation.py
Location	GEO_RELAY_GROUND_STATIONS
Confidence	96%

Problem:
The GEO_RELAY_GROUND_STATIONS list reuses IDs (e.g., GS_046, GS_030, GS_042) that already exist in the CHINA_GROUND_STATIONS list. If IDs are expected to be unique identifiers, this will cause collisions when the data is merged or looked up, leading to incorrect station mapping or overwriting of entries.

Suggested Fix:
Assign unique IDs to the GEO_RELAY_GROUND_STATIONS entries, e.g., use a different prefix such as RELAY_GS_001, RELAY_GS_002, etc., or ensure the IDs do not clash with existing ground station IDs.

---

6. 🐛 Error code constants use strings instead of numeric codes

Field	Details
Severity	🟠 High
Type	Bug
File	backend/envelope.py
Location	ERROR_CODE definition
Confidence	94%

Problem:
The ERROR_CODE mapping defines non-OK error codes as string values (e.g., "BAD_REQUEST": "BAD_REQUEST"). The rest of the system (including the frontend) expects numeric error codes to compare against zero. Returning string error codes can cause type mismatches, leading to client-side logic failures when checking code === 0 or handling specific error numbers.

Suggested Fix:
Replace the string values with appropriate numeric error codes that align with errors.ERROR_CODES, e.g., "BAD_REQUEST": 400, "VALIDATION_FAILED": 422, etc.

---

7. 🔒 Unsafe use of environment variable to access arbitrary logging attributes

Field	Details
Severity	🟠 High
Type	Security
File	backend/logging_config.py
Location	_resolve_log_level
Confidence	96%

Problem:
The function _resolve_log_level reads LOG_LEVEL or SMARTNODE_LOG_LEVEL from the environment and uses getattr(logging, raw, logging.INFO) to obtain the log level. An attacker can set the environment variable to any attribute name present in the logging module (e.g., 'getLogger', 'dict'), causing the function to return a non‑integer object. This value is later passed to logging.basicConfig and to max(), which can raise exceptions or alter logging behavior, leading to denial‑of‑service or information leakage.

Suggested Fix:
Validate the environment value against an explicit whitelist of allowed level names (DEBUG, INFO, WARNING, ERROR, CRITICAL) and map them to the corresponding logging constants, or fall back to a safe default.

---

8. 🐛 Incorrect use of internal _value attribute on Counter objects

Field	Details
Severity	🟠 High
Type	Bug
File	backend/metrics.py
Location	update_simulation_metrics
Confidence	97%

Problem:
The code attempts to increment Prometheus Counter metrics via the internal _value.inc() attribute (e.g., sim_total_requests_total._value.inc(...)). The Counter API provides a public inc() method directly on the Counter instance; the _value attribute is an internal implementation detail and may not exist, leading to an AttributeError at runtime and preventing metric updates.

Suggested Fix:
Replace the internal attribute access with the public Counter API: use sim_total_requests_total.inc(delta), sim_accepted_requests_total.inc(delta), and sim_completed_requests_total.inc(delta) (and similarly for other counters). Remove the # type: ignore[union-attr] comments as they are no longer needed.

---

9. 🐛 Incorrect distance used for GEO1-to-ground segment

Field	Details
Severity	🟠 High
Type	Bug
File	backend/orbit.py
Location	calculate_multi_hop_relay_rate
Confidence	96%

Problem:
In calculate_multi_hop_relay_rate the second argument to link_budget_relay (dist_geo_gs_km) is incorrectly set to dist1_km, which is the LEO‑to‑GEO1 distance. This causes the GEO1‑to‑ground link budget to be computed with the wrong distance, leading to inaccurate rate calculations.

Suggested Fix:
Compute the correct GEO1‑to‑ground distance using _slant_range_km(geo1_pos, {'lat': gs['lat'], 'lon': gs['lon'], 'alt': gs.get('alt', 0.0)}) and pass that value as dist_geo_gs_km.

---

10. 🐛 Unbounded growth of _hits dictionary causing memory leak

Field	Details
Severity	🟠 High
Type	Bug
File	backend/ratelimit.py
Location	SlidingWindowLimiter.check
Confidence	97%

Problem:
The limiter stores a list of timestamps for each key in the _hits dict, but never removes a key when its list becomes empty after pruning old entries. Over time, as new identities (e.g., IPs or JWT sub values) appear, the dictionary retains empty lists, leading to unbounded memory consumption.

Suggested Fix:
After pruning old timestamps, delete the key if the list is empty. For example:

with self._lock:
    q = self._hits.setdefault(key, [])
    while q and q[0] < cutoff:
        q.pop(0)
    if not q:
        del self._hits[key]
        return True, limit, 0.0
    ...

---

11. 🔒 Untrusted API base allows token leakage

Field	Details
Severity	🟠 High
Type	Security
File	frontend/app.js
Location	resolveApiBase / saveApiBase / fetchJson
Confidence	96%

Problem:
The application accepts an api query parameter (or a value from localStorage) to override the backend API base URL. This value is used directly in fetchJson and other fetch calls, and the JWT token from smartnode_token is automatically added to the Authorization header for every request. An attacker can craft a link such as https://victim.com/app.html?api=https://evil.com and trick a logged‑in user to visit it. The app will then send the user's JWT to the attacker‑controlled domain, leading to credential theft (SSRF / open redirect style vulnerability).

Suggested Fix:
Restrict the allowed API base to a whitelist of trusted origins or require explicit user confirmation before changing it. Alternatively, store the API base only in a secure configuration (e.g., environment variable) and do not allow it to be overridden via URL parameters.

---

12. 🐛 Versioned API paths not matched against public prefixes

Field	Details
Severity	🟡 Medium
Type	Bug
File	backend/auth.py
Location	is_public_path
Confidence	93%

Problem:
The function normalizes the path only for exact public path matching, but the subsequent prefix check uses the original path. For versioned endpoints like "/api/v1/frontend/...", the prefix "/frontend" is not recognized, causing the request to be treated as protected instead of public. This can lead to unexpected authentication failures for legitimate public resources.

Suggested Fix:
Apply the same normalization to the prefix check, e.g., replace the path with the normalized version before iterating over PUBLIC_PREFIXES, or include versioned prefixes in PUBLIC_PREFIXES.

---

13. 🐛 Duplicate request IDs can accumulate in satellite usage list

Field	Details
Severity	🟡 Medium
Type	Bug
File	backend/resources.py
Location	occupy_satellite_only
Confidence	96%

Problem:
The occupy_satellite_only method appends req_id to the satellite usage list without checking if it is already present. If the same request ID is occupied multiple times, release will remove only one occurrence, leaving stale entries that cause the resource to appear still occupied and can lead to incorrect scheduling or resource leakage.

Suggested Fix:
Add a duplicate check before appending, similar to the occupy method:

def occupy_satellite_only(self, req_id, sat_id):
    self.usage["satellites"].setdefault(sat_id, [])
    if req_id not in self.usage["satellites"][sat_id]:
        self.usage["satellites"][sat_id].append(req_id)

---

14. 🐛 Incompatible fallback format between to_yaml and from_yaml when PyYAML is unavailable

Field	Details
Severity	🟡 Medium
Type	Bug
File	backend/scenario.py
Location	ScenarioManager.to_yaml / ScenarioManager.from_yaml
Confidence	96%

Problem:
When PyYAML is not installed, to_yaml falls back to returning a JSON string prefixed with a comment line ("# PyYAML 不可用，以下为 JSON 格式\n"). However, from_yaml in the same fallback path attempts to parse the entire input with json.loads, which will fail because the leading comment makes the text invalid JSON. This causes a runtime error when loading a scenario saved via the fallback to_yaml method.

Suggested Fix:
Modify to_yaml to return pure JSON without the comment when PyYAML is unavailable, or adjust from_yaml to strip comment lines before JSON parsing. For example, replace the fallback return with return ScenarioManager.to_json(data) and document that the output is JSON when PyYAML is missing.

---

15. 🐛 Restoring request ID may set it to None

Field	Details
Severity	🟡 Medium
Type	Bug
File	backend/snapshot.py
Location	_rebuild_requests
Confidence	96%

Problem:
When rebuilding TransmissionRequest objects from a snapshot, the code unconditionally assigns req.id = d.get("id", req.id). If the snapshot contains an explicit null (None) for the id field, the request's id becomes None. Subsequent logic that assumes id is an integer can raise TypeError or cause incorrect behavior, leading to a failure during snapshot restoration.

Suggested Fix:
Only overwrite the id when the snapshot provides a non‑null integer value, e.g., if "id" in d and isinstance(d["id"], int): req.id = d["id"].

---

16. 🐛 Missing response.ok check leads to unhandled JSON parse errors

Field	Details
Severity	🟡 Medium
Type	Bug
File	frontend/app.js
Location	refreshRequestList
Confidence	94%

Problem:
In refreshRequestList the code performs await fetch(this.apiUrl(url), ...) and immediately calls await resp.json() without checking resp.ok. If the server returns a non‑2xx status with a non‑JSON body (e.g., HTML error page), resp.json() will throw, causing an unhandled promise rejection and breaking the UI. The function also does not handle error responses that contain a JSON payload with a non‑zero code field.

Suggested Fix:
Check resp.ok before parsing JSON and handle error cases. For example:

const resp = await fetch(this.apiUrl(url), { headers: { 'Content-Type': 'application/json' } });
if (!resp.ok) {
  const errText = await resp.text();
  throw new Error(`Request failed: ${resp.status} ${errText}`);
}
const payload = await resp.json();
if (payload.code !== 0) {
  throw new Error(payload.message || 'Unexpected error');
}
// proceed with payload.data

---

17. 🐛 Extra blank line creates malformed SSE frame

Field	Details
Severity	🔵 Low
Type	Bug
File	backend/stream.py
Location	_format_sse
Confidence	96%

Problem:
The _format_sse function appends two empty strings before joining with "\n", resulting in three consecutive newlines after the data section ("\n\n\n"). SSE frames should be terminated by a single blank line ("\n\n"). The extra newline causes an unintended empty event to be sent, which can confuse clients or lead to unnecessary processing.

Suggested Fix:
Remove one of the two lines.append("") calls so that only a single blank line is added before joining, e.g., keep only one lines.append("") before the return.

---

About this report

This report was generated using Advanced AI models.
Only findings with ≥90% confidence are included.
False positives are possible — use your own judgment.