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

fix: Authorization headers not showing on kulala.inspect #320

Closed
wants to merge 1 commit into from

Conversation

estacioneto
Copy link

As the title says, it fixes the issue where the headers were not shown on kulala.inspect, which would make kulala.run not work inside the inspect buffer.

The problem was described at #122

@gorillamoe
Copy link
Member

I think that would cause a regression, any thoughts about this PR @Grueslayer? I see the point that inspect not showing the auth headers, but I don't think that this is the "right" approach.

@Grueslayer
Copy link
Contributor

The Authorization header is only removed from the header table when it should be handled by Curl itself (like for BASIC-User-Password / NTLM ....) but for other cases where there is no additional handling needed (like BEARER or BASIC where the Username and Passwort was already Base64-encoded) it keeps in the header list and still must be forwarded to Curl as header.

So the solution needs to be a little bit different, I'll have a look at it in the next days.

@estacioneto
Copy link
Author

Hi, folks!

The Authorization header is only removed from the header table when it should be handled by Curl itself but for other cases where there is no additional handling needed it keeps in the header list and still must be forwarded to Curl as header.

It was not being kept in the headers list and that's how I thought we could solve it, by adding it as an authorization header like it was, with -H.

I'm not sure I understood why a different solution is needed, but of course feel free to change it and fix it in another way as long as the inspect window shows the authentication headers, which is important for me to be able to call kulala.run inside the inspect buffer.

@Grueslayer
Copy link
Contributor

Grueslayer commented Jan 7, 2025

Because the "written" header is mostly not what is really send.

E.g. You have user/passwort credentials like

  • User = MyUsername
  • Password = Abc123

you have to send a header:

Authorization: BASIC TXlVc2VybmFtZTpBYmMxMjM=

But mostly you don't, you are writing:

Authorization: BASIC MyUsername:Abc123

and that is not send in this way. We remove that header and let curl create the right one (Base64 encoded for Basic).
There are also other like AWS. These are totally different from just sending a plain header line. AWS will create two requests!

There are cases where no special treatment are necessary (like shown before, when user already has encoded the Basic authentication in Base64).

Kulala removes the Auth header only in those cases when a special treatment is implemented and handled by curl itself.

So

  1. We can leave the current written Auth header in the lua table
  2. But we need to suppress the -H argument only in those cases where we previously have removed the header line

So instead to remove the header line from the table, we have to set a local boolean (say SkipAuthorizationHeader) at those progam lines and in the loop where Authorization is been added to the curl argument list skip those action only when boolean variable is true.

Inspect shows now in all cases the previosly written line but still can be different what curl is sending to the server!

@Grueslayer
Copy link
Contributor

This could be an approach @gorillamoe may accept:

diff --git a/lua/kulala/parser/init.lua b/lua/kulala/parser/init.lua
index 5ad3f78..6d37eef 100644
--- a/lua/kulala/parser/init.lua
+++ b/lua/kulala/parser/init.lua
@@ -756,6 +756,7 @@ M.parse = function(start_request_linenr)
   end
 
   local auth_header_name, auth_header_value = PARSER_UTILS.get_header(res.headers, "authorization")
+  local auth_header_skip = false
 
   if auth_header_name and auth_header_value then
     local authtype = auth_header_value:match("^(%w+)%s+.*")
@@ -772,7 +773,7 @@ M.parse = function(start_request_linenr)
           table.insert(res.cmd, "--" .. authtype)
           table.insert(res.cmd, "-u")
           table.insert(res.cmd, (authuser or "") .. ":" .. (authpw or ""))
-          res.headers[auth_header_name] = nil
+          auth_header_skip = true
         end
       elseif authtype == "aws" then
         local key, secret, optional = auth_header_value:match("^%w+%s([^%s]+)%s*([^%s]+)[%s$]+(.*)$")
@@ -794,7 +795,7 @@ M.parse = function(start_request_linenr)
           table.insert(res.cmd, "-H")
           table.insert(res.cmd, "x-amz-security-token:" .. token)
         end
-        res.headers[auth_header_name] = nil
+        auth_header_skip = true
       end
     end
   end
@@ -833,8 +834,10 @@ M.parse = function(start_request_linenr)
   end
 
   for key, value in pairs(res.headers) do
-    table.insert(res.cmd, "-H")
-    table.insert(res.cmd, key .. ":" .. value)
+    if not(key:lower() == "authorization" and auth_header_skip) then
+      table.insert(res.cmd, "-H")
+      table.insert(res.cmd, key .. ":" .. value)
+    end
   end
   if res.http_version ~= nil then
     table.insert(res.cmd, "--http" .. res.http_version)

@estacioneto
Copy link
Author

We remove that header and let curl create the right one...

I understand it and that's why I didn't touch the lines above res.headers[auth_header_name] = nil. This way the authorization is still passed to curl the right way.

But we need to suppress the -H argument only in those cases where we previously have removed the header line

Which is what if key ~= auth_header_name then should be doing.

But I understand I might still be missing a scenario and that's why I don't fully understand the difference, but I'll apply the changes you suggested :)

@gorillamoe
Copy link
Member

Hey there, if I understand this correctly, this could also go the way we do with a separate variable used for display, like we already do with body, right? I think this should be the way to go.

I think I can prepare something.

@gorillamoe
Copy link
Member

@estacioneto, can you check the current state, if this is what you have desired?

Now, it shows all headers (including the auth headers) on inpsect.

@estacioneto
Copy link
Author

@gorillamoe it works for me! I'll close the PR.

Thank you so much!

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.

3 participants