View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0009461 | Taler | merchant backend | public | 2025-01-16 15:42 | 2025-07-12 15:38 |
| Reporter | fefe | Assigned To | Christian Grothoff | Resolution deadline | |
| Implementer | Reviewer | ||||
| Release signed by | Deployed by | ||||
| Verifier | Communicator | ||||
| Data protection authority notification required | No | ||||
| CSIRT notification required | No | ||||
| Estimated effort (hours) | Actual effort (hours) | 0 | |||
| Fast-track | No | ||||
| Work package | none | ||||
| Priority | normal | Severity | minor | Reproducibility | have not tried |
| Status | closed | Resolution | no change required | ||
| Product Version | git (master) | ||||
| Target Version | 1.0 stretch goals | Fixed in Version | 1.0 stretch goals | ||
| Summary | 0009461: determine_eligible_accounts: no escaping when building URL | ||||
| Description | ``` 762 if (TALER_amount_is_zero (&kyc_amount)) 763 GNUNET_asprintf (&payto_kycauth, 764 "%s%cmessage=KYC:%s", 765 exchange_account_payto, 766 (NULL == strchr (exchange_account_payto, 767 '?')) 768 ? '?' 769 : '&', 770 merchant_pub_str); 771 else 772 GNUNET_asprintf (&payto_kycauth, 773 "%s%camount=%s&message=KYC:%s", 774 exchange_account_payto, 775 (NULL == strchr (exchange_account_payto, 776 '?')) 777 ? '?' 778 : '&', 779 TALER_amount2s (&kyc_amount), 780 merchant_pub_str); ``` merchant_pub_str comes from GNUNET_STRINGS_data_to_string_alloc so it should be OK, but I'm not so sure about exchange_account_payto. Generally speaking I think abstractions exist to take burden away from the caller. This API doesn't. What if any of the fields contain a '&'? There should be no injection possibility here. Everything should be urlencoded on demand. That does not hurt if it's not needed but prevents catastrophe if it does. | ||||
| Tags | No tags attached. | ||||
| As you say: we know merchant_pub_str was just base32 encoded by us, so it must be fine. The base exchange_account_payto MUST already be a URI, and we did validate that one extensively when we received it. Plus, it may contain complex payment instructions, we are just expected to append one more argument. Parsing the URI first, and then encoding it again -- what would that accomplish? | |
| I double-checked, this is just adding/appending the amount and message arguments to an existing payto URI. We just built the arguments, so there is no chance of '&' or '?' being in them. And the prefix is known to already be a well-formed URI. Escaping it again would be wrong, and parsing it again also pointless. So I think this is fine. Resolving. | |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2025-01-16 15:42 | fefe | New Issue | |
| 2025-01-16 15:42 | fefe | Status | new => assigned |
| 2025-01-16 15:42 | fefe | Assigned To | => Christian Grothoff |
| 2025-01-16 16:03 | Christian Grothoff | Note Added: 0023990 | |
| 2025-01-16 16:03 | Christian Grothoff | Assigned To | Christian Grothoff => fefe |
| 2025-01-16 16:03 | Christian Grothoff | Status | assigned => feedback |
| 2025-03-22 13:09 | Christian Grothoff | Category | mechant backend => merchant backend |
| 2025-07-11 20:42 | Christian Grothoff | Assigned To | fefe => Christian Grothoff |
| 2025-07-11 20:42 | Christian Grothoff | Status | feedback => assigned |
| 2025-07-12 15:38 | Christian Grothoff | Status | assigned => informing |
| 2025-07-12 15:38 | Christian Grothoff | Resolution | open => no change required |
| 2025-07-12 15:38 | Christian Grothoff | Fixed in Version | => 1.0 stretch goals |
| 2025-07-12 15:38 | Christian Grothoff | Note Added: 0025495 | |
| 2025-07-12 15:38 | Christian Grothoff | Status | informing => closed |
| 2025-07-12 15:38 | Christian Grothoff | Product Version | => git (master) |
| 2025-07-12 15:38 | Christian Grothoff | Target Version | => 1.0 stretch goals |