Bug 32713 - x-koha-embed appears to no longer properly validate
Summary: x-koha-embed appears to no longer properly validate
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-01-24 16:22 UTC by Martin Renvoize
Modified: 2023-12-28 20:44 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.04


Attachments
Bug 32713: Unit tests (4.20 KB, patch)
2023-03-02 10:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32713: Throw exception on unexpected x-koha-embed header (2.65 KB, patch)
2023-03-02 10:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32713: Unit tests (4.27 KB, patch)
2023-03-07 11:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32713: Throw exception on unexpected x-koha-embed header (2.72 KB, patch)
2023-03-07 11:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32713: Unit tests (4.33 KB, patch)
2023-03-16 19:03 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32713: Throw exception on unexpected x-koha-embed header (2.78 KB, patch)
2023-03-16 19:03 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32713: (QA follow-up) Tests now require more complex structures (1.41 KB, patch)
2023-03-17 21:28 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2023-01-24 16:22:19 UTC
We migrated x-koha-embed into properly specified header parameters in bug 30536 however it appears that the upstream validation of those headers has since broken and we're now allowing open embed calls through from the API.

At best this can lead to server errors leaking, at worst it exposed methods that should be private to the API.

We need to write some tests to catch this and possibly re-instate the in-koha validation whilst the OpenAPI is updated.

See 8e1265c45ddfc53d4bcbfc51e25b297e3592daf4 for some further details.
Comment 1 Martin Renvoize 2023-02-24 13:28:23 UTC
There are tests to catch this and they are passing still.

I can't remember how we were replicating this.. do you happen to remember Jonathan?
Comment 2 Jonathan Druart 2023-02-24 13:40:43 UTC
Did you try to request with an embed if there is no embed defined in the specs?
Comment 3 Tomás Cohen Arazi 2023-02-24 14:32:42 UTC
I noticed this when reviewing bug 32923.

We need to add an empty x-koha-embed header to all endpoints. Otherwise the embed is passed through.

curl --location --request GET 'http://kohadev.myDNSname.org:8081/api/v1/items' \
--header 'x-koha-embed: extended_attributes' \
--header 'Authorization: Basic dGNvaGVuOnRvbWFzaXRv'


That gives you a 500, because the extended_attributes relation is not valid. When it should've been rejected altogether.
Comment 4 Martin Renvoize 2023-02-27 10:44:44 UTC
Should we add the empty definition to the specs throughout, or should we catch when the header is missing in the spec and throw an error when it's passed in the request?
Comment 5 Jonathan Druart 2023-02-28 07:50:02 UTC
(In reply to Martin Renvoize from comment #4)
> Should we add the empty definition to the specs throughout, or should we
> catch when the header is missing in the spec and throw an error when it's
> passed in the request?

The latter I'd say.
Comment 6 Martin Renvoize 2023-02-28 08:20:22 UTC
There's a third option now I think about it.. we don't throw and error but instead silently ignore the header rather than let it propagate in our code.  In theory, I think that's the most http compliant?
Comment 7 Jonathan Druart 2023-02-28 09:25:09 UTC
(In reply to Martin Renvoize from comment #6)
> There's a third option now I think about it.. we don't throw and error but
> instead silently ignore the header rather than let it propagate in our code.
> In theory, I think that's the most http compliant?

Could make things harder to debug, don't you think?
Comment 8 Martin Renvoize 2023-03-02 10:13:08 UTC
Created attachment 147586 [details] [review]
Bug 32713: Unit tests

This patch adds unit tests to prove we return a 400 and an appropriate
error message when calling an endpoint that is not defined to support
x-koha-embed whilst passing an x-koha-embed header.

Test plan:
1) Run test prior to applying second patch and confirm it fails
2) Run test after applying second patch and confirm it passes
Comment 9 Martin Renvoize 2023-03-02 10:13:11 UTC
Created attachment 147587 [details] [review]
Bug 32713: Throw exception on unexpected x-koha-embed header

This patch adds an exception to stash_embed that is thrown when we
find an x-koha-embed header that we're not expecting.
Comment 10 Jonathan Druart 2023-03-07 11:09:32 UTC
Created attachment 147820 [details] [review]
Bug 32713: Unit tests

This patch adds unit tests to prove we return a 400 and an appropriate
error message when calling an endpoint that is not defined to support
x-koha-embed whilst passing an x-koha-embed header.

Test plan:
1) Run test prior to applying second patch and confirm it fails
2) Run test after applying second patch and confirm it passes

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Jonathan Druart 2023-03-07 11:09:35 UTC
Created attachment 147821 [details] [review]
Bug 32713: Throw exception on unexpected x-koha-embed header

This patch adds an exception to stash_embed that is thrown when we
find an x-koha-embed header that we're not expecting.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 12 Tomás Cohen Arazi 2023-03-16 19:03:40 UTC
Created attachment 148289 [details] [review]
Bug 32713: Unit tests

This patch adds unit tests to prove we return a 400 and an appropriate
error message when calling an endpoint that is not defined to support
x-koha-embed whilst passing an x-koha-embed header.

Test plan:
1) Run test prior to applying second patch and confirm it fails
2) Run test after applying second patch and confirm it passes

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Tomás Cohen Arazi 2023-03-16 19:03:44 UTC
Created attachment 148290 [details] [review]
Bug 32713: Throw exception on unexpected x-koha-embed header

This patch adds an exception to stash_embed that is thrown when we
find an x-koha-embed header that we're not expecting.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Tomás Cohen Arazi 2023-03-16 19:31:29 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 15 Jonathan Druart 2023-03-17 11:03:42 UTC
t/db_dependent/Koha/REST/Plugin/Objects.t is failing
Comment 16 Tomás Cohen Arazi 2023-03-17 21:28:09 UTC
Created attachment 148386 [details] [review]
Bug 32713: (QA follow-up) Tests now require more complex structures

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 17 Tomás Cohen Arazi 2023-03-20 12:18:33 UTC
Follow-up pushed to master.
Comment 18 Matt Blenkinsop 2023-03-21 16:33:08 UTC
Nice work everyone!

Pushed to stable for 22.11.x
Comment 19 Lucas Gass 2023-04-24 16:45:06 UTC
Doesn't apply clean to 22.05.x, no backport. If needed please rebase.