Handle empty URL parameter values

[pgvishnuram]

Description

Resolves an issue when a empty query param is passed or sent its being handled properly rather ending with 500 internal server error

Related Issues

https://github.com/astronomer/issues/issues/5683

Testing

Merging

cherry-pick to release-0.30, 0.32

[danielhoherd]

wouldn't paramName ~= '' only match a blank paramName, which is the opposite of what we want?

[pgvishnuram]

if paramName and paramValue then -- this should be enough we dont need to check paramName to be empty is that okay @danielhoherd

[danielhoherd]

it seems like that would work. the regex specifies that the paramName must be at least one character.

[danielhoherd]

We should tighten this regex up a bit. Here's at least one example:

Suggested change

	local paramName, paramValue = string.match(param, "(.+)=(.*)") -- Allow empty paramValue
	local paramName, paramValue = string.match(param, "([^=]+)=(.*)") -- Allow empty paramValue

I wonder if there is a function we can use to do this, because the regex to reliably capture the parameters could be complex.

[pgvishnuram]

This wasn't causing any issue so we didn't optimize those

[danielhoherd]

it looks like we can use ngx.req.get_uri_args() to get a table of param k=v pairs https://github.com/openresty/lua-nginx-module#ngxreqget_uri_args

[danielhoherd]

I think we should not use a regex to do this since there's the built-in function.

[pgvishnuram]

let me try to optimize the existing code and update the PR tomorrow

[danielhoherd]

@pgvishnuram i think this will do it:

    # redact sensitive data in the URI
    set_by_lua_block $request_uri_masked {
      local requestUri = ngx.var.requestUri
      if requestUri == nil or requestUri == '' then
        return requestUri
      end
      local paramsToMask = {
        token = true
      }
      local args = ngx.req.get_uri_args()

      -- redact all paramsToMask, pass through anything else
      for key, val in pairs(args) do
        if paramsToMask[key] then
          if val and val ~= '.' then -- match only values that have at least one character
            val = "**REDACTED**"
          end
          args[key] = val
        end
      end

      -- Find the position of the original query string
      local qsStart = string.find(requestUri, "?")

      -- Replace the original query string with a redacted version
      local redactedQs = ngx.encode_args(args)
      local masked_url = string.sub(requestUri, 1, qsStart) .. "?" .. redactedQs .. "#edited"

      return masked_url
    }

i can't add it as a suggestion though.

[danielhoherd]

I'm going to bang on this and try to figure it out. It's not quite working yet. The blank values are still causing a problem.

[danielhoherd]

I've been banging on this and I've run into a bunch of edge cases and can't solve them all. I've never done lua before, so that's part of my problem.

One case we should cover is &foo

([^?&]+)=([^&]*) solves the &root= case

[danielhoherd]

After digging banging on the code and digging through the API reference https://github.com/openresty/lua-nginx-module#nginx-api-for-lua I don't see any good way to do the referer other than regex because there is no function to parse the arguments from URL string. All of the URL parsing functions do not take a URL, they imply the URL that is part of the current request.

[pgvishnuram]

set_by_lua_block $request_uri_masked {
      local args = ngx.req.get_uri_args()
      local paramsToMask = {
        token = true
      }
      local maskedArgs = {}
      for paramName, paramValue in pairs(args) do
        -- Check if the parameter should be masked
        if paramsToMask[paramName] then
          paramValue = "##REDACTED##" -- Replace the value with REDACTED
        end

        if paramName and paramValue then -- Check if paramName and paramValue exist
          table.insert(maskedArgs, paramName .. "=" .. paramValue)
        end
      end
      local maskedQueryString = table.concat(maskedArgs, "&")
      local maskedUrl = ngx.var.uri
      if maskedQueryString ~= "" then
        maskedUrl = maskedUrl .. "?" .. maskedQueryString
      end
      
      return maskedUrl
   
    }

[danielhoherd]

Looks good. We've done a lot of back and forth and debugging on this and these changes are good for now. We have more things to tweak in the future if we choose to do so.