[GitHub] [couchdb] rnewson commented on pull request #4755: Add tests for X-Forwarded-For

["rnewson (via GitHub)" <gi...@apache.org>]

rnewson commented on PR #4755:
URL: https://github.com/apache/couchdb/pull/4755#issuecomment-1714283996

   I like the coverage but I don't think it tests _couchdb_. It asserts the behaviour of a mochiweb function, but not whether we call it and use the result to populate `peer`
   
   There are a few problems in general;
   
   1) we don't document that `peer` honors x-forwarded-for in the docs afaict, and obviously there's no test for it
   2) third party applications can't safely use the internal `httpd` record or assume what `peer` means in the absence of 1)
   
   I think this test needs to be on the elixir integration suite instead and assert that the client ip is logged, but also there should be a public function in couchdb that other apps can call that explicitly says how it calculates peer. 
   
   couchdb's current usage is fine, it asks mochiweb for the peer, using a public function, and logs it. Apps making assumptions about that, particularly about unpublished internals, are skating on thin ice. doing so without an integration test just invites disappointment.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org