Bug 24538

Summary: REMOTE_USER set to undef if koha_trusted_proxies contains invalid value
Product: Koha Reporter: David Cook <dcook>
Component: Architecture, internals, and plumbingAssignee: Bugs List <koha-bugs>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: minor    
Priority: P5 - low CC: 1joynelson, martin.renvoize
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25950
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.04
Bug Depends on: 23068    
Bug Blocks:    
Attachments: Bug 24538: Add unit tests for Koha::Middleware::RealIP
Bug 24538: Handle Net::Netmask parser errors
Bug 24538: Add unit tests for Koha::Middleware::RealIP
Bug 24538: Add unit tests for Koha::Middleware::RealIP
Bug 24538: Handle Net::Netmask parser errors
Bug 24538: Add unit tests for Koha::Middleware::RealIP
Bug 24538: Handle Net::Netmask parser errors

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