Skip to content

Conversation

@r-polunin
Copy link

@r-polunin r-polunin commented Jan 28, 2024

Related to:
#968

If we have a chain of requests, i.e: request -> 302 re-direct -> 302 re-direct -> page.
Cookies from the first re-direct response are not propagated further.
This happens because of the way how axios works:
axios/axios#3862

Solution:
During the request preparation, if "Save cookies" checkbox is set, maxRedirections for axios instance will be set to 0 and further re-direction requests will be triggered re-cursively in error handler, new interceptors will be storing and enriching cookies.

Cookies UI:
New element allowing manual addition of the cookies is implemented. Table key is changed from domain to domain + path.

Ruslan Polunin added 2 commits January 28, 2024 11:39
Support for caches propagation in case of 302 re-directs on target page.
@r-polunin r-polunin mentioned this pull request Jan 28, 2024
6 tasks
@martinsefcik
Copy link
Contributor

  1. I didn't check this PR in detail but I would expect to be implemented also for bruno-cli and not only for bruno-app.
  2. It is better for two things (cookie addition and redirection modification) to do two PRs and not to merge it into one PR.
    It is first note mentioned in checklist when creating PR: The pull request only addresses one issue or adds one feature.
  3. Didn't you check this https://www.npmjs.com/package/http-cookie-agent mentioned in axios issue thread you posted Cookies are not automatically carried in redirection axios/axios#3862 (comment) ? If I understood it corectly this agent is handling cookies and we would not need to do redirections manually in such case (?) If it is like that then I like it more.

@r-polunin
Copy link
Author

r-polunin commented Jan 29, 2024

Hi @martinsefcik, thanks for review,

  1. I didn't check this PR in detail but I would expect to be implemented also for bruno-cli and not only for bruno-app.

Hmm, let me check this on the weekend. Seems as there is no cookie support for cli version at all, possibly can go as a separate PR.

  1. It is better for two things (cookie addition and redirection modification) to do two PRs and not to merge it into one PR.
    It is first note mentioned in checklist when creating PR: The pull request only addresses one issue or adds one feature.

Ok, will split

  1. Didn't you check this https://www.npmjs.com/package/http-cookie-agent mentioned in axios issue thread you posted Cookies are not automatically carried in redirection axios/axios#3862 (comment) ? If I understood it correctly this agent is handling cookies and we would not need to do redirections manually in such case (?) If it is like that then I like it more.

Yep, I've checked and played with this one a little.
I've used "export const createCookieAgent(http.Agent);
It was working for my case, but as I see, default httpAgent is overriden in Bruno:
https://github.com/r-polunin/bruno/blob/00c3f42589693c86af618d4b445ee394696e74fc/packages/bruno-electron/src/ipc/network/index.js#L172
I'm not sure how will it behave, if we wrap current custom Agent in Cookie Agent, I will check it on the weekends .

In my implementation, one of issues I didn't consider -> recursive redirects. They won't stop, as there is no max redirects count property anymore, just recursive calls. This one will require a fix If decision is made to keep custom implementation and not the lib.

Will check both approaches on the weekends.

@r-polunin
Copy link
Author

@martinsefcik, good day!
Had some time today to checked the cookie agent library, and faced couple of challenges, with such wrapper:

const wrapInCookieAgent = (AgentClass, ...args) => {
  const CookieAgentClass = createCookieAgent(AgentClass);
  const jar = getCookieJar();
  return new CookieAgentClass({ cookies: { jar }} , ...args);
}
  1. In library's overriding class, Params are sent in super constructor as ...params.
constructor(...params: ConstructorParams) {
      const { cookies: cookieOptions } = (params.find((opt) => {
        return opt != null && typeof opt === 'object' && 'cookies' in opt;
      }) ?? {}) as CookieAgentOptions;

      super(...(params as Params));

https://github.com/3846masa/http-cookie-agent/blob/a2f3b4e0bc9a33e20bc5cbcc395488abbee611f6/src/http/create_cookie_agent.ts#L53

It messes with SocksProxyAgent constructor which is used:
image
As it treats "CookieAgentOptions" required for CookieAgent as a first param.
Do you know any clean way to overcome such limitation?

  1. Bruno has 2 checkboxes: "Store" & "Send" cookies now.
    Both On/Off can be supported.
    "Store" - Off, "Send" - On can be supported too (create new jar instance, copy from the global state)
    But "Store" - On, "Send" - Off can't, as logic is hidden behind private method:
    https://github.com/3846masa/http-cookie-agent/blob/a2f3b4e0bc9a33e20bc5cbcc395488abbee611f6/src/http/create_cookie_agent.ts#L98

If there is no way to handle these challenges in a good way, I can:

  1. Write a similar custom agent to cover our case.
  2. Fork lib rep & try to discuss with author required extensions.
  3. Keep interceptors approach and handle retries.

What would you suggest to be the best way?

@naman-bruno
Copy link
Collaborator

Hey @r-polunin, thanks for taking the time to contribute!
This issue has already been resolved in the latest version.
Closing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants