Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remote address is not set in TomcatServiceInvocationHandler #81

Closed
yfinkelstein opened this issue Jan 11, 2016 · 3 comments
Closed

Remote address is not set in TomcatServiceInvocationHandler #81

yfinkelstein opened this issue Jan 11, 2016 · 3 comments
Assignees
Labels
Milestone

Comments

@yfinkelstein
Copy link

It looks like at the moment remote address is not passed to coyote request. It's quite easy to do with some thing like this in TomcatServiceInvocationHandler.java (I modified the signature to pass the entire context):

    private static Request convertRequest(ServiceInvocationContext ctx) {

        final Request coyoteReq = new Request();
        InetSocketAddress remote = (InetSocketAddress)ctx.remoteAddress();
        //byte[] address = remote.getAddress().getAddress();

        coyoteReq.setRemotePort (remote.getPort());
        coyoteReq.remoteAddr().setString (remote.getHostName());

I had a servlet that was performing some authorization based on the remote address. With the change above is works fine now :)

@trustin trustin added this to the 0.8.0.Final milestone Jan 12, 2016
@trustin trustin self-assigned this Jan 12, 2016
@trustin trustin added the defect label Jan 12, 2016
trustin added a commit to trustin/armeria that referenced this issue Jan 12, 2016
Related: line#81

When TomcatServiceInvocationHandler converts a Netty HTTP request to a
Coyote request, it should set the remoteAddr() and remoteHost() property
of the Coyote request.
@trustin
Copy link
Member

trustin commented Jan 12, 2016

@yfinkelstein Sent a PR #82 for you.

@yfinkelstein
Copy link
Author

Yes, it works. Thanks!

@anuraaga
Copy link
Collaborator

Is it possible to add unit tests?

trustin added a commit to trustin/armeria that referenced this issue Jan 13, 2016
Related: line#81

Motivation:

When TomcatServiceInvocationHandler converts a Netty HTTP request to a
Coyote request, it should set the remoteAddr() and remoteHost() property
of the Coyote request.

Modifications:

- Fill remoteAddr/remoteHost/remotePort/localAddr/localName/localPort of
  Coyote request
- Add ServiceInvocationContext.localAddress() to get the local address
  and port
- Fix the content-type of the test JSP pages

Result:

Fixes line#81
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants