order: Look up prices on the server, not in the POST request. #2750

rekado · 2022-08-22T16:10:54Z

rekado commented

2022-08-22 16:10:54 +00:00

First-time contributor

The web shop form currently submits not only the amount of selected items to cgi-bin/weborder.pl in a POST request, but also submits the price. weborder.pl trusts these prices and uses them to compute the total.

These two commits change this by generating a catalogue.xml containing the prices of all items. weborder.pl then looks up prices in that server-side file instead of accepting arbitrary prices in the user-supplied POST request, thereby guaranteeing that the prices have not been tampered with.

The web shop form currently submits not only the amount of selected items to `cgi-bin/weborder.pl` in a POST request, but also submits the *price*. `weborder.pl` trusts these prices and uses them to compute the total. These two commits change this by generating a catalogue.xml containing the prices of all items. weborder.pl then looks up prices in that server-side file instead of accepting arbitrary prices in the user-supplied POST request, thereby guaranteeing that the prices have not been tampered with.

rekado added 2 commits 2022-08-22 16:10:55 +00:00

order: Generate catalogue of items and prices. 3e9d56071e

weborder: Read prices from catalogue, not from the HTTP request.

continuous-integration/drone/pr Build is passing

Details

975dd6a835

* cgi-bin/weborder.pl: Parse order/catalogue.xml to look up prices for
item id.
* order/order.xsl: Do not generate hidden form input for price.

rekado commented

2022-08-22 16:12:15 +00:00

First-time contributor

One more thing of note: catalogue.xml does not require any translations as it is not intended for presentation on the website. It could just as well be replaced with a database, but I wanted to keep the number of changes to a minimum.

max.mehl requested review from reinhard 2022-08-23 14:37:36 +00:00

reinhard commented

2022-08-28 15:45:22 +00:00

The change looks good to me.

We could consider merging all the item.en.xml into a single file (the contents are not translated so we don't gain anything from having separate files per year), which AFAIU save us the additional step of creating the catalogue.xml file and make things much more straightforward and understandable.

The change looks good to me. We could consider merging all the item.en.xml into a single file (the contents are not translated so we don't gain anything from having separate files per year), which AFAIU save us the additional step of creating the `catalogue.xml` file and make things much more straightforward and understandable.

reinhard declined to review 2022-08-28 15:45:32 +00:00

rekado commented

2022-08-28 21:06:11 +00:00

First-time contributor

I'm glad you suggest merging all the item files! I was taken aback by the complexity of processing the items when I considered implementing changes to the order form.
Having all items in the same XML file would simplify processing and unlock further improvements.

While we're at it, could we also merge all the info.*.xml files (one per language)? Or is there a reason why the year must be encoded in the directory name instead of, say, an XML attribute?

I'm glad you suggest merging all the item files! I was taken aback by the complexity of processing the items when I considered implementing changes to the order form. Having all items in the same XML file would simplify processing and unlock further improvements. While we're at it, could we also merge all the `info.*.xml` files (one per language)? Or is there a reason why the year must be encoded in the directory name instead of, say, an XML attribute?

reinhard commented

2022-08-28 21:53:15 +00:00

The info.*.xml files are a different topic, since they are translated: ideally there we would have a separate file per item, so if the text for a given item is not translated, the fallback to the English text works automatically, and missing translations can easily be found.

So we'd end up with something like:

order/data/items.en.xml
order/data/info/pin-gnu-silver.de.xml
order/data/info/pin-gnu-silver.en.xml
order/data/info/pin-gnu-silver.nl.xml
order/data/info/pin-plussy-green.de.xml
order/data/info/pin-plussy-green.en.xml
order/data/info/pin-plussy-green.nl.xml
order/pictures/pin-gnu-silver-large.jpg
order/pictures/pin-gnu-silver-small.jpg
order/pictures/pin-plussy-green-large.jpg
order/pictures/pin-plussy-green-small.jpg

This obviously requires quite a number of changes, but in the end the result was quite logical, understandable and maintainable.

What do you think?

The `info.*.xml` files are a different topic, since they are translated: ideally there we would have a separate file per item, so if the text for a given item is not translated, the fallback to the English text works automatically, and missing translations can easily be found. So we'd end up with something like: ``` order/data/items.en.xml order/data/info/pin-gnu-silver.de.xml order/data/info/pin-gnu-silver.en.xml order/data/info/pin-gnu-silver.nl.xml order/data/info/pin-plussy-green.de.xml order/data/info/pin-plussy-green.en.xml order/data/info/pin-plussy-green.nl.xml order/pictures/pin-gnu-silver-large.jpg order/pictures/pin-gnu-silver-small.jpg order/pictures/pin-plussy-green-large.jpg order/pictures/pin-plussy-green-small.jpg ``` This obviously requires quite a number of changes, but in the end the result was quite logical, understandable and maintainable. What do you think?

bonnie commented

2022-10-12 13:22:36 +00:00

@rekado Thank you very much for your work on this. I was wondering if theire are any blockers. If yes please let us know and we would do our best to help with them.