ballerina-platform / ballerina-library

The Ballerina Library
https://ballerina.io/learn/api-docs/ballerina/
Apache License 2.0
137 stars 58 forks source link

Cookie is not set if path is empty #6788

Closed Shadow-Devil closed 1 month ago

Shadow-Devil commented 2 months ago

Description: I have the usecase where I'm using cookies together with a GraphQL service. The graphql:Client only has a "execute" method which means, that the whole path has to be provided as the base path to the client.

I enabled Tracing in my Config.toml

[ballerina.http.traceLogAdvancedConfig]
console = true

and saw that the Set-Cookie header in the response was set:

set-cookie: jwt=...; Path=/

but in the next request that I made to the same endpoint this cookie was not set.

After that I swapped the graphql:Client with a regular http:Client and used client->/path() for the request, which worked as expected. The cookie was set in the second request.

Steps to reproduce: Cookie is not set:

import ballerina/graphql;

graphql:Client client = new("http://localhost/path", cookieConfig = {enabled = true});

function main () returns error? {
    json _ = check client->execute("");
    // Cookie should be set now
    json _ = check client->execute(""); 
}

Cookie set:

import ballerina/http;

http:Client client = new("http://localhost", cookieConfig = {enabled = true});

function main () returns error? {
    json _ = check client->/path("");
    // Cookie is set now
    json _ = client->/path(""); 
}

Affected Versions:

OS, DB, other environment details and versions:

Related Issues (optional):

Suggested Labels (optional):

Suggested Assignees (optional):

TharmiganK commented 2 months ago

@Shadow-Devil Can I know the ballerina, http and graphql versions(dependency versions can be found in the Dependencies.toml) which causes this issue?

Shadow-Devil commented 2 months ago

@TharmiganK sure, here are the versions:

distribution-version = "2201.9.2"

[[package]]
org = "ballerina"
name = "http"
version = "2.11.3"

[[package]]
org = "ballerina"
name = "graphql"
version = "1.13.1"
ThisaruGuruge commented 2 months ago

I have tried this locally and was able to reproduce the issue. When the http:Client calls the post method with the path parameter as an empty string, the cookies were not set from the client side.

The following code can be used to reproduce the issue:

import ballerina/http;
import ballerina/io;

final http:Client httpClient = check new("http://localhost:4000/graphql", cookieConfig = {enabled: true}, httpVersion = "1.1");

public function main() returns error? {
    http:Request request = new;
    request.setPayload( {query: string `query { greeting(name: "JKNS") }`});
    request.addHeader("Content-Type", "application/json");
    request.addHeader("Accept", "application/json");

    json response = check httpClient->post("", request);
    io:println(response);

    json response2 = check httpClient->post("", request);
    io:println(response2);
}

Inside the Ballerina GraphQL client, we are using the http:Client and in the post method, we have an empty string since the GraphQL service is always calling the same endpoint.

Shadow-Devil commented 2 months ago

I think the relevant code that checks this path is here

TharmiganK commented 2 months ago

I think the relevant code that checks this path is here

Yes, there is a bug in computing the request path. Will fix this. Thanks for pointing this out

TharmiganK commented 2 months ago

@Shadow-Devil Is this is a blocker for you? If so we could give a patch on top of update 9(2201.9.x), or the expected fix can be added in the next update release - 2201.10.0 which is expected to be released on next month

Shadow-Devil commented 2 months ago

This is not a blocker for me. I'm just using the workaround with the raw http:Client and specifying the path in the remote call. Still thank you for looking into this :)

TharmiganK commented 2 months ago

No problem. Since this is not a blocker, this will be fixed in the next update release. Will give an update here once it is released

TharmiganK commented 2 months ago

@ThisaruGuruge tested this with the following graphql service and client:

Server:

const express = require('express');
const { graphqlHTTP } = require('express-graphql');
const { buildSchema } = require('graphql');
const cookieParser = require('cookie-parser');

const schema = buildSchema(`
  type Query {
    greet(name: String): String
  }
`);

const root = {
    greet: (args, context) => {
        const { name } = args;
        const userCookie = context.cookies.user;
        if (userCookie) {
            return `Hello, ${userCookie}!`;
        } else {
            context.res.cookie('user', name);
            return `Hello!`;
        }
    },
};

const app = express();

app.use(cookieParser());

app.use('/graphql', (req, res, next) => {
    graphqlHTTP({
      schema: schema,
      rootValue: root,
      graphiql: true,
      context: { cookies: req.cookies, res: res },
    })(req, res, next);
});

app.listen(4000, () => {
    console.log('GraphQL server running at http://localhost:4000/graphql');
});

Client:

import ballerina/graphql;
import ballerina/io;

type GreetingResponse record {|
    record {|string greet;|} data;
|};

public function main() returns error? {
    graphql:Client graphqlClient = check new ("http://localhost:4000/graphql", cookieConfig = {enabled: true});

    string greetQuery = "{ greet(name: \"Alice\") }";

    GreetingResponse greetResponse = check graphqlClient->execute(greetQuery);
    io:println(greetResponse.data.greet);

    greetResponse = check graphqlClient->execute(greetQuery);
    io:println(greetResponse.data.greet);
}

Output before the fix:

Hello!
Hello!

Output after the fix:

Hello!
Hello, Alice!
ThisaruGuruge commented 2 months ago

@ThisaruGuruge tested this with the following graphql service and client:

Great. Shall we do a patch release as well for this? If not, at least release this with the next U9 patch.

TharmiganK commented 2 months ago

Great. Shall we do a patch release as well for this? If not, at least release this with the next U9 patch.

Yes, the fix has been added for both master and 2201.9.x branches. Since this is not a blocker for him, this fix will be available in the update 10 and the next update 9 patch.

github-actions[bot] commented 1 month ago

This issue is NOT closed with a proper Reason/ label. Make sure to add proper reason label before closing. Please add or leave a comment with the proper reason label now.

      - Reason/EngineeringMistake - The issue occurred due to a mistake made in the past.
      - Reason/Regression - The issue has introduced a regression.
      - Reason/MultipleComponentInteraction - Issue occured due to interactions in multiple components.
      - Reason/Complex - Issue occurred due to complex scenario.
      - Reason/Invalid - Issue is invalid.
      - Reason/Other - None of the above cases.