Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughReplaced manual date parts with DateTime for clave; expanded consultar with recepcion and comprobante endpoints and curl refactor; switched selected hashing to Argon2id; large genXML refactor to auto-calc totals, trim and validate XML via ValidadorXML; added certificate checks in signing; DB connect refactor and port config; params_set default override changed; ImageResize tool call removed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant API as consultar.php
participant H as Hacienda
rect rgb(232,245,255)
note right of API: Recepción lookup flow
C->>API: r=recepcion (clave, token, client_id)
API->>API: Build curl $args & headers
API->>H: GET /recepcion/{clave}
alt Network error
H--xAPI: curl_error
API-->>C: {Status: error, text: error}
else 200
H-->>API: body (maybe respuesta-xml)
API->>API: Decode, decode respuesta-xml if present, populate DetalleMensaje
API-->>C: {Status: code, text: objeto}
end
end
rect rgb(240,255,240)
note right of API: Comprobante lookup
C->>API: r=comprobante (clave, token, client_id)
API->>H: GET /comprobantes/{clave}
H-->>API: JSON/err
API-->>C: {Status: code, text: data|error}
end
sequenceDiagram
autonumber
participant S as Submitter
participant G as genXML.php
participant V as ValidadorXML
participant F as Firmadohaciendacr
rect rgb(255,250,230)
S->>G: genXML* (datos)
G->>G: Auto-calc per-line totals & taxes
G->>G: Assemble and trim XML
G->>V: validateXml(xml, consecutivo, version)
alt Invalid
V-->>G: {status:error, details}
G-->>S: {clave, xml, validacion:error}
else OK
V-->>G: {status:ok}
G-->>S: {clave, xml, validacion:ok}
end
end
rect rgb(255,240,245)
S->>F: insertaFirma(xml, publicKey, privateKey)
F->>F: Validate inputs present
F->>F: Extract Clave & Emisor ID from XML
F->>F: Read certificate cedula
alt Mismatch
F-->>S: 400 error (cedula/clave/emisor)
else Match
F-->>S: Signed XML
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR implements version 4.4.1 bugfixes for an electronic invoice system, focusing on XML generation improvements, password security upgrades, and database connectivity enhancements.
- Updates password hashing from PASSWORD_DEFAULT to PASSWORD_ARGON2ID for improved security
- Adds database port configuration support and auto-calculation features for XML invoice generation
- Implements comprehensive XML validation and enhanced API response handling
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| www/settings.php | Adds database port configuration with default fallback |
| api/tools/ValidadorXML.php | New XML validation tool for invoice documents |
| api/modules/users/module.php | Updates session key generation to use ARGON2ID hashing |
| api/modules/files/module.php | Comments out ImageResize tool usage |
| api/modules/db/module.php | Refactors database connection to support port configuration |
| api/core/tools.php | Enhances HTTP response handling and CLI mode detection |
| api/core/params.php | Simplifies parameter setting by removing override functionality |
| api/contrib/signXML/Firmadohaciendacr.php | Adds certificate validation against XML content |
| api/contrib/genXML/genXML.php | Major refactor with auto-calculation, validation, and XML optimization |
| api/contrib/facturador/companny_user.php | Updates password hashing to ARGON2ID |
| api/contrib/consultar/module.php | Adds new consultation endpoints |
| api/contrib/consultar/consultar.php | Implements separate functions for reception and invoice queries |
| api/contrib/consultar/README.md | Updates documentation for new endpoints |
| api/contrib/clave/clave.php | Improves error message formatting |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| unset($r["xml"]); | ||
| } | ||
| return (object) $r; | ||
| } |
There was a problem hiding this comment.
The validateXml method is missing a return statement when validation succeeds. This will return null instead of the expected success object.
| $xmlString .= ' | ||
| <OtrasSenas>' . $emisorOtrasSenas . '</OtrasSenas> | ||
| </Ubicacion>'; | ||
| $xmlString .= '<OtrasSenas>123456</OtrasSenas>'; |
There was a problem hiding this comment.
Hardcoded value '123456' for OtrasSenas should be replaced with a configurable parameter or removed if not needed.
| $xmlString .= '<OtrasSenas>123456</OtrasSenas>'; | |
| // Use a configurable value for OtrasSenas instead of a hardcoded one | |
| if (isset($emisorOtrasSenas) && $emisorOtrasSenas !== '') { | |
| $xmlString .= '<OtrasSenas>' . htmlspecialchars($emisorOtrasSenas, ENT_XML1 | ENT_COMPAT, 'UTF-8') . '</OtrasSenas>'; | |
| } |
api/contrib/genXML/genXML.php
Outdated
|
|
||
| // Se calcula de la siguiente manera: se obtiene de la sumatoria de los campos “Subtotal”, “Impuesto Neto”. | ||
|
|
||
| $calculados[$numLinea]['montoTotalLinea'] += ($calculados[$numLinea]['subTotal'] + $calculados[$numLinea]['montoTotalLinea']); |
There was a problem hiding this comment.
This calculation is incorrect. It's adding montoTotalLinea to itself, which will double the value. It should be: $calculados[$numLinea]['montoTotalLinea'] = $calculados[$numLinea]['subTotal'] + $calculados[$numLinea]['impuestoNeto'];
| $calculados[$numLinea]['montoTotalLinea'] += ($calculados[$numLinea]['subTotal'] + $calculados[$numLinea]['montoTotalLinea']); | |
| $calculados[$numLinea]['montoTotalLinea'] = $calculados[$numLinea]['subTotal'] + $calculados[$numLinea]['impuestoNeto']; |
api/contrib/genXML/genXML.php
Outdated
| <TotalDescuentos>' . $totalDescuentos . '</TotalDescuentos>'; | ||
| // agrega los campos del resumen que esten en el array calculados y no esten vacios | ||
| foreach (AUTO_CALCULAR as $campoResumen) { | ||
| if($calculados[$campoResumen]!='') { |
There was a problem hiding this comment.
The comparison should check if the value is not equal to 0 or empty, not string comparison. Use: if($calculados[$campoResumen] != 0) {
| if($calculados[$campoResumen]!='') { | |
| if($calculados[$campoResumen] != 0 && $calculados[$campoResumen] != '') { |
api/contrib/consultar/consultar.php
Outdated
| if( is_object($responseObj) ) { | ||
| // si hay respuesta de hacienda, procesar detalle de mensaje | ||
| if(isset($responseObj->{'respuesta-xml'})) { | ||
| $xmlRespuesta = base64_decode($response->{'respuesta-xml'}); |
There was a problem hiding this comment.
Variable $response should be $responseObj since $response is a string and $responseObj is the decoded JSON object.
| $xmlRespuesta = base64_decode($response->{'respuesta-xml'}); | |
| $xmlRespuesta = base64_decode($responseObj->{'respuesta-xml'}); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
api/core/params.php (3)
32-35: Avoid trim() on non-strings to prevent TypeError and mishandling of false/array valuesIf
$params[$p]isfalseor an array (possible after recent setter changes),trim()will raise a TypeError on PHP 8+. Return non-string values as-is; only trim strings.- if (isset($params[$p]) && trim($params[$p]) != '') - return $params[$p]; - else - return $def; + if (array_key_exists($p, $params)) { + $v = $params[$p]; + if (is_string($v)) { + return trim($v) !== '' ? $v : $def; + } + return $v; + } + return $def;
80-83: Remove stray stdout write in CLI path
echo "loading";emits unsolicited output and can break scripts that parse CLI output. Usegrace_debugonly.- echo "loading"; + // no-op: rely on grace_debug above
102-102: Do not dump parameter definitions to stdout; leaks secrets
print_r($params);may expose sensitive config on CI logs or terminals.- print_r($params); + // Debug print removed to avoid leaking configurationapi/contrib/facturador/companny_user.php (4)
134-141: Fix two issues: insecure session token and SQL injection via dynamic table name.
- Session key: same weakness as users module—don’t derive from time()/rand() or use password_hash for tokens.
- Dynamic table: db_escape() is for values, not identifiers; current string concatenation allows identifier injection.
Apply:
-function companny_users_generateSessionKey($idUser, $idMasterUser) { - $q = sprintf("delete from " . db_escape($idMasterUser) . "_master_sessions where idUser='" . db_escape($idUser) . "'"); - db_query($q, 0); - - $sessionKey = password_hash(time() * rand(0, 1000), PASSWORD_ARGON2ID); - - $q = sprintf("INSERT INTO " . db_escape($idMasterUser) . "_master_sessions (idUser, sessionKey, ip, lastAccess) " - . "VALUES('%s', '%s', '%s', '%s')", db_escape($idUser), db_escape($sessionKey), db_escape($_SERVER['REMOTE_ADDR']), time()); +function companny_users_generateSessionKey($idUser, $idMasterUser) { + // Strictly validate identifier suffix to prevent SQL injection via table name + if (!preg_match('/^\d+$/', (string)$idMasterUser)) { + return ERROR_BAD_REQUEST; + } + $table = sprintf("`%s_master_sessions`", $idMasterUser); + $q = sprintf("DELETE FROM %s WHERE idUser='%s'", $table, db_escape($idUser)); + db_query($q, 0); + + // High-entropy session token + $sessionKey = bin2hex(random_bytes(32)); + + $q = sprintf( + "INSERT INTO %s (idUser, sessionKey, ip, lastAccess) VALUES('%s','%s','%s','%s')", + $table, db_escape($idUser), db_escape($sessionKey), db_escape($_SERVER['REMOTE_ADDR']), time() + ); db_query($q, 0); return $sessionKey; }Follow-up: Apply the same identifier hardening to SELECT/DELETE in companny_users_confirmSessionKey (Lines 216–221) and companny_users_destroySession (Line 245).
105-106: Do not log plaintext or hashed passwords. Remove this debug line.This leaks sensitive data into logs.
Apply:
- grace_debug("### VAlidacion: " . $compannyUser->pwd . " Lo que ingresa" . params_get("pwd")); + // Removed sensitive debug logging
107-115: Migrate legacy hashes to Argon2id on successful login and rehash if needed.You accept MD5 for backward compatibility but don’t upgrade the stored hash. Rehash on both Argon2id verify (if parameters changed) and MD5 path.
Apply:
- if (password_verify(params_get('pwd', ''), $compannyUser->pwd)) { + if (password_verify(params_get('pwd', ''), $compannyUser->pwd)) { + // Upgrade hash if parameters/algorithm changed + if (password_needs_rehash($compannyUser->pwd, PASSWORD_ARGON2ID)) { + $compannyUser->pwd = password_hash(params_get('pwd', ''), PASSWORD_ARGON2ID); + _companny_users_update((array)$compannyUser, $idMasterUser); + } // Create a token grace_debug("Able to login"); return array('sessionKey' => companny_users_generateSessionKey($compannyUser->idUser, $idMasterUser), 'userName' => $compannyUser->userName, 'idUser' => $compannyUser->idUser); - } else if ($compannyUser->pwd == md5(params_get('pwd', ''))) { + } else if ($compannyUser->pwd == md5(params_get('pwd', ''))) { // Create a token grace_debug("Able to login"); + // Migrate from MD5 to Argon2id + $compannyUser->pwd = password_hash(params_get('pwd', ''), PASSWORD_ARGON2ID); + _companny_users_update((array)$compannyUser, $idMasterUser); return array('sessionKey' => companny_users_generateSessionKey($compannyUser->idUser, $idMasterUser), 'userName' => $compannyUser->userName, 'idUser' => $compannyUser->idUser);
211-238: Sessions expire by lastAccess, but you never update lastAccess—either re-enable the updater or update here.Without touching lastAccess, sessions may expire prematurely.
Apply:
- } else { + } else { # Lets confirm the time frame if (conf_get('sessionLifetime', 'users') != -1) { if ((time() - $r->lastAccess) > conf_get('sessionLifetime', 'users')) { grace_debug("User last access is to old"); return false; } - return $r->idUser; + // Refresh last access on valid use + if (!preg_match('/^\d+$/', (string)$idMasterUser)) { + return ERROR_BAD_REQUEST; + } + $table = sprintf("`%s_master_sessions`", $idMasterUser); + $upd = sprintf("UPDATE %s SET lastAccess = '%s' WHERE sessionKey = '%s'", $table, time(), db_escape(params_get('sessionKey',''))); + db_query($upd, 0); + return $r->idUser; } else { return $r->idUser; } }api/contrib/signXML/Firmadohaciendacr.php (1)
201-206: Wrong call to member method and hex prefix handling.
- stringHex2StringDec must be called as $this->stringHex2StringDec.
- Strip the 0x prefix before conversion.
- if (strpos($certData['serialNumber'], "0x") === false) { + if (strpos($certData['serialNumber'], "0x") === false) { // https://bugs.php.net/bug.php?id=77411 $serialNumber = $certData['serialNumber']; } else { - $serialNumber = stringHex2StringDec($certData['serialNumber']); + $serialNumber = $this->stringHex2StringDec(preg_replace('/^0x/i', '', $certData['serialNumber'])); }api/contrib/consultar/consultar.php (1)
144-145: Undefined variable $apiTo in error response.The variable
$apiTois undefined in the error response array. It should be$url.- "to" => $apiTo, + "to" => $url,
🧹 Nitpick comments (11)
api/core/params.php (1)
88-116: Rename local$params(param definitions) to avoid shadowing global$params(actual config)Shadowing increases bug risk and readability cost. Use
$paramDefs(or similar) for the CLI schema.- # The actual params to be considered - $params = array(); + # The actual param definitions to be considered + $paramDefs = array(); - # Extract only the params - foreach ($allParams as $param) { - if (isset($param['params'])) - $params[] = $param['params']; + # Extract only the params + foreach ($allParams as $param) { + if (isset($param['params'])) + $paramDefs[] = $param['params']; } - # For some reason they are stored in pos 0 of the array - $params = $params[0]; + # For some reason they are stored in pos 0 of the array + $paramDefs = $paramDefs[0]; - foreach ($params as $p) { + foreach ($paramDefs as $p) { $longOpt = $p['key']; $opts .= $p['cli']; # Is it mandatory? if ($p['req']) { $opts .= ":"; $longOpt .= ":"; } else { $opts .= "::"; $longOpt .= "::"; } $longOpts[] = $longOpt; } @@ - foreach ($params as $p) { + foreach ($paramDefs as $p) { # Was it sent as longOpt? if (isset($args[$p['key']])) params_set($p['key'], $args[$p['key']]); else if (isset($args[$p['cli']])) params_set($p['key'], $args[$p['cli']]); else if ($p['req'] == false) # If it is optional, I will load the default value? params_set($p['key'], $p['def']); } @@ - params_verifyRequest($params); + params_verifyRequest($paramDefs);Also applies to: 124-132
api/contrib/facturador/companny_user.php (1)
5-10: Harden getIdUser() against empty results.db_query($q, 2) may return an empty array; accessing $result[0] will notice/warn.
Apply:
function getIdUser($sessionKey) { $q = "SELECT `idUser` FROM `sessions` WHERE `sessionKey`='" . db_escape($sessionKey) . "'"; $result = db_query($q, 2); - $idUser = $result[0]->idUser; - return $idUser; + if (!is_array($result) || count($result) === 0) { + return 0; + } + return (int)$result[0]->idUser; }api/contrib/clave/clave.php (1)
88-88: Echoing raw user input in error string; consider sanitization or consistent pattern.Including [$consecutivo] in the message helps debugging but can introduce noisy logs or, if rendered unsafely in HTML, XSS risk. Either:
- keep JSON-only responses and ensure frontend escapes, or
- wrap the value with a safe formatter, or
- remove the echo to match other numeric checks.
api/modules/files/module.php (1)
159-159: Removing ImageResize loader may break files_resizeImg at runtime.files_resizeImg instantiates \Eventviva\ImageResize. If autoloading isn’t configured, this will now fatal.
- Option A: lazy-load inside files_resizeImg.
- Option B: ensure Composer autoload provides Eventviva\ImageResize.
Example lazy-load (outside this hunk, at start of files_resizeImg):
if (!class_exists('\Eventviva\ImageResize')) { tools_useTool('ImageResize.php'); }api/tools/ValidadorXML.php (1)
37-44: Docblock parameter mismatch and naming clarity.@params mention $tipo_doc, but the signature receives $consecutivo and infers tipo from it. Update docblock to avoid confusion, or accept $tipo_doc explicitly.
- * @param string $tipo_doc - tipo de documento a validar + * @param string $consecutivo - consecutivoFinal (20 dígitos) del comprobante; se usa para derivar el tipo_docapi/contrib/signXML/Firmadohaciendacr.php (2)
162-166: Timezone literal is hardcoded; prefer from config.Consider conf_get('timezone', 'core', '-06:00') to avoid surprises across deployments.
256-299: String-based XML assembly/insertion is brittle; prefer DOM for signature injection.Direct string replace of closing tags can break on formatting/namespace differences. Consider building nodes with DOMDocument and appendChild at the document element.
I can provide a DOM-based injection patch if desired.
api/contrib/consultar/consultar.php (1)
115-116: Inconsistent error message for client_id validation.The error message in
consultarComprobante()is generic ("Ha ocurrido un error en el client_id") whileconsultarRecepcion()provides the actual client_id value. Consider making them consistent.- return "Ha ocurrido un error en el client_id."; + return "El client_id proporcionado (".params_get("client_id").") no es válido.";api/contrib/genXML/genXML.php (3)
769-783: Consider extracting tax rate mapping to a constant.The tax rate mapping could be defined as a class constant for better maintainability and reusability across different document types.
+const TARIFA_IMPUESTO_MAP = [ + "01" => 0, // 0% (Artículo 32, num 1, RLIVA) + "02" => 0.01, // Tarifa reducida 1% + "03" => 0.02, // Tarifa reducida 2% + "04" => 0.04, // Tarifa reducida 4% + "05" => 0, // Transitorio 0% + "06" => 0.04, // Transitorio 4% + "07" => 0.08, // Tarifa transitoria 8% + "08" => 0.13, // Tarifa general 13% + "09" => 0.005, // Tarifa reducida 0.5% + "10" => 0, // Tarifa Exenta + "11" => 0 // Tarifa 0% sin derecho a crédito +]; // Then use it as: -$impuestoPorTarifa = [/* ... */]; -$calculados[$l]['tarifa'] = round($impuestoPorTarifa[$i->codigoTarifa], 2)*100; +$calculados[$l]['tarifa'] = round(self::TARIFA_IMPUESTO_MAP[$i->codigoTarifa], 2)*100;
189-205: Consider removing commented-out code blocks.There are large blocks of commented-out code for the manual total parameters. Since you're moving to auto-calculation, these should be removed to keep the code clean.
- /* - $totalServGravados = params_get("total_serv_gravados"); - // ... (all the commented parameters) - $totalComprobante = params_get("total_comprobante"); - */ - - $totalDescuentos = params_get("total_descuentos"); + $totalDescuentos = params_get("total_descuentos");Also applies to: 1237-1256
776-776: Fix typo in comment.There's a small typo in the tarifa comment.
- "09" => 0.05, // Tarifa reducida 0.5% + "09" => 0.005, // Tarifa reducida 0.5%
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
api/contrib/clave/clave.php(1 hunks)api/contrib/consultar/README.md(1 hunks)api/contrib/consultar/consultar.php(3 hunks)api/contrib/consultar/module.php(1 hunks)api/contrib/facturador/companny_user.php(3 hunks)api/contrib/genXML/genXML.php(48 hunks)api/contrib/signXML/Firmadohaciendacr.php(3 hunks)api/core/params.php(1 hunks)api/core/tools.php(2 hunks)api/modules/db/module.php(2 hunks)api/modules/files/module.php(1 hunks)api/modules/users/module.php(1 hunks)api/tools/ValidadorXML.php(1 hunks)www/settings.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
api/contrib/signXML/Firmadohaciendacr.php (1)
api/core/tools.php (1)
tools_reply(19-111)
api/modules/users/module.php (1)
api/modules/crypto/crypto.php (1)
crypto_encrypt(32-48)
api/modules/db/module.php (2)
api/core/conf.php (1)
conf_get(26-34)api/core/grace.php (1)
grace_debug(101-105)
api/core/tools.php (1)
api/core/conf.php (1)
conf_get(26-34)
api/contrib/consultar/consultar.php (1)
api/core/params.php (1)
params_get(24-36)
api/tools/ValidadorXML.php (1)
api/core/conf.php (1)
conf_get(26-34)
api/contrib/genXML/genXML.php (3)
api/core/tools.php (2)
tools_useTool(175-183)tools_reply(19-111)api/core/params.php (1)
params_get(24-36)api/tools/ValidadorXML.php (2)
ValidadorXML(3-97)validateXml(43-96)
🪛 LanguageTool
api/contrib/consultar/README.md
[misspelling] ~1-~1: Si es adjetivo o nombre, se escribe con tilde.
Context: # Modulo para hacer comprobar estado de los comp...
(DIACRITICS_02)
[uncategorized] ~1-~1: Probablemente falta un determinante.
Context: # Modulo para hacer comprobar estado de los comprobantes y su estado de rece...
(AI_ES_GGEC_MISSING_DETERMINER)
[uncategorized] ~1-~1: Probablemente falta un signo diacrítico.
Context: ...tado de los comprobantes y su estado de recepcion Se envia: * w : consultar * r : r...
(AI_ES_GGEC_MISSING_ORTHOGRAPHY_DIACRITIC)
[uncategorized] ~3-~3: Probablemente falta un signo diacrítico.
Context: ...mprobantes y su estado de recepcion Se envia: * w : consultar * r : recepcion o...
(AI_ES_GGEC_MISSING_ORTHOGRAPHY_DIACRITIC)
[uncategorized] ~6-~6: Probablemente falta un signo diacrítico.
Context: ...n Se envia: * w : consultar * r : recepcion o comprobante * clave : Clave numeric...
(AI_ES_GGEC_MISSING_ORTHOGRAPHY_DIACRITIC)
[uncategorized] ~7-~7: Probablemente falta un signo diacrítico.
Context: ...cepcion o comprobante * clave : Clave numerica del comprobante * token: Token obteni...
(AI_ES_GGEC_MISSING_ORTHOGRAPHY_DIACRITIC)
[typographical] ~8-~8: Mayúsculas y minúsculas recomendadas.
Context: ...ave numerica del comprobante * token: Token obtenido de Hacienda
(AI_ES_GGEC_REPLACEMENT_CASING_UPPERCASE)
🪛 GitHub Actions: PHPUnit Tests
api/contrib/genXML/genXML.php
[error] 33-33: Call to undefined function tools_useTool() in api/contrib/genXML/genXML.php:33. Command: './vendor/bin/phpunit --stderr --debug --bootstrap vendor/autoload.php tests/api_contrib_genXML_FE.php' (exit code 255).
🔇 Additional comments (17)
api/modules/db/module.php (1)
51-52: Remove BC alias; no references todb_Connect()found
Ranrg -nP '\bdb_Connect\s*\(' -C2and found zero matches—renaming to
conectarBd()is safe.api/core/params.php (1)
41-41: Verify backward compatibility for removed$overrideparameterAt api/core/params.php (line 41), the signature now reads
function params_set($p, $val = false)removing the third
$overrideargument. An internal search found no three-argument calls, but this is a public API change that will trigger PHP 8+ ArgumentCountError for any downstream callers and drops the original “do-not-overwrite” semantics. Confirm no external/plugins depend on this parameter or reintroduce it (with deprecation) to prevent runtime errors and behavior drift.api/contrib/facturador/companny_user.php (1)
412-415: Argon2id adoption looks good—ensure environment support and set a rehash policy.
- Keep Argon2id, but verify the runtime supports it; fall back to PASSWORD_DEFAULT if not.
- Pair with the login-side migration (see Lines 107–115).
Run-time check to confirm support:
// At bootstrap if (!defined('PASSWORD_ARGON2ID')) { /* log/warn */ }Optional hardening:
-$pwd = password_hash($compannyUserDets['pwd'], PASSWORD_ARGON2ID); +// Optionally tune costs to your infra; defaults are fine if unsure. +$pwd = password_hash($compannyUserDets['pwd'], PASSWORD_ARGON2ID);api/core/tools.php (1)
88-99: Behavior change: tools_reply now returns JSON in web mode instead of exiting. Verify all call sites.Previously, callers could assume tools_reply printed and terminated. Now in non-CLI it returns a string and does not exit, which can cause duplicate outputs or continued execution after an intended error.
- Either keep exiting in both modes, or
- Document this change and update all call sites to echo/return the JSON and stop execution.
Would you like me to scan the repository for direct calls to tools_reply and flag those that rely on termination semantics?
api/contrib/consultar/consultar.php (3)
33-34: Include the client_id value in the error message.Good improvement for debugging - now the error message includes the actual client_id that was provided, making troubleshooting easier.
36-53: Good refactoring of curl options into a separate array.Moving curl options to a separate
$argsarray improves code readability and maintainability. The removal of the Postman-Token header is also appropriate for production code.
59-67: Improved error handling with standardized response structure.The error handling now returns a consistent associative array with Status, to, and text fields, which provides better structure for API responses.
api/contrib/consultar/module.php (2)
34-44: Routing updates align correctly with renamed functions.The routing configuration correctly updates the endpoint from
consultarComtorecepcionwith the corresponding actionconsultarRecepcion, maintaining backward compatibility concerns should be considered.
45-56: New comprobante endpoint properly configured.The new
comprobanteroute is correctly configured with the same required parameters and access controls as the reception endpoint.api/contrib/consultar/README.md (1)
6-8: Documentation correctly reflects the API changes.The updated documentation accurately describes the new endpoints (
recepcionandcomprobante) and clarifies that the token is obtained from Hacienda.api/contrib/genXML/genXML.php (7)
35-133: Excellent addition of auto-calculation helper functions.The new helper functions (
array_all,array_any,validarLinea,autoCalcularLinea) and theAUTO_CALCULARconstant provide a robust framework for automatic calculation of totals. This reduces manual calculation errors and improves maintainability.
165-165: Removal of deprecated parameter is good cleanup.Removing the deprecated
omitir_receptorparameter helps clean up the codebase.
269-279: Good validation for required medios_pago field.The validation ensures that
medios_pagois provided when the payment condition requires it (not credit, consignment, or other special conditions).
549-910: Excellent implementation of automatic tax calculation.The automatic calculation of taxes based on
codigoTarifais well-implemented. The tax rate mapping is correct for Costa Rica's tax system, and the validation ensures calculated values match provided values when both exist.
1018-1043: Excellent validation of payment methods total.The validation ensures that the sum of all payment methods equals the total invoice amount, preventing accounting discrepancies.
1166-1184: Good addition of XML validation and response structure.The XML validation using ValidadorXML and the consistent response structure with validation results improves data quality and error handling. The XML trimming also helps reduce file size.
4077-4081: Email validation with proper error handling in genXMLFec.Good that you're returning an error response instead of just logging invalid emails. This provides immediate feedback to the API consumer.
| @@ -1,7 +1,8 @@ | |||
| # Modulo para hacer comprobar estado de los comprobantes | |||
| # Modulo para hacer comprobar estado de los comprobantes y su estado de recepcion | |||
There was a problem hiding this comment.
Fix Spanish spelling and grammar issues.
The header has spelling errors that should be corrected.
-# Modulo para hacer comprobar estado de los comprobantes y su estado de recepcion
+# Módulo para comprobar el estado de los comprobantes y su estado de recepción📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Modulo para hacer comprobar estado de los comprobantes y su estado de recepcion | |
| # Módulo para comprobar el estado de los comprobantes y su estado de recepción |
🧰 Tools
🪛 LanguageTool
[misspelling] ~1-~1: Si es adjetivo o nombre, se escribe con tilde.
Context: # Modulo para hacer comprobar estado de los comp...
(DIACRITICS_02)
[uncategorized] ~1-~1: Probablemente falta un determinante.
Context: # Modulo para hacer comprobar estado de los comprobantes y su estado de rece...
(AI_ES_GGEC_MISSING_DETERMINER)
[uncategorized] ~1-~1: Probablemente falta un signo diacrítico.
Context: ...tado de los comprobantes y su estado de recepcion Se envia: * w : consultar * r : r...
(AI_ES_GGEC_MISSING_ORTHOGRAPHY_DIACRITIC)
🤖 Prompt for AI Agents
In api/contrib/consultar/README.md around line 1, the header contains Spanish
spelling/grammar errors; replace it with a corrected, accented and concise
version such as: "Módulo para comprobar el estado de los comprobantes y su
recepción" (ensure "Módulo" and "recepción" include accents and remove the
redundant "hacer").
| $temp = rand(0, 1000) + time(); | ||
|
|
||
| $compannyUser->pwd = password_hash($temp, PASSWORD_DEFAULT); | ||
| $compannyUser->pwd = password_hash($temp, PASSWORD_ARGON2ID); | ||
| grace_debug("New tmp pwd: " . $compannyUser->pwd); | ||
|
|
There was a problem hiding this comment.
Don’t email a new plaintext password; issue a time-bound reset token instead.
Current flow generates and emails a password, which is insecure and hard to audit. Prefer a reset token + link.
Minimal improvement if you must keep this flow:
- Store a forced-change flag and require update on next login.
- Set very short validity and invalidate all sessions.
Better approach (recommended):
- Create a reset token with random_bytes(32), store hashed, email a one-time link, and let the user set a new password.
api/tools/ValidadorXML.php
Outdated
| $baseFolder = conf_get('coreInstall', 'modules', ''); | ||
| $baseFolder = str_replace('/api', '/www', $baseFolder); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Path construction is brittle (str_replace '/api'→'/www').
Conf values may not include '/api', yielding wrong paths. Prefer a dedicated config key (e.g., xsdBasePath) or compute relative to known anchor.
Minimal hardening:
- $baseFolder = conf_get('coreInstall', 'modules', '');
- $baseFolder = str_replace('/api', '/www', $baseFolder);
+ $baseFolder = rtrim(conf_get('xsdBasePath', 'core', str_replace('/api', '/www', conf_get('coreInstall', 'modules', ''))), '/');🏁 Script executed:
#!/bin/bash
# Locate conf_get implementation and usages
rg -n "function\s\+conf_get" -C3 --type=php
rg -n "conf_get\(" -C3 --type=php
# Find where 'coreInstall' config is defined or referenced
rg -n "coreInstall" -C3 --type=phpLength of output: 28066
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' api/core/conf.phpLength of output: 1385
Use a dedicated config key for the XSD base path instead of brittle str_replace('/api', '/www', …).
Replacing '/api'→'/www' will break whenever coreInstall doesn’t include '/api'. Define a new config['xsd']['basePath'] in your settings and call conf_get('basePath','xsd', …) (with rtrim) here.
🤖 Prompt for AI Agents
In api/tools/ValidadorXML.php around lines 46-47, replace the brittle
str_replace-based derivation of the XSD base path with a dedicated config key:
read the path via conf_get('basePath','xsd','') (or equivalent config API),
apply rtrim to remove any trailing slashes, and use that value instead of
manipulating conf_get('coreInstall','modules',...) with str_replace; update
config files to include config['xsd']['basePath'] and ensure callers use this
canonical, trimmed base path.
| $schemas = [ | ||
| "01" => "$baseFolder/xsd/FacturaElectronica_V$version.xsd", | ||
| "02" => "$baseFolder/xsd/FacturaCompra_V$version.xsd", | ||
| "03" => "$baseFolder/xsd/NotaCreditoElectronica_V$version.xsd", | ||
| "04" => "$baseFolder/xsd/TiqueteElectronico_V$version.xsd", | ||
| "05" => "$baseFolder/xsd/MensajeHacienda_V$version.xsd", | ||
| ]; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
XSD mapping likely incorrect for '02' and message types; verify filenames.
- '02' (Nota de Débito Electrónica) should point to NotaDebitoElectronica_V$version.xsd, not FacturaCompra.
- Hacienda messages for 05–07 are typically MensajeReceptor*.xsd in v4.4.
Proposed mapping:
- "02" => "$baseFolder/xsd/FacturaCompra_V$version.xsd",
+ "02" => "$baseFolder/xsd/NotaDebitoElectronica_V$version.xsd",
- "05" => "$baseFolder/xsd/MensajeHacienda_V$version.xsd",
+ "05" => "$baseFolder/xsd/MensajeReceptor_V$version.xsd",🏁 Script executed:
#!/bin/bash
# List available XSDs to confirm expected names for v4.4
fd -a --glob 'www/xsd/*V4.4*.xsd' || true
rg -n "schemaLocation|xsd" -C1 www/xsd 2>/dev/null | head -n 200 || trueLength of output: 10915
Fix XSD schema mappings for codes 02 and 05
- Code "02": reference
NotaDebitoElectronica_V$version.xsd(Debit Note) instead ofFacturaCompra_V$version.xsd. - Code "05": reference
MensajeReceptor_V$version.xsd(Acceptance Confirmation) instead ofMensajeHacienda_V$version.xsd.
- "02" => "$baseFolder/xsd/FacturaCompra_V$version.xsd",
+ "02" => "$baseFolder/xsd/NotaDebitoElectronica_V$version.xsd",
- "05" => "$baseFolder/xsd/MensajeHacienda_V$version.xsd",
+ "05" => "$baseFolder/xsd/MensajeReceptor_V$version.xsd",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $schemas = [ | |
| "01" => "$baseFolder/xsd/FacturaElectronica_V$version.xsd", | |
| "02" => "$baseFolder/xsd/FacturaCompra_V$version.xsd", | |
| "03" => "$baseFolder/xsd/NotaCreditoElectronica_V$version.xsd", | |
| "04" => "$baseFolder/xsd/TiqueteElectronico_V$version.xsd", | |
| "05" => "$baseFolder/xsd/MensajeHacienda_V$version.xsd", | |
| ]; | |
| $schemas = [ | |
| "01" => "$baseFolder/xsd/FacturaElectronica_V$version.xsd", | |
| "02" => "$baseFolder/xsd/NotaDebitoElectronica_V$version.xsd", | |
| "03" => "$baseFolder/xsd/NotaCreditoElectronica_V$version.xsd", | |
| "04" => "$baseFolder/xsd/TiqueteElectronico_V$version.xsd", | |
| "05" => "$baseFolder/xsd/MensajeReceptor_V$version.xsd", | |
| ]; |
🤖 Prompt for AI Agents
In api/tools/ValidadorXML.php around lines 48 to 55, update the $schemas mapping
so that code "02" points to
"$baseFolder/xsd/NotaDebitoElectronica_V$version.xsd" instead of
"FacturaCompra_V$version.xsd" and code "05" points to
"$baseFolder/xsd/MensajeReceptor_V$version.xsd" instead of
"MensajeHacienda_V$version.xsd"; preserve the same $baseFolder and $version
interpolation and keep the rest of the mapping entries unchanged.
| libxml_use_internal_errors(true); | ||
| $xml = new DOMDocument(); | ||
| $xml->loadXML($contenido_xml); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden XML loading and handle load failures.
Prevent XXE/network fetch and bail early on invalid XML:
- $xml = new DOMDocument();
- $xml->loadXML($contenido_xml);
+ $xml = new DOMDocument();
+ if (@$xml->loadXML($contenido_xml, LIBXML_NONET) === false) {
+ return (object)[
+ "status" => "error",
+ "message" => "Error: XML inválido o mal formado"
+ ];
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| libxml_use_internal_errors(true); | |
| $xml = new DOMDocument(); | |
| $xml->loadXML($contenido_xml); | |
| libxml_use_internal_errors(true); | |
| $xml = new DOMDocument(); | |
| if (@$xml->loadXML($contenido_xml, LIBXML_NONET) === false) { | |
| return (object)[ | |
| "status" => "error", | |
| "message" => "Error: XML inválido o mal formado" | |
| ]; | |
| } |
🤖 Prompt for AI Agents
In api/tools/ValidadorXML.php around lines 77-80, the XML is loaded without
blocking external entity/network fetches and without checking for load failures;
update load to use LIBXML_NONET to prevent XXE/network fetches (and call
libxml_disable_entity_loader(true) for older PHP versions if needed), keep
libxml_use_internal_errors(true), check the boolean return of $xml->loadXML(...,
LIBXML_NONET), collect libxml_get_errors() and clear them with
libxml_clear_errors() on failure, and bail/return/throw with a clear error
message so invalid XML is handled early.
| # Database port | ||
| $config['db']['port'] = getenv('DB_PORT') ?: 3306; // Default to 3306 if not set |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sanitize and validate DB_PORT; cast to int and enforce valid range
Avoid treating numeric strings loosely and ensure an integer within 1–65535. This prevents bad ports and type issues downstream.
-# Database port
-$config['db']['port'] = getenv('DB_PORT') ?: 3306; // Default to 3306 if not set
+# Database port
+$__port = getenv('DB_PORT');
+$config['db']['port'] = (is_numeric($__port) ? max(1, min(65535, (int)$__port)) : 3306);
+unset($__port);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Database port | |
| $config['db']['port'] = getenv('DB_PORT') ?: 3306; // Default to 3306 if not set | |
| # Database port | |
| $__port = getenv('DB_PORT'); | |
| $config['db']['port'] = (is_numeric($__port) | |
| ? max(1, min(65535, (int)$__port)) | |
| : 3306 | |
| ); | |
| unset($__port); |
🤖 Prompt for AI Agents
In www/settings.php around lines 49 to 50, the DB_PORT value is taken directly
from getenv and left as a string or fallback value; change this to cast the
environment value to an integer, validate it is a number within the TCP port
range 1–65535, and fall back to 3306 if missing or out of range. Specifically,
read getenv('DB_PORT'), trim and check is_numeric, cast to (int), enforce 1 <=
port <= 65535, and assign the validated int to $config['db']['port']; if
validation fails, use 3306 (optionally emit a warning/log).
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 (4)
api/contrib/consultar/consultar.php (4)
24-26: Standardize return type and avoid strlen(false) warning for clave.Use a robust empty check and return the structured payload used elsewhere.
- if ($clave == "" || strlen($clave) == 0) - return "La clave no puede ser en blanco"; + if (empty($clave) || !is_string($clave)) { + return array( + "Status" => 400, + "to" => null, + "text" => "La clave no puede estar vacía" + ); + }
36-51: Enable TLS verification and adjust headers for GET.Disabling SSL verification is unsafe; also prefer Accept for GET.
CURLOPT_MAXREDIRS => 10, - CURLOPT_SSL_VERIFYHOST => 0, - CURLOPT_SSL_VERIFYPEER => 0, - CURLOPT_TIMEOUT => 30, + CURLOPT_SSL_VERIFYHOST => 2, + CURLOPT_SSL_VERIFYPEER => 1, + CURLOPT_CONNECTTIMEOUT => 10, + CURLOPT_TIMEOUT => 30, CURLOPT_HTTP_VERSION => CURL_HTTP_VERSION_1_1, - CURLOPT_CUSTOMREQUEST => "GET", + CURLOPT_HTTPGET => true, CURLOPT_HTTPHEADER => array( "Authorization: Bearer " . params_get('token'), "Cache-Control: no-cache", - "Content-Type: application/x-www-form-urlencoded" + "Accept: application/json" ),
141-149: Undefined variable $apiTo; use the actual request URL.This breaks the error path.
- "to" => $apiTo, + "to" => $args[CURLOPT_URL],
150-154: Return a standardized response object (Status/to/text).Keep API outputs consistent with consultarRecepcion.
- $response = json_decode($response); - return $response; + $responseObj = json_decode($response); + return array( + "Status" => $status, + "to" => $url, + "text" => $responseObj + );
♻️ Duplicate comments (1)
api/contrib/consultar/consultar.php (1)
79-81: Return structured payload instead of raw $response.This was already flagged; see fix above returning the array with Status/to/text.
🧹 Nitpick comments (3)
api/contrib/consultar/consultar.php (3)
59-67: Return the full request URL on cURL error.Helps debugging which resource failed.
- $arrayResp = array( + $arrayResp = array( "Status" => $status, - "to" => $url, + "to" => $args[CURLOPT_URL], "text" => $err );
46-48: Validate token before making the request.Avoid a pointless 401 by rejecting empty tokens early (apply similarly in consultarComprobante()).
- "Authorization: Bearer " . params_get('token'), + // Validar token antes de construir headers + "Authorization: Bearer " . params_get('token'),Follow-up (example guard to add near the top):
$token = params_get('token'); if (empty($token) || !is_string($token)) { return array("Status"=>400,"to"=>null,"text"=>"El token no puede estar vacío"); }
45-46: Prefer CURLOPT_HTTPGET over CUSTOMREQUEST for GET.Clearer intent and lets cURL set method semantics.
- CURLOPT_CUSTOMREQUEST => "GET", + CURLOPT_HTTPGET => true,Also applies to: 127-128
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
api/contrib/consultar/consultar.php(3 hunks)api/core/tools.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/core/tools.php
🧰 Additional context used
🧬 Code graph analysis (1)
api/contrib/consultar/consultar.php (1)
api/core/params.php (1)
params_get(24-36)
| if ($url == null) | ||
| return "Ha ocurrido un error en el client_id."; | ||
| return "El client_id proprocionado (".params_get("client_id").") no es válido. "; | ||
|
|
There was a problem hiding this comment.
Fix typo and return a structured error for invalid client_id.
Keep responses consistent and correct the Spanish typo.
- if ($url == null)
- return "El client_id proprocionado (".params_get("client_id").") no es válido. ";
+ if ($url == null) {
+ return array(
+ "Status" => 400,
+ "to" => null,
+ "text" => "El client_id proporcionado (" . params_get("client_id") . ") no es válido."
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ($url == null) | |
| return "Ha ocurrido un error en el client_id."; | |
| return "El client_id proprocionado (".params_get("client_id").") no es válido. "; | |
| if ($url == null) { | |
| return array( | |
| "Status" => 400, | |
| "to" => null, | |
| "text" => "El client_id proporcionado (" . params_get("client_id") . ") no es válido." | |
| ); | |
| } |
🤖 Prompt for AI Agents
In api/contrib/consultar/consultar.php around lines 33 to 35, fix the Spanish
typo ("proprocionado" → "proporcionado") and replace the plain string return
with a structured JSON error response (e.g., an object with keys like "error"
and "message" that includes the provided client_id via params_get("client_id")).
Also set the response Content-Type to application/json and return an appropriate
HTTP status (400) for invalid client_id.
api/contrib/consultar/consultar.php
Outdated
| if( is_object($responseObj) ) { | ||
| // si hay respuesta de hacienda, procesar detalle de mensaje | ||
| if(isset($responseObj->{'respuesta-xml'})) { | ||
| $xmlRespuesta = base64_decode($responseObj->{'respuesta-xml'}); | ||
| $xmlRespuesta = preg_replace('/>\s+</', '><', $xmlRespuesta); | ||
| $startLength = strlen("tiene los siguientes errores: ")+7; | ||
| $startPos = strpos($xmlRespuesta, "tiene los siguientes errores: ", 0); | ||
| if(!$startPos) { | ||
| // si no hay errores, retornar la respuesta normal | ||
| return $response; | ||
| } | ||
| $endPos = strpos($xmlRespuesta, "</DetalleMensaje>", $startPos); | ||
| $detalleMensaje = substr($xmlRespuesta, $startPos+$startLength, $endPos -($startPos+$startLength+1)); | ||
| $separadorMensaje = " \n"; | ||
| $responseObj->{'DetalleMensaje'} = explode($separadorMensaje,trim($detalleMensaje,$separadorMensaje)); | ||
| } |
There was a problem hiding this comment.
Fix falsey strpos check, guard base64 decode, and off-by-one in substring.
Current logic can misclassify position 0 as “not found” and may slice incorrectly.
- if(isset($responseObj->{'respuesta-xml'})) {
- $xmlRespuesta = base64_decode($responseObj->{'respuesta-xml'});
+ if (isset($responseObj->{'respuesta-xml'})) {
+ $xmlRespuesta = base64_decode($responseObj->{'respuesta-xml'}, true);
+ if ($xmlRespuesta === false) {
+ return array("Status" => $status, "to" => $url, "text" => $responseObj);
+ }
$xmlRespuesta = preg_replace('/>\s+</', '><', $xmlRespuesta);
- $startLength = strlen("tiene los siguientes errores: ")+7;
- $startPos = strpos($xmlRespuesta, "tiene los siguientes errores: ", 0);
- if(!$startPos) {
- // si no hay errores, retornar la respuesta normal
- return $response;
- }
+ $startLength = strlen("tiene los siguientes errores: ");
+ $startPos = strpos($xmlRespuesta, "tiene los siguientes errores: ");
+ if ($startPos === false) {
+ // no hay errores, devolver respuesta normal
+ return array("Status" => $status, "to" => $url, "text" => $responseObj);
+ }
$endPos = strpos($xmlRespuesta, "</DetalleMensaje>", $startPos);
- $detalleMensaje = substr($xmlRespuesta, $startPos+$startLength, $endPos -($startPos+$startLength+1));
+ $detalleMensaje = substr($xmlRespuesta, $startPos + $startLength, $endPos - ($startPos + $startLength));
$separadorMensaje = " \n";
$responseObj->{'DetalleMensaje'} = explode($separadorMensaje,trim($detalleMensaje,$separadorMensaje));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if( is_object($responseObj) ) { | |
| // si hay respuesta de hacienda, procesar detalle de mensaje | |
| if(isset($responseObj->{'respuesta-xml'})) { | |
| $xmlRespuesta = base64_decode($responseObj->{'respuesta-xml'}); | |
| $xmlRespuesta = preg_replace('/>\s+</', '><', $xmlRespuesta); | |
| $startLength = strlen("tiene los siguientes errores: ")+7; | |
| $startPos = strpos($xmlRespuesta, "tiene los siguientes errores: ", 0); | |
| if(!$startPos) { | |
| // si no hay errores, retornar la respuesta normal | |
| return $response; | |
| } | |
| $endPos = strpos($xmlRespuesta, "</DetalleMensaje>", $startPos); | |
| $detalleMensaje = substr($xmlRespuesta, $startPos+$startLength, $endPos -($startPos+$startLength+1)); | |
| $separadorMensaje = " \n"; | |
| $responseObj->{'DetalleMensaje'} = explode($separadorMensaje,trim($detalleMensaje,$separadorMensaje)); | |
| } | |
| if (is_object($responseObj)) { | |
| // si hay respuesta de hacienda, procesar detalle de mensaje | |
| if (isset($responseObj->{'respuesta-xml'})) { | |
| // strict decode and bail on failure | |
| $xmlRespuesta = base64_decode($responseObj->{'respuesta-xml'}, true); | |
| if ($xmlRespuesta === false) { | |
| return array( | |
| "Status" => $status, | |
| "to" => $url, | |
| "text" => $responseObj | |
| ); | |
| } | |
| $xmlRespuesta = preg_replace('/>\s+</', '><', $xmlRespuesta); | |
| // find the error section | |
| $startLength = strlen("tiene los siguientes errores: "); | |
| $startPos = strpos($xmlRespuesta, "tiene los siguientes errores: "); | |
| if ($startPos === false) { | |
| // no hay errores, devolver respuesta normal | |
| return array( | |
| "Status" => $status, | |
| "to" => $url, | |
| "text" => $responseObj | |
| ); | |
| } | |
| $endPos = strpos($xmlRespuesta, "</DetalleMensaje>", $startPos); | |
| $detalleMensaje = substr( | |
| $xmlRespuesta, | |
| $startPos + $startLength, | |
| $endPos - ($startPos + $startLength) | |
| ); | |
| $separadorMensaje = " \n"; | |
| $responseObj->{'DetalleMensaje'} = explode( | |
| $separadorMensaje, | |
| trim($detalleMensaje, $separadorMensaje) | |
| ); | |
| } |
🤖 Prompt for AI Agents
In api/contrib/consultar/consultar.php around lines 71 to 86, the code treats a
strpos result of 0 as “not found”, doesn't guard base64_decode for failure, and
computes the substring length with an off-by-one; change the strpos check to
strict comparison (=== false) so position 0 is treated as found, only call
base64_decode if respuesta-xml exists and check its return is not false before
using it, and compute the substring length as $endPos - ($startPos +
$startLength) (remove the extra +1) so substr extracts the full message; keep
the separator handling but trim using the separator characters properly before
explode.
| function consultarComprobante() | ||
| { | ||
| $curl = curl_init(); | ||
| $clave = params_get('clave'); | ||
|
|
||
| if ($clave == "" || strlen($clave) == 0) | ||
| return "La clave no puede ser en blanco"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same clave handling as consultarRecepcion: standardize and avoid warnings.
- if ($clave == "" || strlen($clave) == 0)
- return "La clave no puede ser en blanco";
+ if (empty($clave) || !is_string($clave)) {
+ return array(
+ "Status" => 400,
+ "to" => null,
+ "text" => "La clave no puede estar vacía"
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function consultarComprobante() | |
| { | |
| $curl = curl_init(); | |
| $clave = params_get('clave'); | |
| if ($clave == "" || strlen($clave) == 0) | |
| return "La clave no puede ser en blanco"; | |
| function consultarComprobante() | |
| { | |
| $curl = curl_init(); | |
| $clave = params_get('clave'); | |
| - if ($clave == "" || strlen($clave) == 0) | |
| if (empty($clave) || !is_string($clave)) { | |
| return array( | |
| "Status" => 400, | |
| "to" => null, | |
| "text" => "La clave no puede estar vacía" | |
| ); | |
| } | |
| // …rest of function… | |
| } |
🤖 Prompt for AI Agents
In api/contrib/consultar/consultar.php around lines 101 to 107, the current
clave handling uses params_get('clave') and then calls strlen which can emit
warnings if clave is null; standardize to the same approach used in
consultarRecepcion by normalizing and trimming the input: assign $clave =
trim(params_get('clave') ?? ''); then check with if ($clave === '') return "La
clave no puede ser en blanco"; this avoids warnings from strlen and null values
and ensures consistent validation.
api/contrib/consultar/consultar.php
Outdated
| if ($url == null) | ||
| return "Ha ocurrido un error en el client_id."; |
There was a problem hiding this comment.
Return structured error and include the provided client_id.
- if ($url == null)
- return "Ha ocurrido un error en el client_id.";
+ if ($url == null) {
+ return array(
+ "Status" => 400,
+ "to" => null,
+ "text" => "El client_id proporcionado (" . params_get("client_id") . ") no es válido."
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ($url == null) | |
| return "Ha ocurrido un error en el client_id."; | |
| // Replace the one-line error return with a structured response including the invalid client_id | |
| if ($url == null) { | |
| return array( | |
| "Status" => 400, | |
| "to" => null, | |
| "text" => "El client_id proporcionado (" . params_get("client_id") . ") no es válido." | |
| ); | |
| } |
🤖 Prompt for AI Agents
In api/contrib/consultar/consultar.php around lines 115-116, the function
currently returns a plain string when $url is null; replace this with a
structured JSON error response that includes the provided client_id (e.g.,
{"error":"invalid_client_id","message":"Ha ocurrido un error en el
client_id.","client_id":<client_id>}), set the proper Content-Type:
application/json header and an appropriate HTTP status code (e.g., 400), and
ensure you reference the actual client_id variable rather than omitting it.
api/contrib/consultar/consultar.php
Outdated
| $args = array( | ||
| CURLOPT_URL => $url . $clave, | ||
| CURLOPT_RETURNTRANSFER => true, | ||
| CURLOPT_ENCODING => "", | ||
| CURLOPT_MAXREDIRS => 10, | ||
| CURLOPT_SSL_VERIFYHOST => 0, | ||
| CURLOPT_SSL_VERIFYPEER => 0, | ||
| CURLOPT_TIMEOUT => 30, | ||
| CURLOPT_HTTP_VERSION => CURL_HTTP_VERSION_1_1, | ||
| CURLOPT_CUSTOMREQUEST => "GET", | ||
| CURLOPT_HTTPHEADER => array( | ||
| "Authorization: Bearer " . params_get('token'), | ||
| "Cache-Control: no-cache", | ||
| "Content-Type: application/x-www-form-urlencoded" | ||
| ), | ||
| ); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mirror cURL hardening from consultarRecepcion.
Enable TLS verification, add connect timeout, prefer HTTPGET, and use Accept.
CURLOPT_MAXREDIRS => 10,
- CURLOPT_SSL_VERIFYHOST => 0,
- CURLOPT_SSL_VERIFYPEER => 0,
- CURLOPT_TIMEOUT => 30,
+ CURLOPT_SSL_VERIFYHOST => 2,
+ CURLOPT_SSL_VERIFYPEER => 1,
+ CURLOPT_CONNECTTIMEOUT => 10,
+ CURLOPT_TIMEOUT => 30,
CURLOPT_HTTP_VERSION => CURL_HTTP_VERSION_1_1,
- CURLOPT_CUSTOMREQUEST => "GET",
+ CURLOPT_HTTPGET => true,
CURLOPT_HTTPHEADER => array(
"Authorization: Bearer " . params_get('token'),
"Cache-Control: no-cache",
- "Content-Type: application/x-www-form-urlencoded"
+ "Accept: application/json"
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $args = array( | |
| CURLOPT_URL => $url . $clave, | |
| CURLOPT_RETURNTRANSFER => true, | |
| CURLOPT_ENCODING => "", | |
| CURLOPT_MAXREDIRS => 10, | |
| CURLOPT_SSL_VERIFYHOST => 0, | |
| CURLOPT_SSL_VERIFYPEER => 0, | |
| CURLOPT_TIMEOUT => 30, | |
| CURLOPT_HTTP_VERSION => CURL_HTTP_VERSION_1_1, | |
| CURLOPT_CUSTOMREQUEST => "GET", | |
| CURLOPT_HTTPHEADER => array( | |
| "Authorization: Bearer " . params_get('token'), | |
| "Cache-Control: no-cache", | |
| "Content-Type: application/x-www-form-urlencoded" | |
| ), | |
| ); | |
| $args = array( | |
| CURLOPT_URL => $url . $clave, | |
| CURLOPT_RETURNTRANSFER => true, | |
| CURLOPT_ENCODING => "", | |
| CURLOPT_MAXREDIRS => 10, | |
| CURLOPT_SSL_VERIFYHOST => 2, | |
| CURLOPT_SSL_VERIFYPEER => 1, | |
| CURLOPT_CONNECTTIMEOUT => 10, | |
| CURLOPT_TIMEOUT => 30, | |
| CURLOPT_HTTP_VERSION => CURL_HTTP_VERSION_1_1, | |
| CURLOPT_HTTPGET => true, | |
| CURLOPT_HTTPHEADER => array( | |
| "Authorization: Bearer " . params_get('token'), | |
| "Cache-Control: no-cache", | |
| "Accept: application/json" | |
| ), | |
| ); |
🤖 Prompt for AI Agents
In api/contrib/consultar/consultar.php around lines 118 to 134, mirror the
hardening used in consultarRecepcion by enabling TLS verification (set
CURLOPT_SSL_VERIFYHOST to 2 and CURLOPT_SSL_VERIFYPEER to true), add a connect
timeout (CURLOPT_CONNECTTIMEOUT, e.g. 10 seconds), prefer using CURLOPT_HTTPGET
=> true instead of CURLOPT_CUSTOMREQUEST => "GET", and replace/augment headers
to include an Accept header (e.g. "Accept: application/json") while removing or
leaving Content-Type only if required for GET requests; ensure the Authorization
header remains unchanged.
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 (4)
api/contrib/consultar/consultar.php (2)
24-26: Null-safe clave check and structured error response.Avoid strlen on null and keep responses consistent with the rest of the module.
- if ($clave == "" || strlen($clave) == 0) - return "La clave no puede ser en blanco"; + $clave = trim((string)(params_get('clave') ?? '')); + if ($clave === '') { + return array( + "Status" => 400, + "to" => null, + "text" => "La clave no puede estar vacía" + ); + }
165-178: Undefined variable $apiTo; also standardize return shape to match consultarRecepcion.- if ($err) - { - $arrayResp = array( - "Status" => $status, - "to" => $apiTo, - "text" => $err - ); - return $arrayResp; - } - else - { - $response = json_decode($response); - return $response; - } + if ($err) { + return array( + "Status" => $status, + "to" => $url, + "text" => $err + ); + } + + $decoded = json_decode($response); + return array( + "Status" => $status, + "to" => $url, + "text" => $decoded !== null ? $decoded : $response + );api/contrib/genXML/genXML.php (2)
343-349: Fix phone validation: comparing numeric value instead of length.You’re comparing the phone number’s numeric value to min/max length constants, causing false negatives.
Apply this diff:
-if ($emisorCodPaisTel != '' && $emisorTel != '' && $emisorTel >= EMISORNUMEROTELMIN && $emisorTel <= EMISORNUMEROTELMAX) { +if ($emisorCodPaisTel != '' && $emisorTel != '' && ctype_digit((string)$emisorTel) + && strlen((string)$emisorTel) >= EMISORNUMEROTELMIN && strlen((string)$emisorTel) <= EMISORNUMEROTELMAX) {
736-757: Don’t multiply MontoDescuento by cantidad.Spec and example show montoDescuento is a line amount, not per-unit. Multiplying doubles the discount and breaks SubTotal.
Apply this diff:
- $c['montoDescuento'] = floatval($c['montoDescuento']) * $d->cantidad; + $c['montoDescuento'] = round((float)$c['montoDescuento'], DECIMALES);
♻️ Duplicate comments (10)
api/core/params.php (1)
44-44: Default override=true restores unconditional-overwrite semantics — LGTM.Matches prior recommendation to reintroduce override semantics with a safe default.
To catch any callers that relied on a previous default of false, scan usages:
#!/usr/bin/env bash # All call sites (with context) rg -nC2 '\bparams_set\s*\(' # Calls that explicitly pass false (good to double-check intent) rg -nP '\bparams_set\s*\(\s*[^,]+,\s*[^,]+,\s*false\b' # Potential bulk-set call sites (return shape may matter) rg -nP '\bparams_set\s*\(\s*(\[|array\s*\()' -nC2api/core/tools.php (1)
52-78: Guard $response['text']; reindex after unset and don't assume existence.Avoid notices and preserve array JSON shape when removing index 0. Also guard the final assignment.
- if (is_numeric($response['Status'])) { + if (is_numeric($response['Status'])) { $httpStatus = $response['Status']; $killMe = $response['Status'] >= 299; - if(is_array($response['text']) && count($response['text']) > 1) { + if (isset($response['text']) && is_array($response['text']) && count($response['text']) > 1) { // cuando la respuesta es un array de Hacienda, el primer elemento es el código de estado // y el resto es el mensaje - unset($response['text'][0]); + unset($response['text'][0]); + $response['text'] = array_values($response['text']); // reindexar } } else { @@ - $response = $response['text']; + $response = $response['text'] ?? $response;api/contrib/consultar/consultar.php (6)
36-52: Harden cURL: enable TLS verification, add connect timeout, prefer HTTPGET, use Accept header.CURLOPT_URL => $url . $clave, CURLOPT_RETURNTRANSFER => true, - CURLOPT_HEADER => true, + CURLOPT_HEADER => true, CURLOPT_ENCODING => "", CURLOPT_MAXREDIRS => 10, - CURLOPT_SSL_VERIFYHOST => 0, - CURLOPT_SSL_VERIFYPEER => 0, + CURLOPT_SSL_VERIFYHOST => 2, + CURLOPT_SSL_VERIFYPEER => 1, + CURLOPT_CONNECTTIMEOUT => 10, CURLOPT_TIMEOUT => 30, CURLOPT_HTTP_VERSION => CURL_HTTP_VERSION_1_1, - CURLOPT_CUSTOMREQUEST => "GET", + CURLOPT_HTTPGET => true, CURLOPT_HTTPHEADER => array( "Authorization: Bearer " . params_get('token'), "Cache-Control: no-cache", - "Content-Type: application/x-www-form-urlencoded" + "Accept: application/json" ),
33-35: Invalid client_id: fix typo and return a structured 400.- if ($url == null) - return "El client_id proprocionado (".params_get("client_id").") no es válido. "; + if ($url === null) { + return array( + "Status" => 400, + "to" => null, + "text" => "El client_id proporcionado (" . params_get("client_id") . ") no es válido." + ); + }
139-141: Invalid client_id: return structured 400 with provided client_id.- if ($url == null) - return "Ha ocurrido un error en el client_id."; + if ($url === null) { + return array( + "Status" => 400, + "to" => null, + "text" => "El client_id proporcionado (" . params_get("client_id") . ") no es válido." + ); + }
142-157: Harden cURL (TLS verify, connect timeout, GET, Accept header).CURLOPT_URL => $url . $clave, CURLOPT_RETURNTRANSFER => true, CURLOPT_ENCODING => "", CURLOPT_MAXREDIRS => 10, - CURLOPT_SSL_VERIFYHOST => 0, - CURLOPT_SSL_VERIFYPEER => 0, + CURLOPT_SSL_VERIFYHOST => 2, + CURLOPT_SSL_VERIFYPEER => 1, + CURLOPT_CONNECTTIMEOUT => 10, CURLOPT_TIMEOUT => 30, CURLOPT_HTTP_VERSION => CURL_HTTP_VERSION_1_1, - CURLOPT_CUSTOMREQUEST => "GET", + CURLOPT_HTTPGET => true, CURLOPT_HTTPHEADER => array( "Authorization: Bearer " . params_get('token'), "Cache-Control: no-cache", - "Content-Type: application/x-www-form-urlencoded" + "Accept: application/json" ),
75-93: respuesta-xml parsing: guard base64_decode, fix strpos falsey check and substring math.- if(isset($responseObj->{'respuesta-xml'})) { - - $xmlRespuesta = base64_decode($responseObj->{'respuesta-xml'}); - $xmlRespuesta = preg_replace('/>\s+</', '><', $xmlRespuesta); - $startLength = strlen("tiene los siguientes errores: ")+7; - $startPos = strpos($xmlRespuesta, "tiene los siguientes errores: ", 0); - if(!$startPos) { - // si no hay errores, retornar la respuesta normal - return $responseObj; - } - $endPos = strpos($xmlRespuesta, "</DetalleMensaje>", $startPos); - $detalleMensaje = substr($xmlRespuesta, $startPos+$startLength, $endPos -($startPos+$startLength+1)); - $separadorMensaje = " \n"; - $responseObj->{'DetalleMensaje'} = explode($separadorMensaje,trim($detalleMensaje,$separadorMensaje)); - } + if (isset($responseObj->{'respuesta-xml'})) { + $xmlRespuesta = base64_decode($responseObj->{'respuesta-xml'}, true); + if ($xmlRespuesta === false) { + return array("Status" => $status, "to" => $url, "text" => $responseObj); + } + $xmlRespuesta = preg_replace('/>\s+</', '><', $xmlRespuesta); + $needle = "tiene los siguientes errores: "; + $startPos = strpos($xmlRespuesta, $needle); + if ($startPos === false) { + return array("Status" => $status, "to" => $url, "text" => $responseObj); + } + $startLength = strlen($needle); + $endPos = strpos($xmlRespuesta, "</DetalleMensaje>", $startPos); + if ($endPos !== false) { + $detalleMensaje = substr($xmlRespuesta, $startPos + $startLength, $endPos - ($startPos + $startLength)); + $separadorMensaje = " \n"; + $responseObj->{'DetalleMensaje'} = explode($separadorMensaje, trim($detalleMensaje, $separadorMensaje)); + } + }
130-132: Duplicate clave check: apply the same null-safe, structured response.- if ($clave == "" || strlen($clave) == 0) - return "La clave no puede ser en blanco"; + $clave = trim((string)(params_get('clave') ?? '')); + if ($clave === '') { + return array( + "Status" => 400, + "to" => null, + "text" => "La clave no puede estar vacía" + ); + }api/contrib/genXML/genXML.php (2)
330-341: Do not hardcode OtrasSenas; use provided value and escape it.Hardcoded "123456" leaks into all FE XMLs and breaks data integrity. Also, align the Distrito check with the others.
Apply this diff:
- if ($emisorProv != '' && $emisorCanton != '' && $emisorDistrito) { + if ($emisorProv != '' && $emisorCanton != '' && $emisorDistrito != '') { $xmlString .= '<Ubicacion> <Provincia>' . $emisorProv . '</Provincia> <Canton>' . $emisorCanton . '</Canton> <Distrito>' . $emisorDistrito . '</Distrito>'; if ($emisorBarrio != '') { $xmlString .= '<Barrio>' . $emisorBarrio . '</Barrio>'; } - $xmlString .= '<OtrasSenas>123456</OtrasSenas>'; + if ($emisorOtrasSenas != '') { + $xmlString .= '<OtrasSenas>' . htmlspecialchars($emisorOtrasSenas, ENT_XML1 | ENT_COMPAT, 'UTF-8') . '</OtrasSenas>'; + } $xmlString .= '</Ubicacion>'; }
33-34: Blocking: load core libs before calling tools_useTool().This triggers the pipeline failure (undefined function). Require core/conf and core/tools before using tools_useTool.
Apply this diff:
+require_once dirname(__DIR__, 2) . '/core/conf.php'; +require_once dirname(__DIR__, 2) . '/core/tools.php'; tools_useTool('ValidadorXML.php');
🧹 Nitpick comments (12)
api/contrib/clave/clave.php (4)
57-58: Allow optional fechaEmision override; “now” may misalign contingency/back-dated docsUsing CR timezone is good. Consider allowing an optional
fechaEmisionparam (defaults to now) and validate it to keep the clave date consistent with the document’s emission date.- $date = new DateTime('now', new DateTimeZone('America/Costa_Rica')); - $fechaClave = $date->format('dmy'); + $fechaEmision = params_get('fechaEmision'); // opcional: ISO 8601 o 'Y-m-d H:i:s' + $tz = new DateTimeZone('America/Costa_Rica'); + if ($fechaEmision) { + try { + $date = new DateTime($fechaEmision, $tz); + } catch (Exception $e) { + return "El parametro fechaEmision no es valido"; + } + } else { + $date = new DateTime('now', $tz); + } + $fechaClave = $date->format('dmy');
87-87: Avoid echoing raw consecutivo in error messagePrevents long/unsanitized input from being reflected in responses/logs.
- return "El parametro consecutivo [$consecutivo] no es numeral"; + return "El parametro consecutivo no es numeral";
189-195: Validate final clave (length=50, numeric) before returningAdds a cheap integrity guard and clearer error if construction ever drifts.
- $clave = $codigoPais . $fechaClave . $identificacion . $consecutivoFinal . $codSituacion . $codigoSeguridad; + $clave = $codigoPais . $fechaClave . $identificacion . $consecutivoFinal . $codSituacion . $codigoSeguridad; + if (strlen($clave) !== 50 || !ctype_digit($clave)) { + return "Clave generada invalida"; + }I can add a small unit test asserting structure and length (50) across a matrix of inputs if helpful.
117-117: Remove or guard debug call in production pathAvoids accidental leakage/noise in API responses/logs.
- grace_debug($tipoDoc); + if (defined('APP_DEBUG') && APP_DEBUG) { + grace_debug($tipoDoc); + }api/core/params.php (2)
39-43: Docblock is clearer; add PHPDoc tags for toolability.Specify param/return types and note that $p is ignored on bulk sets.
/** * Set config parameters. * - When $val is array, performs a bulk set. * - $override=true overwrites existing keys; false preserves existing values. * Returns the persisted value(s) from $params. + * + * @param string|mixed $p Key for scalar set (ignored when $val is array) + * @param mixed|array $val Value for scalar set, or assoc array for bulk set + * @param bool $override When true, overwrite existing keys + * @return mixed|array|null Persisted value(s); null if scalar write skipped and key absent */
50-56: Preserve input key order in bulk-set return.array_intersect_key returns keys in $params’ order. If callers expect the order of provided $val, return in that order without changing semantics.
- // Return the slice that reflects what is now stored - return array_intersect_key($params, $val); + // Return the slice that reflects what is now stored, preserving input key order + $out = []; + foreach (array_keys($val) as $k) { + if (array_key_exists($k, $params)) { + $out[$k] = $params[$k]; + } + } + return $out;api/core/tools.php (2)
85-86: Do not override a successful non-200 status to 200.Preserve upstream 2xx codes (e.g., 201/204) rather than forcing 200.
- } else { - $httpStatus = 200; - } + }
121-125: Set a single JSON Content-Type header (with charset).Remove the text/html header and emit only application/json.
- http_response_code($response['httpStatus']); - header('Content-Type: text/html; charset=utf-8'); - header('Content-Type: application/json'); + http_response_code($response['httpStatus']); + header('Content-Type: application/json; charset=utf-8');api/contrib/consultar/consultar.php (1)
142-178: DRY: extract common cURL args into a helper to avoid divergence.Both consultarRecepcion and consultarComprobante share nearly identical cURL setup and error handling. Consider a small internal helper (e.g., buildCurlArgs($url, $headers, $withHeaders = false)) to keep options consistent.
api/contrib/genXML/genXML.php (3)
1467-1471: Consistent error handling for emisor email in NC.Here you return 400 (good), but FE and others only log. Consider aligning behavior across generators.
Do you want a unifying helper for email validation + reply?
351-356: Escape/sanitize receptor email and other free-text fields.CorreoElectronico and free-text tags should be trimmed/escaped to avoid invalid XML on special chars.
I can generate a small helper to safely append optional string tags with htmlspecialchars and length checks.
Also applies to: 407-410
23-23: Typo in constant name or unused constant.
CODIDOREFVALUESlooks misspelled and is unused; consider renaming or removing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
api/contrib/clave/clave.php(3 hunks)api/contrib/consultar/consultar.php(3 hunks)api/contrib/crlibreall/module.php(1 hunks)api/contrib/genXML/genXML.php(62 hunks)api/contrib/genXML/module.php(6 hunks)api/core/params.php(1 hunks)api/core/tools.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
api/core/tools.php (1)
api/core/conf.php (1)
conf_get(26-34)
api/contrib/consultar/consultar.php (1)
api/core/params.php (1)
params_get(24-36)
api/contrib/genXML/genXML.php (4)
api/core/tools.php (2)
tools_useTool(169-177)tools_reply(19-102)api/core/params.php (1)
params_get(24-36)api/tools/ValidadorXML.php (2)
ValidadorXML(3-97)validateXml(43-96)api/core/conf.php (1)
conf_get(26-34)
🪛 GitHub Actions: PHPUnit Tests
api/contrib/genXML/genXML.php
[error] 33-33: Call to undefined function tools_useTool() in api/contrib/genXML/genXML.php:33. Command: './vendor/bin/phpunit --stderr --debug --bootstrap vendor/autoload.php tests/api_contrib_genXML_FE.php'.
🔇 Additional comments (4)
api/core/params.php (1)
57-60: Drop null-handing check—params_setnever returnsnull.Likely an incorrect or invalid review comment.
api/contrib/crlibreall/module.php (1)
113-114: Making informacion_referencia optional for FE path looks good.Confirm downstream genXML handles missing reference info gracefully (no required XSD nodes emitted).
api/contrib/genXML/module.php (1)
121-123: Optionalizing informacion_referencia across all genXML routes: OK.Ensure genXML.php defaults omit the node entirely when absent to satisfy XSD, and validators don’t expect it by default.
Also applies to: 196-198, 271-273, 345-347, 418-420, 478-480
api/contrib/genXML/genXML.php (1)
279-289: Good: medios_pago required when condicion_venta not in {02,08,10}.This prevents inconsistent payments vs totals.
api/contrib/consultar/consultar.php
Outdated
| if(empty($body)) { | ||
| $header_lines = explode("\r\n", trim($headers)); | ||
| $header_array = []; | ||
| foreach ($header_lines as $line) { | ||
| if (strpos($line, ':') !== false) { | ||
| list($key, $value) = explode(': ', $line, 2); | ||
| $header_array[$key] = $value; | ||
| } | ||
| } | ||
| return array( | ||
| "Status" => $header_array['x-http-status'], | ||
| "to" => $url, | ||
| "text" => $header_array['x-error-cause'] | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Header-only error path: guard missing Hacienda headers and fall back to $status.
- return array(
- "Status" => $header_array['x-http-status'],
- "to" => $url,
- "text" => $header_array['x-error-cause']
- );
+ return array(
+ "Status" => isset($header_array['x-http-status']) ? (int)$header_array['x-http-status'] : $status,
+ "to" => $url,
+ "text" => $header_array['x-error-cause'] ?? 'Error desconocido'
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(empty($body)) { | |
| $header_lines = explode("\r\n", trim($headers)); | |
| $header_array = []; | |
| foreach ($header_lines as $line) { | |
| if (strpos($line, ':') !== false) { | |
| list($key, $value) = explode(': ', $line, 2); | |
| $header_array[$key] = $value; | |
| } | |
| } | |
| return array( | |
| "Status" => $header_array['x-http-status'], | |
| "to" => $url, | |
| "text" => $header_array['x-error-cause'] | |
| ); | |
| } | |
| if (empty($body)) { | |
| $header_lines = explode("\r\n", trim($headers)); | |
| $header_array = []; | |
| foreach ($header_lines as $line) { | |
| if (strpos($line, ':') !== false) { | |
| list($key, $value) = explode(': ', $line, 2); | |
| $header_array[$key] = $value; | |
| } | |
| } | |
| return array( | |
| "Status" => isset($header_array['x-http-status']) | |
| ? (int)$header_array['x-http-status'] | |
| : $status, | |
| "to" => $url, | |
| "text" => $header_array['x-error-cause'] ?? 'Error desconocido' | |
| ); | |
| } |
🤖 Prompt for AI Agents
In api/contrib/consultar/consultar.php around lines 102 to 116, the header-only
error path assumes Hacienda headers x-http-status and x-error-cause always
exist; add guards to check those keys in the parsed $header_array before using
them and fall back to the existing $status variable (or a sensible default like
'unknown') for "Status" and a generic message for "text" when headers are
missing; ensure header parsing is resilient (trim lines, handle headers without
space after colon) and return the array using the guarded values.
api/contrib/genXML/genXML.php
Outdated
| if(isset($d->codigoCABYS) && in_array($d->codigoCABYS[0], ['5','6','7','8','9'], true)) { | ||
| // Servicio | ||
| $calculados[$resumenKeys['servicio']] += ($calculados[$l]['montoTotal']); | ||
| } else { | ||
| // Mercancia | ||
| $calculados[$resumenKeys['mercancia']] += ($calculados[$l]['montoTotal']); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard CABYS index access; cast to string.
Accessing $d->codigoCABYS[0] on numeric input can emit notices.
Apply this diff:
- if(isset($d->codigoCABYS) && in_array($d->codigoCABYS[0], ['5','6','7','8','9'], true)) {
+ $codigoCABYSStr = (string)$d->codigoCABYS;
+ if ($codigoCABYSStr !== '' && in_array($codigoCABYSStr[0], ['5','6','7','8','9'], true)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(isset($d->codigoCABYS) && in_array($d->codigoCABYS[0], ['5','6','7','8','9'], true)) { | |
| // Servicio | |
| $calculados[$resumenKeys['servicio']] += ($calculados[$l]['montoTotal']); | |
| } else { | |
| // Mercancia | |
| $calculados[$resumenKeys['mercancia']] += ($calculados[$l]['montoTotal']); | |
| } | |
| $codigoCABYSStr = (string)$d->codigoCABYS; | |
| if ($codigoCABYSStr !== '' && in_array($codigoCABYSStr[0], ['5','6','7','8','9'], true)) { | |
| // Servicio | |
| $calculados[$resumenKeys['servicio']] += ($calculados[$l]['montoTotal']); | |
| } else { | |
| // Mercancia | |
| $calculados[$resumenKeys['mercancia']] += ($calculados[$l]['montoTotal']); | |
| } |
api/contrib/genXML/genXML.php
Outdated
| // $totalComprobante = params_get("total_comprobante"); | ||
|
|
There was a problem hiding this comment.
NC: $totalComprobante is undefined (commented out) but later used.
This will produce notices and invalid XML. Restore the param or compute it.
Apply one of the following:
- Minimal restore:
- // $totalComprobante = params_get("total_comprobante");
+ $totalComprobante = params_get("total_comprobante");- Prefer computed:
+ $totalComprobanteCalc = (float)$totalVentasNeta + (float)$totalImp + (float)($totalOtrosCargos ?: 0) - (float)($totalIVADevuelto ?: 0);
@@
- <TotalComprobante>' . $totalComprobante . '</TotalComprobante>
+ <TotalComprobante>' . number_format($totalComprobanteCalc, DECIMALES, '.', '') . '</TotalComprobante>Also applies to: 2036-2038
🤖 Prompt for AI Agents
In api/contrib/genXML/genXML.php around lines 1344-1345 (and similarly at
2036-2038), $totalComprobante is referenced but commented out/undefined; restore
it or compute it before use. Fix by either reintroducing the
params_get("total_comprobante") assignment where appropriate, or (preferred)
calculate $totalComprobante from the invoice line items (sum of each line's
total = quantity * unit_price + taxes/adjustments as used elsewhere), assign the
correctly formatted numeric string (same decimal places/locale as other monetary
values) to $totalComprobante before it’s inserted into the XML, and ensure any
related code paths use this computed value to avoid notices and invalid XML.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/contrib/signXML/Firmadohaciendacr.php (2)
201-206: Fix fatal: call instance method via $this->stringHex2StringDec.Currently it calls an undefined global function, causing a fatal error when serialNumber is hex.
- $serialNumber = stringHex2StringDec($certData['serialNumber']); + $serialNumber = $this->stringHex2StringDec($certData['serialNumber']);
114-116: Validate base64 and XML load errors.base64_decode can return false; loadXML can fail. Return a 400 early in those cases.
- $xml1 = base64_decode($xmlsinfirma); + $xml1 = base64_decode($xmlsinfirma, true); + if ($xml1 === false) { + tools_reply(["Status" => 400, "text" => "XML base64 inválido"]); + return base64_encode(""); + } ... - $d->loadXML($xml); + if (@$d->loadXML($xml) === false) { + tools_reply(["Status" => 400, "text" => "XML mal formado"]); + return; + }Also applies to: 135-139
♻️ Duplicate comments (3)
api/contrib/signXML/Firmadohaciendacr.php (3)
128-133: Return immediately after tools_reply to stop further processing.Without an early return, execution continues and attempts to sign with invalid state.
if (is_null($this->publicKey) || is_null($this->privateKey) || is_null($xml)) { tools_reply([ "Status" => 400, "text" => "Al firmar, el documento o las claves no pueden estar vacías", ]); + return; }
174-186: Replace fragile parsing with DOM/XPath and compare the exact 12-digit identificación segment. Add early return.strpos/substr is brittle and checking “contains” can pass false positives. Extract the 12-digit identificación segment from CLAVE (pos 9, len 12) and compare to the certificate ID normalized and left-padded to 12.
- $startPos = strpos($xml, "<Clave>", 0); - $clave = substr($xml, $startPos+7, 50); - - $issuerCedula = $certData['subject']['serialNumber']; - $issuerCedula = preg_replace('/^(CPF|CPJ)/', '', $issuerCedula); - $issuerCedula = str_replace('-', '', $issuerCedula); - $issuerCedula = ltrim($issuerCedula, '0'); - if(strpos($clave, $issuerCedula) === false){ - tools_reply([ - "Status" => 400, - "text" => "El número de cédula del certificado ($issuerCedula) no coincide con el indicado en la CLAVE en el XML ($clave)", - ]); - } + $xpath = new DOMXPath($d); + $claveNode = $xpath->query('/*/*[local-name()="Clave"]')->item(0); + if (!$claveNode) { + tools_reply(["Status" => 400, "text" => "El XML no contiene la etiqueta <Clave>"]); + return; + } + $claveRaw = trim($claveNode->textContent); + $claveNorm = preg_replace('/\s+/', '', $claveRaw); + if (strlen($claveNorm) < 21) { // requiere al menos 21 para extraer desde pos 9, largo 12 + tools_reply(["Status" => 400, "text" => "El valor de <Clave> es inválido o incompleto"]); + return; + } + $issuerCedula = $certData['subject']['serialNumber']; + $issuerCedula = preg_replace('/^(CPF|CPJ)/', '', $issuerCedula); + $issuerCedula = str_replace('-', '', $issuerCedula); + $issuerCedula = ltrim($issuerCedula, '0'); + $issuerCedula12 = str_pad($issuerCedula, 12, '0', STR_PAD_LEFT); + $claveIdent = substr($claveNorm, 9, 12); + if ($claveIdent !== $issuerCedula12) { + tools_reply([ + "Status" => 400, + "text" => "La cédula del certificado ($issuerCedula12) no coincide con la identificación en la CLAVE ($claveIdent)" + ]); + return; + }
188-199: Use XPath for Emisor/Identificacion/Numero (and Receptor fallback). Add early return.The current strpos/substr will break on formatting changes or missing tags. Also consider MensajeReceptor where Receptor may be relevant.
- $startPos = strpos($xml, "<Emisor>", 0); - $startPos = strpos($xml, "<Identificacion>", $startPos); - $startPos = strpos($xml, "<Numero>", $startPos); - $endPos = strpos($xml, "</Numero>", $startPos); - $emisorNumero = substr($xml, $startPos+8, $endPos -($startPos+8)); - - if($emisorNumero !== $issuerCedula){ - tools_reply([ - "Status" => 400, - "text" => "El número de cédula del certificado ($issuerCedula) no coincide con el indicado en el EMISOR en el XML ($emisorNumero)", - ]); - } + $emisorNumeroNode = $xpath->query('/*/*[local-name()="Emisor"]/*[local-name()="Identificacion"]/*[local-name()="Numero"]')->item(0); + if (!$emisorNumeroNode) { + // fallback para documentos donde solo exista Receptor + $emisorNumeroNode = $xpath->query('/*/*[local-name()="Receptor"]/*[local-name()="Identificacion"]/*[local-name()="Numero"]')->item(0); + } + if (!$emisorNumeroNode) { + tools_reply(["Status" => 400, "text" => "El XML no contiene Emisor/Receptor con Identificacion/Numero"]); + return; + } + $emisorNumero = trim($emisorNumeroNode->textContent); + if ($emisorNumero !== $issuerCedula) { + tools_reply([ + "Status" => 400, + "text" => "El número de cédula del certificado ($issuerCedula) no coincide con el indicado en el XML ($emisorNumero)", + ]); + return; + }
🧹 Nitpick comments (3)
api/contrib/signXML/Firmadohaciendacr.php (3)
208-247: Ensure the SignedProperties digest matches the embedded element.You compute $propDigest from $aconop (with added xmlns) but embed $prop. If those differ in namespace declarations under canonicalization, digest verification can fail. Either embed the same string you hashed or hash what you embed.
Minimal tweak:
- $prop = '<xades:SignedProperties Id="' . $this->SignedProperties . '">' . + $prop = '<xades:SignedProperties Id="' . $this->SignedProperties . '">' . ... $aconop = str_replace('<xades:SignedProperties', '<xades:SignedProperties ' . $xmnls_signedprops, $prop); $propDigest = $this->retC14DigestSha1($aconop); + // Embed the exact content that was hashed + $prop = $aconop;And ensure the ds:Object later embeds $prop (unchanged).
52-60: Rename or document: retC14DigestSha1 uses SHA-256, not SHA-1.Name is misleading; either rename to retC14DigestSha256 or update the comment to avoid confusion.
- public function retC14DigestSha1($strcadena) + public function retC14DigestSha256($strcadena) ... - return base64_encode(hash('sha256', $strcadena, true)); + return base64_encode(hash('sha256', $strcadena, true));And update the few call sites accordingly.
162-162: Optional: derive timezone dynamically or from config.Hardcoding -06:00 is fine for CR, but consider conf_get to keep this portable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
api/contrib/signXML/Firmadohaciendacr.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
api/contrib/signXML/Firmadohaciendacr.php (1)
api/core/tools.php (1)
tools_reply(19-102)
| echo "Error: No se puede leer el almacén de certificados o la clave no es la correcta [$clavecertificado].\n"; | ||
| exit; | ||
| } |
There was a problem hiding this comment.
Do not log secrets (certificate passphrase leaked).
The error message includes [$clavecertificado], which is sensitive. Avoid printing passphrases anywhere (stdout, logs).
Apply:
- echo "Error: No se puede leer el almacén de certificados o la clave no es la correcta [$clavecertificado].\n";
+ echo "Error: No se puede leer el almacén de certificados o la clave no es la correcta.\n";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "Error: No se puede leer el almacén de certificados o la clave no es la correcta [$clavecertificado].\n"; | |
| exit; | |
| } | |
| echo "Error: No se puede leer el almacén de certificados o la clave no es la correcta.\n"; | |
| exit; |
🤖 Prompt for AI Agents
In api/contrib/signXML/Firmadohaciendacr.php around lines 98-100 the error
prints the certificate passphrase variable [$clavecertificado], leaking a
secret; remove the passphrase from any output and replace the echo with a
generic error message (e.g. "Error: No se puede leer el almacén de certificados
o la clave no es la correcta.") and exit; if additional context is needed for
debugging, log a non-sensitive identifier (certificate filename or fingerprint)
or store the detailed error in a secure audit log accessible only to authorized
operators, never include the passphrase in stdout/logs.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
api/tools/ValidadorXML.php (3)
81-96: Add explicit success return and harden XML load/validation (XXE-safe, base64 support, restore libxml state).On success, the function returns null. Also, XML is loaded without guarding against XXE/network fetch and without checking load failures. Patch below consolidates: base64 decode if provided, use LIBXML_NONET, return early on load failure, restore libxml error state on all paths, and return a success object.
- libxml_use_internal_errors(true); - $xml = new DOMDocument(); - $xml->loadXML($contenido_xml); - - if (!$xml->schemaValidate($schemas[$tipo_doc])) { - $errors = ValidadorXML::libxml_display_errors(); - $r = [ - "status" => "error", - "schema" => $schemas[$tipo_doc], - "message" => $errors, - "xml" => $contenido_xml - ]; - if(count($errors) === 0) { - $r["status"] = "ok"; - $r["message"] = "El XML es válido"; - unset($r["xml"]); - } - return (object) $r; - } + $prevUseInternal = libxml_use_internal_errors(true); + $xml = new DOMDocument(); + // Acepta XML en base64 o como cadena UTF-8 directa + $decoded = base64_decode($contenido_xml, true); + $xmlString = ($decoded !== false) ? $decoded : $contenido_xml; + if (@$xml->loadXML($xmlString, LIBXML_NONET) === false) { + $errors = ValidadorXML::libxml_display_errors(); + libxml_use_internal_errors($prevUseInternal); + return (object)[ + "status" => "error", + "message" => $errors ?: "Error: XML inválido o mal formado" + ]; + } + + if (!$xml->schemaValidate($schemas[$tipo_doc])) { + $errors = ValidadorXML::libxml_display_errors(); + $r = [ + "status" => "error", + "schema" => $schemas[$tipo_doc], + "message" => $errors ?: "Error: el XML no cumple con el XSD especificado", + ]; + // Solo incluir el XML con una bandera de configuración explícita + if (conf_get('includeXmlOnValidationError', 'xsd', false)) { + $r["xml"] = $xmlString; + } + libxml_use_internal_errors($prevUseInternal); + return (object) $r; + } + libxml_use_internal_errors($prevUseInternal); + return (object) ["status" => "ok", "message" => "El XML es válido"];
46-47: Avoid brittle path rewriting; add a dedicated config and robust fallback.The current regex only matches “…/api/” trailing slash and can yield “…/www/” double segment or miss “…/api” without slash. Prefer a dedicated xsd base path with a safe sibling fallback.
- $baseFolder = conf_get('coreInstall', 'modules', ''); - $baseFolder = preg_replace('/\/api\/$/', '/www/', $baseFolder); + // Prefer a dedicated config; fallback derives sibling 'www' next to 'api' + $baseFolder = rtrim(conf_get('xsdBasePath', 'core', ''), '/'); + if ($baseFolder === '') { + $root = rtrim(conf_get('coreInstall', 'modules', ''), '/'); + $baseFolder = preg_replace('#/api$#', '/www', $root); + }
48-55: Fix XSD mappings for "02" and "05".Debit Note ("02") and Message ("05") point to likely wrong XSDs. Update to NotaDebitoElectronica and MensajeReceptor respectively. Please verify filenames in www/xsd.
"01" => "$baseFolder/xsd/FacturaElectronica_V$version.xsd", - "02" => "$baseFolder/xsd/FacturaCompra_V$version.xsd", + "02" => "$baseFolder/xsd/NotaDebitoElectronica_V$version.xsd", "03" => "$baseFolder/xsd/NotaCreditoElectronica_V$version.xsd", "04" => "$baseFolder/xsd/TiqueteElectronico_V$version.xsd", - "05" => "$baseFolder/xsd/MensajeHacienda_V$version.xsd", + "05" => "$baseFolder/xsd/MensajeReceptor_V$version.xsd",To verify filenames exist for the selected version:
#!/bin/bash # List XSDs for v4.4 and check the two expected files fd -a --glob 'www/xsd/*V4.4*.xsd' || true
🧹 Nitpick comments (3)
api/tools/ValidadorXML.php (3)
6-23: Enrich error details with line and column numbers.Include line/column/code to speed up pinpointing schema violations.
switch ($error->level) { @@ } - $r["mensaje"] = $error->message; + $r["mensaje"] = trim($error->message); + if (isset($error->line)) $r["linea"] = $error->line; + if (isset($error->column)) $r["columna"] = $error->column; + if (isset($error->code)) $r["codigo"] = $error->code;
37-42: DocBlock mismatches parameters and encoding.Update docs to match the actual signature and accepted encodings (raw or base64).
- * @param string $contenido_xml - XML sin firmar en base64 - * @param string $tipo_doc - tipo de documento a validar - * @return type + * @param string $contenido_xml - XML sin firmar (cadena UTF-8 o base64) + * @param string $consecutivo - consecutivo; de aquí se deriva el tipo de documento + * @return object - {status: "ok"|"error", message: string|array, schema?: string, xml?: string}
45-56: Validateconsecutivobefore extractingtipo_doc.Insert the following check before the existing
substrcall in api/tools/ValidadorXML.php:- $tipo_doc = substr($consecutivo, 8, 2); + if (!is_string($consecutivo) || strlen($consecutivo) < 10) { + return (object)[ + "status" => "error", + "message" => "Error: consecutivo inválido; no se puede derivar el tipo de documento" + ]; + } + $tipo_doc = substr($consecutivo, 8, 2);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/tools/ValidadorXML.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
api/tools/ValidadorXML.php (1)
api/core/conf.php (1)
conf_get(26-34)
api/tools/ValidadorXML.php
Outdated
| public static function validateXml($contenido_xml, $consecutivo, $version = "4.4") | ||
| { | ||
| $tipo_doc = substr($consecutivo, 8, 2); | ||
| $baseFolder = conf_get('coreInstall', 'modules', ''); | ||
| $baseFolder = preg_replace('/\/api\/$/', '/www/', $baseFolder); | ||
| $schemas = [ | ||
| "01" => "$baseFolder/xsd/FacturaElectronica_V$version.xsd", | ||
| "02" => "$baseFolder/xsd/FacturaCompra_V$version.xsd", | ||
| "03" => "$baseFolder/xsd/NotaCreditoElectronica_V$version.xsd", | ||
| "04" => "$baseFolder/xsd/TiqueteElectronico_V$version.xsd", | ||
| "05" => "$baseFolder/xsd/MensajeHacienda_V$version.xsd", | ||
| ]; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate and sanitize the $version input to prevent path traversal.
Since $version interpolates into a filesystem path, constrain it to digits and dots.
- $schemas = [
+ if (!preg_match('/^\d+(?:\.\d+)*$/', $version)) {
+ return (object)[
+ "status" => "error",
+ "message" => "Error: versión inválida ($version)"
+ ];
+ }
+ $schemas = [📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static function validateXml($contenido_xml, $consecutivo, $version = "4.4") | |
| { | |
| $tipo_doc = substr($consecutivo, 8, 2); | |
| $baseFolder = conf_get('coreInstall', 'modules', ''); | |
| $baseFolder = preg_replace('/\/api\/$/', '/www/', $baseFolder); | |
| $schemas = [ | |
| "01" => "$baseFolder/xsd/FacturaElectronica_V$version.xsd", | |
| "02" => "$baseFolder/xsd/FacturaCompra_V$version.xsd", | |
| "03" => "$baseFolder/xsd/NotaCreditoElectronica_V$version.xsd", | |
| "04" => "$baseFolder/xsd/TiqueteElectronico_V$version.xsd", | |
| "05" => "$baseFolder/xsd/MensajeHacienda_V$version.xsd", | |
| ]; | |
| public static function validateXml($contenido_xml, $consecutivo, $version = "4.4") | |
| { | |
| $tipo_doc = substr($consecutivo, 8, 2); | |
| $baseFolder = conf_get('coreInstall', 'modules', ''); | |
| $baseFolder = preg_replace('/\/api\/$/', '/www/', $baseFolder); | |
| // Ensure $version contains only digits and dots to avoid path traversal | |
| if (!preg_match('/^\d+(?:\.\d+)*$/', $version)) { | |
| return (object)[ | |
| "status" => "error", | |
| "message" => "Error: versión inválida ($version)" | |
| ]; | |
| } | |
| $schemas = [ | |
| "01" => "$baseFolder/xsd/FacturaElectronica_V$version.xsd", | |
| "02" => "$baseFolder/xsd/FacturaCompra_V$version.xsd", | |
| "03" => "$baseFolder/xsd/NotaCreditoElectronica_V$version.xsd", | |
| "04" => "$baseFolder/xsd/TiqueteElectronico_V$version.xsd", | |
| "05" => "$baseFolder/xsd/MensajeHacienda_V$version.xsd", | |
| ]; |
🤖 Prompt for AI Agents
In api/tools/ValidadorXML.php around lines 43 to 55, the $version value is
interpolated directly into filesystem paths which allows path traversal;
validate and sanitize $version before use by restricting it to an allowlist or a
strict pattern (e.g. only digits and dots) and reject or normalize any other
input, or map allowed version strings to known filenames; then use the
sanitized/mapped version when composing the $schemas paths so only safe,
expected paths are constructed.
api/tools/ValidadorXML.php
Outdated
| $r = [ | ||
| "status" => "error", | ||
| "schema" => $schemas[$tipo_doc], | ||
| "message" => $errors, | ||
| "xml" => $contenido_xml | ||
| ]; | ||
| if(count($errors) === 0) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Do not echo back full XML by default (PII leakage risk).
Returning the entire XML in errors can leak sensitive invoice data. Gate it behind an explicit config flag (as in the consolidated patch) or remove it.
- $r = [
+ $r = [
"status" => "error",
"schema" => $schemas[$tipo_doc],
"message" => $errors,
- "xml" => $contenido_xml
];
+ // Opcional: solo incluir el XML si está habilitado explícitamente
+ if (conf_get('includeXmlOnValidationError', 'xsd', false)) {
+ $r["xml"] = $contenido_xml;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $r = [ | |
| "status" => "error", | |
| "schema" => $schemas[$tipo_doc], | |
| "message" => $errors, | |
| "xml" => $contenido_xml | |
| ]; | |
| if(count($errors) === 0) { | |
| $r = [ | |
| "status" => "error", | |
| "schema" => $schemas[$tipo_doc], | |
| "message" => $errors, | |
| ]; | |
| // Opcional: solo incluir el XML si está habilitado explícitamente | |
| if (conf_get('includeXmlOnValidationError', 'xsd', false)) { | |
| $r["xml"] = $contenido_xml; | |
| } | |
| if (count($errors) === 0) { |
🤖 Prompt for AI Agents
In api/tools/ValidadorXML.php around lines 83 to 89, the code currently includes
the full XML content in the error response which risks leaking PII; change this
so the "xml" field is only included when an explicit configuration flag (e.g.
SHOW_FULL_XML_ON_ERROR or similar) is enabled, defaulting to false. Implement a
config lookup at the top of the function or class, remove "xml" from the default
$r array, and conditionally add $r['xml'] = $contenido_xml (or a
redacted/truncated version) only if the config flag is true; ensure the flag is
documented and defaults to off so production does not expose full XML by
default.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/contrib/genXML/module.php (1)
523-523: Remove trailing blank line.There's an unnecessary trailing blank line at the end of the file.
Apply this diff to remove the trailing blank line:
-
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/contrib/genXML/module.php(7 hunks)www/xsd/NotaCreditoElectronica_V4.4.xsd(0 hunks)
💤 Files with no reviewable changes (1)
- www/xsd/NotaCreditoElectronica_V4.4.xsd
🔇 Additional comments (6)
api/contrib/genXML/module.php (6)
121-121: LGTM - Consistent with system-wide optimization.The change to make
informacion_referencianon-required aligns with the broader genXML refactor mentioned in the AI summary. This parameter is now consistently marked as optional across all document types (FE, NC, ND, TE, FEC, FEE).
196-196: LGTM - Consistent parameter optimization.Making
informacion_referenciaoptional for credit notes (NC) maintains consistency with the FE path changes and supports the broader genXML validation improvements.
271-271: LGTM - Consistent parameter optimization.Making
informacion_referenciaoptional for debit notes (ND) maintains consistency with other document types in this refactor.
345-345: LGTM - Consistent parameter optimization.Making
informacion_referenciaoptional for electronic tickets (TE) maintains consistency with other document types in this refactor.
418-418: LGTM - Consistent parameter optimization.Making
informacion_referenciaoptional for purchase invoices (FEC) maintains consistency with other document types in this refactor.
478-478: LGTM - Consistent parameter optimization.Making
informacion_referenciaoptional for export invoices (FEE) maintains consistency with other document types in this refactor.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
api/core/tools.php (2)
57-61: Guard array access and reindex after unset.The code assumes
$response['text']exists and is an array without verification, and afterunset($response['text'][0]), the array may not be zero-indexed for JSON encoding.Based on learnings
Apply this diff:
- if (is_numeric($response['Status'])) { + if (is_numeric($response['Status'])) { $httpStatus = $response['Status']; $killMe = $response['Status'] >= 299; - if(is_array($response['text']) && count($response['text']) > 1) { + if (isset($response['text']) && is_array($response['text']) && count($response['text']) > 1) { // cuando la respuesta es un array de Hacienda, el primer elemento es el código de estado // y el resto es el mensaje unset($response['text'][0]); + $response['text'] = array_values($response['text']); // reindexar para JSON } }
77-77: Guard against missing 'text' key.The assignment assumes
$response['text']exists, which may not be true for all response structures.Apply this diff:
- $response = $response['text']; + $response = $response['text'] ?? $response;api/contrib/consultar/consultar.php (5)
84-88: Fix falsy strpos check and inconsistent return format.
- Line 84:
!$startPostreats position 0 as "not found"- Line 87: Returns
$responseObjdirectly instead of the wrapped array format used elsewhereBased on learnings
Apply this diff:
$startLength = strlen("tiene los siguientes errores: ") + 7; $startPos = strpos($xmlRespuesta, "tiene los siguientes errores: ", 0); - if (!$startPos) { + if ($startPos === false) { // si no hay errores, retornar la respuesta normal - return $responseObj; + return array( + "Status" => $status, + "to" => $url, + "text" => $responseObj + ); }
90-90: Fix off-by-one error in substr length calculation.The calculation adds an extra
+1that shortens the extracted message by one character.Based on learnings
Apply this diff:
- $detalleMensaje = substr($xmlRespuesta, $startPos + $startLength, $endPos - ($startPos + $startLength + 1)); + $detalleMensaje = substr($xmlRespuesta, $startPos + $startLength, $endPos - ($startPos + $startLength));
111-115: Guard against missing Hacienda headers.The code assumes
x-http-statusandx-error-causeheaders always exist, which may not be true.Based on learnings
Apply this diff:
return array( - "Status" => $header_array['x-http-status'], + "Status" => isset($header_array['x-http-status']) ? (int)$header_array['x-http-status'] : $status, "to" => $url, - "text" => $header_array['x-error-cause'] + "text" => $header_array['x-error-cause'] ?? 'Error desconocido' );
151-152: Enable TLS verification for production security.Same security issue as in
consultarRecepcion- SSL verification is disabled.Apply this diff:
- CURLOPT_SSL_VERIFYHOST => 0, - CURLOPT_SSL_VERIFYPEER => 0, + CURLOPT_SSL_VERIFYHOST => 2, + CURLOPT_SSL_VERIFYPEER => 1, + CURLOPT_CONNECTTIMEOUT => 10,
42-43: Enable TLS verification for production security.Disabling SSL verification (VERIFYHOST=0, VERIFYPEER=0) exposes the application to man-in-the-middle attacks.
Apply this diff:
- CURLOPT_SSL_VERIFYHOST => 0, - CURLOPT_SSL_VERIFYPEER => 0, + CURLOPT_SSL_VERIFYHOST => 2, + CURLOPT_SSL_VERIFYPEER => 1, + CURLOPT_CONNECTTIMEOUT => 10,
🧹 Nitpick comments (4)
api/contrib/consultar/consultar.php (4)
24-25: Use empty() to avoid strlen warnings on null.Using
strlen()on a potentially null value can emit warnings. The validation also has redundant checks.Apply this diff:
- if ($clave == "" || strlen($clave) == 0) + if (empty($clave) || !is_string($clave)) return "La clave no puede ser en blanco";
81-81: Use strict mode for base64_decode.Without strict mode,
base64_decodereturns false for invalid input but may also return an empty string, making error detection ambiguous.Apply this diff:
- $xmlRespuesta = base64_decode($responseObj->{'respuesta-xml'}); + $xmlRespuesta = base64_decode($responseObj->{'respuesta-xml'}, true); + if ($xmlRespuesta === false) { + return array("Status" => $status, "to" => $url, "text" => $responseObj); + }
131-133: Use empty() to avoid strlen warnings on null.Same issue as in
consultarRecepcion-strlen()can warn on null values.Apply this diff:
- if ($clave == "" || strlen($clave) == 0) { + if (empty($clave) || !is_string($clave)) { return "La clave no puede ser en blanco"; }
199-205: Remove unused JSON decode or use decoded object.Line 199 decodes
$bodyinto$responseObjbut line 204 returns the raw$bodystring instead. Either remove the unused decode or return$responseObjas the text field.If you intend to return the decoded object:
$responseObj = json_decode($body); return tools_reply(array( "Status" => $status, "to" => $url, - "text" => $body + "text" => $responseObj ?? $body ), true);Or if raw body is intended, remove the unused decode:
- $responseObj = json_decode($body); - return tools_reply(array( "Status" => $status, "to" => $url, "text" => $body ), true);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/contrib/consultar/consultar.php(2 hunks)api/core/tools.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
api/core/tools.php (1)
api/core/conf.php (1)
conf_get(26-34)
api/contrib/consultar/consultar.php (2)
api/core/params.php (1)
params_get(24-36)api/core/tools.php (1)
tools_reply(19-103)
🪛 PHPMD (2.15.0)
api/contrib/consultar/consultar.php
199-199: Avoid unused local variables such as '$responseObj'. (undefined)
(UnusedLocalVariable)
| return tools_reply(array( | ||
| "Status" => $header_array['x-http-status'] ?? $status, | ||
| "to" => $url, | ||
| "text" => $header_array['x-error-cause'] | ||
| ), true); |
There was a problem hiding this comment.
Guard x-error-cause access consistently.
Line 193 correctly guards x-http-status with null coalescing, but line 195 still accesses x-error-cause without verification.
Apply this diff:
return tools_reply(array(
"Status" => $header_array['x-http-status'] ?? $status,
"to" => $url,
- "text" => $header_array['x-error-cause']
+ "text" => $header_array['x-error-cause'] ?? 'Error desconocido'
), true);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return tools_reply(array( | |
| "Status" => $header_array['x-http-status'] ?? $status, | |
| "to" => $url, | |
| "text" => $header_array['x-error-cause'] | |
| ), true); | |
| return tools_reply(array( | |
| "Status" => $header_array['x-http-status'] ?? $status, | |
| "to" => $url, | |
| "text" => $header_array['x-error-cause'] ?? 'Error desconocido' | |
| ), true); |
🤖 Prompt for AI Agents
In api/contrib/consultar/consultar.php around lines 192 to 196, the return
builds a response but accesses $header_array['x-error-cause'] without guarding
it; change that access to use a null-coalescing fallback (or isset) like
$header_array['x-error-cause'] ?? null (or empty string) so the key is checked
before use and prevents undefined index notices, leaving the rest of the return
structure unchanged.
| } else { | ||
| http_response_code(200); | ||
| $httpStatus = 200; | ||
| } |
There was a problem hiding this comment.
Remove status override that discards computed HTTP status.
Line 85 unconditionally sets $httpStatus = 200 for non-error paths, discarding the status computed earlier (lines 54-76). This means success responses always return 200 even if a different success status was intended.
Apply this diff:
if ($killMe) {
if (is_string($response)) {
$response = "ERROR: " . $response;
}
- } else {
- $httpStatus = 200;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else { | |
| http_response_code(200); | |
| $httpStatus = 200; | |
| } | |
| if ($killMe) { | |
| if (is_string($response)) { | |
| $response = "ERROR: " . $response; | |
| } | |
| } |
🤖 Prompt for AI Agents
In api/core/tools.php around lines 84 to 86, remove the unconditional override
that sets $httpStatus = 200; so that previously computed status values are
preserved; either delete the else block entirely or change it to only set a
default when $httpStatus is not already set (e.g., check for null/empty) so
success responses keep their intended HTTP status.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/modules/wirez/messagesSend.php (1)
56-56: Fix incorrect json_decode usage and double-escaping.Line 56 has a critical bug:
ENT_QUOTESis not a valid parameter forjson_decode. The second parameter expects a boolean (associative), not an HTML entity constant.- When coerced to
true,json_decodereturns an array, causingaddslashes()to fail with a type error.- Using both
addslashes()anddb_escape()is redundant sincedb_escape()already handles escaping.Apply this diff to fix the issue:
- db_escape(addslashes(json_decode(params_get('text',''), ENT_QUOTES))), + db_escape(json_decode(params_get('text',''))),
♻️ Duplicate comments (2)
api/core/tools.php (2)
52-78: Previous review feedback not addressed: guard$response['text']access and reindex array.The issues flagged in the previous review remain:
- Line 57 accesses
$response['text']without verifying the key exists- Line 60's
unset($response['text'][0])leaves a gap in array indices, causingjson_encodeto produce an object instead of an array- Line 77 assigns
$response['text']without a fallbackApply this diff:
if (is_array($response) && isset($response['Status'])) { // propagar el codigo de estado HTTP cuando se recibe en el error if (is_numeric($response['Status'])) { $httpStatus = $response['Status']; $killMe = $response['Status'] >= 299; - if(is_array($response['text']) && count($response['text']) > 1) { + if(isset($response['text']) && is_array($response['text']) && count($response['text']) > 1) { // cuando la respuesta es un array de Hacienda, el primer elemento es el código de estado // y el resto es el mensaje unset($response['text'][0]); + $response['text'] = array_values($response['text']); // reindex for JSON array } } else { switch ($response['Status']) { case 'error': $httpStatus = 500; $killMe = true; break; case 'ok': $httpStatus = 200; break; default: $httpStatus = 400; $killMe = true; } } - $response = $response['text']; + $response = $response['text'] ?? $response; }
80-86: Previous review feedback not addressed: remove status override that discards computed HTTP status.Line 85 unconditionally sets
$httpStatus = 200, discarding any success status computed earlier (e.g., a 201 or 202 from Hacienda). This means all non-error responses return 200 regardless of the original status.Apply this diff:
if ($killMe) { if (is_string($response)) { $response = "ERROR: " . $response; } - } else { - $httpStatus = 200; }
🧹 Nitpick comments (2)
api/modules/wirez/messagesSend.php (1)
74-78: Consider using prepared statements for better security.While
db_escape()provides basic SQL injection protection, prepared statements with parameterized queries offer stronger security guarantees and are the recommended best practice for database queries.Example refactor using prepared statements:
$stmt = $dbConn->prepare("INSERT INTO conversations (idUser, idRecipient, timestamp, subject) VALUES(?, ?, ?, ?)"); $stmt->bind_param("ssss", $idUser, $idRecipient, $timestamp, $subject); $stmt->execute();api/core/tools.php (1)
88-101: CLI path uses exception for response - verify this is intentional.Throwing an exception containing the JSON response (lines 89-93) is unusual for CLI mode. Typically, CLI should print output and exit. If this pattern is intentional (e.g., for testing frameworks that catch exceptions), consider adding a comment explaining the design choice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/core/tools.php(2 hunks)api/modules/wirez/messagesSend.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
api/core/tools.php (1)
api/core/conf.php (1)
conf_get(26-34)
api/modules/wirez/messagesSend.php (1)
api/modules/db/module.php (1)
db_escape(151-155)
🔇 Additional comments (3)
api/modules/wirez/messagesSend.php (1)
78-78: Critical syntax fix - LGTM!This change correctly closes the
sprintfcall by adding the missing closing parenthesis. Without this fix, the code would fail with a PHP parse error.api/core/tools.php (2)
21-50: Initial error mapping looks reasonable.The switch statement correctly maps error constants to HTTP status codes. Note that HTTP 440 (line 34) is a non-standard Microsoft extension; consider using 401 or 419 for broader client compatibility if needed, though this is acceptable for internal use.
104-117: LGTM!The function correctly handles output, logging, and exit. The TODO comment about relocating helper functions is noted.
| function tools_returnJson($response, $addHeaders = true) | ||
| { | ||
| if ($addHeaders && conf_get('mode', 'core', 'web') != 'cli') { | ||
| http_response_code($response['httpStatus']); | ||
| header('Content-Type: text/html; charset=utf-8'); | ||
| header('Content-Type: application/json'); | ||
| } | ||
|
|
||
| return json_encode($response); | ||
| } |
There was a problem hiding this comment.
Guard httpStatus access and remove duplicate Content-Type header.
Two issues:
- Line 122 accesses
$response['httpStatus']without verifying the key exists—callers outsidetools_replymay omit it. - Lines 123-124 set
Content-Typetwice; the second header overwrites the first, making line 123 redundant.
Apply this diff:
function tools_returnJson($response, $addHeaders = true)
{
if ($addHeaders && conf_get('mode', 'core', 'web') != 'cli') {
- http_response_code($response['httpStatus']);
- header('Content-Type: text/html; charset=utf-8');
+ http_response_code($response['httpStatus'] ?? 200);
header('Content-Type: application/json');
}
-
+
return json_encode($response);
}🤖 Prompt for AI Agents
In api/core/tools.php around lines 119 to 128, guard access to
$response['httpStatus'] before calling http_response_code and remove the
duplicate Content-Type header: check with isset($response['httpStatus']) (and
optionally is_int/is_numeric) before calling
http_response_code($response['httpStatus']), and replace the two header() calls
with a single header('Content-Type: application/json; charset=utf-8') so
Content-Type is set only once.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| tools_reply([ | ||
| "Status" => 400, | ||
| "text" => "Al firmar, el documento o las claves no pueden estar vacías", | ||
| ]); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
api/tools/ValidadorXML.php (5)
66-67: Path construction remains brittle despite regex improvement.The
preg_replace('/\/api\/$/', '/www/', ...)approach only works when the path ends exactly with/api/. If the configuration changes or doesn't match this pattern, the XSD files won't be found. As previously suggested, use a dedicated config key likeconf_get('basePath','xsd','')to avoid fragile string manipulation.
63-74: Critical: Validate $version parameter to prevent path traversal.The
$versionparameter is interpolated directly into filesystem paths (lines 69-73) without validation. An attacker could pass values like"../../../etc/passwd"to access arbitrary files. Validate that$versionmatches an expected pattern (e.g.,/^\d+\.\d+$/) before use.🔎 Proposed fix
public static function validateXml(string $contenido_xml, string $consecutivo, $version = "4.4"): object { + // Validate version to prevent path traversal + if (!preg_match('/^\d+\.\d+$/', $version)) { + return (object) [ + "status" => "error", + "message" => "Error: versión inválida ($version)" + ]; + } + $tipo_doc = substr($consecutivo, 8, 2);
68-74: Incorrect XSD mappings for document types "02" and "05".Based on Costa Rica's electronic invoicing specifications:
- Code "02" represents Nota de Débito Electrónica (Debit Note), not Factura de Compra
- Code "05" represents Mensaje Receptor (Acceptance Message), not Mensaje Hacienda
These incorrect mappings will cause validation failures for legitimate documents.
🔎 Proposed fix
$schemas = [ "01" => "$baseFolder/xsd/FacturaElectronica_V$version.xsd", - "02" => "$baseFolder/xsd/FacturaCompra_V$version.xsd", + "02" => "$baseFolder/xsd/NotaDebitoElectronica_V$version.xsd", "03" => "$baseFolder/xsd/NotaCreditoElectronica_V$version.xsd", "04" => "$baseFolder/xsd/TiqueteElectronico_V$version.xsd", - "05" => "$baseFolder/xsd/MensajeHacienda_V$version.xsd", + "05" => "$baseFolder/xsd/MensajeReceptor_V$version.xsd", ];Verify the correct XSD filenames exist in the repository:
#!/bin/bash # Check for XSD files to confirm correct naming conventions fd -t f -e xsd . www/xsd/ | grep -E "(NotaDebito|MensajeReceptor|FacturaCompra)" | head -20
97-99: Critical: XML loading vulnerable to XXE attacks and missing error handling.The
loadXMLcall lacks theLIBXML_NONETflag, allowing external entity/network fetches (XXE vulnerability). Additionally, there's no check for load failures, which could cause unexpected behavior with malformed XML.🔎 Proposed fix
libxml_use_internal_errors(true); $xml = new DOMDocument(); - $xml->loadXML($contenido_xml); + if (@$xml->loadXML($contenido_xml, LIBXML_NONET) === false) { + $errors = self::libxml_display_errors(); + return (object) [ + "status" => "error", + "message" => empty($errors) ? "XML inválido o mal formado" : $errors + ]; + }
101-115: PII leakage risk: Full XML content exposed in error responses.Line 107 includes the entire XML document in error responses, which may contain sensitive invoice data (customer names, tax IDs, amounts, etc.). This poses a privacy and compliance risk, especially under regulations like GDPR or Costa Rica's data protection laws.
Gate this behind an explicit configuration flag that defaults to
false, or remove it entirely.🔎 Proposed fix
$errors = self::libxml_display_errors(); $r = [ "status" => "error", "schema" => $schemas[$tipo_doc], "message" => $errors, - "xml" => $contenido_xml ]; + // Only include XML if explicitly enabled for debugging + if (conf_get('includeXmlOnValidationError', 'xsd', false)) { + $r["xml"] = $contenido_xml; + }
🧹 Nitpick comments (2)
api/contrib/firmarXML/xmlseclibs/src/XMLSecurityDSig.php (1)
283-303: Refactor switch-case to if-elseif-else for clarity.The use of
strpos() !== falseas case expressions in a switch statement is non-idiomatic and confusing. While it works due to PHP's loose comparison, an if-elseif-else structure would be clearer and more maintainable. Additionally, both v4.3 and v4.4 cases have identical implementation, which could be consolidated.🔎 Proposed refactor
public function setSignPolicy(){ + if ($this->xmlFirstChild === null) { + throw new Exception("xmlFirstChild is not initialized"); + } $xmlns = $this->xmlFirstChild->getAttribute('xmlns'); - switch ($xmlns){ - case (strpos($xmlns, 'v4.3') !== false): - $this->signPolicy = [ - "name" => "", - "url" => "https://cdn.comprobanteselectronicos.go.cr/xml-schemas/Resoluci%C3%B3n_General_sobre_disposiciones_t%C3%A9cnicas_comprobantes_electr%C3%B3nicos_para_efectos_tributarios.pdf", - "digest" => "DWxin1xWOeI8OuWQXazh4VjLWAaCLAA954em7DMh0h8=" // Base64_Encode(Hash_File(SHA_256)) - ]; - break; - case (strpos($xmlns, 'v4.4') !== false): - $this->signPolicy = [ - "name" => "", - "url" => "https://cdn.comprobanteselectronicos.go.cr/xml-schemas/Resoluci%C3%B3n_General_sobre_disposiciones_t%C3%A9cnicas_comprobantes_electr%C3%B3nicos_para_efectos_tributarios.pdf", - "digest" => "DWxin1xWOeI8OuWQXazh4VjLWAaCLAA954em7DMh0h8=" // Base64_Encode(Hash_File(SHA_256)) - ]; - break; - default: - throw new Exception("Cannot validate version: Unsupported Version"); + if (strpos($xmlns, 'v4.3') !== false || strpos($xmlns, 'v4.4') !== false) { + $this->signPolicy = [ + "name" => "", + "url" => "https://cdn.comprobanteselectronicos.go.cr/xml-schemas/Resoluci%C3%B3n_General_sobre_disposiciones_t%C3%A9cnicas_comprobantes_electr%C3%B3nicos_para_efectos_tributarios.pdf", + "digest" => "DWxin1xWOeI8OuWQXazh4VjLWAaCLAA954em7DMh0h8=" // Base64_Encode(Hash_File(SHA_256)) + ]; + } else { + throw new Exception("Cannot validate version: Unsupported Version"); } }api/tools/ValidadorXML.php (1)
144-147: Add input length validation to prevent warnings.The method extracts positions 8-9 from
$consecutivowithout validating the string length. If a short or invalid consecutivo is passed, this could produce warnings or unexpected results.🔎 Proposed fix
public static function getDocumentTypeFromConsecutivo(string $consecutivo): string { + if (strlen($consecutivo) < 10) { + return ''; // or throw an exception + } return substr($consecutivo, 8, 2); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/contrib/firmarXML/xmlseclibs/src/XMLSecurityDSig.phpapi/contrib/genXML/genXML.phpapi/tools/ValidadorXML.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Seer Code Review
🔇 Additional comments (3)
api/contrib/firmarXML/xmlseclibs/src/XMLSecurityDSig.php (1)
286-299: Verify whether v4.3 and v4.4 should share the same policy document or if v4.3 requires its own.v4.3 and v4.4 cases use identical URL and digest values. v4.3 appears only in this switch statement with no other references in the codebase, while v4.4 is actively used throughout the schema generation code. Confirm whether:
- Both versions intentionally reference the same policy document, or
- v4.3 should have a distinct policy URL/digest (or be removed if no longer supported)
api/tools/ValidadorXML.php (2)
19-53: LGTM: Error formatting helpers are well-structured.The private helper methods correctly format libxml errors into structured arrays and properly clear errors after collection.
117-120: Well done: Success return statement correctly added.This properly addresses the previous issue where the function would return
nullfor valid XML. The success case now returns a consistent object structure.
| @@ -283,6 +283,13 @@ public function setCanonicalMethod($method) | |||
| public function setSignPolicy(){ | |||
| $xmlns = $this->xmlFirstChild->getAttribute('xmlns'); | |||
There was a problem hiding this comment.
Add null check before accessing xmlFirstChild.
The xmlFirstChild property is nullable (declared as DOMElement|null at Line 99), but there's no null check before calling getAttribute(). This could cause a fatal error if xmlFirstChild is not initialized.
🔎 Proposed fix
public function setSignPolicy(){
+ if ($this->xmlFirstChild === null) {
+ throw new Exception("xmlFirstChild is not initialized");
+ }
$xmlns = $this->xmlFirstChild->getAttribute('xmlns');🤖 Prompt for AI Agents
In api/contrib/firmarXML/xmlseclibs/src/XMLSecurityDSig.php around line 284, the
code accesses $this->xmlFirstChild without checking for null; add a null check
before calling getAttribute() — if xmlFirstChild is null, handle it by returning
a sensible default (e.g., empty string), throwing a clear exception, or
otherwise short-circuiting the method; update the code to only call
$this->xmlFirstChild->getAttribute('xmlns') when xmlFirstChild is an instance of
DOMElement and document the chosen behavior in a brief comment.
| public static function getSupportedDocumentTypes(): array | ||
| { | ||
| return [ | ||
| "01" => "Factura Electrónica", | ||
| "02" => "Factura de Compra", | ||
| "03" => "Nota de Crédito Electrónica", | ||
| "04" => "Tiquete Electrónico", | ||
| "05" => "Mensaje Hacienda", | ||
| ]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Document type label for "05" inconsistent with corrected mapping.
The label "Mensaje Hacienda" should be "Mensaje Receptor" to match the corrected XSD mapping for acceptance confirmation messages.
🔎 Proposed fix
return [
"01" => "Factura Electrónica",
"02" => "Factura de Compra",
"03" => "Nota de Crédito Electrónica",
"04" => "Tiquete Electrónico",
- "05" => "Mensaje Hacienda",
+ "05" => "Mensaje Receptor",
];Note: The label for "02" should also be updated to "Nota de Débito Electrónica" once the XSD mapping is corrected.
🤖 Prompt for AI Agents
In api/tools/ValidadorXML.php around lines 127 to 136, update the document type
labels to match the corrected XSD mapping: change the label for key "05" from
"Mensaje Hacienda" to "Mensaje Receptor", and proactively change the label for
key "02" from "Factura de Compra" to "Nota de Débito Electrónica" so the mapping
stays consistent once the XSD is updated.
| return tools_reply(array( | ||
| "Status" => $header_array['x-http-status'] ?? $status, | ||
| "to" => $url, | ||
| "text" => $header_array['x-error-cause'] | ||
| ), true); |
There was a problem hiding this comment.
Bug: The function consultarComprobante calls tools_reply, which contains an exit statement. This causes the script to terminate abruptly instead of returning a value to the caller.
Severity: HIGH | Confidence: High
🔍 Detailed Analysis
The function consultarComprobante has several paths, including on cURL error or when the response body is empty, where it calls return tools_reply(...). The tools_reply function, via _tools_reply, executes an exit; statement, which terminates the entire PHP script. This prevents the return statement from ever executing and violates the expectation of the calling function tools_proccesPath, which anticipates a return value. Instead of a graceful error response, the request will terminate unexpectedly, making the actual error difficult to diagnose.
💡 Suggested Fix
In consultarComprobante, instead of calling tools_reply, construct the response array and return it directly. This allows the response to propagate back to the calling function tools_proccesPath for proper handling, consistent with the behavior of the sibling function consultarRecepcion.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: api/contrib/consultar/consultar.php#L192-L196
Potential issue: The function `consultarComprobante` has several paths, including on
cURL error or when the response body is empty, where it calls `return tools_reply(...)`.
The `tools_reply` function, via `_tools_reply`, executes an `exit;` statement, which
terminates the entire PHP script. This prevents the `return` statement from ever
executing and violates the expectation of the calling function `tools_proccesPath`,
which anticipates a return value. Instead of a graceful error response, the request will
terminate unexpectedly, making the actual error difficult to diagnose.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8114258
| $httpStatus = $response['Status']; | ||
| $killMe = $response['Status'] >= 299; |
There was a problem hiding this comment.
Bug: The $killMe error flag is incorrectly overwritten based on the Status code, causing connection failures to be reported as HTTP 200 OK successes.
Severity: HIGH
Suggested Fix
The logic should not overwrite $killMe if it's already true. Change the assignment to only set $killMe if it's not already set, for example: if ($killMe !== true) { $killMe = $response['Status'] >= 299; }.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: api/core/tools.php#L55-L56
Potential issue: The `tools_reply()` function unconditionally overwrites the `$killMe`
parameter if the response array contains a numeric `Status`. When a caller, such as
`consultarComprobante()`, experiences a cURL connection failure, it passes `Status: 0`
and `$killMe: true`. The line `$killMe = $response['Status'] >= 299;` then overwrites
`$killMe` to `false` (since `0 >= 299` is false). This causes the function to
incorrectly report a successful HTTP 200 OK response to the client, masking the
underlying connection failure.
Did we get this right? 👍 / 👎 to inform future reviews.
Summary by CodeRabbit
New Features
Security
Improvements
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.