Skip to content

Commit

Permalink
fix(plugins) urldecode path fragments which accept spaces
Browse files Browse the repository at this point in the history
In some plugins (basic-auth, hmac-auth, jwt, key-auth, oauth2)
we have path fragments which represent plugin fields that may
contain spaces in their definitions. This means that, if a
username for basic-auth contains a space, for example, then
it is reasonable to perform this request:

```
http ":8001/consumers/bob/basic-auth/John Doe/"
```

which, due to URL-encoding, results in a request to
http://localhost:8001/consumers/bob/basic-auth/John%20Doe/ --
we are currently returning 404 for those queries.

This patch performs URL-decoding on those path fragments,
reusing the urldecode function that was present in the
aws-lambda plugin.

It also modifies the test suites for these plugins using values
with spaces for the relevant fields, and using the URL-encoded
version of these fields when querying the Admin API.

Fixes #3247.
  • Loading branch information
hishamhm committed Feb 23, 2018
1 parent 637532e commit 2278d44
Show file tree
Hide file tree
Showing 12 changed files with 137 additions and 74 deletions.
11 changes: 5 additions & 6 deletions kong/plugins/aws-lambda/v4.lua
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@
local resty_sha256 = require "resty.sha256"
local pl_string = require "pl.stringx"
local openssl_hmac = require "openssl.hmac"
local utils = require "kong.tools.utils"


local urldecode = utils.urldecode


local ALGORITHM = "AWS4-HMAC-SHA256"

Expand Down Expand Up @@ -32,12 +37,6 @@ local function percent_encode(char)
return string.format("%%%02X", string.byte(char))
end

local function urldecode(str)
return (str:gsub("%%(%x%x)", function(c)
return string.char(tonumber(c, 16))
end))
end

local function canonicalise_path(path)
local segments = {}
for segment in path:gmatch("/([^/]*)") do
Expand Down
5 changes: 3 additions & 2 deletions kong/plugins/basic-auth/api.lua
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
local crud = require "kong.api.crud_helpers"
local utils = require "kong.tools.utils"

