Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR enhances POI import functionality by strengthening KML file parsing capabilities and adding POI-specific image field support throughout the mapping layer. Changes include network-link resolution for KML resources, updated data transfer objects with a new ChangesPOI Import Feature
Sequence DiagramsequenceDiagram
participant User as User
participant Web as Web Controller
participant KML as KML Provider
participant Remote as Remote KML Server
participant API as API Controller
participant View as View
User->>Web: POST ImportPOIs (file + type)
Web->>Web: Validate file & type
Web->>KML: ImportFile(stream)
KML->>KML: ExtractCoordinates (root)
KML->>KML: Find NetworkLink elements
loop Each NetworkLink
KML->>Remote: Fetch remote KML/KMZ
Remote-->>KML: KML/KMZ content
KML->>KML: ExtractCoordinates (remote)
end
KML-->>Web: Coordinates[]
Web->>API: Map coordinates to POI models
API->>API: Set PoiImage, clear ImagePath
API-->>Web: POI count
Web->>Web: Store result in TempData
Web-->>User: Redirect to POIs view
User->>Web: GET POIs
Web->>View: Render with Message/ErrorMessage
View-->>User: POI list with feedback
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Web/Resgrid.Web/Areas/User/Controllers/MappingController.cs (1)
336-355:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winValidate that the selected POI type belongs to the current department before import.
Line 336 only checks
TypeId > 0; it does not verify tenant ownership. A forgedTypeIdcan associate imported POIs with another department’s type.Suggested fix
if (modal.TypeId <= 0) ModelState.AddModelError("TypeId", "Please select a POI type before importing."); + + var poiType = modal.TypeId > 0 ? await _mappingService.GetTypeByIdAsync(modal.TypeId) : null; + if (poiType == null || poiType.DepartmentId != DepartmentId) + ModelState.AddModelError("TypeId", "Invalid POI type selected."); if (ModelState.IsValid) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Controllers/MappingController.cs` around lines 336 - 355, The code only checks modal.TypeId > 0 but must also verify the POI type belongs to the current department before importing; fetch the POI type by modal.TypeId (e.g., via the existing POI type service/repository used elsewhere in MappingController), confirm its DepartmentId (or OwnerId) matches the current user's department id/context, and if not call ModelState.AddModelError("TypeId", "Selected POI type is not valid for your department.") and abort the import (do not call _mappingService.SavePOIAsync). Place this validation immediately after the TypeId check and before calling _kmlProvider.ImportFile so forged TypeIds cannot be used.
🧹 Nitpick comments (2)
Web/Resgrid.Web/Areas/User/Views/Mapping/POIs.cshtml (1)
41-46: ⚡ Quick winAdd ARIA alert roles for status banners.
At Line 41 and Line 45, add
role="alert"so success/error feedback is announced immediately to screen readers.Suggested diff
- <div class="alert alert-success">@Model.Message</div> + <div class="alert alert-success" role="alert">@Model.Message</div> ... - <div class="alert alert-danger">@Model.ErrorMessage</div> + <div class="alert alert-danger" role="alert">@Model.ErrorMessage</div>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Views/Mapping/POIs.cshtml` around lines 41 - 46, Add ARIA alert roles to the success and error feedback divs so screen readers announce them immediately: update the <div> that renders Model.Message (the success banner) and the <div> that renders Model.ErrorMessage (the error banner) to include role="alert" attributes so both status banners are exposed to assistive technologies.Providers/Resgrid.Providers.Geo/KmlProvider.cs (1)
95-101: ⚖️ Poor tradeoffRefactor
ResolveNetworkLinkto useIHttpClientFactoryand make the call chain async-aware.The concerns are valid:
HttpClient per-call pattern — Creating a new
HttpClientper request inside ausingblock causes socket exhaustion under load. The codebase already usesIHttpClientFactoryelsewhere; use it here instead.Sync-over-async blocking — The
.GetAwaiter().GetResult()calls block the thread. SinceImportPOIsinMappingControlleris already async (line 319), consider exposing anImportFileAsyncoverload inIKmlProviderto allow proper await through. This avoids blocking in an async context.Refactoring the private
ResolveNetworkLinkhelper to async (and makingImportFileasync-aware) would be moderate effort but eliminates both issues in a single flow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Providers/Resgrid.Providers.Geo/KmlProvider.cs` around lines 95 - 101, The ResolveNetworkLink method currently creates a new HttpClient in a using and blocks with .GetAwaiter().GetResult(); change ResolveNetworkLink to be async (e.g., Task<...> ResolveNetworkLinkAsync) and use an IHttpClientFactory-injected HttpClient (request via _httpClientFactory.CreateClient()) with await on async calls (GetAsync, ReadAsStreamAsync) instead of blocking; add an async overload to IKmlProvider (e.g., ImportFileAsync) and update callers such as MappingController.ImportPOIs to await through the new async surface so no synchronous blocking remains and HttpClient usage is pooled via IHttpClientFactory.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Providers/Resgrid.Providers.Geo/KmlProvider.cs`:
- Around line 86-99: ResolveNetworkLink currently fetches an untrusted href
directly which enables SSRF and unbounded response sizes; fix by validating and
sanitizing href before any network call: in ResolveNetworkLink, parse href into
a Uri and require AbsoluteUri with Scheme == "http" or "https", reject any other
schemes, then resolve the hostname (Dns.GetHostAddresses or equivalent) and
reject addresses that are loopback, link‑local (169.254/16), or private RFC1918
ranges and known metadata IPs (e.g., 169.254.169.254); also enforce a max
response size by checking response.Content.Headers.ContentLength against a
configured cap and when reading the response stream only read up to the cap
(abort/truncate if exceeded) instead of loading the entire body, and ensure
redirects are limited/disabled as needed before calling httpClient.GetAsync;
reference symbols: ResolveNetworkLink, href, httpClient.GetAsync, and consider
adding a helper IsSafeRemoteUri used by ResolveNetworkLink for these checks.
- Line 103: The current KMZ detection using href.EndsWith(".kmz", ...) is
brittle; update the logic in KmlProvider.cs where isKmz is set to robustly
detect KMZs by first parsing the href into a Uri (use Uri.TryCreate and inspect
Uri.AbsolutePath for a .kmz suffix) and then falling back to content-based
checks: when fetching the resource, prefer checking the HTTP response
Content-Type for application/vnd.google-earth.kmz and
application/vnd.google-earth.kml+xml, and if Content-Type is missing or
ambiguous, peek the response stream's first bytes to detect ZIP magic bytes
(PK\x03\x04) to mark isKmz; replace the single EndsWith check with this combined
approach and ensure streams/requests are disposed correctly.
- Around line 41-55: Replace the System.Diagnostics.Debug.WriteLine call in
KmlProvider.ImportFile with Resgrid.Framework.Logging by adding "using
Resgrid.Framework.Logging;" and calling LogException or LogError (e.g.,
LogException) to capture the exception details; also remove the empty catch
blocks around ResolveNetworkLink and the bounds calculation and in those catches
call LogException/LogError with context (include the networkLink or bounds
variable and method name) so failures aren't silently swallowed—locate the
try/catch surrounding ResolveNetworkLink and the outer try/catch in ImportFile
to apply these changes.
In `@Web/Resgrid.Web.Services/Controllers/v4/MappingController.cs`:
- Around line 472-473: The mapping currently sets ImagePath = null while
assigning PoiImage = poiType.Image, which breaks older clients; update the
mapping in MappingController (the code that maps poiType → DTO where ImagePath
and PoiImage are set) to populate ImagePath with the same value as PoiImage
(e.g., ImagePath = poiType.Image, PoiImage = poiType.Image) so both fields carry
the icon URL during migration; apply the same change in the other mapping sites
noted (the other occurrences that set ImagePath = null and PoiImage =
poiType.Image).
In `@Web/Resgrid.Web/Areas/User/Controllers/MappingController.cs`:
- Around line 318-320: The ImportPOIs POST action in MappingController is
missing CSRF protection; add the [ValidateAntiForgeryToken] attribute above the
ImportPOIs method to enable anti-forgery validation for that state-changing POST
endpoint (ensure the view/form that posts to ImportPOIs includes the
anti-forgery token as well); locate the ImportPOIs method in MappingController
and annotate it with ValidateAntiForgeryToken to secure the endpoint.
---
Outside diff comments:
In `@Web/Resgrid.Web/Areas/User/Controllers/MappingController.cs`:
- Around line 336-355: The code only checks modal.TypeId > 0 but must also
verify the POI type belongs to the current department before importing; fetch
the POI type by modal.TypeId (e.g., via the existing POI type service/repository
used elsewhere in MappingController), confirm its DepartmentId (or OwnerId)
matches the current user's department id/context, and if not call
ModelState.AddModelError("TypeId", "Selected POI type is not valid for your
department.") and abort the import (do not call _mappingService.SavePOIAsync).
Place this validation immediately after the TypeId check and before calling
_kmlProvider.ImportFile so forged TypeIds cannot be used.
---
Nitpick comments:
In `@Providers/Resgrid.Providers.Geo/KmlProvider.cs`:
- Around line 95-101: The ResolveNetworkLink method currently creates a new
HttpClient in a using and blocks with .GetAwaiter().GetResult(); change
ResolveNetworkLink to be async (e.g., Task<...> ResolveNetworkLinkAsync) and use
an IHttpClientFactory-injected HttpClient (request via
_httpClientFactory.CreateClient()) with await on async calls (GetAsync,
ReadAsStreamAsync) instead of blocking; add an async overload to IKmlProvider
(e.g., ImportFileAsync) and update callers such as MappingController.ImportPOIs
to await through the new async surface so no synchronous blocking remains and
HttpClient usage is pooled via IHttpClientFactory.
In `@Web/Resgrid.Web/Areas/User/Views/Mapping/POIs.cshtml`:
- Around line 41-46: Add ARIA alert roles to the success and error feedback divs
so screen readers announce them immediately: update the <div> that renders
Model.Message (the success banner) and the <div> that renders Model.ErrorMessage
(the error banner) to include role="alert" attributes so both status banners are
exposed to assistive technologies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 24b051bc-c983-4392-afb4-164988ddad27
📒 Files selected for processing (9)
Providers/Resgrid.Providers.Geo/KmlProvider.csWeb/Resgrid.Web.Services/Controllers/v4/MappingController.csWeb/Resgrid.Web.Services/Models/v4/Mapping/GetMapDataResult.csWeb/Resgrid.Web.Services/Models/v4/Mapping/PoiResultModels.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/Resgrid.Web/Areas/User/Controllers/MappingController.csWeb/Resgrid.Web/Areas/User/Models/Mapping/POIsView.csWeb/Resgrid.Web/Areas/User/Views/Mapping/ImportPOIs.cshtmlWeb/Resgrid.Web/Areas/User/Views/Mapping/POIs.cshtml
Comment on lines
+41
to
55
| try | ||
| { | ||
| ResolveNetworkLink(networkLink, coordinates); | ||
| } | ||
| catch | ||
| { | ||
| // Skip failed network link resolution | ||
| } | ||
| } | ||
| } | ||
| } | ||
| catch | ||
| catch (Exception ex) | ||
| { | ||
|
|
||
| System.Diagnostics.Debug.WriteLine($"KmlProvider.ImportFile error: {ex.Message}"); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the framework logging API is the project-wide convention and locate its namespace.
rg -nP '\bLogging\.(LogException|LogError|LogInfo|LogDebug)\s*\(' --type=cs -C1 | head -n 60
echo '---'
fd -t f -e cs -i 'Logging' | headRepository: Resgrid/Core
Length of output: 3937
🏁 Script executed:
#!/bin/bash
# Verify the actual code in KmlProvider.cs
if [ -f "Providers/Resgrid.Providers.Geo/KmlProvider.cs" ]; then
echo "=== KmlProvider.cs exists ==="
wc -l Providers/Resgrid.Providers.Geo/KmlProvider.cs
echo ""
echo "=== Lines 40-60 ==="
sed -n '40,60p' Providers/Resgrid.Providers.Geo/KmlProvider.cs
echo ""
echo "=== Lines 70-85 ==="
sed -n '70,85p' Providers/Resgrid.Providers.Geo/KmlProvider.cs
echo ""
echo "=== Check imports/usings ==="
head -n 20 Providers/Resgrid.Providers.Geo/KmlProvider.cs
else
echo "File not found at expected path"
fd -t f -i 'KmlProvider.cs'
fiRepository: Resgrid/Core
Length of output: 1397
🏁 Script executed:
#!/bin/bash
# Check logging pattern in other provider files
echo "=== Logging usage in Provider files ==="
rg -l 'Logging\.(LogException|LogError|LogInfo|LogDebug)' Providers/ --type=cs | head -5
echo ""
echo "=== Sample provider logging pattern ==="
rg -A3 -B3 'Logging\.(LogException|LogError)' Providers/ --type=cs -m 3Repository: Resgrid/Core
Length of output: 20251
🏁 Script executed:
#!/bin/bash
# Verify if KmlProvider has a Logging using statement anywhere
echo "=== Full usings section of KmlProvider.cs ==="
sed -n '1,15p' Providers/Resgrid.Providers.Geo/KmlProvider.cs
echo ""
echo "=== Search for Logging usage in KmlProvider ==="
grep -i 'logging' Providers/Resgrid.Providers.Geo/KmlProvider.cs || echo "No Logging references found"
echo ""
echo "=== Verify ExtractCoordinates method has second empty catch ==="
sed -n '60,90p' Providers/Resgrid.Providers.Geo/KmlProvider.cs | grep -A10 -B2 'catch'Repository: Resgrid/Core
Length of output: 893
Replace Debug.WriteLine with Resgrid.Framework.Logging and log caught exceptions instead of silently swallowing them.
Per coding guidelines, all logging must use Resgrid.Framework.Logging.LogException() / LogError() / LogInfo() / LogDebug(). Currently, line 54 uses System.Diagnostics.Debug.WriteLine, which bypasses the logging pipeline and won't appear in production logs. Additionally, the empty catch blocks at lines 45–48 and 76–79 silently discard exceptions during network-link resolution and bounds calculation, making failures undiagnosable in production.
Proposed change
Add using Resgrid.Framework.Logging; to the imports, then:
- try
- {
- ResolveNetworkLink(networkLink, coordinates);
- }
- catch
- {
- // Skip failed network link resolution
- }
+ try
+ {
+ ResolveNetworkLink(networkLink, coordinates);
+ }
+ catch (Exception linkEx)
+ {
+ Logging.LogException(linkEx);
+ }
...
- catch (Exception ex)
- {
- System.Diagnostics.Debug.WriteLine($"KmlProvider.ImportFile error: {ex.Message}");
- }
+ catch (Exception ex)
+ {
+ Logging.LogException(ex);
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Providers/Resgrid.Providers.Geo/KmlProvider.cs` around lines 41 - 55, Replace
the System.Diagnostics.Debug.WriteLine call in KmlProvider.ImportFile with
Resgrid.Framework.Logging by adding "using Resgrid.Framework.Logging;" and
calling LogException or LogError (e.g., LogException) to capture the exception
details; also remove the empty catch blocks around ResolveNetworkLink and the
bounds calculation and in those catches call LogException/LogError with context
(include the networkLink or bounds variable and method name) so failures aren't
silently swallowed—locate the try/catch surrounding ResolveNetworkLink and the
outer try/catch in ImportFile to apply these changes.
Comment on lines
+86
to
+99
| private static void ResolveNetworkLink(NetworkLink networkLink, List<Coordinates> coordinates) | ||
| { | ||
| if (networkLink?.Link?.Href == null) | ||
| return; | ||
|
|
||
| var href = networkLink.Link.Href.OriginalString; | ||
| if (string.IsNullOrWhiteSpace(href)) | ||
| return; | ||
|
|
||
| using (var httpClient = new HttpClient { Timeout = System.TimeSpan.FromSeconds(30) }) | ||
| { | ||
| var response = httpClient.GetAsync(href).GetAwaiter().GetResult(); | ||
| if (!response.IsSuccessStatusCode) | ||
| return; |
Contributor
There was a problem hiding this comment.
Critical: SSRF risk — href comes from untrusted user-uploaded KML.
ResolveNetworkLink fetches arbitrary URLs taken straight from an uploaded KML file. A malicious KML can point a NetworkLink/Link/Href at internal resources (e.g., http://169.254.169.254/latest/meta-data/ for cloud metadata, http://localhost:<port>/..., intranet hosts, or non‑HTTP schemes), turning the import flow into a Server‑Side Request Forgery primitive. There is also no maximum response size, so a hostile or oversized resource can exhaust memory.
At minimum, validate that href is an absolute http/https URI, reject loopback / link‑local / private / metadata addresses, and cap Content-Length plus the bytes read from the response stream.
🛡️ Sketch of validation + size cap
- var href = networkLink.Link.Href.OriginalString;
- if (string.IsNullOrWhiteSpace(href))
- return;
-
- using (var httpClient = new HttpClient { Timeout = System.TimeSpan.FromSeconds(30) })
- {
- var response = httpClient.GetAsync(href).GetAwaiter().GetResult();
- if (!response.IsSuccessStatusCode)
- return;
+ var href = networkLink.Link.Href.OriginalString;
+ if (!IsSafeRemoteUri(href, out var uri))
+ return;
+
+ using (var httpClient = new HttpClient { Timeout = System.TimeSpan.FromSeconds(30) })
+ {
+ var response = httpClient.GetAsync(uri).GetAwaiter().GetResult();
+ if (!response.IsSuccessStatusCode)
+ return;
+
+ const long MaxBytes = 10 * 1024 * 1024; // 10 MB cap
+ if (response.Content.Headers.ContentLength is long len && len > MaxBytes)
+ return;Where IsSafeRemoteUri parses the string, rejects non‑http(s) schemes, and resolves the host to verify it is not loopback / private / link‑local / metadata. Also consider reading the stream into a length‑limited buffer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Providers/Resgrid.Providers.Geo/KmlProvider.cs` around lines 86 - 99,
ResolveNetworkLink currently fetches an untrusted href directly which enables
SSRF and unbounded response sizes; fix by validating and sanitizing href before
any network call: in ResolveNetworkLink, parse href into a Uri and require
AbsoluteUri with Scheme == "http" or "https", reject any other schemes, then
resolve the hostname (Dns.GetHostAddresses or equivalent) and reject addresses
that are loopback, link‑local (169.254/16), or private RFC1918 ranges and known
metadata IPs (e.g., 169.254.169.254); also enforce a max response size by
checking response.Content.Headers.ContentLength against a configured cap and
when reading the response stream only read up to the cap (abort/truncate if
exceeded) instead of loading the entire body, and ensure redirects are
limited/disabled as needed before calling httpClient.GetAsync; reference
symbols: ResolveNetworkLink, href, httpClient.GetAsync, and consider adding a
helper IsSafeRemoteUri used by ResolveNetworkLink for these checks.
Comment on lines
+472
to
+473
| ImagePath = null, | ||
| PoiImage = poiType.Image, |
Contributor
There was a problem hiding this comment.
Preserve ImagePath during migration to PoiImage to avoid breaking existing clients.
These lines hard-null ImagePath. For API compatibility, populate both fields (at least temporarily) so older clients continue rendering POI icons.
Suggested fix
- ImagePath = null,
+ ImagePath = poiType.Image,
PoiImage = poiType.Image,- ImagePath = null,
+ ImagePath = poiType.Image,
PoiImage = poiType.Image,- ImagePath = null,
+ ImagePath = poiType.Image,
PoiImage = poiType.Image,- ImagePath = null,
+ ImagePath = poiType.Image,
PoiImage = poiType.Image,Also applies to: 634-635, 654-655, 670-671
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Web/Resgrid.Web.Services/Controllers/v4/MappingController.cs` around lines
472 - 473, The mapping currently sets ImagePath = null while assigning PoiImage
= poiType.Image, which breaks older clients; update the mapping in
MappingController (the code that maps poiType → DTO where ImagePath and PoiImage
are set) to populate ImagePath with the same value as PoiImage (e.g., ImagePath
= poiType.Image, PoiImage = poiType.Image) so both fields carry the icon URL
during migration; apply the same change in the other mapping sites noted (the
other occurrences that set ImagePath = null and PoiImage = poiType.Image).
Member
Author
|
Approve |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit
Release Notes
New Features
Improvements