Skip to content

Fix Illegal Characters #132

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.

Sign up for GitHub

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

Merged
vnoves merged 1 commit into from
Nov 6, 2025
Merged

Fix Illegal Characters #132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 18 additions & 9 deletions Common_glTF_Exporter/Utils/RevitIniReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,17 +39,26 @@ public static List<string> GetAdditionalRenderAppearancePaths()

foreach (var p in paths)
{
string trimmedPath = p.Trim();
string cleanedPath = p.Trim().Trim('"');

// If already rooted, normalize it
if (Path.IsPathRooted(trimmedPath))
{
absolutePaths.Add(Path.GetFullPath(trimmedPath));
}
else
// Replace | with ; or split further if needed
cleanedPath = cleanedPath.Replace('|', ';');

foreach (var subPath in cleanedPath.Split(new[] { ';' }, StringSplitOptions.RemoveEmptyEntries))
{
// Combine with user profile to resolve relative path
string fullPath = Path.GetFullPath(Path.Combine(userProfile, trimmedPath));
string trimmedPath = subPath.Trim().Trim('"');

if (Path.GetInvalidPathChars().Any(c => trimmedPath.Contains(c)))
{
// Skip or log invalid path
Console.WriteLine($"Skipping invalid path: {trimmedPath}");
continue;
}
Comment on lines +51 to +56
Copy link

@coderabbitai coderabbitai bot Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Invalid character validation may be insufficient, and logging is inappropriate.

Two concerns:

  1. Path.GetInvalidPathChars() only returns null and control characters (0-31), but does NOT include characters like <, >, *, ? that are invalid in Windows paths. While the code handles | and ", other invalid characters might slip through and cause Path.GetFullPath() to throw on lines 59-60.

  2. Console.WriteLine is inappropriate for Revit plugins, as there's no console and the message will be lost. Consider using a proper logging framework or the Revit API's logging mechanisms.

Verify the sufficiency of the current validation:

🏁 Script executed:

#!/bin/bash
# Check if there's error handling around Path.GetFullPath calls
rg -nP --type=cs -C3 'Path\.GetFullPath\(' -g '!**/test/**'

Length of output: 1455


---

🌐 Web query:

```web
Path.GetInvalidPathChars C# .NET what characters does it return

💡 Result:

Path.GetInvalidPathChars returns a char[] clone of the internal RealInvalidPathChars array — on Windows .NET that list is:

  • '"' (double quote)
  • '<' (less-than)
  • '>' (greater-than)
  • '|' (vertical bar)
  • '\0' (null)
  • control characters U+0001 through U+001F

(So: '"', '<', '>', '|', '\0', and chars 1..31). The exact set can vary by platform and file system and the method's docs say it is not guaranteed to be complete. [1][2][3]

Sources:
[1] .NET Path.cs source (RealInvalidPathChars and InvalidFileNameChars lists).
[2] Microsoft docs: Path.GetInvalidPathChars (remarks about not guaranteed complete).
[3] Discussion / examples (Stack Overflow).

Invalid character validation is incomplete, and logging is inappropriate for Revit plugins.

Two concerns:

  1. Path.GetInvalidPathChars() returns ", <, >, |, null, and control characters, but does NOT include * or ?, which are also invalid in Windows paths. Characters that slip through validation could cause Path.GetFullPath() on lines 59–60 to throw an exception. Currently there is no try-catch around these calls to handle failures gracefully.

  2. Console.WriteLine() is inappropriate for Revit plugins, as there's no console and messages are lost. Consider using a proper logging framework or the Revit API's logging mechanisms.


string fullPath = Path.IsPathRooted(trimmedPath)
? Path.GetFullPath(trimmedPath)
: Path.GetFullPath(Path.Combine(userProfile, trimmedPath));
Comment on lines +58 to +60
Copy link

@coderabbitai coderabbitai bot Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Add exception handling for Path.GetFullPath().

Path.GetFullPath() can throw ArgumentException, PathTooLongException, NotSupportedException, and ArgumentNullException even after validation. If an invalid path slips through the validation at line 51, this will cause unhandled exceptions.

Wrap the path resolution in a try-catch block:

-                            string fullPath = Path.IsPathRooted(trimmedPath)
-                                ? Path.GetFullPath(trimmedPath)
-                                : Path.GetFullPath(Path.Combine(userProfile, trimmedPath));
-
-                            absolutePaths.Add(fullPath);
+                            try
+                            {
+                                string fullPath = Path.IsPathRooted(trimmedPath)
+                                    ? Path.GetFullPath(trimmedPath)
+                                    : Path.GetFullPath(Path.Combine(userProfile, trimmedPath));
+
+                                absolutePaths.Add(fullPath);
+                            }
+                            catch (Exception ex) when (ex is ArgumentException || ex is PathTooLongException || ex is NotSupportedException)
+                            {
+                                // Log using appropriate Revit logging mechanism
+                                // For now, skip the invalid path
+                            }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In Common_glTF_Exporter/Utils/RevitIniReader.cs around lines 58 to 60, the
Path.GetFullPath calls can still throw ArgumentException, PathTooLongException,
NotSupportedException or ArgumentNullException; wrap the path resolution in a
try-catch that catches those exceptions (or Exception if you prefer broader
safety), log or record the failure with the offending trimmedPath and exception
message, and skip or continue processing that entry (e.g., treat fullPath as
invalid and do not add it to results) so the parser does not crash when an
invalid path slips through validation.


absolutePaths.Add(fullPath);
}
}
Expand Down
4 changes: 3 additions & 1 deletion Common_glTF_Exporter/Windows/ErrorWindow.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,9 @@
</TextBlock>

<StackPanel Orientation="Horizontal"
VerticalAlignment="Bottom">
VerticalAlignment="Bottom"
HorizontalAlignment="Center"
Margin="0,0,0,15">
<Button Content="Report Error"
Style="{DynamicResource MainButtonStyle}"
Background="Transparent"
Expand Down
Loading