node-modules / urllib

Request HTTP(s) URLs in a complex world.
MIT License
726 stars 117 forks source link

FIX: Include query parameters in digest uri #396

Closed adapt0 closed 2 years ago

adapt0 commented 2 years ago

Discovered that digest authentication was failing with request 3.1.2 when the URL contains query parameters:

await urllib.request(
  'http://${address}/api/v1/public?sys.info.model',
  {
    method: 'POST',
    digestAuth: `${user}:${pass}`,
  }
);

This fails as the uri parameter in the authorization header doesn't match the requested uri:

GET /api/v1/public?sys.info.model HTTP/1.1
...
authorization: Digest username="test", realm="M8 Processor", nonce="eH2KYs4TQ5RG6kGR2VWAhLvSX6l0Vthg", uri="/api/v1/public", response="fbf377fb93eb5a5b4455a298eb68a439", opaque="0e8ccb02b0ae1507f8237e97540fedb1", qop=auth, nc=00000002, cnonce="6e4ca7a668f60e18"

HTTP/1.1 401 Unauthorized

(note the lack of query in uri="/api/v1/public")

Per section 2.1.2 in RFC2069:

digest-uri          = "uri" "=" digest-uri-value
digest-uri-value    = request-uri         ; As specified by HTTP/1.1

and section 3.2.1 in RFC2068:

URI            = ( absoluteURI | relativeURI ) [ "#" fragment ]

absoluteURI    = scheme ":" *( uchar | reserved )

relativeURI    = net_path | abs_path | rel_path

net_path       = "//" net_loc [ abs_path ]
abs_path       = "/" rel_path
rel_path       = [ path ] [ ";" params ] [ "?" query ]

query is considered part of the URI

Specifically Passport.js digest strategy checks that the uri matches. And also confirmed this same request with Postman which did include the query parameters and successfully authenticated.

As such here's a PR which appends the query parameters, along with an additional test case to make sure they made it through.

Thanks for your great lib!

lgtm-com[bot] commented 2 years ago

This pull request introduces 4 alerts when merging e317eebc03ca29c40d98243bf1b16c66ac5ce4ac into 64bbe48810bc2621e308879d27ce57d149acdfdf - view on LGTM.com

new alerts:

codecov[bot] commented 2 years ago

Codecov Report

Merging #396 (e317eeb) into master (64bbe48) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #396   +/-   ##
=======================================
  Coverage   98.67%   98.67%           
=======================================
  Files           4        4           
  Lines         378      378           
  Branches      124      124           
=======================================
  Hits          373      373           
  Misses          5        5           
Impacted Files Coverage Δ
src/HttpClient.ts 98.55% <ø> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

fengmk2 commented 2 years ago

3.1.3

fengmk2 commented 2 years ago

@adapt0 Thanks!