Problem/Motivation

It seems like a bug that this module is a request event subscriber rather than a middleware. Was that intentional?

Some middlewares expect to have access to the client IP address and other aspects of the request that require the trusted reverse proxy logic already being in place.

Steps to reproduce

Use another module with middleware, for example ban module, which expects to be able to get the client IP address.

Presumably this could also affect middlewares which expect to determine whether or not the request is secure (HTTPS).

Proposed resolution

Convert the request event subscriber to middleware.

Remaining tasks

If this is in fact a bug, work on a merge request.

User interface changes

API changes

Data model changes

Issue fork reverse_proxy_header-3402311

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mfb created an issue. See original summary.

bohart’s picture

bohart
Location Lutsk, Ukraine
commented
Title: Should this module be a middleware rather than request event subscriber? » Replace event_subscriber service with http_middleware to support more use cases
Category: Bug report » Task
Status: Active » Needs work

Mark, thanks for raising the topic and providing the use case with `ban` module.
The proposed change sounds entirely reasonable.

Here is a great example where another module does the same (switched from event_subscriber to http_middleware to support more use cases):
https://git.drupalcode.org/project/cloudflare/-/commit/0c59e6f187a62e879...

It would be fantastic if someone from the community could work on this and provide a merge request.
Thanks!

Andrii Momotov made their first commit to this issue’s fork.

andrii momotov’s picture

andrii momotov commented
Assigned: Unassigned » andrii momotov

Andrii Momotov opened merge request !5

andrii momotov’s picture

andrii momotov commented
Status: Needs work » Needs review

Replaced event_subscriber service with http_middleware. Please check. Thank you!

andrii momotov’s picture

andrii momotov commented
Assigned: andrii momotov » Unassigned
bohart’s picture

bohart
Location Lutsk, Ukraine
commented
Status: Needs review » Needs work

@Andrii Momotov , thanks for your contribution!

Tests are failed, so mark this as "needs work".

taraskorpach made their first commit to this issue’s fork.

taraskorpach’s picture

taraskorpach
he/him
Location Lutsk 🇺🇦
commented
Status: Needs work » Needs review

The tests are fixed now, please review them. I've added a return type in accordance to the parent interface as well.

andrii momotov’s picture

andrii momotov commented
Status: Needs review » Needs work

It seems that the issue requires additional work because services added to https://git.drupalcode.org/ for this module report a series of problems:

- Merge request pipeline #78092 passed with warnings
- Code Quality scans found 7 new findings and 7 fixed findings.
- Test summary: 8 failed, 21 total tests

See more details here:
https://git.drupalcode.org/project/reverse_proxy_header/-/merge_requests/5

Therefore, I am changing the status to "Needs work".

elaman made their first commit to this issue’s fork.

elaman changed the visibility of the branch 1.0.x to hidden.

elaman changed the visibility of the branch 1.1.x to hidden.

elaman opened merge request !10

elaman’s picture

elaman
Primary language English
Location Philadelphia, United States
commented
Status: Needs work » Needs review

Did MR for 1.1.x with code quality and tests fixed.

elaman’s picture

elaman
Primary language English
Location Philadelphia, United States
commented
Version: 1.0.x-dev » 1.1.x-dev

mhh made their first commit to this issue’s fork.

mhh’s picture

mhh commented

I added additional empty checks if the reverse_proxy_header is not set.

elaman’s picture

elaman
Primary language English
Location Philadelphia, United States
commented

Rebased.