search

line / armeria

Your go-to microservice framework for any situation, from the creator of Netty et al. You can build any type of microservice leveraging your favorite technologies, including gRPC, Thrift, Kotlin, Retrofit, Reactive Streams, Spring Boot and Dropwizard.
https://armeria.dev
Apache License 2.0
4.82k stars 917 forks source link

Cookie header doesn't work when debugging with DocService #3284

Open progresivoJS opened 3 years ago

progresivoJS commented 3 years ago

Problem

While I follows annotated-http-service, I found that setting Cookie header doesn't work when debugging with DocService.

How to test

I tested it with InjectionService.header() method as follows.

DocService

When debugging with the DocService, it doesn't work. image

Other HTTP client

It does work well when working with the HTTP client of IntelliJ or cURL. image

curl -X GET --location "http://localhost:8080/injection/header" \
    -H "x-armeria-text: hello" \
    -H "x-armeria-sequence: 1" \
    -H "Cookie: x=y" \
    -H "Content-Type: application/json; charset=utf-8"

I wonder if it's bug or known issue, or if I'm missing something.

ikhoon commented 3 years ago

I wonder if it's bug or known issue, or if I'm missing something.

That sounds like a bug.

minwoox commented 3 years ago

Thanks for the report, @progresivoJS! Are you going to handle this? or we can take care of this. 😄

progresivoJS commented 3 years ago

@minwoox Yes, I'd like to try to handle this issue, but it may takes some time to understand the mechanism of DocService. Can I try it if it's not a very urgent issue right now?

minwoox commented 3 years ago

I guess it's not urgent so you can try it anytime. 😄 Because you probably have to debug TypeScript code, please feel free to let us know if you are not comfortable with it. You might start how the headers are sent: https://github.com/line/armeria/blob/master/docs-client/src/containers/MethodPage/DebugPage.tsx#L409 https://github.com/line/armeria/blob/master/docs-client/src/lib/transports/annotated-http.ts#L85

progresivoJS commented 3 years ago

Thank you! 😄 I'll give it a try.

anuraaga commented 3 years ago

I suspect it's just not possible to override the cookie, since this is a browser. Properly reading / writing cookies is very important to browser security. The solution is probably to have a separate text box for cookie, transmit that in a separate header, and copy it back in the server. This may be tricky though since services are supposed to be agnostic to the use of doc service.

minwoox commented 3 years ago

I suspect it's just not possible to override the cookie, since this is a browser.

Ah, I couldn't think of it. 😄 Maybe it's cross-domain issues? That's a good idea to use a separate text box, but I wonder if we could just handle without it. 🤔

anuraaga commented 3 years ago

@minwoox I'm not aware of anyway to override the behavior of what comes in with set-cookie is returned in cookie, it's not even really a cross-origin issue it's very much same-origin in nature. For dev servers it gets even more messed up with many different dev servers all setting cookies on localhost :) One of the reasons why Postman requires a native agent binary to support all features.

minwoox commented 3 years ago

I just found and checked that we can override the cookie using document.cookie = 'a=c; path=/; domain=127.0.0.1'; Not sure if it's a good idea, though. 😉

anuraaga commented 3 years ago

Does it override all cookies sent, including httponly ones? I'm not too sure what the interaction with JS cookies and HTTP cookies is now-adays since most people don't manipulate cookie in JS anymore. But I'm guessing it'd be a combination of them, JS shouldn't be allowed to override http-only cookies no matter what it does, that's the security model. I'm guessing best we'd get natively is to have a superset of what the user provided in the form.

minwoox commented 3 years ago

JS shouldn't be allowed to override http-only cookies no matter what it does, that's the security model.

Yeah, I have checked that we cannot access the http-only cookies using document.cookie. Because of that, probable it's the best solution to handle cookies in a separate header as you mentioned, I guess. 🤔

trustin commented 3 years ago

+1 for sending Cookies as a dedicated separate header such as X-DocService-Cookies and rewriting internally somehow? We probably need to write a proxy inside DocService to do that properly.

anuraaga commented 3 years ago

We probably need to write a proxy inside DocService to do that properly.

Requests are sent to APIs, not DocService, which makes it a bit tricky I think. But for such a specialized and well-contained use case, I could imagine even just ArmeriaHttpUtil doing the translation to not introduce too many moving parts, another option that just seems like possibly too much complexity would be adding a decorator in front when detecting DocService is used.

trustin commented 3 years ago

Requests are sent to APIs, not DocService, which makes it a bit tricky I think.

I was rather thinking of updating the frontend to send all requests to DocService first and then let DocService dispatch it to the actual service, after translating cookies, etc.

anuraaga commented 3 years ago

Requests getting sent to the APIs was always quite nice to reason about so I don't know if that would be better. On the topic of cookies, for example, I understand wanting to use a text box but I've always relied on real cookies (it's why I added the JS injection a long time ago). If they're path-based cookies than I think a proxy couldn't work no matter how hard we tried, and reproducing decorators is doable but seems very complex. Hitting real API endpoints has always felt safest to me and I think one header, Cookie, could be handled specially instead of changing the whole model.

trustin commented 3 years ago

@anuraaga Agreed.