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.
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?
Did you try to request with an embed if there is no embed defined in the specs?
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.
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?
(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.
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?
(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?
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
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.
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>
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>
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>
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>
Pushed to master for 23.05. Nice work everyone, thanks!
t/db_dependent/Koha/REST/Plugin/Objects.t is failing
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>
Follow-up pushed to master.
Nice work everyone! Pushed to stable for 22.11.x
Doesn't apply clean to 22.05.x, no backport. If needed please rebase.