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

Incorrect url double encoding for path params #10

Open
markbastian opened this issue Mar 30, 2020 · 1 comment
Open

Incorrect url double encoding for path params #10

markbastian opened this issue Mar 30, 2020 · 1 comment

Comments

@markbastian
Copy link

markbastian commented Mar 30, 2020

While attempting to sign a request to ElasticSearch in which a document id contained a "/" an incorrect signature was produced, resulting in the call to fail.

The failed case looked like so:
https://search-my-es-domain.us-east-1.es.amazonaws.com/index/_doc/foo%2Fbar

The "foo%2Fbar" id is the correct url encoding of the document with internal ElasticSearch id of "foo/bar".

Per This document from Amazon the correct behavior is that "Each path segment must be URI-encoded twice."

This means the canonical-url should encode the above id as "foo%252Fbar" with a canonical uri of:

(canonical-uri "/index/_doc/foo%2Fbar")
=> "/index/_doc/foo%252Fbar"

I was able to get my request to succeed with the following code changes to aws-sig4.auth/canonical-uri and I believe this is a more correct solution:

(defn double-encode [uri]
  (cs/join "/" (map codec/url-encode (cs/split uri #"/"))))

(defn canonical-uri [uri]
  (if (empty? uri)
    "/"
    (let [normalized (-> uri double-encode pathetic/normalize)]
      (cond-> normalized
              (and (pos? (count normalized)) (.endsWith uri "/"))
              (str "/")))))

;Added to my own code to hotfix
(alter-var-root #'aws-sig4.auth/canonical-uri (constantly canonical-uri))

I attempted to create a PR but certain test fail, including the following path "/%20/foo". However, I am unsure if the test is correct.

@ovan
Copy link
Contributor

ovan commented Apr 21, 2020

@markbastian Sorry for late response. We've retired this library from internal use so it doesn't get actively monitored. That said, I'd be happy to take a PR and have a look at the test failure. Can you open?

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

No branches or pull requests

2 participants