Potential fix for code scanning alert no. 24: Server-side request forgery - #27
Conversation
…gery Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Review changes with |
|
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information |
✅ Deploy Preview for larme ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
View changes in DiffLens |
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| const apiRes = await fetch(url.toString()); | ||
| const data = await apiRes.json(); | ||
|
|
||
| return res.status(apiRes.status).json(data); |
There was a problem hiding this comment.
The code does not check if the fetch to the external API was successful (e.g., apiRes.ok) before attempting to parse the response as JSON and returning it. If the external API returns a non-JSON response or an error status, this could result in an unhandled exception or misleading error propagation.
Recommendation:
Add a check for apiRes.ok and handle error responses explicitly:
if (!apiRes.ok) {
const errorText = await apiRes.text();
return res.status(502).json({ error: 'Failed to fetch AQICN data', details: errorText });
}This ensures that only successful responses are parsed as JSON and that errors are handled gracefully.
| const apiRes = await fetch(url.toString()); | ||
| const data = await apiRes.json(); | ||
|
|
||
| return res.status(apiRes.status).json(data); |
There was a problem hiding this comment.
Returning the status code from the external API directly (res.status(apiRes.status)) may expose internal details or result in inappropriate status codes for the client. For example, the external API might return a 403 or 429, which may not be meaningful or actionable for your API consumers.
Recommendation:
Map external API errors to appropriate client-facing status codes (e.g., 502 for upstream errors) and avoid leaking internal status codes directly.
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe ChangesAQICN API Proxy Hardening
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Vulnerable Libraries (4)
More info on how to fix Vulnerable Libraries in JavaScript. 👉 Go to the dashboard for detailed results. 📥 Happy? Share your feedback with us. |
PR Summary
|
|
View changes in DiffLens |
1 similar comment
|
View changes in DiffLens |
|
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information |
|
View changes in DiffLens |
| const apiRes = await fetch(url.toString()); | ||
| const data = await apiRes.json(); |
There was a problem hiding this comment.
Lack of timeout for external API request
The fetch call to the AQICN API does not specify a timeout. If the external service is slow or unresponsive, this could cause the handler to hang indefinitely, impacting performance and reliability.
Recommendation:
Use a timeout mechanism (e.g., AbortController) to ensure the request fails gracefully after a reasonable period:
const controller = new AbortController();
const timeout = setTimeout(() => controller.abort(), 5000);
try {
const apiRes = await fetch(url.toString(), { signal: controller.signal });
// ...
} finally {
clearTimeout(timeout);
}| console.error("AQICN handler error:", err); | ||
| res.status(500).json({ error: err.message }); |
There was a problem hiding this comment.
Error handling exposes internal error details
The error handler returns err.message directly to the client, which may leak internal error details or sensitive information.
Recommendation:
Return a generic error message to clients and log the detailed error internally:
console.error("AQICN handler error:", err);
res.status(500).json({ error: "Internal server error" });
Reviewer's GuideHardened the AQICN proxy handler against SSRF by validating/normalizing lat/lon query parameters and constructing the upstream URL with the URL API, while preserving the existing response forwarding behavior and aligning string quotes with project style. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
View changes in DiffLens |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces validation for the lat and lon query parameters in the AQICN API handler, ensuring they represent valid coordinates, and refactors the URL construction to use the URL object. The review feedback correctly identifies an edge case where empty or whitespace-only query parameters evaluate to 0 when converted to a number, thereby bypassing validation, and provides a code suggestion to handle this scenario.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const rawLat = Array.isArray(req.query.lat) | ||
| ? req.query.lat[0] | ||
| : req.query.lat; | ||
| const rawLon = Array.isArray(req.query.lon) | ||
| ? req.query.lon[0] |
There was a problem hiding this comment.
In JavaScript/TypeScript, Number(''), Number(' '), or Number(null) evaluates to 0. If a client sends empty or whitespace-only values for lat or lon (e.g., ?lat=&lon=), they will be parsed as 0, bypassing the intended validation and querying the coordinates 0,0 instead of returning a 400 Bad Request error.\n\nTo prevent this, ensure that rawLat and rawLon are non-empty, non-whitespace strings before converting them to numbers.
const rawLat = Array.isArray(req.query.lat) ? req.query.lat[0] : req.query.lat;\n const rawLon = Array.isArray(req.query.lon) ? req.query.lon[0] : req.query.lon;\n\n const lat = rawLat && rawLat.trim() !== '' ? Number(rawLat) : NaN;\n const lon = rawLon && rawLon.trim() !== '' ? Number(rawLon) : NaN;
|
 |
