ipAddress request attribute not available when passing through API Client

[clarkwinkelmann]

Bug Report

Current Behavior
The user IP is not always available to extension middlewares that react to user login or registration. The IP is available when the user is logged in/created via the REST API but not when created via the web UI.

Steps to Reproduce
Use middleware

<?php

namespace MyExtension\Middlewares;

use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\ServerRequestInterface;
use Psr\Http\Server\MiddlewareInterface;
use Psr\Http\Server\RequestHandlerInterface;

class MyMiddleware implements MiddlewareInterface
{
    public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
    {
        if ($request->getUri()->getPath() === '/token') {
            var_dump($request->getAttribute('ipAddress'));
            die();
        }

        return $handler->handle($request);
    }
}

Register middleware on api:

return [
    (new Extend\Middleware('api'))
        ->add(Middlewares\MyMiddleware::class),
];

Perform request to POST /api/token or POST /api/users and notice IP is dumped.

Perform request to POST /login or POST /register and notice NULL is dumped.

Expected Behavior
In a way this could be intended behavior since the request is internal, but I don't think we ever intentionally made this and in any case there should be a way to find the original request IP from there.

The IP should be available, or at least the same default as 127.0.0.1 should be used instead of null.

Environment

• Flarum version: 1.0.4

Possible Solution
The ipAddress request attribute should probably be copied over in the ApiClient like the session and actor are:

https://github.com/flarum/core/blob/104a31ba306d6298a70c7f011d46a6484c36fad2/src/Api/Client.php#L133-L137

I don't think it makes sense to re-run the ProcessIp middleware since that middleware already runs at the most early stages of the application. However there's the risk that a middleware under forum and a different middleware under api conflict in how they want to modify the attribute.

https://github.com/flarum/core/blob/eb4b18a979c7406cbf154a107662652d282fe415/src/Foundation/InstalledApp.php#L58

Additional Context
This affects my audit log extension that reads the IP during all requests.

[tankerkiller125]

The reason for the ProcessIp middleware isn't just to set the attribute, the end goal is that it will also deal with Proxies and things of that nature to always return the client IP. We just haven't actually implemented that part. So I'm not sure how your fix would fit into that.

[luceos]

This makes a lot of sense.

[clarkwinkelmann]

I think the value should be just kept verbatim and passed down, like the session attribute is. No need to process it twice.

[askvortsov1]

Agreed. Iet's drop the middleware and pass the attribute explicitly.