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

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 6 months ago
continuous-integration/drone/pr Build is passing Details
975dd6a835
weborder: Read prices from catalogue, not from the HTTP request.
Poster

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 5 months ago
Collaborator

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 5 months ago
Poster

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

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

@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.
All checks were successful
continuous-integration/drone/pr Build is passing
Required
Details
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
Sign in to join this conversation.
Loading…
There is no content yet.