|
Overall Grade |
Security  Reliability  Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Ruby |  |
Jun 20, 2026 1:17a.m. | Review ↗ |
| Rust | |
Jun 20, 2026 1:17a.m. | Review ↗ |
| JavaScript |  |
Jun 20, 2026 1:17a.m. | Review ↗ |
| Scala | |
Jun 20, 2026 1:17a.m. | Review ↗ |
| Shell | |
Jun 20, 2026 1:17a.m. | Review ↗ |
| Secrets | |
Jun 20, 2026 1:17a.m. | Review ↗ |
| Terraform | |
Jun 20, 2026 1:17a.m. | Review ↗ |
| Swift | |
Jun 20, 2026 1:17a.m. | Review ↗ |
| SQL | |
Jun 20, 2026 1:17a.m. | Review ↗ |
| Code coverage |  |
Jun 20, 2026 1:17a.m. | Review ↗ |
| C & C++ | |
Jun 20, 2026 1:17a.m. | Review ↗ |
| C# | |
Jun 20, 2026 1:17a.m. | Review ↗ |
| Ansible | |
Jun 20, 2026 1:17a.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
airmerge | 4221863 | Jun 20 2026, 01:19 AM |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="api/aqicn.ts">
<violation number="1" location="api/aqicn.ts:15">
P1: Custom agent: **Require API Auth, Validation, and Tenant Scoping**
Empty string lat/lon query parameters bypass validation because `Number('')` returns `0`, which passes the bounds check. Empty values should be rejected with a 400 error instead of silently defaulting to `0,0`.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| ? req.query.lon[0] | ||
| : req.query.lon; | ||
|
|
||
| const lat = Number(rawLat); |
There was a problem hiding this comment.
P1: Custom agent: Require API Auth, Validation, and Tenant Scoping
Empty string lat/lon query parameters bypass validation because Number('') returns 0, which passes the bounds check. Empty values should be rejected with a 400 error instead of silently defaulting to 0,0.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At api/aqicn.ts, line 11:
<comment>Empty string lat/lon query parameters bypass validation because `Number('')` returns `0`, which passes the bounds check. Empty values should be rejected with a 400 error instead of silently defaulting to `0,0`.</comment>
<file context>
@@ -5,9 +5,23 @@ if (!TOKEN) throw new Error('Missing environment variable AQICN_TOKEN');
+ const rawLat = Array.isArray(req.query.lat) ? req.query.lat[0] : req.query.lat;
+ const rawLon = Array.isArray(req.query.lon) ? req.query.lon[0] : req.query.lon;
+
+ const lat = Number(rawLat);
+ const lon = Number(rawLon);
+
</file context>
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| BestPractice | 4 medium 2 minor |
| ErrorProne | 1 high |
| Security | 1 critical |
| CodeStyle | 1 minor |
| Complexity | 1 medium |
🟢 Metrics 9 complexity · -1 duplication
Metric Results Complexity 9 Duplication -1
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
PR Summary by QodoHarden AQICN proxy against SSRF via lat/lon validation and safe URL construction Description
Diagram
High-Level Assessment
Files changed (1)
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="api/aqicn.ts" line_range="8-25" />
<code_context>
+ ? req.query.lon[0]
+ : req.query.lon;
+
+ const lat = Number(rawLat);
+ const lon = Number(rawLon);
+
+ const isValidLat = Number.isFinite(lat) && lat >= -90 && lat <= 90;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Handle empty or missing lat/lon strings before numeric conversion
`Number(rawLat)` / `Number(rawLon)` interpret `''` as `0`, so `?lat=&lon=` would incorrectly pass the range check as `(0, 0)`. Consider explicitly rejecting empty or whitespace-only strings before conversion and returning the existing 400 response in those cases.
```suggestion
const rawLat = Array.isArray(req.query.lat)
? req.query.lat[0]
: req.query.lat;
const rawLon = Array.isArray(req.query.lon)
? req.query.lon[0]
: req.query.lon;
const hasValidRawLat =
typeof rawLat === "string" && rawLat.trim().length > 0;
const hasValidRawLon =
typeof rawLon === "string" && rawLon.trim().length > 0;
if (!hasValidRawLat || !hasValidRawLon) {
return res
.status(400)
.json({ error: "Invalid lat/lon query parameters" });
}
const lat = Number(rawLat);
const lon = Number(rawLon);
const isValidLat = Number.isFinite(lat) && lat >= -90 && lat <= 90;
const isValidLon = Number.isFinite(lon) && lon >= -180 && lon <= 180;
if (!isValidLat || !isValidLon) {
return res
.status(400)
.json({ error: "Invalid lat/lon query parameters" });
}
```
</issue_to_address>
### Comment 2
<location path="api/aqicn.ts" line_range="34-37" />
<code_context>
const data = await apiRes.json();
return res.status(apiRes.status).json(data);
} catch (err: any) {
- console.error('AQICN handler error:', err);
+ console.error("AQICN handler error:", err);
res.status(500).json({ error: err.message });
}
-}
</code_context>
<issue_to_address>
**suggestion:** Avoid relying on `err.message` for all error types in the catch handler
Since `err` is `any`, non-Error values (like strings or plain objects) won’t have a `message` property, so the response could be `{ error: undefined }`. Normalize the error first (e.g., check `err instanceof Error` and otherwise use a generic fallback message) so the client always receives a usable error string.
```suggestion
} catch (err: unknown) {
console.error("AQICN handler error:", err);
let message = "Internal server error";
if (err instanceof Error && err.message) {
message = err.message;
} else if (typeof err === "string") {
message = err;
}
res.status(500).json({ error: message });
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const rawLat = Array.isArray(req.query.lat) | ||
| ? req.query.lat[0] | ||
| : req.query.lat; | ||
| const rawLon = Array.isArray(req.query.lon) | ||
| ? req.query.lon[0] | ||
| : req.query.lon; | ||
|
|
||
| const lat = Number(rawLat); | ||
| const lon = Number(rawLon); | ||
|
|
||
| const isValidLat = Number.isFinite(lat) && lat >= -90 && lat <= 90; | ||
| const isValidLon = Number.isFinite(lon) && lon >= -180 && lon <= 180; | ||
|
|
||
| if (!isValidLat || !isValidLon) { | ||
| return res | ||
| .status(400) | ||
| .json({ error: "Invalid lat/lon query parameters" }); | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Handle empty or missing lat/lon strings before numeric conversion
Number(rawLat) / Number(rawLon) interpret '' as 0, so ?lat=&lon= would incorrectly pass the range check as (0, 0). Consider explicitly rejecting empty or whitespace-only strings before conversion and returning the existing 400 response in those cases.
| const rawLat = Array.isArray(req.query.lat) | |
| ? req.query.lat[0] | |
| : req.query.lat; | |
| const rawLon = Array.isArray(req.query.lon) | |
| ? req.query.lon[0] | |
| : req.query.lon; | |
| const lat = Number(rawLat); | |
| const lon = Number(rawLon); | |
| const isValidLat = Number.isFinite(lat) && lat >= -90 && lat <= 90; | |
| const isValidLon = Number.isFinite(lon) && lon >= -180 && lon <= 180; | |
| if (!isValidLat || !isValidLon) { | |
| return res | |
| .status(400) | |
| .json({ error: "Invalid lat/lon query parameters" }); | |
| } | |
| const rawLat = Array.isArray(req.query.lat) | |
| ? req.query.lat[0] | |
| : req.query.lat; | |
| const rawLon = Array.isArray(req.query.lon) | |
| ? req.query.lon[0] | |
| : req.query.lon; | |
| const hasValidRawLat = | |
| typeof rawLat === "string" && rawLat.trim().length > 0; | |
| const hasValidRawLon = | |
| typeof rawLon === "string" && rawLon.trim().length > 0; | |
| if (!hasValidRawLat || !hasValidRawLon) { | |
| return res | |
| .status(400) | |
| .json({ error: "Invalid lat/lon query parameters" }); | |
| } | |
| const lat = Number(rawLat); | |
| const lon = Number(rawLon); | |
| const isValidLat = Number.isFinite(lat) && lat >= -90 && lat <= 90; | |
| const isValidLon = Number.isFinite(lon) && lon >= -180 && lon <= 180; | |
| if (!isValidLat || !isValidLon) { | |
| return res | |
| .status(400) | |
| .json({ error: "Invalid lat/lon query parameters" }); | |
| } |
| } catch (err: any) { | ||
| console.error('AQICN handler error:', err); | ||
| console.error("AQICN handler error:", err); | ||
| res.status(500).json({ error: err.message }); | ||
| } |
There was a problem hiding this comment.
suggestion: Avoid relying on err.message for all error types in the catch handler
Since err is any, non-Error values (like strings or plain objects) won’t have a message property, so the response could be { error: undefined }. Normalize the error first (e.g., check err instanceof Error and otherwise use a generic fallback message) so the client always receives a usable error string.
| } catch (err: any) { | |
| console.error('AQICN handler error:', err); | |
| console.error("AQICN handler error:", err); | |
| res.status(500).json({ error: err.message }); | |
| } | |
| } catch (err: unknown) { | |
| console.error("AQICN handler error:", err); | |
| let message = "Internal server error"; | |
| if (err instanceof Error && err.message) { | |
| message = err.message; | |
| } else if (typeof err === "string") { | |
| message = err; | |
| } | |
| res.status(500).json({ error: message }); | |
| } |
|
View changes in DiffLens |
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
Code Review SummaryStatus: No New Issues Found | Recommendation: Review existing comments before merge All issues in this PR have been previously identified by other review bots. The key concerns are: Already-Flagged Issues Summary
Files Reviewed
Reviewed by laguna-m.1-20260312:free · Input: 233.3K · Output: 3.4K · Cached: 37.9K |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const rawLat = Array.isArray(req.query.lat) | ||
| ? req.query.lat[0] | ||
| : req.query.lat; | ||
| const rawLon = Array.isArray(req.query.lon) | ||
| ? req.query.lon[0] | ||
| : req.query.lon; | ||
|
|
||
| const lat = Number(rawLat); | ||
| const lon = Number(rawLon); | ||
|
|
||
| const isValidLat = Number.isFinite(lat) && lat >= -90 && lat <= 90; | ||
| const isValidLon = Number.isFinite(lon) && lon >= -180 && lon <= 180; | ||
|
|
||
| if (!isValidLat || !isValidLon) { | ||
| return res | ||
| .status(400) | ||
| .json({ error: "Invalid lat/lon query parameters" }); | ||
| } |
There was a problem hiding this comment.
🚩 Inconsistent validation patterns between API handlers
The api/aqicn.ts handler now validates lat/lon query parameters and uses new URL for safe URL construction, but api/owm/[type].ts:9-16 still uses the old pattern (req.query as any with raw string interpolation into URLs). This creates an inconsistency: one handler is hardened against malformed input while the other is not. Consider applying the same validation pattern to the OWM handler.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const lat = Number(rawLat); | ||
| const lon = Number(rawLon); |
There was a problem hiding this comment.
📝 Info: Number('') coerces to 0, which passes lat/lon validation
If lat or lon is passed as an empty string (e.g., ?lat=&lon=1), Number('') evaluates to 0, which is a valid latitude and longitude. This means an empty query parameter would be silently treated as 0 rather than rejected. In practice this is unlikely to be a real problem since 0,0 is a valid coordinate (Gulf of Guinea), but it's a subtle behavioral difference from what a user might expect when omitting a value.
Was this helpful? React with 👍 or 👎 to provide feedback.
Code Review by Qodo
1. Empty coords become zero
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| const lat = Number(rawLat); | ||
| const lon = Number(rawLon); | ||
|
|
||
| const isValidLat = Number.isFinite(lat) && lat >= -90 && lat <= 90; | ||
| const isValidLon = Number.isFinite(lon) && lon >= -180 && lon <= 180; | ||
|
|
||
| if (!isValidLat || !isValidLon) { | ||
| return res | ||
| .status(400) | ||
| .json({ error: "Invalid lat/lon query parameters" }); | ||
| } |
There was a problem hiding this comment.
1. Empty coords become zero 🐞 Bug ≡ Correctness
The handler uses Number(rawLat/rawLon) without rejecting empty strings, so requests like ?lat=&lon= get coerced to 0 and pass the finite/range checks, resulting in an upstream request for geo:0;0 instead of a 400. This returns data for the wrong location and masks malformed client requests.
Agent Prompt
### Issue description
`Number("")` (or whitespace) coerces to `0`, which is finite and in-bounds, so the handler treats empty query params as valid coordinates and fetches data for `(0,0)`.
### Issue Context
The current validation only checks `Number.isFinite()` and bounds after coercion.
### Fix Focus Areas
- api/aqicn.ts[8-25]
### Suggested fix
1. Reject arrays (optional; see separate note) and require `rawLat`/`rawLon` to be non-empty strings.
2. Trim before parsing and reject `""` / whitespace.
3. Then parse and bounds-check as you do today.
Example approach:
- if (typeof rawLat !== 'string' || rawLat.trim() === '') -> 400
- same for rawLon
- const lat = Number(rawLat.trim())
- const lon = Number(rawLon.trim())
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Approve with suggestions
This PR hardens the AQICN proxy against SSRF by validating lat/lon and constructing URLs safely. The fix is correct and addresses the alert. However, the same vulnerability exists in another endpoint, and the new validation logic lacks test coverage.
📄 Documentation Diagram
This diagram documents the refactored AQICN proxy handler with input validation and safe URL construction.
sequenceDiagram
participant Client
participant Handler as Vercel Handler (api/aqicn.ts)
participant AQICN as AQICN API
Client->>Handler: GET /api/aqicn?lat=...&lon=...
note over Handler: Extract & validate lat/lon<br/>(single value, finite, bounds)
alt invalid lat/lon
Handler-->>Client: 400 { error: "Invalid lat/lon query parameters" }
else valid
Handler->>Handler: Build URL using URL API + token
Handler->>AQICN: fetch(URL)
AQICN-->>Handler: Response
Handler-->>Client: Proxy status & body
end
🌟 Strengths
- Proper input validation (type, bounds) and secure URL construction via
URL/URLSearchParams. - Error response (400) clearly communicates invalid input.
| Priority | File | Category | Impact Summary (≤12 words) | Anchors |
|---|---|---|---|---|
| P2 | api/aqicn.ts | Bug | Error contract change may break callers | symbol:fetchAqicn, path:src/api.ts |
| P2 | api/aqicn.ts | Architecture | SSRF fix not applied to owm endpoint | path:api/owm/[type].ts |
| P2 | api/aqicn.ts | Testing | No tests for validation logic | - |
🔍 Notable Themes
- Incomplete hardening: The same SSRF pattern exists in
api/owm/[type].ts(unaddressed) and validation logic lacks unit tests, increasing regression risk.
💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
| const url = new URL(`https://api.waqi.info/feed/geo:${lat};${lon}/`); | ||
| url.searchParams.set("token", TOKEN); |
There was a problem hiding this comment.
P2 | Confidence: High
Speculative: This PR hardens api/aqicn.ts against SSRF by validating and constructing URLs safely. However, an identical vulnerable pattern exists in api/owm/[type].ts (lines 7–24), which directly interpolates unsanitized query parameters (lat, lon, start, end, type) into a fetch URL. This file is not modified in this PR but presents the same class of vulnerability (code‑scanning alert #24 likely only flagged aqicn.ts). To achieve consistent security posture, the same validation and safe‑URL construction should be applied to the OWM endpoint and any other endpoints that forward user‑supplied parameters to external APIs.
| if (!isValidLat || !isValidLon) { | ||
| return res | ||
| .status(400) | ||
| .json({ error: "Invalid lat/lon query parameters" }); | ||
| } |
There was a problem hiding this comment.
P2 | Confidence: Medium
Speculative: The PR changes the error contract for invalid lat/lon input from an external API error (or fetch failure) to a locally generated HTTP 400 response with a fixed JSON body. The external caller fetchAqicn (in src/api.ts) uses axios.get which treats non-2xx responses as promise rejections. If the calling code in Dashboard.tsx (or other consumers) does not have a catch handler or does not expect a 400, this change could cause unhandled promise rejections or misleading error displays. The exact impact depends on the error‑handling logic of consumers, which is not visible in the provided context. To verify, review the consumer's error handling and ensure it can gracefully handle HTTP 400 with the new error shape, or wrap the call in a try‑catch on the consumer side.
| const isValidLat = Number.isFinite(lat) && lat >= -90 && lat <= 90; | ||
| const isValidLon = Number.isFinite(lon) && lon >= -180 && lon <= 180; |
There was a problem hiding this comment.
P2 | Confidence: Medium
Speculative: The validation logic for lat/lon (array handling, Number conversion, finite check, and range bounds) has no corresponding unit tests. Given this change is a security fix intended to prevent SSRF, the lack of tests creates risk that edge cases (e.g., empty string ""→0, null→NaN, whitespace, non‑decimal numbers, scientific notation) are not correctly handled. Adding a small test suite (e.g., Vitest or Jest) covering valid, invalid, and boundary inputs would improve long‑term maintainability and reduce regression risk.
1 participant
Potential fix for https://github.com/LCSOGthb/AirMerge/security/code-scanning/24
To fix this without changing intended functionality, validate and normalize
lat/lonbefore building the outbound URL:latin[-90,90],lonin[-180,180]).URL/URLSearchParams(not raw string concatenation).400for invalid input, instead of attempting fetch.In
api/aqicn.ts, update the handler body around lines 8–10 to:URLwith fixed base and safe path segment using numeric values.No new dependency is required.
Suggested fixes powered by Copilot Autofix. Review carefully before merging.
Summary by Sourcery
Harden AQICN API proxy request handling by validating latitude/longitude query parameters before forwarding requests.
Bug Fixes:
Enhancements:
Summary by cubic
Hardened the AQICN proxy against SSRF by validating
lat/lonand building the request URL safely. Invalid coordinates now return 400, addressing code scanning alert #24.Bug Fixes
lat/lonvalues, parse as finite numbers, and enforce bounds (lat−90..90,lon−180..180).URL/URLSearchParamsinstead of string concatenation.Refactors
api/aqicn.tswith project formatters; no behavior change.Written for commit 4221863. Summary will update on new commits.