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

Open
rekado wants to merge 2 commits from rekado/fsfe-website:catalogue into master
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
continuous-integration/drone/pr Build is passing Details
975dd6a835
weborder: Read prices from catalogue, not from the HTTP request.
* 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.
Author
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.

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
Member

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 refused to review 2022-08-28 15:45:32 +00:00
Author
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?
Member

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?
Member

@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.

@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.
Member

@reinhard is there anything I could do to help with this PR?

@reinhard is there anything I could do to help with this PR?
All checks were successful
continuous-integration/drone/pr Build is passing
Required
Details
This pull request has changes conflicting with the target branch.
  • cgi-bin/weborder.pl
Sign in to join this conversation.
No description provided.