Bug 24538 - REMOTE_USER set to undef if koha_trusted_proxies contains invalid value
Summary: REMOTE_USER set to undef if koha_trusted_proxies contains invalid value
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on: 23068
Blocks:
  Show dependency treegraph
 
Reported: 2020-01-30 04:12 UTC by David Cook
Modified: 2020-03-13 01:12 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.04


Attachments
Bug 24538: Add unit tests for Koha::Middleware::RealIP (5.31 KB, patch)
2020-02-19 02:16 UTC, David Cook
Details | Diff | Splinter Review
Bug 24538: Handle Net::Netmask parser errors (1.42 KB, patch)
2020-02-19 02:16 UTC, David Cook
Details | Diff | Splinter Review
Bug 24538: Add unit tests for Koha::Middleware::RealIP (5.37 KB, patch)
2020-02-19 12:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24538: Add unit tests for Koha::Middleware::RealIP (5.37 KB, patch)
2020-02-19 12:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24538: Handle Net::Netmask parser errors (1.50 KB, patch)
2020-02-19 12:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24538: Add unit tests for Koha::Middleware::RealIP (5.42 KB, patch)
2020-02-19 21:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24538: Handle Net::Netmask parser errors (1.54 KB, patch)
2020-02-19 21:45 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2020-01-30 04:12:28 UTC
If koha_trusted_proxies contains an invalid or unparseable value, say due to a typo, like "1.1.1.", it'll treat it as a 0.0.0.0/0 mask and match against everything. 

In my tests, this leads to REMOTE_USER being set to undef.

(On the plus side, at least a 0.0.0.0/0 mask leads to zero privileges rather than all privileges.)
Comment 1 David Cook 2020-01-30 04:14:18 UTC
The solution should probably use either the new2 constructor (code smell...) or check the $objet->{'ERROR'} value as per https://metacpan.org/pod/Net::Netmask#CONSTRUCTING. 

(Note that I actually found this while testing a patch for https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24537)
Comment 2 David Cook 2020-02-19 02:16:18 UTC
Created attachment 99232 [details] [review]
Bug 24538: Add unit tests for Koha::Middleware::RealIP

This patch adds some unit tests for Koha::Middleware::RealIP
Comment 3 David Cook 2020-02-19 02:16:21 UTC
Created attachment 99233 [details] [review]
Bug 24538: Handle Net::Netmask parser errors

This patch switches from the new() to new2() constructor,
which will return an undef value when it fails to parse a value.

This patch warns on parser failures, but otherwise silently drops
the invalid value, and returns objects for any valid input it can parse.
This way one mistake won't disable the whole feature.

To test:
1. Run the unit test t/Koha/Middlware/RealIP.t
Comment 4 David Cook 2020-02-19 02:17:45 UTC
I'd suggest applying the "Add unit tests..." patch first, running it, seeing the failures, and then applying the 2nd patch, and then running the unit test again (and seeing the successes).
Comment 5 Kyle M Hall 2020-02-19 12:08:53 UTC
Created attachment 99243 [details] [review]
Bug 24538: Add unit tests for Koha::Middleware::RealIP

This patch adds some unit tests for Koha::Middleware::RealIP

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 6 Kyle M Hall 2020-02-19 12:12:44 UTC
Created attachment 99244 [details] [review]
Bug 24538: Add unit tests for Koha::Middleware::RealIP

This patch adds some unit tests for Koha::Middleware::RealIP

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Kyle M Hall 2020-02-19 12:13:09 UTC
Created attachment 99245 [details] [review]
Bug 24538: Handle Net::Netmask parser errors

This patch switches from the new() to new2() constructor,
which will return an undef value when it fails to parse a value.

This patch warns on parser failures, but otherwise silently drops
the invalid value, and returns objects for any valid input it can parse.
This way one mistake won't disable the whole feature.

To test:
1. Run the unit test t/Koha/Middlware/RealIP.t

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Martin Renvoize 2020-02-19 21:45:48 UTC
Created attachment 99283 [details] [review]
Bug 24538: Add unit tests for Koha::Middleware::RealIP

This patch adds some unit tests for Koha::Middleware::RealIP

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2020-02-19 21:45:51 UTC
Created attachment 99284 [details] [review]
Bug 24538: Handle Net::Netmask parser errors

This patch switches from the new() to new2() constructor,
which will return an undef value when it fails to parse a value.

This patch warns on parser failures, but otherwise silently drops
the invalid value, and returns objects for any valid input it can parse.
This way one mistake won't disable the whole feature.

To test:
1. Run the unit test t/Koha/Middlware/RealIP.t

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2020-02-19 21:46:27 UTC
Cheers guys, Passing QA
Comment 11 David Cook 2020-02-20 04:02:52 UTC
Thanks for the quick turnaround on this, guys. Really appreciated.

I love Koha::Middleware::RealIP, so glad I was able to contribute :D.
Comment 12 Martin Renvoize 2020-02-20 08:46:19 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 13 Joy Nelson 2020-03-13 01:12:10 UTC
Pushed to 19.11.x branch for 19.11.04