return {
["/consumers/:username_or_id/basic-auth/"] = {
Expand Down Expand Up @@ -27,7 +28,7 @@ return {
local credentials, err = crud.find_by_id_or_field(
dao_factory.basicauth_credentials,
{ consumer_id = self.params.consumer_id },
self.params.credential_username_or_id,
utils.urldecode(self.params.credential_username_or_id),
"username"
)

Expand Down Expand Up @@ -63,7 +64,7 @@ return {
local credentials, err = crud.find_by_id_or_field(
dao_factory.basicauth_credentials,
nil,
self.params.credential_username_or_id,
utils.urldecode(self.params.credential_username_or_id),
"username"
)

Expand Down
5 changes: 3 additions & 2 deletions kong/plugins/hmac-auth/api.lua
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
local crud = require "kong.api.crud_helpers"
local utils = require "kong.tools.utils"

return{
["/consumers/:username_or_id/hmac-auth/"] = {
Expand Down Expand Up @@ -28,7 +29,7 @@ return{
local credentials, err = crud.find_by_id_or_field(
dao_factory.hmacauth_credentials,
{ consumer_id = self.params.consumer_id },
self.params.hmac_username_or_id,
utils.urldecode(self.params.hmac_username_or_id),
"username"
)

Expand Down Expand Up @@ -64,7 +65,7 @@ return{
local credentials, err = crud.find_by_id_or_field(
dao_factory.hmacauth_credentials,
{},
self.params.hmac_username_or_id,
utils.urldecode(self.params.hmac_username_or_id),
"username"
)

Expand Down
5 changes: 3 additions & 2 deletions kong/plugins/jwt/api.lua
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
local crud = require "kong.api.crud_helpers"
local utils = require "kong.tools.utils"

return {
["/consumers/:username_or_id/jwt/"] = {
Expand Down Expand Up @@ -28,7 +29,7 @@ return {
local credentials, err = crud.find_by_id_or_field(
dao_factory.jwt_secrets,
{ consumer_id = self.params.consumer_id },
self.params.jwt_key_or_id,
utils.urldecode(self.params.jwt_key_or_id),
"key"
)

Expand Down Expand Up @@ -64,7 +65,7 @@ return {
local credentials, err = crud.find_by_id_or_field(
dao_factory.jwt_secrets,
nil,
self.params.jwt_key_or_id,
utils.urldecode(self.params.jwt_key_or_id),
"key"
)

Expand Down
5 changes: 3 additions & 2 deletions kong/plugins/key-auth/api.lua
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
local crud = require "kong.api.crud_helpers"
local utils = require "kong.tools.utils"

return {
["/consumers/:username_or_id/key-auth/"] = {
Expand Down Expand Up @@ -27,7 +28,7 @@ return {
local credentials, err = crud.find_by_id_or_field(
dao_factory.keyauth_credentials,
{ consumer_id = self.params.consumer_id },
self.params.credential_key_or_id,
utils.urldecode(self.params.credential_key_or_id),
"key"
)

Expand Down Expand Up @@ -63,7 +64,7 @@ return {
local credentials, err = crud.find_by_id_or_field(
dao_factory.keyauth_credentials,
{},
self.params.credential_key_or_id,
utils.urldecode(self.params.credential_key_or_id),
"key"
)

Expand Down
3 changes: 2 additions & 1 deletion kong/plugins/oauth2/api.lua
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
local crud = require "kong.api.crud_helpers"
local utils = require "kong.tools.utils"

return {
["/oauth2_tokens/"] = {
Expand Down Expand Up @@ -80,7 +81,7 @@ return {
local credentials, err = crud.find_by_id_or_field(
dao_factory.oauth2_credentials,
{ consumer_id = self.params.consumer_id },
self.params.clientid_or_id,
utils.urldecode(self.params.clientid_or_id),
"client_id"
)

Expand Down
11 changes: 11 additions & 0 deletions kong/tools/utils.lua
Original file line number Diff line number Diff line change
Expand Up @@ -734,4 +734,15 @@ _M.validate_header_name = function(name)
"', allowed characters are A-Z, a-z, 0-9, '_', and '-'"
end


--- url-decodes a string.
-- @param str (string) The input string, e.g. "Some%20Key".
-- @return the output string, "Some Key".
_M.urldecode = function(str)
return (str:gsub("%%(%x%x)", function(c)
return string.char(tonumber(c, 16))
end))
end


return _M
17 changes: 11 additions & 6 deletions spec/03-plugins/10-key-auth/01-api_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -149,11 +149,14 @@ describe("Plugin: key-auth (API)", function()

describe("/consumers/:consumer/key-auth/:id", function()
local credential
local url_key
before_each(function()
helpers.dao:truncate_table("keyauth_credentials")
credential = assert(helpers.dao.keyauth_credentials:insert {
consumer_id = consumer.id
consumer_id = consumer.id,
key = "Some Key",
})
url_key = "Some%20Key"
end)
describe("GET", function()
it("retrieves key-auth credential by id", function()
Expand All @@ -168,7 +171,7 @@ describe("Plugin: key-auth (API)", function()
it("retrieves key-auth credential by key", function()
local res = assert(admin_client:send {
method = "GET",
path = "/consumers/bob/key-auth/" .. credential.key
path = "/consumers/bob/key-auth/" .. url_key
})
local body = assert.res_status(200, res)
local json = cjson.decode(body)
Expand Down Expand Up @@ -212,7 +215,7 @@ describe("Plugin: key-auth (API)", function()
it("updates a credential by key", function()
local res = assert(admin_client:send {
method = "PATCH",
path = "/consumers/bob/key-auth/" .. credential.key,
path = "/consumers/bob/key-auth/" .. url_key,
body = {
key = "4321UPD"
},
Expand Down Expand Up @@ -416,13 +419,15 @@ describe("Plugin: key-auth (API)", function()

describe("/key-auths/:credential_key_or_id/consumer", function()
describe("GET", function()
local credential
local credential, url_key

setup(function()
helpers.dao:truncate_table("keyauth_credentials")
credential = assert(helpers.dao.keyauth_credentials:insert {
consumer_id = consumer.id
consumer_id = consumer.id,
key = "Some Key",
})
url_key = "Some%20Key"
end)

it("retrieve Consumer from a credential's id", function()
Expand All @@ -437,7 +442,7 @@ describe("Plugin: key-auth (API)", function()
it("retrieve a Consumer from a credential's key", function()
local res = assert(admin_client:send {
method = "GET",
path = "/key-auths/" .. credential.key .. "/consumer"
path = "/key-auths/" .. url_key .. "/consumer"
})
local body = assert.res_status(200, res)
local json = cjson.decode(body)
Expand Down
28 changes: 15 additions & 13 deletions spec/03-plugins/11-basic-auth/02-api_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ local utils = require "kong.tools.utils"

describe("Plugin: basic-auth (API)", function()
local consumer, admin_client
local plugin_username = "spongebob squarepants"
local url_username = "spongebob%20squarepants"
setup(function()
helpers.run_migrations()

Expand Down Expand Up @@ -31,7 +33,7 @@ describe("Plugin: basic-auth (API)", function()
method = "POST",
path = "/consumers/bob/basic-auth",
body = {
username = "bob",
username = plugin_username,
password = "kong"
},
headers = {
Expand All @@ -41,14 +43,14 @@ describe("Plugin: basic-auth (API)", function()
local body = assert.res_status(201, res)
local json = cjson.decode(body)
assert.equal(consumer.id, json.consumer_id)
assert.equal("bob", json.username)
assert.equal(plugin_username, json.username)
end)
it("encrypts the password", function()
local res = assert(admin_client:send {
method = "POST",
path = "/consumers/bob/basic-auth",
body = {
username = "bob",
username = plugin_username,
password = "kong"
},
headers = {
Expand Down Expand Up @@ -86,7 +88,7 @@ describe("Plugin: basic-auth (API)", function()
method = "POST",
path = "/consumers/bob/basic-auth",
body = {
username = "bob",
username = plugin_username,
password = "kong"
},
headers = {
Expand All @@ -100,7 +102,7 @@ describe("Plugin: basic-auth (API)", function()
method = "POST",
path = "/consumers/bob/basic-auth",
body = {
username = "bob",
username = plugin_username,
password = "kong"
},
headers = {
Expand All @@ -118,7 +120,7 @@ describe("Plugin: basic-auth (API)", function()
method = "PUT",
path = "/consumers/bob/basic-auth",
body = {
username = "bob",
username = plugin_username,
password = "kong"
},
headers = {
Expand All @@ -128,7 +130,7 @@ describe("Plugin: basic-auth (API)", function()
local body = assert.res_status(201, res)
local json = cjson.decode(body)
assert.equal(consumer.id, json.consumer_id)
assert.equal("bob", json.username)
assert.equal(plugin_username, json.username)
end)
describe("errors", function()
it("returns bad request", function()
Expand Down Expand Up @@ -179,7 +181,7 @@ describe("Plugin: basic-auth (API)", function()
before_each(function()
helpers.dao:truncate_table("basicauth_credentials")
credential = assert(helpers.dao.basicauth_credentials:insert {
username = "bob",
username = plugin_username,
password = "kong",
consumer_id = consumer.id
})
Expand All @@ -197,7 +199,7 @@ describe("Plugin: basic-auth (API)", function()
it("retrieves basic-auth credential by username", function()
local res = assert(admin_client:send {
method = "GET",
path = "/consumers/bob/basic-auth/" .. credential.username
path = "/consumers/bob/basic-auth/" .. url_username
})
local body = assert.res_status(200, res)
local json = cjson.decode(body)
Expand Down Expand Up @@ -245,7 +247,7 @@ describe("Plugin: basic-auth (API)", function()

local res = assert(admin_client:send {
method = "PATCH",
path = "/consumers/bob/basic-auth/" .. credential.username,
path = "/consumers/bob/basic-auth/" .. url_username,
body = {
password = "upd4321"
},
Expand Down Expand Up @@ -309,7 +311,7 @@ describe("Plugin: basic-auth (API)", function()
helpers.dao:truncate_table("basicauth_credentials")
assert(helpers.dao.basicauth_credentials:insert {
consumer_id = consumer.id,
username = "bob"
username = plugin_username
})
consumer2 = assert(helpers.dao.consumers:insert {
username = "bob-the-buidler"
Expand Down Expand Up @@ -403,7 +405,7 @@ describe("Plugin: basic-auth (API)", function()
helpers.dao:truncate_table("basicauth_credentials")
credential = assert(helpers.dao.basicauth_credentials:insert {
consumer_id = consumer.id,
username = "bob"
username = plugin_username,
})
end)
it("retrieve consumer from a basic-auth id", function()
Expand All @@ -418,7 +420,7 @@ describe("Plugin: basic-auth (API)", function()
it("retrieve consumer from a basic-auth username", function()
local res = assert(admin_client:send {
method = "GET",
path = "/basic-auths/" .. credential.username .. "/consumer"
path = "/basic-auths/" .. url_username .. "/consumer"
})
local body = assert.res_status(200, res)
local json = cjson.decode(body)
Expand Down
Loading

0 comments on commit 2278d44

Please sign in to comment.