Corregir crash al parsear Subject DN con comas en la razón social#7
Corregir crash al parsear Subject DN con comas en la razón social#7jonas1ara wants to merge 1 commit intoFiscalAPI:mainfrom
Conversation
El SAT emite certificados FIEL donde la razón social (O=) puede contener
comas (ej: "SEMILLAS, HORTALIZAS Y FLORES"). El código anterior hacía
Split(',') sobre el Subject completo y luego accedía a Split('=')[1]
directamente, lanzando IndexOutOfRangeException cuando un fragmento no
contenía '='.
Solución: usar Split('=', 2) con máximo 2 partes y filtrar fragmentos
sin '=' antes de mapear a KeyValuePair. Aplicado en SubjectKeyValuePairs
e IssuerKeyValuePairs.
Esto resolvio mi problema al tratar con una razon social que lleva una coma en el nombre
📝 WalkthroughWalkthroughCertificate issuer and subject parsing now uses Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Core/Certificate.cs (1)
132-138: Mismo defecto y duplicación de lógica conSubjectKeyValuePairs.
IssuerKeyValuePairsaplica exactamente la misma cadena de operaciones queSubjectKeyValuePairs(Lines 156–160) y, por tanto, hereda el mismo problema descrito arriba: si elIssuer(la AC del SAT) trae un atributo con coma escapada/entrecomillada, también se descarta silenciosamente. Al refactorizar para usarX500DistinguishedName(ver comentario enSubjectKeyValuePairs), extrae un único helper privado (p. ej.ParseDistinguishedName(X500DistinguishedName)) y reutilízalo en ambos getters: usa_x509Certificate2.IssuerNamey_x509Certificate2.SubjectNamepara no re-parsear strings ya descodificados por el runtime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Certificate.cs` around lines 132 - 138, Refactor IssuerKeyValuePairs and SubjectKeyValuePairs to avoid duplicating brittle string-splitting logic: create a private helper ParseDistinguishedName(X500DistinguishedName dn) that extracts key/value pairs from the runtime-parsed dn (handling escaped commas/quotes correctly), and have both getters call this helper using _x509Certificate2.IssuerName and _x509Certificate2.SubjectName respectively instead of re-parsing the raw Issuer/Subject strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Core/Certificate.cs`:
- Around line 156-160: The current SubjectKeyValuePairs/IssuerKeyValuePairs
logic naively splits Subject/Issuer strings on commas which corrupts values
containing escaped commas; replace both identical blocks with a shared helper
ParseDistinguishedName that accepts an X500DistinguishedName, calls
dn.Decode(X500DistinguishedNameFlags.UseNewLines), splits on newline characters
(not commas), then splits each line on '=' (limit 2), trims and returns
List<KeyValuePair<string,string>>; update SubjectKeyValuePairs and
IssuerKeyValuePairs to call
ParseDistinguishedName(_x509Certificate2.SubjectName) /
ParseDistinguishedName(_x509Certificate2.IssuerName) and add a unit test with
CN=FOO, O="ACME, S.A. DE C.V.", C=MX to assert Organization preserves the
embedded comma.
---
Nitpick comments:
In `@Core/Certificate.cs`:
- Around line 132-138: Refactor IssuerKeyValuePairs and SubjectKeyValuePairs to
avoid duplicating brittle string-splitting logic: create a private helper
ParseDistinguishedName(X500DistinguishedName dn) that extracts key/value pairs
from the runtime-parsed dn (handling escaped commas/quotes correctly), and have
both getters call this helper using _x509Certificate2.IssuerName and
_x509Certificate2.SubjectName respectively instead of re-parsing the raw
Issuer/Subject strings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| get => _x509Certificate2.Subject.Split(',') | ||
| .Select(x => new KeyValuePair<string, string>(x.Split('=')[0].Trim(), x.Split('=')[1].Trim())).ToList(); | ||
| .Select(x => x.Split('=', 2)) | ||
| .Where(parts => parts.Length == 2) | ||
| .Select(parts => new KeyValuePair<string, string>(parts[0].Trim(), parts[1].Trim())) | ||
| .ToList(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
X500DistinguishedName Decode UseNewLines .NET 8 escape comma
💡 Result:
In .NET 8, X500DistinguishedName supports formatting/decoding options via X500DistinguishedNameFlags. To control comma/newline escaping: 1) UseNewLines (for decoding/formatting) - X500DistinguishedNameFlags.UseNewLines (value 256) is the flag that indicates the distinguished name uses the new line character. This is part of the X500DistinguishedNameFlags enum used with X500DistinguishedName’s decode/encode operations. 2) UseCommas vs UseNewLines are mutually exclusive - The flags doc states you should use only one of UseCommas, UseNewLines, and UseSemicolons. 3) “escape comma” in the input string (parsing) - The X500DistinguishedName(string ...) constructor parses a textual DN using the platform parsing routine (it’s backed by an X500 name parsing implementation). If you pass an input like ...OU=Docs, Adatum..., the parser may fail with “invalid X500 name ... delimiter/value” depending on the exact escaping/quoting the parser expects. - There isn’t a dedicated “escape comma in the input string” flag. The flags primarily affect how .NET formats/decodes the DN, not whether it accepts arbitrary escaped-comma syntax in the constructor input. Practical guidance - If your goal is to round-trip and avoid comma-delimiter ambiguities, prefer building the DN with X500DistinguishedNameBuilder (stronger than parsing a raw escaped string). - If you must parse the raw input, you may need to rewrite/quote the escaped comma according to what the parser accepts (e.g., converting the escaped comma form into a quoted form around the attribute value). Relevant API surfaces - X500DistinguishedName.Decode(X500DistinguishedNameFlags) decodes using the specified characteristics. - X500DistinguishedNameFlags includes UseNewLines and UseCommas and documents their intended usage constraints.
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.x509certificates.x500distinguishednameflags?view=net-10.0
- 2: https://msdn.microsoft.com/library/system.security.cryptography.x509certificates.x500distinguishednameflags
- 3: https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.x509certificates.x500distinguishedname.decode?view=net-6.0
- 4: https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.x509certificates.x500distinguishedname.-ctor?view=net-9.0
- 5: https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.x509certificates.x500distinguishednamebuilder?view=net-10.0
🌐 Web query:
.NET X509Certificate2 Subject RFC 2253 escaped comma format
💡 Result:
In .NET, X509Certificate2.SubjectName gives you an X500DistinguishedName object (not a raw RFC 2253 string). The X500DistinguishedName API uses a “comma-delimited distinguished name” string format internally, and escaping rules for special characters follow the LDAP/RFC distinguished-name string escaping mechanism. For the specific case of an escaped comma in RFC 2253 (i.e., a comma that is part of the attribute value, not an RDN delimiter): - RFC 2253 says the comma character “,” must be escaped in attribute values by prefixing it with a backslash. - So the correct representation of a literal comma inside an attribute value is: , (backslash + comma). RFC 2253 escaping rule (comma in value): - The character to escape is one of: ",", "^", "", ... (including comma). If it is in that list, it is prefixed by a backslash. Otherwise it may be hex-escaped. Practical .NET guidance: 1) Prefer to avoid manual parsing/escaping of Subject strings. Use X509Certificate2.GetNameInfo(...) when you just need specific components, because it “should always be used instead of parsing the certificate”. 2) If you must round-trip/format the DN string yourself, use X500DistinguishedName and its Decode/Name/Format facilities rather than string operations. The X500DistinguishedNameFlags enum controls formatting (use commas/semicolons, etc.), but RFC 2253 escaping is still required for commas that belong to attribute values. Example (conceptual): - If the certificate subject has an attribute like CN=“R. Smith, O=Big Company” (comma inside CN value), RFC 2253 requires the comma be escaped in the string form as CN=R. Smith, O=Big Company (the comma after “Smith” is preceded by a backslash so it’s not treated as an RDN separator). Answer to your query wording (“.NET X509Certificate2 Subject RFC 2253 escaped comma format”): - The correct RFC 2253 escaped-comma syntax is: \, i.e. a literal “comma inside an attribute value” is represented as a backslash followed by a comma.
Citations:
- 1: https://datatracker.ietf.org/doc/html/rfc2253
- 2: https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.x509certificates.x509certificate2.subjectname?view=net-9.0
- 3: https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.x509certificates.x509certificate2.getnameinfo?view=net-9.0
- 4: https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.x509certificates.x500distinguishednameflags?view=net-10.0
- 5: https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.x509certificates.x500distinguishedname.decode?view=net-6.0
🌐 Web query:
X500DistinguishedName.Decode UseNewLines vs UseCommas practical example
💡 Result:
Practical difference - X500DistinguishedName.Decode(flag) uses the flag to choose the separator used when rendering RDNs (relative distinguished names) into a string. - X500DistinguishedNameFlags.UseCommas renders the DN using comma+space separators. - X500DistinguishedNameFlags.UseNewLines renders the DN using the platform new-line sequence instead of comma separators. These flags are mutually exclusive with each other (Microsoft notes only one of UseCommas/UseNewLines/UseSemicolons should be specified). What you should do in code 1) Create an X500DistinguishedName from the certificate/ASN.1 value (so the DN parsing is already done). 2) Call Decode(...) with either UseCommas or UseNewLines depending on how you want to display it. Example (C#) using System; using System.Security.Cryptography.X509Certificates; class Demo { static void Main { // Example: use the subject DN from a certificate you already have. // Replace with your own X509Certificate2. X509Certificate2 cert = GetSomeCertificate; X500DistinguishedName dn = new X500DistinguishedName(cert.SubjectName.Name); string commas = dn.Decode(X500DistinguishedNameFlags.UseCommas); string newLines = dn.Decode(X500DistinguishedNameFlags.UseNewLines); Console.WriteLine("-- commas --"); Console.WriteLine(commas); Console.WriteLine("-- new lines --"); Console.WriteLine(newLines); } static X509Certificate2 GetSomeCertificate => throw new NotImplementedException; } Notes about “UseNewLines vs UseCommas” - UseCommas affects only the rendered layout (separator) of the decoded string. - UseNewLines affects only the rendered layout (separator) of the decoded string. Common gotcha (input parsing vs output formatting) - The DN string constructor X500DistinguishedName(string, flags) is for parsing/encoding a DN into ASN.1. Separators/escaping in the input have to be valid for what the underlying encoder expects. - Separate from Decode formatting: if your input DN contains commas inside attribute values, you may need proper quoting/escaping; otherwise you can get “invalid X500 name … delimiter” exceptions. If you tell me your exact DN example and your target .NET version (e.g., .NET 6/7/8), I can show the safest way to display it (Decode) and the safest way to ingest it (constructor vs X500DistinguishedNameBuilder).
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.x509certificates.x500distinguishedname.decode?view=net-6.0
- 2: https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.x509certificates.x500distinguishednameflags?view=net-10.0
- 3: https://github.com/dotnet/dotnet/blob/b0f34d51fccc69fd334253924abd8d6853fad7aa/src/runtime/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X500DistinguishedName.cs
- 4: https://learn.microsoft.com/en-us/windows/win32/api/certenroll/ne-certenroll-x500nameflags
- 5: https://stackoverflow.com/questions/79910134/x500distinguishedname-doesnt-work-with-escaped-commas
🏁 Script executed:
# Check if the Certificate.cs file exists and review the current implementation
fd -e cs -p 'Certificate.cs' | head -5Repository: FiscalAPI/fiscalapi-credentials-net
Length of output: 119
🏁 Script executed:
# Read the Certificate.cs file to verify the implementation
cat -n Core/Certificate.cs | head -200Repository: FiscalAPI/fiscalapi-credentials-net
Length of output: 7611
🏁 Script executed:
# Search for the exact context around lines 134-138 and 156-160
sed -n '80,170p' Core/Certificate.cs | cat -nRepository: FiscalAPI/fiscalapi-credentials-net
Length of output: 3698
🏁 Script executed:
# Search for test files related to Certificate.cs to understand the PR context
fd -e cs -p 'Test.*Certificate|Certificate.*Test' | head -10Repository: FiscalAPI/fiscalapi-credentials-net
Length of output: 61
🏁 Script executed:
# Check git history for recent changes to Certificate.cs
git log --oneline -10 -- Core/Certificate.cs 2>/dev/null || echo "Git not available"Repository: FiscalAPI/fiscalapi-credentials-net
Length of output: 152
🏁 Script executed:
# Search for any mentions of "comma" or "SEMILLAS" in the repo (PR context)
rg -i "comma|semillas" --type cs | head -20Repository: FiscalAPI/fiscalapi-credentials-net
Length of output: 61
Silent data truncation when Organization name contains commas: requires proper RFC 2253 parsing
The crash is fixed (no more IndexOutOfRangeException), but Split(',') on RFC 2253–formatted Subject DNs causes silent truncation when attribute values contain commas. In RFC 2253, a comma within a value is escaped as \, (e.g., O=SEMILLAS\, HORTALIZAS Y FLORES, C=MX). Naively splitting by , fragments this into O=SEMILLAS\, HORTALIZAS Y FLORES, C=MX—the middle fragment is silently dropped by the Where(parts => parts.Length == 2) filter, leaving Organization with a truncated and corrupted value (SEMILLAS\ instead of the full name). This is worse than the original crash because it silently corrupts the Organization field, which is critical for Mexican SAT invoicing.
Recommended fix: Use X500DistinguishedName.Decode(X500DistinguishedNameFlags.UseNewLines) to render each RDN on a separate line, preserving escaped characters within values. Extract a shared helper method to eliminate duplication (lines 134–138 and 156–160 are identical):
public List<KeyValuePair<string, string>> IssuerKeyValuePairs
{
get => ParseDistinguishedName(_x509Certificate2.IssuerName);
}
public List<KeyValuePair<string, string>> SubjectKeyValuePairs
{
get => ParseDistinguishedName(_x509Certificate2.SubjectName);
}
private static List<KeyValuePair<string, string>> ParseDistinguishedName(X500DistinguishedName dn)
{
var formatted = dn.Decode(X500DistinguishedNameFlags.UseNewLines);
return formatted
.Split(new[] { '\r', '\n' }, StringSplitOptions.RemoveEmptyEntries)
.Select(line => line.Split('=', 2))
.Where(parts => parts.Length == 2)
.Select(parts => new KeyValuePair<string, string>(parts[0].Trim(), parts[1].Trim()))
.ToList();
}Add a test with CN=FOO, O="ACME, S.A. DE C.V.", C=MX to verify Organization returns the full name with the embedded comma intact.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Core/Certificate.cs` around lines 156 - 160, The current
SubjectKeyValuePairs/IssuerKeyValuePairs logic naively splits Subject/Issuer
strings on commas which corrupts values containing escaped commas; replace both
identical blocks with a shared helper ParseDistinguishedName that accepts an
X500DistinguishedName, calls dn.Decode(X500DistinguishedNameFlags.UseNewLines),
splits on newline characters (not commas), then splits each line on '=' (limit
2), trims and returns List<KeyValuePair<string,string>>; update
SubjectKeyValuePairs and IssuerKeyValuePairs to call
ParseDistinguishedName(_x509Certificate2.SubjectName) /
ParseDistinguishedName(_x509Certificate2.IssuerName) and add a unit test with
CN=FOO, O="ACME, S.A. DE C.V.", C=MX to assert Organization preserves the
embedded comma.
|
@jonas1ara muchasgracias por tu contribución, la revisaré ASAP :) QQ. Hiciste testing con razones sociales sin coma como parte del nombre / LegalName ? |
|
@mendozagit si, tengo 400 razones sociales y 1 sola tenia coma, en este momento ya descargue 14 sin coma y sigue trabajando bien. Revísala con calma, si ayuda a la librería, un gusto. |
2 participants
El SAT emite certificados FIEL donde la razón social (O=) puede contener
comas (ej: "SEMILLAS, HORTALIZAS Y FLORES"). El código anterior hacía
Split(',') sobre el Subject completo y luego accedía a Split('=')[1]
directamente, lanzando IndexOutOfRangeException cuando un fragmento no
contenía '='.
Solución: usar Split('=', 2) con máximo 2 partes y filtrar fragmentos
sin '=' antes de mapear a KeyValuePair. Aplicado en SubjectKeyValuePairs
e IssuerKeyValuePairs.
Esto resolvio mi problema al tratar con una razon social que lleva una coma en el nombre
Summary by CodeRabbit
Bug Fixes